Skip to content
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

Spurious, positionless overrides-nullary-method warning when extending under separate compilation #12851

Closed
SethTisue opened this issue Aug 24, 2023 · 7 comments · Fixed by scala/scala#10509
Assignees
Milestone

Comments

@SethTisue
Copy link
Member

SethTisue commented Aug 24, 2023

Scala version: 2.13.11

T.scala

trait T1 {
  def method: Int
}
trait T2 extends T1 {
  def method() = 0
}

C.scala

class C extends T2

Reproduction steps

scalac T.scala
scalac -classpath . C.scala

Result

compiling T.scala produces a correct warning:

T.scala:5: warning: method with a single empty parameter list overrides method without any parameter list
  def method() = 0
      ^
1 warning

but then compiling C.scala produces this:

warning: method with a single empty parameter list overrides method without any parameter list
1 warning

we shouldn't be issuing this warning at all IMO, but also note the lack of position information, which means it's difficult to figure out where it's even coming from

Context

This came up over at scala/scala#10484 , where after a recent change for 2.13.12 the attempt to issue the warning actually crashes the compiler.

@SethTisue
Copy link
Member Author

@som-snytt @lrytz You two want to race to see who fixes it first...?

History: the warning originally landed in scala/scala#8846 (Scala 2.13.3) and then it was moved from namers to refchecks in scala/scala#10345 (Scala 2.13.11).

@som-snytt
Copy link

Thanks for the repro! I'll take a look, as I must refresh my memory for namePos of valdef and the Giants aren't playing today.

@som-snytt
Copy link

I'm glad I fixed crasher handling in partest at some point.

?! 1 - neg/t12851                                [caught UnsupportedOperationException - Position.point on NoPosition]

@SethTisue
Copy link
Member Author

SethTisue commented Aug 25, 2023

I thought the same problem might exist for nullary-overrides-nilary ("method without a parameter list overrides a method with a single empty one"), but it doesn't reproduce that way 'round 🤔

@som-snytt
Copy link

s/spurious/scurrilous

@lrytz
Copy link
Member

lrytz commented Aug 25, 2023

➜ sandbox scv 2.13.12-bin-fa1c9eb A.scala B.scala
A.scala:5: warning: method with a single empty parameter list overrides method without any parameter list
  def method() = 0
      ^
1 warning


➜ sandbox scv 2.13.12-bin-910c59d A.scala B.scala
A.scala:5: warning: method with a single empty parameter list overrides method without any parameter list [quickfixable]
  def method() = 0
      ^
A.scala:5: warning: method with a single empty parameter list overrides method without any parameter list
  def method() = 0
      ^
2 warnings

so the second warning started showing up with scala/scala#10484.

I think before we only got a single warning because the second one was filtered by the FilteringReporter. After that PR, the two warnings no longer have the same message because one has [quickfixable], but not the other. So they are both shown.

@som-snytt
Copy link

The forthcoming PR addresses both when to warn for the override and the position to warn at. (The symbol position should be tree.namePos.)

The reason the inverse case (def m overrides def m()) does not warn for separate compilation is that the parens are injected by namer; a tree attachment enables warning. In separate compilation, there is no attachment and both signatures have parens.

I haven't looked at the double-warning question yet, but I remember that the "extended explanation" feature (which I remove in a current PR for 2.13.13) had the same problem. That is where a message text could have a "fold line":

Bad thing!
----
Let me explain more...

where you'd see the long form only once, and only the top half after that. The short form was registered with the filtering reporter. (I think that feature is nicer than re-run with -explain-more for more explanation, but it was not adopted.)

If actionable messages may/may not have an addendum, maybe the addendum should be stripped before registration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants