Enforce matching targetNames when overriding

odersky · odersky · commit 6310ec2013b4 · 2020-11-10T11:59:13.000+01:00
diff --git a/compiler/src/dotty/tools/dotc/config/Printers.scala b/compiler/src/dotty/tools/dotc/config/Printers.scala
@@ -37,6 +37,7 @@ object Printers {
   val pickling = noPrinter
   val quotePickling = noPrinter
   val plugins = noPrinter
+  val refcheck = noPrinter
   val simplify = noPrinter
   val staging = noPrinter
   val subtyping = noPrinter
diff --git a/compiler/src/dotty/tools/dotc/core/Denotations.scala b/compiler/src/dotty/tools/dotc/core/Denotations.scala
@@ -986,35 +986,36 @@ object Denotations {
 
     def matches(other: SingleDenotation)(using Context): Boolean =
       symbol.hasTargetName(other.symbol.targetName)
-      && {
-        val d = signature.matchDegree(other.signature)
-        d match
-          case FullMatch =>
-            true
-          case MethodNotAMethodMatch =>
-            !ctx.erasedTypes && {
-              val isJava = symbol.is(JavaDefined)
-              val otherIsJava = other.symbol.is(JavaDefined)
-              // A Scala zero-parameter method and a Scala non-method always match.
-              if !isJava && !otherIsJava then
-                true
-              // Java allows defining both a field and a zero-parameter method with the same name,
-              // so they must not match.
-              else if isJava && otherIsJava then
-                false
-              // A Java field never matches a Scala method.
-              else if isJava then
-                symbol.is(Method)
-              else // otherIsJava
-                other.symbol.is(Method)
-            }
-          case ParamMatch =>
-            // The signatures do not tell us enough to be sure about matching
-            !ctx.erasedTypes && info.matches(other.info)
-          case noMatch =>
-            false
-      }
-    end matches
+      && matchesLoosely(other)
+
+    /** matches without a target name check */
+    def matchesLoosely(other: SingleDenotation)(using Context): Boolean =
+      val d = signature.matchDegree(other.signature)
+      d match
+        case FullMatch =>
+          true
+        case MethodNotAMethodMatch =>
+          !ctx.erasedTypes && {
+            val isJava = symbol.is(JavaDefined)
+            val otherIsJava = other.symbol.is(JavaDefined)
+            // A Scala zero-parameter method and a Scala non-method always match.
+            if !isJava && !otherIsJava then
+              true
+            // Java allows defining both a field and a zero-parameter method with the same name,
+            // so they must not match.
+            else if isJava && otherIsJava then
+              false
+            // A Java field never matches a Scala method.
+            else if isJava then
+              symbol.is(Method)
+            else // otherIsJava
+              other.symbol.is(Method)
+          }
+        case ParamMatch =>
+          // The signatures do not tell us enough to be sure about matching
+          !ctx.erasedTypes && info.matches(other.info)
+        case noMatch =>
+          false
 
     def mapInherited(ownDenots: PreDenotation, prevDenots: PreDenotation, pre: Type)(using Context): SingleDenotation =
       if hasUniqueSym && prevDenots.containsSym(symbol) then NoDenotation
diff --git a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala
@@ -18,6 +18,7 @@ import config.{ScalaVersion, NoScalaVersion}
 import Decorators._
 import typer.ErrorReporting._
 import config.Feature.{warnOnMigration, migrateTo3}
+import config.Printers.refcheck
 import reporting._
 import scala.util.matching.Regex._
 import Constants.Constant
@@ -236,6 +237,26 @@ 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
+          member.name.is(DefaultGetterName) || // default getters are not checked for compatibility
+          memberTp.overrides(otherTp,
+              member.matchNullaryLoosely || other.matchNullaryLoosely || fallBack)
+      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.
        */
@@ -245,7 +266,7 @@ object RefChecks {
         else self.memberInfo(member)
       def otherTp(self: Type) = self.memberInfo(other)
 
-      report.debuglog("Checking validity of %s overriding %s".format(member.showLocated, other.showLocated))
+      refcheck.println(i"check override ${infoString(member)} overriding ${infoString(other)}")
 
       def noErrorType = !memberTp(self).isErroneous && !otherTp(self).isErroneous
 
@@ -265,6 +286,12 @@ object RefChecks {
           infoStringWithLocation(other), infoString(member), msg, addendum)
       }
 
+      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))
+
       def emitOverrideError(fullmsg: String) =
         if (!(hasErrors && member.is(Synthetic) && member.is(Module))) {
           // suppress errors relating toi synthetic companion objects if other override
@@ -291,29 +318,14 @@ object RefChecks {
           (if (otherAccess == "") "public" else "at least " + otherAccess))
       }
 
-      def compatibleTypes(memberTp: Type, otherTp: Type): 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
-            member.name.is(DefaultGetterName) || // default getters are not checked for compatibility
-            memberTp.overrides(otherTp,
-                member.matchNullaryLoosely || other.matchNullaryLoosely ||
-                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
-        }
+      def overrideTargetNameError() =
+        val otherTargetName = i"@targetName(${other.targetName})"
+        if member.hasTargetName(member.name) then
+          overrideError(i"misses a target name annotation $otherTargetName")
+        else if other.hasTargetName(other.name) then
+          overrideError(i"should not have a @targetName annotation since the overridden member hasn't one either")
+        else
+          overrideError(i"has a different target name annotation; it should be $otherTargetName")
 
       //Console.println(infoString(member) + " overrides " + infoString(other) + " in " + clazz);//DEBUG
 
@@ -359,7 +371,9 @@ object RefChecks {
            && (ob.isContainedIn(mb) || other.isAllOf(JavaProtected))
              // m relaxes o's access boundary,
              // or o is Java defined and protected (see #3946)
-      if (!isOverrideAccessOK)
+      if !member.hasTargetName(other.targetName) then
+        overrideTargetNameError()
+      else if (!isOverrideAccessOK)
         overrideAccessError()
       else if (other.isClass)
         // direct overrides were already checked on completion (see Checking.chckWellFormed)
@@ -428,8 +442,8 @@ object RefChecks {
         overrideError("is not inline, cannot implement an inline method")
       else if (other.isScala2Macro && !member.isScala2Macro) // (1.11)
         overrideError("cannot be used here - only Scala-2 macros can override Scala-2 macros")
-      else if (!compatibleTypes(memberTp(self), otherTp(self)) &&
-                 !compatibleTypes(memberTp(upwardsSelf), otherTp(upwardsSelf)))
+      else if (!compatTypes(memberTp(self), otherTp(self)) &&
+                 !compatTypes(memberTp(upwardsSelf), otherTp(upwardsSelf)))
         overrideError("has incompatible type" + err.whyNoMatchStr(memberTp(self), otherTp(self)))
       else if (member.targetName != other.targetName)
         if (other.targetName != other.name)
@@ -451,7 +465,27 @@ object RefChecks {
           }*/
     }
 
-    val opc = new OverridingPairs.Cursor(clazz)
+    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.isType
+        || {
+          val sd1 = sym1.asSeenFrom(clazz.thisType)
+          val sd2 = sym2.asSeenFrom(clazz.thisType)
+          sd1.matchesLoosely(sd2)
+          && (sym1.hasTargetName(sym2.targetName)
+             || compatibleTypes(sym1, sd1.info, sym2, sd2.info))
+        }
+    end opc
+
     while opc.hasNext do
       checkOverride(opc.overriding, opc.overridden)
       opc.next()
diff --git a/tests/neg/targetName-override.check b/tests/neg/targetName-override.check
@@ -1,16 +1,19 @@
+-- Error: tests/neg/targetName-override.scala:16:35 --------------------------------------------------------------------
+16 |  @targetName("foo1") override def foo() = 1    // error: should not have a target name
+   |                                   ^
+   |error overriding method foo in class Alpha of type (): Int;
+   |  method foo of type (): Int should not have a @targetName annotation since the overridden member hasn't one either
+-- Error: tests/neg/targetName-override.scala:18:25 --------------------------------------------------------------------
+18 |  @targetName("baz") def foo(x: String): String = x ++ x   // error: has a different target name annotation
+   |                         ^
+   | error overriding method foo in class Alpha of type (x: String): String;
+   |   method foo of type (x: String): String has a different target name annotation; it should be @targetName(foo1)
+-- Error: tests/neg/targetName-override.scala:20:15 --------------------------------------------------------------------
+20 |  override def ++ (xs: Alpha[String]): Alpha[String] = this   // error: misses a targetname annotation
+   |               ^
+   |      error overriding method ++ in class Alpha of type (xs: Alpha[String]): Alpha[String];
+   |        method ++ of type (xs: Alpha[String]): Alpha[String] misses a target name annotation @targetName(append)
 -- Error: tests/neg/targetName-override.scala:14:6 ---------------------------------------------------------------------
 14 |class Beta extends Alpha[String] {  // error: needs to be abstract
    |      ^
    |class Beta needs to be abstract, since there is a deferred declaration of method foo in class Alpha of type (x: String): String which is not implemented in a subclass
--- [E038] Declaration Error: tests/neg/targetName-override.scala:16:35 -------------------------------------------------
-16 |  @targetName("foo1") override def foo() = 1    // error: different signature than overridden
-   |                                   ^
-   |                                   method foo has a different target name than the overridden declaration
-
-longer explanation available when compiling with `-explain`
--- [E038] Declaration Error: tests/neg/targetName-override.scala:20:15 -------------------------------------------------
-20 |  override def ++ (xs: Alpha[String]): Alpha[String] = this   // error: different signature than overidden
-   |               ^
-   |               method ++ has a different target name than the overridden declaration
-
-longer explanation available when compiling with `-explain`
diff --git a/tests/neg/targetName-override.scala b/tests/neg/targetName-override.scala
@@ -5,18 +5,18 @@ abstract class Alpha[T] {
 
   def foo() = 1
 
-  @targetName("bar") def foo(x: T): T
+  @targetName("foo1") def foo(x: T): T
 
   @targetName("append") def ++ (xs: Alpha[T]): Alpha[T] = this
 
 }
 
 class Beta extends Alpha[String] {  // error: needs to be abstract
 
-  @targetName("foo1") override def foo() = 1    // error: different signature than overridden
+  @targetName("foo1") override def foo() = 1    // error: should not have a target name
 
-  @targetName("baz") def foo(x: String): String = x ++ x   // OK,
+  @targetName("baz") def foo(x: String): String = x ++ x   // error: has a different target name annotation
 
-  override def ++ (xs: Alpha[String]): Alpha[String] = this   // error: different signature than overidden
+  override def ++ (xs: Alpha[String]): Alpha[String] = this   // error: misses a targetname annotation
 
 }
diff --git a/tests/pos/targetName-override.scala b/tests/pos/targetName-override.scala
@@ -0,0 +1,11 @@
+import scala.annotation.targetName
+
+class A:
+  @targetName("ff") def f(x: => Int): Int = x
+  def f(x: => String): Int = x.length
+
+
+class B extends A:
+  @targetName("ff") override def f(x: => Int): Int = x // OK
+  override def f(x: => String): Int = x.length // OK
+

Original file line number	Diff line number	Diff line change
`@@ -5,18 +5,18 @@ abstract class Alpha[T] {`
`5`	`5`
`6`	`6`	`def foo() = 1`
`7`	`7`
`8`		`- @targetName("bar") def foo(x: T): T`
	`8`	`+ @targetName("foo1") def foo(x: T): T`
`9`	`9`
`10`	`10`	`@targetName("append") def ++ (xs: Alpha[T]): Alpha[T] = this`
`11`	`11`
`12`	`12`	`}`
`13`	`13`
`14`	`14`	`class Beta extends Alpha[String] { // error: needs to be abstract`
`15`	`15`
`16`		`- @targetName("foo1") override def foo() = 1 // error: different signature than overridden`
	`16`	`+ @targetName("foo1") override def foo() = 1 // error: should not have a target name`
`17`	`17`
`18`		`- @targetName("baz") def foo(x: String): String = x ++ x // OK,`
	`18`	`+ @targetName("baz") def foo(x: String): String = x ++ x // error: has a different target name annotation`
`19`	`19`
`20`		`- override def ++ (xs: Alpha[String]): Alpha[String] = this // error: different signature than overidden`
	`20`	`+ override def ++ (xs: Alpha[String]): Alpha[String] = this // error: misses a targetname annotation`
`21`	`21`
`22`	`22`	`}`