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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
03e4ab3
Add error message about double definition (#1589)
Mar 2, 2018
06c7a51
remove unused methods
Mar 3, 2018
d4455fd
fix error counts for tests/neg/singletonOrs
Mar 3, 2018
aa9ce63
WIP try to improve error messages
Mar 9, 2018
aa803ed
fix off by one error when printing line of previously declared symbol
Mar 11, 2018
68dcdec
don't show error about signatures when a var/val and a def are clashing
Mar 11, 2018
4df133e
call it declaration consistently
Mar 11, 2018
b6e1090
remove Signature.NoMatch from cases because it should not happen
Mar 11, 2018
5672fcf
remove redundant asTerm
Mar 11, 2018
c569c8a
fix grammar and wording
Mar 11, 2018
e4c64b7
remove double definition in singletonOr test
Mar 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.java
View file
Open in desktop
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
13 changes: 13 additions & 0 deletions compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2079,4 +2079,17 @@ object messages {
}
val explanation = ""
}

case class DoubleDeclaration(decl: Symbol, previousSymbol: Symbol)(implicit ctx: Context) extends Message(DoubleDeclarationID) {
val kind = "Duplicate Symbol"
val msg = {
val details = decl.asTerm.signature.matchDegree(previousSymbol.asTerm.signature) match {
case Signature.NoMatch => "" // matchDegree also returns NoMatch if one of the terms is not a method
Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds plausible, but I'm not sure how it relates to realMethod and why that code is there — not within my "review budget" I fear. I really want to approve this PR but I'm not sure it's not introducing regressions. Finding another reviewer.

Copy link
Contributor

Choose a reason for hiding this comment

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

OTOH, not sure isRealMethod worked anyway: for abstract class A(val a: Int) { def a: String } on master I get:

scala> abstract class A(val a: Int) { def a: String }
1 |abstract class A(val a: Int) { def a: String }
  |                                   ^
  |                           method a is already defined as value a: Int
  |                           (the definitions have matching type signatures)

case Signature.ParamMatch => "\nOverloads with equal parameter types but different return types are not allowed."
case Signature.FullMatch => "\nThe definitions have the same signature after erasure."
}
hl"${decl.showLocated} is already defined as ${previousSymbol.showDcl} in line ${previousSymbol.pos.line}." + details
}
val explanation = ""
}
}
6 changes: 1 addition & 5 deletions compiler/src/dotty/tools/dotc/typer/Checking.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -623,11 +623,7 @@ trait Checking {
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.

maybe keep using explanation in the DoubleDeclaration class, or move def explanation there?

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
21 changes: 17 additions & 4 deletions compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTests.scala
View file
Open in desktop
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")
}
}
29 changes: 29 additions & 0 deletions tests/neg/doubleDefinition.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
trait A
trait B

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
}
4 changes: 2 additions & 2 deletions tests/neg/singletonOrs.scala
View file
Open in desktop
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 foo: 1 | 2 = 1 // error // error // error
Copy link
Contributor

Choose a reason for hiding this comment

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

@smarter This is weird, the change seems unrelated. You might want to look into this

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure about the internals, but I'm guessing that the error about singleton types in unions are from the typer phase and the compiler exists afterwards. But that wouldn't explain the different behaviour in this branch.

Manually compiling tests/neg/singletonOrs.scala gives

2 |   def foo: 1 | 2 = 1 // error // error
  |            ^
  |            Singleton type Int(1) is not allowed in a union type
-- Error: tests/neg/singletonOrs.scala:2:16 ------------------------------------
2 |   def foo: 1 | 2 = 1 // error // error
  |                ^
  |                Singleton type Int(2) is not allowed in a union type
-- Error: tests/neg/singletonOrs.scala:3:12 ------------------------------------
3 |   def bar: 3 | 4 = foo // error // error
  |            ^
  |            Singleton type Int(3) is not allowed in a union type
-- Error: tests/neg/singletonOrs.scala:3:16 ------------------------------------
3 |   def bar: 3 | 4 = foo // error // error
  |                ^
  |                Singleton type Int(4) is not allowed in a union type
-- Error: tests/neg/singletonOrs.scala:4:12 ------------------------------------
4 |   def foo: 1 | 2 = 1 // error // error // error
  |            ^
  |            Singleton type Int(1) is not allowed in a union type
-- Error: tests/neg/singletonOrs.scala:4:16 ------------------------------------
4 |   def foo: 1 | 2 = 1 // error // error // error
  |                ^
  |                Singleton type Int(2) is not allowed in a union type
-- [E119] Duplicate Symbol Error: tests/neg/singletonOrs.scala:4:7 -------------
4 |   def foo: 1 | 2 = 1 // error // error // error
  |       ^
  |method foo in object Test is already defined as def foo: => 
  |  <error Singleton type Int(1) is not allowed in a union type> | 
  |    <error Singleton type Int(2) is not allowed in a union type> at line 2.
  |The definitions have matching type signatures after erasure.
-- [E119] Duplicate Symbol Error: tests/neg/singletonOrs.scala:5:7 -------------
5 |   def bar: 1 = foo // error
  |       ^
  |method bar in object Test is already defined as def bar: => 
  |  <error Singleton type Int(3) is not allowed in a union type> | 
  |    <error Singleton type Int(4) is not allowed in a union type> at line 3.
  |Overloads with matching parameter types are not allowed.

I think the last two error messages are very confusing. The previous error is displayed when previousDecl.showDcl is called. That seemed like a nice way to show the conflicting declaration. Is there an alternative?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Until you get more competent answers) If showDcl is the proper API (as I'm guessing), my ignorant vote is to stick to showDcl and maybe open an issue to improve its output.

On the error count, I suspect it's some side effect of how errors are combined/filtered—but not sure.

def bar: 1 = foo // error
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I think a better solution is to rename the last two methods. This test is not meant to test duplicate definition

}