Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify Enums #4003

Merged
merged 17 commits into from Feb 24, 2018
Merged

Simplify Enums #4003

merged 17 commits into from Feb 24, 2018

Conversation

@odersky
Copy link
Contributor

odersky commented Feb 15, 2018

I had a lingering unease that the rules for enums were too complex, and the syntax too cumbersome. I now think I have found a way to simplify things considerably. The key was to take away one capability: that cases can have bodies which can define members. Arguably, if we choose an ADT decomposition of a problem it's good style to write all methods using pattern matching instead of overriding individual cases. So this removes an unnecessary choice. What's more, once we have eliminated case bodies we also have eliminated scope confusion. All that remains are the case parameters and extends clause. Extends clauses of cases can be handled like super-calls in constructors: I.e. the enclosing enum class is not visible for them.

This means we can treat enums unequivocally as classes. They can have methods and other statements just like other classes can. Cases in enums are seen as a form of constructors. We do not need a distinction between enum class and enum object anymore. Enums can have companion objects just like normal classes can, of course.

This also means that type parameters of enums scope naturally over cases, just like they scope
over secondary constructors. We do not need to repeat them in cases anymore, which is a huge relief.

This first commit changes the syntax, docs and tests. It still needs to be implemented.
So tests should fail right now.

odersky added 7 commits Feb 15, 2018
I had a lingering unease that the rules for enums were too complex, and the syntax too cumbersome.
I now think I have found a way to simplify things considerably. The key was to take away one
capabilitiy: that cases can have bodies which can define members. Arguably, if we choose
an ADT decompositon of a problem it's good style to write all methods using pattern matching instead
of overriding individual cases. So this removes an unnecessary choice. What's more, once
we have eliminated case bodies we also have eliminated scope confusion. All that remains
are the case parameters and extends clause. Extends clauses of cases can be handled like supercalls
in constructors: I.e. the enclosing enum class is not visible for them.

This means we can treat enums unequivocally as classes. They can have methods and other statements
just like other classes can. Cases in enums are seen as a form of constructors. We do not
need a distinction between enum class and enum object anymore. Enums can have
companion objects just like normal classes can, of course.

This also means that type parameters of enums scope naturally over cases, just like they scope
over secondary constructors. We do not need to repeat them in cases anymore, which is a huge relief.

This first commit changes the syntax and docs. It still needs to be implemented.
We have the DerivedTypeTree abstraction to generate a TypeTree
during desugaring that watches a symbol that does not yet exist.
We now need to generalize this so that we can also create term references
(i.e. term idents) that watch a symbol. The necessity comes from enums
an enum like

    enum Color { case C1; ...; case Cn; ... }

needs to be expanded to

    sealed abstract class Color {
      import Color.{C1, ..., Cn}
      ...
    }
    object Color { case class C1; ...; case class Cn }

Otherwise the `...` in class `Color` could not refer to the cases. The problem is
that we cannot simply write an untyped ident `Color` in the import clause, because
a different `Color` might be defined or inherited in the `enum`, so we would get
a wrong binding. We need to refer to the `Color` companion object as a symbol, but
this one does not yet exist at the point where we expand. Hence the need for
a new mechanism.

An added complexity comes from the fact that these references go to the ValDef part
of a synthesized companion objects. But these objects might be merged with user-defined
ones later. We have to make sure that References attachments are correctly passed along
in such merges.
Implement as is described in the docs. Update docs and tests where necessary.
odersky added 6 commits Feb 17, 2018
A case class like

    case class C[T]

is OK, it cannot be mistaken for an object.
Previously, enum cases moved from the enum to its companion
object could "accidentally" refer to definitions defined
in the object, but inaccessible from the enum. We now
check that no such accesses occur.
Without that change, a reference to an enclosing type gets
always converted to a TypeTree. This undermines access checking
for constructors, which is implemented in the next commit.
We handle a supercall of a secondary constructor specially,
preventing accesses to members of the enclosing class. We need to
do the same for parameters of such constructors.

We now use two different mechanisms for that: superCallContexts for the
super call and checkRefsLegal for the parameters. We should experiment
at some point with doing the supercall checks also with chekRefsLegal.
@odersky

This comment has been minimized.

Copy link
Contributor Author

odersky commented Feb 18, 2018

I think this is done now. In summary, the syntax is much nicer and the changes in parsing and translation were straightforward. The tough part was to implement this rather vague idea that enum cases are treated like constructors for scope checking. The details of this turned out to be very tricky. This goes to show that some things are easy in the sense of "intuitive" but really hard to drill down.

The previous scheme was comparatively non-intuitive (since we needed explicit enum classes and objects) but much easier to check.

@odersky odersky requested a review from biboudis Feb 18, 2018
@allanrenucci

This comment has been minimized.

Copy link
Member

allanrenucci commented Feb 19, 2018

I tried the code below and was surprised it didn't compile. Is it indented?

object E4 {
  type INT = Integer
  val defaultX = 2
}

enum E4 {
  import E4._

  case C1(x: INT) // error: illegal reference
  case C2(x: Int = defaultX) // error: illegal reference
  case C3[T <: INT] // error: illegal reference

  def bar: INT = defaultX // But this is OK?
}
@liufengyun

This comment has been minimized.

Copy link
Contributor

liufengyun commented Feb 19, 2018

The check in checkEnumCompanions is a little too strict, maybe we should use a context that already have the stats indexed in the checking?

enum Color(val x: Int) {
  case Green  extends Color(3)
  case Red    extends Color(2)
  case Violet extends Color(Green.x + Red.x)        // error
}

object Test {
  abstract class Color(val x: Int)
  case object Green  extends Color(3)
  case object Red    extends Color(2)
  case object Violet extends Color(Green.x + Red.x) // ok
}
case mdef: untpd.TypeDef if mdef.mods.hasMod[untpd.Mod.Enum] =>
enumContexts(mdef1.symbol) = ctx
case _ =>
}

This comment has been minimized.

Copy link
@liufengyun

liufengyun Feb 19, 2018

Contributor

It seems ctx is not actually the context (which contains class members) used in checking member definitions, thus the following code fails the check:

enum Color(val x: Int) {
  case Green  extends Color(3)
  case Red    extends Color(2)
  case Violet extends Color(Green.x + Red.x)        // error
}

Is this intentional?

Edited: I was wrong, it's the right context, the problem is that when we checkEnumCaseRefsLegal for Color$, it checks Green.x + Red.x, of course it cannot be resolved!

This comment has been minimized.

Copy link
@odersky

odersky Feb 19, 2018

Author Contributor

Yes, I think we need a refinement here. This accessibility checking is quite a can of worms.

@odersky

This comment has been minimized.

Copy link
Contributor Author

odersky commented Feb 19, 2018

I tried the code below and was surprised it didn't compile. Is it indented?

Yes, that's as intended.

@odersky odersky merged commit 01f3094 into lampepfl:master Feb 24, 2018
2 checks passed
2 checks passed
CLA User signed CLA
Details
continuous-integration/drone/pr the build was successful
Details
@allanrenucci allanrenucci deleted the dotty-staging:simplify-enums branch Sep 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.