Skip to content

Finish typing of pylint.pyreverse.utils #6549

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

Merged
merged 7 commits into from
May 11, 2022
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 34 additions & 36 deletions pylint/pyreverse/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,17 @@
import shutil
import subprocess
import sys
from collections.abc import Callable
from typing import TYPE_CHECKING, Any

import astroid
from astroid import nodes

if TYPE_CHECKING:
from pylint.pyreverse.diadefslib import DiaDefGenerator
from pylint.pyreverse.inspector import Linker


RCFILE = ".pyreverserc"


Expand Down Expand Up @@ -46,7 +53,7 @@ def insert_default_options() -> None:
PROTECTED = re.compile(r"^_\w*$")


def get_visibility(name):
def get_visibility(name: str) -> str:
"""Return the visibility from a name: public, protected, private or special."""
if SPECIAL.match(name):
visibility = "special"
Expand All @@ -60,37 +67,18 @@ def get_visibility(name):
return visibility


ABSTRACT = re.compile(r"^.*Abstract.*")
FINAL = re.compile(r"^[^\W\da-z]*$")


def is_abstract(node):
"""Return true if the given class node correspond to an abstract class
definition.
"""
return ABSTRACT.match(node.name)


def is_final(node):
"""Return true if the given class/function node correspond to final
definition.
"""
return FINAL.match(node.name)
Comment on lines -63 to -78
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dead code -> removed instead of adding typing



def is_interface(node):
def is_interface(node: nodes.ClassDef) -> bool:
# bw compatibility
return node.type == "interface"
Copy link
Member

Choose a reason for hiding this comment

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

So, here, mypy thinks we return "Any" ?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, no clue why. Maybe because the type attribute is monkey-patched on the node class.
But even VS Code recognises node.type correctly as str.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you, that explains why this happens here.
On lines 213 and 215 I still don't know why mypy complains here:

def get_annotation_label(ann: nodes.Name | nodes.NodeNG) -> str:
    if isinstance(ann, nodes.Name) and ann.name is not None:
        return ann.name. # <-- l.213
    if isinstance(ann, nodes.NodeNG):
        return ann.as_string()  # <-- l.215
    return ""

Using typing.reveal_type(), both ann.name and ann.as_string() are recognised as str, not Any.

Copy link
Member

Choose a reason for hiding this comment

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

Although we have already added some annotations to astroid, mypy will not use them until we add a py.typed file (once they are done). Until then all astroid types are basically inferred as Any.

Copy link
Member

Choose a reason for hiding this comment

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



def is_exception(node):
def is_exception(node: nodes.ClassDef) -> bool:
# bw compatibility
return node.type == "exception"


# Helpers #####################################################################

_CONSTRUCTOR = 1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dead code

_SPECIAL = 2
_PROTECTED = 4
_PRIVATE = 8
Expand All @@ -111,7 +99,7 @@ def is_exception(node):
class FilterMixIn:
"""Filter nodes according to a mode and nodes' visibility."""

def __init__(self, mode):
def __init__(self, mode: str) -> None:
"""Init filter modes."""
__mode = 0
for nummod in mode.split("+"):
Expand All @@ -121,7 +109,7 @@ def __init__(self, mode):
print(f"Unknown filter mode {ex}", file=sys.stderr)
self.__mode = __mode

def show_attr(self, node):
def show_attr(self, node: nodes.NodeNG | str) -> bool:
"""Return true if the node should be treated."""
visibility = get_visibility(getattr(node, "name", node))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
visibility = get_visibility(getattr(node, "name", node))
visibility = get_visibility(getattr(node, "name", node))

Unrelated, but this is quite a funny hack 😄

return not self.__mode & VIS_MOD[visibility]
Expand All @@ -137,11 +125,17 @@ class ASTWalker:
the node in lower case
"""

def __init__(self, handler):
def __init__(self, handler: DiaDefGenerator | Linker | LocalsVisitor) -> None:
self.handler = handler
self._cache = {}

def walk(self, node, _done=None):
self._cache: dict[
type[nodes.NodeNG],
tuple[
Callable[[nodes.NodeNG], Any] | None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we know what these Callable return?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As mentioned above: most of the time None, but for the one leave_project method it is a tuple of ClassDiagram and optionally PackageDiagram. So a more precise type would be:
Callable[[nodes.NodeNG], None | tuple[ClassDiagram] | tuple[PackageDiagram, ClassDiagram]]

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be in favour of using the narrowed down type if we can. We can always change it after a refactor later.

Callable[[nodes.NodeNG], Any] | None,
],
] = {}

def walk(self, node: nodes.NodeNG, _done: set[nodes.NodeNG] | None = None) -> None:
"""Walk on the tree from <node>, getting callbacks from handler."""
if _done is None:
_done = set()
Expand All @@ -155,7 +149,11 @@ def walk(self, node, _done=None):
self.leave(node)
assert node.parent is not node

def get_callbacks(self, node):
def get_callbacks(
self, node: nodes.NodeNG
) -> tuple[
Callable[[nodes.NodeNG], Any] | None, Callable[[nodes.NodeNG], Any] | None
]:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There must be a better way to make a shorthand notation for it (maybe by creating a TypeVar?), but I could not come up with it.
Maybe one of the typing pros @DanielNoord or @cdce8p can help?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use:
MyRecurringType = Callable[[int, int, int, int, int, int, ...], float] at the top of the file just below the imports and then re-use it throughout the file.
The only thing you need to look out for is that Callable from collections.abc then needs to be imported from typing. As this way MyRecurringType is defined at runtime and on <3.8 Callable isn't subscriptable at runtime.

Copy link
Member

Choose a reason for hiding this comment

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

For the callable parameter type, take a look at my comment here as it's not completely correct (but probably the most practical):
https://github.com/PyCQA/pylint/blob/8df920b341a872b85754c910598d27a8846988f9/pylint/utils/ast_walker.py#L18-L24

--
As a side note: Shouldn't the return type be None instead of Any?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you two for the suggestions! I somehow thought just declaring a variable with the type might not work, but I should have probably just tried it. 😄

@cdce8p in most cases, yes - but pyreverse is a bit special here: LocalVisitor.visit(self, node) returns the result of the leave_* method to the caller. In practice, this will be the result from DefaultDiadefGenerator.leave_project(self, _), which is a tuple of ClassDiagram and PackageDiagram.
This could surely be made nicer with a future refactor.

"""Get callbacks from handler for the visited node."""
klass = node.__class__
methods = self._cache.get(klass)
Expand All @@ -173,13 +171,13 @@ def get_callbacks(self, node):
e_method, l_method = methods
return e_method, l_method

def visit(self, node):
def visit(self, node: nodes.NodeNG) -> None:
"""Walk on the tree from <node>, getting callbacks from handler."""
method = self.get_callbacks(node)[0]
if method is not None:
method(node)

def leave(self, node):
def leave(self, node: nodes.NodeNG) -> None:
"""Walk on the tree from <node>, getting callbacks from handler."""
method = self.get_callbacks(node)[1]
if method is not None:
Expand All @@ -189,11 +187,11 @@ def leave(self, node):
class LocalsVisitor(ASTWalker):
"""Visit a project by traversing the locals dictionary."""

def __init__(self):
def __init__(self) -> None:
super().__init__(self)
self._visited = set()
self._visited: set[nodes.NodeNG] = set()

def visit(self, node):
def visit(self, node: nodes.NodeNG) -> Any:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this truly Any? Don't all visit_ methods 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.

With the updated typing, should this be Union[Tuple[ClassDiagram], Tuple[PackageDiagram, ClassDiagram], None]?

If that's the case, the visit method is probably not type compatible with the base implementation in ASTWalker.visit with is annotated to only return None. The type of the child implementation should be a subclass of the parent impl.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. I can update both methods to use Union[Tuple[ClassDiagram], Tuple[PackageDiagram, ClassDiagram], None].

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On second thought:
This also comes down to design problems.
ASTWalker.visit does in fact always return None (there is no return inside the method).
LocalsVisitor (a subclass of ASTWalker) overrides visit and returns whatever the leave method for this node type returns. That's why I put Any here in the first place.

Currently the ASTWalker class is never used on its own. We could simply remove it and move the relevant methods into LocalsVisitor.

Copy link
Member

Choose a reason for hiding this comment

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

That should work. Alternatively, ASTWalker.visit can be annotated with the return type from LocalsVisitor.visit. Doesn't matter that only None is actually returned there. But with that the subclass implementation is compatible.

"""Launch the visit starting from the given node."""
if node in self._visited:
return None
Expand Down Expand Up @@ -253,7 +251,7 @@ def get_annotation(
return ann


def infer_node(node: nodes.AssignAttr | nodes.AssignName) -> set:
def infer_node(node: nodes.AssignAttr | nodes.AssignName) -> set[Any]:
"""Return a set containing the node annotation if it exists
otherwise return a set of the inferred types using the NodeNG.infer method.
"""
Expand All @@ -269,7 +267,7 @@ def infer_node(node: nodes.AssignAttr | nodes.AssignName) -> set:
return {ann} if ann else set()


def check_graphviz_availability():
def check_graphviz_availability() -> None:
"""Check if the ``dot`` command is available on the machine.

This is needed if image output is desired and ``dot`` is used to convert
Expand Down