In selector check, prefix of reference must match import qualifier

[som-snytt]

Fixes #19657 Fixes https://github.com/scala/scala3/issues/20520 Fixes https://github.com/scala/scala3/issues/19998 Fixes https://github.com/scala/scala3/issues/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?

[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 https://github.com/scala/scala3/issues/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. https://github.com/scala/scala3/issues/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.

• [x] https://github.com/scala/scala3/issues/21807

More reminders:

• [ ] https://github.com/scala/scala3/issues/21805 compiletime awareness
• [x] https://github.com/scala/scala3/issues/18564 erasedValue, summonInline
• [x] https://github.com/scala/scala3/issues/21525 TypeTest
• [x] https://github.com/scala/scala3/issues/19912 test
• [x] https://github.com/scala/scala3/issues/17753 transparent inline without dependency on prefix -> unused stabilizer
• [x] https://github.com/scala/scala3/issues/17318 importing member of literal type, plus narrowing

Adjacent:

• [ ] https://github.com/scala/scala3/issues/18854 underscore assignment in for, warn like nonunit-statement
• [x] https://github.com/scala/scala3/issues/17394 test unrelated fix
• [x] https://github.com/scala/scala3/issues/18674 another test for unrelated fix
• [x] https://github.com/scala/scala3/issues/18653 nowarn
• [x] https://github.com/scala/scala3/issues/18341 nowarn
• [ ] https://github.com/scala/scala3/issues/17378 subclassing unused

[som-snytt]

squashed because couldn't be bothered to rebase the conflict. I saved a branch in case.

Still need to fix the unpickle test failure. Then I will book the progress to avoid further conflicts. More accommodations are needed for compiletime.

[som-snytt]

This breaks the unpickler test, or rather this is what unbreaks it:

--- a/compiler/src/dotty/tools/dotc/Compiler.scala
+++ b/compiler/src/dotty/tools/dotc/Compiler.scala
@@ -51,7 +51,7 @@ class Compiler {
     List(new Staging) ::            // Check staging levels and heal staged types
     List(new Splicing) ::           // Replace level 1 splices with holes
     List(new PickleQuotes) ::       // Turn quoted trees into explicit run-time data structures
-    List(CheckUnused.PostInlining()) ::  // Check for unused elements
+    List(new CheckUnused.PostInlining) ::  // Check for unused elements
     Nil

I don't have bandwidth to investigate why.

[som-snytt]

@sjrd This is "ready for review." I've addressed all your concerns from July 3.

PR is squashed, but previous work is mostly of archaeological interest.

resolveUsage is the logic for deciding whether a name was obtained from an import or definition. (That includes the fix in the PR title.)

It walks Context owners like a name look-up, but attempts some caching at certain contexts. That is for correctness and hopefully efficiency (but I haven't tested performance yet).

RefInfos contains the import, definitions, and references that are collected. It omits the stacks for scoping from the previous implementation.

It attempts to track nested Inlined call site positions: resolveUsage happens if the current tree is at a call pos, or if it looks like summonInline (by its span of zero extent). It also counts inline def to avoid warning there; a new option (to be done) will enable linting inline defs. This is similar to Scala 2, where macro expansions "consume" elements but don't introduce new warnings; but an option allows linting before or after expansion.

There are various tiresome mechanisms which are expected to just work without tweaking options.

Another principle is to minimize surface area with other components; this is restricted to a few attachments.

A few minor clean-ups remain, including ensuring that compiletime doesn't warn (because information is elided early). I speculated that it might be useful to run again after erasure. Avoiding false positives is a high priority, but also inline and compiletime (or let's just say language features) should have linting support. I haven't verified that lints are amenable to suppression; there are tickets for nowarn; but suppression is deemed a last resort.

There are 100 LOC for deleting unused imports which should be moved to a cupboard, as it is supplementary, to be brought out on special occasions; it's a port from Scala 2.

[som-snytt]

@sjrd I appreciate the review. I have addressed the tweaks and also a first refactor of the warnings loop, and also a refactor of resolveUsage.

The cache logic was flawed: now it caches the Precedence of the result. Otherwise, a subsequent look-up may have a more deeply nested result and doesn't know whether to keep it. (Contexts are ordered but don't bear nesting levels, so the highest precedence candidate wins.)

I will add commits for further accommodation of compiletime and inline, but do not plan to squash.

You may wish to wait for more tests, or review at your leisure. I'll ping you again if I've exhausted my resources.

[som-snytt]

I commented that -Werror spoils the output of tests/warn, but that is not true (at least, as of now):

Compilation failed for: tests/warn/i21805.scala
-> following the diagnostics:
 at 0: No warnings can be incurred under -Werror (or -Xfatal-warnings)
 at 7: unused import

[som-snytt]

@sjrd that is probably my limit for this PR

[som-snytt]

Squashed, fixed conflict in posttyper (!), amended the comment, left the change for import chaining as a separate commit for visibility (it's just a tweak but not fully vetted).

I'll expect to follow up, for bugs or known false positives with compiletime ops.

[sjrd]

There are some test failures in the CI. Can you take a look?

[987Nabil]

Can this be back ported to 3.3.x?

[Gedochao]

Can this be back ported to 3.3.x?

As far as I can tell, it hasn't been assessed yet. Timeline-wise, it ought to be checked for 3.3.7. cc @tgodzik

[tgodzik]

We'll try to backport it for sure, though this might be quite difficult due to amount of changes. But the plan is to do it.