Implement simple type hints for _ast_gen.py and c_ast.py#600
Conversation
eliben
left a comment
There was a problem hiding this comment.
Can you please isolate just the typing changes for the AST so I can review?
No need to add annotations to ast_gen.py itself -- it's just a simple script that glues strings together
|
Typing changes isolated! |
Repiteo
left a comment
There was a problem hiding this comment.
While the typing changes are now isolated, this consequently re-reveals the bugs that they identified. Here's the most obvious offenders:
| def children(self) -> None: | ||
| """A sequence of all children that are Nodes""" | ||
| pass |
There was a problem hiding this comment.
Typing systems expect subclass return types to match their base class. Given this is an abstract method that's never called, this should be adjusted to:
| def children(self) -> None: | |
| """A sequence of all children that are Nodes""" | |
| pass | |
| def children(self) -> tuple[tuple[str, Node], ...]: | |
| """A sequence of all children that are Nodes""" | |
| raise NotImplementedException() |
| __slots__: ClassVar[tuple[str, ...]] = () | ||
| """ Abstract base class for AST nodes. | ||
| """ |
There was a problem hiding this comment.
Should be moved below the docstring so it actually can function as a docstring
| __slots__: ClassVar[tuple[str, ...]] = () | |
| """ Abstract base class for AST nodes. | |
| """ | |
| """Abstract base class for AST nodes.""" | |
| __slots__: ClassVar[tuple[str, ...]] = () |
| def is_empty(v: str | None) -> None: | ||
| v is None or (hasattr(v, "__len__") and len(v) == 0) |
There was a problem hiding this comment.
This was definitely intended as a conditional, but it never returns
| def is_empty(v: str | None) -> None: | |
| v is None or (hasattr(v, "__len__") and len(v) == 0) | |
| def is_empty(v: str | None) -> bool: | |
| return v is None or (hasattr(v, "__len__") and len(v) == 0) |
| def __iter__(self) -> Generator[Node, None, None]: | ||
| return | ||
| yield |
There was a problem hiding this comment.
Not a typing warning/error per se, but this yield is considered unreachable; a way to handle this without making the IDE yell would be
| def __iter__(self) -> Generator[Node, None, None]: | |
| return | |
| yield | |
| def __iter__(self) -> Generator[Node, None, None]: | |
| yield from () |
| from collections.abc import Callable, Generator | ||
| from typing import Any, ClassVar, IO, TYPE_CHECKING | ||
|
|
||
| if TYPE_CHECKING: |
There was a problem hiding this comment.
Is it necessary to use this check?
In other parts of pycparser from typing .... imports are done unguarded
There was a problem hiding this comment.
You can do it unguarded, but it'll cause circular imports down the road. If I tried this without the conditional, from .c_parser import Coord would fail for that reason.
Given you're only doing those imports for type hints in pycparser, they should probably be behind a TYPE_CHECKING conditional too, but that's outside the scope of this PR
| from .c_parser import Coord | ||
|
|
||
| def _repr(obj): | ||
| def _repr(obj: Any | None) -> str: |
There was a problem hiding this comment.
What benefit does "Any | None" provide?
There was a problem hiding this comment.
Making the typing explicit; that's about it.
There was a problem hiding this comment.
Doesn't Any already subsume the | None part, making it obsolete?
|
|
||
| class Node: | ||
| __slots__ = () | ||
| __slots__: ClassVar[tuple[str, ...]] = () |
There was a problem hiding this comment.
is it really necessary to annotate __slots__? what check fails if we don't?
There was a problem hiding this comment.
mypy gives an error in __repr__ when trying to iterate over self.__slots__[:-2]:
Unsupported left operand type for + ("Never") Mypy[operator]
Without the hint, it resolves __slots__ as a tuple with no entries. The hint makes it see a tuple with an indeterminite amount of entries, so it will no longer complain
|
Rebased with the |
|
I fixed the missed return you've mentioned, but otherwise I don't feel this is worthwhile at this time. I personally only prefer using |
|
Fair enough. If I do any future passes, it'll be via |
The typing logic for this was derived exclusively from
_c_ast.cfg; as such, it's quite rudamentary. Anything more complex would necessitate expanding the configuation with explicit typing data, or making a whitelist for "expected" types. Part of the type validation process meant updating certain semantics or syntax, as well as exposing a few bugs that've since been resolved.