Fix Patch.replaceSymbols bug that affects ScalaTest rule. (#1293)

Previously, the ScalaTest autofix rule didn't properly handle cases with
renamed imports due to a bug in Scalafix. This commit fixes that bug so
that the ScalaTest rule works correctly as expected.

Author: olafurpg

scalafix-core/src/main/scala/scalafix/internal/patch/ReplaceSymbolOps.scala

@@ -3,6 +3,7 @@ package scalafix.internal.patch
 import scala.meta._
 import scala.meta.internal.trees._
 
+import scalafix.internal.util.SymbolOps
 import scalafix.internal.util.SymbolOps.Root
 import scalafix.internal.util.SymbolOps.SignatureName
 import scalafix.patch.Patch
@@ -38,14 +39,19 @@ object ReplaceSymbolOps {
             (Symbol.Global(qual, Signature.Type(name)).syntax -> to) ::
             Nil
       }.toMap
-    def loop(ref: Ref, sym: Symbol): (Patch, Symbol) = {
+    def loop(ref: Ref, sym: Symbol, isImport: Boolean): (Patch, Symbol) = {
       (ref, sym) match {
         // same length
         case (a @ Name(_), Symbol.Global(Symbol.None, SignatureName(b))) =>
           ctx.replaceTree(a, b) -> Symbol.None
         // ref is shorter
-        case (a @ Name(_), sym @ Symbol.Global(_, SignatureName(b))) =>
-          ctx.replaceTree(a, b) -> sym
+        case (a @ Name(_), sym @ Symbol.Global(owner, SignatureName(b))) =>
+          if (isImport) {
+            val qual = SymbolOps.toTermRef(sym)
+            ctx.replaceTree(a, qual.syntax) -> Symbol.None
+          } else {
+            ctx.replaceTree(a, b) -> sym
+          }
         // ref is longer
         case (
               Select(qual, Name(_)),
@@ -57,7 +63,7 @@ object ReplaceSymbolOps {
               Select(qual: Ref, a @ Name(_)),
               Symbol.Global(symQual, SignatureName(b))
             ) =>
-          val (patch, toImport) = loop(qual, symQual)
+          val (patch, toImport) = loop(qual, symQual, isImport)
           (patch + ctx.replaceTree(a, b)) -> toImport
       }
     }
@@ -79,6 +85,13 @@ object ReplaceSymbolOps {
         result
       }
     }
+    object Identifier {
+      def unapply(tree: Tree): Option[(Name, Symbol)] = tree match {
+        case n: Name => n.symbol.map(s => n -> s)
+        case Init(n: Name, _, _) => n.symbol.map(s => n -> s)
+        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 {
@@ -88,14 +101,37 @@ object ReplaceSymbolOps {
       n.parent match {
         case Some(i @ Importee.Name(_)) =>
           ctx.removeImportee(i)
+        case Some(i @ Importee.Rename(name, rename)) =>
+          i.parent match {
+            case Some(Importer(ref, `i` :: Nil)) =>
+              Patch.replaceTree(ref, SymbolOps.toTermRef(to.owner).syntax) +
+                Patch.replaceTree(name, to.signature.name)
+            case Some(Importer(ref, _)) =>
+              Patch.removeImportee(i) +
+                Patch.addGlobalImport(
+                  Importer(
+                    SymbolOps.toTermRef(to.owner),
+                    List(
+                      Importee.Rename(Term.Name(to.signature.name), rename)
+                    )
+                  )
+                )
+            case _ => Patch.empty
+          }
         case Some(parent @ Select(_, `n`)) if isSelected =>
-          val (patch, imp) = loop(parent, to)
+          val (patch, imp) =
+            loop(parent, to, isImport = n.parents.exists(_.is[Import]))
           ctx.addGlobalImport(imp) + patch
-        case Some(_) =>
+        case Some(Identifier(parent, Symbol.Global(_, sig)))
+            if sig.name != parent.value =>
+          Patch.empty // do nothing because it was a renamed symbol
+        case Some(parent) =>
           val addImport =
             if (n.isDefinition) Patch.empty
             else ctx.addGlobalImport(to)
           addImport + ctx.replaceTree(n, to.signature.name)
+        case _ =>
+          Patch.empty
       }
     }
     patches.asPatch

scalafix-tests/input/src/main/scala/org/scalatest_autofix/Matchers.scala

@@ -0,0 +1,9 @@
+/*
+ignore = true
+*/
+package org.scalatest_autofix
+
+object Matchers extends Matchers
+class Matchers {
+  def shouldBe(n: Int): Unit = ???
+}

scalafix-tests/input/src/main/scala/tests/scalatest_autofix/ScalatestAutofixRule.scala

@@ -0,0 +1,10 @@
+/*
+rule = ScalatestAutofixRule
+*/
+package tests.scalatest_autofix
+
+import org.scalatest_autofix.Matchers._
+
+object ScalatestAutofixRule {
+  def foo(): Unit = shouldBe(1)
+}

scalafix-tests/input/src/main/scala/tests/scalatest_autofix/ScalatestAutofixRule2.scala

@@ -0,0 +1,22 @@
+/*
+rule = ScalatestAutofixRule
+*/
+package tests.scalatest_autofix
+
+import scala.collection.mutable
+
+object ScalatestAutofixRule2 {
+  object WithRename {
+    import org.scalatest_autofix.{Matchers => ScalaTestMatchers}
+
+    class UsesRename extends ScalaTestMatchers
+    class UsesOriginal extends org.scalatest_autofix.Matchers {
+      val x = mutable.ListBuffer.empty[Int]
+    }
+  }
+  object WithoutRename {
+    import org.scalatest_autofix.Matchers
+    class UsesRename extends Matchers
+    class UsesOriginal extends org.scalatest_autofix.Matchers
+  }
+}

scalafix-tests/output/src/main/scala/org/scalatest_autofix/matchers/should/Matchers.scala

@@ -0,0 +1,6 @@
+package org.scalatest_autofix.matchers.should
+
+object Matchers extends Matchers
+class Matchers {
+  def shouldBe(n: Int): Unit = ???
+}

scalafix-tests/output/src/main/scala/tests/scalatest_autofix/ScalatestAutofixRule.scala

@@ -0,0 +1,7 @@
+package tests.scalatest_autofix
+
+import org.scalatest_autofix.matchers.should.Matchers._
+
+object ScalatestAutofixRule {
+  def foo(): Unit = shouldBe(1)
+}

scalafix-tests/output/src/main/scala/tests/scalatest_autofix/ScalatestAutofixRule2.scala

@@ -0,0 +1,20 @@
+package tests.scalatest_autofix
+
+import scala.collection.mutable
+import org.scalatest_autofix.matchers
+import org.scalatest_autofix.matchers.should.Matchers
+
+object ScalatestAutofixRule2 {
+  object WithRename {
+    import org.scalatest_autofix.matchers.should.{Matchers => ScalaTestMatchers}
+
+    class UsesRename extends ScalaTestMatchers
+    class UsesOriginal extends matchers.should.Matchers {
+      val x = mutable.ListBuffer.empty[Int]
+    }
+  }
+  object WithoutRename {
+    class UsesRename extends Matchers
+    class UsesOriginal extends matchers.should.Matchers
+  }
+}

scalafix-tests/unit/src/main/resources/META-INF/services/scalafix.v1.Rule

@@ -2,6 +2,7 @@ banana.rule.SemanticRuleV1
 banana.rule.SyntacticRuleV1
 banana.rule.CommentFileNonAtomic
 banana.rule.CommentFileAtomic
+scalafix.test.ScalatestAutofixRule
 scalafix.test.ExplicitSynthetic
 scalafix.tests.cli.CrashingRule
 scalafix.tests.cli.NoOpRule

scalafix-tests/unit/src/main/scala/scalafix/test/ScalatestAutofixRule.scala

@@ -0,0 +1,12 @@
+package scalafix.test
+
+import scalafix.v1.SemanticRule
+import scalafix.v1._
+
+class ScalatestAutofixRule extends SemanticRule("ScalatestAutofixRule") {
+  override def fix(implicit doc: SemanticDocument): Patch = {
+    Patch.replaceSymbols(
+      "org.scalatest_autofix.Matchers" -> "org.scalatest_autofix.matchers.should.Matchers"
+    )
+  }
+}