'move' refactoring doesn't retain 'from' imports

[bwendling]

If you run the program below from the top-level rope directory, you'll see that the 'from' import statements aren't retained. For example, this is what I get as part of the output:

--- a/build/lib.linux-x86_64-2.7/rope/contrib/codeassist.py
+++ b/build/lib.linux-x86_64-2.7/rope/contrib/codeassist.py
@@ -10,11 +10,11 @@
 from rope.base import pynames
 from rope.base import pynamesdef
 from rope.base import pyobjects
-from rope.base import pyobjectsdef
 from rope.base import pyscopes
 from rope.base import worder
 from rope.contrib import fixsyntax
 from rope.refactor import functionutils
+import rope.base.tmp.pyobjectsdef

I expect the new 'import' statement to be something like:

from rope.base.tmp import pyobjectsdef

Is this intentional?

#!/usr/bin/python

import os
import sys
from rope.base import fscommands
from rope.base import libutils
from rope.base import project
from rope.contrib import generate
from rope.refactor import move

ROOT = os.getcwd()

src_name = sys.argv[1]
dst_dir = sys.argv[2]
p = project.Project(ROOT, fscommands.create_fscommands(ROOT))
dst = libutils.path_to_resource(p, dst_dir)
if not dst.exists():
  dst = generate.create_package(p, PackageFromFolder(dst_dir))
print '*' * 10, src_name, '*' * 10
src = libutils.path_to_resource(p, src_name)
mover = move.create_move(p, src)
changes = mover.get_changes(dst)
print('changes:\n{}'.format(changes.get_description()))

$ python ./myrope.py rope/base/pyobjectsdef.py rope/base/tmp

[aligrudi]

Bill Wendling [email protected] wrote:

If you run the program below from the top-level rope directory, you'll see that the 'from' import statements aren't retained. For example, this is what I get as part of the output:

--- a/build/lib.linux-x86_64-2.7/rope/contrib/codeassist.py
+++ b/build/lib.linux-x86_64-2.7/rope/contrib/codeassist.py
@@ -10,11 +10,11 @@
 from rope.base import pynames
 from rope.base import pynamesdef
 from rope.base import pyobjects
-from rope.base import pyobjectsdef
 from rope.base import pyscopes
 from rope.base import worder
 from rope.contrib import fixsyntax
 from rope.refactor import functionutils
+import rope.base.tmp.pyobjectsdef

I expect the new 'import' statement to be something like:

from rope.base.tmp import pyobjectsdef

Is this intentional?

From-imports are probably replaced with absolute imports in MoveModule._change_moving_module() of rope/refactor/move.py: To make sure you can remove the following two lines from _change_moving_module() and see what happens.

        source = self.import_tools.relatives_to_absolutes(pymodule)
        pymodule = self.tools.new_pymodule(pymodule, source)

If this is the real cause, there should be a problem in ImportTools.relatives_to_absolutes(). The call seems necessary, since the relative imports of the moving module should be updated. Is RelativeToAbsoluteVisitor.visitFromImport() in rope/refactor/importutils/actions.py working correctly?

#!/usr/bin/python

import os
import sys
from rope.base import fscommands
from rope.base import libutils
from rope.base import project
from rope.contrib import generate
from rope.refactor import move

ROOT = os.getcwd()

src_name = sys.argv[1]
dst_dir = sys.argv[2]
p = project.Project(ROOT, fscommands.create_fscommands(ROOT))
dst = libutils.path_to_resource(p, dst_dir)
if not dst.exists():
  dst = generate.create_package(p, PackageFromFolder(dst_dir))
print '*' * 10, src_name, '*' * 10
src = libutils.path_to_resource(p, src_name)
mover = move.create_move(p, src)
changes = mover.get_changes(dst)
print('changes:\n{}'.format(changes.get_description()))

Thanks for the concise test case.

Ali

[bwendling]

On Thu, Sep 11, 2014 at 3:07 AM, Ali Gholami Rudi [email protected] wrote:

From-imports are probably replaced with absolute imports in MoveModule._change_moving_module() of rope/refactor/move.py: To make sure you can remove the following two lines from _change_moving_module() and see what happens.

source = self.import_tools.relatives_to_absolutes(pymodule) pymodule = self.tools.new_pymodule(pymodule, source)

If this is the real cause, there should be a problem in ImportTools.relatives_to_absolutes(). The call seems necessary, since the relative imports of the moving module should be updated. Is RelativeToAbsoluteVisitor.visitFromImport() in rope/refactor/importutils/actions.py working correctly?

Unfortunately, that didn't work. I suspect that part of the problem is that it's always building a "NormalImport" to replace things via the MoveModule._new_import method(). I'll check out the "visitFromImport()" next.

I tried a hack where it would return a "FromImport" when needed. It didn't work in all situations (some of the tests failed), but did give better output for the testcase I wrote.

-bw

[aligrudi]

Bill Wendling [email protected] wrote:

From-imports are probably replaced with absolute imports in MoveModule._change_moving_module() of rope/refactor/move.py: To make sure you can remove the following two lines from _change_moving_module() and see what happens.

source = self.import_tools.relatives_to_absolutes(pymodule) pymodule = self.tools.new_pymodule(pymodule, source)

If this is the real cause, there should be a problem in ImportTools.relatives_to_absolutes(). The call seems necessary, since the relative imports of the moving module should be updated. Is RelativeToAbsoluteVisitor.visitFromImport() in rope/refactor/importutils/actions.py working correctly?

Unfortunately, that didn't work. I suspect that part of the problem is that it's always building a "NormalImport" to replace things via the MoveModule._new_import method(). I'll check out the "visitFromImport()" next.

Indeed. Thinking more about that, I think it was intentional: in _change_occurrences_in_module() first all occurrences of the module name are replaced with its complete name when updating the sources; when moving pkg1.mod to pkg2.mod, every reference to mod (either mod after "from pkg1 import mod" or pkg1.mod after "import pkg1.mod") are replaced with pkg2.mod (the line self.tools._rename_in_module(...) in _change_occurrences_in_module()). Then, the old imports are removed and "import pkg2.mod" is added if necessary.

There are ways of preserving from-imports but it may not be straightforward. Of course the easiest solution (probably similar to the one you implemented) is to use from-imports, regardless of whether the original module used it or not (this would not work if the name "mod" in my example is used already):

• _new_modname() should return mod instead of pkg2.mod
• _new_import() should return "from pkg2 import mod"

To handle "import pkg1.mod", it should be possible to detect the existence of "from pkg1 import mod" and, depending on that, either add normal imports (as we already do) or add a new import "from pkg2 import mod". That would make move.py more complex; there may be many special cases to handle.

Ali

[bwendling]

On Thu, Sep 11, 2014 at 10:38 PM, Ali Gholami Rudi <[email protected]

wrote:

Bill Wendling [email protected] wrote:

Unfortunately, that didn't work. I suspect that part of the problem is that it's always building a "NormalImport" to replace things via the MoveModule._new_import method(). I'll check out the "visitFromImport()" next.

Indeed. Thinking more about that, I think it was intentional: in _change_occurrences_in_module() first all occurrences of the module name are replaced with its complete name when updating the sources; when moving pkg1.mod to pkg2.mod, every reference to mod (either mod after "from pkg1 import mod" or pkg1.mod after "import pkg1.mod") are replaced with pkg2.mod (the line self.tools._rename_in_module(...) in _change_occurrences_in_module()). Then, the old imports are removed and "import pkg2.mod" is added if necessary.

There are ways of preserving from-imports but it may not be straightforward. Of course the easiest solution (probably similar to the one you implemented) is to use from-imports, regardless of whether the original module used it or not (this would not work if the name "mod" in my example is used already):

• _new_modname() should return mod instead of pkg2.mod
• _new_import() should return "from pkg2 import mod"

To handle "import pkg1.mod", it should be possible to detect the existence of "from pkg1 import mod" and, depending on that, either add normal imports (as we already do) or add a new import "from pkg2 import mod". That would make move.py more complex; there may be many special cases to handle.

I understand. Let me describe the hack I did that "almost worked". :-) In MoveModule._change_occurences_in_module(), I check to see if the occurrence of the import statement is a 'from' import or 'normal' import. If it's a from import, I build a FromImport but don't call self.tools.rename_in_module(). Instead, I just use the module's source code as is. In MoveModule._change_moving_module, I removed the 'self.import_tools.relatives_to_absolutes()' and 'self.tools.new_pymodule()' calls.

Like I said, this caused two testcases to fail. (I think that they were edge cases or instances of removing now-superfluous import statements.) But it did seem to work at least for my needs. :-)

What are your thoughts?

-bw

P.S. I've attached the actual diff for your perusal.

diff --git a/rope/refactor/move.py b/rope/refactor/move.py
index 1c9a03b..1636619 100644
--- a/rope/refactor/move.py
+++ b/rope/refactor/move.py
@@ -423,14 +423,20 @@ class MoveModule(object):
             return destname + '.' + self.old_name
         return self.old_name
-    def _new_import(self, dest):
-    def _new_normal_import(self, dest):
       return importutils.NormalImport([(self._new_modname(dest), None)])
-    def _new_from_import(self, dest):
-        dest_path = self._new_modname(dest).split('.')
-        module_name = '.'.join(dest_path[:-1])
-        name = dest_path[-1]
-        return importutils.FromImport(module_name, 0, [(name, None)])
  +
   def _change_moving_module(self, changes, dest):
       if not self.source.is_folder():
           pymodule = self.project.get_pymodule(self.source)
-            source = self.import_tools.relatives_to_absolutes(pymodule)
-            pymodule = self.tools.new_pymodule(pymodule, source)
-            #source = self.import_tools.relatives_to_absolutes(pymodule)
-            #pymodule = self.tools.new_pymodule(pymodule, source)
           source = self._change_occurrences_in_module(dest, pymodule)
           source = self.tools.new_source(pymodule, source)
           if source != self.source.read():
  @@ -444,9 +450,20 @@ class MoveModule(object):
       if pymodule is None:
           pymodule = self.project.get_pymodule(resource)
       new_name = self._new_modname(dest)
-        new_import = self._new_import(dest)
-        source = self.tools.rename_in_module(
-            new_name, imports=True, pymodule=pymodule, resource=resource)
-        dest_name = new_name.split('.')
-        if (self.tools.occurs_in_from_statement(pymodule=pymodule,
-                                                resource=resource,
-                                                imports=True) and
-                len(dest_name) > 1):
-            print('new_name: {}'.format(new_name))
-            print('dest_name: {}'.format(dest_name))
-            new_import = self._new_from_import(dest)
-            source = resource.read() if resource else pymodule.source_code
-        else:
-            new_import = self._new_normal_import(dest)
-            source = self.tools.rename_in_module(
- 
             new_name, imports=True, pymodule=pymodule, resource=resource)
  
  
  +
       should_import = self.tools.occurs_in_module(
           pymodule=pymodule, resource=resource, imports=False)
       pymodule = self.tools.new_pymodule(pymodule, source)
  @@ -517,11 +534,22 @@ class _MoveTools(object):
       return source
  
   def occurs_in_module(self, pymodule=None, resource=None, imports=True):
-        for occurrence in self._find_occurrences(pymodule, resource, imports):
-            return True
-        return False
  +
-    def occurs_in_from_statement(self, pymodule=None, resource=None,
-                                 imports=True):
-        for occurrence in self._find_occurrences(pymodule, resource, imports):
-            if occurrence.is_in_from_statement():
-                return True
-        return False
  +
-    def _find_occurrences(self, pymodule=None, resource=None, imports=True):
       finder = self._create_finder(imports)
       for occurrence in finder.find_occurrences(pymodule=pymodule,
                                                 resource=resource):
-            return True
-        return False
- 
         yield occurrence
  
  
   def _create_finder(self, imports):
       return occurrences.create_finder(self.project, self.old_name,
  diff --git a/rope/refactor/occurrences.py b/rope/refactor/occurrences.py
  index 14a2d7d..9a116f5 100644
  --- a/rope/refactor/occurrences.py
  +++ b/rope/refactor/occurrences.py
  @@ -142,8 +142,15 @@ class Occurrence(object):
  
   @utils.saveit
   def is_in_import_statement(self):
-        return (self.tools.word_finder.is_from_statement(self.offset) or
-                self.tools.word_finder.is_import_statement(self.offset))
-        return self.is_in_from_statement() or self.is_in_import_statement()
  +
-    @utils.saveit
-    def is_in_from_statement(self):
-        return self.tools.word_finder.is_from_statement(self.offset)
  +
-    @utils.saveit
-    def is_in_import_statement(self):
- 
     return self.tools.word_finder.is_import_statement(self.offset)
  
  
   def is_called(self):
       return self.tools.word_finder.is_a_function_being_called(self.offset)

[bwendling]

On Sun Sep 14 2014 at 1:52:06 PM Bill Wendling [email protected] wrote:

P.S. I've attached the actual diff for your perusal.

Just to clarify, the diff I attached is not being submitted as something I think should go in (at least not without a lot of fixing). It's definitely in the "hack" category, and not ready for prime-time. :-)

-bw

[aligrudi]

Bill Wendling [email protected] wrote:

There are ways of preserving from-imports but it may not be straightforward. Of course the easiest solution (probably similar to the one you implemented) is to use from-imports, regardless of whether the original module used it or not (this would not work if the name "mod" in my example is used already):

• _new_modname() should return mod instead of pkg2.mod
• _new_import() should return "from pkg2 import mod"

To handle "import pkg1.mod", it should be possible to detect the existence of "from pkg1 import mod" and, depending on that, either add normal imports (as we already do) or add a new import "from pkg2 import mod". That would make move.py more complex; there may be many special cases to handle.

I understand. Let me describe the hack I did that "almost worked". :-) In MoveModule._change_occurences_in_module(), I check to see if the occurrence of the import statement is a 'from' import or 'normal' import. If it's a from import, I build a FromImport but don't call self.tools.rename_in_module(). Instead, I just use the module's source code as is. In MoveModule._change_moving_module, I removed the 'self.import_tools.relatives_to_absolutes()' and 'self.tools.new_pymodule()' calls.

Was this (removing relatives_to_absolutes()) necessary? For relative imports in the moving module, this call seems necessary to me.

Like I said, this caused two testcases to fail. (I think that they were edge cases or instances of removing now-superfluous import statements.) But it did seem to work at least for my needs. :-)

What are your thoughts?

Great job and I am impressed! I have not checked the details and the boundary conditions though, like if there are both types of accesses, then you probably need to first rename all occurrences with self.tools.rename_in_module(module_name, ...) to make them use the imported module name; i.e. renaming pkg1.mod to mod, in my example. Or even using the same trick to always modify the accesses to the moving module to use from-imports and then ignoring the pkg1.mod case altogether (but I do not know, maybe this is not a good idea).

Ali

[bwendling]

On Mon, Sep 15, 2014 at 9:14 AM, Ali Gholami Rudi [email protected] wrote:

Bill Wendling [email protected] wrote:

There are ways of preserving from-imports but it may not be straightforward. Of course the easiest solution (probably similar to the one you implemented) is to use from-imports, regardless of whether the original module used it or not (this would not work if the name "mod" in my example is used already):

• _new_modname() should return mod instead of pkg2.mod
• _new_import() should return "from pkg2 import mod"

To handle "import pkg1.mod", it should be possible to detect the existence of "from pkg1 import mod" and, depending on that, either add normal imports (as we already do) or add a new import "from pkg2 import mod". That would make move.py more complex; there may be many special cases to handle.

I understand. Let me describe the hack I did that "almost worked". :-) In MoveModule._change_occurences_in_module(), I check to see if the occurrence of the import statement is a 'from' import or 'normal' import. If it's a from import, I build a FromImport but don't call self.tools.rename_in_module(). Instead, I just use the module's source code as is. In MoveModule._change_moving_module, I removed the 'self.import_tools.relatives_to_absolutes()' and 'self.tools.new_pymodule()' calls.

Was this (removing relatives_to_absolutes()) necessary? For relative imports in the moving module, this call seems necessary to me.

Like I said, this caused two testcases to fail. (I think that they were edge cases or instances of removing now-superfluous import statements.) But it did seem to work at least for my needs. :-)

What are your thoughts?

Great job and I am impressed! I have not checked the details and the boundary conditions though, like if there are both types of accesses, then you probably need to first rename all occurrences with self.tools.rename_in_module(module_name, ...) to make them use the imported module name; i.e. renaming pkg1.mod to mod, in my example. Or even using the same trick to always modify the accesses to the moving module to use from-imports and then ignoring the pkg1.mod case altogether (but I do not know, maybe this is not a good idea).

Hi Ali,

Thanks! :-) Here is a cleaned up patch. It suffers from the problem you pointed out. It won't remove duplicate 'import' statements. I was wondering if you had any other ideas on how to achieve that? It might save me some hacking about. :-)

-bw

diff --git a/rope/base/worder.py b/rope/base/worder.py
index c85c6b3..35bc3ca 100644
--- a/rope/base/worder.py
+++ b/rope/base/worder.py
@@ -74,6 +74,9 @@ class Worder(object):
     def is_from_statement(self, offset):
         return self.code_finder.is_from_statement(offset)
-    def is_from_star_statement(self, offset):
-        return self.code_finder.is_from_star_statement(offset)
  +
   def get_from_aliased(self, offset):
       return self.code_finder.get_from_aliased(offset)

@@ -329,6 +332,18 @@ class _RealFinder(object):
         from_names = self._find_first_non_space_char(from_names)
         return self._find_import_end(from_names) >= offset
-    def is_from_star_statement(self, offset):
-        try:
-            last_from = self.code.rindex('from ', 0, offset)
-            from_import = self.code.index(' import ', last_from)
-            from_names = from_import + 8
-        except ValueError:
-            return False
-        from_names = self._find_first_non_space_char(from_names)
-        if self._find_import_end(from_names) < offset:
-            return False
- 
     return self.code[from_names] == '*'
  
  
  +
   def is_from_statement_module(self, offset):
       if offset >= len(self.code) - 1:
           return False
  diff --git a/rope/refactor/move.py b/rope/refactor/move.py
  index 1c9a03b..f5a00e1 100644
  --- a/rope/refactor/move.py
  +++ b/rope/refactor/move.py
  @@ -423,9 +423,14 @@ class MoveModule(object):
           return destname + '.' + self.old_name
       return self.old_name
-    def _new_import(self, dest):
-    def _new_normal_import(self, dest):
       return importutils.NormalImport([(self._new_modname(dest), None)])
-    def _new_from_import(self, dest_name):
-        module_name = '.'.join(dest_name[:-1])
-        name = dest_name[-1]
-        return importutils.FromImport(module_name, 0, [(name, None)])
  +
   def _change_moving_module(self, changes, dest):
       if not self.source.is_folder():
           pymodule = self.project.get_pymodule(self.source)
  @@ -444,11 +449,23 @@ class MoveModule(object):
       if pymodule is None:
           pymodule = self.project.get_pymodule(resource)
       new_name = self._new_modname(dest)
-        new_import = self._new_import(dest)
-        source = self.tools.rename_in_module(
-            new_name, imports=True, pymodule=pymodule, resource=resource)
-        should_import = self.tools.occurs_in_module(
-            pymodule=pymodule, resource=resource, imports=False)
-        dest_name = new_name.split('.')
-        (in_from_stmt, is_star) = self.tools.occurs_in_from_statement(
-            pymodule=pymodule, resource=resource, imports=True)
  +
-        if in_from_stmt and len(dest_name) > 1:
-            if is_star:
-              dest_name.append('*')
-            new_import = self._new_from_import(dest_name)
-            source = resource.read() if resource else pymodule.source_code
-            should_import = True
-        else:
-            new_import = self._new_normal_import(dest)
-            source = self.tools.rename_in_module(
-                new_name, imports=True, pymodule=pymodule, resource=resource)
-            should_import = self.tools.occurs_in_module(
- 
             pymodule=pymodule, resource=resource, imports=False)
  
  
  +
       pymodule = self.tools.new_pymodule(pymodule, source)
       source = self.tools.remove_old_imports(pymodule)
       if should_import:
  @@ -517,11 +534,29 @@ class _MoveTools(object):
       return source
  
   def occurs_in_module(self, pymodule=None, resource=None, imports=True):
-        for occurrence in self._find_occurrences(pymodule, resource, imports):
-            return True
-        return False
  +
-    def occurs_in_from_statement(self, pymodule=None, resource=None,
-                                 imports=True):
-        for occurrence in self._find_occurrences(pymodule, resource, imports):
-            if occurrence.is_in_from_statement():
-                return (True, occurrence.is_from_star_statement())
-        return (False, False)
  +
-    def occurs_in_normal_import_statement(self, pymodule=None, resource=None,
-                                          imports=True):
-        for occurrence in self._find_occurrences(pymodule, resource, imports):
-            if occurrence.is_in_normal_import_statement():
-                return True
-        return False
  +
-    def _find_occurrences(self, pymodule=None, resource=None, imports=True):
       finder = self._create_finder(imports)
       for occurrence in finder.find_occurrences(pymodule=pymodule,
                                                 resource=resource):
-            return True
-        return False
- 
         yield occurrence
  
  
   def _create_finder(self, imports):
       return occurrences.create_finder(self.project, self.old_name,
  diff --git a/rope/refactor/occurrences.py b/rope/refactor/occurrences.py
  index 14a2d7d..380255c 100644
  --- a/rope/refactor/occurrences.py
  +++ b/rope/refactor/occurrences.py
  @@ -142,8 +142,20 @@ class Occurrence(object):
  
   @utils.saveit
   def is_in_import_statement(self):
-        return (self.tools.word_finder.is_from_statement(self.offset) or
-                self.tools.word_finder.is_import_statement(self.offset))
-        return self.is_in_from_statement() or \
-            self.is_in_normal_import_statement()
  +
-    @utils.saveit
-    def is_in_from_statement(self):
-        return self.tools.word_finder.is_from_statement(self.offset)
  +
-    @utils.saveit
-    def is_in_normal_import_statement(self):
-        return self.tools.word_finder.is_import_statement(self.offset)
  +
-    @utils.saveit
-    def is_from_star_statement(self):
- 
     return self.tools.word_finder.is_from_star_statement(self.offset)
  
  
   def is_called(self):
       return self.tools.word_finder.is_a_function_being_called(self.offset)

[MJuddBooth]

Coming across this now as I have the same issue. I'm willing to put some time into this. @gwelymernans, could you repost the patch - github seems to be trying to parse it as markdown and I can't figure out how to undo it - as it is, the patch is unusable. Or, maybe someone can tell me how to view as raw...

[mcepl]

https://gist.github.com/mcepl/34cf6dfc99598573ea36b1c901c325b9 but I think it is still not correct: those '+' characters are somewhat weird and it is all about removal of code, nothing added. I hope it helps at least somehow.

[MJuddBooth]

Thanks, that should help. I think the "removing" is just the order of the diff.

[soupytwist]

@MJuddBooth, can you please clarify your issue? In fact, I would prefer for you to create a new bug with steps to reproduce it if possible. This patch is very old and doesn't make sense to me (maybe the removals / additions are backwards).

Can you take a look at #214? Are these the same issue? If so, can we move the discussion to there?

[MJuddBooth]

Mine seems to be pretty close to if not exactly the same as @gwelymernans. The current code base still shows the same behavior, namely

from a.b.c import x becomes import a.b.d

if x is moved to a.b.d and

x(foo) becomes a.b.d.x(foo).

I can try to flesh out the original test case. This is similar to #214, but for that issue there is a pref setting that looks like it may fix it.