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.

Sign up for GitHub

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

Scalafix rename that leads to a name collision breaks compilation #1695

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Scalafix rename that leads to a name collision breaks compilation #1695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions scalafix-core/src/main/scala/scalafix/internal/patch/ReplaceSymbolOps.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import scalafix.patch.Patch
import scalafix.patch.Patch.internal.ReplaceSymbol
import scalafix.syntax._
import scalafix.v0._
import scala.annotation.tailrec


object ReplaceSymbolOps {
private object Select {
Expand All @@ -19,6 +21,20 @@ object ReplaceSymbolOps {
case _ => None
}
}
private def extractImports(stats: Seq[Stat]): Seq[Import] = {
stats
.takeWhile(_.is[Import])
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this useful?

.collect { case i: Import => i }
}

@tailrec private final def getGlobalImports(ast: Tree): Seq[Import] =
Copy link
Collaborator

Choose a reason for hiding this comment

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

this could be more granular and collect relevant scoped imports, all the way to the global ones at the top

ast match {
case Pkg(_, Seq(pkg: Pkg)) => getGlobalImports(pkg)
case Source(Seq(pkg: Pkg)) => getGlobalImports(pkg)
case Pkg(_, stats) => extractImports(stats)
case Source(stats) => extractImports(stats)
case _ => Nil
}

def naiveMoveSymbolPatch(
moveSymbols: Seq[ReplaceSymbol]
Expand Down Expand Up @@ -92,6 +108,7 @@ object ReplaceSymbolOps {
case _ => None
}
}

val patches = ctx.tree.collect { case n @ Move(to) =>
// was this written as `to = "blah"` instead of `to = _root_.blah`
val isSelected = to match {
Expand Down Expand Up @@ -126,10 +143,20 @@ object ReplaceSymbolOps {
if sig.name != parent.value =>
Patch.empty // do nothing because it was a renamed symbol
case Some(parent) =>
val causesCollision = getGlobalImports(ctx.tree).exists { importStmt =>
Copy link
Author

Choose a reason for hiding this comment

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

It would be more performant to get the global imports from the start and manipulate them as patches are processed. We want to handle for foo.Bar => foo.Baz and bar.Foo => bar.Baz without breaking compilation, so we have to consider previous renames as well. Now that I think more on it, I guess for completeness we'd also want to handle non-import collisions. Open to suggestions on these.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that I think more on it, I guess for completeness we'd also want to handle non-import collisions. Open to suggestions on these.

Indeed, although this would be more complex as there is not enough information in the tree nor in semanticdb to list all locals for example. You'd have to start a presentation compiler instance (outside the scalafix API), just like ExplicitResultTypes, which would greatly increase code complexity and ease-of-use (as scalafix's Scala binary version must match the target's). Without going that way, there are some heuristics that could prevent non-import collisions (getting visibile members from super traits, getting local val names via the tree, etc), but it's a lot of work to cover them all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be more performant to get the global imports from the start

Indeed - is it possible to call getGlobalImports once outside the tree traversal?

importStmt.importers.flatMap(_.importees).exists {
case Importee.Name(name) => name.value == to.signature.name
case Importee.Rename(_, rename) => rename.value == to.signature.name
case _ => false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wildcards can also cause collisions (although we can't tell using Scalafix APIs). This is probably outside the scope of this PR, but we could have an opt-in "safe" flag that disable global imports whenever it finds wildcard imports.

}
}
val addImport =
if (n.isDefinition) Patch.empty
if (n.isDefinition || causesCollision) Patch.empty
else ctx.addGlobalImport(to)
addImport + ctx.replaceTree(n, to.signature.name)
if(causesCollision)
addImport + ctx.replaceTree(n, to.owner.syntax + to.signature.name)
else
addImport + ctx.replaceTree(n, to.signature.name)
case _ =>
Patch.empty
}
Expand Down
7 changes: 6 additions & 1 deletion scalafix-tests/input/src/main/scala-2/test/ReplaceSymbol.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ patches.replaceSymbols = [
to = "com.geirsson.mutable.CoolBuffer" }
{ from = "scala.collection.mutable.HashMap"
to = "com.geirsson.mutable.unsafe.CoolMap" }
{ from = "scala.collection.immutable.TreeMap"
to = "com.geirsson.immutable.SortedMap" }
{ from = "scala.math.sqrt"
to = "com.geirsson.fastmath.sqrt" }
// normalized symbol renames all overloaded methods
Expand All @@ -29,6 +31,8 @@ patches.replaceSymbols = [
*/
package fix

import scala.collection.immutable.SortedMap
import scala.collection.immutable.TreeMap
import scala.collection.mutable.HashMap
import scala.collection.mutable.ListBuffer
import scala.collection.mutable
Expand All @@ -42,8 +46,9 @@ object ReplaceSymbol {
"blah".substring(1)
"blah".substring(1, 2)
val u: mutable.HashMap[Int, Int] = HashMap.empty[Int, Int]
val v: SortedMap[Int, Int] = TreeMap.empty[Int, Int]
val x: ListBuffer[Int] = ListBuffer.empty[Int]
val y: mutable.ListBuffer[Int] = mutable.ListBuffer.empty[Int]
val z: scala.collection.mutable.ListBuffer[Int] =
scala.collection.mutable.ListBuffer.empty[Int]
}
}
11 changes: 11 additions & 0 deletions scalafix-tests/output/src/main/scala-2/com/geirsson/mutable/immutable.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package com.geirsson

import scala.collection.immutable.TreeMap

package object immutable {
type SortedMap[A, B] = TreeMap[A, B]

object SortedMap {
def empty[A : Ordering, B]: SortedMap[A, B] = TreeMap.empty[A, B]
}
}
4 changes: 3 additions & 1 deletion scalafix-tests/output/src/main/scala-2/test/ReplaceSymbol.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package fix

import scala.collection.immutable.SortedMap
import com.geirsson.Future
import com.geirsson.{ fastmath, mutable }
import com.geirsson.mutable.{ CoolBuffer, unsafe }
Expand All @@ -13,8 +14,9 @@ object ReplaceSymbol {
"blah".substringFrom(1)
"blah".substringBetween(1, 2)
val u: unsafe.CoolMap[Int, Int] = CoolMap.empty[Int, Int]
val v: SortedMap[Int, Int] = com.geirsson.immutable.SortedMap.empty[Int, Int]
val x: CoolBuffer[Int] = CoolBuffer.empty[Int]
val y: mutable.CoolBuffer[Int] = mutable.CoolBuffer.empty[Int]
val z: com.geirsson.mutable.CoolBuffer[Int] =
com.geirsson.mutable.CoolBuffer.empty[Int]
}
}