From 147df0daa67d11503d54053d4f3df23f2787c8c3 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 12 Mar 2019 09:20:39 +0100 Subject: [PATCH 1/4] Add double-defs checking for mixin forwarders And demonstrate that the current scheme of generating mixin forwarders before erasure is broken: - Because they're generated before erasure, their erasure in the class where their defined might differ from the erasure of the method they forward to. This is normally OK since they'll get a bridge. - However, mixin forwarders can clash with other methods as demonstrated by neg/mixin-forwarder-clash1, but these clashes won't be detected under separate compilation since double-defs checks are only done based on the signature of methods where they're defined, so neg/mixin-forwarder-clash2 fails. Checking for clashes against the signatures of all possible forwarders would be really annoying and possibly expensive, so instead the next commit solves this issue by moving mixin forwarder generation after erasure. --- .../dotc/transform/ElimErasedValueType.scala | 29 ++++--------- tests/neg/mixin-forwarder-clash1.scala | 41 +++++++++++++++++++ tests/neg/mixin-forwarder-clash2/A_1.scala | 23 +++++++++++ tests/neg/mixin-forwarder-clash2/B_2.scala | 9 ++++ 4 files changed, 81 insertions(+), 21 deletions(-) create mode 100644 tests/neg/mixin-forwarder-clash1.scala create mode 100644 tests/neg/mixin-forwarder-clash2/A_1.scala create mode 100644 tests/neg/mixin-forwarder-clash2/B_2.scala diff --git a/compiler/src/dotty/tools/dotc/transform/ElimErasedValueType.scala b/compiler/src/dotty/tools/dotc/transform/ElimErasedValueType.scala index 8e974f41730e..546e8319f7f5 100644 --- a/compiler/src/dotty/tools/dotc/transform/ElimErasedValueType.scala +++ b/compiler/src/dotty/tools/dotc/transform/ElimErasedValueType.scala @@ -20,7 +20,7 @@ object ElimErasedValueType { * of methods that now have the same signature but were not considered matching * before erasure. */ -class ElimErasedValueType extends MiniPhase with InfoTransformer { +class ElimErasedValueType extends MiniPhase with InfoTransformer { thisPhase => import tpd._ @@ -75,11 +75,9 @@ class ElimErasedValueType extends MiniPhase with InfoTransformer { /** Check that we don't have pairs of methods that override each other after * this phase, yet do not have matching types before erasure. - * The before erasure test is performed after phase elimRepeated, so we - * do not need to special case pairs of `T* / Seq[T]` parameters. */ private def checkNoClashes(root: Symbol)(implicit ctx: Context) = { - val opc = new OverridingPairs.Cursor(root) { + val opc = new OverridingPairs.Cursor(root)(ctx.withPhase(thisPhase)) { override def exclude(sym: Symbol) = !sym.is(Method) || sym.is(Bridge) || super.exclude(sym) override def matches(sym1: Symbol, sym2: Symbol) = @@ -89,35 +87,24 @@ class ElimErasedValueType extends MiniPhase with InfoTransformer { val site = root.thisType val info1 = site.memberInfo(sym1) val info2 = site.memberInfo(sym2) - def isDefined(sym: Symbol) = sym.originDenotation.validFor.firstPhaseId <= ctx.phaseId - if (isDefined(sym1) && isDefined(sym2) && !info1.matchesLoosely(info2)) - // The reason for the `isDefined` condition is that we need to exclude mixin forwarders - // from the tests. For instance, in compileStdLib, compiling scala.immutable.SetProxy, line 29: - // new AbstractSet[B] with SetProxy[B] { val self = newSelf } - // This generates two forwarders, one in AbstractSet, the other in the anonymous class itself. - // Their signatures are: - // method map: [B, That] - // (f: B => B)(implicit bf: scala.collection.generic.CanBuildFrom[scala.collection.immutable.Set[B], B, That]): That override in anonymous class scala.collection.AbstractSet[B] with scala.collection.immutable.SetProxy[B]{...} and - // method map: [B, That](f: B => B)(implicit bf: scala.collection.generic.CanBuildFrom[scala.collection.Set[B], B, That]): That override in class AbstractSet - // These have same type after erasure: - // (f: Function1, bf: scala.collection.generic.CanBuildFrom): Object - // - // The problem is that `map` was forwarded twice, with different instantiated types. - // Maybe we should move mixin forwarding after erasure to avoid redundant forwarders like these. + if (!info1.matchesLoosely(info2)) ctx.error(DoubleDefinition(sym1, sym2, root), root.sourcePos) } val earlyCtx = ctx.withPhase(ctx.elimRepeatedPhase.next) while (opc.hasNext) { val sym1 = opc.overriding val sym2 = opc.overridden + // Do the test at the earliest phase where both symbols existed. + val phaseId = + sym1.originDenotation.validFor.firstPhaseId max sym2.originDenotation.validFor.firstPhaseId checkNoConflict(sym1, sym2, sym1.info)(earlyCtx) opc.next() } } - override def transformTypeDef(tree: TypeDef)(implicit ctx: Context): Tree = { + override def prepareForTypeDef(tree: TypeDef)(implicit ctx: Context): Context = { checkNoClashes(tree.symbol) - tree + ctx } override def transformInlined(tree: Inlined)(implicit ctx: Context): Tree = diff --git a/tests/neg/mixin-forwarder-clash1.scala b/tests/neg/mixin-forwarder-clash1.scala new file mode 100644 index 000000000000..41b25c20eb3d --- /dev/null +++ b/tests/neg/mixin-forwarder-clash1.scala @@ -0,0 +1,41 @@ +class Foo + +// Using self-types to force mixin forwarders + +trait OneA[X] { + def concat(suffix: Int): X = ??? +} + +trait OneB[X] { self: OneA[X] => + override def concat(suffix: Int): X = ??? +} + +trait TwoA[Y <: Foo] { + def concat[Dummy](suffix: Int): Y = ??? +} + +trait TwoB[Y <: Foo] { self: TwoA[Y] => + override def concat[Dummy](suffix: Int): Y = ??? +} + +class Bar1 extends OneA[Foo] with OneB[Foo] + // Because mixin forwarders are generated before erasure, we get: + // override def concat(suffix: Int): Foo + +class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] // error + // We get a mixin forwarder for TwoB: + // override def concat[Dummy](suffix: Int): Foo + // which gets erased to: + // override def concat(suffix: Int): Foo + // This clashes with the forwarder generated in Bar1, and the compiler detects that: + // + // |class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] + // | ^ + // | Name clash between defined and inherited member: + // | override def concat(suffix: Int): Foo in class Bar1 and + // | override def concat: [Dummy](suffix: Int): Foo in class Bar2 + // | have the same type after erasure. + // + // But note that the compiler is able to see the mixin forwarder in Bar1 + // only because of joint compilation, this doesn't work with separate + // compilation as in mixin-forwarder-clash2. diff --git a/tests/neg/mixin-forwarder-clash2/A_1.scala b/tests/neg/mixin-forwarder-clash2/A_1.scala new file mode 100644 index 000000000000..1aaabe95eed8 --- /dev/null +++ b/tests/neg/mixin-forwarder-clash2/A_1.scala @@ -0,0 +1,23 @@ +class Foo + +// Using self-types to force mixin forwarders + +trait OneA[X] { + def concat(suffix: Int): X = ??? +} + +trait OneB[X] { self: OneA[X] => + override def concat(suffix: Int): X = ??? +} + +trait TwoA[Y <: Foo] { + def concat[Dummy](suffix: Int): Y = ??? +} + +trait TwoB[Y <: Foo] { self: TwoA[Y] => + override def concat[Dummy](suffix: Int): Y = ??? +} + +class Bar1 extends OneA[Foo] with OneB[Foo] +// Because mixin forwarders are generated before erasure, we get: +// override def concat(suffix: Int): Foo diff --git a/tests/neg/mixin-forwarder-clash2/B_2.scala b/tests/neg/mixin-forwarder-clash2/B_2.scala new file mode 100644 index 000000000000..704980d9fc92 --- /dev/null +++ b/tests/neg/mixin-forwarder-clash2/B_2.scala @@ -0,0 +1,9 @@ +class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] // error + // We get a mixin forwarder for TwoB: + // override def concat[Dummy](suffix: Int): Foo + // which gets erased to: + // override def concat(suffix: Int): Foo + // This clashes with the forwarder generated in Bar1, but + // unlike with mixin-forwarder-clash1, the compiler + // doesn't detect that due to separate compilation, + // so this test case fails. From 3138447a18cfeb5dd6dba76b511c331af7a6fe0d Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Sun, 10 Mar 2019 23:52:32 +0100 Subject: [PATCH 2/4] Move mixin forwarders generation after erasure * Pros - When forwarding before erasure, we might end up needing a bridge, the non-bridged forwarder therefore serves no practical purpose and can cause clashes. These are problematic for two reasons: 1. Valid Scala 2 code might break, and can only be fixed in binary incompatible way, like with https://github.com/scala/scala/commit/43663326a8191250c78aaabb89898410372a21b9 and https://github.com/scala/scala/commit/e3ef6573a55cb6e7ecf80ee6ebd312876f7f12df 2. Clashes might not be detected under separate compilation, as demonstrated by the previous commit. Forwarding after erasure solves both of these problems. - Scala 2 has always done it this way, so we're less likely to run into surprising problems. - Compiling the 2.12 standard library got 20% faster, I suspect that the previous hotspot in `isCurrent` is no longer so hot when run after erasure, so I removed the comment there. * Cons - When forwarding after erasure, generic signatures will be missing, Scala 2 tries to restore this information but that doesn't always work (https://github.com/scala/bug/issues/8905). We'll either have to do something similar, or investigate a different solution like https://github.com/scala/scala/pull/7843. --- compiler/src/dotty/tools/dotc/Compiler.scala | 2 +- .../dotty/tools/dotc/transform/Mixin.scala | 22 +++++++-- .../dotty/tools/dotc/transform/MixinOps.scala | 8 +--- .../tools/dotc/transform/ResolveSuper.scala | 34 ++++---------- tests/neg/mixin-forwarder-clash1.check | 5 +++ tests/neg/mixin-forwarder-clash1.scala | 25 +++++------ tests/neg/mixin-forwarder-clash2.check | 5 +++ tests/neg/mixin-forwarder-clash2/A_1.scala | 8 ++-- tests/neg/mixin-forwarder-clash2/B_2.scala | 21 ++++++--- tests/pos/mixin-forwarder-clash1.scala | 45 +++++++++++++++++++ tests/pos/mixin-forwarder-clash2/A_1.scala | 23 ++++++++++ tests/pos/mixin-forwarder-clash2/B_2.scala | 14 ++++++ 12 files changed, 150 insertions(+), 62 deletions(-) create mode 100644 tests/neg/mixin-forwarder-clash1.check create mode 100644 tests/neg/mixin-forwarder-clash2.check create mode 100644 tests/pos/mixin-forwarder-clash1.scala create mode 100644 tests/pos/mixin-forwarder-clash2/A_1.scala create mode 100644 tests/pos/mixin-forwarder-clash2/B_2.scala diff --git a/compiler/src/dotty/tools/dotc/Compiler.scala b/compiler/src/dotty/tools/dotc/Compiler.scala index 96404798a651..6fc01fcee7c7 100644 --- a/compiler/src/dotty/tools/dotc/Compiler.scala +++ b/compiler/src/dotty/tools/dotc/Compiler.scala @@ -82,7 +82,7 @@ class Compiler { new CollectNullableFields, // Collect fields that can be nulled out after use in lazy initialization new ElimOuterSelect, // Expand outer selections new AugmentScala2Traits, // Augments Scala2 traits with additional members needed for mixin composition. - new ResolveSuper, // Implement super accessors and add forwarders to trait methods + new ResolveSuper, // Implement super accessors new FunctionXXLForwarders, // Add forwarders for FunctionXXL apply method new ArrayConstructors) :: // Intercept creation of (non-generic) arrays and intrinsify. List(new Erasure) :: // Rewrite types to JVM model, erasing all type parameters, abstract types and refinements. diff --git a/compiler/src/dotty/tools/dotc/transform/Mixin.scala b/compiler/src/dotty/tools/dotc/transform/Mixin.scala index 1d7029c8433b..604719b5067d 100644 --- a/compiler/src/dotty/tools/dotc/transform/Mixin.scala +++ b/compiler/src/dotty/tools/dotc/transform/Mixin.scala @@ -86,6 +86,14 @@ object Mixin { * * def x_=(y: T) = () * + * 3.5 (done in `mixinForwarders`) For every method + * ` def f[Ts](ps1)...(psN): U` imn M` that needs to be disambiguated: + * + * def f[Ts](ps1)...(psN): U = super[M].f[Ts](ps1)...(psN) + * + * A method in M needs to be disambiguated if it is concrete, not overridden in C, + * and if it overrides another concrete method. + * * 4. (done in `transformTemplate` and `transformSym`) Drop all parameters from trait * constructors. * @@ -239,7 +247,7 @@ class Mixin extends MiniPhase with SymTransformer { thisPhase => else initial // transformFollowing call is needed to make memoize & lazy vals run - transformFollowing(DefDef(implementation(getter.asTerm), rhs)) + transformFollowing(DefDef(mkForwarder(getter.asTerm), rhs)) } else if (isScala2x || was(getter, ParamAccessor | Lazy)) EmptyTree else initial @@ -248,7 +256,15 @@ class Mixin extends MiniPhase with SymTransformer { thisPhase => def setters(mixin: ClassSymbol): List[Tree] = for (setter <- mixin.info.decls.filter(setr => setr.isSetter && !was(setr, Deferred))) - yield transformFollowing(DefDef(implementation(setter.asTerm), unitLiteral.withSpan(cls.span))) + yield transformFollowing(DefDef(mkForwarder(setter.asTerm), unitLiteral.withSpan(cls.span))) + + def mixinForwarders(mixin: ClassSymbol): List[Tree] = + for (meth <- mixin.info.decls.toList if needsForwarder(meth)) + yield { + util.Stats.record("mixin forwarders") + transformFollowing(polyDefDef(mkForwarder(meth.asTerm), forwarder(meth))) + } + cpy.Template(impl)( constr = @@ -259,7 +275,7 @@ class Mixin extends MiniPhase with SymTransformer { thisPhase => if (cls is Trait) traitDefs(impl.body) else { val mixInits = mixins.flatMap { mixin => - flatten(traitInits(mixin)) ::: superCallOpt(mixin) ::: setters(mixin) + flatten(traitInits(mixin)) ::: superCallOpt(mixin) ::: setters(mixin) ::: mixinForwarders(mixin) } superCallOpt(superCls) ::: mixInits ::: impl.body }) diff --git a/compiler/src/dotty/tools/dotc/transform/MixinOps.scala b/compiler/src/dotty/tools/dotc/transform/MixinOps.scala index 611c621f8750..44040d454902 100644 --- a/compiler/src/dotty/tools/dotc/transform/MixinOps.scala +++ b/compiler/src/dotty/tools/dotc/transform/MixinOps.scala @@ -18,7 +18,7 @@ class MixinOps(cls: ClassSymbol, thisPhase: DenotTransformer)(implicit ctx: Cont map(n => ctx.getClassIfDefined("org.junit." + n)). filter(_.exists) - def implementation(member: TermSymbol): TermSymbol = { + def mkForwarder(member: TermSymbol): TermSymbol = { val res = member.copy( owner = cls, name = member.name.stripScala2LocalSuffix, @@ -45,12 +45,6 @@ class MixinOps(cls: ClassSymbol, thisPhase: DenotTransformer)(implicit ctx: Cont def isCurrent(sym: Symbol): Boolean = ctx.atPhase(thisPhase) { implicit ctx => cls.info.nonPrivateMember(sym.name).hasAltWith(_.symbol == sym) - // this is a hot spot, where we spend several seconds while compiling stdlib - // unfortunately it will discard and recompute all the member chaches, - // both making itself slow and slowing down anything that runs after it - // because resolveSuper uses hacks with explicit adding to scopes through .enter - // this cannot be fixed by a smarter caching strategy. With current implementation - // we HAVE to discard caches here for correctness } /** Does `method` need a forwarder to in class `cls` diff --git a/compiler/src/dotty/tools/dotc/transform/ResolveSuper.scala b/compiler/src/dotty/tools/dotc/transform/ResolveSuper.scala index 970984bd380d..daf82cbe064b 100644 --- a/compiler/src/dotty/tools/dotc/transform/ResolveSuper.scala +++ b/compiler/src/dotty/tools/dotc/transform/ResolveSuper.scala @@ -14,27 +14,16 @@ import NameKinds._ import ResolveSuper._ import reporting.diagnostic.messages.IllegalSuperAccessor -/** This phase adds super accessors and method overrides where - * linearization differs from Java's rule for default methods in interfaces. - * In particular: +/** This phase implements super accessors in classes that need them. * - * For every trait M directly implemented by the class (see SymUtils.mixin), in - * reverse linearization order, add the following definitions to C: + * For every trait M directly implemented by the class (see SymUtils.mixin), in + * reverse linearization order, add the following definitions to C: * - * 3.1 (done in `superAccessors`) For every superAccessor - * ` def super$f[Ts](ps1)...(psN): U` in M: + * For every superAccessor ` def super$f[Ts](ps1)...(psN): U` in M: * - * def super$f[Ts](ps1)...(psN): U = super[S].f[Ts](ps1)...(psN) + * def super$f[Ts](ps1)...(psN): U = super[S].f[Ts](ps1)...(psN) * - * where `S` is the superclass of `M` in the linearization of `C`. - * - * 3.2 (done in `methodOverrides`) For every method - * ` def f[Ts](ps1)...(psN): U` in M` that needs to be disambiguated: - * - * def f[Ts](ps1)...(psN): U = super[M].f[Ts](ps1)...(psN) - * - * A method in M needs to be disambiguated if it is concrete, not overridden in C, - * and if it overrides another concrete method. + * where `S` is the superclass of `M` in the linearization of `C`. * * This is the first part of what was the mixin phase. It is complemented by * Mixin, which runs after erasure. @@ -59,17 +48,10 @@ class ResolveSuper extends MiniPhase with IdentityDenotTransformer { thisPhase = for (superAcc <- mixin.info.decls.filter(_.isSuperAccessor)) yield { util.Stats.record("super accessors") - polyDefDef(implementation(superAcc.asTerm), forwarder(rebindSuper(cls, superAcc))) - } - - def methodOverrides(mixin: ClassSymbol): List[Tree] = - for (meth <- mixin.info.decls.toList if needsForwarder(meth)) - yield { - util.Stats.record("method forwarders") - polyDefDef(implementation(meth.asTerm), forwarder(meth)) + polyDefDef(mkForwarder(superAcc.asTerm), forwarder(rebindSuper(cls, superAcc))) } - val overrides = mixins.flatMap(mixin => superAccessors(mixin) ::: methodOverrides(mixin)) + val overrides = mixins.flatMap(superAccessors) cpy.Template(impl)(body = overrides ::: impl.body) } diff --git a/tests/neg/mixin-forwarder-clash1.check b/tests/neg/mixin-forwarder-clash1.check new file mode 100644 index 000000000000..fff73b4ef2cc --- /dev/null +++ b/tests/neg/mixin-forwarder-clash1.check @@ -0,0 +1,5 @@ +<527..527> in mixin-forwarder-clash1.scala +Name clash between inherited members: +override def concat(suffix: Int): X in trait OneB at line 10 and +override def concat: [Dummy](suffix: Int): Y in trait TwoB at line 18 +have the same type after erasure. diff --git a/tests/neg/mixin-forwarder-clash1.scala b/tests/neg/mixin-forwarder-clash1.scala index 41b25c20eb3d..530df1bee1e8 100644 --- a/tests/neg/mixin-forwarder-clash1.scala +++ b/tests/neg/mixin-forwarder-clash1.scala @@ -10,32 +10,29 @@ trait OneB[X] { self: OneA[X] => override def concat(suffix: Int): X = ??? } -trait TwoA[Y <: Foo] { +trait TwoA[Y/* <: Foo*/] { def concat[Dummy](suffix: Int): Y = ??? } -trait TwoB[Y <: Foo] { self: TwoA[Y] => +trait TwoB[Y/* <: Foo*/] { self: TwoA[Y] => override def concat[Dummy](suffix: Int): Y = ??? } class Bar1 extends OneA[Foo] with OneB[Foo] - // Because mixin forwarders are generated before erasure, we get: - // override def concat(suffix: Int): Foo + // Because mixin forwarders are generated after erasure, we get: + // override def concat(suffix: Int): Object class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] // error // We get a mixin forwarder for TwoB: - // override def concat[Dummy](suffix: Int): Foo - // which gets erased to: - // override def concat(suffix: Int): Foo + // override def concat(suffix: Int): Object // This clashes with the forwarder generated in Bar1, and the compiler detects that: // // |class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] // | ^ - // | Name clash between defined and inherited member: - // | override def concat(suffix: Int): Foo in class Bar1 and - // | override def concat: [Dummy](suffix: Int): Foo in class Bar2 - // | have the same type after erasure. + // | Name clash between inherited members: + // | override def concat(suffix: Int): Object in trait OneB at line 10 and + // | override def concat: [Dummy](suffix: Int): Y in trait TwoB at line 18 + // | have the same type after erasure. // - // But note that the compiler is able to see the mixin forwarder in Bar1 - // only because of joint compilation, this doesn't work with separate - // compilation as in mixin-forwarder-clash2. + // This also works with separate compilation as demonstrated by + // mixin-forwarder-clash2. diff --git a/tests/neg/mixin-forwarder-clash2.check b/tests/neg/mixin-forwarder-clash2.check new file mode 100644 index 000000000000..42653538e72a --- /dev/null +++ b/tests/neg/mixin-forwarder-clash2.check @@ -0,0 +1,5 @@ +<6..6> in B_2.scala +Name clash between inherited members: +override def concat(suffix: Int): X in trait OneB and +override def concat: [Dummy](suffix: Int): Y in trait TwoB +have the same type after erasure. diff --git a/tests/neg/mixin-forwarder-clash2/A_1.scala b/tests/neg/mixin-forwarder-clash2/A_1.scala index 1aaabe95eed8..4841df3670b7 100644 --- a/tests/neg/mixin-forwarder-clash2/A_1.scala +++ b/tests/neg/mixin-forwarder-clash2/A_1.scala @@ -10,14 +10,14 @@ trait OneB[X] { self: OneA[X] => override def concat(suffix: Int): X = ??? } -trait TwoA[Y <: Foo] { +trait TwoA[Y/* <: Foo*/] { def concat[Dummy](suffix: Int): Y = ??? } -trait TwoB[Y <: Foo] { self: TwoA[Y] => +trait TwoB[Y/* <: Foo*/] { self: TwoA[Y] => override def concat[Dummy](suffix: Int): Y = ??? } class Bar1 extends OneA[Foo] with OneB[Foo] -// Because mixin forwarders are generated before erasure, we get: -// override def concat(suffix: Int): Foo +// Because mixin forwarders are generated after erasure, we get: +// override def concat(suffix: Int): Object diff --git a/tests/neg/mixin-forwarder-clash2/B_2.scala b/tests/neg/mixin-forwarder-clash2/B_2.scala index 704980d9fc92..6723785bdf94 100644 --- a/tests/neg/mixin-forwarder-clash2/B_2.scala +++ b/tests/neg/mixin-forwarder-clash2/B_2.scala @@ -1,9 +1,16 @@ class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] // error // We get a mixin forwarder for TwoB: - // override def concat[Dummy](suffix: Int): Foo - // which gets erased to: - // override def concat(suffix: Int): Foo - // This clashes with the forwarder generated in Bar1, but - // unlike with mixin-forwarder-clash1, the compiler - // doesn't detect that due to separate compilation, - // so this test case fails. + // override def concat(suffix: Int): Object + // This clashes with the forwarder generated in Bar1, and + // we can detect that even with separate compilation, + // because forwarders are always generated after erasure + // so their signature matches the erased signature of the + // method they forward to, which double-defs check will + // consider: + // + // |class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] + // | ^ + // | Name clash between inherited members: + // | override def concat(suffix: Int): X in trait OneB and + // | override def concat: [Dummy](suffix: Int): Y in trait TwoB + // | have the same type after erasure. diff --git a/tests/pos/mixin-forwarder-clash1.scala b/tests/pos/mixin-forwarder-clash1.scala new file mode 100644 index 000000000000..ef52a17b4e60 --- /dev/null +++ b/tests/pos/mixin-forwarder-clash1.scala @@ -0,0 +1,45 @@ +// This test case used to fail when mixin forwarders were generated before erasure, +// it doesn't anymore since the forwarders generated after erasure do not clash, +// the comments are preserved for posterity. + +class Foo + +// Using self-types to force mixin forwarders + +trait OneA[X] { + def concat(suffix: Int): X = ??? +} + +trait OneB[X] { self: OneA[X] => + override def concat(suffix: Int): X = ??? +} + +trait TwoA[Y <: Foo] { + def concat[Dummy](suffix: Int): Y = ??? +} + +trait TwoB[Y <: Foo] { self: TwoA[Y] => + override def concat[Dummy](suffix: Int): Y = ??? +} + +class Bar1 extends OneA[Foo] with OneB[Foo] + // Because mixin forwarders are generated before erasure, we get: + // override def concat(suffix: Int): Foo + +class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] // error + // We get a mixin forwarder for TwoB: + // override def concat[Dummy](suffix: Int): Foo + // which gets erased to: + // override def concat(suffix: Int): Foo + // This clashes with the forwarder generated in Bar1, and the compiler detects that: + // + // |class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] + // | ^ + // | Name clash between defined and inherited member: + // | override def concat(suffix: Int): Foo in class Bar1 and + // | override def concat: [Dummy](suffix: Int): Foo in class Bar2 + // | have the same type after erasure. + // + // But note that the compiler is able to see the mixin forwarder in Bar1 + // only because of joint compilation, this doesn't work with separate + // compilation as in mixin-forwarder-clash2. diff --git a/tests/pos/mixin-forwarder-clash2/A_1.scala b/tests/pos/mixin-forwarder-clash2/A_1.scala new file mode 100644 index 000000000000..1aaabe95eed8 --- /dev/null +++ b/tests/pos/mixin-forwarder-clash2/A_1.scala @@ -0,0 +1,23 @@ +class Foo + +// Using self-types to force mixin forwarders + +trait OneA[X] { + def concat(suffix: Int): X = ??? +} + +trait OneB[X] { self: OneA[X] => + override def concat(suffix: Int): X = ??? +} + +trait TwoA[Y <: Foo] { + def concat[Dummy](suffix: Int): Y = ??? +} + +trait TwoB[Y <: Foo] { self: TwoA[Y] => + override def concat[Dummy](suffix: Int): Y = ??? +} + +class Bar1 extends OneA[Foo] with OneB[Foo] +// Because mixin forwarders are generated before erasure, we get: +// override def concat(suffix: Int): Foo diff --git a/tests/pos/mixin-forwarder-clash2/B_2.scala b/tests/pos/mixin-forwarder-clash2/B_2.scala new file mode 100644 index 000000000000..00c72f665ca3 --- /dev/null +++ b/tests/pos/mixin-forwarder-clash2/B_2.scala @@ -0,0 +1,14 @@ +// This test case was supposed to fail when mixin forwarders were generated before erasure, +// but didn't due to separate compilation unlike mixin-forwarder-clash1, +// it's not supposed to fail anymore since the forwarders generated after erasure do not clash, +// the comments are preserved for posterity. + +class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] // error + // We get a mixin forwarder for TwoB: + // override def concat[Dummy](suffix: Int): Foo + // which gets erased to: + // override def concat(suffix: Int): Foo + // This clashes with the forwarder generated in Bar1, but + // unlike with mixin-forwarder-clash1, the compiler + // doesn't detect that due to separate compilation, + // so this test case fails. From c16fe105da31a366e9864f9d829f8bead3a1bda2 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Thu, 7 Mar 2019 17:39:13 +0100 Subject: [PATCH 3/4] Relax erased type check Now that mixin forwarders are generated after erasure, we may end up with a reference to ThisType(*:) after erasure in Tuple.scala, because *: extends NonEmptyTuple which erases to Product, and Product has methods that should get mixin forwarders. This fixes the tastyBootstrap test broken after the last commit. --- compiler/src/dotty/tools/dotc/transform/Erasure.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/transform/Erasure.scala b/compiler/src/dotty/tools/dotc/transform/Erasure.scala index 2fd68a391c43..1f468cb3a839 100644 --- a/compiler/src/dotty/tools/dotc/transform/Erasure.scala +++ b/compiler/src/dotty/tools/dotc/transform/Erasure.scala @@ -135,7 +135,7 @@ class Erasure extends Phase with DenotTransformer { def assertErased(tp: Type, tree: tpd.Tree = tpd.EmptyTree)(implicit ctx: Context): Unit = { def isAllowed(cls: Symbol, sourceName: String) = - tp.typeSymbol == cls && ctx.compilationUnit.source.file.name == sourceName + tp.widen.typeSymbol == cls && ctx.compilationUnit.source.file.name == sourceName assert(isErasedType(tp) || isAllowed(defn.ArrayClass, "Array.scala") || isAllowed(defn.TupleClass, "Tuple.scala") || From 96cc754e70661f09615a8208c7dd0c7879b7baa2 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Mon, 11 Mar 2019 01:16:04 +0100 Subject: [PATCH 4/4] AugmentScala2Traits: Handle traits that erase to a Scala 2 trait There is only one of those so far: NonEmptyTuple which erases to Product. Without this commit, the mixin forwarders to Product in the *: class in Tuple.scala were only generated if we happened to have seen augmented Product in a previous compilation unit in the same run. --- .../dotty/tools/dotc/core/TypeErasure.scala | 20 +++++++++---------- .../dotc/transform/AugmentScala2Traits.scala | 7 +++++-- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/TypeErasure.scala b/compiler/src/dotty/tools/dotc/core/TypeErasure.scala index 165df00f6994..3315bacdfb7e 100644 --- a/compiler/src/dotty/tools/dotc/core/TypeErasure.scala +++ b/compiler/src/dotty/tools/dotc/core/TypeErasure.scala @@ -36,6 +36,16 @@ object TypeErasure { private def erasureDependsOnArgs(sym: Symbol)(implicit ctx: Context) = sym == defn.ArrayClass || sym == defn.PairClass + def normalizeClass(cls: ClassSymbol)(implicit ctx: Context): ClassSymbol = { + if (cls.owner == defn.ScalaPackageClass) { + if (defn.specialErasure.contains(cls)) + return defn.specialErasure(cls) + if (cls == defn.UnitClass) + return defn.BoxedUnitClass + } + cls + } + /** A predicate that tests whether a type is a legal erased type. Only asInstanceOf and * isInstanceOf may have types that do not satisfy the predicate. * ErasedValueType is considered an erased type because it is valid after Erasure (it is @@ -530,16 +540,6 @@ class TypeErasure(isJava: Boolean, semiEraseVCs: Boolean, isConstructor: Boolean this(tp) } - private def normalizeClass(cls: ClassSymbol)(implicit ctx: Context): ClassSymbol = { - if (cls.owner == defn.ScalaPackageClass) { - if (defn.specialErasure.contains(cls)) - return defn.specialErasure(cls) - if (cls == defn.UnitClass) - return defn.BoxedUnitClass - } - cls - } - /** The name of the type as it is used in `Signature`s. * Need to ensure correspondence with erasure! */ diff --git a/compiler/src/dotty/tools/dotc/transform/AugmentScala2Traits.scala b/compiler/src/dotty/tools/dotc/transform/AugmentScala2Traits.scala index d63d7303a1f4..c95c516865e6 100644 --- a/compiler/src/dotty/tools/dotc/transform/AugmentScala2Traits.scala +++ b/compiler/src/dotty/tools/dotc/transform/AugmentScala2Traits.scala @@ -39,8 +39,11 @@ class AugmentScala2Traits extends MiniPhase with IdentityDenotTransformer { this override def transformTemplate(impl: Template)(implicit ctx: Context): Template = { val cls = impl.symbol.owner.asClass - for (mixin <- cls.mixins if mixin.is(Scala2x) && !mixin.is(Scala2xPartiallyAugmented)) - augmentScala2Trait(mixin) + for (mixin <- cls.mixins) { + val erasedMixin = TypeErasure.normalizeClass(mixin) + if (erasedMixin.is(Scala2x) && !erasedMixin.is(Scala2xPartiallyAugmented)) + augmentScala2Trait(erasedMixin) + } impl }