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 OrganizeImports.removeUnused in Scala 3.4+ #1800

Merged
merged 1 commit into from
Feb 3, 2024

Conversation

tanishiking
Copy link
Contributor

@tanishiking tanishiking commented Jun 30, 2023

Second half of / closes #1682

This PR is mostly for verify scala/scala3#17835 actually unlock OrganizeImports.removeUnused (and UnusedImports) in scalafix, and it looks like it does 🎉

Screen.Recording.2023-06-30.at.14.57.19.mov

Once this PR scala/scala3#17835 has merged and released, scalafix-organize-imports should be able to run OrganizeImports.removeUnused based on the diagnostics information in SemanticDB emit from Scala3 compiler.

In order to make OrganizeImports rule to work with Scala 3, this PR added a few adjustments to the rule.

@tanishiking
Copy link
Contributor Author

RemoveUnused can be supported in the same way #1728 👍

Copy link
Collaborator

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

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

Thank you so much for your work upstream and taking the time to double-check it here! Added some comments for later, I'll be happy to take over if you don't have bandwidth.

Doc updates I have identified so far:

  • 1. The [`removeUnused`](OrganizeImports.md#removeunused) option must be
    explicitly set to `false` - the rule currently doesn’t remove unused
    imports as it is currently not supported by the compiler.
  • > The `removeUnused` option is currently not supported for source files
    > compiled with Scala 3, as the [compiler cannot issue warnings for unused
    > imports
    > yet](https://docs.scala-lang.org/scala3/guides/migration/options-lookup.html#warning-settings).
    > As a result, you must set `removeUnused` to `false` when running the
    > rule on source files compiled with Scala 3.
  • removeUnused = true // not supported in Scala 3

project/ScalafixBuild.scala Outdated Show resolved Hide resolved
project/Dependencies.scala Outdated Show resolved Hide resolved
@tanishiking
Copy link
Contributor Author

Thank you for the comments! I'll get back to this once the upstream change has merged, maybe I'll ask someone to take over (maybe I'll do by myself). Anyway, I'll let you know :)

@tanishiking
Copy link
Contributor Author

tanishiking commented Oct 19, 2023

Finally scala/scala3#17835 is merged, and we'll be able to support this feature from the next release of the Scala3 🎉

@tanishiking
Copy link
Contributor Author

I think we don't need to rush until 3.4.0 is released, but now the test works fine with

$ sbt -Dscala3.nightly=3.4.0-RC1-bin-20231024-15033c7-NIGHTLY
sbt:scalafix> expect3Target3_4_0-RC1-bin-20231024-15033c7-NIGHTLY/test

@bjaglin
Copy link
Collaborator

bjaglin commented Dec 19, 2023

I think we don't need to rush until 3.4.0 is released, but now the test works fine with

For the inpatients (like me!) wondering about the timing, see scala/scala3#17835 (comment)

@bjaglin bjaglin changed the title Support OrganizeImports.removeUnused in Scala 3 Support OrganizeImports.removeUnused in Scala 3.4+ Feb 3, 2024
@bjaglin
Copy link
Collaborator

bjaglin commented Feb 3, 2024

I've got #1728 sorted out and there are conflicts, so I'll squash and rebase your PR.

@bjaglin bjaglin force-pushed the organize-imports-3 branch 2 times, most recently from 43aee81 to 02033a4 Compare February 3, 2024 15:06
@@ -1,6 +1,6 @@
package scalafix.test.cli

import scala.meta._
Copy link
Collaborator

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

https://github.com/scalacenter/scalafix/actions/runs/7767691536/job/21184796798?pr=1800

[error] --- /home/runner/work/scalafix/scalafix/scalafix-tests/integration/src/main/scala/scalafix/test/cli/Rules.scala
[error] +++ <expected fix>
[error] @@ -1,7 +1,5 @@
[error]  package scalafix.test.cli
[error]  
[error] -import scala.meta._
[error] -
[error]  import metaconfig.Configured
[error]  import scalafix.internal.util.PositionSyntax._
[error]  import scalafix.internal.util.SymbolOps
...
[error] (integration2_13 / scalafixAll) scalafix.sbt.ScalafixFailed: TestError

But the file won't compile without that...

Copy link
Collaborator

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

This is clearly a side effect of this PR as we are dogfooding, but it might be specific to the complex build of the project and not representative. The weird thing is that I can't reproduce locally. Investigating.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Relaxing the isUnused() implementation exposed a pretty serious bug: the mutable collections used in OrganizeImports are not cleared from one file to another, as the class is instantiated only once per scalafix invocation.

private val unusedImporteePositions: mutable.Set[Position] =
mutable.Set.empty[Position]
private val diagnostics: ArrayBuffer[Diagnostic] =
ArrayBuffer.empty[Diagnostic]

I'll fix it in a separate PR, and rebase this PR to remove the need for the hacky second commit.

Once this PR scala/scala3#17835
has merged and released, scalafix-organize-imports should be
able to run OrganizeImports.removeUnused based on the
diagnostics information in SemanticDB emit from Scala3 compiler.

In order to make OrganizeImports rule to work with Scala 3,
this commit added a few adjustments to the rule.
@bjaglin
Copy link
Collaborator

bjaglin commented Feb 3, 2024

Rebased against #1921. This is it ✌️ I can't believe this is finally happening 🥳

Thanks so much for the perseverance on the dotty front @tanishiking !

@bjaglin bjaglin merged commit de53a33 into scalacenter:main Feb 3, 2024
8 checks passed
@bjaglin bjaglin mentioned this pull request Feb 3, 2024
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.

scala 3: RemoveUnused & OrganizeImports.removeUnused
2 participants