In selector check, prefix of reference must match import qualifier #20894

som-snytt · 2024-06-29T06:50:06Z

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
Fixes #18564
Fixes #22376
Fixes #21525

som-snytt · 2024-07-01T12:24:31Z

Further tweaks forthcoming.

Gedochao · 2024-07-03T10:08:03Z

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

compiler/src/dotty/tools/dotc/transform/CheckUnused.scala

som-snytt · 2024-07-03T11:15:41Z

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 · 2024-07-07T13:27:24Z

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 · 2024-10-02T19:37:28Z

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 · 2024-10-06T21:02:04Z

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 · 2024-10-06T21:07:39Z

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

som-snytt · 2024-10-09T06:12:57Z

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 · 2024-10-09T07:40:39Z

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 · 2024-11-05T16:36:55Z

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 · 2025-01-13T16:24:36Z

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 · 2025-01-15T19:19:09Z

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 · 2025-01-15T21:31:22Z

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

sjrd · 2025-01-16T10:12:28Z

library/src/scala/compiletime/package.scala

Suspicious change of permissions on this file.

Thanks, I noticed it: I had touched then reverted the file but haven't fixed this permissions glitch yet. I'd like to figure out what lapse of gitfu caused it.

shell history says that instead of git reset, I grabbed the raw file from github. Aha! That was in a period of rolling back unnecessary & distracting tweaks for this PR.

sjrd · 2025-01-16T10:23:42Z

tests/warn/i15503f.scala

+    def unapply(x: Expr[Option[T]])(using Quotes) = x match
+      case '{ Option[T](${Expr(y)}) } => Some(Option(y))
+      case '{ None } => Some(None)
+      //case '{ ${Expr(opt)} : Some[T] } => Some(opt) // make Type param unused after typer


Is this a TODO?

It's the same as the previous case, but the parameter using Type is unused, but does not warn under -Wunused:implicits. Now I'm not sure why. It's not named and Type has no declared members.

Apparently something to do with quote pickling, not printed by -Vprint. Is there a tool for showing trees that I don't know about? How can I reason about data structures that are invisible?

To follow up, the TODO was that I wanted to show the implicit arg unused, but didn't know enough about quoting to do it; in addition, I lacked tooling just to show me the Ident tree in question.

sjrd · 2025-01-16T10:26:28Z

tests/warn/i15503kb/power.scala

@@ -0,0 +1,15 @@
+
+object Power:
+  import scala.concurrent.* // warn [taps mic]


What does "[taps mic]" mean?

Make sure it's warning. Inside these tests, if it's testing that elements do not warn, then you'd never know if warnings were turned off (for whatever feature). In retrospect, I'm leaning toward preferring a fixed corpus which is run under various settings: then all the code is tried under all the options, in some combination. (The fixed corpus need not be one huge file, but a test group.)

I've begun (recently) to add -Werror in tests/warn to signal that no warnings are expected. That assists clarity. If there is a warning, though, you don't get a nice vulpix explanation.

sjrd · 2025-01-16T10:42:39Z

compiler/src/dotty/tools/dotc/transform/CheckUnused.scala

+    ) || (
+         sel.isWildcard && sel.isGiven
+      && imp.expr.tpe.allMembers.exists(_.symbol.isCanEqual)
+      || imp.expr.tpe.member(sel.name.toTermName).hasAltWith(_.symbol.isCanEqual)


I'm very confused about the relative precedence of && and || in this method. They seem fishy. Consider adding relevant parentheses and/or indentation.

I'd also like to delete that whole thing: I don't think strict no implicit warn is useful if the check is working correctly (as it ought to be), and ~~also I'm not sure CanEqual needs special treatment any more.~~ CanEqual under strictEquality remains problematic. There is no residuum to indicate how it was resolved.

sjrd · 2025-01-16T10:43:12Z

compiler/src/dotty/tools/dotc/transform/CheckUnused.scala

-      (imp.expr.tpe.member(sel.name.toTermName).alternatives
-        .exists(p => p.symbol.isOneOf(GivenOrImplicit) && p.symbol.typeRef.baseClasses.exists(_.derivesFrom(defn.CanEqualClass))))
+  extension (imp: Import)
+    /** Is it the first import clause in a statement? `a.x` in `import a.x, b,{y, z}` */


Suggested change

/** Is it the first import clause in a statement? `a.x` in `import a.x, b,{y, z}` */

/** Is it the first import clause in a statement? `a.x` in `import a.x, b.{y, z}` */

?

sjrd · 2025-01-16T13:54:57Z

compiler/src/dotty/tools/dotc/transform/CheckUnused.scala

-        loopOnNormalizedPrefixes(tree.typeOpt.normalizedPrefix, depth = 0)
-        ud.registerUsed(tree.symbol, Some(tree.name))
-      }
+      // resolve if inlined at the position of the call, or is zero extent summon


This comment does not seem to account for the first condition: refInfos.inlined.isEmpty. That's getting me confused.

That is the normal case, the tree is not an inline expansion. But now I think most Scala 3 code is inlined. Also this reverse-engineering from the synthetic position was just an observation; I'm not sure this is robust.

sjrd · 2025-01-16T13:56:41Z

compiler/src/dotty/tools/dotc/transform/CheckUnused.scala

+      resolveUsage(tree.tpe.classSymbol, tree.name, tree.tpe.importPrefix.skipPackageObject)
+    tree
+
+  // import x.y; x may be rewritten x.y, also import x.z as y


"x may be rewritten x.y" looks weird? What does that mean?

that is https://github.com/scala/scala3/blob/3.6.3/compiler/src/dotty/tools/dotc/typer/Typer.scala#L586 except the comment should say y is rewritten.

sjrd · 2025-01-16T13:57:36Z

compiler/src/dotty/tools/dotc/transform/CheckUnused.scala

+    val name = tree.removeAttachment(OriginalName).getOrElse(nme.NO_NAME)
+    if tree.span.isSynthetic && tree.symbol == defn.TypeTest_unapply then
+      tree.qualifier.tpe.underlying.finalResultType match
+      case AppliedType(_, args) => // if tycon.typeSymbol == defn.TypeTestClass


Leftover comment?

documentary? It has already checked for TypeTest.unapply, so the extractor should be a TypeTest[A, B], where the user wrote case _: B.

Some combination of redundant code and comments should help me remember later what I was working out, for some definition of "working memory".

sjrd · 2025-01-16T14:10:14Z

compiler/src/dotty/tools/dotc/transform/CheckUnused.scala

+    case _ if tree.isType =>
+      //println(s"OTHER TYPE ${tree.getClass} ${tree.show}")
+    case _ =>
+      //println(s"OTHER ${tree.getClass} ${tree.show}")


putting them back under a line comment was the clean-up!

sjrd · 2025-01-16T14:11:30Z

compiler/src/dotty/tools/dotc/transform/CheckUnused.scala

+
+  type MessageInfo = (UnusedSymbol, SrcPos, String) // string is origin or empty
+
+  def warnings(using Context): Array[MessageInfo] =


I haven't looked at this method. It seems daunting. Is there any way to make it less dense / more approachable?

Yes, I'll undertake that in a separate commit. Probably just a method for each case (params, privates, imports etc). I'll also move the "rewrite" logic out of the way. I did verify that the rewrite correctly edits the imports in the repo.

Prefer context functions for brevity. Avoid intermediate collections. Check scope type Use type test of prefix for usages in scope Style tweaks in CheckUnused Attachment tracks derivation No warn serialization methods Assume tpd Refactor traverser to miniphase callbacks Avoid tracking red herrings Mono Mega Phase Consider superclass context Filter member of refinement, handle Annotated TypeTree is usage of simple name Handle quotes and splices Tighten allowance for serialization methods Restore inferred type is not a usage, noprefix is in import Warn for top level private Import given takes NoPrefix usage Don't ignore params of public methods Accept updated semanticdb output Show misplaced warn comment, unfulfilled expectations Rewrite Restore functionality Patvars Absolution of canonical names defn.LanguageFeatureMetaAnnot tweak conditions for warning Handle match types No transparent inline exclusion Rewrite imports, supply origin No warn inline proxy Original of literal Typos in build Turn off boolean setting Use canonical names Tweak param test and test for import precedence Do resolve imports post inlining Excuse only empty interfaces as unused bound Adjust duplicate test, don't forgive anonfun Detect summon inline Restore previous CheckShadowing with tweaks Use result of TypeTest Maybe just ignore in inline defs Avoid warnings Different coping mechanism No warn only toplevel imports, absolve more patvars by selector Update semanticdb metac.expect Empty block is trivial Warn unassigned mutable patvars Tweak warning

som-snytt · 2025-01-17T22:57:15Z

@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 · 2025-01-18T00:22:01Z

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 mentioned this pull request Jul 1, 2024

-Wunused false negative in for-comprehension #18289

Open

Gedochao requested a review from sjrd July 3, 2024 10:01

Gedochao assigned sjrd Jul 3, 2024

sjrd requested changes Jul 3, 2024

View reviewed changes

compiler/src/dotty/tools/dotc/transform/CheckUnused.scala Outdated Show resolved Hide resolved

compiler/src/dotty/tools/dotc/transform/CheckUnused.scala Outdated Show resolved Hide resolved

compiler/src/dotty/tools/dotc/transform/CheckUnused.scala Outdated Show resolved Hide resolved

som-snytt force-pushed the issue/19657-unused-import-given branch from 634fdb0 to 741473e Compare July 7, 2024 03:32

som-snytt assigned som-snytt and unassigned sjrd Jul 7, 2024

som-snytt changed the title ~~In selector check, respect prefix and compare finalResultType to bound~~ In selector check, prefix of reference must match import qualifier Jul 9, 2024

som-snytt force-pushed the issue/19657-unused-import-given branch 2 times, most recently from aa1526f to b6c0fb6 Compare July 14, 2024 16:02

som-snytt force-pushed the issue/19657-unused-import-given branch from 09d418a to 75d4d24 Compare August 16, 2024 22:46

som-snytt closed this Sep 15, 2024

som-snytt reopened this Oct 2, 2024

som-snytt force-pushed the issue/19657-unused-import-given branch from 75d4d24 to 0f4be8a Compare October 2, 2024 18:55

som-snytt marked this pull request as ready for review October 2, 2024 20:48

som-snytt marked this pull request as draft October 3, 2024 17:43

This was referenced Oct 18, 2024

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

Open

Merge linting phases #18735

Closed

som-snytt force-pushed the issue/19657-unused-import-given branch from eb1bf06 to e8ca0cc Compare November 5, 2024 16:11

som-snytt marked this pull request as ready for review November 6, 2024 12:08

som-snytt force-pushed the issue/19657-unused-import-given branch 2 times, most recently from 9fac2ae to e7422fa Compare January 7, 2025 20:12

som-snytt mentioned this pull request Jan 9, 2025

No warning for unused patvars in for comprehension assignment scala/bug#13070

Closed

som-snytt force-pushed the issue/19657-unused-import-given branch 5 times, most recently from dbd7c5b to fb6fa47 Compare January 13, 2025 16:21

som-snytt force-pushed the issue/19657-unused-import-given branch 5 times, most recently from e85b80f to 5df9e7d Compare January 15, 2025 13:31

som-snytt force-pushed the issue/19657-unused-import-given branch from 5df9e7d to f77e46a Compare January 15, 2025 19:19

som-snytt marked this pull request as ready for review January 15, 2025 20:35

som-snytt force-pushed the issue/19657-unused-import-given branch from f77e46a to 470db35 Compare January 16, 2025 01:25

sjrd requested changes Jan 16, 2025

View reviewed changes

som-snytt added 5 commits January 16, 2025 11:21

Prefer dotc.util.chaining

9f9cb2a

Repair file perms

ceee4b4

Tweaks per review

4479b5d

Split warnings method

add05d0

som-snytt force-pushed the issue/19657-unused-import-given branch from 470db35 to add05d0 Compare January 17, 2025 05:02

Refactor resolveUsage, cache Precedence for correctness

20ccfe3

som-snytt force-pushed the issue/19657-unused-import-given branch from 308e3e9 to 20ccfe3 Compare January 17, 2025 23:17

		@@ -0,0 +1,15 @@

		object Power:
		import scala.concurrent.* // warn [taps mic]

	/** Is it the first import clause in a statement? `a.x` in `import a.x, b,{y, z}` */
	/** Is it the first import clause in a statement? `a.x` in `import a.x, b.{y, z}` */


		type MessageInfo = (UnusedSymbol, SrcPos, String) // string is origin or empty

		def warnings(using Context): Array[MessageInfo] =

In selector check, prefix of reference must match import qualifier #20894

Are you sure you want to change the base?

In selector check, prefix of reference must match import qualifier #20894

Conversation

som-snytt commented Jun 29, 2024 • edited Loading

som-snytt commented Jul 1, 2024

Gedochao commented Jul 3, 2024

sjrd left a comment

Choose a reason for hiding this comment

som-snytt commented Jul 3, 2024

som-snytt commented Jul 7, 2024

som-snytt commented Oct 2, 2024 • edited Loading

som-snytt commented Oct 6, 2024 • edited Loading

som-snytt commented Oct 6, 2024

som-snytt commented Oct 9, 2024

sjrd commented Oct 9, 2024

som-snytt commented Nov 5, 2024 • edited Loading

som-snytt commented Jan 13, 2025

som-snytt commented Jan 15, 2025

som-snytt commented Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

som-snytt Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

som-snytt commented Jan 17, 2025

som-snytt commented Jan 18, 2025

som-snytt commented Jun 29, 2024 •

edited

Loading

som-snytt commented Oct 2, 2024 •

edited

Loading

som-snytt commented Oct 6, 2024 •

edited

Loading

som-snytt commented Nov 5, 2024 •

edited

Loading

som-snytt commented Jan 15, 2025 •

edited

Loading

som-snytt Jan 16, 2025 •

edited

Loading