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

expand the possible inputs to reversed #13269

Closed
wants to merge 2 commits into from
Closed

Conversation

tungol
Copy link
Contributor

@tungol tungol commented Dec 19, 2024

A bit experimental.

related to #13218 and #11645

@tungol tungol marked this pull request as draft December 19, 2024 10:18
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

spark (https://github.com/apache/spark)
+ python/pyspark/pandas/base.py:1663: error: Unused "type: ignore" comment  [unused-ignore]

pytest (https://github.com/pytest-dev/pytest)
+ src/_pytest/monkeypatch.py:402: error: Unused "type: ignore" comment  [unused-ignore]
+ src/_pytest/monkeypatch.py:407: error: Unused "type: ignore" comment  [unused-ignore]

@tungol
Copy link
Contributor Author

tungol commented Dec 19, 2024

I don't think this really does what we'd want because mypy doesn't handle __new__ methods that return something other than a subclass of the type, and the new branch of the overload returns reversed[T] as far as mypy is concerned.

It's a tough spot to be in. We can expand what reversed will accept in it's constructor, but without mypy support it's only going to result in reversed[T]. If the actual return type is too divergent from that, that's an issue.

I'll check out the mypy-primer hits though to see what's going on and if they're actually being cleared in a reasonable way.

Related: python/mypy#15182

@tungol
Copy link
Contributor Author

tungol commented Dec 19, 2024

That said this does work as expected in pyright.

@@ -1730,11 +1730,16 @@ def pow(base: _SupportsSomeKindOfPow, exp: complex, mod: None = None) -> complex

quit: _sitebuiltins.Quitter

class _SupportsReversed(Protocol[_T_co]):
def __reversed__(self) -> _T_co: ...

class reversed(Generic[_T]):
@overload
def __new__(cls, sequence: Reversible[_T], /) -> Iterator[_T]: ... # type: ignore[misc]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we even need this overload. The second overload should include this one.

@srittau
Copy link
Collaborator

srittau commented Dec 19, 2024

We could also use some test cases for this.

@tungol
Copy link
Contributor Author

tungol commented Dec 19, 2024

I don't think we can do this without mypy support, whether that's python/mypy#15182 or special-casing for reversed.

For example, it's true that we don't need the first overload, but without that, and with the current state of mypy, we would get reversed(list[int]) -> reversed[list[int]] from mypy. Right now, the unpacking in __new__(cls, sequence: Reversible[_T], /) -> Iterator[_T] means we get reversed[int] from mypy.

We could, with more safety, expand this to accept __reversed__ -> Iterable[T], but it's not ideal because mypy will still incorrectly infer reversed[T], which is a full iterator.

So I don't really think it's worth worrying about test cases; this is busted and we shouldn't do it right now.

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

Successfully merging this pull request may close these issues.

2 participants