Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR: Fix/suppress all mypy complaints (based on master) #674

Merged
merged 8 commits into from Mar 2, 2023
Merged

PR: Fix/suppress all mypy complaints (based on master) #674

merged 8 commits into from Mar 2, 2023

Conversation

edreamleo
Copy link
Contributor

@edreamleo edreamleo commented Feb 25, 2023

Summary

This PR contains the minimum changes (to the master branch) needed to fix and suppress all mypy complaints without removing the wildcard import in rope.base.ast.

This PR:

  • Shows that mypy is not adversely affected by the wildcard import in rope.base.ast!!
  • Suppresses all mypy complaints, mostly with # type: ignore comments.
    Comments describe all interesting/unusual suppressions.
  • Adds two missing base-class methods whose absence rightly generated mypy errors.

Details

  • Add # type: ignore as needed to suppress warnings about optional imports.
  • rope.base.ast: mypy rightly complains about an import, but this import does not mypy significantly!
  • rope.base.oi.type_hinting.evaluate: Suppress complaints about functions decorated with @multi.
  • rope.base.oi.type_hinting.utils: Replace a type comment with a proper annotation.
    The new annotation uses a type alias, PyObj, showing that annotations need not be visually daunting.
  • rope.base.prefs: Suppress all mypy errors by adding # type: ignore as the first line of the file.
  • rope.base.project: Add # type: ignore to compensate for suppressing mypy in rope.base.prefs.
  • Add two missing methods: pyobjects.PyObject.get_module and pyobjects.PyDefinedObject.get_name.
    Imo:
    • mypy rightly complained about these methods.
    • Adding these two missing method will help simplify the Abstract classes later on.
  • Add (straightforward) annotations of List and Dict types as needed.
  • MoveGlobal.get_changes: Revise existing annotations and suppress complaints.
    Comments suggest actual code revisions, but such changes should wait for a separate PR.

@edreamleo edreamleo changed the title PR: Add minimal mypy annotations PR: Fix/suppress all mypy complaints Feb 25, 2023
@edreamleo edreamleo changed the title PR: Fix/suppress all mypy complaints PR: Fix/suppress all mypy complaints (based on master) Feb 25, 2023
@edreamleo
Copy link
Contributor Author

@lieryan This PR is a milestone in my thinking and workflow:

  • It shows that my concerns about the wildcard import were baseless.
  • I'm amazed how easy it is to create a PR that merges code from my fork to python-rope.

I plan no further work on this PR. Please let me know if you have any concerns or suggestions.

@edreamleo
Copy link
Contributor Author

@lieryan Please comment on this PR in a timely way. I sometimes feel like I'm talking to myself.

If you are too busy to take a close look, just saying that would be courteous.

@@ -225,6 +228,9 @@ def get_module(self):
current_object = current_object.parent
return current_object

def get_name(self):
return None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this method and get_module() aren't supposed to be called, it's just a dummy implementation to satisfy mypy.

Maybe it should raise NotImplementedError or something like that instead to indicate that only the child class versions of these methods should actually get called.

Copy link
Contributor Author

@edreamleo edreamleo Feb 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's true that Rope probably never calls PyDefinedObject.get_name.

Imo, however, having to create these two methods indicates a more serious flaw in the Abstract classes.

At present, the draft PR #675 implements only step 1 of what might become a multi-step project. This first step merely moves methods from the Abstract classes into what seem to me to be the more natural places, namely the po.PyFunction and po.PyClass classes (where po is my abbreviation for rope.base.pyobjects).

Step 2 of PR #675 proposes to make PyDefinedObject a subclass of po.PyObject. However, this step is likely controversial and I have no great attachment to this step.

Oh what the heck. I may as well say here something that I was planning to save for a discussion.

After weeks of studying Rope's code I finally see the following:

  1. Knowledge is a blessing as well as a curse :-)
    Knowledge helps experienced devs know what's important and what isn't.
  2. For newbies the lack of knowledge is a curse.
    We can drown in details because we don't know which tidbit might be crucial.
    That's why your comments about inference were so important to me.
    I've just spent several hours revising the second comment of forked issue #3.
    I could not have done so without your help.
  3. Imo, Rope uses a strange form of inheritance—inheritance via mro.
    I understand what's happening now that I have spent days trying to revise it.
    But the class hierarchy will be difficult for newbies to understand without help.
    The Theory of Operation should explain in detail what is going on.
    The alternative is to use a more straightforward form of inheritance.
    The (closed) PR PR: Simplify Rope's class hierarchy #664 demonstrates that such a simplification straightforward!
    The PR may have gone too far by eliminating the Abstract class entirely. It's an open question.

Summary

Newbies will struggle to understand the relations between Rope's most important classes.

PR #664 shows that the class hierarchy can be simplified. If this doesn't happen the Theory of Operation should contain a section about how Rope's classes are related.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh. The phrase "inheritance by mro" is a fail.

The Aha is that the classes of interest, po.PyObject, po.PyDefinedObject and the three Abstract classes are (mostly) used as mixin classes. None of these classes are subclasses of the others, except that the Abstract classes are indeed subclasses of po.PyObject.

The existing class declarations are:

class BuiltinClass(_BuiltinElement, pyobjects.AbstractClass):
class BuiltinFunction(_BuiltinElement, pyobjects.AbstractFunction):
class BuiltinModule(pyobjects.AbstractModule):
class Generator(pyobjects.AbstractClass):
class Iterator(pyobjects.AbstractClass):
class Lambda(pyobjects.AbstractFunction):
class AbstractClass(PyObject):
class AbstractFunction(PyObject):
class PyObject:
class PyDefinedObject:
class AbstractModule(PyObject):
class PyClass(PyDefinedObject, AbstractClass):
class PyFunction(PyDefinedObject, AbstractFunction):
class _PyModule(PyDefinedObject, AbstractModule):

In other words, PyDefinedObject acts like a PyObject even thought PyDefinedObject is not a subclass of PyObject. Why? Because all subclasses of PyDefinedObject are also subclasses of an Abstract class so that PyObject appears in the mro.

There is a weird kind of logic in this scheme. It's crazy, but imo it's not crazy enough to be right. PR #664 demonstrates that a simpler (more conventional) scheme is feasible.

@@ -324,15 +324,21 @@ def get_changes(
will be applied to all python files.

"""
# To do (in another PR): Create a new var: dest_resource: resource.Reource
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't really recommend creating a new argument here. get_changes() is a public API, pretty much all integrations that supports refactoring would have directly called it. I would be very cautious on changing the API here as it's likely going to break some integrations.

What I'd recommend here is to annotate is_folder() to become a TypeGuard. Unfortunately, TypeGuard is only available in Python 3.9 and above.

IMO, we don't need to maintain backwards compatibility with type annotation. If static checks doesn't pass in older Python version, as long as the code is unaffected at runtime, that's perfectly fine to me.

Alternatively, we could add development dependency on typing-extensions. which has a backport of typing.TypeGuard.

Copy link
Contributor Author

@edreamleo edreamleo Feb 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment wasn't clear. I wasn't suggesting changing the api, I was suggesting adding a new var (not argument). This change would be an example of what I called (in another reply) "splitting" a var. mypy will complain about vars that hold values of differing types.

Imo, it's no big deal whether the existing code ever changes to please mypy.

@@ -5,7 +5,8 @@
from rope.base import fscommands

try:
from ast import _const_node_type_names
# Suppress the mypy complaint: Module "ast" has no attribute "_const_node_type_names"
from ast import _const_node_type_names # type:ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of sprinkling so many type: ignore everywhere. Is there any way to tell mypy to just ignore untyped modules by default.

Copy link
Contributor Author

@edreamleo edreamleo Feb 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lieryan There are several ways of telling mypy to ignore untyped modules:

  • This PR inserts a type: ignore as the very first line of rope.base.prefs to suppress all checks there.
  • .mypy config files supposedly can suppress checks to specific modules.
    However, this capability seems buggy. It seems to suppress initial checks,
    but inferences in other files seem to "bleed into" the supposedly suppressed modules.

One alternative to type: ignore is annotating something as Any. However, this just substitutes one suppression pattern for another.

Let's look at the type: ignore statements in this PR:

  • The type: ignore on line 8 of rope.base.ast is a special case. I don't know any way to remove it.
  • All the suppressions in fscommands, doa, runmod, numpydocstrings, and project suppress
    warnings about third-party imports that might fail. These suppressions seem harmless and natural.
  • oi.type_hinting_evaluate: suppress (valid) mypy complaints about the @multi decorator.
    I don't understand why @multi is needed, so I can't suggest any alternative.
    My guess: type: ignore is likely the only way.
  • I know of no easy way to avoid the type: ignore in contrib.autoimport.sqlite,
    but I didn't try very hard—this file doesn't interest me at present.
  • Afaik, the type: ignore suppressions in refactor.move can only be removed by "splitting" a name which is assigned values with different types.
    This coding style becomes natural when using mypy. In my experience it always clarifies the code.
    But this PR avoids substantive changes to the code.

Summary

Most type: ignore suppressions seem harmless. The others seem unavoidable.

I'm not bothered by the type: ignore comments. There aren't that many, and I've carefully documented all the unusual suppressions.

@edreamleo
Copy link
Contributor Author

@lieryan Thanks for all your comments. Please resolve the conversations when you are ready. For now I have nothing more to add.

I'll be happy to make any changes you would like. I would appreciate getting this PR merged asap. I would like to use it as the base for future work.

Seems like an unnecessary mental indirection to alias this when it's
just used twice in the same type definition. Maybe if there's more of
this pair, then it might be worth it to define this alias.
@lieryan lieryan enabled auto-merge March 2, 2023 05:14
@lieryan
Copy link
Member

lieryan commented Mar 2, 2023

Thanks for fixing these mypy issues

@lieryan lieryan disabled auto-merge March 2, 2023 05:15
@lieryan lieryan enabled auto-merge March 2, 2023 05:16
@lieryan lieryan merged commit 7e1c897 into python-rope:master Mar 2, 2023
@edreamleo
Copy link
Contributor Author

@lieryan You're welcome!

@edreamleo edreamleo deleted the ekr-annotations branch March 2, 2023 09:03
@edreamleo
Copy link
Contributor Author

@lieryan I wish to acknowledge that my concerns about from ast import * were unfounded. That is, this PR shows that the wildcard import does not impede mypy significantly. It's sometimes a great relief to admit that I was wrong, and this is one of those times :-)

@lieryan lieryan added this to the 1.8.0 milestone May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants