From abaa912c274fb6a3bf218543c4f50eab66acfad4 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Sat, 23 Jul 2022 12:27:11 +0100 Subject: [PATCH] Introduce Y045: Ban returning `(Async)Iterable` from `__(a)iter__` --- CHANGELOG.md | 4 +++ README.md | 1 + pyi.py | 80 ++++++++++++++++++++++++++++++++++++--------- tests/classdefs.pyi | 12 ++++++- 4 files changed, 81 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 499a3f0f..4443ffa5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,10 @@ Features: is not yet released, but will be in the next version of `typing_extensions`.) * Introduce Y044: Discourage unnecessary `from __future__ import annotations` import. Contributed by Torsten Wörtwein. +* Introduce Y045: Ban returning `(Async)Iterable` from `__(a)iter__` methods. +* Slightly expand Y034 to cover the case where a class inheriting from `(Async)Iterator` + returns `(Async)Iterable` from `__(a)iter__`. These classes should nearly always return + `Self` from these methods. ## 22.5.1 diff --git a/README.md b/README.md index 6f6bb786..60204518 100644 --- a/README.md +++ b/README.md @@ -75,6 +75,7 @@ currently emitted: | Y041 | Y041 detects redundant numeric unions. For example, PEP 484 specifies that type checkers should treat `int` as an implicit subtype of `float`, so `int` is redundant in the union `int \| float`. In the same way, `int` is redundant in the union `int \| complex`, and `float` is redundant in the union `float \| complex`. | Y043 | Do not use names ending in "T" for private type aliases. (The "T" suffix implies that an object is a `TypeVar`.) | Y044 | `from __future__ import annotations` has no effect in stub files, as forward references in stubs are enabled by default. +| Y045 | `__iter__` methods should never return `Iterable[T]`, as they should always return some kind of iterator. Many error codes enforce modern conventions, and some cannot yet be used in all cases: diff --git a/pyi.py b/pyi.py index 3d9c54e9..358bc30d 100644 --- a/pyi.py +++ b/pyi.py @@ -309,19 +309,27 @@ def _is_object(node: ast.expr | None, name: str, *, from_: Container[str]) -> bo strings. >>> from functools import partial - >>> _is_Literal = partial(_is_object, name="Literal", from_=_TYPING_MODULES) - >>> _is_Literal(_ast_node_for("Literal")) + >>> _is_AsyncIterator = partial(_is_object, name="AsyncIterator", from_=_TYPING_MODULES | {"collections.abc"}) + >>> _is_AsyncIterator(_ast_node_for("AsyncIterator")) True - >>> _is_Literal(_ast_node_for("typing.Literal")) + >>> _is_AsyncIterator(_ast_node_for("typing.AsyncIterator")) True - >>> _is_Literal(_ast_node_for("typing_extensions.Literal")) + >>> _is_AsyncIterator(_ast_node_for("typing_extensions.AsyncIterator")) + True + >>> _is_AsyncIterator(_ast_node_for("collections.abc.AsyncIterator")) True """ - return _is_name(node, name) or ( - isinstance(node, ast.Attribute) - and node.attr == name - and isinstance(node.value, ast.Name) - and node.value.id in from_ + if _is_name(node, name): + return True + if not (isinstance(node, ast.Attribute) and node.attr == name): + return False + node_value = node.value + if isinstance(node_value, ast.Name): + return node_value.id in from_ + return ( + isinstance(node_value, ast.Attribute) + and isinstance(node_value.value, ast.Name) + and f"{node_value.value.id}.{node_value.attr}" in from_ ) @@ -338,6 +346,10 @@ def _is_object(node: ast.expr | None, name: str, *, from_: Container[str]) -> bo _is_Self = partial(_is_object, name="Self", from_=({"_typeshed"} | _TYPING_MODULES)) _is_TracebackType = partial(_is_object, name="TracebackType", from_={"types"}) _is_builtins_object = partial(_is_object, name="object", from_={"builtins"}) +_is_Iterable = partial(_is_object, name="Iterable", from_={"typing", "collections.abc"}) +_is_AsyncIterable = partial( + _is_object, name="AsyncIterable", from_={"collections.abc"} | _TYPING_MODULES +) def _get_name_of_class_if_from_modules( @@ -471,8 +483,6 @@ def _get_collections_abc_obj_id(node: ast.expr | None) -> str | None: ) -_ITER_METHODS = frozenset({("Iterator", "__iter__"), ("AsyncIterator", "__aiter__")}) - _INPLACE_BINOP_METHODS = frozenset( { "__iadd__", @@ -523,9 +533,16 @@ def _has_bad_hardcoded_returns( ) return_obj_name = _get_collections_abc_obj_id(returns) - return (return_obj_name, method_name) in _ITER_METHODS and any( - _get_collections_abc_obj_id(base_node) == return_obj_name - for base_node in classdef.bases + bases = {_get_collections_abc_obj_id(base_node) for base_node in classdef.bases} + + return ( + method_name == "__iter__" + and return_obj_name in {"Iterable", "Iterator"} + and "Iterator" in bases + ) or ( + method_name == "__aiter__" + and return_obj_name in {"AsyncIterable", "AsyncIterator"} + and "AsyncIterator" in bases ) @@ -1352,6 +1369,30 @@ def _Y034_error( ) self.error(node, error_message) + def _check_iter_returns( + self, node: ast.FunctionDef, returns: ast.expr | None + ) -> None: + if _is_Iterable(returns) or ( + isinstance(returns, ast.Subscript) and _is_Iterable(returns.value) + ): + msg = Y045.format( + iter_method="__iter__", good_cls="Iterator", bad_cls="Iterable" + ) + self.error(node, msg) + + def _check_aiter_returns( + self, node: ast.FunctionDef, returns: ast.expr | None + ) -> None: + if _is_AsyncIterable(returns) or ( + isinstance(returns, ast.Subscript) and _is_AsyncIterable(returns.value) + ): + msg = Y045.format( + iter_method="__aiter__", + good_cls="AsyncIterator", + bad_cls="AsyncIterable", + ) + self.error(node, msg) + def _visit_synchronous_method(self, node: ast.FunctionDef) -> None: method_name = node.name all_args = node.args @@ -1361,6 +1402,14 @@ def _visit_synchronous_method(self, node: ast.FunctionDef) -> None: if _has_bad_hardcoded_returns(node, classdef=classdef): return self._Y034_error(node=node, cls_name=classdef.name) + returns = node.returns + + if method_name == "__iter__": + return self._check_iter_returns(node, returns) + + if method_name == "__aiter__": + return self._check_aiter_returns(node, returns) + if method_name in {"__exit__", "__aexit__"}: return self._check_exit_method(node=node, method_name=method_name) @@ -1375,7 +1424,7 @@ def _visit_synchronous_method(self, node: ast.FunctionDef) -> None: if method_name in {"__repr__", "__str__"}: if ( len(non_kw_only_args) == 1 - and _is_object(node.returns, "str", from_={"builtins"}) + and _is_object(returns, "str", from_={"builtins"}) and not any(_is_abstractmethod(deco) for deco in node.decorator_list) ): self.error(node, Y029) @@ -1674,3 +1723,4 @@ def parse_options( Y041 = 'Y041 Use "{implicit_supertype}" instead of "{implicit_subtype} | {implicit_supertype}" (see "The numeric tower" in PEP 484)' Y043 = 'Y043 Bad name for a type alias (the "T" suffix implies a TypeVar)' Y044 = 'Y044 "from __future__ import annotations" has no effect in stub files.' +Y045 = 'Y045 "{iter_method}" methods should return an {good_cls}, not an {bad_cls}' diff --git a/tests/classdefs.pyi b/tests/classdefs.pyi index 4c075720..0e498a74 100644 --- a/tests/classdefs.pyi +++ b/tests/classdefs.pyi @@ -5,7 +5,7 @@ import builtins import collections.abc import typing from abc import abstractmethod -from collections.abc import AsyncIterator, Iterator +from collections.abc import AsyncIterable, AsyncIterator, Iterable, Iterator from typing import Any, overload import typing_extensions @@ -87,9 +87,19 @@ class BadIterator2(typing.Iterator[int]): # Y027 Use "collections.abc.Iterator[ class BadIterator3(typing.Iterator[int]): # Y027 Use "collections.abc.Iterator[T]" instead of "typing.Iterator[T]" (PEP 585 syntax) def __iter__(self) -> collections.abc.Iterator[int]: ... # Y034 "__iter__" methods in classes like "BadIterator3" usually return "self" at runtime. Consider using "_typeshed.Self" in "BadIterator3.__iter__", e.g. "def __iter__(self: Self) -> Self: ..." +class BadIterator4(Iterator[int]): + # Note: *Iterable*, not *Iterator*, returned! + def __iter__(self) -> Iterable[int]: ... # Y034 "__iter__" methods in classes like "BadIterator4" usually return "self" at runtime. Consider using "_typeshed.Self" in "BadIterator4.__iter__", e.g. "def __iter__(self: Self) -> Self: ..." + +class IteratorReturningIterable: + def __iter__(self) -> Iterable[str]: ... # Y045 "__iter__" methods should return an Iterator, not an Iterable + class BadAsyncIterator(collections.abc.AsyncIterator[str]): def __aiter__(self) -> typing.AsyncIterator[str]: ... # Y034 "__aiter__" methods in classes like "BadAsyncIterator" usually return "self" at runtime. Consider using "_typeshed.Self" in "BadAsyncIterator.__aiter__", e.g. "def __aiter__(self: Self) -> Self: ..." # Y027 Use "collections.abc.AsyncIterator[T]" instead of "typing.AsyncIterator[T]" (PEP 585 syntax) +class AsyncIteratorReturningAsyncIterable: + def __aiter__(self) -> AsyncIterable[str]: ... # Y045 "__aiter__" methods should return an AsyncIterator, not an AsyncIterable + class Abstract(Iterator[str]): @abstractmethod def __iter__(self) -> Iterator[str]: ...