Merge pull request #6025 from dotty-staging/fix/double-defs

Improve double definition errors

Author: smarter

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

@@ -82,6 +82,10 @@ case class Signature(paramsSig: List[TypeName], resSig: TypeName) {
       else NoMatch
     else NoMatch
 
+  /** Does this signature potentially clash with `that` ? */
+  def clashes(that: Signature)(implicit ctx: Context): Boolean =
+    matchDegree(that) == FullMatch
+
   /** name.toString == "" or name.toString == "_" */
   private def isWildcard(name: TypeName) = name.isEmpty || name == tpnme.WILDCARD
 

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

@@ -797,19 +797,27 @@ object Symbols {
   NoDenotation // force it in order to set `denot` field of NoSymbol
 
   implicit class Copier[N <: Name](sym: Symbol { type ThisName = N })(implicit ctx: Context) {
-    /** Copy a symbol, overriding selective fields */
+    /** Copy a symbol, overriding selective fields.
+     *  Note that `coord` and `associatedFile` will be set from the fields in `owner`, not
+     *  the fields in `sym`.
+     */
     def copy(
         owner: Symbol = sym.owner,
         name: N = (sym.name: N), // Dotty deviation: type ascription to avoid leaking private sym (only happens in unpickling), won't be needed once #1723 is fixed
         flags: FlagSet = sym.flags,
         info: Type = sym.info,
         privateWithin: Symbol = sym.privateWithin,
-        coord: Coord = sym.coord,
-        associatedFile: AbstractFile = sym.associatedFile): Symbol =
+        coord: Coord = NoCoord, // Can be `= owner.coord` once we boostrap
+        associatedFile: AbstractFile = null // Can be `= owner.associatedFile` once we boostrap
+    ): Symbol = {
+      val coord1 = if (coord == NoCoord) owner.coord else coord
+      val associatedFile1 = if (associatedFile == null) owner.associatedFile else associatedFile
+
       if (sym.isClass)
-        ctx.newClassSymbol(owner, name.asTypeName, flags, _ => info, privateWithin, coord, associatedFile)
+        ctx.newClassSymbol(owner, name.asTypeName, flags, _ => info, privateWithin, coord1, associatedFile1)
       else
-        ctx.newSymbol(owner, name, flags, info, privateWithin, coord)
+        ctx.newSymbol(owner, name, flags, info, privateWithin, coord1)
+    }
   }
 
   /** Makes all denotation operations available on symbols */

compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.java

@@ -128,7 +128,7 @@ public enum ErrorMessageID {
     PolymorphicMethodMissingTypeInParentID,
     ParamsNoInlineID,
     JavaSymbolIsNotAValueID,
-    DoubleDeclarationID,
+    DoubleDefinitionID,
     MatchCaseOnlyNullWarningID,
     ImportRenamedTwiceID,
     TypeTestAlwaysSucceedsID,

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

@@ -2137,18 +2137,38 @@ object messages {
     val explanation: String = ""
   }
 
-  case class DoubleDeclaration(decl: Symbol, previousDecl: Symbol)(implicit ctx: Context) extends Message(DoubleDeclarationID) {
+  case class DoubleDefinition(decl: Symbol, previousDecl: Symbol, base: Symbol)(implicit ctx: Context) extends Message(DoubleDefinitionID) {
     val kind: String = "Duplicate Symbol"
     val msg: String = {
       val details = if (decl.isRealMethod && previousDecl.isRealMethod) {
         // compare the signatures when both symbols represent methods
         decl.signature.matchDegree(previousDecl.signature) match {
-          /* case Signature.NoMatch => // can't happen because decl.matches(previousDecl) is checked before reporting this error */
-          case Signature.ParamMatch => "\nOverloads with matching parameter types are not allowed."
-          case _ /* Signature.FullMatch */ => "\nThe definitions have matching type signatures after erasure."
+          case Signature.NoMatch =>
+            "" // shouldn't be reachable
+          case Signature.ParamMatch =>
+            "have matching parameter types."
+          case Signature.FullMatch =>
+            "have the same type after erasure."
         }
       } else ""
-      hl"${decl.showLocated} is already defined as ${previousDecl.showDcl} ${if (previousDecl.span.exists) s"at line ${previousDecl.sourcePos.line + 1}" else ""}." + details
+      def symLocation(sym: Symbol) = {
+        val lineDesc =
+          if (sym.span.exists && sym.span != sym.owner.span)
+            s" at line ${sym.sourcePos.line + 1}" else ""
+        i"in ${sym.owner}${lineDesc}"
+      }
+      val clashDescription =
+        if (decl.owner eq previousDecl.owner)
+          "Double definition"
+        else if ((decl.owner eq base) || (previousDecl eq base))
+          "Name clash between defined and inherited member"
+        else
+          "Name clash between inherited members"
+
+      em"""$clashDescription:
+          |${previousDecl.showDcl} ${symLocation(previousDecl)} and
+          |${decl.showDcl} ${symLocation(decl)}
+          |""" + details
     }
     val explanation: String = ""
   }

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

@@ -7,6 +7,7 @@ import MegaPhase._
 import Types._, Contexts._, Flags._, DenotTransformers._
 import Symbols._, StdNames._, Trees._
 import TypeErasure.ErasedValueType, ValueClasses._
+import reporting.diagnostic.messages.DoubleDefinition
 
 object ElimErasedValueType {
   val name: String = "elimErasedValueType"
@@ -103,12 +104,7 @@ class ElimErasedValueType extends MiniPhase with InfoTransformer {
         //
         // The problem is that `map` was forwarded twice, with different instantiated types.
         // Maybe we should move mixin forwarding after erasure to avoid redundant forwarders like these.
-        ctx.error(
-            em"""double definition:
-                |$sym1: $info1 in ${sym1.owner} and
-                |$sym2: $info2 in ${sym2.owner}
-                |have same type after erasure: $info""",
-            root.sourcePos)
+        ctx.error(DoubleDefinition(sym1, sym2, root), root.sourcePos)
     }
     val earlyCtx = ctx.withPhase(ctx.elimRepeatedPhase.next)
     while (opc.hasNext) {

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

@@ -752,10 +752,10 @@ trait Checking {
         def javaFieldMethodPair =
           decl.is(JavaDefined) && other.is(JavaDefined) &&
           decl.is(Method) != other.is(Method)
-        if (decl.matches(other) && !javaFieldMethodPair) {
+        if ((decl.signature.clashes(other.signature) || decl.matches(other)) && !javaFieldMethodPair) {
           def doubleDefError(decl: Symbol, other: Symbol): Unit =
             if (!decl.info.isErroneous && !other.info.isErroneous)
-              ctx.error(DoubleDeclaration(decl, other), decl.sourcePos)
+              ctx.error(DoubleDefinition(decl, other, cls), decl.sourcePos)
           if (decl is Synthetic) doubleDefError(other, decl)
           else doubleDefError(decl, other)
         }

compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTests.scala

@@ -1514,7 +1514,7 @@ class ErrorMessagesTests extends ErrorMessagesTest {
     }.expect { (ictx, messages) =>
       implicit val ctx: Context = ictx
       assertMessageCount(1, messages)
-      val DoubleDeclaration(symbol, previousSymbol) :: Nil = messages
+      val DoubleDefinition(symbol, previousSymbol, _) :: Nil = messages
       assertEquals(symbol.name.mangledString, "a")
   }
 

tests/neg/doubleDefinition.check

@@ -0,0 +1,94 @@
+<2471..2471> in doubleDefinition.scala
+Double definition:
+def foo: [T <: Int](x: T): T in class Test20 at line 143 and
+def foo: [S <: Int, T <: Int](x: S): T in class Test20 at line 144
+have the same type after erasure.
+<2338..2338> in doubleDefinition.scala
+Double definition:
+def foo: [T <: Int](x: T): T in class Test19 at line 137 and
+def foo(x: Int): Int in class Test19 at line 138
+have the same type after erasure.
+<2016..2016> in doubleDefinition.scala
+Double definition:
+var foo: Int in class Test16 at line 115 and
+def foo: => Int in class Test16 at line 116
+<1949..1949> in doubleDefinition.scala
+Double definition:
+var foo: Int in class Test15 at line 111 and
+def foo: => String in class Test15 at line 112
+<1885..1885> in doubleDefinition.scala
+Double definition:
+var foo: Int in class Test14 at line 107 and
+def foo: => Int in class Test14 at line 108
+<1827..1827> in doubleDefinition.scala
+Double definition:
+var foo: Int in class Test13 at line 103 and
+def foo: => String in class Test13 at line 104
+<1772..1772> in doubleDefinition.scala
+Double definition:
+val foo: Int in class Test12 at line 99 and
+def foo: => Int in class Test12 at line 100
+<1705..1705> in doubleDefinition.scala
+Double definition:
+val foo: Int in class Test11 at line 95 and
+def foo: => String in class Test11 at line 96
+<1641..1641> in doubleDefinition.scala
+Double definition:
+val foo: Int in class Test10 at line 91 and
+def foo: => Int in class Test10 at line 92
+<1583..1583> in doubleDefinition.scala
+Double definition:
+val foo: Int in class Test9 at line 87 and
+def foo: => String in class Test9 at line 88
+<1481..1481> in doubleDefinition.scala
+Double definition:
+var foo: Int in class Test8d at line 81 and
+def foo: => Int in class Test8d at line 82
+<1428..1428> in doubleDefinition.scala
+Double definition:
+def foo: => Int in class Test8c at line 76 and
+var foo: Int in class Test8c at line 77
+<1375..1375> in doubleDefinition.scala
+Double definition:
+def foo: => Int in class Test8b at line 71 and
+val foo: Int in class Test8b at line 72
+<1322..1322> in doubleDefinition.scala
+Double definition:
+val foo: Int in class Test8 at line 66 and
+def foo: => Int in class Test8 at line 67
+<1236..1236> in doubleDefinition.scala
+Double definition:
+def foo(x: List[A]): A => A in trait Test7 at line 61 and
+def foo(x: List[A]): (B, B) => B in trait Test7 at line 62
+have matching parameter types.
+<984..984> in doubleDefinition.scala
+Double definition:
+def foo(x: List[A]): A => A in trait Test6 at line 54 and
+def foo(x: List[B]): B => B in trait Test6 at line 55
+have the same type after erasure.
+<739..739> in doubleDefinition.scala
+Double definition:
+var foo: Int in class Test4d at line 40 and
+def foo: => Int in class Test4d at line 41
+<686..686> in doubleDefinition.scala
+Double definition:
+def foo: => Int in class Test4c at line 35 and
+var foo: Int in class Test4c at line 36
+<633..633> in doubleDefinition.scala
+Double definition:
+def foo: => Int in class Test4b at line 30 and
+val foo: Int in class Test4b at line 31
+<580..580> in doubleDefinition.scala
+Double definition:
+val foo: Int in class Test4 at line 25 and
+def foo: => Int in class Test4 at line 26
+<494..494> in doubleDefinition.scala
+Double definition:
+def foo(x: List[A]): A => A in class Test3 at line 20 and
+def foo(x: List[A]): (B, B) => B in class Test3 at line 21
+have matching parameter types.
+<242..242> in doubleDefinition.scala
+Double definition:
+def foo(x: List[A]): A => A in class Test2 at line 13 and
+def foo(x: List[B]): B => B in class Test2 at line 14
+have the same type after erasure.

tests/neg/doubleDefinition.scala

@@ -131,3 +131,15 @@ class Test18 {
 	def foo(a: A) = 1
 	def foo(b: B) = 1
 }
+
+// Error when overloading polymorphic and non-polymorphic methods
+class Test19 {
+  def foo[T <: Int](x: T): T = x
+  def foo(x: Int): Int = x // error
+}
+
+// Error when overloading polymorphic methods
+class Test20 {
+  def foo[T <: Int](x: T): T = x
+  def foo[S <: Int, T <: Int](x: S): T = ??? // error
+}