Fix variable scope issue when renaming list comprehension variables (#293)

CHANGELOG.md

@@ -15,6 +15,7 @@
 - #398, #104 Fix parsing of nested `with` statement/context manager (@climbus)
 - #391, #376 Fix improper replacement when extracting attribute access expression with `similar=True` (@climbus)
 - #396 Fix improper replacement when extracting index access expression with `similar=True` (@lieryan)
+- #293 Fix rename global var affects list comprehension (@climbus)
 
 ## Misc
 

rope/base/evaluate.py

@@ -84,7 +84,7 @@ def get_pyname_at(self, offset):
 
     def get_primary_and_pyname_at(self, offset):
         lineno = self.lines.get_line_number(offset)
-        holding_scope = self.module_scope.get_inner_scope_for_line(lineno)
+        holding_scope = self.module_scope.get_inner_scope_for_offset(offset)
         # function keyword parameter
         if self.worder.is_function_keyword_parameter(offset):
             keyword_name = self.worder.get_word_at(offset)

rope/base/pyscopes.py

@@ -101,14 +101,15 @@ def get_logical_end(self):
     def get_kind(self):
         pass
 
-    @utils.saveit
     def get_region(self):
-        node = patchedast.patch_ast(
-            self.pyobject.get_ast(), self.pyobject.get_module().source_code
-        )
+        self._calculate_scope_regions_for_module()
+        node = self.pyobject.get_ast()
         region = patchedast.node_region(node)
         return region
 
+    def _calculate_scope_regions_for_module(self):
+        self._get_global_scope()._calculate_scope_regions()
+
     def in_region(self, offset):
         """Checks if offset is in scope region"""
 
@@ -135,6 +136,14 @@ def get_name(self, name):
                 return self.builtin_names[name]
             raise exceptions.NameNotFoundError("name %s not found" % name)
 
+    @utils.saveit
+    def _calculate_scope_regions(self):
+        source = self._get_source()
+        patchedast.patch_ast(self.pyobject.get_ast(), source)
+
+    def _get_source(self):
+        return self.pyobject.source_code
+
     def get_names(self):
         if self.names.get() is None:
             result = dict(self.builtin_names)
@@ -181,8 +190,8 @@ def _visit_comprehension(self):
             new_visitor = self.visitor(self.pycore, self.pyobject)
             for node in ast.get_child_nodes(self.pyobject.get_ast()):
                 ast.walk(node, new_visitor)
-            self.names = new_visitor.names
-            self.names.update(self.parent.get_names())
+            self.names = dict(self.parent.get_names())
+            self.names.update(new_visitor.names)
             self.defineds = new_visitor.defineds
 
     def get_logical_end(self):

rope/refactor/patchedast.py

@@ -97,14 +97,6 @@ def __call__(self, node):
             node.sorted_children = ast.get_children(node)
 
     def _handle(self, node, base_children, eat_parens=False, eat_spaces=False):
-        if hasattr(node, "region"):
-            # ???: The same node was seen twice; what should we do?
-            warnings.warn(
-                "Node <%s> has been already patched; please report!"
-                % node.__class__.__name__,
-                RuntimeWarning,
-            )
-            return
         base_children = collections.deque(base_children)
         self.children_stack.append(base_children)
         children = collections.deque()
@@ -884,6 +876,9 @@ def _With(self, node):
             children.extend(node.body)
         self._handle(node, children)
 
+    def _AsyncWith(self, node):
+        return self._With(node)
+
     def _child_nodes(self, nodes, separator):
         children = []
         for index, child in enumerate(nodes):

rope/refactor/rename.py

@@ -187,13 +187,8 @@ def get_old_name(self):
         return word_finder.get_primary_at(self.offset)
 
     def _get_scope_offset(self):
-        lines = self.pymodule.lines
-        scope = self.pymodule.get_scope().get_inner_scope_for_line(
-            lines.get_line_number(self.offset)
-        )
-        start = lines.get_line_start(scope.get_start())
-        end = lines.get_line_end(scope.get_end())
-        return start, end
+        scope = self.pymodule.get_scope().get_inner_scope_for_offset(self.offset)
+        return scope.get_region()
 
     def get_changes(self, new_name, only_calls=False, reads=True, writes=True):
         changes = ChangeSet(

ropetest/codeanalyzetest.py

@@ -502,6 +502,17 @@ def test_one_liners_with_line_breaks2(self):
         pyname = name_finder.get_pyname_at(code.rindex("var"))
         self.assertEqual(pymod["var"], pyname)
 
+    def test_var_in_list_comprehension_differs_from_var_outside(self):
+        code = "var = 1\n[var for var in range(1)]\n"
+        pymod = libutils.get_string_module(self.project, code)
+
+        name_finder = rope.base.evaluate.ScopeNameFinder(pymod)
+
+        outside_pyname = name_finder.get_pyname_at(code.index("var"))
+        inside_pyname = name_finder.get_pyname_at(code.rindex("var"))
+
+        self.assertNotEqual(outside_pyname, inside_pyname)
+
 
 class LogicalLineFinderTest(unittest.TestCase):
     def setUp(self):

ropetest/pyscopestest.py

@@ -3,6 +3,8 @@
 except ImportError:
     import unittest
 
+from textwrap import dedent
+
 from rope.base import libutils
 from rope.base.pyobjects import get_base_type
 from ropetest import testutils
@@ -328,6 +330,19 @@ def test_get_scope_for_offset_for_scope_with_indent(self):
         inner_scope = scope.get_scopes()[0]
         self.assertEqual(inner_scope, scope.get_inner_scope_for_offset(10))
 
+    @testutils.only_for("3.5")
+    def test_get_scope_for_offset_for_function_scope_and_async_with_statement(self):
+        scope = libutils.get_string_scope(
+            self.project,
+            dedent("""\
+                async def func():
+                    async with a_func() as var:
+                        print(var)
+            """)
+        )
+        inner_scope = scope.get_scopes()[0]
+        self.assertEqual(inner_scope, scope.get_inner_scope_for_offset(27))
+
     def test_getting_overwritten_scopes(self):
         scope = libutils.get_string_scope(
             self.project, "def f():\n    pass\ndef f():\n    pass\n"
@@ -414,3 +429,33 @@ def test_get_inner_scope_for_nested_list_comprhension(self):
         self.assertEqual(len(scope.get_scopes()[0].get_scopes()), 1)
         self.assertIn("j", scope.get_scopes()[0].get_scopes()[0])
         self.assertIn("i", scope.get_scopes()[0].get_scopes()[0])
+
+    def test_get_scope_region(self):
+        scope = libutils.get_string_scope(
+            self.project,
+            dedent("""
+                def func1(ala):
+                   pass
+
+                def func2(o):
+                   pass
+            """),
+        )
+
+        self.assertEqual(scope.get_region(), (0, 48))
+        self.assertEqual(scope.get_scopes()[0].get_region(), (1, 24))
+        self.assertEqual(scope.get_scopes()[1].get_region(), (26, 47))
+
+    def test_only_get_inner_scope_region(self):
+        scope = libutils.get_string_scope(
+            self.project,
+            dedent("""
+                def func1(ala):
+                   pass
+
+                def func2(o):
+                   pass
+            """),
+        )
+
+        self.assertEqual(scope.get_scopes()[1].get_region(), (26, 47))

ropetest/refactor/renametest.py

@@ -199,10 +199,7 @@ def test_renaming_generator_comprehension_loop_variables(self):
             refactored,
         )
 
-    @unittest.expectedFailure
     def test_renaming_comprehension_loop_variables_scope(self):
-        # FIXME: variable scoping for comprehensions is incorrect, we currently
-        #        don't create a scope for comprehension
         code = dedent("""\
             [b_var for b_var, c_var in d_var if b_var == c_var]
             b_var = 10
@@ -1500,6 +1497,19 @@ def xxx_test_with_statement_variables_should_not_leak(self):
         """)
         self.assertEqual(expected, mod1.read())
 
+    def test_rename_in_list_comprehension(self):
+        code = dedent("""\
+            some_var = 1
+            compr = [some_var for some_var in range(10)]
+        """)
+        offset = code.index("some_var")
+        refactored = self._local_rename(code, offset, "new_var")
+        expected = dedent("""\
+            new_var = 1
+            compr = [some_var for some_var in range(10)]
+        """)
+        self.assertEqual(refactored, expected)
+
 
 class ChangeOccurrencesTest(unittest.TestCase):
     def setUp(self):