Skip to content

Add error message about double definition (#1589) #4064

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Mar 19, 2018
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ public enum ErrorMessageID {
PolymorphicMethodMissingTypeInParentID,
ParamsNoInlineID,
JavaSymbolIsNotAValueID,
DoubleDeclarationID,
;

public int errorNumber() {
Expand Down
16 changes: 16 additions & 0 deletions compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2079,4 +2079,20 @@ object messages {
}
val explanation = ""
}

case class DoubleDeclaration(decl: Symbol, previousDecl: Symbol)(implicit ctx: Context) extends Message(DoubleDeclarationID) {
val kind = "Duplicate Symbol"
val msg = {
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."
}
} else ""
hl"${decl.showLocated} is already defined as ${previousDecl.showDcl} at line ${previousDecl.pos.line + 1}." + details
}
val explanation = ""
}
}
14 changes: 5 additions & 9 deletions compiler/src/dotty/tools/dotc/typer/Checking.scala
Original file line number Diff line number Diff line change
Expand Up @@ -611,23 +611,19 @@ trait Checking {
}
}

/** Check that class does not define same symbol twice */
def checkNoDoubleDefs(cls: Symbol)(implicit ctx: Context): Unit = {
/** Check that class does not declare same symbol twice */
def checkNoDoubleDeclaration(cls: Symbol)(implicit ctx: Context): Unit = {
val seen = new mutable.HashMap[Name, List[Symbol]] {
override def default(key: Name) = Nil
}
typr.println(i"check no double defs $cls")
typr.println(i"check no double declarations $cls")

def checkDecl(decl: Symbol): Unit = {
for (other <- seen(decl.name)) {
typr.println(i"conflict? $decl $other")
if (decl.matches(other)) {
def doubleDefError(decl: Symbol, other: Symbol): Unit = {
def ofType = if (decl.isType) "" else em": ${other.info}"
def explanation =
if (!decl.isRealMethod) ""
else "\n(the definitions have matching type signatures)"
ctx.error(em"$decl is already defined as $other$ofType$explanation", decl.pos)
ctx.error(DoubleDeclaration(decl, other), decl.pos)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You must not drop the info on matching type signatures. Either you preserve it or you figure out why it's there by walking git blame (which leads for instance to #597) and then realize you should preserve it.
Methods can be overloaded, but ths code reject attempts at overloading that define methods with matching signatures.

For instance, for this code from #597 we need to explain users that this sort of overload is not allowed:

trait A
trait B

class Test {
  def foo(x: List[A]): Function1[A, A] = ???
  def foo(x: List[B]): Function2[B, B, B] = ??? //error
}

I think this code would make a good testcase.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your feedback. If I understand correctly, the code given in #597 should not compile. I added a test case in tests/neg/, but the tests failed because it compiles successfully.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh! I just wanted to say "please don't remove the message", but I should still have tried.

The behavior of decl.matches(other) probably changed since #597 was filed. I did more experiments and asked around to figure this out.

So, here is the current behavior:

scala> class Test {
           def foo(x: List[A]): Function1[A, A] = ???
           def foo(x: List[B]): Function1[B, B] = ??? // error
         }
3 |    def foo(x: List[B]): Function1[B, B] = ??? // error
  |        ^
  |        method foo is already defined as method foo: (x: List[A]): A => A
  |        (the definitions have matching type signatures)

That code fails because, while the overload is "acceptable" (you can tell at call site which one to call, I forget the right word), the two functions erase to the same "signature" (which here means JVM signature, according to @smarter).

The regression I was observing is that here "the definitions have matching type signatures" is important. I'll accept this PR as soon as that message is restored with the same check (I'm not sure why isRealMethod is tested, please still keep it).

Here or in further PRs, we might want some clearer message there. Scalac gives:

<console>:15: error: double definition:
def foo(x: List[A]): A => A at line 14 and
def foo(x: List[B]): B => B at line 15
have same type after erasure: (x: List)Function1
                  def foo(x: List[B]): Function1[B, B] = ??? // error

Instead,

class Test {
  def foo(x: List[A]): Function1[A, A] = ???
  def foo(x: List[B]): Function2[B, B, B] = ??? //error
}

is supported because the overload is acceptable and they erase to different JVM signatures (because of the different return types).

Finally, this code is rejected because this overload isn't acceptable:

scala> class Test {
           def foo(x: List[A]): Function1[A, A] = ???
           def foo(x: List[A]): Function2[B, B, B] = ??? // error
         }
3 |    def foo(x: List[A]): Function2[B, B, B] = ??? // error
  |        ^
  |        method foo is already defined as method foo: (x: List[A]): A => A
  |        (the definitions have matching type signatures)

This should arguably give a different error:

scala> class Test {
     |            def foo(x: List[A]): Function1[A, A] = ???
     |            def foo(x: List[A]): Function2[B, B, B] = ??? // error
     |          }
<console>:15: error: method foo is defined twice;
  the conflicting method foo was defined at line 14:16
                  def foo(x: List[A]): Function2[B, B, B] = ??? // error
                      ^

}
if (decl is Synthetic) doubleDefError(other, decl)
else doubleDefError(decl, other)
Expand Down Expand Up @@ -870,7 +866,7 @@ trait NoChecking extends ReChecking {
override def checkImplicitParamsNotSingletons(vparamss: List[List[ValDef]])(implicit ctx: Context): Unit = ()
override def checkFeasibleParent(tp: Type, pos: Position, where: => String = "")(implicit ctx: Context): Type = tp
override def checkInlineConformant(tree: Tree, what: => String)(implicit ctx: Context) = ()
override def checkNoDoubleDefs(cls: Symbol)(implicit ctx: Context): Unit = ()
override def checkNoDoubleDeclaration(cls: Symbol)(implicit ctx: Context): Unit = ()
override def checkParentCall(call: Tree, caller: ClassSymbol)(implicit ctx: Context) = ()
override def checkSimpleKinded(tpt: Tree)(implicit ctx: Context): Tree = tpt
override def checkNotSingleton(tpt: Tree, where: String)(implicit ctx: Context): Tree = tpt
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1506,7 +1506,7 @@ class Typer extends Namer
// Expand comments and type usecases
cookComments(body1.map(_.symbol), self1.symbol)(ctx.localContext(cdef, cls).setNewScope)

checkNoDoubleDefs(cls)
checkNoDoubleDeclaration(cls)
val impl1 = cpy.Template(impl)(constr1, parents1, self1, body1)
.withType(dummy.termRef)
checkVariance(impl1)
Expand Down
21 changes: 17 additions & 4 deletions compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,10 @@ package dotty.tools
package dotc
package reporting

import core.Contexts.Context
import diagnostic.messages._
import dotty.tools.dotc.core.Flags
import dotty.tools.dotc.core.Flags.FlagSet
import dotty.tools.dotc.core.Contexts.Context
import dotty.tools.dotc.core.Types.WildcardType
import dotty.tools.dotc.parsing.Tokens
import dotty.tools.dotc.reporting.diagnostic.messages._
import org.junit.Assert._
import org.junit.Test

Expand Down Expand Up @@ -1293,4 +1291,19 @@ class ErrorMessagesTests extends ErrorMessagesTest {

assert(ctx.reporter.hasErrors)
}

@Test def typeDoubleDeclaration =
checkMessagesAfter("frontend") {
"""
|class Foo {
| val a = 1
| val a = 2
|}
""".stripMargin
}.expect { (ictx, messages) =>
implicit val ctx: Context = ictx
assertMessageCount(1, messages)
val DoubleDeclaration(symbol, previousSymbol) :: Nil = messages
assertEquals(symbol.name.mangledString, "a")
}
}
133 changes: 133 additions & 0 deletions tests/neg/doubleDefinition.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
trait A
trait B

// test with classes

class Test1 {
def foo(x: List[A]): Function1[A, A] = ???
def foo(x: List[B]): Function2[B, B, B] = ???
// ok, different jvm signature
}

class Test2 {
def foo(x: List[A]): Function1[A, A] = ???
def foo(x: List[B]): Function1[B, B] = ??? // error: same jvm signature
// scalac calls this "have same type after erasure"
}

class Test3 {
// overload with same argument type, but different return types
def foo(x: List[A]): Function1[A, A] = ???
def foo(x: List[A]): Function2[B, B, B] = ??? // error
}

class Test4 {
val foo = 1
def foo = 2 // error
}

class Test4b {
def foo = 2
val foo = 1 // error
}

class Test4c {
def foo = 2
var foo = 1 // error
}

class Test4d {
var foo = 1
def foo = 2 // error
}


// test with traits

trait Test5 {
def foo(x: List[A]): Function1[A, A] = ???
def foo(x: List[B]): Function2[B, B, B] = ???
// ok, different jvm signature
}

trait Test6 {
def foo(x: List[A]): Function1[A, A] = ???
def foo(x: List[B]): Function1[B, B] = ??? // error: same jvm signature
// scalac calls this "have same type after erasure"
}

trait Test7 {
// overload with same argument type, but different return types
def foo(x: List[A]): Function1[A, A] = ???
def foo(x: List[A]): Function2[B, B, B] = ??? // error
}

class Test8 {
val foo = 1
def foo = 2 // error
}

class Test8b {
def foo = 2
val foo = 1 // error
}

class Test8c {
def foo = 2
var foo = 1 // error
}

class Test8d {
var foo = 1
def foo = 2 // error
}

// test method and contructor argument clashing

class Test9(val foo: Int) {
def foo: String // error
}

class Test10(val foo: Int) {
def foo: Int // error
}

abstract class Test11(val foo: Int) {
def foo: String // error
}

abstract class Test12(val foo: Int) {
def foo: Int // error
}

class Test13(var foo: Int) {
def foo: String // error
}

class Test14(var foo: Int) {
def foo: Int // error
}

abstract class Test15(var foo: Int) {
def foo: String // error
}

abstract class Test16(var foo: Int) {
def foo: Int // error
}

// don't error when shadowing

class Test17 {
val foo = 1
def bar() = {
val foo = ""
}
}

// no error when overloading

class Test18 {
def foo(a: A) = 1
def foo(b: B) = 1
}
8 changes: 4 additions & 4 deletions tests/neg/singletonOrs.scala
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
object Test {
def foo: 1 | 2 = 1 // error // error
def bar: 3 | 4 = foo // error // error
def foo: 1 | 2 = 1 // error // error
def bar: 1 = foo
def a: 1 | 2 = 1 // error // error
def b: 3 | 4 = a // error // error
def c: 1 | 2 = 1 // error // error
def d: 1 = a
}