Skip to content

Refine hasCommonParent criterion in OverridingPairs #11741

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

Merged
merged 1 commit into from
Mar 17, 2021
Merged
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
5 changes: 5 additions & 0 deletions compiler/src/dotty/tools/dotc/transform/Bridges.scala
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ class Bridges(root: ClassSymbol, thisPhase: DenotTransformer)(using Context) {

private class BridgesCursor(using Context) extends OverridingPairs.Cursor(root) {

override def isSubParent(parent: Symbol, bc: Symbol)(using Context) =
true
// Never consider a bridge if there is a superclass that would contain it
// See run/t2857.scala for a test that would break with a VerifyError otherwise.

/** 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
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
package dotty.tools.dotc
package dotty.tools
package dotc
package transform

import ast.{Trees, tpd}
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Need to compute suparents before erasure to not filter out parents
// Need to compute subparents before erasure to not filter out parents

// that are bypassed with different types. See neg/11719a.scala.
atPhase(elimRepeatedPhase.next) { super.isSubParent(parent, bc) }
override def exclude(sym: Symbol) =
!sym.is(Method) || sym.is(Bridge) || super.exclude(sym)
override def matches(sym1: Symbol, sym2: Symbol) =
Expand Down
16 changes: 15 additions & 1 deletion compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,25 @@ object OverridingPairs {
decls
}

/** Is `parent` a qualified sub-parent of `bc`?
* @pre `parent` is a parent class of `base` and it derives from `bc`.
* @return true if the `bc`-basetype of the parent's type is the same as
* the `bc`-basetype of base. In that case, overriding checks
* relative to `parent` already subsume overriding checks
* relative to `base`. See neg/11719a.scala for where this makes
* a difference.
*/
protected def isSubParent(parent: Symbol, bc: Symbol)(using Context) =
bc.typeParams.isEmpty
|| self.baseType(parent).baseType(bc) == self.baseType(bc)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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)))


private val subParents = MutableSymbolMap[BitSet]()

for bc <- base.info.baseClasses do
var bits = BitSet.empty
for i <- 0 until parents.length do
if parents(i).derivesFrom(bc) then bits += i
if parents(i).derivesFrom(bc) && isSubParent(parents(i), bc)
then bits += i
subParents(bc) = bits

private def hasCommonParentAsSubclass(cls1: Symbol, cls2: Symbol): Boolean =
Expand Down
2 changes: 1 addition & 1 deletion tests/neg/i11130.scala
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
trait PBar extends S[Bar] {
override type T = Bar
}
class PFooBar extends PBar with PFoo {
class PFooBar extends PBar with PFoo { // error
override type T >: Bar // error
}

Expand Down
12 changes: 12 additions & 0 deletions tests/neg/i11719.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
class Base
class Sub extends Base

trait A[+T] {
def get: T = ???
}

trait B extends A[Base] {
override def get: Base = new Base
}

class C extends B with A[Sub] // error: method get in trait B is not a legal implementation
19 changes: 19 additions & 0 deletions tests/neg/i11719a.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
class Base
class Sub extends Base

trait A[+T] {
def get(f: T => Boolean): Unit = {}
}

trait B extends A[Base] {
override def get(f: Base => Boolean): Unit = f(new Base)
}

class C extends B with A[Sub] // error: Name clash between inherited members:

object Test {
def main(args: Array[String]): Unit = {
val c = new C
c.get((x: Sub) => true) // ClassCastException: Base cannot be cast to Sub
}
}