PR: move special case from walk to _ScopeVisitor._ImportFrom

[edreamleo]

An experimental fix for #537. This PR should not be merged without further discussion, study and testing.

I speculate that the special case may have been added to the walk function for "generality/safety", but clearly the special case logically belongs elsewhere.

• All unit tests pass.

[lieryan]

I'm not quite sure why this special case of overwriting the ast was added there, but from the comment, this seems to imply that it's for Python 2.

If that is the reason, then we should just remove the Python 2 version of the code entirely, we no longer support Python 2. We can remove the comment as well if it's no longer relevant.

[edreamleo]

@lieryan Hmm. A unit test will fail if the special case is completely removed. It's still a big mystery.

I agree that the comments re the test aren't very useful.

[lieryan]

IIUC, this is because the special case is normalising the ast so that the it looks more like how Python 2 parses things rather than how Python 3 does things. That's why removing it causes a test to fail, because the current code is expecting the ast to behave like Python 2, namely that .module is an empty string rather than None.

The ideal fix is to remove that special case, and instead fix the code so it expects .module to be set to None instead for these cases.

[lieryan]

Assuming there's no other use of node.module, then changing this line:

imported_module = pynames.ImportedModule(self.get_module(), node.module, level)

To this:

 imported_module = pynames.ImportedModule(self.get_module(), node.module if node.module is not None else "", level)

Should fix it.

Alternatively, node.module or ""

[lieryan]

Hi @edreamleo, thanks for looking into this. I believe this PR and the additional commit 3eba333 should solve the issue, but do let me know if you think there's more that meets the eye.

[edreamleo]

@lieryan Many thanks, Lie, for this work. It looks better than the code I was imagining. In particular:

• Replacing else: with elif self.module_name is not None: in pynames.py.
• Replacing the argument node.module or "" with node.module in pyobjectsdef.py.

Both these changes help remove the distinction between None and "" in Rope's code, which imo is a big deal.

[edreamleo]

@lieryan I'm having trouble syncing my master master (in my fork) with the upstream master. Everything appears up-to-date. I followed these instructions.

Are you sure your master contains the new code?

[edreamleo]

@lieryan In my master branch (not yet containing this PR), there are three instances of the string or "". This PR removes two of them. _PatchingASTWalker._ImportFrom contains the third:

children.extend([node.module or "", "import"])

I wonder what would happen (after merging this PR) if this code became:

children.extend([node.module, "import"])

[lieryan]

The children in _PatchingASTWalker walkers contains either a list of (string) tokens or ast nodes. Essentially, it maps out the ast to the corresponding original source code to be used during code generation, effectively making it a bit like a concrete syntax tree.

I guess if you really want to avoid adding an empty string token there, you can change the code to append the token:

    children = ["from"]
    if node.level:
        children.append("." * node.level)
    if node.module is not None:
        children.append(node.module)
    children.append("import")
    children.extend(self._child_nodes(node.names, ","))

Looking at the code further though, I think this code has another bug to begin with. You can add spaces between the dots, this is valid Python code:

from xml . etree import cElementTree
from . . . pkg . mod import something

But because ast normalises the module name when parsing, node.module == "xml.etree" the current sorted_children would require there being no whitespaces between the module names and dots; so rope currently would error out on above code.

Edit: created ticket #559, fixed in PR #560

[edreamleo]

@lieryan Thanks for these comments. I've noted them and will investigate further as soon as I begin studying Rope's inference machinery. It will be several days at least before that happens.