Skip to content

No warning for parameter of overriding method #22757

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

Merged
merged 1 commit into from
Mar 11, 2025

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Mar 9, 2025

Quiet the unused parameter warning when the method is an override.

Fixes #22742

@som-snytt som-snytt marked this pull request as ready for review March 10, 2025 01:58
@Gedochao Gedochao requested review from sjrd and KacperFKorban March 10, 2025 08:46
@Gedochao
Copy link
Contributor

@som-snytt what's the reasoning behind the option being -Y instead of -W or -X?
If we consider this a fix for #22742, I wonder if we shouldn't have something with proper guarantees (which -Y private flags don't have).

I'd rather have smth like -Wunused-heuristics down the road. We could also leave the ticket open and stabilise this until 3.8, at which time we'd rename it to -W/-X?

@som-snytt
Copy link
Contributor Author

@Gedochao I think a "fix" or workaround (for the expectation that it doesn't warn when overriding) is useful for 3.7. (I like that this is a small delta in LOC.)

Experience suggests that a "false positive" or noise makes a warning unusable.

OTOH, altering settings (names or behaviors) makes cross-compilation harder. Maybe that is less of a concern in LTS world?

Currently, we're stuck with -Wunused:strict-no-implicit-warn,unsafe-warn-patvars because it's hard to take away an option; the words are baked in stone.

If a -Y is deemed terrible by the team, the alternative is not to offer an option (at least not yet), but default to "fewest warnings".

I didn't do that because I wanted to make progress on "strict" mode; this is a gray area because offering too many "knobs" is counterproductive: it's awkward to configure the build, and only the typelevel plugin handles the "matrix" of options.

I just talked myself into "default to fewest warnings." Am I permitted to offer -Y for those mavericks who want more warnings?

@som-snytt
Copy link
Contributor Author

Edit: I will push without -Y so that we don't need to have that conversation. This fix will just restore "don't warn when overriding".

@som-snytt som-snytt force-pushed the issue/22742-heuristics-override branch from e3005f0 to f25adb9 Compare March 10, 2025 15:03
@som-snytt som-snytt changed the title -Yunused:strict,fewer,fewest for heuristics No warning for parameter of overriding method Mar 10, 2025
@sjrd sjrd enabled auto-merge March 10, 2025 15:20
@som-snytt
Copy link
Contributor Author

Of course I must update other check files, because the "no warning" is unconditional now.

@som-snytt
Copy link
Contributor Author

som-snytt commented Mar 10, 2025

Example where I would say this rule is suboptimal.

override def equals(other: Any) = toString.nonEmpty
  • "But I don't control the signature!"
  • "What if I want to ignore the parameter!"

I think it must be written def equals(@unused other: Any). That is not burdensome, and is shorter than a code comment. I don't have to wonder if it's a typo.

The "trivial RHS" check tries to cover "obvious that parameters are not used", such that it's not likely to be human error. However, if start coding with a "default" RHS (that does not warn), what if I "forget" to improve it. (Presumably, a test is also lacking.)

"Dev" mode could be lax, "Integration/C.I." mode could be strict.

Similarly def equals(other: Any) = true // subclasses must override. Maybe in Scala 3 you can make it abstract (deferred)? for that use case.

@som-snytt som-snytt force-pushed the issue/22742-heuristics-override branch from f25adb9 to 825baf9 Compare March 10, 2025 17:56
@sjrd sjrd merged commit fe982db into scala:main Mar 11, 2025
26 checks passed
@som-snytt som-snytt deleted the issue/22742-heuristics-override branch March 11, 2025 14:22
@WojciechMazur WojciechMazur added this to the 3.7.0 milestone Mar 11, 2025
sjrd added a commit that referenced this pull request Apr 3, 2025
Fixes #22895 
Fixes #22896 
Follow-up to #22729 
Follow-up to #22757 

Since Scala 2, the check for unused parameters does not warn for an
overriding method, since the signature is dictated by the overridden
method. That behavior was intentionally omitted in 3.7 because it was
introduced in Scala 2 before `@nowarn` and `@unused` were available.
(The warning would be too noisy without mitigation.) A compiler option
for these heuristics was tried in a previous commit, but reverted.
(Compiler options are difficult to evolve once added.) So these commits
restore this heuristic without further ado. An option for "strict mode"
without allowances is future work.

The other issue was that detecting an override in a Java parent did not
work. The commit always checks `is(Override)` before searching for an
overridden member, since it is not necessary to verify the modifier
(which happens in RefChecks). Incorrect overriding would result in false
negatives, which is acceptable since it will error later.

Further asymmetry between checking implicit params and explicit class
param accessors may or may not be dubious.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive unused warnings in 3.7 nightly on method overrides
5 participants