Refine overriding pairs in RefChecks

We now exclude a pair of overriding / overridden symbols that
have a common subclass parent of the base class only under the
additional condition that their signatures also match.

#12828 shows a case where this matters:
```scala
trait Foo[A]:
  def foo(x: A): Unit

trait Bar[A] extends Foo[A]:
  def foo(x: A & String): Unit = println(x.toUpperCase)

object Baz extends Bar[Int] // error overriding foo: incompatible type

@main def run() = Baz.foo(42)
```
When checking `Baz`, there is a common subclass parent (namely `Bar`),
but the signatures of the two `foo` definitions as seen from `Bar` are
different, so we cannot assume that the pair has already been checked.

Fixes #12828

Author: odersky

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

@@ -1,8 +1,9 @@
-package dotty.tools.dotc
+package dotty.tools
+package dotc
 package transform
 
 import core._
-import Flags._, Symbols._, Contexts._, Scopes._, Decorators._
+import Flags._, Symbols._, Contexts._, Scopes._, Decorators._, Types.Type
 import collection.mutable
 import collection.immutable.BitSet
 import scala.annotation.tailrec
@@ -35,9 +36,9 @@ object OverridingPairs {
      */
     protected def parents: Array[Symbol] = base.info.parents.toArray.map(_.typeSymbol)
 
-    /** Does `sym1` match `sym2` so that it qualifies as overriding.
-     *  Types always match. Term symbols match if their membertypes
-     *  relative to <base>.this do
+    /** Does `sym1` match `sym2` so that it qualifies as overriding when both symbols are
+     *  seen as members of `self`? Types always match. Term symbols match if their membertypes
+     *  relative to `self` do.
      */
     protected def matches(sym1: Symbol, sym2: Symbol): Boolean =
       sym1.isType || sym1.asSeenFrom(self).matches(sym2.asSeenFrom(self))
@@ -85,11 +86,22 @@ object OverridingPairs {
         then bits += i
       subParents(bc) = bits
 
-    private def hasCommonParentAsSubclass(cls1: Symbol, cls2: Symbol): Boolean =
-      (subParents(cls1) intersect subParents(cls2)).nonEmpty
+    /** Is the override of `sym1` and `sym2` already handled when checking
+     *  a parent of `self`?
+     */
+    private def isHandledByParent(sym1: Symbol, sym2: Symbol): Boolean =
+      val commonParents = subParents(sym1.owner).intersect(subParents(sym2.owner))
+      commonParents.nonEmpty
+      && commonParents.exists(i => canBeHandledByParent(sym1, sym2, parents(i).thisType))
+
+    /** Can pair `sym1`/`sym2` be handled by parent `parentType` which is a common subtype
+     *  of both symbol's owners? Assumed to be true by default, but overridden in RefChecks.
+     */
+    protected def canBeHandledByParent(sym1: Symbol, sym2: Symbol, parentType: Type): Boolean =
+      true
 
     /** The scope entries that have already been visited as overridden
-     *  (maybe excluded because of hasCommonParentAsSubclass).
+     *  (maybe excluded because of already handled by a parent).
      *  These will not appear as overriding
      */
     private val visited = util.HashSet[Symbol]()
@@ -134,28 +146,22 @@ object OverridingPairs {
      *    overridden = overridden member of the pair, provided hasNext is true
      */
     @tailrec final def next(): Unit =
-      if (nextEntry ne null) {
+      if nextEntry != null then
         nextEntry = decls.lookupNextEntry(nextEntry)
-        if (nextEntry ne null)
-          try {
+        if nextEntry != null then
+          try
             overridden = nextEntry.sym
-            if (overriding.owner != overridden.owner && matches(overriding, overridden)) {
+            if overriding.owner != overridden.owner && matches(overriding, overridden) then
               visited += overridden
-              if (!hasCommonParentAsSubclass(overriding.owner, overridden.owner)) return
-            }
-          }
-          catch {
-            case ex: TypeError =>
-              // See neg/i1750a for an example where a cyclic error can arise.
-              // The root cause in this example is an illegal "override" of an inner trait
-              report.error(ex, base.srcPos)
-          }
-        else {
+              if !isHandledByParent(overriding, overridden) then return
+          catch case ex: TypeError =>
+            // See neg/i1750a for an example where a cyclic error can arise.
+            // The root cause in this example is an illegal "override" of an inner trait
+            report.error(ex, base.srcPos)
+        else
           curEntry = curEntry.prev
           nextOverriding()
-        }
         next()
-      }
 
     nextOverriding()
     next()

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

@@ -370,10 +370,13 @@ object RefChecks {
           //Console.println(infoString(member) + " shadows1 " + infoString(other) " in " + clazz);//DEBUG
           return
         val parentSymbols = clazz.info.parents.map(_.typeSymbol)
-        if (parentSymbols exists (p => subOther(p) && subMember(p) && deferredCheck))
+        def matchIn(parent: Symbol): Boolean = considerMatching(member, other, parent.thisType)
+        if parentSymbols.exists(p =>
+          subOther(p) && subMember(p) && deferredCheck && matchIn(p))
+        then
           //Console.println(infoString(member) + " shadows2 " + infoString(other) + " in " + clazz);//DEBUG
           return
-        if (parentSymbols forall (p => subOther(p) == subMember(p)))
+        if parentSymbols.forall(p => subOther(p) == subMember(p) && matchIn(p)) then
           //Console.println(infoString(member) + " shadows " + infoString(other) + " in " + clazz);//DEBUG
           return
       }
@@ -496,26 +499,30 @@ object RefChecks {
           }*/
     }
 
-    val opc = new OverridingPairs.Cursor(clazz):
-
-      /** 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
-       */
-      override def matches(sym1: Symbol, sym2: Symbol): Boolean =
-        !(sym1.owner.is(JavaDefined, butNot = Trait) && sym2.owner.is(JavaDefined, butNot = Trait)) && // javac already handles these checks
+    /** 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 =
+      !(sym1.owner.is(JavaDefined, butNot = Trait) && sym2.owner.is(JavaDefined, butNot = Trait)) && // javac already handles these checks
         (sym1.isType || {
-          val sd1 = sym1.asSeenFrom(clazz.thisType)
-          val sd2 = sym2.asSeenFrom(clazz.thisType)
+          val sd1 = sym1.asSeenFrom(self)
+          val sd2 = sym2.asSeenFrom(self)
           sd1.matchesLoosely(sd2)
           && (sym1.hasTargetName(sym2.targetName)
              || compatibleTypes(sym1, sd1.info, sym2, sd2.info))
         })
-    end opc
+
+    val opc = new OverridingPairs.Cursor(clazz):
+      override def matches(sym1: Symbol, sym2: Symbol): Boolean =
+        considerMatching(sym1, sym2, self)
+      override def canBeHandledByParent(sym1: Symbol, sym2: Symbol, parentType: Type): Boolean =
+        considerMatching(sym1, sym2, parentType)
+         .showing(i"already handled ${sym1.showLocated}: ${sym1.asSeenFrom(parentType).signature}, ${sym2.showLocated}: ${sym2.asSeenFrom(parentType).signature} = $result", refcheck)
 
     while opc.hasNext do
       checkOverride(opc.overriding, opc.overridden)

tests/neg/i12828.check

@@ -0,0 +1,7 @@
+-- [E163] Declaration Error: tests/neg/i12828.scala:7:7 ----------------------------------------------------------------
+7 |object Baz extends Bar[Int] // error overriding foo: incompatible type
+  |       ^
+  |       error overriding method foo in trait Foo of type (x: Int): Unit;
+  |         method foo in trait Bar of type (x: Int & String): Unit has incompatible type
+
+longer explanation available when compiling with `-explain`

tests/neg/i12828.scala

@@ -0,0 +1,9 @@
+trait Foo[A]:
+  def foo(x: A): Unit
+
+trait Bar[A] extends Foo[A]:
+  def foo(x: A & String): Unit = println(x.toUpperCase)
+
+object Baz extends Bar[Int] // error overriding foo: incompatible type
+
+@main def run() = Baz.foo(42)