Scalafix rename that leads to a name collision breaks compilation #1695
Conversation
| @@ -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 => | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Welcome to the project and thanks a lot for putting this together! This feature has a lot of shortcomings, and this does remove one 👍
Don't forget to run scalafixAll before committing (see CI failure https://github.com/scalacenter/scalafix/actions/runs/3266135538/jobs/5369523474).
| @@ -19,6 +21,20 @@ object ReplaceSymbolOps { | |||
| case _ => None | |||
| } | |||
| } | |||
| private def extractImports(stats: Seq[Stat]): Seq[Import] = { | |||
| stats | |||
| .takeWhile(_.is[Import]) | |||
| @@ -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 => | |||
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
| .collect { case i: Import => i } | ||
| } | ||
|
|
||
| @tailrec private final def getGlobalImports(ast: Tree): Seq[Import] = |
There was a problem hiding this comment.
this could be more granular and collect relevant scoped imports, all the way to the global ones at the top
| @@ -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 => | |||
There was a problem hiding this comment.
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?
|
@jmcrawford45 will you have time to pursue this? |
|
@jmcrawford45 I would love to get that in, any chance you could address the outstanding comments? Thanks! |
Fixes #1305