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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0ab5815
Add minimal mypy annotations to eliminate all complaints. Add two nec…
edreamleo Feb 25, 2023
df8224a
Explain the suppression in rope.base.ast
edreamleo Feb 25, 2023
bcabc86
Remove unnecessary 'Any' annotation to reduce the diffs
edreamleo Feb 25, 2023
f1310de
Merge branch 'master' into ekr-annotations
edreamleo Feb 25, 2023
845786b
Improve a comment
edreamleo Feb 25, 2023
7af5370
Inline PyObj
lieryan Mar 2, 2023
bcc5cf8
Add a comment on resolve_type()
lieryan Mar 2, 2023
0d13f87
Update CHANGELOG.md
lieryan Mar 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

- #650 Install pre-commit hooks on rope repository (@lieryan)
- #655 Remove unused __init__() methods (@edreamleo, @lieryan)
- #674 Fix/supress all mypy complaints


# Release 1.7.0
Expand Down
3 changes: 2 additions & 1 deletion rope/base/ast.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited

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.

except ImportError:
# backported from stdlib `ast`
assert sys.version_info < (3, 8)
Expand Down
8 changes: 4 additions & 4 deletions rope/base/fscommands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ def read(self, path):
class SubversionCommands:
def __init__(self, *args):
self.normal_actions = FileSystemCommands()
import pysvn
import pysvn # type:ignore

self.client = pysvn.Client()

Expand Down Expand Up @@ -114,9 +114,9 @@ def __init__(self, root):
self.repo = self.hg.hg.repository(self.ui, root)

def _import_mercurial(self):
import mercurial.commands
import mercurial.hg
import mercurial.ui
import mercurial.commands # type:ignore
import mercurial.hg # type:ignore
import mercurial.ui # type:ignore

return mercurial

Expand Down
4 changes: 2 additions & 2 deletions rope/base/oi/doa.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
import hmac

try:
import cPickle as pickle
import cPickle as pickle # type:ignore
except ImportError:
import pickle
import pickle # type:ignore
import marshal
import os
import socket
Expand Down
2 changes: 1 addition & 1 deletion rope/base/oi/runmod.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ def __rope_start_everything():
import sys

try:
import cPickle as pickle
import cPickle as pickle # type:ignore
except ImportError:
import pickle
import base64
Expand Down
26 changes: 13 additions & 13 deletions rope/base/oi/type_hinting/evaluate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class S(SymbolBase):
s.lbp = max(bp, s.lbp)
return s

@multi
@multi # type:ignore
def infix(self, name, bp):
symbol = self.symbol(name, bp)

Expand All @@ -79,7 +79,7 @@ def led(self, left, parser):
self.second = parser.expression(bp)
return self

@multi
@multi # type:ignore
def infix_r(self, name, bp):
symbol = self.symbol(name, bp)

Expand All @@ -101,16 +101,16 @@ def led(self, left, parser):
self.third = parser.expression(symbol2.lbp + 0.1)
return self

@multi
def prefix(self, name, bp):
@multi # type:ignore
def prefix(self, name, bp): # type:ignore
symbol = self.symbol(name, bp)

@method(symbol)
def nud(self, parser):
self.first = parser.expression(bp)
return self

@multi
@multi # type:ignore
def postfix(self, name, bp):
symbol = self.symbol(name, bp)

Expand Down Expand Up @@ -237,18 +237,18 @@ def bind(fn):
symbol("(end)")


@method(symbol("(name)"))
@method(symbol("(name)")) # type:ignore
def nud(self, parser):
return self


@method(symbol("(name)"))
@method(symbol("(name)")) # type:ignore
def evaluate(self, pyobject):
return utils.resolve_type(self.value, pyobject)


# Parametrized objects
@method(symbol("["))
@method(symbol("[")) # type:ignore
def led(self, left, parser):
self.first = left
self.second = []
Expand All @@ -264,15 +264,15 @@ def led(self, left, parser):
return self


@method(symbol("["))
@method(symbol("[")) # type:ignore
def evaluate(self, pyobject):
return utils.parametrize_type(
self.first.evaluate(pyobject), *[i.evaluate(pyobject) for i in self.second]
)


# Anonymous Function Calls
@method(symbol("("))
@method(symbol("(")) # type:ignore
def nud(self, parser):
self.second = []
if parser.token.name != ")":
Expand All @@ -288,7 +288,7 @@ def nud(self, parser):


# Function Calls
@method(symbol("("))
@method(symbol("(")) # type:ignore
def led(self, left, parser):
self.first = left
self.second = []
Expand All @@ -304,13 +304,13 @@ def led(self, left, parser):
return self


@method(symbol("("))
@method(symbol("(")) # type:ignore
def evaluate(self, pyobject):
# TODO: Implement me
raise NotImplementedError


@method(symbol("or"))
@method(symbol("or")) # type:ignore
@method(symbol("|"))
def evaluate(self, pyobject):
# TODO: Implement me
Expand Down
4 changes: 2 additions & 2 deletions rope/base/oi/type_hinting/providers/numpydocstrings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from rope.base.oi.type_hinting.providers import docstrings

try:
from numpydoc.docscrape import NumpyDocString
from numpydoc.docscrape import NumpyDocString # type:ignore
except ImportError:
NumpyDocString = None

Expand Down Expand Up @@ -40,4 +40,4 @@ def __call__(self, docstring, param_name):


if not NumpyDocString:
NumPyDocstringParamParser = _DummyParamParser
NumPyDocstringParamParser = _DummyParamParser # type:ignore
13 changes: 10 additions & 3 deletions rope/base/oi/type_hinting/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from __future__ import annotations

import logging
from typing import Optional, Union
from typing import TYPE_CHECKING, Optional, Union

import rope.base.utils as base_utils
from rope.base import evaluate
Expand Down Expand Up @@ -72,8 +74,10 @@ def get_mro(pyclass):
return class_list


def resolve_type(type_name, pyobject):
# type: (str, Union[PyDefinedObject, PyObject]) -> Optional[PyDefinedObject, PyObject]
def resolve_type(
type_name: str,
pyobject: Union[PyDefinedObject, PyObject],
) -> Optional[Union[PyDefinedObject, PyObject]]:
"""
Find proper type object from its name.
"""
Expand All @@ -82,6 +86,9 @@ def resolve_type(type_name, pyobject):
logging.debug("Looking for %s", type_name)
if "." not in type_name:
try:
# XXX: this looks incorrect? It doesn't seem like it would work
# correctly if you have a type/class not defined in the
# module/global scope
ret_type = (
pyobject.get_module().get_scope().get_name(type_name).get_object()
)
Expand Down
2 changes: 2 additions & 0 deletions rope/base/prefs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# mypy reports many problems.
# type: ignore
"""Rope preferences."""
from dataclasses import asdict, dataclass
from textwrap import dedent
Expand Down
10 changes: 7 additions & 3 deletions rope/base/project.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from __future__ import annotations
import contextlib
import json
import os
Expand All @@ -10,13 +11,16 @@
import rope.base.resourceobserver as resourceobserver
from rope.base import exceptions, history, pycore, taskhandle, utils
from rope.base.exceptions import ModuleNotFoundError
from rope.base.prefs import Prefs, get_config

# At present rope.base.prefs starts with `# type:ignore`.
# As a result, mypy knows nothing about Prefs and get_config.
from rope.base.prefs import Prefs, get_config # type:ignore
from rope.base.resources import File, Folder, _ResourceMatcher

try:
import cPickle as pickle
import cPickle as pickle # type:ignore
except ImportError:
import pickle
import pickle # type:ignore


class _Project:
Expand Down
6 changes: 6 additions & 0 deletions rope/base/pyobjects.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ def get_attribute(self, name):
raise exceptions.AttributeNotFoundError("Attribute %s not found" % name)
return self.get_attributes()[name]

def get_module(self):
return None

def get_type(self):
return self.type

Expand Down Expand Up @@ -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 •
edited

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.


def get_doc(self) -> Optional[str]:
if len(self.get_ast().body) > 0:
expr = self.get_ast().body[0]
Expand Down
2 changes: 1 addition & 1 deletion rope/contrib/autoimport/sqlite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,7 @@ def filter_folders(folder: Path) -> bool:

folders = self.project.get_python_path_folders()
folder_paths = map(lambda folder: Path(folder.real_path), folders)
folder_paths = filter(filter_folders, folder_paths)
folder_paths = filter(filter_folders, folder_paths) # type:ignore
return list(OrderedDict.fromkeys(folder_paths))

def _get_available_packages(self) -> List[Package]:
Expand Down
3 changes: 2 additions & 1 deletion rope/refactor/extract.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import re
from typing import Dict
from contextlib import contextmanager
from itertools import chain

Expand Down Expand Up @@ -34,7 +35,7 @@
# There are a few more helper functions and classes used by above
# classes.
class _ExtractRefactoring:
kind_prefixes = {}
kind_prefixes: Dict[str, str] = {}
edreamleo marked this conversation as resolved.
Show resolved Hide resolved

def __init__(self, project, resource, start_offset, end_offset, variable=False):
self.project = project
Expand Down
5 changes: 4 additions & 1 deletion rope/refactor/importutils/importinfo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
from typing import List, Tuple


class ImportStatement:
"""Represent an import in a module

Expand Down Expand Up @@ -187,7 +190,7 @@ def is_star_import(self):


class EmptyImport(ImportInfo):
names_and_aliases = []
names_and_aliases: List[Tuple[str, str]] = []

def is_empty(self):
return True
Expand Down
18 changes: 12 additions & 6 deletions rope/refactor/move.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from __future__ import annotations

import typing
from typing import Union
from typing import Optional, List, Union

from rope.base import (
codeanalyze,
Expand Down Expand Up @@ -310,8 +310,8 @@ def _is_variable(self, pyname):

def get_changes(
self,
dest: Union[None, str, resources.Resource],
resources=None,
dest: Optional[Union[str, resources.Resource]],
resources: Optional[List[resources.File]] = None,
task_handle=taskhandle.DEFAULT_TASK_HANDLE,
):
"""Return the changes needed for this refactoring
Expand All @@ -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 •
edited

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.

# Doing so should remove the type:ignore below.
if isinstance(dest, str):
dest = self.project.find_module(dest)
if resources is None:
resources = self.project.get_python_files()
# The previous guards protect against this mypy complaint:
# "Resource" has no attribute "has_child"
if dest is None or not dest.exists():
raise exceptions.RefactoringError("Move destination does not exist.")
if dest.is_folder() and dest.has_child("__init__.py"):
dest = dest.get_child("__init__.py")
if dest.is_folder():
if dest.is_folder() and dest.has_child("__init__.py"): # type:ignore
dest = dest.get_child("__init__.py") # type:ignore
# The previous guards protect against this mypy complaint:
# Item "None" of "Union[str, Resource, None]" has no attribute "is_folder"
if dest.is_folder(): # type:ignore
raise exceptions.RefactoringError(
"Move destination for non-modules should not be folders."
)
Expand Down