diff --git a/compiler/src/dotty/tools/dotc/transform/Bridges.scala b/compiler/src/dotty/tools/dotc/transform/Bridges.scala new file mode 100644 index 000000000000..0667532797db --- /dev/null +++ b/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 + } +} \ No newline at end of file diff --git a/compiler/src/dotty/tools/dotc/transform/ElimErasedValueType.scala b/compiler/src/dotty/tools/dotc/transform/ElimErasedValueType.scala index 48be02fa167d..0fd942ea5568 100644 --- a/compiler/src/dotty/tools/dotc/transform/ElimErasedValueType.scala +++ b/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) diff --git a/compiler/src/dotty/tools/dotc/transform/Erasure.scala b/compiler/src/dotty/tools/dotc/transform/Erasure.scala index 245b153ba9a6..1d761f32acb0 100644 --- a/compiler/src/dotty/tools/dotc/transform/Erasure.scala +++ b/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) } diff --git a/compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala b/compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala index 1f1f371a6ec8..cbd79d5c50d6 100644 --- a/compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala +++ b/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) diff --git a/compiler/test/dotc/tests.scala b/compiler/test/dotc/tests.scala index 3c07df88a952..17b386cbff7a 100644 --- a/compiler/test/dotc/tests.scala +++ b/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") diff --git a/tests/untried/neg/valueclasses-doubledefs.scala b/tests/neg/customArgs/valueclasses-doubledefs.scala similarity index 62% rename from tests/untried/neg/valueclasses-doubledefs.scala rename to tests/neg/customArgs/valueclasses-doubledefs.scala index 87bcf8fee3bc..dc55c9221008 100644 --- a/tests/untried/neg/valueclasses-doubledefs.scala +++ b/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 } + diff --git a/tests/neg/customArgs/valueclasses-doubledefs2.scala b/tests/neg/customArgs/valueclasses-doubledefs2.scala new file mode 100644 index 000000000000..dcd7bdf8d444 --- /dev/null +++ b/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 diff --git a/tests/untried/neg/valueclasses-pavlov.scala b/tests/neg/customArgs/valueclasses-pavlov.scala similarity index 72% rename from tests/untried/neg/valueclasses-pavlov.scala rename to tests/neg/customArgs/valueclasses-pavlov.scala index d0c1ed59ba10..6eb9e8b7c72b 100644 --- a/tests/untried/neg/valueclasses-pavlov.scala +++ b/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 } } diff --git a/tests/neg/i1240a.scala b/tests/pending/neg/i1240a.scala similarity index 73% rename from tests/neg/i1240a.scala rename to tests/pending/neg/i1240a.scala index dc711454e0a6..31d0ed22f085 100644 --- a/tests/neg/i1240a.scala +++ b/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? } diff --git a/tests/pending/run/bridges.scala b/tests/run/bridges.scala similarity index 100% rename from tests/pending/run/bridges.scala rename to tests/run/bridges.scala diff --git a/tests/run/i2337.scala b/tests/run/i2337.scala new file mode 100644 index 000000000000..9a7fd13f1aaf --- /dev/null +++ b/tests/run/i2337.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 + } +} diff --git a/tests/run/i2337b.scala b/tests/run/i2337b.scala new file mode 100644 index 000000000000..2be84267aa16 --- /dev/null +++ b/tests/run/i2337b.scala @@ -0,0 +1,37 @@ +/* +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] + def bar = coll +} + +class Foo[T] extends Seq[T] with Super2[T] { +} + +object Test { + def main(args: Array[String]): Unit = { + val foo = new Foo[Int] + foo.s + val su2: Super2[Int] = foo + su2.bar + } +} diff --git a/tests/untried/neg/valueclasses-doubledefs.check b/tests/untried/neg/valueclasses-doubledefs.check deleted file mode 100644 index ec513aca6b9e..000000000000 --- a/tests/untried/neg/valueclasses-doubledefs.check +++ /dev/null @@ -1,7 +0,0 @@ -valueclasses-doubledefs.scala:5: error: double definition: -def apply(x: Double): String at line 4 and -def apply(x: Meter): String at line 5 -have same type after erasure: (x: Double)String - def apply(x: Meter) = x.toString - ^ -one error found diff --git a/tests/untried/neg/valueclasses-pavlov.check b/tests/untried/neg/valueclasses-pavlov.check deleted file mode 100644 index 17102a0c68d8..000000000000 --- a/tests/untried/neg/valueclasses-pavlov.check +++ /dev/null @@ -1,7 +0,0 @@ -valueclasses-pavlov.scala:8: error: double definition: -def foo(x: String): String at line 7 and -def foo(x: Box2): String at line 8 -have same type after erasure: (x: String)String - def foo(x: Box2) = "foo(Box2): ok" - ^ -one error found