Fix #9011: Make single enum values inherit from Product #9018

odersky · 2020-05-21T13:18:04Z

No description provided.

This was done just because compiling with a non-empty classpaths leads to conflicts when main is used as an annotation.

odersky · 2020-05-21T13:18:14Z

Based on #9017. It needs provablyDisjoint to be fixed first.

smarter · 2020-05-21T13:24:13Z

library/src/scala/EnumValue.scala

@@ -0,0 +1,10 @@
+package scala
+
+trait EnumValue extends Product:


For parity with case object this should extend Serializable too I think:

Suggested change

trait EnumValue extends Product:

trait EnumValue extends Product with Serializable:

Actually it probably makes more sense to have Product and Serializable added as parents to the enum class itself, that way they never show up when lubbing enum cases.

Yes, but I am not sure about what other effects this will have.

odersky · 2020-05-21T14:15:02Z

The failure comes from EnumValue now leaking into a result type of a union. We could fix it locally in the test cases. But I'd like to try first whether we can come up with a systematic fix.

We now strip EnumValue parents from inferred types, unless they are required by the bound. This is analogous to widen unions and singletons. It should be generalized to more types, not just EnumValue.

so that these traits do not leak into inferred types of unions of cases.

We now drop EnumValue just when inferring types of enum cases. This can be done unconditionally since in any case every enum case extends Enum, which extends Product and Serializable. EnumValue is just an invisible implementation bundle. This means we disable for now the more general handling in widenInferred.

It is no longer visible in the types of enum constants, so no need to keep it in Scala. Also, update doc pages to match the new scheme.

smarter · 2020-05-21T20:59:45Z

compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala

+    def dropProtected(tp: Type): Type = tp.dealias match
+      case tp @ AndType(tp1, tp2) =>
+        if isProtected(tp1) then tp2
+        else if isProtected(tp2) then tp1
+        else tp.derivedAndType(dropProtected(tp1), dropProtected(tp2))


This can drop aliases in situation where it isn't necessary, this can be avoided like this:

Suggested change

def dropProtected(tp: Type): Type = tp.dealias match

case tp @ AndType(tp1, tp2) =>

if isProtected(tp1) then tp2

else if isProtected(tp2) then tp1

else tp.derivedAndType(dropProtected(tp1), dropProtected(tp2))

def dropProtected(tp: Type): Type = tp.dealias match

case tpd @ AndType(tp1, tp2) =>

if isProtected(tp1) then tp2

else if isProtected(tp2) then tp1

else

val tpdw = tp.derivedAndType(dropProtected(tp1), dropProtected(tp2))

if tpdw ne tpd then tpdw else tp

smarter · 2020-05-22T13:15:12Z

compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala

@@ -300,6 +300,8 @@ trait ConstraintHandling[AbstractContext] {
   *      (i.e. `inst.widenSingletons <:< bound` succeeds with satisfiable constraint)
   *   2. If `inst` is a union type, approximate the union type from above by an intersection
   *      of all common base types, provided the result is a subtype of `bound`.
+   *   3. (currently not enabled) If `inst` is an intersection with some protected base types, drop


I wouldn't use the word protected, unless the plan is to actually use the protected modifier with them. Maybe restricted ?

Yes. For while I thought one could use protected for this, but now I think, probably not.

smarter · 2020-05-22T13:16:53Z

compiler/src/dotty/tools/dotc/typer/Namer.scala

+        case tp @ AndType(tp1, tp2) =>
+          if isEnumValue(tp1) then tp2
+          else if isEnumValue(tp2) then tp1
+          else tp.derivedAndType(dropEnumValue(tp1), dropEnumValue(tp2))


Same suggestion as above about dealiasing:

Suggested change

case tp @ AndType(tp1, tp2) =>

if isEnumValue(tp1) then tp2

else if isEnumValue(tp2) then tp1

else tp.derivedAndType(dropEnumValue(tp1), dropEnumValue(tp2))

case tpd @ AndType(tp1, tp2) =>

if isEnumValue(tp1) then tp2

else if isEnumValue(tp2) then tp1

else

val tpdw = tpd.derivedAndType(dropEnumValue(tp1), dropEnumValue(tp2))

if tpdw ne tpd then tpdw else tp

smarter · 2020-05-22T16:31:20Z

compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala

+      else /*widenRestricted*/(widenOr(widenSingle(inst)))
+        // widenRestricted is currently not called since it's special cased in `dropEnumValue`
+        // in `Namer`. It's left in here in case we want to generalize the scheme to other
+        // "protected inheritance" classes.


I'm not sure we should leave dead code like this around without a clear idea of what we want to do with it. Is this something you're planning to experiment with more? I think it would make sense to use this mechanism for getting rid of Serializable and Product so that they don't show up when lubbing case classes.

Very nice, thanks! Can you leave a reference to this issue number in the comment?

smarter

Otherwise LGTM.

smarter · 2020-05-22T17:30:12Z

tests/run/i9011.scala

+  def check(elem: Eq[_])(x: Any, y: Any): Boolean =
+    elem.asInstanceOf[Eq[Any]].eqv(x, y)
+
+  def iterator[T](p: T) = p.asInstanceOf[Product].productIterator


Going back to the original issue (#9011), is this cast actually safe?

The example in the docs suggests that if we have a Mirror.ProductOf[T] then we can safely cast an instance of T to Product (in order to access productIterator etc.). But this is not the case.

If this isn't actually guaranteed, then the documentation needs to be updated to not rely on this cast in an example.

odersky · 2020-05-22T17:41:10Z

We can leave #9011 open with that remark.

…

On Fri, May 22, 2020 at 7:30 PM Guillaume Martres ***@***.***> wrote: ***@***.**** requested changes on this pull request. Otherwise LGTM. ------------------------------ In tests/run/i9011.scala <#9018 (comment)>: > +} + +object Eq { + given Eq[Int] { + def eqv(x: Int, y: Int) = x == y + } + + inline def summonAll[T <: Tuple]: List[Eq[_]] = inline erasedValue[T] match { + case _: Unit => Nil + case _: (t *: ts) => summonInline[Eq[t]] :: summonAll[ts] + } + + def check(elem: Eq[_])(x: Any, y: Any): Boolean = + elem.asInstanceOf[Eq[Any]].eqv(x, y) + + def iterator[T](p: T) = p.asInstanceOf[Product].productIterator Going back to the original issue, is this cast actually safe? The example in the docs suggests that if we have a Mirror.ProductOf[T] then we can safely cast an instance of T to Product (in order to access productIterator etc.). But this is not the case. If this isn't actually guaranteed, then the documentation needs to be updated to not rely on this cast in an example. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9018 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGCKVXAN3OSUBMVPD5NIOTRS2ZDXANCNFSM4NG2T5YA> .

-- Prof. Martin Odersky LAMP/IC, EPFL

odersky added 2 commits May 21, 2020 15:15

Fix scala#9011: Make single enum values inherit from Product

1e13a92

Rename main objects in tests to _main

ba466e9

This was done just because compiling with a non-empty classpaths leads to conflicts when main is used as an annotation.

smarter reviewed May 21, 2020

View reviewed changes

odersky added 2 commits May 21, 2020 15:38

Make all enum cases instances of EnumValue statically

67f0786

Make EnumValue inherit Serializable

323d228

odersky added 4 commits May 21, 2020 18:26

Strip EnumValue parent from inferred types

29ac3a6

We now strip EnumValue parents from inferred types, unless they are required by the bound. This is analogous to widen unions and singletons. It should be generalized to more types, not just EnumValue.

Update semanticDB checks

1003adb

Make Enum extend Product and Serializable

9a887fe

so that these traits do not leak into inferred types of unions of cases.

odersky force-pushed the fix-#9011b branch from 072269c to 33233e6 Compare May 22, 2020 11:18

Move EnumValue to scala.runtime

8832569

It is no longer visible in the types of enum constants, so no need to keep it in Scala. Also, update doc pages to match the new scheme.

odersky force-pushed the fix-#9011b branch from 33233e6 to 8832569 Compare May 22, 2020 12:13

Update semanticDB expects again

3a352e6

smarter requested changes May 22, 2020

View reviewed changes

smarter reviewed May 22, 2020

View reviewed changes

Address review comments

1cd85bc

odersky force-pushed the fix-#9011b branch from 723ea0a to 1cd85bc Compare May 22, 2020 16:32

Add link to issue

74cc1b1

smarter requested changes May 22, 2020

View reviewed changes

smarter merged commit 2bcd4f6 into scala:master May 22, 2020

smarter deleted the fix-#9011b branch May 22, 2020 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #9011: Make single enum values inherit from Product #9018

Fix #9011: Make single enum values inherit from Product #9018

odersky commented May 21, 2020

odersky commented May 21, 2020 •

edited

Loading

smarter May 21, 2020

smarter May 21, 2020

odersky May 21, 2020

odersky commented May 21, 2020

smarter May 21, 2020

smarter May 22, 2020

odersky May 22, 2020

smarter May 22, 2020

smarter May 22, 2020

odersky May 22, 2020

smarter May 22, 2020

smarter left a comment

smarter May 22, 2020 •

edited

Loading

odersky commented May 22, 2020 via email

		@@ -0,0 +1,10 @@
		package scala

		trait EnumValue extends Product:

	trait EnumValue extends Product:
	trait EnumValue extends Product with Serializable:

Fix #9011: Make single enum values inherit from Product #9018

Fix #9011: Make single enum values inherit from Product #9018

Conversation

odersky commented May 21, 2020

odersky commented May 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky commented May 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarter left a comment

Choose a reason for hiding this comment

smarter May 22, 2020 • edited Loading

Choose a reason for hiding this comment

odersky commented May 22, 2020 via email

odersky commented May 21, 2020 •

edited

Loading

smarter May 22, 2020 •

edited

Loading