Merge pull request #12982 from dotty-staging/fix-12828

Refine overriding pairs in RefChecks

Author: smarter

compiler/src/dotty/tools/dotc/core/Denotations.scala

@@ -1021,7 +1021,7 @@ object Denotations {
      *  erasure (see i8615b, i9109b), Erasure takes care of adding any necessary
      *  bridge to make this work at runtime.
      */
-    def matchesLoosely(other: SingleDenotation)(using Context): Boolean =
+    def matchesLoosely(other: SingleDenotation, alwaysCompareTypes: Boolean = false)(using Context): Boolean =
       if isType then true
       else
         val thisLanguage = SourceLanguage(symbol)
@@ -1031,7 +1031,7 @@ object Denotations {
         val otherSig = other.signature(commonLanguage)
         sig.matchDegree(otherSig) match
           case FullMatch =>
-            true
+            !alwaysCompareTypes || info.matches(other.info)
           case MethodNotAMethodMatch =>
             !ctx.erasedTypes && {
               // A Scala zero-parameter method and a Scala non-method always match.

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

@@ -36,7 +36,7 @@ class Bridges(root: ClassSymbol, thisPhase: DenotTransformer)(using Context) {
       !sym.isOneOf(MethodOrModule) || super.exclude(sym)
   }
 
-  //val site = root.thisType
+  val site = root.thisType
 
   private var toBeRemoved = immutable.Set[Symbol]()
   private val bridges = mutable.ListBuffer[Tree]()
@@ -77,7 +77,13 @@ class Bridges(root: ClassSymbol, thisPhase: DenotTransformer)(using Context) {
                       |clashes with definition of the member itself; both have erased type ${info(member)(using elimErasedCtx)}."""",
                   bridgePosFor(member))
     }
-    else if (!bridgeExists)
+    else if !inContext(preErasureCtx)(site.memberInfo(member).matches(site.memberInfo(other))) then
+      // Neither symbol signatures nor pre-erasure types seen from root match; this means
+      // according to Scala 2 semantics there is no override.
+      // A bridge might introduce a classcast exception.
+      // Example where this was observed: run/i12828a.scala and MapView in stdlib213
+      report.log(i"suppress bridge in $root for ${member} in ${member.owner} and ${other.showLocated} since member infos ${site.memberInfo(member)} and ${site.memberInfo(other)} do not match")
+    else if !bridgeExists then
       addBridge(member, other)
   }
 

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
@@ -33,11 +34,11 @@ object OverridingPairs {
      *  pair has already been treated in a parent class.
      *  This may be refined in subclasses. @see Bridges for a use case.
      */
-    protected def parents: Array[Symbol] = base.info.parents.toArray.map(_.typeSymbol)
+    protected def parents: Array[Symbol] = base.info.parents.toArray.map(_.classSymbol)
 
-    /** 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)))
+
+    /** 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, parent: Symbol): 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

@@ -323,6 +323,14 @@ object RefChecks {
             overrideErrorMsg("no longer has compatible type"),
             (if (member.owner == clazz) member else clazz).srcPos))
 
+      /** 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
+       *  error messages. Also, bridges are not generated in this case.
+       *  Type members are always assumed to match.
+       */
+      def trueMatch: Boolean =
+        member.isType || memberTp(self).matches(otherTp(self))
+
       def emitOverrideError(fullmsg: Message) =
         if (!(hasErrors && member.is(Synthetic) && member.is(Module))) {
           // suppress errors relating toi synthetic companion objects if other override
@@ -333,7 +341,7 @@ object RefChecks {
         }
 
       def overrideError(msg: String, compareTypes: Boolean = false) =
-        if (noErrorType)
+        if trueMatch && noErrorType then
           emitOverrideError(overrideErrorMsg(msg, compareTypes))
 
       def autoOverride(sym: Symbol) =
@@ -360,24 +368,6 @@ object RefChecks {
 
       //Console.println(infoString(member) + " overrides " + infoString(other) + " in " + clazz);//DEBUG
 
-      // return if we already checked this combination elsewhere
-      if (member.owner != clazz) {
-        def deferredCheck = member.is(Deferred) || !other.is(Deferred)
-        def subOther(s: Symbol) = s derivesFrom other.owner
-        def subMember(s: Symbol) = s derivesFrom member.owner
-
-        if (subOther(member.owner) && deferredCheck)
-          //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))
-          //Console.println(infoString(member) + " shadows2 " + infoString(other) + " in " + clazz);//DEBUG
-          return
-        if (parentSymbols forall (p => subOther(p) == subMember(p)))
-          //Console.println(infoString(member) + " shadows " + infoString(other) + " in " + clazz);//DEBUG
-          return
-      }
-
       /* Is the intersection between given two lists of overridden symbols empty? */
       def intersectionIsEmpty(syms1: Iterator[Symbol], syms2: Iterator[Symbol]) = {
         val set2 = syms2.toSet
@@ -412,7 +402,7 @@ object RefChecks {
         overrideError("cannot be used here - class definitions cannot be overridden")
       else if (!other.is(Deferred) && member.isClass)
         overrideError("cannot be used here - classes can only override abstract types")
-      else if (other.isEffectivelyFinal) // (1.2)
+      else if other.isEffectivelyFinal then // (1.2)
         overrideError(i"cannot override final member ${other.showLocated}")
       else if (member.is(ExtensionMethod) && !other.is(ExtensionMethod)) // (1.3)
         overrideError("is an extension method, cannot override a normal method")
@@ -433,9 +423,11 @@ object RefChecks {
           member.setFlag(Override)
         else if (member.isType && self.memberInfo(member) =:= self.memberInfo(other))
           () // OK, don't complain about type aliases which are equal
-        else if (member.owner != clazz && other.owner != clazz &&
-                 !(other.owner derivesFrom member.owner))
-          emitOverrideError(
+        else if member.owner != clazz
+             && other.owner != clazz
+             && !other.owner.derivesFrom(member.owner)
+        then
+          overrideError(
             s"$clazz inherits conflicting members:\n  "
               + infoStringWithLocation(other) + "  and\n  " + infoStringWithLocation(member)
               + "\n(Note: this can be resolved by declaring an override in " + clazz + ".)")
@@ -496,25 +488,51 @@ 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
-        (sym1.isType || {
-          val sd1 = sym1.asSeenFrom(clazz.thisType)
-          val sd2 = sym2.asSeenFrom(clazz.thisType)
-          sd1.matchesLoosely(sd2)
+    /** 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)
+
+      private def inLinearizationOrder(sym1: Symbol, sym2: Symbol, parent: Symbol): Boolean =
+        val owner1 = sym1.owner
+        val owner2 = sym2.owner
+        def precedesIn(bcs: List[ClassSymbol]): Boolean = (bcs: @unchecked) match
+          case bc :: bcs1 =>
+            if owner1 eq bc then true
+            else if owner2 eq bc then false
+            else precedesIn(bcs1)
+          case _ =>
+            false
+        precedesIn(parent.asClass.baseClasses)
+
+      // We can exclude pairs safely from checking only under two additional conditions
+      //   - their signatures also match in the parent class.
+      //     See neg/i12828.scala for an example where this matters.
+      //   - 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)
+         .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
 
     while opc.hasNext do
@@ -528,13 +546,11 @@ object RefChecks {
     //
     //   class A { type T = B }
     //   class B extends A { override type T }
-    for
-      dcl <- clazz.info.decls.iterator
-      if dcl.is(Deferred)
-      other <- dcl.allOverriddenSymbols
-      if !other.is(Deferred)
-    do
-      checkOverride(dcl, other)
+    for dcl <- clazz.info.decls.iterator do
+      if dcl.is(Deferred) then
+        for other <- dcl.allOverriddenSymbols do
+          if !other.is(Deferred) then
+            checkOverride(dcl, other)
 
     printMixinOverrideErrors()
 
@@ -578,7 +594,8 @@ object RefChecks {
         def isConcrete(sym: Symbol) = sym.exists && !sym.isOneOf(NotConcrete)
         clazz.nonPrivateMembersNamed(mbr.name)
           .filterWithPredicate(
-            impl => isConcrete(impl.symbol) && mbrDenot.matchesLoosely(impl))
+            impl => isConcrete(impl.symbol)
+              && mbrDenot.matchesLoosely(impl, alwaysCompareTypes = true))
           .exists
 
       /** The term symbols in this class and its baseclasses that are

tests/neg/OpaqueEscape.scala

@@ -7,7 +7,7 @@ def unwrap(i:Wrapped):Int
   def wrap(i:Int):Wrapped
 }
 class Escaper extends EscaperBase{  // error: needs to be abstract
-  override def unwrap(i:Int):Int = i // error overriding method unwrap
+  override def unwrap(i:Int):Int = i // was error overriding method unwrap, now OK
   override def wrap(i:Int):Int = i  // error overriding method wrap
 }
 val e = new Escaper:EscaperBase

tests/neg/i11828.check

@@ -0,0 +1,8 @@
+-- Error: tests/neg/i12828.scala:7:7 -----------------------------------------------------------------------------------
+7 |object Baz extends Bar[Int] // error overriding foo: incompatible type
+  |       ^
+  |       object creation impossible, since def foo(x: A): Unit in trait Foo is not defined 
+  |       (Note that
+  |        parameter A in def foo(x: A): Unit in trait Foo does not match
+  |        parameter Int & String in def foo(x: A & String): Unit in trait Bar
+  |        )

tests/neg/i12828.check

@@ -0,0 +1,8 @@
+-- Error: tests/neg/i12828.scala:7:7 -----------------------------------------------------------------------------------
+7 |object Baz extends Bar[Int] // error: not implemented
+  |       ^
+  |       object creation impossible, since def foo(x: A): Unit in trait Foo is not defined 
+  |       (Note that
+  |        parameter A in def foo(x: A): Unit in trait Foo does not match
+  |        parameter Int & String in def foo(x: A & String): Unit in trait Bar
+  |        )

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: not implemented
+
+@main def run() = Baz.foo(42)

tests/neg/i12828c.scala

@@ -0,0 +1,12 @@
+abstract class Foo[A] {
+  def foo(x: A): Unit
+}
+abstract class Bar[A] extends Foo[A] {
+  def foo(x: A with String): Unit = println(x.toUpperCase)
+}
+object Baz extends Bar[Int] // error: not implemented (same as Scala 2)
+                            // Scala 2 gives: object creation impossible. Missing implementation for `foo`
+
+object Test {
+  def main(args: Array[String]) = Baz.foo(42)
+}

tests/neg/i12828d.scala

@@ -0,0 +1,18 @@
+trait A[X] {
+  def foo(x: X): Unit =
+    println("A.foo")
+}
+trait B[X] extends A[X] {
+  def foo(x: Int): Unit =
+    println("B.foo")
+}
+object C extends B[Int] // error: conflicting members
+                        // Scala 2: same
+
+object Test {
+  def main(args: Array[String]) = {
+    C.foo(1)
+    val a: A[Int] = C
+    a.foo(1)
+  }
+}