Introduce Y045: Ban returning (Async)Iterable from __(a)iter__ methods

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
 

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:

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}'

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