In selector check, prefix of reference must match import qualifier

[som-snytt]

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.

The commits are not organized around these changes; commits are preserved here just for comparison to previous art, so that useful existing behaviors do not regress.

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

[som-snytt]

Further tweaks forthcoming.

[Gedochao]

@som-snytt how much work do you think it still requires?
It seems we might want to backport the fix for #20860 to 3.5.0 at the very least...
do you need any help?

[sjrd]

The first commit is a bit suspicious, and might be the cause of the CI failures.

The second commit looks good and might pass more easily if submitted as an independent PR.

[som-snytt]

I'll submit commit 2 separately, as I was about to do except it's one line.

I'll follow up the fix for commit 1 after the American holiday.

[som-snytt]

The previous test fails were because the new prefix check (in isInImport) doesn't apply when isDerived and also for top-level defs where the prefix becomes a package object (not sure if that =:= test should work, or if there's a convenient idiom, or if the check is even useful). The derived check uses dealiased types, not sure if a better mechanism is warranted.

I'll clean up and also look for improvements.

[som-snytt]

Rebased and split out commits for easier review.

This includes adding the prefix type to Usage; simplifying gathering the warnings in getUnused, avoiding intermediate collections and extra sorting; mild refactors for readability.

This is the July commit and not the August rewrite, which fixes scope bugs by leveraging normal miniphase traversal and avoiding the special traverser; which allows putting CheckUnused in a single megaphase with CheckShadowing, so that is worth returning to. I'll do that if this PR of limited scope receives a review.

Example extra test fixed in August:

object Constants:
  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

As the comment reminds me, it incorrectly detects the superconstructor ref as resolved by the nested import.

Also note that the inner wildcard is not ambiguous because both imports resolve to the same symbol. But a further improvement would be to warn that the inner import is, if not unused, then spurious.

Edit: redrafting to add the August rewrites.

[som-snytt]

Cherry-picked some old commits.

As a reminder to future self, the mega phase was broken because CheckShadowing sees the nested X as shadowing.

class F[X, M[N[X]]]

The fix [sic] is to ignore everything nested because it ignores constructor-owned X and M. [Previous attempt had been to short-circuit transformDeep so CheckShadowing doesn't see subtrees of "other" trees.]

The fix [also sic] to superclass context noted in previous comment is also novel: since all the "context" approximation is for the purpose of import tracking, it doesn't need to capture all the subtlety of a true super class constructor context (with class parameters in scope). Instead, just detect that a reference occurred in a parent tree, and then kick it upstairs in popScope.

The mechanism for tracking derives is entirely reworked.

The "inner traverser" is removed in favor of matching "other" trees and transforming their parts. I see there was some long discussion about this.

[som-snytt]

warn/unused-privates has NO warn comments to revisit.

[som-snytt]

I'll follow up the fix for commit 1 after the American holiday.

That is Thanksgiving Day shortly after the national election, of course.

[sjrd]

Don't hesitate to ping me when you would like me to take another look. I noticed you were still actively making changes, so I didn't pay attention to every push.

[som-snytt]

I see I was in the middle of adding commits a month ago and did not implement the speculation from my previous comment.

My brain glazes over when I look at this PR. Also I have to re-read "getting started as a contributor" to understand "testing your changes". But I see this PR includes showing vulpix results more nicely.

To reduce the heuristics which are dubious or less-strongly motivated, this reverts #16865 and warns in overrides. The original motivation in Scala 2 was that an override is constrained in its signature and shouldn't be required to use every parameter. Maybe that dates from before @unused, which should be used to indicate an unused parameter.

This keeps the "trivial method" heuristic: don't warn on

def f(x: Int) = ???
def f(x: Int) = 42

however, it ought to be possible to audit suppressed warnings (of all kinds and mechanisms).

[som-snytt]

@sjrd thanks I have no love for this code or US politics, but this PR seems to address some functional issues.

The PR embraces the miniphase API, but maybe that is the wrong direction.

The phase is really a recheck: given context and scopes as at typer, it should track definitions and imports (things that introduce names) and usages. Per scope, it only needs to look up a reference once (to see if it is satisfied by a def or import). I could try that out as a follow-up to see if it is feasible.

The weaknesses of the current approach are that it doesn't model scopes precisely and isn't efficient (references bubble up the stack, including uninteresting refs such as Object).

[som-snytt]

There are pos tests that check for "no warnings" using -Werror as in Scala 2 partest, but should be under warn which will check that it produces zero warnings.

[som-snytt]

I didn't get around yet to handling constructor contexts when I worked around superconstructor contexts. #21917

Note to self: don't neglect secondary constructors, which are lexically nested but typechecked like primary ctor in outer context.

[som-snytt]

That link is a reminder to add origin to diagnostics.

• -Wunused:imports should specify origin of diagnostic (fully-qualified selector string representation) #21807

More reminders:

• -Wunused false positive on compiletime usage #21805 compiletime awareness
• -Wunused:imports falsely reports imported givens (for derivation) #18564 erasedValue, summonInline
• false-positive "unused import" warning #21525 TypeTest
• False unused import with -Wunused:imports #19912 test
• -Wunused:locals gives false positives for some calls to transparent inline functions #17753 transparent inline without dependency on prefix -> unused stabilizer
• WarnUnused false positive when imported literal integer is narrowed #17318 importing member of literal type, plus narrowing

Adjacent:

• Nothing warns about unconsumed values in for comprehension #18854 underscore assignment in for, warn like nonunit-statement
• -Wunused:imports causes an error on methods that come from an external library #17394 test unrelated fix
• unhandled exception while running checkUnusedPostInlining  #18674 another test for unrelated fix
• Ensure lint warnings are configurable via nowarn #18653 nowarn
• -Wunused:nowarn falsely reports that an annotation does not suppress any warnings #18341 nowarn
• -Wunused ignores annotations extended from unused #17378 subclassing unused