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

Regression for -Wunused:import in business4s/decisions4s #21420

Closed
WojciechMazur opened this issue Aug 23, 2024 · 3 comments · Fixed by #20894 or #22314
Closed

Regression for -Wunused:import in business4s/decisions4s #21420

WojciechMazur opened this issue Aug 23, 2024 · 3 comments · Fixed by #20894 or #22314
Assignees
Labels
area:linting Linting warnings enabled with -W or -Xlint itype:bug regression This worked in a previous version but doesn't anymore
Milestone

Comments

@WojciechMazur
Copy link
Contributor

Compiler version

3.5.0 and later
Bisect points to 975df4a

Minimized code

//> using options -Wunused:imports -Werror
object decisions4s{
  trait HKD
  trait DecisionTable
}

object DiagnosticsExample {
  import decisions4s.HKD
  case class Input[F[_]]() extends HKD
  import decisions4s.*
  val decisionTable: DecisionTable = ??? // warn
}

Output

[warn] ./test.scala:8:22
[warn] unused import
[warn]   import decisions4s.HKD
[warn]                

Expectation

Should not report warnings if wildcard import is placed after usages of explicit import statement.

@WojciechMazur WojciechMazur added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label area:linting Linting warnings enabled with -W or -Xlint regression This worked in a previous version but doesn't anymore and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Aug 23, 2024
@som-snytt
Copy link
Contributor

som-snytt commented Aug 23, 2024

I have an improvement PR and also a branch for a rewrite of the unused check, where I noticed that the scoping in "super position" is not correct:

object Constants:
  final val i = 42

class `scope of super`:
  import Constants.i // bad warn
  class C(x: Int):
    def y = x
  class D extends C(i):
    import Constants.* // does not resolve i in C(i)
    def m = i

confirming with current scalacQ

➜  snips ~/projects/dotty/bin/scalacQ -d /tmp/sandbox -Wunused:imports import-scope.scala
-- [E198] Unused Symbol Warning: import-scope.scala:5:19 -----------------------
5 |  import Constants.i // bad warn
  |                   ^
  |                   unused import
1 warning found

I don't think the crude scoping in the check could ever quite work; if it is a regression here in the May improvements, I assume that is because more is correctly detected.

By "crude", I mean that typer contexts are approximated.

I noticed it (on my rewrite branch, which doesn't go far) by turning on the lint in the dotty build, i.e., dogfooding.

@som-snytt
Copy link
Contributor

I'll try to come up with a scheme for handling the import contexts. I devised a kludge for superclass constructor context; either I will kludge again, or switch to Context for scoping, a change I assume is inevitable.

@som-snytt
Copy link
Contributor

Duplicate of #17667 where the bookkeeping mechanism does not understand normal contexts (and import contexts).

KacperFKorban added a commit that referenced this issue Jan 13, 2025
…hen collecting them in CheckUnused (#22314)

Also make the wildcard selectors exclusion-aware in CheckUnused

closes #21420
@WojciechMazur WojciechMazur added this to the 3.6.4 milestone Jan 16, 2025
bracevac pushed a commit to dotty-staging/dotty that referenced this issue Jan 19, 2025
…hen collecting them in CheckUnused (scala#22314)

Also make the wildcard selectors exclusion-aware in CheckUnused

closes scala#21420
sjrd added a commit that referenced this issue Jan 28, 2025
…20894)

This PR changes the `CheckUnused` phase to rely on the `MiniPhase` API
(instead of custom traversal). That improves fidelity to `Context`
(instead of approximate scoping).

The phase should work seamlessly with subsequent linting phases
(currently, `CheckShadowed`).

It is a goal of the PR to eliminate false reports. It is also a goal not
to regress previous work on efficiency.

A remaining limitation of the current approach is that contexts don't
provide a nesting level. Practically, this means that for a wildcard
import nested below a higher precedence named import, the wildcard is
deemed "unused". (A more general tool for "managing" or "formatting"
imports could do more to pick a preferred scope.)

This PR adds `-Wunused:patvars`, as forward-ported from Scala 2: it
relies on attachments for some details about desugaring, but otherwise
uses positions (where only the original patvar has a non-synthetic
position).

As in Scala 2, it does not warn about patvars with the "canonical" name
of a case class element (this is complicated by type tests and the
quotes API); other exclusions are to be ported, such as "name derived
from the match selector".

Support is added for `-Wconf:origin=full.path.selector`, as in Scala 2.
That allows, for example:
```
-Wconf:origin=scala.util.chaining.given:s
```
to exclude certain blessed imports from warnings, or to work around
false positives (should they arise).

Support is added to `-rewrite` unused imports. There are no options to
"format"; instead, textual deletions preserve existing formatting,
except that blank lines are removed and braces removed when there is
only one selector.

Notable fixes are to support `compiletime` and `inline`; there are more
fixes to pursue in this area.

Fixes #19657
Fixes #20520
Fixes #19998
Fixes #18313
Fixes #17371
Fixes #18708
Fixes #21917
Fixes #21420
Fixes #20951
Fixes #19252
Fixes #18289
Fixes #17667
Fixes #17252
Fixes #21807
Fixes #17753
Fixes #17318
Fixes #18564
Fixes #22376
Fixes #21525
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 regression This worked in a previous version but doesn't anymore
Projects
None yet
4 participants