Finish typing of pylint.pyreverse.utils
#6549
Conversation
DudeNr33
added
pyreverse
| 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) |
There was a problem hiding this comment.
Dead code -> removed instead of adding typing
| # bw compatibility | ||
| return node.type == "exception" | ||
|
|
||
|
|
||
| # Helpers ##################################################################### | ||
|
|
||
| _CONSTRUCTOR = 1 |
pylint/pyreverse/utils.py
Outdated
| ) -> tuple[ | ||
| Callable[[nodes.NodeNG], Any] | None, Callable[[nodes.NodeNG], Any] | None | ||
| ]: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Pull Request Test Coverage Report for Build 2308294733
💛 - Coveralls |
|
|
||
|
|
||
| def is_interface(node): | ||
| def is_interface(node: nodes.ClassDef) -> bool: | ||
| # bw compatibility | ||
| return node.type == "interface" |
There was a problem hiding this comment.
So, here, mypy thinks we return "Any" ?!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
pylint/pyreverse/utils.py
Outdated
| self._cache: dict[ | ||
| type[nodes.NodeNG], | ||
| tuple[ | ||
| Callable[[nodes.NodeNG], Any] | None, |
There was a problem hiding this comment.
Don't we know what these Callable return?
There was a problem hiding this comment.
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]]
There was a problem hiding this comment.
I'd be in favour of using the narrowed down type if we can. We can always change it after a refactor later.
|
|
||
| def visit(self, node): | ||
| def visit(self, node: nodes.NodeNG) -> Any: |
There was a problem hiding this comment.
Is this truly Any? Don't all visit_ methods return None?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good catch. I can update both methods to use Union[Tuple[ClassDiagram], Tuple[PackageDiagram, ClassDiagram], None].
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
I'm trying to incorporate the last changes requested by @DanielNoord, but I'm running into strange problems. I introduced a variable for the callback types in the if TYPE_CHECKING:
# pylint: disable=unsupported-binary-operation
from pylint.pyreverse.diadefslib import DiaDefGenerator
from pylint.pyreverse.diagrams import ClassDiagram, PackageDiagram
from pylint.pyreverse.inspector import Linker
_CallbackT = Callable[
[nodes.NodeNG], tuple[ClassDiagram] | tuple[PackageDiagram, ClassDiagram] | None
]
_CallbackTupleT = tuple[_CallbackT | None, _CallbackT | None]When I stage those changes and run However, as soon as I run Shouldn't the results be the same? |
Try |
|
I think as those lines are in the
Might be a caching problem? The CI re-uses a cached environment for |
Yeah, technically true. I'm not sure though mypy knows that the type alias is defined in a
Similar issue with PEP 604, i.e. Union operator. Use |
Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
|
Thanks a lot for your help, that would have cost me ages to figure out on my own. |
|
|
||
| _CallbackT = Callable[ | ||
| [nodes.NodeNG], | ||
| Union[Tuple[ClassDiagram], Tuple[PackageDiagram, ClassDiagram], None], |
There was a problem hiding this comment.
Returning tuples of different lengths is probably not the best API design
There was a problem hiding this comment.
No doubt about that. The code where this happens is over 16 years old, and I haven't touched those parts of pyreverse yet. I can take a look at refactoring those parts that we found to be problematic due to the typing in a follow-up PR.
|
|
||
| def visit(self, node): | ||
| def visit(self, node: nodes.NodeNG) -> Any: |
There was a problem hiding this comment.
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.
Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
| @@ -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)) | |||
There was a problem hiding this comment.
| visibility = get_visibility(getattr(node, "name", node)) | |
| visibility = get_visibility(getattr(node, "name", node)) |
Unrelated, but this is quite a funny hack 😄
I annotated both parent and child implementation with |
and preferred name in
script/.contributors_aliases.jsonType of Changes
Description
This module already had some typing, added typing to the classes and functions that did not have any yet.
Running with
mypy --strictfour issues remain, which I can't wrap my head around whymypycomplains about them: