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

Add typing to NodeNG.nodes_of_class #1168

Merged
merged 6 commits into from
Sep 13, 2021
Merged

Add typing to NodeNG.nodes_of_class #1168

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bb682a0
Add typing to ``NodeNG.nodes_of_class``
DanielNoord Sep 11, 2021
86c559f
Add Tuple typing import
DanielNoord Sep 12, 2021
fbd24e2
Adopt code review
DanielNoord Sep 12, 2021
5e9baf9
Update astroid/nodes/scoped_nodes.py
cdce8p Sep 13, 2021
80e5627
Update astroid/nodes/node_ng.py
cdce8p Sep 13, 2021
4b60299
Remove redundant docstrings
cdce8p Sep 13, 2021
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
53 changes: 50 additions & 3 deletions astroid/nodes/node_ng.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,20 @@
import pprint
import typing
from functools import singledispatch as _singledispatch
from typing import ClassVar, Optional
from typing import ClassVar, Iterator, Optional, Tuple, Type, TypeVar, Union, overload

from astroid import decorators, util
from astroid.exceptions import AstroidError, InferenceError, UseInferenceDefault
from astroid.manager import AstroidManager
from astroid.nodes.as_string import AsStringVisitor
from astroid.nodes.const import OP_PRECEDENCE

# These TypeVar's are used to type NodeNG.nodes_of_class()
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
T_Nodes = TypeVar("T_Nodes", bound="NodeNG")
T_Nodes2 = TypeVar("T_Nodes2", bound="NodeNG")
T_Nodes3 = TypeVar("T_Nodes3", bound="NodeNG")
SkipKlassT = Union[None, Type["NodeNG"], Tuple[Type["NodeNG"], ...]]


class NodeNG:
"""A node of the new Abstract Syntax Tree (AST).
Expand Down Expand Up @@ -179,7 +185,7 @@ def accept(self, visitor):
func = getattr(visitor, "visit_" + self.__class__.__name__.lower())
return func(self)

def get_children(self):
def get_children(self) -> Iterator["NodeNG"]:
"""Get the child nodes below this node.

:returns: The children.
Expand Down Expand Up @@ -402,7 +408,48 @@ def set_local(self, name, stmt):
"""
self.parent.set_local(name, stmt)

def nodes_of_class(self, klass, skip_klass=None):
@overload
def nodes_of_class(
self,
klass: Type[T_Nodes],
skip_klass: SkipKlassT = None,
) -> Iterator[T_Nodes]:
...

@overload
def nodes_of_class(
self,
klass: Tuple[Type[T_Nodes], Type[T_Nodes2]],
skip_klass: SkipKlassT = None,
) -> Union[Iterator[T_Nodes], Iterator[T_Nodes2]]:
...

@overload
def nodes_of_class(
self,
klass: Tuple[Type[T_Nodes], Type[T_Nodes2], Type[T_Nodes3]],
skip_klass: SkipKlassT = None,
) -> Union[Iterator[T_Nodes], Iterator[T_Nodes2], Iterator[T_Nodes3]]:
...

@overload
def nodes_of_class(
self,
klass: Tuple[Type[T_Nodes], ...],
skip_klass: SkipKlassT = None,
) -> Iterator[T_Nodes]:
...

def nodes_of_class( # type: ignore # mypy doesn't correctly recognize the overloads
self,
klass: Union[
Type[T_Nodes],
Tuple[Type[T_Nodes], Type[T_Nodes2]],
Tuple[Type[T_Nodes], Type[T_Nodes2], Type[T_Nodes3]],
Tuple[Type[T_Nodes], ...],
],
skip_klass: SkipKlassT = None,
) -> Union[Iterator[T_Nodes], Iterator[T_Nodes2], Iterator[T_Nodes3]]:
"""Get the nodes (including this one or below) of the given types.

:param klass: The types of node to search for.
Expand Down
6 changes: 5 additions & 1 deletion astroid/nodes/scoped_nodes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1734,7 +1734,11 @@ def infer_yield_result(self, context=None):
:returns: What the function yields
:rtype: iterable(NodeNG or Uninferable) or None
"""
for yield_ in self.nodes_of_class(node_classes.Yield):
for (
yield_
) in self.nodes_of_class( # pylint: disable=not-an-iterable # See https://github.com/PyCQA/astroid/issues/1015
Copy link
Member

Choose a reason for hiding this comment

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

Just realized that we would need to add this to every call in pylint as well 😕
The issue should probably be fixed before we can merge it.

Copy link
Collaborator Author

@DanielNoord DanielNoord Sep 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I did a pip install -e local_astroid_folder and succeeded in running the pre-commit to test for this. (in my local pylint folder of course)

The problem comes from overloads where the ... is on a separate line (I think).
We don't have those in pylint (yet), so I think for the time-being we are fine.

You might want to check this though by doing pip -e as well, just to be sure!

Copy link
Member

Choose a reason for hiding this comment

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

Just tested it as well and you're right. At the moment pylint doesn't complain. The reason however isn't that we don't use overloads. It's actually that node in visit_xxx(self, node) cannot be inferred and thus no error is emitted. In contrast, just doing something like this will easily produce one:

def func():
    node = nodes.For()
    for n in node.nodes_of_class(nodes.Name):  # not-an-iterable
        pass

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Didn't we just add typing to all visit_xxx(self, node) calls in pylint? Or is that not enough to help mypy?

I saw you just assigned somebody to #1015. I think we should block this until that issue has been fixed, right?

Copy link
Member

Choose a reason for hiding this comment

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

Except from some corner cases, type annotations aren't used to infer values in pylint. That is different in mypy.

I think we should block this until that issue has been fixed, right?

Since this doesn't actually cause any issues with pylint, I would say we can merge it now. It doesn't necessarily need to wait for #1015.

node_classes.Yield
):
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
if yield_.value is None:
const = node_classes.Const(None)
const.parent = yield_
Expand Down