Skip to content

False positive unused warnings in 3.7 nightly on method overrides #22742

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
mrdziuban opened this issue Mar 7, 2025 · 10 comments · Fixed by #22757
Closed

False positive unused warnings in 3.7 nightly on method overrides #22742

mrdziuban opened this issue Mar 7, 2025 · 10 comments · Fixed by #22757
Assignees
Labels
area:linting Linting warnings enabled with -W or -Xlint itype:bug
Milestone

Comments

@mrdziuban
Copy link

Compiler version

3.7.0-RC1-bin-20250306-73ba485-NIGHTLY

Minimized code

With -Wunused:explicits enabled:

trait Foldable[F[_]] {
  def foldLeft[A, B](fa: F[A], b: B)(f: (B, A) => B): B
}

type Id[A] = A

given foldableId: Foldable[Id] =
  new Foldable[Id] {
    def foldLeft[A, B](fa: Id[A], b: B)(f: (B, A) => B): B = b
  }

Output

-- [E198] Unused Symbol Warning: /Users/matt/scala3.7-nightly-unused/src/main/scala/example/Test.scala:9:23
9 |    def foldLeft[A, B](fa: Id[A], b: B)(f: (B, A) => B): B = b
  |                       ^^
  |                       unused explicit parameter
-- [E198] Unused Symbol Warning: /Users/matt/scala3.7-nightly-unused/src/main/scala/example/Test.scala:9:40
9 |    def foldLeft[A, B](fa: Id[A], b: B)(f: (B, A) => B): B = b
  |                                        ^
  |                                        unused explicit parameter

Expectation

The parameters should not be reported unused since they're necessary to satisfy the method override

@mrdziuban mrdziuban added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 7, 2025
@som-snytt
Copy link
Contributor

som-snytt commented Mar 7, 2025

This change was intentional, as there was negative feedback about the heuristics in Scala 2.

I did not have a plan yet for gathering input, so I'll do that on this ticket. If the heuristic is desirable, should it be under a compiler option? Should all the heuristics (such as don't warn if the RHS is ???) be under a flag? and what is the default?

As a footnote, to the best of my memory, the heuristics preceded the introduction of @unused and @nowarn and -Wconf, when a lint for unused parameters was deemed to be impossibly noisy.

My experiment with this code base suggested just adding @unused to unused elements (rather than using brittle heuristics, which can be head-scratchers as to whether they apply).

@mrdziuban
Copy link
Author

re: the heuristics, I would love to see more consistency with them, or at least some documentation of them. For example, why doesn't this similar code produce a warning?

trait Functor[F[_]] {
  def map[A, B](fa: F[A])(f: A => B): F[B]
}

type Never[A] = Unit

given functorNever: Functor[Never] =
  new Functor[Never] {
    def map[A, B](fa: Never[A])(f: A => B): Never[B] = ()
  }

I'm guessing it's because the RHS is considered "trivial" (related: #16640) but it doesn't seem any less trivial to me than the first example -- they're both a single expression

@som-snytt
Copy link
Contributor

@mrdziuban Thanks, that is an example of the DX I'd like to avoid.

"Obviously" I did not remove all the heuristics, but I don't know yet how to offer "quiet" warnings.

A similar heuristic is that returning a single arg is "trivial".

The original ticket to restore the exclusion for overrides is #16865
There is an existing test.

Possible feature goal:

  1. don't warn because I'm in the middle of coding the implementation (which is why I wrote ???)
  2. don't warn because this implementation is obviously a trivial default intended to be overridden in subclasses
  3. don't warn because I don't control the obsolete signature of the overridden method

I think 2&3 deserve annotations. For 1, I think the ideal is "gradual warnings": I need it to warn before integration testing, and hopefully before I create a PR, but not when I save the source file.

@som-snytt
Copy link
Contributor

som-snytt commented Mar 7, 2025

It's also desirable to avoid excessive "config". That is good for scalafix but not for the compiler. Maybe this ticket is really: "Lint should be finally moved out of the compiler into Abide Scalafix."

@som-snytt
Copy link
Contributor

Minimally, -Yunused:help could explain available heuristics (as a form of documentation).

Hypothetical -Yunused:-override,-unimplemented,-trivial to "turn off" certain heuristics.

@mrdziuban
Copy link
Author

Hypothetical -Yunused:-override,-unimplemented,-trivial to "turn off" certain heuristics.

Something like that level of granularity sounds ideal to me.

Also just wanted to say thank you for all the work you're putting into linting! I'm testing the 3.7 nightly releases against my large codebase and there are a ton of new, valid unused warnings on imports and parameters that I'm very happy to get rid of.

@som-snytt
Copy link
Contributor

OK I will learn from @lrytz who did this granularity with -Xsource:3. I will try to level up.

@som-snytt
Copy link
Contributor

The PR is quite simple and not granular, in hopes that it can make the cut-off for 3.7.0.

@Gedochao Gedochao added area:linting Linting warnings enabled with -W or -Xlint and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 10, 2025
@Gedochao
Copy link
Contributor

The PR is quite simple and not granular, in hopes that it can make the cut-off for 3.7.0.

The cut-off is tomorrow, in case it gets delayed, I'll tag @WojciechMazur so that we remember to potentially backport this in RC.

@som-snytt
Copy link
Contributor

The PR does not add an option, but just silences the warning in question.

A future version can acquire fancy knobs and levers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:linting Linting warnings enabled with -W or -Xlint itype:bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants