PR for #570: disambiguate the name 'ast'

[edreamleo]

See #570 for the notable changes.

Other changes

• _search_in_f_string In rope/refactor/occurrences.py must use the parse and walk functions from stdlib.ast, not the parse and walk functions from astutils. The old code obscured this fact!
• Eliminate a pyflakes error in _analyze_node in soa.py.
• Remove several unused imports (reported by pyflakes).

[edreamleo]

@lieryan I now see how to explain the merits of this PR.

Rope's old code violates two design principles:

1: Modules, not names, create encapsulation.
2: Never confuse module names.

PR #572: fixed a violation of Principle 2.

This PR fixes violations of both principles.

Problems with Rope's legacy code

The present code misuses the name ast, perhaps in a mistaken attempt at creating encapsulation. ast means different things in different contexts. Misusing the name ast:

• confuses all readers, newbies and experts alike.
• makes later refactoring harder, not easier.

Future refactorings

Let's consider how this PR will simplify future refactorings.

Case 1: Enhance ast.parse.

PR #577 is a stopgap. We desire a version of ast.parse that can handle all python versions that Rope supports.

This PR encapsulates/wraps astutils.parse. If parse ever changes, none of Rope's code will change (except for astutils.py itself). For example, we might create a separate module containing only the new parse function. Only astutils will know of that module.

Case 2: Replace ast nodes with wrapped nodes.

In the (unlikely) event that Rope will ever want to use some wrapped version of ast.AST, this PR's code will simplify that change. Just replace ast with (say) ast2. This PR makes that replacement safe. There will be no false matches with functions in astutils.

Summary

Rope's legacy code violates two design principles. This PR fixes both.

Misusing the ast name is an unforced error. Let's fix it now.

[edreamleo]

@lieryan I awoke just now, in the middle of the night, understanding what you have been trying to do, namely have rope.base.ast be a wrapper now for stdlib.ast.

Fair enough, but the name clash between rope.base.ast and stdlib.ast is unacceptably bad style.

I am willing to create another PR with the following changes:

• Rename rope.base.ast to rope.base.ast2 (or any other name of your choosing).
• Change ast to ast2 everywhere, except in _search_in_f_string.
• In ast2, replace from ast import * with an explicit list of ast.AST nodes.
  The ast2 module should avoid redefining the parse and walk functions implicitly imported from stdlib.ast.
  Removing the wildcard import will help pylint, pyflakes, flake8 and mypy.

What do you think?

[edreamleo]

@lieryan When I awoke the second time this morning I saw there is an even simpler way:

• Rename rope.base.ast to rope.base.astwrapper.
• Access parse and walk everywhere with from rope.base.astwrapper import parse, walk
  (except in _search_in_f_string as before).
• Leave all other references to ast alone.
• In astwrapper, remove from ast import *

Advantages

• Removes the unacceptable confusion between stdlib.ast and rope.base.ast.
• Simplifies all references to parse and walk.
• Minimizes the diffs in the new PR.
• Creates a distinction between rope.base.astwrapper and rope.base.utils.astutils.
  The former contains functions that do appear in stdlib.ast.
  The latter contains functions that do not appear in stdlib.ast.
  The docstrings for rope.base.astwrapper and rope.base.utils.astutils should mention this distinction.
• Compatible with pyflakes. Helps pylint, flake8, and mypy.
• The new PR would make it easier (later) to wrap ast.AST nodes themselves:
  • Define wrappers for ast.AST nodes in rope.base.astwrapper or a separate helper module.
  • Rename astwrapper to ast2.
  • Change ast to ast2 (almost) everywhere.

@lieryan Your comments, please.

[edreamleo]

@lieryan There is another confusion that should be addressed.

At present, rope.base.astutils contains a single function, get_name_levels. This function does not appear in stdlib.ast.

I recommend moving three functions now in rope.base.ast that do not appear in stdlib.ast to rope.base.astutils. That way, the distinction mentioned in the previous comment is established:

• rope.base.astwrapper will contain functions that do appear in stdlib.ast.
• rope.base.astutils will contain functions that do not appear in stdlib.ast.

[edreamleo]

@lieryan Perhaps you wonder how I can confidently make the suggestions I am making.

Leo makes global reasoning straightforward:

• Leo's cff (global search) command takes away most of the risks involved in search/replace operations.
  Details omitted. It's a Leo thing.
• It's easy to move functions from one module to another by moving the nodes in the Leo outline.
• pyflakes checks for undefined or unused names whenever I save a file.
• Unit tests ensure that no unusual constraints have been violated.

It's that easy.

Update: And it's way easy to move from file to file. No need to open files in a text editor.

[lieryan]

You should wake up the third time again to come up with something even more simplerer 😜

I don't agree that the "name clash", so to speak is a major issue though, or an issue at all. As people wiser than me said "Namespaces are one honking good idea", the names live in different namespaces, so there isn't really a name clash per se, the names aren't clashing.

If you really feel strongly about the issue, I don't mind changing it, but I disagree that renaming it to astutils/astwrappers/etc is an improvement. That rope.base.ast have a name that resembles standard library ast is intentional, it's to highlight the similarity between the two modules, that rope.base.ast is a drop in replacement for stdlib ast. It's not really important for rope.base.ast to only provide features that are available in stdlib ast, we are extending the stdlib module, so by Liskov Substitution Principle, adding new functionality is allowed.

There are really only a few things that really bothers me with the current ast structure:

• rope's walk and parse has a signature that isn't compatible with ast.walk, it should be renamed to something like walk_visitor(). Alternatively, it should be changed to call stdlib walk internally, but I think there's many problems with that
• occurrences.py should be re-written to not use stdlib ast directly, but import it through rope's ast just like everything else.

---

As for the other issue, I don't think removing from ast import * will be an improvement over the current code. Yes it freaks out pyflakes, but the suggested alternatives creates much worse problems. It creates an unacceptable maintenance overhead to have to manually update the import list to keep up with the standard library, especially since you will have to make it work with five different major python versions at the same time, because almost every major version of Python has added/removed new node types and deprecated old ones. The stdlib ast module has changed in backwards incompatible ways and will continue to do so in the future; it's just going to be busy work to try to keep the import list up to date. If you change it to explicit import, one wrong import in this file will just break rope entirely, which is undesirable.

I suggest just turning off flakes for this file. And I don't know if it'll work, but maybe we can define __all__ to hint to tools on what names are actually available, without the risk of breaking rope or the complexity of wrapping everything with try: except ImportError.

[lieryan]

I think what we can do with the import of ast, is to always import this module with an alias: import rope.base.ast as rast.

That way, even without looking at the imports, the code that uses rope ast will always unambiguously look different than code that uses stdlib ast.

Code that uses rope's ast will look like: isinstance(node, rast.Constant) vs stdlib's ast isinstance(node, ast.Constant)

[edreamleo]

@lieryan I've just proposed PR #580, which I created before reading your latest comments here.

Let me think sleep on on your latest comments before saying more.