Merge pull request #13092 from dotty-staging/fix-13087

Refine "can be handled by parent" condition when generting bridges

Author: odersky

compiler/src/dotty/tools/dotc/transform/Bridges.scala

@@ -34,6 +34,9 @@ class Bridges(root: ClassSymbol, thisPhase: DenotTransformer)(using Context) {
 
     override def exclude(sym: Symbol) =
       !sym.isOneOf(MethodOrModule) || super.exclude(sym)
+
+    override def canBeHandledByParent(sym1: Symbol, sym2: Symbol, parent: Symbol): Boolean =
+      OverridingPairs.isOverridingPair(sym1, sym2, parent.thisType)
   }
 
   val site = root.thisType

compiler/src/dotty/tools/dotc/transform/OverridingPairs.scala

@@ -4,6 +4,7 @@ package transform
 
 import core._
 import Flags._, Symbols._, Contexts._, Scopes._, Decorators._, Types.Type
+import NameKinds.DefaultGetterName
 import collection.mutable
 import collection.immutable.BitSet
 import scala.annotation.tailrec
@@ -16,12 +17,12 @@ import scala.annotation.tailrec
  *  Adapted from the 2.9 version of OverridingPairs. The 2.10 version is IMO
  *  way too unwieldy to be maintained.
  */
-object OverridingPairs {
+object OverridingPairs:
 
   /** The cursor class
    *  @param base   the base class that contains the overriding pairs
    */
-  class Cursor(base: Symbol)(using Context) {
+  class Cursor(base: Symbol)(using Context):
 
     private val self = base.thisType
 
@@ -165,5 +166,64 @@ object OverridingPairs {
 
     nextOverriding()
     next()
-  }
-}
+  end Cursor
+
+  /** Is this `sym1` considered an override of `sym2` (or vice versa) if both are
+   *  seen as members of `site`?
+   *  We declare a match if either we have a full match including matching names
+   *  or we have a loose match with different target name but the types are the same.
+   *  We leave out pairs of methods in Java classes under the assumption since these
+   *  have already been checked and handled by javac.
+   *  This leaves two possible sorts of discrepancies to be reported as errors
+   *  in `RefChecks`:
+   *
+   *    - matching names, target names, and signatures but different types
+   *    - matching names and types, but different target names
+   *
+   *  This method is used as a replacement of `matches` in some subclasses of
+   *  OverridingPairs.
+   */
+  def isOverridingPair(sym1: Symbol, sym2: Symbol, self: Type)(using Context): Boolean =
+    if     sym1.owner.is(JavaDefined, butNot = Trait)
+        && sym2.owner.is(JavaDefined, butNot = Trait)
+    then false // javac already handles these checks and inserts bridges
+    else if sym1.isType then true
+    else
+      val sd1 = sym1.asSeenFrom(self)
+      val sd2 = sym2.asSeenFrom(self)
+      sd1.matchesLoosely(sd2)
+        && (sym1.hasTargetName(sym2.targetName)
+            || isOverridingPair(sym1, sd1.info, sym2, sd2.info))
+
+  /** Let `member` and `other` be members of some common class C with types
+   *  `memberTp` and `otherTp` in C. Are the two symbols considered an overriding
+   *  pair in C? We assume that names already match so we test only the types here.
+   *  @param fallBack  A function called if the initial test is false and
+   *                   `member` and `other` are term symbols.
+   */
+  def isOverridingPair(member: Symbol, memberTp: Type, other: Symbol, otherTp: Type, fallBack: => Boolean = false)(using Context): Boolean =
+    if member.isType then // intersection of bounds to refined types must be nonempty
+      memberTp.bounds.hi.hasSameKindAs(otherTp.bounds.hi)
+      && (
+        (memberTp frozen_<:< otherTp)
+        || !member.owner.derivesFrom(other.owner)
+            && {
+              // if member and other come from independent classes or traits, their
+              // bounds must have non-empty-intersection
+              val jointBounds = (memberTp.bounds & otherTp.bounds).bounds
+              jointBounds.lo frozen_<:< jointBounds.hi
+            }
+      )
+    else
+      // releaxed override check for explicit nulls if one of the symbols is Java defined,
+      // force `Null` being a subtype of reference types during override checking
+      val relaxedCtxForNulls =
+        if ctx.explicitNulls && (member.is(JavaDefined) || other.is(JavaDefined)) then
+          ctx.retractMode(Mode.SafeNulls)
+        else ctx
+      member.name.is(DefaultGetterName) // default getters are not checked for compatibility
+      || memberTp.overrides(otherTp,
+            member.matchNullaryLoosely || other.matchNullaryLoosely || fallBack
+          )(using relaxedCtxForNulls)
+
+end OverridingPairs

compiler/src/dotty/tools/dotc/typer/RefChecks.scala

@@ -17,6 +17,7 @@ import config.Printers.{checks, noPrinter}
 import scala.util.{Try, Failure, Success}
 import config.{ScalaVersion, NoScalaVersion}
 import Decorators._
+import OverridingPairs.isOverridingPair
 import typer.ErrorReporting._
 import config.Feature.{warnOnMigration, migrateTo3}
 import config.Printers.refcheck
@@ -264,31 +265,6 @@ object RefChecks {
       i"${if (showLocation) sym1.showLocated else sym1}$infoStr"
     }
 
-    def compatibleTypes(member: Symbol, memberTp: Type, other: Symbol, otherTp: Type, fallBack: => Boolean = false): Boolean =
-      try
-        if (member.isType) // intersection of bounds to refined types must be nonempty
-          memberTp.bounds.hi.hasSameKindAs(otherTp.bounds.hi) &&
-          ((memberTp frozen_<:< otherTp) ||
-            !member.owner.derivesFrom(other.owner) && {
-              // if member and other come from independent classes or traits, their
-              // bounds must have non-empty-intersection
-              val jointBounds = (memberTp.bounds & otherTp.bounds).bounds
-              jointBounds.lo frozen_<:< jointBounds.hi
-            })
-        else
-          // releaxed override check for explicit nulls if one of the symbols is Java defined,
-          // force `Null` being a subtype of reference types during override checking
-          val relaxedCtxForNulls =
-            if ctx.explicitNulls && (member.is(JavaDefined) || other.is(JavaDefined)) then
-              ctx.retractMode(Mode.SafeNulls)
-            else ctx
-          member.name.is(DefaultGetterName) // default getters are not checked for compatibility
-          || memberTp.overrides(otherTp, member.matchNullaryLoosely || other.matchNullaryLoosely || fallBack)(using relaxedCtxForNulls)
-      catch case ex: MissingType =>
-        // can happen when called with upwardsSelf as qualifier of memberTp and otherTp,
-        // because in that case we might access types that are not members of the qualifier.
-        false
-
     /* Check that all conditions for overriding `other` by `member`
        * of class `clazz` are met.
        */
@@ -318,10 +294,15 @@ object RefChecks {
       }
 
       def compatTypes(memberTp: Type, otherTp: Type): Boolean =
-        compatibleTypes(member, memberTp, other, otherTp,
-          fallBack = warnOnMigration(
-            overrideErrorMsg("no longer has compatible type"),
-            (if (member.owner == clazz) member else clazz).srcPos))
+        try
+          isOverridingPair(member, memberTp, other, otherTp,
+            fallBack = warnOnMigration(
+              overrideErrorMsg("no longer has compatible type"),
+              (if (member.owner == clazz) member else clazz).srcPos))
+        catch case ex: MissingType =>
+          // can happen when called with upwardsSelf as qualifier of memberTp and otherTp,
+          // because in that case we might access types that are not members of the qualifier.
+          false
 
       /** Do types of term members `member` and `other` as seen from `self` match?
        *  If not we treat them as not a real override and don't issue override
@@ -488,29 +469,9 @@ object RefChecks {
           }*/
     }
 
-    /** We declare a match if either we have a full match including matching names
-     *  or we have a loose match with different target name but the types are the same.
-     *  This leaves two possible sorts of discrepancies to be reported as errors
-     *  in `checkOveride`:
-     *
-     *    - matching names, target names, and signatures but different types
-     *    - matching names and types, but different target names
-     */
-    def considerMatching(sym1: Symbol, sym2: Symbol, self: Type): Boolean =
-      if     sym1.owner.is(JavaDefined, butNot = Trait)
-          && sym2.owner.is(JavaDefined, butNot = Trait)
-      then false // javac already handles these checks
-      else if sym1.isType then true
-      else
-        val sd1 = sym1.asSeenFrom(self)
-        val sd2 = sym2.asSeenFrom(self)
-        sd1.matchesLoosely(sd2)
-          && (sym1.hasTargetName(sym2.targetName)
-             || compatibleTypes(sym1, sd1.info, sym2, sd2.info))
-
     val opc = new OverridingPairs.Cursor(clazz):
       override def matches(sym1: Symbol, sym2: Symbol): Boolean =
-        considerMatching(sym1, sym2, self)
+        isOverridingPair(sym1, sym2, self)
 
       private def inLinearizationOrder(sym1: Symbol, sym2: Symbol, parent: Symbol): Boolean =
         val owner1 = sym1.owner
@@ -530,7 +491,7 @@ object RefChecks {
       //   - They overriding/overridden appear in linearization order.
       //     See neg/i5094.scala for an example where this matters.
       override def canBeHandledByParent(sym1: Symbol, sym2: Symbol, parent: Symbol): Boolean =
-        considerMatching(sym1, sym2, parent.thisType)
+        isOverridingPair(sym1, sym2, parent.thisType)
          .showing(i"already handled ${sym1.showLocated}: ${sym1.asSeenFrom(parent.thisType).signature}, ${sym2.showLocated}: ${sym2.asSeenFrom(parent.thisType).signature} = $result", refcheck)
         && inLinearizationOrder(sym1, sym2, parent)
     end opc

tests/run/i1240.scala

@@ -19,7 +19,9 @@ object Test {
       // In Java, this gives an error like this:
       // methods foo(A) from C[D] and foo(String) from C[D] are inherited with the same signature
       // But the analogous example with `b1` compiles OK in Java.
-    assert(b2.foo(new D) == "T foo")
+    assert(b2.foo(new D) == "D foo")
+      // Here we get "D foo" since a bridge method for foo(x: D) was inserted
+      // in the anonymous class of b2.
   }
 }
 

tests/run/i13087.scala

@@ -0,0 +1,38 @@
+import scala.collection.mutable.Builder
+
+class DDD[S,T,A]
+
+trait NN[S, T, A, K[_], +D <: DDD[Set[S],T,K[A]]]
+class NNN[S, T, K[_], A] extends NN[S, T, A, K, DDD[Set[S],T,K[A]]]
+
+object NN {
+  def newBuilder[S, T, A, K[_]]:
+      NNbuilder[S, T, K, A, DDD[Set[S],T,K[A]], NN[S,T,A,K,?], Unit] =
+    new NNNbuilder[S, T, K, A]
+}
+
+// Remove the type parameter E, hardcoding in E := Unit, and the issue
+// goes away.
+trait NNbuilder
+  [S, T, K[_], A, +D <: DDD[Set[S],T,K[A]], +N <: NN[S,T,A,K,D], E]
+    extends Builder[Unit, N] {
+  def clear(): Unit = throw new UnsupportedOperationException()
+  final def addOne(builder: E): this.type = this
+}
+
+// Unfold this class defn, and the issue goes away
+abstract class AbstractNNNbuilder
+  [S, T, K[_], A, +D <: DDD[Set[S],T,K[A]], +N <: NN[S,T,A,K,D], E]
+    extends NNbuilder[S,T,K,A,D,N,E]
+
+class NNNbuilder[S, T, K[_], A] extends AbstractNNNbuilder[
+  S, T, K, A, DDD[Set[S], T, K[A]], NNN[S, T, K, A], Unit
+] {
+  override def result(): NNN[S, T, K, A] = new NNN[S, T, K, A]
+}
+
+@main def Test: Unit = {
+  val builder = NN.newBuilder[String, Char, Int, Set]
+  builder += ()
+  builder.result()
+}

tests/run/i13087a.scala

@@ -0,0 +1,17 @@
+trait SimpleTrait[T] {
+  def myMethod(t: T): Int
+  def doIt(t: T): Unit = {
+    myMethod(t) // java.lang.AbstractMethodError: Method BadClass.myMethod(Ljava/lang/Object;)I is abstract
+ }
+}
+
+abstract class SimpleClass[T] extends SimpleTrait[T] {
+  def myMethod(t: String): Int = 5
+}
+
+class BadClass extends SimpleClass[String]
+
+object Test {
+  def main(args: Array[String]): Unit =
+    (new BadClass).doIt("foobar")
+}