Warn unused imports

[som-snytt]

Forward port the Scala 2 feature, tracking imports when contexts are created, and registering when selectors are used.

Some support for -rewrite to remove unused selectors (if there is only one import on the source line), disabled.

[som-snytt]

It doesn't like the mutation in Contexts:

possible data race involving globally reachable variable _initialStore in object Contexts: dotty.tools.dotc.util.Store
  use -Ylog:checkReentrant+ to find out more about why the variable is reachable.

neg/i13558.scala needs updated check for prettier-printed wildcard

                    Reference to id is ambiguous,
                    it is both imported by import testcode.ExtensionB.*
                    and imported subsequently by import testcode.ExtensionA.*

-Werror not recognized? The improved error output looks nice.

 Wrong number of errors encountered when compiling out/compileNeg/neg/unused-imports
expected: 5, actual: 1
Unfulfilled expectations:
tests/neg/unused-imports.scala:12
tests/neg/unused-imports.scala:5
tests/neg/unused-imports.scala:6
Unexpected errors:

Reported errors:
 at 11: Found:    ("42" : String)\Required: Int

[som-snytt]

-Werror is an established option. Maybe scaladoc test didn't like it? as I recall from the previous failure.

It looks like -Wunused:imports is not set in the right place for one of the test regimes. The build is really quite complicated. I am getting better at "run a test" but still have no idea about what is run for CI. I'm trying out each piece of the sbt command to see I can witness which fails. In particular, "it would be nice" if CI output had headers saying what is running, instead of just continuous output from sbt.

Putting this at the top of my test is supposed to mean I don't have to jury-rig something with "tests that require a setting" or however that works:

// scalac: -Wunused:imports -Werror -feature

[som-snytt]

Not my imagination that this works:

sbt:scala3> testCompilation tests/neg/unused-imports
[info] Test run started
[info] Test dotty.tools.dotc.CompilationTests.negAll started
[=======================================] completed (1/1, 0 failed, 2s)

and

sbt:scala3> scala3-bootstrapped/testCompilation tests/neg/unused-imports
[info] Test run started
[info] Test dotty.tools.dotc.CompilationTests.negAll started
[=======================================] completed (1/1, 0 failed, 2s)

but

sbt:scala3> scala3-bootstrapped/test
[snip]
[info] Test dotty.tools.dotc.CompilationTests.negAll started

Wrong number of errors encountered when compiling out/compileNeg/neg/unused-imports
expected: 5, actual: 1
Unfulfilled expectations:
tests/neg/unused-imports.scala:12
tests/neg/unused-imports.scala:5
tests/neg/unused-imports.scala:6
Unexpected errors:

Reported errors:
 at 11: Found:    ("42" : String)\Required: Int
[=======================================] completed (1380/1380, 1 failed, 58s)
[error] Test dotty.tools.dotc.CompilationTests.negAll failed: java.lang.AssertionError: Neg test should have failed, but did not, took 58.136 se
[error]     at dotty.tools.vulpix.ParallelTesting$CompilationTest.checkExpectedErrors(ParallelTesting.scala:1003)
[error]     at dotty.tools.dotc.CompilationTests.negAll(CompilationTests.scala:190)
[error]     at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[error]     at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
[error]     at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[error]     at java.lang.reflect.Method.invoke(Method.java:568)
[error]     ...

[som-snytt]

maybe I'll just delete disable the test.

[som-snytt]

The fixup edits are moved to #14533 The rewrite result didn't get manual edits so it could be re-run any time. That was mostly just a test. (Still managed a few conflicts, however.)

[som-snytt]

Rebased and squashed, but I haven't made the test work under all regimes. This PR feels like a long time ago but it says just a fortnight.

Edit: test should be neg-with-compiler

[info] Test dotty.tools.dotc.BootstrappedOnlyCompilationTests.negWithCompiler started
[=======================================] completed (1/1, 0 failed, 0s)

Edit: or maybe that is not enough to survive vulpix scala3-bootstrapped/test is the command to beat. Maybe I'll have to take the vulpix coursera after all.

[som-snytt]

*** error while checking tests/neg-with-compiler/unused-imports-live.scala after phase capturedVars ***
class dotty.tools.dotc.reporting.Diagnostic$Error at tests/neg-with-compiler/unused-imports-live.scala:<1172..1172>: Definition of implicit conversion method mkOrderingOps should be enabled

seems to be #14637 but I also haven't drilled into how vulpix is compiling it.

Yes, got it to pass by removing -feature from the test file.

Or, vulpix is not a great experience. I remember hating partest when it was half-baked.

[info] Test dotty.tools.dotc.BootstrappedOnlyCompilationTests.negWithCompiler started

No errors found when compiling neg test out/compileNegWithCompiler/neg-with-compiler/unused-imports-live
[=======================================] completed (2/2, 1 failed, 1s)

[griggt]

I see that the magic "source version" (e.g. 3.0, future, etc.) imports are currently always flagged as unused. Consider this snippet:

import language.future

class Baz[T]
def foo[T: Baz](x: T)(using Ordering[T]) = ???
val result = foo(42)(using Baz[Int])

$ scalac -Wunused:imports snippet.scala
-- Warning: snippet.scala:1:16 -----------------------------------
1 |import language.future
  |                ^^^^^^
  |                Unused import

but if I remove the language.future import:

$ scalac -Wunused:imports snippet.scala 
-- Error: snippet.scala:5:20 ---------------------------------------------------------------------------------------
5 |val result = foo(42)(using Baz[Int])
  |             ^^^^^^^^^^^^^^^^^^^^^^^
  |             not enough arguments for method foo: (implicit evidence$1: Baz[Int], x$2: Ordering[Int]): Nothing

IMO, the "source version" magic imports are distinct from the "feature" magic imports, and should always be considered as used.

[som-snytt]

@griggt Thanks, I see the test has language.future, so I must have tried it out. I'll try to rediscover the mechanism. The test needs to be under neg-with-compiler to work under different test modes, I think is what I learned at some point, so I'll attempt it.

Imports with corresponding compiler options are tricky. Do I want to see a warning for language.postfixOps because the compiler option was supplied somewhere in a build file? Anything unused may be redundant, such as var x shadowed by another due to a merge mistake. What about duplicate imports? Currently I would expect a warning, but I have not pondered the case recently.

I see I had a commit for feature imports before I squashed.

[griggt]

The issue with the test is weird. I don't see any reason that it needs to be in neg-with-compiler, however.

The symptoms lead me to believe it has something to do with parallelism, as it works when run singly using testCompilation.

Try putting the test file in its own subdirectory under tests/neg, e.g. tests/neg/unused-imports/unused-imports.scala with the check file as tests/neg/unused-imports.check (yes, in the parent directory, the check file should have the same name as the subdirectory; it will need updating to reflect the paths).

[som-snytt]

Hey, don't break my brain! The comment in the test says

// disabled because doesn't work under scala3-bootstrapped/test
// works under testCompilation

Under neg-with, it only runs when bootstrapped, so that's correct. If vulpix is making me think too hard, the overhead isn't worth it. (Normally I enjoy testing puzzles like any other.)

I see the language version code, so I will try to do something not bad.

Actually, sourceVersion in file shadows compiler options, but language feature in options means redundant import is not checked.

[som-snytt]

Ignore my previous theory on how to use vulpix. I'll look at what sbt command it's actually running and then try what you said.

 Wrong number of errors encountered when compiling out/compileNegWithCompiler/neg-with-compiler/unused-imports
expected: 5, actual: 1
Unfulfilled expectations:
tests/neg-with-compiler/unused-imports.scala:5
tests/neg-with-compiler/unused-imports.scala:6
tests/neg-with-compiler/unused-imports.scala:12
Unexpected errors:

Reported errors:
 at 11: Found:    ("42" : String)\Required: Int

It doesn't list multiple unfulfilled expectations on a line.

[griggt]

I've looked into the test problem some more and I'm convinced the issue is shared state corruption when multiple compilations are running in parallel. I think the fix is something like this:

diff --git a/compiler/src/dotty/tools/dotc/core/Contexts.scala b/compiler/src/dotty/tools/dotc/core/Contexts.scala
index a0ceddce9d..ea616b5843 100644
--- a/compiler/src/dotty/tools/dotc/core/Contexts.scala
+++ b/compiler/src/dotty/tools/dotc/core/Contexts.scala
@@ -60,7 +60,7 @@ object Contexts:
   private val (notNullInfosLoc,     store8) = store7.newLocation[List[NotNullInfo]]()
   private val (importInfoLoc,       store9) = store8.newLocation[ImportInfo | Null]()
   private val (typeAssignerLoc,    store10) = store9.newLocation[TypeAssigner](TypeAssigner)
-  private val (usagesLoc,          store11) = store10.newLocation[Usages](Usages())
+  private val (usagesLoc,          store11) = store10.newLocation[Usages]()
 
   private val initialStore = store11
 
@@ -837,6 +837,7 @@ object Contexts:
     store = initialStore
       .updated(settingsStateLoc, settingsGroup.defaultState)
       .updated(notNullInfosLoc, Nil)
+      .updated(usagesLoc, Usages())
       .updated(compilationUnitLoc, NoCompilationUnit)
     searchHistory = new SearchRoot
     gadt = EmptyGadtConstraint

The test should be able to live in tests/neg/unused-imports.scala.

[som-snytt]

@griggt Thanks for the hint!

[som-snytt]

Update includes all of @griggt suggestions.

Intuitively, I'd like to know if my import language.next changed any behavior, but the behavior is too diffuse. For instance, some code just checks version eq future and does something different. Moreover, version selection might be a backstop against future edits. Therefore, just ignore the imports.

Similarly, masking imports are still ignored. Even if you track that something was masked, such imports can be useful as a defense mechanism.

-rewrite support is moved to TyperPhase.emitDiagnostics. -Yrewrite-imports -Wunused:imports -rewrite to enable, baroque enough to be safe. Either rewrite or report, not both. (The rewrite test requires that?) It will report what it could not rewrite; the restriction is still that import is by itself on the line, modulo comments.

Noting that it's much easier to add compiler options to test code as "directives" than touch the test rig; tests/rewrites requires at least mentioning the test but perhaps could just compile in dir with all options as directives, or -rewrite among the defaults.

[griggt]

I'm currently running this through the community build (-Wunused:imports -rewrite -Yrewrite-imports)

I'll post the results when complete, but thus far it's definitely uncovered some issues.

[som-snytt]

Rebased, but I haven't looked at the latest comments from May 11 or commit from May 6. It looks like the follow-up is for locating selectors to rewrite, handling language imports (including version), and cross-compilation (such as import x.y as z).

If a language flag is required under Scala 2 but not 3, I don't think it's a goal here to differentiate? or my previous suggestions was yet another -Y flag to say lint the language imports, too.

[som-snytt]

Tweaked the rewrite facility. It might be useful for projects who can't depend on scalafix, not that you can't depend on scalafix. It surely needs tweaks for weird multiline imports.

[som-snytt]

• Dude, please get rid of the red ex, finally.

• OK, I'll try that.

bad option '-Yerased-terms' was ignored

[som-snytt]

Reopen to mark import used after implicit ranking, preparatory to seeing if it works the same way on Scala 2.

[som-snytt]

Not sure how the build diff, reverted in the last commit, caused the settings error -Yerased-terms. I'll save that puzzle for some future three-day weekend.

[ivan-klass]

For those who is also wondering why this one is closed, I believe the issue is superseded with
#16157