Skip to content

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

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

Closed
SethTisue opened this issue Feb 19, 2023 · 8 comments
Closed

Comments

@SethTisue
Copy link
Member

SethTisue commented Feb 19, 2023

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 SethTisue added the lint label Feb 19, 2023
@SethTisue SethTisue changed the title -Wunused:params gives false negatives when override is missing -Wunused:params gives false negatives when implementing abstract method Feb 19, 2023
@SethTisue
Copy link
Member Author

SethTisue commented Feb 19, 2023

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
Copy link

In scala/scala#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 SethTisue added this to the 2.13.11 milestone Feb 19, 2023
@SethTisue
Copy link
Member Author

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

@SethTisue
Copy link
Member Author

SethTisue commented Feb 19, 2023

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
Copy link

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

@SethTisue SethTisue assigned SethTisue and unassigned som-snytt Feb 20, 2023
@SethTisue SethTisue modified the milestones: 2.13.11, Backlog Feb 20, 2023
@SethTisue
Copy link
Member Author

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 scala/scala3#17185

@SethTisue SethTisue removed their assignment Jun 15, 2023
@SethTisue
Copy link
Member Author

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
Copy link

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.

@som-snytt som-snytt closed this as not planned Won't fix, can't repro, duplicate, stale Oct 17, 2023
@SethTisue SethTisue removed this from the Backlog milestone Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants