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.

Author: smarter

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) =
@@ -90,34 +88,25 @@ class ElimErasedValueType extends MiniPhase with InfoTransformer {
       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 = math.max(
+        sym1.originDenotation.validFor.firstPhaseId,
+        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 =

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.

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

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.