Merge pull request #156 from mlangc/1002650-rename-fails-in-for-compr…

…ehensions

Fix rename in for comprehensions

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

@@ -33,7 +33,8 @@ trait GlobalIndexes extends Indexes with DependentSymbolExpanders with Compilati
           LazyValAccessor with
           OverridesInSuperClasses with
           ClassVals with
-          CaseClassVals {
+          CaseClassVals with
+          TermsWithMissingRanges {
 
             val cus = compilationUnits
           }

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

@@ -8,6 +8,8 @@ package analysis
 import tools.nsc.symtab.Flags
 import scala.tools.refactoring.common.TracingImpl
 import scala.tools.refactoring.common.PositionDebugging
+import scala.reflect.internal.util.RangePosition
+import scala.reflect.internal.util.OffsetPosition
 
 /**
  * Provides various traits that are used by the indexer
@@ -205,4 +207,37 @@ trait DependentSymbolExpanders extends TracingImpl {
       case _ => Nil
     })
   }
+
+  /**
+   * Associates term symbols with missing ranges to related symbols that have ranges.
+   *
+   * The reason that we need this is that in some cases, the PC generates multiple
+   * symbols for one and the same symbol in user source code, one of them with a
+   * proper range position, and others just with offset positions. One place where
+   * this happens is in desugared for comprehensions with filter clauses.
+   * See Assembler Ticket #1002650.
+   */
+  trait TermsWithMissingRanges extends SymbolExpander { this: IndexLookup =>
+    protected abstract override def doExpand(s: Symbol): List[Symbol] = {
+      termsWithSamePointButRange(s) ::: super.doExpand(s)
+    }
+
+    private def termsWithSamePointButRange(s: Symbol): List[Symbol] = {
+      val lookAtSymbol = {
+        s != NoSymbol && !s.isSynthetic && s.isInstanceOf[TermSymbol] &&
+          s.pos.isInstanceOf[OffsetPosition] && !s.pos.isInstanceOf[RangePosition]
+      }
+
+      if (!lookAtSymbol) {
+        Nil
+      } else {
+        allDefinedSymbols().filter { ds =>
+          ds.pos match {
+            case rp: RangePosition => rp.point == s.pos.point && ds.nameString == s.nameString
+            case _ => false
+          }
+        }
+      }
+    }
+  }
 }

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

@@ -252,52 +252,6 @@ trait TreeTraverser extends TracingImpl {
 
       t match {
 
-        /* For comprehensions are tricky, especially when combined with a filter:
-         * The following code:
-         *
-         *   for (foo <- List("santa", "claus") if foo.startsWith("s")) yield foo
-         *
-         * is converted to
-         *
-         *   List("santa", "claus").withFilter(foo => foo.startsWith("s")).map(foo => foo)
-         *
-         * so we have several foo ValDefs/RefTrees that need to be related with
-         * each other.
-         */
-        case Apply(generator, (Function(arg :: _, body)) :: Nil)
-          if arg.pos.startOrPoint < generator.pos.startOrPoint &&
-             between(arg, generator).contains("<-") =>
-
-          val referencesToVal = {
-            val fromBody = body collect {
-              case t: RefTree if t.name == arg.name => t
-            }
-            val fromGenerator = generator.collect {
-              case Apply(Select(_, nme.withFilter), (Function((v: ValDef) :: _, body)) :: Nil)
-                if !v.pos.isRange && v.name == arg.name =>
-                  body collect {
-                    case t: RefTree if t.symbol == v.symbol => t
-                  }
-            }.flatten
-
-            fromBody ++ fromGenerator
-          }
-
-          val valDefs = arg :: {
-            generator collect {
-              case Apply(Select(_, nme.withFilter), (Function((v: ValDef) :: _, _)) :: Nil)
-                if !v.pos.isRange && v.name == arg.name =>
-                  v
-            }
-          }
-
-          referencesToVal foreach { ref =>
-            valDefs foreach { valDef =>
-              f(valDef.symbol, ref)
-              f(ref.symbol, valDef)
-            }
-          }
-
         case t: TypeTree if t.original != null =>
 
           (t.original, t.tpe) match {
@@ -420,11 +374,5 @@ trait TreeTraverser extends TracingImpl {
           super.traverse(t)
       }
     }
-
-    private def between(t1: Tree, t2: Tree) = {
-      if(t1.pos.isRange && t2.pos.isRange)
-        t1.pos.source.content.slice(t1.pos.end, t2.pos.start).mkString
-      else ""
-    }
   }
 }

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

@@ -3799,4 +3799,133 @@ class Blubb
     }
     """.replace("?", "$") -> TaggedAsGlobalRename
   } prepareAndApplyRefactoring(prepareAndRenameTo("franzi"))
+
+  /*
+   * See Assembla Ticket 1002650
+   */
+  @Test
+  def testRenameWithForComprehensions1002650Ex1() = new FileSet {
+    """
+    class Bug1 {
+      for {
+        (/*(*/renameMe/*)*/, b) <- Seq((1, 2)) if renameMe % 2 == 0
+      } {
+        println(renameMe)
+      }
+    }
+    """ becomes
+    """
+    class Bug1 {
+      for {
+        (/*(*/ups/*)*/, b) <- Seq((1, 2)) if ups % 2 == 0
+      } {
+        println(ups)
+      }
+    }
+    """ -> TaggedAsLocalRename
+  } prepareAndApplyRefactoring(prepareAndRenameTo("ups"))
+
+  @Test
+  def testRenameWithForComprehensions1002650Ex2() = new FileSet {
+    """
+    class Bug1 {
+      for {
+        (renameMe, b) <- Seq((1, 2)) if /*(*/renameMe/*)*/ % 2 == 0
+      } {
+        println(renameMe)
+      }
+    }
+    """ becomes
+    """
+    class Bug1 {
+      for {
+        (ups, b) <- Seq((1, 2)) if /*(*/ups/*)*/ % 2 == 0
+      } {
+        println(ups)
+      }
+    }
+    """ -> TaggedAsLocalRename
+  } prepareAndApplyRefactoring(prepareAndRenameTo("ups"))
+
+  @Test
+  def testRenameWithForComprehensions1002650Ex3() = new FileSet {
+    """
+    class Bug1 {
+      for {
+        (renameMe, b) <- Seq((1, 2)) if renameMe % 2 == 0
+      } {
+        println(/*(*/renameMe/*)*/)
+      }
+    }
+    """ becomes
+    """
+    class Bug1 {
+      for {
+        (ups, b) <- Seq((1, 2)) if ups % 2 == 0
+      } {
+        println(/*(*/ups/*)*/)
+      }
+    }
+    """ -> TaggedAsLocalRename
+  } prepareAndApplyRefactoring(prepareAndRenameTo("ups"))
+
+  @Test
+  def testRenameWithForComprehensions1002650Ex4() = new FileSet {
+    """
+    class Bug2 {
+      for {
+        /*(*/tryRenameMe/*)*/ <- Option(Option(1))
+      } {
+        for {
+          tryRenameMe <- tryRenameMe
+        } yield {
+          tryRenameMe + 1
+        }
+      }
+    }
+    """ becomes
+    """
+    class Bug2 {
+      for {
+        /*(*/ups/*)*/ <- Option(Option(1))
+      } {
+        for {
+          tryRenameMe <- ups
+        } yield {
+          tryRenameMe + 1
+        }
+      }
+    }
+    """ -> TaggedAsLocalRename
+  } prepareAndApplyRefactoring(prepareAndRenameTo("ups"))
+
+  @Test
+  def testRenameWithForComprehensions1002650Ex5() = new FileSet {
+    """
+    class Bug2 {
+      for {
+        tryRenameMe <- Option(Option(1))
+      } {
+        for {
+          tryRenameMe <- tryRenameMe
+        } yield {
+          /*(*/tryRenameMe/*)*/ + 1
+        }
+      }
+    }
+    """ becomes
+    """
+    class Bug2 {
+      for {
+        tryRenameMe <- Option(Option(1))
+      } {
+        for {
+          ups <- tryRenameMe
+        } yield {
+          /*(*/ups/*)*/ + 1
+        }
+      }
+    }
+    """ -> TaggedAsLocalRename
+  } prepareAndApplyRefactoring(prepareAndRenameTo("ups"))
 }