Skip to content

Fix #2072: erasure and refchecks #2085

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 38 additions & 12 deletions compiler/src/dotty/tools/dotc/transform/Erasure.scala
Original file line number Diff line number Diff line change
Expand Up @@ -594,14 +594,19 @@ object Erasure extends TypeTestsCasts{

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
if (ctx.owner.isClass) {
val newBridges = addBridges(stats, stats1)(ctx)
val keptStats = stats1.filter(x => !newBridges.exists(_.symbol == x.symbol))
keptStats ::: newBridges.filter(!_.isEmpty)
}
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] = {
emittedBridges: ListBuffer[tpd.Tree] = ListBuffer[tpd.Tree]()): List[tpd.Tree] = {
after match {
case Nil => emittedBridges.toList
case (member: DefDef) :: newTail =>
Expand Down Expand Up @@ -629,7 +634,7 @@ object Erasure extends TypeTestsCasts{
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)
emittedBridges.find(stat => stat.isDef && (stat.asInstanceOf[DefDef].name == bridge.name) && stat.tpe.widen =:= bridge.info.widen)
.map(_.symbol))
clash match {
case Some(cl) =>
Expand All @@ -642,7 +647,22 @@ object Erasure extends TypeTestsCasts{
}

val bridgeImplementations = minimalSet.map {
sym => makeBridgeDef(member, sym)(ctx)
sym =>
if (member.symbol.is(Flags.Deferred)) {
if (!sym.is(Flags.Deferred)) {
// sometimes we need to forward an abstract method in a subclass to a non-abstract method in parent.
// see https://github.com/lampepfl/dotty/issues/2072 for illustrations

val d = member.symbol.copySymDenotation(initFlags = member.symbol.flags.&~(Flags.Deferred))
d.installAfter(ctx.erasurePhase.asInstanceOf[Erasure])
// Dark magic. Don't look here.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a helpful comment. Someone might need to look at this eventually and will be very confused. Also do you need both installAfter and transformAfter ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also do you need both installAfter and transformAfter ?

Yes. The comment here would entirely repeat all the history of transformAfter. I don't think it's worth simplifying it here. To understand why it's needed here one need to read the entire description of 979ee0f which introduces transformAfter

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then the comment can say "See the commit message of 979ee0f for more information."

d.transformAfter(ctx.erasurePhase.asInstanceOf[Erasure],
x => if (x.is(Flags.Deferred)) x.copySymDenotation(initFlags = x.flags.&~(Flags.Deferred)) else x
)
makeBridgeDef(newSymbol.owner, sym, newSymbol)(ctx)
}
else EmptyTree
} else makeBridgeDef(newSymbol.owner, newSymbol, sym)(ctx)
}
emittedBridges ++= bridgeImplementations
} catch {
Expand All @@ -665,28 +685,34 @@ object Erasure extends TypeTestsCasts{

/** Create a bridge DefDef which overrides a parent method.
*
* @param newDef The DefDef which needs bridging because its signature
* @param newDefSym 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 makeBridgeDef(currentClass: Symbol, newDefSym: Symbol, parentSym: Symbol)(implicit ctx: Context): tpd.DefDef = {

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
val bridge =
if (newDefSym.owner == currentClass) {
// we are forwarding a method in a superclass to a implementation in a subclass
val s = ctx.newSymbol(currentClass,
parentSym.name, parentSym.flags &~ excluded | Flags.Bridge, parentSym.info, coord = newDefSym.owner.coord).asTerm
s.enteredAfter(ctx.phase.prev.asInstanceOf[DenotTransformer]) // this should be safe, as we're executing in context of next phase
s
} else {
// we are forwarding a method defined in a subclass to an implementation in a superclass
parentSym.asTerm
}
ctx.debuglog(s"generating bridge from ${newDefSym} to $bridge")

val sel: Tree = This(currentClass).select(newDefSym.termRef)
val sel: Tree = This(currentClass.asClass).select(newDefSym.termRef)

val resultType = parentSym.info.widen.resultType

Expand Down
17 changes: 12 additions & 5 deletions compiler/src/dotty/tools/dotc/transform/TypeTestsCasts.scala
Original file line number Diff line number Diff line change
Expand Up @@ -96,23 +96,30 @@ trait TypeTestsCasts {
/** Transform isInstanceOf OrType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not specific to OrType anymore

*
* expr.isInstanceOf[A | B] ~~> expr.isInstanceOf[A] | expr.isInstanceOf[B]
* expr.isInstanceOf[A & B] ~~> expr.isInstanceOf[A] & expr.isInstanceOf[B]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind moving this commit to a separate PR? It's not related to fixing #2072 and I'd like to get it in soon because #2082 depends on it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An identical commit has now been merged as part of #2082

*
* The transform happens before erasure of `argType`, thus cannot be merged
* with `transformIsInstanceOf`, which depends on erased type of `argType`.
*/
def transformOrTypeTest(qual: Tree, argType: Type): Tree = argType.dealias match {
def transformTypeTest(qual: Tree, argType: Type): Tree = argType.dealias match {
case OrType(tp1, tp2) =>
evalOnce(qual) { fun =>
transformOrTypeTest(fun, tp1)
.select(nme.OR)
.appliedTo(transformOrTypeTest(fun, tp2))
transformTypeTest(fun, tp1)
.select(defn.Boolean_||)
.appliedTo(transformTypeTest(fun, tp2))
}
case AndType(tp1, tp2) =>
evalOnce(qual) { fun =>
transformTypeTest(fun, tp1)
.select(defn.Boolean_&&)
.appliedTo(transformTypeTest(fun, tp2))
}
case _ =>
transformIsInstanceOf(qual, erasure(argType))
}

if (sym eq defn.Any_isInstanceOf)
transformOrTypeTest(qual, tree.args.head.tpe)
transformTypeTest(qual, tree.args.head.tpe)
else if (sym eq defn.Any_asInstanceOf)
transformAsInstanceOf(erasure(tree.args.head.tpe))
else tree
Expand Down
3 changes: 2 additions & 1 deletion compiler/src/dotty/tools/dotc/typer/RefChecks.scala
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,8 @@ object RefChecks {
def ignoreDeferred(member: SingleDenotation) =
member.isType ||
member.symbol.is(SuperAccessor) || // not yet synthesized
member.symbol.is(JavaDefined) && hasJavaErasedOverriding(member.symbol)
member.symbol.is(JavaDefined) && hasJavaErasedOverriding(member.symbol) ||
member.symbol.allOverriddenSymbols.exists(!_.is(Flags.Deferred)) // has an implementation in super

// 2. Check that only abstract classes have deferred members
def checkNoAbstractMembers(): Unit = {
Expand Down
2 changes: 2 additions & 0 deletions tests/pos/i2073.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
final case class Fix[F[_]](unfix: F[Fix[F]]) extends AnyVal

28 changes: 28 additions & 0 deletions tests/run/i2072.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
trait T {
def m = "" ;
val v = "" ;
lazy val lv = "";
object o
}

// deferred methods always lose against concrete ones, so none of these declarations actually survive in bytecode
// the mixed in members from T take precedence in the linearisation order
abstract class AC extends T {
def m: Any
def v: Any
def lv: Any
def o: Any
}

class C extends AC{}

object Test {
def main(args: Array[String]): Unit = {
val c = new C
c.m
c.v
c.lv
c.o
}
}

28 changes: 28 additions & 0 deletions tests/run/i2072b.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
trait T {
def m = "" ;
val v = "" ;
lazy val lv = "";
object o;
}

// deferred methods always lose against concrete ones, so none of these declarations actually survive in bytecode
// the mixed in members from T take precedence in the linearisation order
abstract class AC{
def m: Any
def v: Any
def lv: Any
def o: Any
}

class C extends AC with T{}

object Test {
def main(args: Array[String]): Unit = {
val c = new C
c.m
c.v
c.lv
c.o
}
}

41 changes: 41 additions & 0 deletions tests/run/i2072c.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
trait T { lazy val x: String = "foo" }
trait U { def x: Any }
class AC extends T with U // { def x: Any }

abstract class B { def x: Any }
class C extends B { def x: String = "abc" }

package p2 {
trait T1 { def f: Any }
trait T2 extends T1 { def f: Number = ??? }
trait T3 extends T1 { override def f: Integer = ??? }
class C extends T2 with T3
}

package p3 {

trait A { def f: Any }
trait B extends A { def f: String }
class C extends B { def f = "abc" }

}

object Test {
def main(args: Array[String]): Unit = {
val ac = new AC
ac.x
(ac: T).x
(ac: U).x
(new C).x
((new C): B).x
val p2c = new p2.C
p2c.f
(p2c: p2.T1).f
(p2c: p2.T2).f
(p2c: p2.T3).f
val p3c = new p3.C
p3c.f
(p3c: p3.A).f
(p3c: p3.B).f
}
}