scala / scala3

The Scala 3 compiler, also known as Dotty.
https://dotty.epfl.ch
Apache License 2.0
5.86k stars 1.06k forks source link

document enum cases cannot use private val as part of definition #11249

Closed ekrich closed 2 years ago

ekrich commented 3 years ago

Compiler version

Scala 3.0.0-M3

Minimized example

enum Planet(mass: Double, radius: Double) extends java.lang.Enum[Planet]:
   private final val G = 6.67300E-11
   private final val mm = 3.303e+23

   def surfaceGravity = G * mass / (radius * radius)
   def surfaceWeight(otherMass: Double) = otherMass * surfaceGravity

   case Mercury extends Planet(mm, 2.4397e6)
   case Venus   extends Planet(4.869e+24, 6.0518e6)
   case Earth   extends Planet(5.976e+24, 6.37814e6)
   case Mars    extends Planet(6.421e+23, 3.3972e6)
   case Jupiter extends Planet(1.9e+27,   7.1492e7)
   case Saturn  extends Planet(5.688e+26, 6.0268e7)
   case Uranus  extends Planet(8.686e+25, 2.5559e7)
   case Neptune extends Planet(1.024e+26, 2.4746e7)
end Planet

import Planet._

@main def trySomeThings: Unit =
  val earthWeight = 120.toDouble
  val mass = earthWeight / Earth.surfaceGravity
  for p <- values do
    println(s"Your weight on $p is ${p.surfaceWeight(mass)}")
end trySomeThings

Output

The output on Scastie was Not found: mm. The following is a real world output.

[error] -- [E006] Not Found Error: /Users/eric/workspace/sjavatime/sjavatime/shared/src/main/scala-3/java/time/temporal/ChronoUnit.scala:12:50 
[error] 12 |  case NANOS extends ChronoUnit(Duration.OneNano, isTimeBasedFlag)
[error]    |                                                  ^^^^^^^^^^^^^^^
[error]    |                                              Not found: isTimeBasedFlag

Expectation

It should be possible to work. This can make the code much more readable.

enum ChronoUnit(duration: Duration,
    flags: Int) extends jl.Enum[ChronoUnit] with TemporalUnit {
  // TODO: find out way this doesn't work
  private final val isTimeBasedFlag = 1
  // private final val isDateBasedFlag = 2

  case NANOS extends ChronoUnit(Duration.OneNano, isTimeBasedFlag)

  case MICROS extends ChronoUnit(Duration.OneMicro, 1)
bishabosha commented 3 years ago

Whats going on here after desugaring is the equivalent of

abstract class Planet(mass: Double) {
  private val mm = 3.303e+23
}
object Planet {
  val Mercury = new Planet(mm) {}
//  |                      ^^
//  |                      Not found: mm
}

To abstract out mm you can declare a companion object for Planet and put it there:

enum Planet(mass: Double) {                                                                                      
  case Mercury extends Planet(Planet.mm)
}
object Planet {
  private val mm = 3.303e+23
}
ekrich commented 3 years ago

Ok, that seems reasonable. Maybe an example in the docs would help. Is there a way to link this to a docs enhancement so this can be closed?

Can you import Planet._ inside to avoid typing the long version?

bishabosha commented 3 years ago

Can you import Planet._ inside to avoid typing the long version?

Unfortunately, in this case the import does not help. There is an explicit check to make sure that illegal references to companion members aren't made from an enum case definition, which errors with the import. Perhaps it can be reviewed for this example: https://github.com/lampepfl/dotty/blob/c49d6c06f213b3ba886cf96cf8962195062a2cac/compiler/src/dotty/tools/dotc/typer/Checking.scala#L1186

ekrich commented 3 years ago

I do not get a warning or an error if I import inside the enum definition. If I try and use it I get the following:

[info] compiling 33 Scala sources to /Users/eric/workspace/sjavatime/sjavatime/js/target/scala-3.0.0-M3/classes ...
[error] -- Error: /Users/eric/workspace/sjavatime/sjavatime/shared/src/main/scala-3/java/time/temporal/ChronoUnit.scala:9:50 
[error] 9 |  case NANOS extends ChronoUnit(Duration.OneNano, isTimeBasedFlag)
[error]   |                                                  ^^^^^^^^^^^^^^^
[error]   |                 illegal reference to value isTimeBasedFlag from enum case
[error] one error found

I get the same error if I put it in the outer scope. So we are stuck using a prefix from from the same enum object pair?

Is Scala 3 more picky about imports or is this the only case?

ekrich commented 3 years ago

Another thing to consider is that it can be used after the case definitions.

  case NOVEMBER extends Month(11, 30)

  case DECEMBER extends Month(12, 31)

  import Month._
 // used later in methods that follow
som-snytt commented 3 years ago

That is interesting (I see ekrich also noticed that it doesn't matter if the import is in the outer context):

scala> {
     | import E._
     | enum E(n: Int) extends Enum[E] { case X extends E(k) }
     | object E { private final val k = 42 }
     | }
3 |enum E(n: Int) extends Enum[E] { case X extends E(k) }
  |                                                  ^
  |                               illegal reference to value k from enum case

(After reading the docs and experimenting,) I would expect it to work like the context in which x is accessed (and it wasn't obvious to me that this works):

scala> {
     | import X._
     | class Y(n: Int)
     | class X extends Y(x)
     | object X { private def x = 42 }
     | }
// defined class Y
// defined class X
// defined object X

It's not obvious to me that it should work with the import inside the enum definition, because other normal lexical scoping doesn't work:

scala> enum E(n: Int) { case X extends E(n) }
1 |enum E(n: Int) { case X extends E(n) }
  |                                  ^
  |                                  Not found: n

where I would expect a different error message.

Another unusual message:

scala> enum E(val n: Int) { case X extends E(X.n) }
1 |enum E(val n: Int) { case X extends E(X.n) }
  |                                      ^
  |                                      Recursive value X needs type

Neither message is inscrutable.

I suspected there might be anomalies such as seen in Scala 2 with default constructor arg definitions, but found none.

ekrich commented 3 years ago

Overall, I think this is going to create a lot of confusion because people are used to that behavior and it seems to be a special case. We certainly want to avoid puzzlers.

odersky commented 3 years ago

Is there a way to link this to a docs enhancement so this can be closed?

Docs enhancements are done though regular PRs to this repo, so yes.

abgruszecki commented 3 years ago

It seems that we came to the conclusion that the behaviour is an intended (albeit confusing) feature. I'm closing the issue, if you feel that there's something to be done please say so / re-open it.

bishabosha commented 3 years ago

I think this was kept open because the documentation needs to be updated mention caveats of desugaring enum cases to be defined in the companion

vincenzobaz commented 3 years ago

Minimized code:

enum SuperEnum(private val privateSecret: Int):
  case MagicCase extends SuperEnum(privateSecret)
som-snytt commented 2 years ago

Example in linked PR.