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

Support RemoveUnused in Scala 3.4+ #1728

Merged
merged 5 commits into from
Feb 3, 2024
Merged

Conversation

bjaglin
Copy link
Collaborator

@bjaglin bjaglin commented Jan 28, 2023

First half of #1682

scala/scala3#16157

@bjaglin bjaglin force-pushed the scala3unused branch 3 times, most recently from cf2ca39 to 37cd1ef Compare January 29, 2023 00:30
@bjaglin bjaglin mentioned this pull request May 14, 2023
@bjaglin bjaglin changed the title support RemoveUnused on Scala 3.3.0-RC2 support RemoveUnused on Scala 3.3.0-RC6 May 15, 2023
@bjaglin bjaglin force-pushed the scala3unused branch 6 times, most recently from 0acaa71 to 0207e37 Compare May 18, 2023 12:54
@bjaglin
Copy link
Collaborator Author

bjaglin commented May 18, 2023

Trying to understand why unused warnings don't show up as diagnostics in the SemanticDB files produced by dotty

➜  cat Test.scala                                                                       
//> using scala 3.3.0-RC6
//> using options -Wunused:all -Ysemanticdb

import java.util.List

object Test
➜  scala-cli Test.scala       
[warn] ./Test.scala:4:18
[warn] unused import
[warn] import java.util.List
[warn]                  ^^^^
[error]  No main class found
➜  metap Test.scala.semanticdb
tmp/Test.scala
--------------

Summary:
Schema => SemanticDB v4
Uri => tmp/Test.scala
Text => empty
Language => Scala
Symbols => 1 entries
Occurrences => 4 entries

Symbols:
_empty_/Test. => final object Test extends Object { self: Test.type => +1 decls }

Occurrences:
[3:7..3:11) => java/
[3:12..3:16) => java/util/
[3:17..3:21) => java/util/List#
[5:7..5:11) <= _empty_/Test.

@bjaglin bjaglin changed the title support RemoveUnused on Scala 3.3.0-RC6 support RemoveUnused on Scala 3.3.0 May 18, 2023
@tgodzik
Copy link
Contributor

tgodzik commented May 18, 2023

Trying to understand why unused warnings don't show up as diagnostics in the SemanticDB files produced by dotty

➜  cat Test.scala                                                                       
//> using scala 3.3.0-RC6
//> using options -Wunused:all -Ysemanticdb

import java.util.List

object Test
➜  scala-cli Test.scala       
[warn] ./Test.scala:4:18
[warn] unused import
[warn] import java.util.List
[warn]                  ^^^^
[error]  No main class found
➜  metap Test.scala.semanticdb
tmp/Test.scala
--------------

Summary:
Schema => SemanticDB v4
Uri => tmp/Test.scala
Text => empty
Language => Scala
Symbols => 1 entries
Occurrences => 4 entries

Symbols:
_empty_/Test. => final object Test extends Object { self: Test.type => +1 decls }

Occurrences:
[3:7..3:11) => java/
[3:12..3:16) => java/util/
[3:17..3:21) => java/util/List#
[5:7..5:11) <= _empty_/Test.

Looks like we don't actually write them :| I will ask someone to take a look.

@bjaglin
Copy link
Collaborator Author

bjaglin commented May 18, 2023

Looks like we don't actually write them :| I will ask someone to take a look.

@tgodzik I came to the same (sad) conclusion and was about to open a ticket on dotty

@tgodzik
Copy link
Contributor

tgodzik commented May 18, 2023

Looks like we don't actually write them :| I will ask someone to take a look.

@tgodzik I came to the same (sad) conclusion and was about to open a ticket on dotty

I will try a quick look, but that means it will mos tlikely be in 3.3.1 :/

@bjaglin
Copy link
Collaborator Author

bjaglin commented May 18, 2023

scala/scala3#17535

@bjaglin bjaglin changed the title support RemoveUnused on Scala 3.3.0 support RemoveUnused in Scala 3 Jun 30, 2023
@bjaglin bjaglin changed the title support RemoveUnused in Scala 3 Support RemoveUnused in Scala 3 Jun 30, 2023
@anasbarg
Copy link

Any updates regarding this PR?

@tgodzik
Copy link
Contributor

tgodzik commented Dec 11, 2023

I think we are waiting for the new Scala 3 release

@bjaglin
Copy link
Collaborator Author

bjaglin commented Dec 11, 2023

I think we are waiting for the new Scala 3 release

Indeed, as soon as I see activity on the release train, I'll put that back in shape (and align with #1800). The dotty PR has been marked as backport:nominated, so hopefully this makes it to Scala 3.3 LTS 🤞

@bjaglin
Copy link
Collaborator Author

bjaglin commented Dec 19, 2023

From the dotty PR

This particular change will land in 3.3.3 (that should have parity with 3.4.0). It will be available as an RC in late January and as a stable release in early March.

@bjaglin bjaglin force-pushed the scala3unused branch 2 times, most recently from 0698713 to acbd4b7 Compare February 3, 2024 12:19
@bjaglin bjaglin changed the title Support RemoveUnused in Scala 3 Support RemoveUnused in Scala 3.4+ Feb 3, 2024
@bjaglin bjaglin force-pushed the scala3unused branch 2 times, most recently from 7ba8bef to 945baa2 Compare February 3, 2024 13:16
@@ -67,7 +64,7 @@ object Main {
}
```

Remove unused pattern match variables:
Remove unused pattern match variables (requires `-Wunused:unsafe-warn-patvars` on Scala 3):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -9,7 +9,7 @@ title: Installation

**Java LTS (8, 11, 17 or 21)**

**Scala 2.12, 2.13 or 3.x** (all rules are not available for Scala 3.x)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only ExplicitResultTypes remains unsupported, and it's documented in rule docs, so I think we can drop this verbose disclaimer

version.startsWith("3.0") ||
version.startsWith("3.1") ||
version.startsWith("3.2") ||
version.startsWith("3.3")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3.3.3 is likely to get the semanticdb fix but I'll update when I see it happening in a RC

case _ => importee.pos
}
isUnusedImport.exists { unused =>
unused.start <= pos.start && pos.end <= unused.end
Copy link
Collaborator Author

@bjaglin bjaglin Feb 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

semanticdb diagnostics emitted by Scala 3 can be wider than those emiited by Scala 2's semanticdb-scalac (which do match the tree positions apart from renames, handled above)

@@ -87,7 +84,7 @@ object Main {
}
```

Remove unused function parameters:
Remove unused function parameters (Scala 2 only):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite the presence of -Wunused:params and the explicit mention of it in scala/scala3#16157, I did not manage to get diagnostics (nor warnings), so I am leaving this out for now

$ metap ./scalafix-tests/input/target/jvm-3.4.0-RC2/meta/META-INF/semanticdb/scalafix-tests/input/src/main/scala/test/removeUnused/RemoveUnusedParams.scala.semanticdb 
scalafix-tests/input/src/main/scala/test/removeUnused/RemoveUnusedParams.scala
------------------------------------------------------------------------------

Summary:
Schema => SemanticDB v4
Uri => scalafix-tests/input/src/main/scala/test/removeUnused/RemoveUnusedParams.scala
Text => empty
Language => Scala
Symbols => 11 entries
Occurrences => 28 entries

Symbols:
local0 => param unused: String
local1 => param unused: String
local2 => param used: String
local3 => param unused: Long
local4 => implicit param string: String
test/removeUnused/UnusedParams. => final object UnusedParams extends Object { self: UnusedParams.type => +5 decls }
test/removeUnused/UnusedParams.f. => val method fFunction1[String, Unit]
test/removeUnused/UnusedParams.ff. => val method ffFunction1[String, Unit]
test/removeUnused/UnusedParams.fs. => val method fsFunction2[String, Long, Unit]
test/removeUnused/UnusedParams.g(). => method g(x: Function1[String, Unit]): Unit
test/removeUnused/UnusedParams.g().(x) => param x: Function1[String, Unit]

Occurrences:
[3:8..3:12) => test/
[3:13..3:25) <= test/removeUnused/
[5:7..5:19) <= test/removeUnused/UnusedParams.
[6:6..6:7) <= test/removeUnused/UnusedParams.f.
[6:9..6:15) => scala/Predef.String#
[6:19..6:23) => scala/Unit#
[6:26..6:32) <= local0
[6:36..6:43) => scala/Predef.println(+1).
[7:6..7:8) <= test/removeUnused/UnusedParams.ff.
[7:12..7:18) <= local1
[7:20..7:26) => scala/Predef.String#
[7:31..7:38) => scala/Predef.println(+1).
[8:6..8:8) <= test/removeUnused/UnusedParams.fs.
[8:12..8:16) <= local2
[8:18..8:24) => scala/Predef.String#
[8:26..8:32) <= local3
[8:34..8:38) => scala/Long#
[8:43..8:50) => scala/Predef.println(+1).
[8:51..8:55) => local2
[9:6..9:7) <= test/removeUnused/UnusedParams.g().
[9:8..9:9) <= test/removeUnused/UnusedParams.g().(x)
[9:11..9:17) => scala/Predef.String#
[9:21..9:25) => scala/Unit#
[9:28..9:32) => scala/Unit#
[9:35..9:38) => scala/Predef.`???`().
[10:2..10:3) => test/removeUnused/UnusedParams.g().
[10:13..10:19) <= local4
[10:23..10:30) => scala/Predef.println(+1).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -107,6 +107,7 @@ Use `MethodSignature.parameterLists` to look up parameters of a method.
def printMethodParameters(symbol: Symbol): Unit = {
symbol.info.get.signature match {
case signature @ MethodSignature(typeParameters, parameterLists, _) =>
println("signature = " + signature)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes in ScalafixBuild enabled more unused warnings on the builds

@bjaglin bjaglin marked this pull request as ready for review February 3, 2024 14:10
@bjaglin bjaglin merged commit e4eb15d into scalacenter:main Feb 3, 2024
8 checks passed
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.

3 participants