PR: Remove strange indirections in pynamesdef & pyobjectsdef

[edreamleo]

rope/base/oi/soi/pynamesdef.py contains the following mysterious imports:

from rope.base import pynames
from rope.base.pynames import *

This PR suggests that these imports should be:

from rope.base import pynames, utils

Failing unit tests highlight the ostensible reason for the import *, namely even more mysterious indirections in rope.base.pyobjectsdef.

The diffs are revealing. The old code (in rope.base.pyobjectsdef) refers to a class X as if it resided in rope.base.pynamesdef. In fact, class X resides in rope.base.pynames.

For each class X, cffs (global searches) reveal that there is only one class X anywhere in Rope with that name. There can be no doubt that the proposed code works exactly the same way as the legacy code.

What is going on???

I am at a loss for a reasonable explanation for the legacy code.

Could the misleading qualifications in rope.base.pyobjectsdef be a bizarre form of comment? If so, there are better solutions:

• Add a (new!) docstring in rope.base.pyobjectsdef.
• Add individual comments in rope.base.pyobjectsdef.
• Rename each class X.
• Move each class X to (I presume) rope.base.pynamesdef.

Any of these fixes would be preferable to the present code.

@lieryan Do you have any explanation for these mysteries?

P.S. I am beginning to wonder whether the original author might have deliberately used weird constructions as a kind of old-fashioned unit test. 50 years ago, when I was a graduate student in CS at U.W. Wisconsin, the state of the art in testing compilers was to compile the compiler with itself. (The round-trip should be identical) Yes, such tests are woefully inadequate, but that was a long time ago.

[lieryan]

It's nothing "mysterious". Just because the behavior is the same, doesn't mean they are semantically equivalent.

Some functions can only work because rope can find the definition of objects/names (e.g. a = ..., def x()), other functions doesn't really care about the definition, so it works with objects/names without caring whether we can infer what the actual type of the objects/names or where they are have been defined (e.g. a.foo + x(), where inferring what a and x is non-trivial, but you can infer that a has a property foo and x is a callable), and most code can work with either one.

The rope code doesn't always distinguish them in implementations, but these are three conceptually different facets of objects that shouldn't be mixed together, therefore they have separate types. It's not often always necessary that rope treats the different facets differently, but it's often very necessary to understand the intent of the code when trying to comprehend what the code is trying to do.

In a sense, it is kinda a form of lexical type hinting, with no strict runtime typing.

Removing these distinctions may not cause rope to behave any differently, but the codebase will lose information about the original intent of the author; and if we want to add standard typing-based type hints, you are going to end up having to re-discover these lost information.

Also, my understanding from reading the git history is that part of the reason the original author made the pynames/pynamesdef and pyobjects/pyobjects def distinctions is because when it was originally implemented within the same module, it was causing pathological circular import issues. Trying to merge these modules again, you're likely going to hit the same issue again at some point.

[edreamleo]

@lieryan I remain unconvinced. I am working on a proof that the supposedly important distinction is empty.

This proof will involve a detailed examination of all the X names: where they are called and how Rope uses them.

Please stay tuned.

[edreamleo]

@lieryan I was mistaken. The present diffs do not suffice for the proof.

[edreamleo]

@lieryan Hiding in plain sight in the diffs--the PR changes the qualifications of only 6 names:

DefinedName, EvaluatedName, UnboundName,
AssignmentValue, 
ImportedModule, ImportedName

Surprise! Rope already explicitly distinguishes between three kinds of names.

A full proof must examine how Rope uses these 6 names.

[edreamleo]

@lieryan I am committed to convincing you that this PR (and others) are sound and that the old code is just plain wrong.

Removing these distinctions may not cause rope to behave any differently, but the codebase will lose information about the original intent of the author;

Unlike legislators and courts, Rope devs need not worry about "original intent" :-) Python has come a long way since Rope's early days.

and if we want to add standard typing-based type hints, you are going to end up having to re-discover these lost information.

Imo, this statement is mistaken, for two reasons. First, mypy is extraordinarily easy to use. Second, there is no lost information to re-discover! This PR and (the closed, still desirable) PR #539 will surely make mypy's task easier.

BTW, after learning about postponed evaluations, I created Leo PR #3004. The PR changed 70 files for the better. The big surprise was that eliminating the else clauses allowed pyflakes to show that some annotations were never used!

I'll create a new PR that annotates at least the 6 classes whose qualifications this PR changes, namely AssignmentValue, DefinedName, EvaluatedName, ImportedModule, ImportedName, and UnboundName,

QQQ
Also, my understanding from reading the git history is that part of the reason the original author made the pynames/pynamesdef and pyobjects/pyobjects def distinctions is because when it was originally implemented within the same module, it was causing pathological circular import issues. Trying to merge these modules again, you're likely going to hit the same issue again at some point.
QQQ

I have a lot of experience with avoiding circular references. I'll create a new PR demonstrating that merging pynames/pynamesdef and pyobjects/pyobjectsdef can be done without fuss.

Summary

Too much respect for original intent and existing code is tying Rope's code into knots. It's time to move on.

I'll create two new PRs that will show:

• that pynames/pynamesdef and pyobjects/pyobjectsdef modules can be merged.
• that mypy can annotate the 6 classes whose qualifications this PR changes.

[edreamleo]

I am working on a proof that the supposedly important distinction is empty. This proof will involve a detailed examination of all the X names.

I doubt that any "proof" will convince you to eliminate empty distinctions. Still, I have enjoyed my investigations--they have deepened my understanding of Rope.

Only the following files reference the 6 X classes: builtins.py, codeassist.py, module_imports.py, move.py, occurances.py, pickle.py, pynames.py, pynamesdef.py, pyscopes.py, rename.py, and soi.py.

There is no need to discuss pynames.py and pynamesdef.py here. These files merely define the X classes. In any case, we need a new PR to show that merging these files is feasible.

Some of these files have nothing directly to do with soi:

pickle.py is a utility.

codeassist.py, move.py, and rename.py are clients of soi: they use soi but have no effect on soi.

The following files are involved with soi:

builtins.py references only UnboundName in Property.__init__.

pyscopes.py references only AssignedName and EvaluatedName, and only in
FunctionScope.invalidate_data.

evaluate.py: The StatementEvaluator and ScopeNameFinder classes contain many unexceptional references to the X classes.

soi.py contains only the following straightforward references to X symbols:

• AssignmentValue in SOAVisitor._evaluate_assign_value.
• UnboundName in _follow_pyname, SOAVisitor._Call & helpers, and SOAVisitor._evaluate_assign_value.

Summary

This analysis does not prove that this PR is sound. Happily, a clean bill of health from pyflakes and pytest should suffice!

I have dug deep to convince myself that eliminating the empty distinctions between pynames/pynamesdef and pyobjects/pyobjectsdef would have no effect on Rope.

I don't expect this analysis to be completely persuasive. Perhaps yet another PR will (at last!!) put this debate to rest.

[edreamleo]

@lieryan I am going to close this issue in favor of PR #614.