[typing] Add typing to the class checkers #7190
Conversation
Pierre-Sassoulas
added
Maintenance
Pierre-Sassoulas
force-pushed
the
add-typing-to-checker-classes
branch
from
691a583
to
391708f
Compare
Pull Request Test Coverage Report for Build 2833516279
💛 - Coveralls |
This comment has been minimized.
This comment has been minimized.
jacobtylerwalls
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -1443,7 +1458,9 @@ def _check_redefined_slots( | |||
| node=slots_node, | |||
| ) | |||
|
|
|||
| def _check_slots_elt(self, elt, node): | |||
| def _check_slots_elt( | |||
| self, elt: nodes.Attribute | nodes.Const, node: nodes.ClassDef | |||
There was a problem hiding this comment.
Where is the type of elt determined? In the calling function it seems to be SuccessfulInferenceResult?
There was a problem hiding this comment.
Most of the values are based on what were the actual type of the node when launching the tests. There's often a lof of condition applied on things after the inference is done so it narrows the type of sub-functions quite a lot sometime.
There was a problem hiding this comment.
Yeah, but just based on the current typing and code this typing is incorrect. We should mostly rely on the code rather than on tests as the tests will never be cover every edge case.
| return isinstance(func, nodes.FunctionDef) and ( | ||
| func.type == "classmethod" or func.name == "__class_getitem__" | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def _is_inferred_instance(expr, klass): | ||
| def _is_inferred_instance(expr: nodes.NodeNG | None, klass: nodes.ClassDef) -> bool: |
There was a problem hiding this comment.
The typing and the docstring in astroid don't correspond with each other here. Shall we just remove the typing and fix that first?
There was a problem hiding this comment.
This would work but maybe we could add best effort values in pylint and see where mypy is unhappy once astroid is actually typed ? (The overload for infer depending on what node we're inferring and based on condition on the node can become quite complicated, and won't be done in a month).
There was a problem hiding this comment.
The issue is that we won't know which signatures to revisit when we finally do update astroid. For any typing for which we are not sure that astroid is correct I think it is often better to leave them be and try and fix astroid first.
| if not self._is_iterator(inferred): | ||
| self.add_message("non-iterator-returned", node=node) | ||
|
|
||
| def _check_len(self, node, inferred): | ||
| def _check_len(self, node: nodes.FunctionDef, inferred: nodes.Const) -> None: |
There was a problem hiding this comment.
Just picking this one as it is the easiest to illustrate, I don't really understand where these typings are coming from? Don't they all come from _protocol_map and called with Callable[[nodes.FunctionDef, Any], None]?
The Any being the result of infer_call_result? Why can this only be Const?
There was a problem hiding this comment.
For:
def __len__(self): # [invalid-length-returned]
return lambda: 3
It's not a const but a ``<class 'astroid.nodes.scoped_nodes.scoped_nodes.Lambda>'. pyannotate must have a bug.
There was a problem hiding this comment.
I think this is not necessarily a bug with pyannotate but just shows that our test case doesn't cover all edge cases. Like I said, perhaps revert the changes here and keep this PR focused on those that are definetly correct?
There was a problem hiding this comment.
our test case doesn't cover all edge cases.
Although true in the general case, I added an assert in def _check_len, the example come from invalid_length_returned.py that is supposed to be launched during functional tests so either pyannotate has a bug or the way we launch tests is not understood.
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
for more information, see https://pre-commit.ci
Pierre-Sassoulas
force-pushed
the
add-typing-to-checker-classes
branch
from
245bc90
to
e69f75e
Compare
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
|
@Pierre-Sassoulas Please don't rebase or force push these larger PRs, pretty difficult to keep track of the changes I already reviewed that way 😅 |
| node: nodes.FunctionDef, | ||
| caller: nodes.FunctionDef, | ||
| context: InferenceContext | None = None, | ||
| ) -> Iterator[nodes.NodeNG | Uninferable | None] | None: |
There was a problem hiding this comment.
See previous comments, why isn't this InferenceResult?
There was a problem hiding this comment.
I did not take most comment into account yet just the one regarding f-string
@Pierre-Sassoulas
Use an fstring so tuples with more than 2 element do not crash
1a06694
@Pierre-Sassoulas
Use standard import for ImportNode
e69f75e
pylint/checkers/utils.py
Outdated
| @@ -1653,7 +1652,7 @@ def get_subscript_const_value(node: nodes.Subscript) -> nodes.Const: | |||
| return inferred | |||
|
|
|||
|
|
|||
| def get_import_name(importnode: ImportNode, modname: str | None) -> str | None: | |||
| def get_import_name(importnode: nodes.ImportNode, modname: str | None) -> str | None: | |||
There was a problem hiding this comment.
This shouldn't work. ImportNode is not part of the nodes module.
There was a problem hiding this comment.
Hmm; it's true I don't see it in astroid.nodes's API. But the tests are green.
There was a problem hiding this comment.
That is because mypy doesn't actually check astroid since it has no py.typed file.
There was a problem hiding this comment.
Ha, I thought the import would have failed but it does not because it's using a namespace and not a direct import (nodes.ImportNode, with from astroid import nodes not from astroid.nodes import ImportNode.
This comment has been minimized.
This comment has been minimized.
|
@Pierre-Sassoulas Would you be okay with me finishing this PR with some final changes? |
|
Sure, I would appreciate that ! |
|
@Pierre-Sassoulas PR is now ready for review! |
This comment has been minimized.
This comment has been minimized.
|
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit b1d0403 |
Type of Changes
Description
Refs #2079