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.

Sign up for GitHub

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

Jump to bottom

incorrect annotations for builtins.reversed #11645

Open
jorenham opened this issue Mar 21, 2024 · 6 comments · May be fixed by #11646
Open

incorrect annotations for builtins.reversed #11645

jorenham opened this issue Mar 21, 2024 · 6 comments · May be fixed by #11646

Comments

@jorenham
Copy link

jorenham commented Mar 21, 2024 •
edited

The builtins.reversed.__new__ signature is currently annotated like (Reversible[T] | SupportsLenAndGetItem[T]) -> Iterator[T]:

typeshed/stdlib/builtins.pyi

Lines 1639 to 1646 in 3d26992

class reversed(Iterator[_T]):
@overload
def __new__(cls, sequence: Reversible[_T], /) -> Iterator[_T]: ... # type: ignore[misc]
@overload
def __new__(cls, sequence: SupportsLenAndGetItem[_T], /) -> Iterator[_T]: ... # type: ignore[misc]
def __iter__(self) -> Self: ...
def __next__(self) -> _T: ...
def __length_hint__(self) -> int: ...

It has two problems:

argument of type SupportsLenAndGetItem[T]

This should return a reversed[T] instance (i.e. Self), instead of "just" an Iterable[T], which is unnecessarily loose.

argument with type that implement __reversed__

This is only handled if the argument's __reversed__ returns an Iterator[T], i.e. arguments that are Reversible[T]. But in reality, reversed(x) is equivalent to x.__reversed__() (if __reversed__ exists). To illustrate:

>>> class Revver[X]:
...     x: X
...     def __init__(self, x: X):
...         self.x = x
...     def __reversed__(self) -> X:
...         return self.x
... 
>>> reversed(Revver(42))
42

However, this case isn't handled because Revver[X] cannot be expressed as a Reversible[?].

So instead, it should look something like this:

class SupportsReversed[T]:
    def __reversed__(self) -> T: ...

class reversed[T](Iterator[T]):
    @overload
    def __new__[R](cls, sequence: SupportsReversed[R], /) -> R: ...
    @overload
    def __new__(cls, sequence: SupportsLenAndGetItem[T], /) -> Self: ...
    def __length_hint__(self) -> int: ...
    # btw, this is why protocols shouldn't be mixed with abstract classes:
    def __iter__(self) -> Self: ...
    def __next__(self) -> T: ...

Let me know if I should make this into PR :)
@jorenham jorenham mentioned this issue Mar 21, 2024
Merged
@JelleZijlstra
Copy link
Member

This looks right; feel free to send a PR. We previously dealt with reversed() in #10655. It's probably worth adding your examples to the test cases.

This was referenced Mar 21, 2024
Open
Open
Open
@Akuli
Copy link
Collaborator

Akuli commented Mar 25, 2024

Do you have real-world use cases for these changes, or is this just correctness for the sake of correctness? From a practical point of view, it makes sense for type checkers to conmplain about returning an integer in __reversed__, and as far as I can tell, a reversed instance doesn't have anything important that Iterator[_T] lacks.

@jorenham
Copy link
Author

@Akuli I was trying to implement await reversed(...) for "async sequences", which aren't (sync) iterators.

@jorenham
Copy link
Author

jorenham commented Mar 25, 2024 •
edited

@Akuli I also stumbled upon this issue whilst writing the callback protocols in optype; an opinionated low-level typing library that I've been working on. To be precise, I encountered it in optype.DoesReversed, which is the callable type of reversed (aliased as optype.do_reversed for the sake of completeness).

@jorenham
Copy link
Author

@Akuli I just thought of another "cool" example: using reversed on scipy.stats distribution instances (i.e. RV's) to "flip" them, so that e.g. reversed(scipy.stats.gumbel_r()) returns the equivalent of scipy.stats.gumbel_l().

I can imagine that using reversed this way (i.e. reversing the domain of a mathematical-isch function) could also be applyied within e.g. numpy.polynomial, sympy, pymc, etc...

@Akuli
Copy link
Collaborator

Akuli commented Mar 26, 2024

Alright, this makes more sense to me now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix annotations for builtins.reversed.__new__ jorenham/typeshed
3 participants
@JelleZijlstra @jorenham @Akuli