MoveModule creates new top-level imports

[SomeoneSerge]

Describe the bug

When MoveModule encounters an import statement inside a function (thus, evaluated at some unknown point later and maybe never), and the import statement concerns the module being moved, the calculated changes include both a modification of the original import statement and an extraneous import statement at the top of the file. This changes the order of imports and, in sufficiently cursed code-bases, may even result in a circular import.

Basically, the issue is that MoveModule appears to consider imports to be side effect-free, which sometimes they aren't.

The extra import is being added here:

rope/rope/refactor/move.py

Lines 594 to 596 in 25586a6

	if should_import:
	pymodule = self.tools.new_pymodule(pymodule, source)
	source = self.tools.add_imports(pymodule, [new_import])

rope/rope/refactor/move.py

Lines 769 to 773 in 25586a6

	def _add_imports_to_module(import_tools, pymodule, new_imports):
	module_with_imports = import_tools.module_imports(pymodule)
	for new_import in new_imports:
	module_with_imports.add_import(new_import)
	return module_with_imports.get_changed_source()

To Reproduce

Steps to reproduce the behavior:

1. Code before refactoring:

# a.py
def use_b_later():
    import b

# b.py
import a

2. Describe the refactoring you want to do

Rename (move) b into prefix.b.

4. Expected code after refactoring:

# a.py
def use_b_later():
    import prefix.b

# b.py
import a

5. Describe the error or unexpected result that you are getting

# a.py
import prefix.b

def use_b_later():
    import prefix.b

# b.py
import a

Reproducible example

$ python << EOF
import shutil
import tempfile
from contextlib import contextmanager
from os import walk
from pathlib import Path

from rope.base.project import Project
from rope.refactor.move import MoveModule


@contextmanager
def temp_tree():
    path = tempfile.mkdtemp()
    try:
        yield Path(path)
    finally:
        shutil.rmtree(path, ignore_errors=True)


with temp_tree() as root:

    (root / "utils.py").write_text(
        """
def use_lazy():
    import lazy"""
    )

    (root / "lazy.py").write_text(
        """
import utils"""
    )

    (root / "prefix").mkdir()
    (root / "prefix" / "__init__.py").touch()

    project = Project(root)

    new_package = project.get_folder("prefix")
    lazy = project.get_module("lazy").get_resource()

    print(MoveModule(project, lazy).get_changes(new_package).get_description())
EOF
Moving module <lazy>:


--- a/lazy.py
+++ b/lazy.py
@@ -1,2 +1 @@
-
-import utils+import utils

--- a/utils.py
+++ b/utils.py
@@ -1,3 +1,3 @@
-
+import prefix.lazy
 def use_lazy():
-    import lazy+    import prefix.lazy
rename from lazy.py
rename to prefix/lazy.py

Editor information (please complete the following information):

• Project Python version: 3.11
• Rope Python version: 3.11
• Rope version: 1.11.0 (also checked 1.9.0)
• Text editor/IDE and version: ...

Additional context

Context: trying to use Rope for some automation in https://github.com/SomeoneSerge/pkgs/blob/master/python-packages/by-name/pr/prefix-python-modules/prefix_python_modules.py

[SomeoneSerge]

My understanding is that in this particular case, create_finder(..., imports=False) is supposed to skip import lazy but it doesn't (do I misunderstand the meaning of imports=False?).

This may be an additional issue overlapping with the original one actually: I can change use_lazy to something like import lazy; lazy.do(), and I would still expect should_import to only scan the scope where lazy was introduced (i.e. the use_lazy function), and to only add normalized imports at the top of the function.

Currently occurs_in_module finds:

(Pdb) b rope/refactor/move.py:741
...
(Pdb) occurrences = list(occurrences)
(Pdb) p [o.tools._source_code[o.get_primary_range()[0] - 7: o.get_primary_range()[1] + 5] for o in occurrences]
['import lazy\n    ', 'zy\n    lazy.do()']
(Pdb) p occurrences[0]._is_in_import_statement
False

[SomeoneSerge]

I tried extending test_is_import_statement with:

    def test_is_import_statement(self):
        code, annotations = self._annotated_code(annotated_code=dedent("""\
            import a.b.c.d
                   ++++++++
            from a.b import c

            import a.b.c.d as d
                   +++++++++++++
            from a.b import c as e

            from a.b import (

                abc

            )

            result = a.b.c.d.f()

            def import_later():

                import lazy
                       +++++
        """))

And the following output, which AFAIU confirms that NoImportsFilter doesn't acknowledge imports nested in functions:

ropetest/codeanalyzetest.py:113: in assert_equal_annotation
    self.fail("".join(msg))
E   AssertionError: Annotation does not match:
E     import a.b.c.d
E     from a.b import c
E     import a.b.c.d as d
E     from a.b import c as e
E     from a.b import (
E         abc
E     )
E     result = a.b.c.d.f()
E     def import_later():
E         import lazy
E   e            ++++
E   a

Did I get the annotation syntax right?

[SomeoneSerge]

            and self._find_word_start(line_start) == last_import

# ...

    def _find_word_start(self, offset):
        current_offset = offset
        while current_offset >= 0 and self._is_id_char(current_offset):
            current_offset -= 1
        return current_offset + 1

H'm, is it intentional that you begin at the leftmost character of the line and then try to go further left?

[lieryan]

Hi @SomeoneSerge, thanks for writing this issue and creating the PR. I'm currently travelling on vacation, so my availability to look into rope issues and review PRs will be limited until December. So don't worry, but I hadn't forgotten about this 🙂, but I won't be actioning the PR until I'm back.

[SomeoneSerge]

I think I just encountered the import inside a function issue with the Rename. Will take some time to document though.

@lieryan any pointers as to where to begin?