Remove Importee.Rename if it does not shadow anything #923

marcelocenerine · 2018-12-16T00:54:49Z

Fixes #915

/cc @hepin1989

UPDATE: I tested the fix on the akka repo after reverting the changes to scala files in this commit: akka/akka@e9fb3a0. These are the only diffs:

diff --git a/akka-bench-jmh/src/main/scala/akka/actor/typed/TypedBenchmarkActors.scala b/akka-bench-jmh/src/main/scala/akka/actor/typed/TypedBenchmarkActors.scala
index 8f0842359a..203c1d70a0 100644
--- a/akka-bench-jmh/src/main/scala/akka/actor/typed/TypedBenchmarkActors.scala
+++ b/akka-bench-jmh/src/main/scala/akka/actor/typed/TypedBenchmarkActors.scala
@@ -6,7 +6,6 @@ package akka.actor.typed

 import akka.Done
 import akka.actor.typed.scaladsl.Behaviors
-import akka.actor.typed.scaladsl.{ ActorContext ⇒ _ }

 import scala.concurrent.duration._

diff --git a/akka-cluster-sharding-typed/src/main/scala/akka/cluster/sharding/typed/scaladsl/ClusterSharding.scala b/akka-cluster-sharding-typed/src/main/scala/akka/cluster/sharding/typed/scaladsl/ClusterSharding.scala
index 890c85e7ff..4e6d93e23d 100644
--- a/akka-cluster-sharding-typed/src/main/scala/akka/cluster/sharding/typed/scaladsl/ClusterSharding.scala
+++ b/akka-cluster-sharding-typed/src/main/scala/akka/cluster/sharding/typed/scaladsl/ClusterSharding.scala
@@ -7,7 +7,6 @@ package scaladsl

 import scala.concurrent.Future

-import scala.collection.{ immutable ⇒ _ }
 import scala.reflect.ClassTag

 import akka.util.Timeout
diff --git a/akka-cluster-typed/src/main/scala/akka/cluster/ddata/typed/scaladsl/DistributedData.scala b/akka-cluster-typed/src/main/scala/akka/cluster/ddata/typed/scaladsl/DistributedData.scala
index 8905fcf26a..ed5dfadda8 100644
--- a/akka-cluster-typed/src/main/scala/akka/cluster/ddata/typed/scaladsl/DistributedData.scala
+++ b/akka-cluster-typed/src/main/scala/akka/cluster/ddata/typed/scaladsl/DistributedData.scala
@@ -11,7 +11,6 @@ import akka.actor.typed.ActorRef
 import akka.actor.ExtendedActorSystem
 import akka.actor.typed.Props
 import akka.cluster.{ ddata ⇒ dd }
-import akka.cluster.typed.{ Cluster ⇒ _ }
 import akka.cluster.ddata.SelfUniqueAddress

 object DistributedData extends ExtensionId[DistributedData] {

marcelocenerine · 2018-12-16T00:57:58Z

scalafix-rules/src/main/scala/scalafix/internal/rule/RemoveUnused.scala

+      fix(doc.tree, unusedTermsPosition.toSet, unusedImportsPosition.toSet)
+    }
+  }
+


Algorithm:

traverses the tree using depth-first traversal

for each enclosing scope (source, package, object, class, def, block, ...)

any used import wildcards AND unused import renames are tracked and passed down into nested scopes following them

any other unused import gets removed

at the end of each scope

any unused import rename (from outer our current scope) whose package is the same as any wildcard import in scope (outer or current) gets unimported

any remaining unused import rename belonging exclusively to the current scope (not the outer) gets removed

👍 would be good to include this description in the comments

olafurpg

Thank you @marcelocenerine! Great job with the thorough test suite and evaluation 👍

I am a bit concerned about the increased complexity from this change. I am wondering if we can achieve the same goal while keeping a stateless tree traverser. I took a stab at this issue here master...olafurpg:fix_rename_unused_import2

In short, we add a new guard for the "unimport rename" case to only unimport renames when they're not part of an importer with a used wildcard.

          val isUsedWildcard = importer.importees.exists {
            case i: Importee.Wildcard => !isUnusedImport(importPosition(i))
            case _ => false
          }
          importer.importees.collect {
            case i @ Importee.Rename(_, to)
                if isUsedWildcard &&
                  isUnusedImport(importPosition(i)) => // unimport rename

This change caused several imports to get removed from the test suite. Consider the following case

bash-3.2$ cat err.scala
import scala.util._
import scala.util.{Success => S}

object A {
  Success(1)
}
bash-3.2$ scalac -Ywarn-unused err.scala
err.scala:2: warning: Unused import
import scala.util.{Success => S}
                   ^
one warning found

I think we are save to remove the rename because Success is anyways brought in through the wildcard import.

olafurpg · 2018-12-17T09:43:46Z

scalafix-rules/src/main/scala/scalafix/internal/rule/RemoveUnused.scala

+      fix(doc.tree, unusedTermsPosition.toSet, unusedImportsPosition.toSet)
+    }
+  }
+


👍 would be good to include this description in the comments

marcelocenerine · 2018-12-17T14:18:27Z

I am a bit concerned about the increased complexity from this change

Indeed it adds some complexity to handle the case where the rename and wildcard are in different import clauses. If the original ask (#614) was only about unused renames in the same import as the wildcard, then your suggestion is good enough.

marcelocenerine · 2018-12-19T09:18:29Z

@olafurpg do you prefer the simpler approach and restrict the fix for the original issue (#614) only to unused renames in the same import as the wildcard?

olafurpg · 2018-12-19T09:21:16Z

I think the simple solution fixes also #915

In the diff here 38db60c#diff-5a5912bc5da72f0e5da63e0919c8d2d0 for example it removes import scala.io.StdIn.{readByte => _} imports

olafurpg

Can we add back the test suite you wrote in the original PR? It was very thorough and I think it can be helpful to catch regressions in the future. Otherwise, LGTM 👍

I'm sorry for the slow response!

olafurpg

Can we add back the test suite you wrote in the original PR? It was very thorough and I think it can be helpful to catch regressions in the future. Otherwise, LGTM 👍

I'm sorry for the slow response!

olafurpg

Can we add back the test suite you wrote in the original PR? It was very thorough and I think it can be helpful to catch regressions in the future. Otherwise, LGTM 👍

I'm sorry for the slow response!

marcelocenerine · 2019-01-15T01:02:44Z

Hey @olafurpg, many of those test cases were more relevant to the original algorithm. I added most of them back, except the ones related to scoping. The new implementation doesn't care about scopes

olafurpg

LGTM 👍 Thank you @marcelocenerine !

dwijnand · 2019-01-16T07:50:12Z

Thanks, @marcelocenerine!

marcelocenerine requested review from olafurpg and ShaneDelmore December 16, 2018 00:54

marcelocenerine commented Dec 16, 2018

View reviewed changes

marcelocenerine force-pushed the fix_rename_unused_import2 branch 2 times, most recently from 77d72f3 to 5ad04a2 Compare December 16, 2018 12:42

olafurpg suggested changes Dec 17, 2018

View reviewed changes

Remove Importee.Rename if there is no used wildcard in the same Importer

141bd62

marcelocenerine force-pushed the fix_rename_unused_import2 branch from 5ad04a2 to 141bd62 Compare December 21, 2018 23:53

olafurpg suggested changes Jan 11, 2019

View reviewed changes

add tests back

3221958

olafurpg approved these changes Jan 16, 2019

View reviewed changes

olafurpg merged commit c76602a into scalacenter:master Jan 16, 2019

olafurpg mentioned this pull request Jan 16, 2019

RemoveUnusedImports does not handle wildcard imports correctly. #614

Closed

marcelocenerine deleted the fix_rename_unused_import2 branch November 23, 2023 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Importee.Rename if it does not shadow anything #923

Remove Importee.Rename if it does not shadow anything #923

marcelocenerine commented Dec 16, 2018 •

edited

marcelocenerine Dec 16, 2018 •

edited

olafurpg Dec 17, 2018

olafurpg left a comment

olafurpg Dec 17, 2018

marcelocenerine commented Dec 17, 2018

marcelocenerine commented Dec 19, 2018

olafurpg commented Dec 19, 2018

olafurpg left a comment

olafurpg left a comment

olafurpg left a comment

marcelocenerine commented Jan 15, 2019

olafurpg left a comment

dwijnand commented Jan 16, 2019

Remove Importee.Rename if it does not shadow anything #923

Remove Importee.Rename if it does not shadow anything #923

Conversation

marcelocenerine commented Dec 16, 2018 • edited

marcelocenerine Dec 16, 2018 • edited

Choose a reason for hiding this comment

olafurpg Dec 17, 2018

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

olafurpg Dec 17, 2018

Choose a reason for hiding this comment

marcelocenerine commented Dec 17, 2018

marcelocenerine commented Dec 19, 2018

olafurpg commented Dec 19, 2018

olafurpg left a comment

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

marcelocenerine commented Jan 15, 2019

olafurpg left a comment

Choose a reason for hiding this comment

dwijnand commented Jan 16, 2019

marcelocenerine commented Dec 16, 2018 •

edited

marcelocenerine Dec 16, 2018 •

edited