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.

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

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

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

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

@bjaglin thank you!

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.

Related: #1168