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/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 } 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/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") || 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 new file mode 100644 index 000000000000..530df1bee1e8 --- /dev/null +++ b/tests/neg/mixin-forwarder-clash1.scala @@ -0,0 +1,38 @@ +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 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(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 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. + // + // 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 new file mode 100644 index 000000000000..4841df3670b7 --- /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 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 new file mode 100644 index 000000000000..6723785bdf94 --- /dev/null +++ b/tests/neg/mixin-forwarder-clash2/B_2.scala @@ -0,0 +1,16 @@ +class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] // error + // We get a mixin forwarder for TwoB: + // 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.