From 4470c9e726ac17ce1f2d85b396da0bb74b179fc7 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 24 Mar 2019 13:03:16 +0100 Subject: [PATCH 1/7] Change enum Desugarings 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). --- docs/docs/reference/enums/desugarEnums.md | 25 ++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/docs/docs/reference/enums/desugarEnums.md b/docs/docs/reference/enums/desugarEnums.md index f2ab5982f08e..fd59e469c2cb 100644 --- a/docs/docs/reference/enums/desugarEnums.md +++ b/docs/docs/reference/enums/desugarEnums.md @@ -25,9 +25,9 @@ some terminology and notational conventions: The desugaring rules imply that class cases are mapped to case classes, and singleton cases are mapped to `val` definitions. -There are eight desugaring rules. Rule (1) desugar enum definitions. Rules +There are nine desugaring rules. Rule (1) desugar enum definitions. Rules (2) and (3) desugar simple cases. Rules (4) to (6) define extends clauses for cases that -are missing them. Rules (7) and (8) define how such cases with extends clauses +are missing them. Rules (7) to (9) define how such cases with extends clauses map into case classes or vals. 1. An `enum` definition @@ -80,7 +80,7 @@ map into case classes or vals. case C extends E[B1, ..., Bn] where `Bi` is `Li` if `Vi = '+'` and `Ui` if `Vi = '-'`. This result is then further - rewritten with rule (7). Simple cases of enums with non-variant type + rewritten with rule (8). Simple cases of enums with non-variant type parameters are not permitted. 5. A class case without an extends clause @@ -91,7 +91,7 @@ map into case classes or vals. case C extends E - This result is then further rewritten with rule (8). + This result is then further rewritten with rule (9). 6. If `E` is an enum with type parameters `Ts`, a class case with neither type parameters nor an extends clause @@ -101,9 +101,20 @@ map into case classes or vals. case C[Ts] extends E[Ts] - This result is then further rewritten with rule (8). For class cases that have type parameters themselves, an extends clause needs to be given explicitly. + This result is then further rewritten with rule (9). For class cases that have type parameters themselves, an extends clause needs to be given explicitly. -7. A value case +7. If `E` is an enum with type parameters `Ts`, a class case without type parameters but with an extends clause + + case C extends + + expands to + + case C[Ts] extends + + provided at least one of the parameters `Ts` is mentioned in a parameter type in + `` or in a type argument in ``. + +8. A value case case C extends @@ -116,7 +127,7 @@ map into case classes or vals. as one of the `enumValues` of the enumeration (see below). `$values` is a compiler-defined private value in the companion object. -8. A class case +9. A class case case C extends From 2d8db010329cdd7598434b27f7954c4bf130596a Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 24 Mar 2019 17:02:56 +0100 Subject: [PATCH 2/7] Implement revised desugaring scheme --- .../src/dotty/tools/dotc/ast/Desugar.scala | 3 +- .../dotty/tools/dotc/ast/DesugarEnums.scala | 41 +++++++++++++++++++ tests/neg/enum-tparams.scala | 40 ++++++++++++++++++ 3 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 tests/neg/enum-tparams.scala diff --git a/compiler/src/dotty/tools/dotc/ast/Desugar.scala b/compiler/src/dotty/tools/dotc/ast/Desugar.scala index b9beb8b7979d..2fc1e8402fe0 100644 --- a/compiler/src/dotty/tools/dotc/ast/Desugar.scala +++ b/compiler/src/dotty/tools/dotc/ast/Desugar.scala @@ -355,7 +355,8 @@ object desugar { val originalVparamss = constr1.vparamss lazy val derivedEnumParams = enumClass.typeParams.map(derivedTypeParam) val impliedTparams = - if (isEnumCase && originalTparams.isEmpty) + if (isEnumCase && originalTparams.isEmpty && + typeParamIsReferenced(enumClass.typeParams, originalVparamss, parents)) derivedEnumParams.map(tdef => tdef.withFlags(tdef.mods.flags | PrivateLocal)) else originalTparams diff --git a/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala b/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala index 8f4e065fbf79..c6b9b6353cc2 100644 --- a/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala +++ b/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala @@ -168,6 +168,47 @@ object DesugarEnums { } } + /** Is a type parameter in `tparams` referenced from an enum class case that has + * given value parameters `vparamss` and given parents `parents`? + */ + def typeParamIsReferenced(tparams: List[TypeSymbol], vparamss: List[List[ValDef]], parents: List[Tree])(implicit ctx: Context): Boolean = { + + val searchRef = new untpd.TreeAccumulator[Boolean] { + var tparamNames = tparams.map(_.name).toSet[Name] + def underBinders(binders: List[MemberDef], op: => Boolean): Boolean = { + val saved = tparamNames + tparamNames = tparamNames -- binders.map(_.name) + try op + finally tparamNames = saved + } + def apply(x: Boolean, tree: Tree)(implicit ctx: Context) = x || { + tree match { + case Ident(name) => tparamNames.contains(name) + case LambdaTypeTree(lambdaParams, body) => + underBinders(lambdaParams, foldOver(x, tree)) + case RefinedTypeTree(parent, refinements) => + val refinementDefs = refinements collect { case r: MemberDef => r } + underBinders(refinementDefs, foldOver(x, tree)) + case _ => foldOver(x, tree) + } + } + } + + def typeHasRef(tpt: Tree) = searchRef(false, tpt) + def valDefHasRef(vd: ValDef) = typeHasRef(vd.tpt) + def parentHasRef(parent: Tree): Boolean = parent match { + case Apply(fn, _) => parentHasRef(fn) + case TypeApply(_, targs) => targs.exists(typeHasRef) + case Select(nu, nme.CONSTRUCTOR) => parentHasRef(nu) + case New(tpt) => typeHasRef(tpt) + case parent if parent.isType => typeHasRef(parent) + } + + parents.isEmpty || // a parent class that refers to type parameters will be generated in this case + vparamss.exists(_.exists(valDefHasRef)) || + parents.exists(parentHasRef) + } + /** A pair consisting of * - the next enum tag * - scaffolding containing the necessary definitions for singleton enum cases diff --git a/tests/neg/enum-tparams.scala b/tests/neg/enum-tparams.scala new file mode 100644 index 000000000000..7c951ca60b95 --- /dev/null +++ b/tests/neg/enum-tparams.scala @@ -0,0 +1,40 @@ +object Test { + + enum Opt[+T] { + case S(x: T) extends Opt[T] + case I(x: Int) extends Opt[Int] + case V() extends Opt[`T`] + case P(x: List[T]) extends Opt[String] + case N extends Opt[Nothing] + } + + type Id[_] + + enum E[F[_], G[_]] { + case C1() extends E[[X] => X, Id] + case C2() extends E[[F] => F, Id] + case C3() extends E[[X] => { type Y = F[Int] }, Id] + case C4() extends E[[X] => { type F = Int }, Id] + case C5() extends E[[F] => G[Int], Id] + } + + Opt.S[Int](1) // OK + Opt.S(1) // OK + Opt.I[Int](1) // error: does not take type parameters + Opt.I(1) // OK + Opt.V[Int]() // OK + Opt.V() // OK + Opt.P[Int](List(1, 2, 3)) // OK + Opt.P(List(1, 2, 3)) // OK + + E.C1[List, Id]() // error: does not take type parameters + E.C1() // OK + E.C2[List, Id]() // error: does not take type parameters + E.C2() // OK + E.C3[List, Id]() // OK + E.C3() // OK + E.C4[List, Id]() // error: does not take type parameters + E.C4() // OK + E.C5[List, Id]() // OK + E.C5() // OK +} \ No newline at end of file From 3f42377bb5aa207adc983b7c934b513b757c0d1e Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 24 Mar 2019 17:24:52 +0100 Subject: [PATCH 3/7] Fix search case --- compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala b/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala index c6b9b6353cc2..c88d92588b6e 100644 --- a/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala +++ b/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala @@ -201,7 +201,7 @@ object DesugarEnums { case TypeApply(_, targs) => targs.exists(typeHasRef) case Select(nu, nme.CONSTRUCTOR) => parentHasRef(nu) case New(tpt) => typeHasRef(tpt) - case parent if parent.isType => typeHasRef(parent) + case parent => parent.isType && typeHasRef(parent) } parents.isEmpty || // a parent class that refers to type parameters will be generated in this case From 9dc2916e15d46fec016c4a19fc5e7cbbf084d1d7 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Thu, 28 Mar 2019 15:29:26 +0100 Subject: [PATCH 4/7] Check for illegal references to enum type parameters --- .../src/dotty/tools/dotc/ast/Desugar.scala | 18 ++++++---- .../dotty/tools/dotc/ast/DesugarEnums.scala | 34 +++++++++++++------ compiler/src/dotty/tools/dotc/ast/Trees.scala | 2 +- docs/docs/reference/enums/desugarEnums.md | 8 +++++ tests/neg/enums.scala | 6 ++++ 5 files changed, 50 insertions(+), 18 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/ast/Desugar.scala b/compiler/src/dotty/tools/dotc/ast/Desugar.scala index 2fc1e8402fe0..e44a7df0d41a 100644 --- a/compiler/src/dotty/tools/dotc/ast/Desugar.scala +++ b/compiler/src/dotty/tools/dotc/ast/Desugar.scala @@ -355,11 +355,14 @@ object desugar { val originalVparamss = constr1.vparamss lazy val derivedEnumParams = enumClass.typeParams.map(derivedTypeParam) val impliedTparams = - if (isEnumCase && originalTparams.isEmpty && - typeParamIsReferenced(enumClass.typeParams, originalVparamss, parents)) - derivedEnumParams.map(tdef => tdef.withFlags(tdef.mods.flags | PrivateLocal)) - else - originalTparams + if (isEnumCase) { + val tparamReferenced = typeParamIsReferenced( + enumClass.typeParams, originalTparams, originalVparamss, parents) + if (originalTparams.isEmpty && (parents.isEmpty || tparamReferenced)) + derivedEnumParams.map(tdef => tdef.withFlags(tdef.mods.flags | PrivateLocal)) + else originalTparams + } + else originalTparams val constrTparams = impliedTparams.map(toDefParam) val constrVparamss = if (originalVparamss.isEmpty) { // ensure parameter list is non-empty @@ -739,8 +742,11 @@ object desugar { if (mods is Package) PackageDef(Ident(moduleName), cpy.ModuleDef(mdef)(nme.PACKAGE, impl).withMods(mods &~ Package) :: Nil) - else if (isEnumCase) + else if (isEnumCase) { + typeParamIsReferenced(enumClass.typeParams, Nil, Nil, impl.parents) + // used to check there are no illegal references to enum's type parameters in parents expandEnumModule(moduleName, impl, mods, mdef.span) + } else { val clsName = moduleName.moduleClassName val clsRef = Ident(clsName) diff --git a/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala b/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala index c88d92588b6e..42cbef882321 100644 --- a/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala +++ b/compiler/src/dotty/tools/dotc/ast/DesugarEnums.scala @@ -168,22 +168,34 @@ object DesugarEnums { } } - /** Is a type parameter in `tparams` referenced from an enum class case that has - * given value parameters `vparamss` and given parents `parents`? + /** Is a type parameter in `enumTypeParams` referenced from an enum class case that has + * given type parameters `caseTypeParams`, value parameters `vparamss` and parents `parents`? + * Issues an error if that is the case but the reference is illegal. + * The reference could be illegal for two reasons: + * - explicit type parameters are given + * - it's a value case, i.e. no value parameters are given */ - def typeParamIsReferenced(tparams: List[TypeSymbol], vparamss: List[List[ValDef]], parents: List[Tree])(implicit ctx: Context): Boolean = { + def typeParamIsReferenced( + enumTypeParams: List[TypeSymbol], + caseTypeParams: List[TypeDef], + vparamss: List[List[ValDef]], + parents: List[Tree])(implicit ctx: Context): Boolean = { - val searchRef = new untpd.TreeAccumulator[Boolean] { - var tparamNames = tparams.map(_.name).toSet[Name] + object searchRef extends UntypedTreeAccumulator[Boolean] { + var tparamNames = enumTypeParams.map(_.name).toSet[Name] def underBinders(binders: List[MemberDef], op: => Boolean): Boolean = { val saved = tparamNames tparamNames = tparamNames -- binders.map(_.name) try op finally tparamNames = saved } - def apply(x: Boolean, tree: Tree)(implicit ctx: Context) = x || { + def apply(x: Boolean, tree: Tree)(implicit ctx: Context): Boolean = x || { tree match { - case Ident(name) => tparamNames.contains(name) + case Ident(name) => + val matches = tparamNames.contains(name) + if (matches && (caseTypeParams.nonEmpty || vparamss.isEmpty)) + ctx.error(i"illegal reference to type parameter $name from enum case", tree.sourcePos) + matches case LambdaTypeTree(lambdaParams, body) => underBinders(lambdaParams, foldOver(x, tree)) case RefinedTypeTree(parent, refinements) => @@ -192,9 +204,11 @@ object DesugarEnums { case _ => foldOver(x, tree) } } + def apply(tree: Tree)(implicit ctx: Context): Boolean = + underBinders(caseTypeParams, apply(false, tree)) } - def typeHasRef(tpt: Tree) = searchRef(false, tpt) + def typeHasRef(tpt: Tree) = searchRef(tpt) def valDefHasRef(vd: ValDef) = typeHasRef(vd.tpt) def parentHasRef(parent: Tree): Boolean = parent match { case Apply(fn, _) => parentHasRef(fn) @@ -204,9 +218,7 @@ object DesugarEnums { case parent => parent.isType && typeHasRef(parent) } - parents.isEmpty || // a parent class that refers to type parameters will be generated in this case - vparamss.exists(_.exists(valDefHasRef)) || - parents.exists(parentHasRef) + vparamss.exists(_.exists(valDefHasRef)) || parents.exists(parentHasRef) } /** A pair consisting of diff --git a/compiler/src/dotty/tools/dotc/ast/Trees.scala b/compiler/src/dotty/tools/dotc/ast/Trees.scala index 8226c6b19133..ba070fb148a0 100644 --- a/compiler/src/dotty/tools/dotc/ast/Trees.scala +++ b/compiler/src/dotty/tools/dotc/ast/Trees.scala @@ -1419,7 +1419,7 @@ object Trees { } def foldMoreCases(x: X, tree: Tree)(implicit ctx: Context): X = { - assert(ctx.reporter.errorsReported || ctx.mode.is(Mode.Interactive)) + assert(ctx.reporter.errorsReported || ctx.mode.is(Mode.Interactive), tree) // In interactive mode, errors might come from previous runs. // In case of errors it may be that typed trees point to untyped ones. // The IDE can still traverse inside such trees, either in the run where errors diff --git a/docs/docs/reference/enums/desugarEnums.md b/docs/docs/reference/enums/desugarEnums.md index fd59e469c2cb..38b18fb3cb51 100644 --- a/docs/docs/reference/enums/desugarEnums.md +++ b/docs/docs/reference/enums/desugarEnums.md @@ -127,6 +127,9 @@ map into case classes or vals. as one of the `enumValues` of the enumeration (see below). `$values` is a compiler-defined private value in the companion object. + It is an error if a value case refers to a type parameter of the enclosing `enum` + in a type argument of ``. + 9. A class case case C extends @@ -145,6 +148,11 @@ map into case classes or vals. where `n` is the ordinal number of the case in the companion object, starting from 0. + It is an error if a value case refers to a type parameter of the enclosing `enum` + in a parameter type in `` or in a type argument of ``, unless that parameter is already + a type parameter of the case, i.e. the parameter name is defined in ``. + + ### Translation of Enumerations Non-generic enums `E` that define one or more singleton cases diff --git a/tests/neg/enums.scala b/tests/neg/enums.scala index 0e120e2bc420..ce2a8b1f88a2 100644 --- a/tests/neg/enums.scala +++ b/tests/neg/enums.scala @@ -24,6 +24,12 @@ enum E4 { case class C4() extends E4 // error: cannot extend enum case object O4 extends E4 // error: cannot extend enum +enum Captured[T] { + case Case1[U](x: T) extends Captured[U] // error: illegal reference to type parameter T from enum case + case Case2[U]() extends Captured[T] // error: illegal reference to type parameter T from enum case + case Case3 extends Captured[T] // error: illegal reference to type parameter T from enum case +} + enum Option[+T] derives Eql { case Some(x: T) case None From 87125598b5cc2704906cfcd7b41a05b0ef8efbc0 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Thu, 28 Mar 2019 18:22:25 +0100 Subject: [PATCH 5/7] Add test --- tests/pos/enums-capture.scala | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 tests/pos/enums-capture.scala diff --git a/tests/pos/enums-capture.scala b/tests/pos/enums-capture.scala new file mode 100644 index 000000000000..d62074bdd75a --- /dev/null +++ b/tests/pos/enums-capture.scala @@ -0,0 +1,7 @@ +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) +} From 7ccc244d691896643c25ff91cd40ccfc692d9256 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Thu, 28 Mar 2019 20:55:23 +0100 Subject: [PATCH 6/7] Fix companion parents for enum case 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. --- compiler/src/dotty/tools/dotc/ast/Desugar.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/ast/Desugar.scala b/compiler/src/dotty/tools/dotc/ast/Desugar.scala index e44a7df0d41a..09035e9b5f2d 100644 --- a/compiler/src/dotty/tools/dotc/ast/Desugar.scala +++ b/compiler/src/dotty/tools/dotc/ast/Desugar.scala @@ -598,7 +598,8 @@ object desugar { if (constrTparams.nonEmpty || constrVparamss.length > 1 || mods.is(Abstract) || - restrictedAccess) anyRef + restrictedAccess || + isEnumCase && applyResultTpt.isEmpty) anyRef else // todo: also use anyRef if constructor has a dependent method type (or rule that out)! (constrVparamss :\ (if (isEnumCase) applyResultTpt else classTypeRef)) ( From d1edad6ed68243f6c005c1f0e54fe3395e55f85c Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Thu, 28 Mar 2019 21:21:08 +0100 Subject: [PATCH 7/7] Never generate function parents for companions of enum cases The previous condition when we can and cannot generate a parent is quite obscure, so we should drop it altogether. --- compiler/src/dotty/tools/dotc/ast/Desugar.scala | 4 ++-- tests/neg/enums.scala | 5 +++++ tests/run/enums.scala | 1 + 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/ast/Desugar.scala b/compiler/src/dotty/tools/dotc/ast/Desugar.scala index 09035e9b5f2d..7e02680ef3fb 100644 --- a/compiler/src/dotty/tools/dotc/ast/Desugar.scala +++ b/compiler/src/dotty/tools/dotc/ast/Desugar.scala @@ -599,10 +599,10 @@ object desugar { constrVparamss.length > 1 || mods.is(Abstract) || restrictedAccess || - isEnumCase && applyResultTpt.isEmpty) anyRef + isEnumCase) anyRef else // todo: also use anyRef if constructor has a dependent method type (or rule that out)! - (constrVparamss :\ (if (isEnumCase) applyResultTpt else classTypeRef)) ( + (constrVparamss :\ classTypeRef) ( (vparams, restpe) => Function(vparams map (_.tpt), restpe)) def widenedCreatorExpr = (creatorExpr /: widenDefs)((rhs, meth) => Apply(Ident(meth.name), rhs :: Nil)) diff --git a/tests/neg/enums.scala b/tests/neg/enums.scala index ce2a8b1f88a2..813af890322a 100644 --- a/tests/neg/enums.scala +++ b/tests/neg/enums.scala @@ -19,6 +19,11 @@ enum E3[-T <: Ordered[T]] { enum E4 { case C + case C4(x: Int) +} +object E4 { + val x1: Int => E4 = C4 // error: found: C4, required: Int => E4 + val x2: Int => E4 = C4(_) // ok } case class C4() extends E4 // error: cannot extend enum diff --git a/tests/run/enums.scala b/tests/run/enums.scala index 1a515db40bda..1382533e9fda 100644 --- a/tests/run/enums.scala +++ b/tests/run/enums.scala @@ -105,6 +105,7 @@ object Test6 { case Green extends Color(3) case Red extends Color(2) case Violet extends Color(Green.x + Red.x) + case RGB(xx: Int) extends Color(xx) } }