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

Fix missing meet case exposed by len narrowing #16470

Merged
merged 1 commit into from
Nov 12, 2023

Conversation

ilevkivskyi
Copy link
Member

Fixes #16468

The fix is straightforward.

Btw when fixing this I noticed that we disregard type arguments when narrowing, for example:

x: Sequence[int]
if isinstance(x, tuple):
    reveal_type(x)  # tuple[Any, ...], but should be `tuple[int, ...]`

I guess fixing this may be tricky, and it is quite old behavior.

Copy link
Contributor

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

websockets (https://github.com/aaugustin/websockets)
+ src/websockets/legacy/auth.py:162: error: Argument 1 to "list" has incompatible type "tuple[str, str] | Iterable[tuple[str, str]]"; expected "Iterable[tuple[str, str]]"  [arg-type]

vision (https://github.com/pytorch/vision)
+ torchvision/transforms/v2/_geometry.py:254: error: Redundant cast to "tuple[float, float]"  [redundant-cast]
+ torchvision/transforms/v2/_geometry.py:257: error: Redundant cast to "tuple[float, float]"  [redundant-cast]

@ilevkivskyi
Copy link
Member Author

New error in websockets is kind of correct. The code should use a TypeGuard to prevent single tuple from getting into second elif (because tuple is also iterable).

and isinstance(x[0], int)
and isinstance(x[1], int)
):
reveal_type(x) # N: Revealed type is "Tuple[builtins.int, builtins.int]"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add else: reveal_type(x)? It might give some unexpected result.

Copy link
Member Author

Choose a reason for hiding this comment

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

The else branch reveals Union[builtins.int, typing.Sequence[builtins.int], builtins.tuple[Any, ...]] because of the behavior I mentioned in the description. I intentionally didn't add this, since I don't want to create an impression this is intentional.

@hauntsaninja hauntsaninja merged commit efa5dcb into python:master Nov 12, 2023
18 checks passed
@ilevkivskyi ilevkivskyi deleted the fix-len-narrowing branch November 12, 2023 20:37
@ilevkivskyi ilevkivskyi mentioned this pull request Nov 12, 2023
2 tasks
JukkaL pushed a commit that referenced this pull request Nov 22, 2023
Fixes #16468

The fix is straightforward.

Btw when fixing this I noticed that we disregard type arguments when
narrowing, for example:
```python
x: Sequence[int]
if isinstance(x, tuple):
    reveal_type(x)  # tuple[Any, ...], but should be `tuple[int, ...]`
```
I guess fixing this may be tricky, and it is quite old behavior.
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.

Invalid "Value of type Never is not indexable"
3 participants