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 3 commits
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
67 changes: 32 additions & 35 deletions pylint/pyreverse/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,23 @@
import shutil
import subprocess
import sys
from typing import TYPE_CHECKING, Any, Callable, Optional, Tuple, Union

import astroid
from astroid import nodes

if TYPE_CHECKING:
from pylint.pyreverse.diadefslib import DiaDefGenerator
from pylint.pyreverse.diagrams import ClassDiagram, PackageDiagram
from pylint.pyreverse.inspector import Linker

_CallbackT = Callable[
[nodes.NodeNG],
Optional[Union[Tuple[ClassDiagram], Tuple[PackageDiagram, ClassDiagram]]],
]
_CallbackTupleT = Tuple[Optional[_CallbackT], Optional[_CallbackT]]


RCFILE = ".pyreverserc"


Expand Down Expand Up @@ -46,7 +59,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 +73,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 +105,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 +115,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 +131,14 @@ class ASTWalker:
the node in lower case
"""

def __init__(self, handler):
def __init__(self, handler: DiaDefGenerator | Linker | LocalsVisitor) -> None:
self.handler = handler
self._cache = {}
self._cache: dict[
type[nodes.NodeNG],
_CallbackTupleT,
] = {}

def walk(self, node, _done=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 +152,7 @@ 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) -> _CallbackTupleT:
"""Get callbacks from handler for the visited node."""
klass = node.__class__
methods = self._cache.get(klass)
Expand All @@ -173,13 +170,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 +186,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 +250,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 +266,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