scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

`-Wunused:params` gives false negatives when implementing abstract method #12733

Closed SethTisue closed 11 months ago

SethTisue commented 1 year ago

with this code:

trait C {
  def foo(n: Int): Int
}
trait D extends C {
  def foo(n: Int): Int = "".size
}

we see:

% scala-cli compile -S 3.nightly -Wunused:all .
[warn] ./S.scala:5:11: unused explicit parameter
[warn]   def foo(n: Int): Int = "".size
[warn]           ^
% scala-cli compile -S 2.nightly -Wunused .
%

It seems to me that Scala 3 is correct to warn here and Scala 2 is mistaken not to.

This isn't contrived code; this is minimized from my attempt to enable Scala 3's unused warnings over in scala/scala-parser-combinators.

Vaguely in the same area: #11343.

@som-snytt this will interest you, I think.

SethTisue commented 1 year ago

A strange wrinkle here is that Scala 3 only issues the warning if the override keyword is omitted. The method is an override regardless of whether I explicitly say it, so that seems like inconsistent behavior. I'll wait to report that over in lampepfl/dotty until we've discussed here, in case there's some aspect of the design I'm failing to grok (here, and/or on #11343, where I've also commented).

som-snytt commented 1 year ago

In https://github.com/scala/scala/pull/10305 I added -Xlint:non-strict to enable the heuristics that reduce warnings. The "reduced warnings" would be enabled by -Xlint (but you could turn it off), but not enabled by -Wunused:params alone.

In this case, the heuristic is don't warn when the param is required by a superinterface.

As usual, some people prefer "strict" for "always warn me, it's never OK even for trivial methods".

I have not kept all my fan mail to actually count the votes for which heuristics are most favored. So the new option returns the power to the people.

Another heuristic is for def f(x: Int) = ??? which is obviously a placeholder.

SethTisue commented 1 year ago

ah, interesting. okay, maybe that one will land at some point

SethTisue commented 1 year ago

I guess the only thing I need to report to the Scala 3 folks is the presence or absence of override making a difference, though perhaps that was somehow a design decision too 🤷

som-snytt commented 1 year ago

I recently learned that override is optional if there is a self-type with a matching definition.

SethTisue commented 1 year ago

Scala 3 stopped warning sometime between 3.3.0-RC3 and 3.3.0-RC4:

% scala-cli compile -S 3.3.0-RC3 .
Compiling project (Scala 3.3.0-RC3, JVM)
[warn] ./S.scala:8:11
[warn] unused explicit parameter
[warn]   def foo(n: Int): Int = "".size
[warn]           ^
Compiled project (Scala 3.3.0-RC3, JVM)
% scala-cli compile -S 3.3.0-RC4 .
Compiling project (Scala 3.3.0-RC4, JVM)
Compiled project (Scala 3.3.0-RC4, JVM)

and intentionally so, judging from the discussion on https://github.com/lampepfl/dotty/issues/17185

SethTisue commented 1 year ago

so arguably this ticket could be closed, since Scala 2 is now in line with Scala 3 here. as you see fit, @som-snytt

som-snytt commented 11 months ago

Since Scala 3 has caught up with Scala 2, I'll close the ticket.

The behavior is intended -- don't warn about param required by inherited signature -- but the caveats to ticket closure are that it's a dumb heuristic because @unused (did it predate the annotation, etc) and two, we'd like to make the "non-strict" heuristics opt-in (or maybe opt-out). The option was removed from the linked PR, see above.