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 negative narrowing of tuples in match statement #17817

Conversation

brianschubert
Copy link
Collaborator

Fixes #17328

Before

Lines marked with !!! denote incorrect behavior. (Playground link)

from typing import Literal

m4: tuple[Literal[1], int]
match m4:
    case (1, 5):
        reveal_type(m4)  # N: Revealed type is "tuple[Literal[1], Literal[5]]"
    case (1, 6):
        reveal_type(m4)  # !!! E: Statement is unreachable  [unreachable]
    case _:
        reveal_type(m4)  # !!! N: Revealed type is "tuple[Never, builtins.int]"

m5: tuple[Literal[1, 2], Literal["a", "b"]]

match m5:
    case (1, str()):
        reveal_type(m5)  # N: Revealed type is "tuple[Literal[1], Union[Literal['a'], Literal['b']]]"
    case _:
        reveal_type(m5)  # !!! N: Revealed type is "tuple[Literal[2], Never]"

match m5:
    case (1, "a"):
        reveal_type(m5)  # N: Revealed type is "tuple[Literal[1], Literal['a']]"
    case _:
        reveal_type(m5)  # !!! N: Revealed type is "tuple[Literal[2], Literal['b']]"

After

from typing import Literal

m4: tuple[Literal[1], int]
match m4:
    case (1, 5):
        reveal_type(m4)  # N: Revealed type is "tuple[Literal[1], Literal[5]]"
    case (1, 6):
        reveal_type(m4)  # N: Revealed type is "tuple[Literal[1], Literal[6]]"
    case _:
        reveal_type(m4)  # N: Revealed type is "tuple[Literal[1], builtins.int]"

m5: tuple[Literal[1, 2], Literal["a", "b"]]

match m5:
    case (1, str()):
        reveal_type(m5)  # N: Revealed type is "tuple[Literal[1], Union[Literal['a'], Literal['b']]]"
    case _:
        reveal_type(m5)  # N: Revealed type is "tuple[Literal[2], Union[Literal['a'], Literal['b']]]"

match m5:
    case (1, "a"):
        reveal_type(m5)  # N: Revealed type is "tuple[Literal[1], Literal['a']]"
    case _:
        reveal_type(m5)  # N: Revealed type is "tuple[Union[Literal[1], Literal[2]], Union[Literal['a'], Literal['b']]]"

@brianschubert
Copy link
Collaborator Author

Details

For the sake of explanation, consider the setup from the linked issue:

x: tuple[Literal[1], int]
match x:
    case (1, 5):
        ...

The root cause of this issue comes from this snippet:

mypy/mypy/checkpattern.py

Lines 307 to 314 in 9ffb9dd

for inner_type, new_inner_type in zip(inner_types, new_inner_types):
(narrowed_inner_type, inner_rest_type) = (
self.chk.conditional_types_with_intersection(
new_inner_type, [get_type_range(inner_type)], o, default=new_inner_type
)
)
narrowed_inner_types.append(narrowed_inner_type)
inner_rest_types.append(inner_rest_type)

This loop iterates of the current types of the tuple positions (inner_types = [Literal[1], int]) and the proposed new types from the match subpatterns (new_inner_tuples = [Literal[1], Literal[5]]). For each tuple position, it computes the positive-narrowed type if the subpattern matches (narrowed_inner_type) and the negative-narrowed type if the subpattern fails to match (inner_rest_type).

The problem appears to be that the arguments being passed to conditional_types_with_intersection are flipped. Instead of asking the question "what happens when I narrow the old type to the new type?", it's asking the question "what happens when I narrow the new type to the old type?". As a specific example, consider the second iteration where inner_type=int and new_inner_type=Literal[5]. What this check should do is decide that narrowing int to Literal[5] produces a positively-narrowed type of Literal[5] and the negatively-narrowed type of int. Instead, it considers narrowing Literal[5] to int, and decides that the negatively-narrowed type uninhabited/Never (bad).

In the setup above, this causes mypy to deduce that the pattern (1, 5) unconditionally matches tuple[Literal[1], int] (bad), which leads subsequent patterns being considered unreachable (bad).

The fix is to swap the arguments, so that it correctly considers the case of narrowing old-type to new-type. This fixes the linked issue.

However, only applying the fix above leads to a test regression; specifically, this test case will fail:

m2: Tuple[int | str]
match m2:
case (int(),):
reveal_type(m2) # N: Revealed type is "Tuple[builtins.int]"
case r2:
reveal_type(m2) # N: Revealed type is "Tuple[builtins.str]"

To my understanding, this test is included erroneously, since this sort of narrowing is not technically implemented (yet). Currently, mypy only applies negative narrowing if all subpatterns unconditionally match:

mypy/mypy/checkpattern.py

Lines 320 to 322 in 9ffb9dd

if all(is_uninhabited(typ) for typ in inner_rest_types):
# All subpatterns always match, so we can apply negative narrowing
rest_type = TupleType(rest_inner_types, current_type.partial_fallback)

Matching int | str against int is not an unconditional match (the correct rest type is str). However, due to the issue above, the rest type was computed as uninhabited/Never and this pattern was considered to be an unconditional match; hence negative narrowing was inappropriately (but ultimately accidentally correctly) applied.

This can be behavior can be preserved by relaxing the negative-narrowing criteria from all subpatterns match unconditionally to at most one subpattern may match conditionally, and the rest match unconditionally. We can then safely negatively-narrow the single position that conditionally matches. This behavior is demonstrated in the before/after example above with the match statements using m5.

Open Issues

Negative narrowing of variadic tuples also has a similar issue:

from typing import Literal, Unpack

m: tuple[int | str, Unpack[tuple[str, ...]]]
match m:
    case (int(), *_):
        reveal_type(m)  # N: Revealed type is "tuple[builtins.int, Unpack[builtins.tuple[builtins.str, ...]]]"
    case _:
        reveal_type(m)  # E: Statement is unreachable

However, the fix is more complicated, so I'm leaving it for a separate PR.

@brianschubert
Copy link
Collaborator Author

CC @freundTech as original author in #10191.

Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Great, thank you again for another helpful writeup!

@hauntsaninja hauntsaninja merged commit 1c01858 into python:master Sep 27, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive: unreachable case when using Literal
2 participants