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 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.

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.
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
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.

@allanrenucci
Copy link
Contributor

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
Copy link
Contributor

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 _ =>
}
Copy link
Contributor

@liufengyun liufengyun Feb 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@odersky
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 scala:master Feb 24, 2018
@allanrenucci allanrenucci deleted the simplify-enums branch September 20, 2018 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants