Fix typing of reversed

[plokmijnuhby]

Reversed has a __new__ method, not an __init__ method. Note that it does not normally actually return an instance of the reversed class.

[plokmijnuhby]

Huh, mypy thinks __new__ should return an instance of reversed anyway? But, it doesn't though?

[Daverball]

__new__ must return a subclass in a typing context, ignoring the error won't change that, mypy will still treat it as a reversed object, so the proper way to do this would be to make reversed a function and have it return the Iterator protocol. But reversed is actually a class in CPython, it just has a weird __new__ function, that can return something other than a reversed instance if the object being passed in implements __reversed__.

So there's no completely type safe to annotate reversed. I think the current way it's annotated is fine. While going the function + Protocol route would be a little bit more type safe in some situations, it also takes away the ability to check for actual reversed instances, which are a thing that does exist.

[plokmijnuhby]

Yes, I see that. I would argue that, if that's the case, this PR is still (marginally) the best way of doing things. In the very rare case where a user calls reversed.__new__ directly, the return type given here is a better fit, and in all other cases the return type is ignored.
While the return type could theoretically be anything, we've specified that the input sequence needs to be an instance of Reversible (ie, with __reversed__ returning an Iterator), a SupportsLenAndGetItem (so the return type will be actually an instance of reversed), or, most likely, both (in which case, we get an Iterator again). So Iterator is always correct given the assumptions we've already made.

[Daverball]

I don't think we should encourage patterns in typeshed that are not considered valid by type checkers, even if that's what the implementation does under the hood.

In this case the lie the type checker tells is rather harmless, since reversed pretty much only provides the minimal Iterator interface anyways. The only bad case is manually calling __length_hint__, since that might not exist.

So I'm personally not convinced that the change is a positive one, but the maintainers of typeshed will have to decide whether it is or not.

BTW minor nit: You used self instead of cls in the annotation of __new__.

[Viicos]

Note this might be working with python/mypy#16020

[JelleZijlstra]

What concrete problem does this solve, in terms of code examples that will typecheck better with this change?

Given that mypy doesn't seem to support this pattern for now, I think we should add test cases to make sure this doesn't regress type checking with mypy (e.g., make sure it doesn't start inferring Any).

[plokmijnuhby]

The main reason this is relevant is if you have a subclass of reversed (which is rare but allowed). An example might look something like this:

class Foo(reversed):
    def __new__(cls, arg):
        res = super().__new__(cls, arg) # shouldn't raise a type error, but currently does
        print(res.__length_hint__()) # should raise a type error, but currently doesn't
        return res

[Akuli]

Annotating something with reversed, as in foo: reversed[int], is very unlikely to do the right thing. For example, if you assign reversed([1, 2, 3]) to a reversed[int], the type checker is happy but inconsistent with runtime:

>>> type(reversed([1, 2, 3]))
<class 'list_reverseiterator'>
>>> isinstance(reversed([1, 2, 3]), reversed)
False

Maybe we should pretend that reversed is a function, as in def reversed(stuff) -> Iterator[_T]? This would give you type checker errors for foo: reversed[int]. You would also get an error for if isinstance(foo, reversed), which IMO would be good because it produces confusing results.

[plokmijnuhby]

The thing is, I think it is was the intention of the cpython developers that __new__ methods could return an object of literally any type. The most obvious evidence for this is that reversed does it, but for more proof look at the documentation for object.__new__ (emphasis added):

Called to create a new instance of class cls. __new__ is a static method (special-cased so you need not declare it as such) that takes the class of which an instance was requested as its first argument. The remaining arguments are those passed to the object constructor expression (the call to the class). The return value of __new__() should be the new object instance (usually an instance of cls). ... If __new__() does not return an instance of cls, then the new instance’s __init__() method will not be invoked.

This seems fairly clearly to allow for this sort of behaviour. If mypy doesn't like it, then mypy is just wrong.

[JelleZijlstra]

If mypy doesn't like it, then mypy is just wrong.

True, but mypy is among the most significant ways people use typeshed, and we'd be doing our end users a disservice if we break type checking reversed() with mypy. That's especially true because the motivation for change here feels very thin: why would you want to subclass reversed?

I'd prefer to have stubs that correctly reflect reality, so we do need a change like the one in this PR, but we should have test cases that demonstrate that this change doesn't cause new problems. Test cases should look something like this:

x: list[int]
assert_type(list(reversed(x)), list[int])

To make sure that we don't get a list[Any] or similar.

[JelleZijlstra]

Thanks, and sorry for the wait!

[github-actions]

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉