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
     scala/scala@4366332
     and
     scala/scala@e3ef657
  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 (scala/bug#8905). We'll either have
  to do something similar, or investigate a different solution like
  scala/scala#7843.

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

@@ -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.

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

@@ -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

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.

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.

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

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.