Merge pull request #2342 from dotty-staging/bridge

New bridge implementation

Author: odersky

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

@@ -0,0 +1,96 @@
+package dotty.tools
+package dotc
+package transform
+
+import core._
+import Symbols._, Types._, Contexts._, Decorators._, SymDenotations._, Flags._, Scopes._
+import DenotTransformers._
+import ast.untpd
+import collection.{mutable, immutable}
+import TypeErasure._
+import ValueClasses.isDerivedValueClass
+
+/** A helper class for generating bridge methods in class `root`. */
+class Bridges(root: ClassSymbol)(implicit ctx: Context) {
+  import ast.tpd._
+
+  assert(ctx.phase == ctx.erasurePhase.next)
+  private val preErasureCtx = ctx.withPhase(ctx.erasurePhase)
+
+  private class BridgesCursor(implicit ctx: Context) extends OverridingPairs.Cursor(root) {
+
+    /** Only use the superclass of `root` as a parent class. This means
+     *  overriding pairs that have a common implementation in a trait parent
+     *  are also counted. This is necessary because we generate bridge methods
+     *  only in classes, never in traits.
+     */
+    override def parents = Array(root.superClass)
+    override def exclude(sym: Symbol) = !sym.is(Method) || super.exclude(sym)
+  }
+
+  //val site = root.thisType
+
+  private var toBeRemoved = immutable.Set[Symbol]()
+  private val bridges = mutable.ListBuffer[Tree]()
+  private val bridgesScope = newScope
+  private val bridgeTarget = mutable.HashMap[Symbol, Symbol]()
+
+  /** Add a bridge between `member` and `other`, where `member` overrides `other`
+   *  before erasure, if the following conditions are satisfied.
+   *
+   *   - `member` and other have different signatures
+   *   - `member` is not inline
+   *   - there is not yet a bridge with the same name and signature in `root`
+   *
+   *  The bridge has the erased info of `other` and forwards to `member`.
+   */
+  private def addBridgeIfNeeded(member: Symbol, other: Symbol) = {
+    def bridgeExists =
+      bridgesScope.lookupAll(member.name).exists(bridge =>
+        bridgeTarget(bridge) == member && bridge.signature == other.signature)
+    if (!(member.is(Inline) || member.signature == other.signature || bridgeExists))
+      addBridge(member, other)
+  }
+
+  /** Generate bridge between `member` and `other`
+   */
+  private def addBridge(member: Symbol, other: Symbol) = {
+    val bridgePos = if (member.owner == root && member.pos.exists) member.pos else root.pos
+    val bridge = other.copy(
+      owner = root,
+      flags = (member.flags | Method | Bridge | Artifact) &~
+        (Accessor | ParamAccessor | CaseAccessor | Deferred | Lazy | Module),
+      coord = bridgePos).enteredAfter(ctx.erasurePhase.asInstanceOf[DenotTransformer]).asTerm
+
+    ctx.debuglog(
+      i"""generating bridge from ${other.showLocated}: ${other.info}
+             |to ${member.showLocated}: ${member.info} @ ${member.pos}
+             |bridge: ${bridge.showLocated} with flags: ${bridge.flags}""")
+
+    bridgeTarget(bridge) = member
+    bridgesScope.enter(bridge)
+
+    if (other.owner == root) {
+      root.delete(other)
+      toBeRemoved += other
+    }
+
+    bridges +=
+      DefDef(bridge, This(root).select(member).appliedToArgss(_)).withPos(bridge.pos)
+  }
+
+  /** Add all necessary bridges to template statements `stats`, and remove at the same
+   *  time deferred methods in `stats` that are replaced by a bridge with the same signature.
+   */
+  def add(stats: List[untpd.Tree]): List[untpd.Tree] =
+    if (root.is(Trait)) stats
+    else {
+      val opc = new BridgesCursor()(preErasureCtx)
+      while (opc.hasNext) {
+        if (!opc.overriding.is(Deferred)) addBridgeIfNeeded(opc.overriding, opc.overridden)
+        opc.next()
+      }
+      if (bridges.isEmpty) stats
+      else stats.filterNot(stat => toBeRemoved contains stat.symbol) ::: bridges.toList
+    }
+}

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

@@ -11,6 +11,9 @@ import TypeErasure.ErasedValueType, ValueClasses._
 /** This phase erases ErasedValueType to their underlying type.
  *  It also removes the synthetic cast methods u2evt$ and evt2u$ which are
  *  no longer needed afterwards.
+ *  Finally, it checks that we don't introduce "double definitions" of pairs
+ *  of methods that now have the same signature but were not considered matching
+ *  before erasure.
  */
 class ElimErasedValueType extends MiniPhaseTransform with InfoTransformer {
 
@@ -67,6 +70,44 @@ class ElimErasedValueType extends MiniPhaseTransform with InfoTransformer {
     transformTypeOfTree(t)
   }
 
+  /** 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) {
+      override def exclude(sym: Symbol) =
+        !sym.is(Method) || sym.is(Bridge) || super.exclude(sym)
+      override def matches(sym1: Symbol, sym2: Symbol) =
+        sym1.signature == sym2.signature
+    }
+    def checkNoConflict(sym1: Symbol, sym2: Symbol, info: Type)(implicit ctx: Context): Unit = {
+      val site = root.thisType
+      val info1 = site.memberInfo(sym1)
+      val info2 = site.memberInfo(sym2)
+      if (!info1.matchesLoosely(info2))
+        ctx.error(
+            em"""double definition:
+                |$sym1: $info1 in ${sym1.owner} ${sym1.flags} and
+                |$sym2: $info2 in ${sym2.owner} ${sym2.flags}
+                |have same type after erasure: $info""",
+            root.pos)
+    }
+    val earlyCtx = ctx.withPhase(ctx.elimRepeatedPhase.next)
+    while (opc.hasNext) {
+      val sym1 = opc.overriding
+      val sym2 = opc.overridden
+      checkNoConflict(sym1, sym2, sym1.info)(earlyCtx)
+      opc.next()
+    }
+  }
+
+  override def transformTypeDef(tree: TypeDef)(implicit ctx: Context, info: TransformerInfo): Tree = {
+    checkNoClashes(tree.symbol)
+    tree
+  }
+
   override def transformInlined(tree: Inlined)(implicit ctx: Context, info: TransformerInfo): Tree =
     transformTypeOfTree(tree)
 

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

@@ -67,7 +67,7 @@ class Erasure extends Phase with DenotTransformer { thisTransformer =>
         val oldOwner = ref.owner
         val newOwner = if (oldOwner eq defn.AnyClass) defn.ObjectClass else oldOwner
         val oldInfo = ref.info
-        val newInfo = transformInfo(ref.symbol, oldInfo)
+        val newInfo = transformInfo(oldSymbol, oldInfo)
         val oldFlags = ref.flags
         val newFlags =
           if (oldSymbol.is(Flags.TermParam) && isCompacted(oldSymbol.owner)) oldFlags &~ Flags.Param
@@ -594,117 +594,10 @@ object Erasure extends TypeTestsCasts{
       EmptyTree
 
     override def typedStats(stats: List[untpd.Tree], exprOwner: Symbol)(implicit ctx: Context): List[Tree] = {
-      val stats1 = Trees.flatten(super.typedStats(stats, exprOwner))
-      if (ctx.owner.isClass) stats1 ::: addBridges(stats, stats1)(ctx) else stats1
-    }
-
-    // this implementation doesn't check for bridge clashes with value types!
-    def addBridges(oldStats: List[untpd.Tree], newStats: List[tpd.Tree])(implicit ctx: Context): List[tpd.Tree] = {
-      val beforeCtx = ctx.withPhase(ctx.erasurePhase)
-      def traverse(after: List[Tree], before: List[untpd.Tree],
-                   emittedBridges: ListBuffer[tpd.DefDef] = ListBuffer[tpd.DefDef]()): List[tpd.DefDef] = {
-        after match {
-          case Nil => emittedBridges.toList
-          case (member: DefDef) :: newTail =>
-            before match {
-              case Nil => emittedBridges.toList
-              case (oldMember: untpd.DefDef) :: oldTail =>
-                try {
-                  val oldSymbol = oldMember.symbol(beforeCtx)
-                  val newSymbol = member.symbol(ctx)
-                  assert(oldSymbol.name(beforeCtx) == newSymbol.name,
-                    s"${oldSymbol.name(beforeCtx)} bridging with ${newSymbol.name}")
-                  val newOverridden = oldSymbol.denot.allOverriddenSymbols.toSet // TODO: clarify new <-> old in a comment; symbols are swapped here
-                  val oldOverridden = newSymbol.allOverriddenSymbols(beforeCtx).toSet // TODO: can we find a more efficient impl? newOverridden does not have to be a set!
-                  def stillInBaseClass(sym: Symbol) = ctx.owner derivesFrom sym.owner
-                  val neededBridges = (oldOverridden -- newOverridden).filter(stillInBaseClass)
-
-                  var minimalSet = Set[Symbol]()
-                  // compute minimal set of bridges that are needed:
-                  for (bridge <- neededBridges) {
-                    val isRequired = minimalSet.forall(nxtBridge => !(bridge.info =:= nxtBridge.info))
-
-                    if (isRequired) {
-                      // check for clashes
-                      val clash: Option[Symbol] = oldSymbol.owner.info.decls.lookupAll(bridge.name).find {
-                        sym =>
-                          (sym.name eq bridge.name) && sym.info.widen =:= bridge.info.widen
-                      }.orElse(
-                        emittedBridges.find(stat => (stat.name == bridge.name) && stat.tpe.widen =:= bridge.info.widen)
-                          .map(_.symbol))
-                      clash match {
-                        case Some(cl) =>
-                          ctx.error(i"bridge for method ${newSymbol.showLocated(beforeCtx)} of type ${newSymbol.info(beforeCtx)}\n" +
-                            i"clashes with ${cl.symbol.showLocated(beforeCtx)} of type ${cl.symbol.info(beforeCtx)}\n" +
-                            i"both have same type after erasure: ${bridge.symbol.info}")
-                        case None => minimalSet += bridge
-                      }
-                    }
-                  }
-
-                  val bridgeImplementations = minimalSet.map {
-                    sym => makeBridgeDef(member, sym)(ctx)
-                  }
-                  emittedBridges ++= bridgeImplementations
-                } catch {
-                  case ex: MergeError => ctx.error(ex.getMessage, member.pos)
-                }
-
-                traverse(newTail, oldTail, emittedBridges)
-              case notADefDef :: oldTail =>
-                traverse(after, oldTail, emittedBridges)
-            }
-          case notADefDef :: newTail =>
-            traverse(newTail, before, emittedBridges)
-        }
-      }
-
-      traverse(newStats, oldStats)
-    }
-
-    private final val NoBridgeFlags = Flags.Accessor | Flags.Deferred | Flags.Lazy | Flags.ParamAccessor
-
-    /** Create a bridge DefDef which overrides a parent method.
-     *
-     *  @param newDef     The DefDef which needs bridging because its signature
-     *                    does not match the parent method signature
-     *  @param parentSym  A symbol corresponding to the parent method to override
-     *  @return           A new DefDef whose signature matches the parent method
-     *                    and whose body only contains a call to newDef
-     */
-    def makeBridgeDef(newDef: tpd.DefDef, parentSym: Symbol)(implicit ctx: Context): tpd.DefDef = {
-      val newDefSym = newDef.symbol
-      val currentClass = newDefSym.owner.asClass
-
-      def error(reason: String) = {
-        assert(false, s"failure creating bridge from ${newDefSym} to ${parentSym}, reason: $reason")
-        ???
-      }
-      var excluded = NoBridgeFlags
-      if (!newDefSym.is(Flags.Protected)) excluded |= Flags.Protected // needed to avoid "weaker access" assertion failures in expandPrivate
-      val bridge = ctx.newSymbol(currentClass,
-        parentSym.name, parentSym.flags &~ excluded | Flags.Bridge, parentSym.info, coord = newDefSym.owner.coord).asTerm
-      bridge.enteredAfter(ctx.phase.prev.asInstanceOf[DenotTransformer]) // this should be safe, as we're executing in context of next phase
-      ctx.debuglog(s"generating bridge from ${newDefSym} to $bridge")
-
-      val sel: Tree = This(currentClass).select(newDefSym.termRef)
-
-      val resultType = parentSym.info.widen.resultType
-
-      val bridgeCtx = ctx.withOwner(bridge)
-
-      tpd.DefDef(bridge, { paramss: List[List[tpd.Tree]] =>
-          implicit val ctx = bridgeCtx
-
-          val rhs = paramss.foldLeft(sel)((fun, vparams) =>
-            fun.tpe.widen match {
-              case mt: MethodType =>
-                Apply(fun, (vparams, mt.paramInfos).zipped.map(adapt(_, _, untpd.EmptyTree)))
-              case a =>
-                error(s"can not resolve apply type $a")
-            })
-          adapt(rhs, resultType)
-      })
+      val stats1 =
+        if (takesBridges(ctx.owner)) new Bridges(ctx.owner.asClass).add(stats)
+        else stats
+      super.typedStats(stats1, exprOwner)
     }
 
     override def adapt(tree: Tree, pt: Type, original: untpd.Tree)(implicit ctx: Context): Tree =
@@ -715,4 +608,7 @@ object Erasure extends TypeTestsCasts{
         else adaptToType(tree, pt)
       }
   }
+
+  def takesBridges(sym: Symbol)(implicit ctx: Context) =
+    sym.isClass && !sym.is(Flags.Trait | Flags.Package)
 }

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

@@ -31,7 +31,9 @@ object OverridingPairs {
      */
     protected def exclude(sym: Symbol): Boolean = !sym.memberCanMatchInheritedSymbols
 
-    /** The parents of base (may also be refined).
+    /** The parents of base that are checked when deciding whether an overriding
+     *  pair has already been treated in a parent class.
+     *  This may be refined in subclasses. @see Bridges for a use case.
      */
     protected def parents: Array[Symbol] = base.info.parents.toArray map (_.typeSymbol)
 

compiler/test/dotc/tests.scala

@@ -193,6 +193,9 @@ class tests extends CompilerTest {
   @Test def neg_i1050 = compileFile(negCustomArgs, "i1050", List("-strict"))
   @Test def neg_i1240 = compileFile(negCustomArgs, "i1240")(allowDoubleBindings)
   @Test def neg_i2002 = compileFile(negCustomArgs, "i2002")(allowDoubleBindings)
+  @Test def neg_valueclasses_doubledefs = compileFile(negCustomArgs, "valueclasses-doubledefs")(allowDoubleBindings)
+  @Test def neg_valueclasses_doubledefs2 = compileFile(negCustomArgs, "valueclasses-doubledefs2")(allowDoubleBindings)
+  @Test def neg_valueclasses_pavlov = compileFile(negCustomArgs, "valueclasses-pavlov")(allowDoubleBindings)
 
   val negTailcallDir = negDir + "tailcall/"
   @Test def neg_tailcall_t1672b = compileFile(negTailcallDir, "t1672b")

tests/untried/neg/valueclasses-doubledefs.scala renamed to tests/neg/customArgs/valueclasses-doubledefs.scala

@@ -2,5 +2,6 @@ class Meter(val x: Double) extends AnyVal
 
 class Foo {
  def apply(x: Double) = x.toString
- def apply(x: Meter) = x.toString
+ def apply(x: Meter) = x.toString // error: double def
 }
+

tests/neg/customArgs/valueclasses-doubledefs2.scala

@@ -0,0 +1,10 @@
+class Meter(val x: Double) extends AnyVal
+
+trait A {
+  def apply(x: Double) = x.toString
+}
+trait B {
+  def apply(x: Meter) = x.toString
+}
+
+object Test extends A with B // error: double def

tests/untried/neg/valueclasses-pavlov.scala renamed to tests/neg/customArgs/valueclasses-pavlov.scala

@@ -5,7 +5,7 @@ trait Foo[T <: AnyVal] extends Any {
 
 class Box1(val value: String) extends AnyVal with Foo[Box2] {
   def foo(x: String) = "foo(String): ok"
-  def foo(x: Box2) = "foo(Box2): ok"
+  def foo(x: Box2) = "foo(Box2): ok"  // error: double def
 }
 
 class Box2(val value: String) extends AnyVal
@@ -17,7 +17,7 @@ object test2a {
     val b1 = new Box1(null)
     val b2 = new Box2(null)
     val f: Foo[Box2] = b1
-    println(f.foo(""))
-    println(f.foo(b2))
+    println(f.foo("")) // error: cannot merge
+    println(f.foo(b2)) // error: cannot merge
   }
 }

tests/neg/i1240a.scala renamed to tests/pending/neg/i1240a.scala

@@ -12,6 +12,10 @@ class B extends A {
   override def give[X] = Nil
   override def foo[B](x: C[B]): C[B] =  {println("B.C"); x} // error: merge error during erasure
   val a: A = this
-  a.foo(a.give[Int]) // what method should be called here in runtime?
+}
+
+object Test extends B {
+  def main(args: Array[String]): Unit =
+    a.foo(a.give[Int]) // what method should be called here in runtime?
 }
 

tests/pending/run/bridges.scala renamed to tests/run/bridges.scala

@@ -0,0 +1,34 @@
+/* 
+Minimized from collection strawman 
+This issue has a lot to do with both mixin and bridge generation and subtleties in JVM spec
+if something breaks in this test, this is not a minor issue. Be careful. Here be dragons.
+*/
+
+trait Define[A] {
+  protected def coll: Define[A]
+  def s = coll
+}
+
+trait Iterable[A] extends Define[A] {
+  protected def coll: this.type = this
+}
+
+trait Seq[A] extends Iterable[A]
+
+trait Super1[A] {
+  protected def coll: Iterable[A]
+}
+
+trait Super2[A] extends Super1[A] {
+  override protected def coll: Seq[A]
+}
+
+class Foo[T] extends Seq[T] with Super2[T] {
+}
+
+object Test {
+  def main(args: Array[String]): Unit = {
+    val foo = new Foo[Int]
+    foo.s
+  }
+}