Skip to content

Commit

Permalink
Optimization in symbol pairs a bit less wrong again
Browse files Browse the repository at this point in the history
The idea is that we can skip a pair of matching symbols
if we're sure to see them again in the same linearisation order.
However, if our non-trait parent is also the owner of the lower
member, this is the last time we'll see this pair -- so don't skip!

The logic originally was written under the assumption that we're
considering a pair strictly higher up in the linearisation order.

Since this cursor is used in three different settings, pull out
the owner-skipping logic to the different Cursor subclasses.
  • Loading branch information
adriaanm committed Mar 29, 2019
1 parent 59975bb commit fde215f
Show file tree
Hide file tree
Showing 9 changed files with 180 additions and 23 deletions.
14 changes: 4 additions & 10 deletions src/compiler/scala/tools/nsc/transform/Erasure.scala
Expand Up @@ -462,20 +462,11 @@ abstract class Erasure extends InfoTransform
override def newTyper(context: Context) = new Eraser(context)

class EnterBridges(unit: CompilationUnit, root: Symbol) {

class BridgesCursor(root: Symbol) extends overridingPairs.Cursor(root) {
// Varargs bridges may need generic bridges due to the non-repeated part of the signature of the involved methods.
// The vararg bridge is generated during refchecks (probably to simplify override checking),
// but then the resulting varargs "bridge" method may itself need an actual erasure bridge.
// TODO: like javac, generate just one bridge method that wraps Seq <-> varargs and does erasure-induced casts
override def exclude(sym: Symbol) = !sym.isMethod || super.exclude(sym)
}

val site = root.thisType
val bridgesScope = newScope
val bridgeTarget = mutable.HashMap[Symbol, Symbol]()

val opc = enteringExplicitOuter { new BridgesCursor(root) }
val opc = enteringExplicitOuter { new overridingPairs.BridgesCursor(root) }

def computeAndEnter(): Unit = {
while (opc.hasNext) {
Expand Down Expand Up @@ -915,6 +906,9 @@ abstract class Erasure extends InfoTransform
|| !sym.hasTypeAt(currentRun.refchecksPhase.id)
)
override def matches(high: Symbol) = !high.isPrivate

// consider all matching pairs
protected def skipOwnerPair(lowClass: Symbol, highClass: Symbol): Boolean = false
}

/** Emit an error if there is a double definition. This can happen if:
Expand Down
22 changes: 21 additions & 1 deletion src/compiler/scala/tools/nsc/transform/OverridingPairs.scala
Expand Up @@ -24,7 +24,7 @@ import scala.reflect.internal.SymbolPairs
abstract class OverridingPairs extends SymbolPairs {
import global._

class Cursor(base: Symbol) extends super.Cursor(base) {
abstract class Cursor(base: Symbol) extends super.Cursor(base) {
/** Symbols to exclude: Here these are constructors and private/artifact symbols,
* including bridges. But it may be refined in subclasses.
*/
Expand All @@ -45,4 +45,24 @@ abstract class OverridingPairs extends SymbolPairs {
&& (lowMemberType matches (self memberType high))
) // TODO we don't call exclude(high), should we?
}

final class RefchecksCursor(base: Symbol) extends Cursor(base) {
// Skip if we know this pair is checked when root == nonTraitParent
// (since it's not a trait, we know the linearisation will be the same and it's safe to delay our checks).
protected def skipOwnerPair(lowClass: Symbol, highClass: Symbol): Boolean =
lowClass != nonTraitParent && nonTraitParent.isNonBottomSubClass(lowClass) && nonTraitParent.isNonBottomSubClass(highClass)
}

final class BridgesCursor(base: Symbol) extends Cursor(base) {
// Varargs bridges may need generic bridges due to the non-repeated part of the signature of the involved methods.
// The vararg bridge is generated during refchecks (probably to simplify override checking),
// but then the resulting varargs "bridge" method may itself need an actual erasure bridge.
// TODO: like javac, generate just one bridge method that wraps Seq <-> varargs and does erasure-induced casts
override def exclude(sym: Symbol) = !sym.isMethod || super.exclude(sym)

// Skip if the (non-trait) class in `parents` is a subclass of the owners of both low and high.
// Correctness of bridge generation relies on visiting each such class only once.
override def skipOwnerPair(lowClass: Symbol, highClass: Symbol): Boolean =
nonTraitParent.isNonBottomSubClass(lowClass) && nonTraitParent.isNonBottomSubClass(highClass)
}
}
2 changes: 1 addition & 1 deletion src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
Expand Up @@ -542,7 +542,7 @@ abstract class RefChecks extends Transform {
}
}

val opc = new overridingPairs.Cursor(clazz)
val opc = new overridingPairs.RefchecksCursor(clazz)
while (opc.hasNext) {
if (!opc.high.isClass)
checkOverride(opc.currentPair)
Expand Down
Expand Up @@ -707,7 +707,9 @@ trait TypeDiagnostics {
if (settings.warnUnusedParams) {
def isImplementation(m: Symbol): Boolean = {
def classOf(s: Symbol): Symbol = if (s.isClass || s == NoSymbol) s else classOf(s.owner)
val opc = new overridingPairs.Cursor(classOf(m))
val opc = new overridingPairs.Cursor(classOf(m)) {
override protected def skipOwnerPair(lowClass: Symbol, highClass: Symbol): Boolean = false
}
opc.iterator.exists(pair => pair.low == m)
}
import PartialFunction._
Expand Down
21 changes: 12 additions & 9 deletions src/reflect/scala/reflect/internal/SymbolPairs.scala
Expand Up @@ -106,6 +106,14 @@ abstract class SymbolPairs {
*/
protected def matches(high: Symbol): Boolean

/** Even if a pair `matches`, should the cursor skip this pair?
*
* @param lowClass owner of the next low symbol
* @param highClass owner of the next hi symbol
* @return whether to skip this pair
*/
protected def skipOwnerPair(lowClass: Symbol, highClass: Symbol): Boolean

protected def bases: List[Symbol] = base.info.baseClasses

/** The scope entries that have already been visited as highSymbol
Expand Down Expand Up @@ -162,7 +170,7 @@ abstract class SymbolPairs {
}

// We can only draw conclusions about linearisation from a non-trait parent; skip Object, being the top of the lattice.
private lazy val nonTraitParent: Symbol =
protected lazy val nonTraitParent: Symbol =
base.info.firstParent.typeSymbol.filter(sym => !sym.isTrait && sym != definitions.ObjectClass)

@tailrec private def advanceNextEntry(): Unit = {
Expand All @@ -172,14 +180,9 @@ abstract class SymbolPairs {
val high = nextEntry.sym
val isMatch = matches(high) && { visited addEntry nextEntry ; true } // side-effect visited on all matches

// Skip nextEntry if the (non-trait) class in `parents` is a subclass of the owners of both low and high.
// this means we'll come back to this entry when we consider `nonTraitParent`, and the linearisation order will be the same
// (this only works for non-trait classes, since subclassing on traits does not imply one's linearisation is contained in the other's)
// This is not just an optimization -- bridge generation relies on visiting each such class only once.
if (!isMatch || (nonTraitParent.isNonBottomSubClass(low.owner) && nonTraitParent.isNonBottomSubClass(high.owner)))
advanceNextEntry()
else
highSymbol = high
// Advance if no match, or if the particular cursor is not intested in this pair
if (!isMatch || skipOwnerPair(low.owner, high.owner)) advanceNextEntry()
else highSymbol = high
}
}
}
Expand Down
8 changes: 8 additions & 0 deletions test/files/neg/t11136_override_conflict.check
@@ -0,0 +1,8 @@
t11136_override_conflict.scala:99: error: incompatible type in overriding
def empty: Qu[A] (defined in trait ArDqOps)
with def empty: ArDq[A] (defined in class ArDq);
found : => ArDq[A]
required: => Qu[A]
class Qu[A] extends ArDq[A]
^
one error found
122 changes: 122 additions & 0 deletions test/files/neg/t11136_override_conflict.scala
@@ -0,0 +1,122 @@

import annotation.unchecked.uncheckedVariance

trait Bldr[-A, +To] { self =>
def result(): To
def mapResult[NTo](f: To => NTo): Bldr[A, NTo] = new Bldr[A, NTo] {
def result(): NTo = f(self.result())
}
}
trait ItFact[+CC[_]] {
def from[A](source: It[A]): CC[A]
def newBuilder[A]: Bldr[A, CC[A]]
}

trait DefaultFromSpecific[+A, +CC[_], +C] {
protected def fromSpecific(coll: It[A @uncheckedVariance]): CC[A @uncheckedVariance] = iterableFactory.from(coll)
protected def newSpecificBuilder: Bldr[A @uncheckedVariance, CC[A @uncheckedVariance]] = iterableFactory.newBuilder[A]
def iterableFactory: ItFact[CC]
}

trait ItOps[+A, +CC[_], +C] {
def it: It[A]

protected def newSpecificBuilder: Bldr[A @uncheckedVariance, C]
protected def fromSpecific(coll: It[A @uncheckedVariance]): C
def iterableFactory: ItFact[CC]

def filter: C = fromSpecific(it)
def strictFilter: C = newSpecificBuilder.result()
def map[B](f: A => B): CC[B] = iterableFactory.newBuilder.result()
}

trait It[+A] extends ItOps[A, It, It[A]] with DefaultFromSpecific[A, It, It[A]] {
def it: It[A] = this
def iterableFactory: ItFact[It] = It
}

object It extends ItFact[It] {
def from[A](source: It[A]): It[A] = new It[A]{}
def newBuilder[A]: Bldr[A,It[A]] = new Bldr[A, It[A]] { def result(): It[A] = new It[A]{} }
}

trait SqOps[A, +CC[_], +C] extends ItOps[A, CC, C]

trait Sq[A] extends It[A] with SqOps[A, Sq, Sq[A]] with DefaultFromSpecific[A, Sq, Sq[A]] {
override def iterableFactory: ItFact[Sq] = Sq

def flup = 0
}

object Sq extends ItFact[Sq] {
def from[A](source: It[A]): Sq[A] = new Sq[A]{}
def newBuilder[A]: Bldr[A, Sq[A]] = new Bldr[A, Sq[A]] { def result(): Sq[A] = new Sq[A]{} }
}

trait Acc[A, +CC[X] <: Sq[X], +C <: Sq[A]] extends Sq[A] with SqOps[A, CC, C] {
protected def fromSpecificImpl(coll: It[A]): C
protected def newSpecificBuilderImpl: Bldr[A, C]
protected def iterableFactoryImpl: ItFact[CC]

protected override def fromSpecific(coll: It[A]): C = fromSpecificImpl(coll)
protected override def newSpecificBuilder: Bldr[A, C] = newSpecificBuilderImpl
override def iterableFactory: ItFact[CC] = iterableFactoryImpl
}
trait AnyAcc[A] extends Acc[A, AnyAcc, AnyAcc[A]] {
protected override def fromSpecificImpl(coll: It[A]): AnyAcc[A] = iterableFactory.from(coll)
protected override def newSpecificBuilderImpl: Bldr[A, AnyAcc[A]] = iterableFactory.newBuilder
override def iterableFactoryImpl: ItFact[AnyAcc] = AnyAcc

def flap = 1
}
object AnyAcc extends ItFact[AnyAcc] {
def from[A](source: It[A]): AnyAcc[A] = new AnyAcc[A] {}
def newBuilder[A]: Bldr[A, AnyAcc[A]] = new Bldr[A, AnyAcc[A]] { def result(): AnyAcc[A] = new AnyAcc[A]{} }
}
trait IntAcc extends Acc[Int, AnyAcc, IntAcc] {
protected override def fromSpecificImpl(coll: It[Int]): IntAcc = new IntAcc{}
protected override def newSpecificBuilderImpl: Bldr[Int, IntAcc] = new Bldr[Int, IntAcc] { def result(): IntAcc = new IntAcc{} }
override def iterableFactoryImpl: ItFact[AnyAcc] = AnyAcc
}

class ArDq[A] extends Sq[A]
with SqOps[A, ArDq, ArDq[A]]
with ArDqOps[A, ArDq, ArDq[A]]
with DefaultFromSpecific[A, ArDq, ArDq[A]] {
override def iterableFactory: ItFact[ArDq] = ArDq
def empty: ArDq[A] = new ArDq[A]{}
}

object ArDq extends ItFact[ArDq] {
override def from[A](source: It[A]): ArDq[A] = new ArDq[A]{}
override def newBuilder[A]: Bldr[A, ArDq[A]] = new Bldr[A, ArDq[A]] { def result(): ArDq[A] = new ArDq[A]{} }
}

trait ArDqOps[A, +CC[_], +C <: AnyRef] extends SqOps[A, CC, C] {
def empty: C
}

class Qu[A] extends ArDq[A]
with SqOps[A, Qu, Qu[A]]
with ArDqOps[A, Qu, Qu[A]]
with DefaultFromSpecific[A, Qu, Qu[A]] {
override def iterableFactory: ItFact[Qu] = Qu
// should get a missing override error, but don't. if `ArDq` and `Qu` are both traits, the error shows.
// override def empty: Qu[A] = new Qu[A]{}
def bazza = 2
}

object Qu extends ItFact[Qu] {
override def from[A](source: It[A]): Qu[A] = new Qu[A]{}
override def newBuilder[A]: Bldr[A, Qu[A]] = new Bldr[A, Qu[A]] { def result(): Qu[A] = new Qu[A]{} }
}

object Test {
def main(args: Array[String]): Unit = {
val sq = new Sq[String] { }
println(sq.filter.flup)
val ia = new IntAcc{}
println(((ia.filter: IntAcc).map(_ => ""): AnyAcc[String]).flap)
(new Qu[String]{}: ArDqOps[String, Qu, Qu[String]]).empty.bazza
}
}
8 changes: 8 additions & 0 deletions test/files/run/extend-global.scala
@@ -0,0 +1,8 @@
import scala.tools.nsc._

// extending Global is a pretty thorough test case of bridge generation
// make sure we don't cause VerifyErrors when messing with symbol pairs
object Test extends Global(new Settings) {
def main(args: Array[String]): Unit = {
}
}
Expand Up @@ -25,7 +25,7 @@ class OverridingPairsTest extends BytecodeTesting {
val g = compiler.global
val C = g.rootMirror.getRequiredClass("p1.L")

val opcs = new g.overridingPairs.Cursor(C).iterator.filter(_.low.nameString == "c").map(c => (c.lowString, c.highString)).toList
val opcs = new g.overridingPairs.RefchecksCursor(C).iterator.filter(_.low.nameString == "c").map(c => (c.lowString, c.highString)).toList

assertEquals(List(("override def c(x: Int): Int in trait SOIO", "final override def c(x: Int): Int in trait SO"),
("override def c(x: Int): Int in trait SOIO", "def c(x: Int): Int in trait IO")), opcs)
Expand Down

0 comments on commit fde215f

Please sign in to comment.