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

Change enum Desugarings #6154

Merged
merged 7 commits into from Mar 29, 2019
Merged

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Mar 24, 2019

Change the condition when type parameters of an enum are added to a class case.

Quoting my comment on #6095:

We have the following possibilities:

  1. type parameters are added only of there is no extends clause,
  2. (all) type parameters are added if one of them is referenced in the case,
  3. type parameters are always added if the case has value parameters (i.e. is not translated to an object).

(1) is what the current rules say. (2) is what Adriaan proposed. (3) is what is currently implemented.

In addition we have to clarify what should happen if a type parameter that is not added is nevertheless referred to in the case. Since enum expansion is done during desugaring, this could silently rebind to some other type. We should come up with a solution that avoids this, if possible.

This PR changes the rules to implement (2).

Change the condition when type parameters of an enum are added to a class case.

Quoting my comment on scala#6095:

We have the following possibilities:

 1. type parameters are added only of there is no extends clause,
 2. (all) type parameters are added if one of them is referenced in the case,
 3. type parameters are always added if the case has value parameters (i.e. is not translated to an object).

(1) is what the current rules say. (2) is what Adriaan proposed. (3) is what is currently implemented.

In addition we have to clarify what should happen if a type parameter that is not added is nevertheless referred to in the case. Since enum expansion is done during desugaring, this could silently rebind to some other type. We should come up with a solution that avoids this, if possible.

This PR changes the rules to implement (2).
@odersky odersky requested review from adriaanm and sjrd March 24, 2019 12:07
Copy link
Contributor

@adriaanm adriaanm left a comment

Choose a reason for hiding this comment

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

👌

@sjrd
Copy link
Member

sjrd commented Mar 28, 2019

As discussed off-line: IIUC, the purpose of this new rule is to avoid the surprising scoping effect that referring to T could otherwise "lexically jump over" the T in the enum declaration, either to not-declared or to a global reference. This is a worthwhile goal, but the new rule is not enough to reach that goal. If I do add a type parameter list with U but no T, and nevertheless refer to a T, the rule will not apply and T will still jump over the one in the enum declaration.

I think we should at least complement the rule by a similar one that reports an error if we do refer to such a T outside of the enum (and displays a non-confusing error message in case no such T exists).

That's still not completely enough, as T could be referred to in the body of the case, for example, or inside terms. That is also a concern when there is no explicit type parameter list, because the rule could entirely fail to notice that T as well.

I wonder whether that "not completely enough" is still "sufficiently complete" to avoid most confusing scenarios--and therefore worth having the new rule at all--or whether the non-completeness suggests that the new rule should not be added at all.

@odersky
Copy link
Contributor Author

odersky commented Mar 28, 2019

OK, I added checks for detecting illegal parameter references, both if a type parameter list is given and also for value cases where no parameter list is given at all.

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

LGTM, although I'd like a test case highlighting what will happen in the following cases:

class T

enum Foo[T](val foo: Any) {
  case Case1(x: Int) extends Foo(new T)
  case Case2[U](x: U) extends Foo(new T)
  case Case3 extends Foo(new T)
}

enum Bar[T] {
  case Case1(x: Int) {
    def foobar: T = new T
  }
  case Case2[U](x: U) {
    def foobar: T = new T
  }
  case Case3 {
    def foobar: T = new T
  }
}

@LPTK
Copy link
Contributor

LPTK commented Mar 28, 2019

Perhaps something similar should be done for value references to parameters and fields?

val test = 0
enum A(p: Int) {
  val test = 1
  case B extends A(test) // confusingly, refers to the outer 'test'
}

Also, does the new test also check for type members?

type Out = String
enum A[T] {
  type Out = Int
  case B extends A[Out] // confusingly, refers to the outer 'Out'
}

@odersky
Copy link
Contributor Author

odersky commented Mar 28, 2019

@sjrd I added a (pos) test for the Foo enum. The Bar enum is not syntactically legal: cases can't have bodies.

@odersky
Copy link
Contributor Author

odersky commented Mar 28, 2019

@LPTK Both of these generalizations would require a completely different - and much more complicated - compiler machinery than we have now. I don't think it's worth it, or at least not now. It could come later if someone wants to invest the effort and the result is not too complex. But I'm skeptical.

@LPTK
Copy link
Contributor

LPTK commented Mar 28, 2019

@odersky wouldn't the check amount to making sure the following two sets do not intersect?

  1. the set of free variable names in each case

  2. the set of variable names bound in the enum (ignoring the cases)

In the case where an enum case contains an extends clause whose type is not
fully specified, we cannot generate a function type parent for the companion
object.

In fact, this business of generating function types as parents of case class
companions is getting more and more fragile. We should maybe get out of it
altogether.
The previous condition when we can and cannot generate a parent is quite obscure,
so we should drop it altogether.
@odersky
Copy link
Contributor Author

odersky commented Mar 29, 2019

@LPTK Yes, but how do you compute these sets? Given imports, inheritance, etc...

@odersky odersky merged commit 5e31e59 into scala:master Mar 29, 2019
@allanrenucci allanrenucci deleted the change-enum-desugaring branch March 29, 2019 08:11
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

4 participants