In selector check, prefix of reference must match import qualifier

[som-snytt]

Fixes #19657
Fixes #20520
Fixes #19998
Fixes #18313
Fixes #17371
Fixes #18708

[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.

Speculation: The miniphase should be rewritten to leverage existing Context and Scope instead of custom structures. The problem of tracking just symbols is simpler (as in Scala 2), but for import tracking, it should use ImportContext etc. (The other PR for import tracking followed Scala 2 and let the compiler record lookups as they happen.) The motivation for proper contexts is so that a "lint framework" can rely on it, instead of custom traversals. For this phase, top-down may be more efficient than bubbling up references until they find a definition or import. Per context, a reference need be looked up only once, and then only if there are outstanding unused definitions or imports.

[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.