Add typing to NodeNG.nodes_of_class

[DanielNoord]

Steps

• For new features or bug fixes, add a ChangeLog entry describing what your PR does.
• Write a good description on what the PR does.

Description

This uses overloads to type the nodes_of_class function.

For some reason pylint doesn't pick up on the iterable context correctly. Some help would be appreciated! This is not shown in the CI but does show up locally.

Type of Changes

	Type
✓	🔨 Refactoring

Related Issue

[DanielNoord]

As said in the description: would like some help figuring out why pylint throws an error. I made a basic test case with overload and pylint did seem to understand them. Not sure why it doesn't pick up that the function will always return an iterable.

EDIT: I have done some additional investigations. The problem is that self.get_children() does not seem to recognise the full function. See the following output:

Pdb) self
<FunctionDef.nodes_of_class l.504 at 0x109915e80>
(Pdb) [i for i in self.get_children()]
[<Decorators l.503 at 0x1094b9580>, <Arguments l.505 at 0x1094b97c0>, <Subscript l.508 at 0x1094b9640>, <Expr l.509 at 0x1094b9220>]

L509 is the ... used in the overload but the other overloads and actual function don't get parsed. This requires some extensive knowledge of the AstroidBuilder, which I don't have. Perhaps one of the maintainers knows if 1) this is a known problem or 2) if there is a possible fix for this or 3) can point me to the function that should be fixed to make this work.

EDIT2: Looks like #1015 has already indicated this

[cdce8p]

Thanks for starting the work on this one! It was long overdue.
I left a few comments. Basically, all current overloads are redundant and can be removed.

--
One area we still need to take a look at is the annotation for klass. As this can be a tuple as well, it complicates things. An idea:

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:
    @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]]:
        ...

    # Technically any number of additional overloads with a different number of
    # items for 'klass' could be added. However, 3 should cover most cases
    # Use the last overload as fallback.
    # Unfortunately, this doesn't work in 'mypy'. Probably because of a bug.

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

    def nodes_of_class(
        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], ...],  # related to the 'mypy' bug. Needs to be excluded as well
        ],
        skip_klass: SkipKlassT = None,
    ) -> Union[Iterator[T_Nodes], Iterator[T_Nodes2], Iterator[T_Nodes3]]:
        # implementation
        pass

--
Regarding the pylint issue. I would think this is a bug. Need to take a look at it sometime. For now, I would recommend to use # pylint: ignore and link the issue.

[DanielNoord]

Wow, this is a much cleaner solution! I did not know bound worked like this. Hopefully it didn't take you you too much time to come up with this.
I do think this will be really helpful in tackling some of the mypy issues for pylint as we get much more precise typing now :)
Glad to have put this on your radar!

[cdce8p]

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.

[DanielNoord]

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!

[cdce8p]

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

[DanielNoord]

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?

[cdce8p]

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.

[cdce8p]

Hopefully it didn't take you you too much time to come up with this.

I spend most of the time dealing with type checker issues to be honest. The one I mentioned for mypy and also opened 2 for pyright.

do think this will be really helpful in tackling some of the mypy issues for pylint as we get much more precise typing now :)

We should be able to remove a few typing.cast as well.

[cdce8p]

@DanielNoord Would you like to work on a follow up PR for pylint to remove now redundant typing.cast calls? Searching for = cast(nodes. should probably do the trick.

[cdce8p]

Looks good 🚀
Thanks @DanielNoord!

[DanielNoord]

@DanielNoord Would you like to work on a follow up PR for pylint to remove now redundant typing.cast calls? Searching for = cast(nodes. should probably do the trick.

Yes! You can assign that (imaginary) issue to me 😄