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.

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

Shouldn't the return type of __reversed__ be an Iterable[T] instead of Iterator[T]? #13218

Open
bluenote10 opened this issue Dec 7, 2024 · 5 comments

Comments

@bluenote10
Copy link

bluenote10 commented Dec 7, 2024

Currently the return type of Reversible.__reversed__ is typed as Iterator[T], which suggests that the returned instance must have both an __next__ and an __iter__ method.

typeshed/stdlib/typing.pyi

Lines 446 to 448 in 0e9c9e1

class Reversible(Iterable[_T_co], Protocol[_T_co]):
@abstractmethod
def __reversed__(self) -> Iterator[_T_co]: ...

However, the following code seems to work fine at runtime:

from dataclasses import dataclass
from typing import Iterator, Iterable

@dataclass
class MyIter:
    _values: list[int]
    
    def __iter__(self) -> Iterator[int]:
        return iter(self._values)
    
    def __reversed__(self) -> Iterator[int]:
        return MyIter(self._values[::-1])

my_iter = MyIter([1, 2, 3])
for x in my_iter:
    print(x)
for x in reversed(my_iter):
    print(x)

Note that __reversed__ here returns a MyIter instance, which has an __iter__ method, but no __next__, i.e., it is an Iterable but not an Iterator. The Python interpreter seems to deal with that fine at runtime, i.e., it doesn't actually seem to need the __next__ method. This is slightly surprising, because the docs specify:

It should return a new iterator object that iterates over all the objects in the container in reverse order.

I.e., it doesn't use the word "iterable object".

Unfortunately, the current signature of __reversed__ means that this example does not type check: Obviously the type checker has to complain about the return MyIter(...) line, because MyIter is indeed only an Iterable (example on mypy playground):

main.py:13: error: Incompatible return value type (got "MyIter", expected "Iterator[int]")  [return-value]
main.py:13: note: "MyIter" is missing following "Iterator" protocol member:
main.py:13: note:     __next__
Found 1 error in 1 file (checked 1 source file)

Now I'm wondering if the signature should actually be def __reversed__(self) -> Iterable[int] to lessen the requirement and match the runtime behavior?

Note that simply changing the return type to Iterable on user side means that the return statement now type checks, but then all usages (reversed(my_iter)) stop to type check because the type checker will no longer consider MyIter as a valid Reversible (modified example on mypy playground):

main.py:18: error: No overload variant of "reversed" matches argument type "MyIter"  [call-overload]
main.py:18: note: Possible overload variants:
main.py:18: note:     def [_T] __new__(cls, Reversible[_T], /) -> reversed[_T]
main.py:18: note:     def [_T] __new__(cls, SupportsLenAndGetItem[_T], /) -> reversed[_T]
Found 1 error in 1 file (checked 1 source file)
@tungol
Copy link
Contributor

tungol commented Dec 19, 2024

I don't find it overly surprising if you look at the range of things that iter can accept versus what the specific thing that the abc "Iterable" means. It'd likely make sense to expand what reversed can accept. I wonder if it can take a getitem-iterable too.

@tungol
Copy link
Contributor

tungol commented Dec 19, 2024

It can; this also works without complaint:

class GetItemIter:
    def __getitem__(self, i: int, /):
        return "abc".__getitem__(i)

    def __reversed__(self):
        return self

a = GetItemIter()

for i in iter(a):
    print(i)

for i in reversed(a):
    print(i)

(Ignoring the fact that no actual reversal takes place.)

@TeamSpen210
Copy link
Contributor

The reason your code works is that for loops always do the equivalent of iter(), so your iterable gets converted. You'd want to do next(reversed(...)) to test the behaviour, which in CPython [just returns the result of the method call with no checks](https://github.com/python/cpython/blob/48c70b8f7dfd00a018abbac50ea987f54fa4db51/Objects/enumobject.c#L369-373].

I think the reason it's defined as returning an iterator is that generally you can't reverse a reverse iterator again, and also the default implementation (which uses just __len__ and __getitem__) is itself only an iterator. I don't think widening the return type is right, it'd make more sense to create a private protocol with two typevars, so the value can be inferred from __reversed__'s return type. But generally type hints don't distinguish between iterator classes, and the docs don't promise that you can return arbitrary classes here.

@tungol
Copy link
Contributor

tungol commented Dec 19, 2024

Previous related discussion about the behavior of reversed: #11645

It seems like there's two modes: If there's a __reversed__, then reversed just returns whatever the result of that method is. So anything that's iterable (via any of the iter protocols) will work as expected, and you can also return non-iterable things this way if you're feeling perverse. Otherwise, it goes to the SupportsLenAndGetItem branch and returns an actual instance of reversed.

@srittau
Copy link
Collaborator

srittau commented Dec 19, 2024

Per discussion: The reversed accepts more than just Reversible arguments. The protocol itself is still defined correctly. One change I could see is altering the definition of reversed.__new__'s first overload to accept a custom protocol, which doesn't rely on __reversed__ returning an iterator. An exploratory PR would be welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants