Scalafix rename that leads to a name collision breaks compilation

[mosesn]

Problem

If a rename causes a collision in the renamed file, you can no longer compile the file. Scalafix does not deal with the collision gracefully.

Proposed Solution

We can infer if the Scalafix rename would cause a collision by inspecting the identifiers that are at the scope of the import when it's renamed. This will be a little tricky (I believe imports are evaluated in order) as we want to know about all of the identifiers that come from imports. If the rename would cause a collision, we inline the fully qualified name and delete the old import, instead of performing the rename.

Next Steps

If someone is inspired to tackle this problem, I would be very grateful! If no one is feeling so inspired, I can take a stab at working on this, but I'll need guidance, since I'm a scalafix newb.

[bjaglin]

Thanks for the report @mosesn. You don't mention any version, so I assume it's not a regression?

To better understand the impact, I wonder if your specific problem is related to the direct usage of Patch.replaceSymbols as a Scalafix rule developer (like https://github.com/scalatest/autofix/blob/d1fad4842e65c9c54e710d8e9b9de39dfe4527ae/3.1.x/rules/src/main/scala/org/scalatest/autofix/v3_1_x/RewriteDeprecatedNames.scala#L11), or its indirect usage of it via --rules=replace:from/to / patches.replaceSymbols syntax (not documented because unsafe) as a Scalafix user?

I am not familiar with ReplaceSymbols, but the problem looks somewhat similar to what ExplicitResultTypes does: finding collision-free identifiers that are as "short" as possible to keep them human-readable. It's not trivial though, and requires help from the compiler at runtime. So I wonder if using fully-qualified-name as replacement target wouldn't workaround on your case?

In any case, the best next step would be to provide a reproduction, either as a snippet here, or even better, as a new/updated test case via testkit input/output files (see https://scalacenter.github.io/scalafix/docs/developers/tutorial.html#write-unit-tests & https://github.com/scalacenter/scalafix/blob/master/CONTRIBUTING.md#testing). The tests & the actual bugfix should touch similar files as https://github.com/scalacenter/scalafix/pull/1293/files.

[mosesn]

@bjaglin I used the nightly build from the day I filed the ticket. However, this issue also affected versions from a few months ago, so I don't believe it's a regression.

Indeed, that RewriteDeprecatedNames is the one that I'm trying to run. So it is Patch.replaceSymbols.

Fully qualified name does work, but I'd rather only use FQN as a fall-back for collisions. This seems like it's a better default for ScalaFix too–it means that ScalaFix developers don't need to guess which identifiers will collide for their customers, and removes a class of compilation errors.

Let me see about writing a reproduction, thanks!

[mosesn]

@bjaglin I made a reproduction case:

mosesn@7b7f536

Output:

=======
=> Diff
=======
--- obtained
+++ expected
@@ -5,3 +5,2 @@
 import com.geirsson.{ fastmath, mutable }
-import com.geirsson.immutable.SortedMap
 import com.geirsson.mutable.{ CoolBuffer, unsafe }
@@ -17,3 +16,3 @@
   val u: unsafe.CoolMap[Int, Int] = CoolMap.empty[Int, Int]
-  val v: SortedMap[Int, Int] = SortedMap.empty[Int, Int]
+  val v: SortedMap[Int, Int] = com.geirsson.immutable.SortedMap.empty[Int, Int]
   val x: CoolBuffer[Int] = CoolBuffer.empty[Int]

In this example, we end up importing two kinds of SortedMaps in the output. However, this will always break compilation.

[mosesn]

@bjaglin happy new year! What would you suggest I do here? Can you give me tips on how to tackle this myself? Or do you think this is best solved by a Scalafix expert?

[bjaglin]

@mosesn I don't have much free time these days. I would need to look at it to give you any pointer. Hopefully I can get to that within one or two weeks.

[mosesn]

@bjaglin thank you!

[bjaglin]

Sorry for the delay on this one. As far as I can tell, there is no easy, silver-bullet way to fix that, as a proper scope analysis would require access to the presentation compiler (like in ExplicitResultTypes), which would imply a major rework.

However, you can probably improve the rule correctness by preventing a global import (and favoring a fully-qualified reference) if there is an existing import with the same target name. It won't cover import wiltdcards though. I expect that fix to be limited to https://github.com/scalacenter/scalafix/blob/main/scalafix-core/src/main/scala/scalafix/internal/patch/ReplaceSymbolOps.scala.

[bjaglin]

Related: #1168