Refine hasCommonParent criterion in OverridingPairs
#11741
Conversation
Fixes scala#11719 `hasCommonParent` is used to avoid re-visiting overriding pairs that were already accounted for in a parent. But that only works if the base types of the classes forming an overriding pair are the same for the parent and the class itself. Otherwise we might miss an overriding relationship.
| @@ -80,6 +81,10 @@ class ElimErasedValueType extends MiniPhase with InfoTransformer { thisPhase => | |||
| private def checkNoClashes(root: Symbol)(using Context) = { | |||
| val opc = atPhase(thisPhase) { | |||
| new OverridingPairs.Cursor(root) { | |||
| override def isSubParent(parent: Symbol, bc: Symbol)(using Context) = | |||
| // Need to compute suparents before erasure to not filter out parents | |||
There was a problem hiding this comment.
| // Need to compute suparents before erasure to not filter out parents | |
| // Need to compute subparents before erasure to not filter out parents |
| */ | ||
| protected def isSubParent(parent: Symbol, bc: Symbol)(using Context) = | ||
| bc.typeParams.isEmpty | ||
| || self.baseType(parent).baseType(bc) == self.baseType(bc) |
There was a problem hiding this comment.
While this fixes the immediate problem, it's worth noting that Scala 2 completely removed the subparent logic in scala/scala@cc55bd9 because:
subclassing on traits does not imply one's linearisation is contained in the other's
The testcase from that commit is:
trait IO {
def c(x: Int): Int = ???
}
trait SO extends IO {
override final def c(x: Int): Int = ???
}
trait SOIO extends IO {
override def c(x: Int): Int = ???
}
trait SOSO extends SOIO with SO
abstract class AS extends SO
class L extends AS with SOSO // error expected: c definined in SOIO overrides final method c in SO Which currently compiles in Dotty, even though we end up overriding a final def with a non-final def. We already have an issue open to keep track of this (#7551) so we don't necessarily need to handle this now.
There was a problem hiding this comment.
OK. We should evaluate the performance implications of subParents before removing them. Also bridge generation currently relies on the check being present, otherwise you would get duplicate bridges.
There was a problem hiding this comment.
Also bridge generation currently relies on the check being present, otherwise you would get duplicate bridges.
Indeed, but Scala 2 had the same issue so scala/scala@cc55bd9 also takes care of that:
// 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)))
Fixes #11719
hasCommonParentis used to avoid re-visiting overriding pairs that werealready accounted for in a parent. But that only works if the base types
of the classes forming an overriding pair are the same for the parent and
the class itself. Otherwise we might miss an overriding relationship.