Merge pull request #152 from mlangc/1002651-rename-breaks-with-interp…

…olated-strings

Fix rename for interpolated strings like f"$x"

src/main/scala/scala/tools/refactoring/analysis/GlobalIndexes.scala

@@ -8,6 +8,8 @@ package analysis
 import collection.mutable.ListBuffer
 import collection.mutable.HashSet
 import annotation.tailrec
+import scala.reflect.internal.util.OffsetPosition
+import scala.reflect.internal.util.RangePosition
 
 /**
  * Provides an implementation of the Indexes.IndexLookup trait
@@ -94,8 +96,7 @@ trait GlobalIndexes extends Indexes with DependentSymbolExpanders with Compilati
     def occurences(s: global.Symbol) = {
       val expandedSymbol = expandSymbol(s)
 
-      expandedSymbol.flatMap { sym =>
-
+      val trees = expandedSymbol.flatMap { sym =>
         val decs = declaration(sym).toList
         val refs = cus.flatMap { cu =>
           cu.references.get(sym).toList.flatten
@@ -104,7 +105,6 @@ trait GlobalIndexes extends Indexes with DependentSymbolExpanders with Compilati
         decs ::: refs
 
       }.filter {
-
         // see SI-6141
         case t: Ident => t.pos.isRange
 
@@ -115,6 +115,11 @@ trait GlobalIndexes extends Indexes with DependentSymbolExpanders with Compilati
           val src = t.pos.source.content.slice(t.pos.start, t.pos.end).mkString
           src != "super"
 
+        // Takes care of renames for interpolated strings (see #1002651), since
+        // the presentation compiler might fail to properly assign range positions.
+        case t: RefTree if !t.pos.isRange && t.pos.isDefined && isArgOfStringContextApply(t) =>
+          true
+
         case t if t.pos.isTransparent =>
           // We generally want to skip transparent positions,
           // so if one of the children is an opaque range, we
@@ -123,7 +128,50 @@ trait GlobalIndexes extends Indexes with DependentSymbolExpanders with Compilati
 
         case t =>
           t.pos.isOpaqueRange
-      }.distinct
+      }
+
+      onlyTreesWithRangesOrOffsetsOutsideOfAllRanges(onlyTreesWithDistinctPositions(trees))
+    }
+
+    private def isArgOfStringContextApply(t: Tree): Boolean = {
+      rootsOf(List(t)).exists { root =>
+        root.exists { tree =>
+          tree match {
+            case Apply(fun, args) if fun.pos.isTransparent && args.contains(t) =>
+              fun.toString().startsWith("scala.StringContext.apply(")
+            case _ =>
+              false
+          }
+        }
+      }
+    }
+
+    private def onlyTreesWithDistinctPositions(trees: List[Tree]): List[Tree] = {
+      trees.groupBy(_.pos).map { case (_, trees) => trees.head }.toList
+    }
+
+    private def onlyTreesWithRangesOrOffsetsOutsideOfAllRanges(trees: List[Tree]): List[Tree] = {
+      val (offsets, ranges) = trees.foldLeft((List[OffsetPosition](), List[RangePosition]())) { case ((offsets, ranges), tree) =>
+        tree.pos match {
+          case range: RangePosition => (offsets, range :: ranges)
+          case offset: OffsetPosition => (offset :: offsets, ranges)
+          case unexpected => throw new AssertionError(s"Cannot handle unexpected position $unexpected in $tree")
+        }
+      }
+
+      val offsetsToKeep = offsets.filter { offset =>
+        ranges.forall { range =>
+          val pointWithinRange = range.start <= offset.point && range.end > offset.point
+          !pointWithinRange
+        }
+      }.toSet
+
+      trees.filter { tree =>
+        tree.pos match {
+          case _: RangePosition => true
+          case offset: OffsetPosition => offsetsToKeep.contains(offset)
+        }
+      }
     }
 
     def allDefinedSymbols = cus.flatMap(_.definitions.keys)

src/main/scala/scala/tools/refactoring/common/PositionDebugging.scala

@@ -33,14 +33,14 @@ object PositionDebugging {
     if (pos != NoPosition && pos.source != NoSourceFile) {
       val posType = getSimpleClassName(pos)
 
-      val (start, end) = {
-        if (!pos.isRange) (pos.point, pos.point)
-        else (pos.start, pos.end)
+      val (start, point, end) = {
+        if (!pos.isRange) (pos.point, pos.point, pos.point)
+        else (pos.start, pos.point, pos.end)
       }
 
       val markerString = {
-        if (start == end) s"(${start})"
-        else s"(${start}, ${end})"
+        if (start == end) s"($start)"
+        else s"($start, $point, $end)"
       }
 
       val relevantSource = {

src/main/scala/scala/tools/refactoring/implementations/MarkOccurrences.scala

@@ -22,7 +22,7 @@ abstract class MarkOccurrences extends common.Selections with analysis.Indexes w
 
     def occurrencesForSymbol(s: Symbol) = {
       val occurrences = index.occurences(s)
-      occurrences map (_.namePosition) filter  (_ != global.NoPosition)
+      occurrences map (_.namePosition) filter  (_.isRange)
     }
 
     val selectedTree = (new FileSelection(file, global.unitOfFile(file).body, from, to)).findSelectedWithPredicate {

src/main/scala/scala/tools/refactoring/implementations/Rename.scala

@@ -15,6 +15,7 @@ import scala.tools.refactoring.util.SourceWithMarker.Movements
 import scala.tools.refactoring.common.TextChange
 import scala.tools.refactoring.common.RenameSourceFileChange
 import scala.tools.refactoring.common.Change
+import scala.reflect.internal.util.OffsetPosition
 
 abstract class Rename extends MultiStageRefactoring with TreeAnalysis with analysis.Indexes with TreeFactory with common.InteractiveScalaCompiler {
 
@@ -160,17 +161,41 @@ abstract class Rename extends MultiStageRefactoring with TreeAnalysis with analy
       }
 
       import Movements._
-      val mvToSymStart = Movements.until(oldName, skipping = (comment | space | reservedName))
+      val mvToSymStartForRangePos = Movements.until(oldName, skipping = (comment | space | reservedName))
       val textChangesWithTrees = occurences.flatMap { occ =>
         occ.namePosition() match {
           case np: RangePosition =>
             // Unfortunately, there are cases when the name position cannot be used directly.
             // Therefore we have to use an appropriate movement to make sure we capture the correct range.
             val srcAtStart = SourceWithMarker.atStartOf(np)
-            mvToSymStart(srcAtStart).map { markerAtSymStart =>
+            mvToSymStartForRangePos(srcAtStart).map { markerAtSymStart =>
               (TextChange(np.source, markerAtSymStart, markerAtSymStart + oldName.length, newName), occ)
             }
 
+          case op: OffsetPosition =>
+            // Normally, we would not like to deal with offset positions here at all.
+            // Unfortunately the compiler emits them instead of range positions in
+            // interpolated strings like f"$x" (see #1002651).
+            val pointIsAtStartOfNameToBeChanged = {
+              val srcBeforePoint = SourceWithMarker.atPoint(op).step(forward = false)
+              val pointIsInMiddleOfId = Movements.id(srcBeforePoint).exists(_ > op.point)
+
+              if (pointIsInMiddleOfId) {
+                false
+              } else {
+                val srcAtPoint = srcBeforePoint.step(forward = true)
+                val srcAfterId = srcAtPoint.moveMarker(Movements.id)
+                val consumedId = srcAtPoint.source.slice(srcAtPoint.marker, srcAfterId.marker).mkString("")
+                consumedId == oldName
+              }
+            }
+
+            if (pointIsAtStartOfNameToBeChanged) {
+              Some((TextChange(op.source, op.point, op.point + oldName.length, newName), occ))
+            } else {
+              None
+            }
+
           case _ =>
             None
         }

src/test/scala/scala/tools/refactoring/tests/implementations/RenameTest.scala

@@ -3752,4 +3752,51 @@ class Blubb
     class SomeClass protected (/*(*/ups/*)*/: Int)
     """ -> TaggedAsGlobalRename
   } prepareAndApplyRefactoring(prepareAndRenameTo("ups"))
+
+  /*
+   * See Assembla Ticket 1002651
+   */
+  @Test
+  def testRenameWithInterpolatedString1002651Ex1() = new FileSet {
+    """
+    class Bug {
+      val /*(*/renameMe/*)*/ = 13
+      val bug = f"?renameMe"
+    }
+    """.replace("?", "$") becomes
+    """
+    class Bug {
+      val /*(*/ups/*)*/ = 13
+      val bug = f"?ups"
+    }
+    """.replace("?", "$") -> TaggedAsGlobalRename
+  } prepareAndApplyRefactoring(prepareAndRenameTo("ups"))
+
+  @Test
+  def testRenameWithInterpolatedString1002651Ex2() = new FileSet {
+    """
+    class Bug {
+      val /*(*/renameMe/*)*/ = 13
+      val renameMeNot = 14
+
+      val bug = f"?renameMe"
+      val moreBugs = f"?renameMe but ?renameMeNot and make sure that ?renameMe is renamed again"
+      val bugsAllOverThePlace = f"Plase, also ?{renameMe} here, but do ?{renameMeNot} here"
+
+      val thisWorkedBefore = s"Please ?renameMe like you did before"
+    }
+    """.replace("?", "$") becomes
+    """
+    class Bug {
+      val /*(*/franzi/*)*/ = 13
+      val renameMeNot = 14
+
+      val bug = f"?franzi"
+      val moreBugs = f"?franzi but ?renameMeNot and make sure that ?franzi is renamed again"
+      val bugsAllOverThePlace = f"Plase, also ?{franzi} here, but do ?{renameMeNot} here"
+
+      val thisWorkedBefore = s"Please ?franzi like you did before"
+    }
+    """.replace("?", "$") -> TaggedAsGlobalRename
+  } prepareAndApplyRefactoring(prepareAndRenameTo("franzi"))
 }