Merge pull request #6079 from dotty-staging/mixin-after-erasure-3

Move mixin forwarders generation after erasure

Author: smarter

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.

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!
    */

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
   }
 

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 <method> <touched> 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 <method> <touched> 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 =

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") ||

compiler/src/dotty/tools/dotc/transform/Mixin.scala

@@ -86,6 +86,14 @@ object Mixin {
  *
  *                <mods> def x_=(y: T) = ()
  *
+ *          3.5 (done in `mixinForwarders`) For every method
+ *          `<mods> def f[Ts](ps1)...(psN): U` imn M` that needs to be disambiguated:
+ *
+ *                <mods> 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
         })

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`

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
- *              `<mods> def super$f[Ts](ps1)...(psN): U` in M:
+ *  For every superAccessor `<mods> def super$f[Ts](ps1)...(psN): U` in M:
  *
- *                <mods> def super$f[Ts](ps1)...(psN): U = super[S].f[Ts](ps1)...(psN)
+ *       <mods> 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
- *              `<mods> def f[Ts](ps1)...(psN): U` in M` that needs to be disambiguated:
- *
- *                <mods> 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)
   }

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.

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.

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.

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

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.

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.