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

jendrikw · 2018-03-02T21:37:32Z

The old error message said "definition", but I think I should be called declaration. Please clearify this. I will update the code if needed.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

jendrikw · 2018-03-02T22:00:55Z

It looks like tests/neg/singletonOrs.scala fails. I think this has nothing to do with my changes. Since I'm not super familiar with the build pipeline, can anyone explain to me why the master branch builds ok?

Blaisorblade · 2018-03-03T00:29:15Z

@jendrikw that tests contains double definition, and i suspect the error markers must be updated. I'm new here so I'm not sure why the error count changed—what errors do you get there? You might need to simply add extra //error markers for the extra errors. That these errors didn't appear before and appear now is likely a side effect simply triggered by your changes.

- Error: tests/neg/singletonOrs.scala:2:12 ------------------------------------
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
  |            ^
  |            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
  |                ^
  |                Singleton type Int(2) is not allowed in a union type
6 errors found
java.lang.RuntimeException: Nonzero exit code returned from runner: 1
	at scala.sys.package$.error(package.scala:27)

Blaisorblade · 2018-03-03T00:30:38Z

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

@@ -627,7 +627,7 @@ trait Checking {
            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)


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

Blaisorblade · 2018-03-08T20:36:02Z

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

-              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)


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.

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.

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 ^

jendrikw · 2018-03-09T23:39:55Z

I have tried to improve the error messages by testing for the different cases. Should I put the details in explanation?

Also, in the error I get from running dotc tests/neg/doubleDefinition.scala in the sbt shell, the conflicting definitions are sais to be in line 14 and 26. These numbers come from previousSymbol.pos.line. Is that the right method? 15 and 27 would make more sense to me.

Comments addressed

Blaisorblade · 2018-03-10T13:00:29Z

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

+    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


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.

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)

Blaisorblade · 2018-03-10T13:11:23Z

I have tried to improve the error messages by testing for the different cases. Should I put the details in explanation?

I want to check a few details but that looks much better! Not sure on explanations, I guess/feel it's there to provide relevant Scala lessons, but it is optional.

~~The line numbers confuse me, but IIUC your PR is not changing those positions, it just didn't fix them yet? Because then I'd merge the PR;~~ I'm not yet unqualified to help with positions, and I hear they aren't trivial in Scalac.

EDIT: sorry misunderstood, you're adding line numbers (but they don't work), trying to guess why they don't work...

Blaisorblade · 2018-03-10T13:31:23Z

For extra fun, positions in the REPL appear correct in master:

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

but also

pgiarrusso@tsf-436-wpa-1-015:~/git/dotty$ dotc -d out tmp/Foo.scala
-- Error: tmp/Foo.scala:28:4 -------------------------------------------
28 |	def foo(x: List[B]): Function1[B, B] = ??? // error: same jvm signature
   |	    ^
   |	    method foo is already defined as method foo: (x: List[A]): A => A
   |	    (the definitions have matching type signatures)
one error found

Blaisorblade · 2018-03-10T13:31:39Z

Found it, after quite a tour, in SourcePosition:

  /** The line of the position, starting at 0 */ // <--- _documented_
  def line: Int = source.offsetToLine(point)
  [...]
  override def toString =
    if (source.exists) s"${source.file}:${line + 1}"

I'm afraid you're supposed to add + 1 before printing.

Blaisorblade · 2018-03-10T13:45:14Z

And you're right regarding declaration/definition, since turning definitions into declarations doesn't affect the errors — say:

abstract 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
}

I guess we want both in tests. And BTW we'd also want a test for when matchDegree returns NoMatch.

As a consequence, I'd say decl in checkNoDoubleDefs is named correctly, but checkNoDoubleDefs is misnamed.

Again, you might want to move concerns that feel too hard into separate issues/PRs, or ask for another reviewer (when they're available) — I'm new here, and learning the code with you (reviewing to help others).
And my requests are turning an "easy" PR into a tougher one (though the errors are becoming better).

allanrenucci · 2018-03-11T12:06:26Z

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

+    val msg = {
+      val details = if (decl.isRealMethod && previousSymbol.isRealMethod) {
+        // compare the signatures when both symbols represent methods
+        decl.asTerm.signature.matchDegree(previousSymbol.asTerm.signature) match {


I believe you don't need asTerm

allanrenucci · 2018-03-11T12:07:52Z

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

+      val details = if (decl.isRealMethod && previousSymbol.isRealMethod) {
+        // compare the signatures when both symbols represent methods
+        decl.asTerm.signature.matchDegree(previousSymbol.asTerm.signature) match {
+          case Signature.NoMatch => ""


This should never happen. I would refactor your code as:

match { case Signature.FullMatch => ... case _ /* Signature.ParamMatch */ => ... }

Do you know why this is not an enum or ADT? Was is because of performance reasons? Using Ints is very unidiomatic scala IMO.

allanrenucci · 2018-03-11T12:12:35Z

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

+        decl.asTerm.signature.matchDegree(previousSymbol.asTerm.signature) match {
+          case Signature.NoMatch => ""
+          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."


The definitions have ~~the same~~ matching type signatures after erasure.

allanrenucci · 2018-03-11T12:13:26Z

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

+        // compare the signatures when both symbols represent methods
+        decl.asTerm.signature.matchDegree(previousSymbol.asTerm.signature) match {
+          case Signature.NoMatch => ""
+          case Signature.ParamMatch => "\nOverloads with equal parameter types but different return types are not allowed."


Overloads with matching parameter types are not allowed

allanrenucci · 2018-03-11T12:14:11Z

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

+          case Signature.FullMatch => "\nThe definitions have the same signature after erasure."
+        }
+      } else ""
+      hl"${decl.showLocated} is already defined as ${previousSymbol.showDcl} in line ${previousSymbol.pos.line + 1}." + details


allanrenucci · 2018-03-11T12:16:03Z

tests/neg/singletonOrs.scala

@@ -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


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

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?

(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.

allanrenucci · 2018-03-12T10:52:34Z

tests/neg/singletonOrs.scala

-   def foo: 1 | 2 = 1 // error // error
-   def bar: 1 = foo
+   def foo: 1 | 2 = 1 // error // error // error
+   def bar: 1 = foo // error


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

Blaisorblade · 2018-03-16T22:46:04Z

Waiting for @allanrenucci before merging.

Add error message about double definition (scala#1589)

03e4ab3

dottybot reviewed Mar 2, 2018

View reviewed changes

Blaisorblade self-requested a review March 3, 2018 00:22

Blaisorblade reviewed Mar 3, 2018

View reviewed changes

Jendrik Wenke added 2 commits March 3, 2018 13:56

remove unused methods

06c7a51

fix error counts for tests/neg/singletonOrs

d4455fd

Blaisorblade self-assigned this Mar 3, 2018

Blaisorblade previously requested changes Mar 8, 2018

View reviewed changes

WIP try to improve error messages

aa9ce63

Blaisorblade reviewed Mar 10, 2018

View reviewed changes

Jendrik Wenke added 3 commits March 11, 2018 01:19

fix off by one error when printing line of previously declared symbol

aa803ed

don't show error about signatures when a var/val and a def are clashing

68dcdec

call it declaration consistently

4df133e

allanrenucci approved these changes Mar 11, 2018

View reviewed changes

allanrenucci reviewed Mar 11, 2018

View reviewed changes

Jendrik Wenke added 3 commits March 11, 2018 22:44

remove Signature.NoMatch from cases because it should not happen

b6e1090

remove redundant asTerm

5672fcf

fix grammar and wording

c569c8a

allanrenucci reviewed Mar 12, 2018

View reviewed changes

remove double definition in singletonOr test

e4c64b7

Blaisorblade approved these changes Mar 13, 2018

View reviewed changes

allanrenucci merged commit ad002ff into scala:master Mar 19, 2018

Blaisorblade mentioned this pull request Mar 19, 2018

Avoid spurious "double-def" error message involving erroneous definition #4141

Merged

jendrikw deleted the error-double-definition branch March 20, 2018 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jendrikw commented Mar 2, 2018 •

edited

Loading

dottybot left a comment

jendrikw commented Mar 2, 2018

Blaisorblade commented Mar 3, 2018

Blaisorblade Mar 3, 2018

Blaisorblade Mar 8, 2018

jendrikw Mar 9, 2018

Blaisorblade Mar 9, 2018

jendrikw commented Mar 9, 2018

Blaisorblade Mar 10, 2018

Blaisorblade Mar 10, 2018

Blaisorblade commented Mar 10, 2018 •

edited

Loading

Blaisorblade commented Mar 10, 2018

Blaisorblade commented Mar 10, 2018 •

edited

Loading

Blaisorblade commented Mar 10, 2018

allanrenucci Mar 11, 2018

allanrenucci Mar 11, 2018

jendrikw Mar 11, 2018

allanrenucci Mar 11, 2018

allanrenucci Mar 11, 2018

allanrenucci Mar 11, 2018

allanrenucci Mar 11, 2018

jendrikw Mar 11, 2018

Blaisorblade Mar 12, 2018

allanrenucci Mar 12, 2018

Blaisorblade commented Mar 16, 2018

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

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

Conversation

jendrikw commented Mar 2, 2018 • edited Loading

dottybot left a comment

Choose a reason for hiding this comment

jendrikw commented Mar 2, 2018

Blaisorblade commented Mar 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jendrikw commented Mar 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blaisorblade commented Mar 10, 2018 • edited Loading

Blaisorblade commented Mar 10, 2018

Blaisorblade commented Mar 10, 2018 • edited Loading

Blaisorblade commented Mar 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blaisorblade commented Mar 16, 2018

jendrikw commented Mar 2, 2018 •

edited

Loading

Blaisorblade commented Mar 10, 2018 •

edited

Loading

Blaisorblade commented Mar 10, 2018 •

edited

Loading