Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Semester Project] Add new front-end phase for unused entities and add support for unused imports #16157

Merged
merged 54 commits into from
Jan 9, 2023

Conversation

PaulCoral
Copy link
Contributor

@PaulCoral PaulCoral commented Oct 9, 2022

This PR, related to my semester project #15503 on adding dotty's linter features, adds the following:

  • Add the CheckUnused front-end phase, which will check the the tree produced by the typer, for unused entites (imports, local defs, ...)
  • Emit warning for -Wunused:imports including given imports and wildcard imports
  • Emit warning for -Wunused:locals
  • Emit warning for -Wunused:privates
  • Emit warning for -Wunused:params
  • Emit warning for -Wunused:patvars
  • Emit warning for -Wunused:unsafe-warn-patvars
  • Emit warning for -Wunused:linted
  • Add a simple fatal-warning compilation-test suit
  • Fixes for the warning format
  • Better help in CLI for -Wunused
  • Add -Wunused:givens alias to -Wunused:implicits

Here are a few examples:

Unused Imports

object Foo {
  import collection.mutable.{Set, Map}

  def main(args: Array[String]) =
    val bar = Set("this","set","is","used")
    println(s"Hello World: $bar")
}
sbt:scala3> scalac -Wunused:imports ../Foo.scala
[...]
-- Warning: ../scratch_files/Hello.scala:2:34 ----------------------------------
2 |  import collection.mutable.{Set, Map}
  |                                  ^^^
  |                                  unused import
1 warning found

Unused local definitions

class Foo {
  def bar =
    val a = 1
    2 + 2
}
sbt:scala3> scalac -Wunused:locals ../Foo.scala
[...]
-- Warning: ../scratch_files/MyHello.scala:3:8 ---------------------------------
3 |    val a = 1
  |    ^^^^^^^^^
  |    unused local definition
1 warning found

Unused private members

class Foo {
  private def a = 1
  private def b = 2

  def doSomething = b
}
sbt:scala3> scalac -Wunused:privates ../Foo.scala
[...]
-- Warning: ../scratch_files/MyHello.scala:2:14 --------------------------------
2 |  private def a = 1
  |  ^^^^^^^^^^^^^^^^^
  |  unused private member
1 warning found

Unused parameters

def foo(a: String)(using Int) = bar
sbt:scala3> scalac -Wunused:params ../scratch_files/Foo.scala
[...]
-- Warning: ../scratch_files/MyHello.scala:1:8 ---------------------------------
1 |def foo(a: String)(using Int) = bar
  |        ^
  |        unused explicit parameter
-- Warning: ../scratch_files/MyHello.scala:1:25 --------------------------------
1 |def foo(a: String)(using Int) = bar
  |                         ^
  |                         unused implicit parameter
2 warnings found

Unused pattern variables

def foo(a: List[Int]) = a match
    case head :: tail => ???
    case Nil => ???
sbt:scala3> scalac -Wunused:unsafe-warn-patvars ../scratch_files/Foo.scala
[...]
-- Warning: ../scratch_files/MyHello.scala:2:9 ---------------------------------
2 |    case head :: tail => ???
  |         ^^^^
  |         unused pattern variable
-- Warning: ../scratch_files/MyHello.scala:2:17 --------------------------------
2 |    case head :: tail => ???
  |                 ^^^^
  |                 unused pattern variable
2 warnings found

Please check the test file for the handled cases.

@odersky
@anatoliykmetyuk

PaulCoral and others added 6 commits September 30, 2022 14:43
- report unused named import
- no given or wildcard import
- issue with method reference (e.g. apply, factory, ...)
Emits a warning when none of element of a wildcard ('_')
import is used (i.e. a "unused import" warning).
- Add various tests for unused warnings
- Fix warning not emitted when imported object is used
  another scope
- Emit warning for unused given imports
- Emit warning for wildcard imports when only given
  instances are used.
  ```
  import SomeGivenImports.given // OK
  import SomeGivenImports._ // error
  ```
@som-snytt
Copy link
Contributor

Have you looked at #14524 where "used" means looked up.

The Scala 2 experience was that deciding what is unused is complicated by desugarings. I don't know if that will be the case for Scala 3.

For imports, especially, it was simple to record the look-up. For example, deciding if a language import is used. (Scala 2 was also complicated by interactions with macros.)

@PaulCoral PaulCoral marked this pull request as ready for review October 11, 2022 10:47
@PaulCoral
Copy link
Contributor Author

PaulCoral commented Oct 11, 2022

@som-snytt
Could you please give more details? In the example you give with macros, it is very different in Scala 3 as macros have been completely reworked since Scala 2, and are now fully named and type checked.

- Add an `isRunnable` methode to the `CheckUnused` phase
  to avoid unecessary costly checks (suggested by @bishabosha)
- Move neg compilation tests out of folder
@PaulCoral PaulCoral requested review from bishabosha and removed request for anatoliykmetyuk October 11, 2022 14:45
@cb372
Copy link
Contributor

cb372 commented Oct 14, 2022

Very excited to see some activity on this! Thank you for working on it, and thanks to @som-snytt for pioneering the effort.

The lack of -Wunused:imports is the biggest Scala 3 pain point for my team, and has triggered some discussion about downgrading to Scala 2. If we could get a Scala 3 release with even a partial/imperfect implementation of -Wunused:imports and no other linter features, we would lap it up!

PaulCoral and others added 5 commits October 14, 2022 19:55
- Do not emit any warning for import exclusion.
- These are hard to interpret. These can be placed
  on purpose to avoid future usage of a symbol.
- Add a compiler settings for unused local definition
- Add check for unused local variable in CheckUnused phase
- Add some "neg-custom-args/fatal-warnings" checks for unused locals definition
- Add the "privates" option for "-Wunused"
- Implement "unused privates member" checks in "CheckUnused" phase
- Add a test suit for the previous point
@PaulCoral PaulCoral marked this pull request as draft October 24, 2022 14:05
@fommil
Copy link

fommil commented Oct 25, 2022

This looks interesting. I would like it if there was a way to obtain the explicit list of used imports (not including the unused ones), so that it can be written out to a file for analysis by external tooling. There is precedent to do this in Haskell's ghc compiler and it leads to an improved UX for users.

@ckipp01
Copy link
Member

ckipp01 commented Jan 9, 2023

🚀 woo! Great job on this @PaulCoral!

@PaulCoral
Copy link
Contributor Author

PaulCoral commented Jan 9, 2023

Thanks @szymon-rd, @lrytz and everyone else involved for reviews, hints and suggestions and also @som-snytt for the very helpful previous work.
Thanks also to @anatoliykmetyuk for supervising this project.

@TonioGela
Copy link

Thanks @PaulCoral!

@odersky
Copy link
Contributor

odersky commented Jan 9, 2023

Great work! I am happy to see this turned out so well.

@igor-ramazanov
Copy link

This is so important for me. Linting was the major blocker of full migration to Dotty. Many thanks to @PaulCoral and everyone else involved!

@szymon-rd
Copy link
Member

Ok, there is a problem, probably with this PR:
https://github.com/scalameta/metals/actions/runs/3882899644/jobs/6623578431#step:4:58

@yzia2000
Copy link
Contributor

I absolutely can't wait to use this. Thanks!

@tgodzik
Copy link
Contributor

tgodzik commented Jan 10, 2023

Ok, there is a problem, probably with this PR: https://github.com/scalameta/metals/actions/runs/3882899644/jobs/6623578431#step:4:58

Looks like we have the flag leftover and it was ignored until now, but seems to fail with the specific repo.

@dsbos
Copy link
Member

dsbos commented Mar 11, 2023

You've probably covered this already, but make sure that the logic correctly handles imports that are used to enable features or suppress warnings, such as import scala.language.adhocExtensions or import scala.language.implicitConversions.

A lack of references to the simple name made visible by such a flagging import (e.g., implicitConversions) would not mean that the import wasn't used, since being "used" for such flagging imports means something other than (just) making usable a simple name and having references to that simple name.

I don't know exactly how "use" of such flagging imports should be defined. For example, should an import scala.language.implicitConversions always be considered used, or should it be considered used only if it's actually suppressing an implicit-conversion warning (that is, there's an implicit conversion declaration in its scope--and there's not another import scala.language.implicitConversions that already does the suppressing, which case one of them might logically be considered unused)?

(Note how the sbt-polecat plug-in (for Scala 2) has or had a problem in which if you try to use import scala.language.implicitConversions to suppress reports of declarations of implicit conversions, it thinks that the import is unused, and then (because sbt-polecat makes warnings into errors), the compilation fails, meaning that you can't apply the standard marking for making declaration of implicits conversions easily visible.)

@dsbos
Copy link
Member

dsbos commented Mar 11, 2023

Yeah, now I see that this PR is already merged. I thought I was looking at a page for a PR or issue that was still open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet