Skip to content

Fix reachability in nested match sequence patterns #19993

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+32 −2
Open

Fix reachability in nested match sequence patterns #19993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion mypy/checkpattern.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,21 @@ def visit_sequence_pattern(self, o: SequencePattern) -> PatternType:
)
)
narrowed_inner_types.append(narrowed_inner_type)
inner_rest_types.append(inner_rest_type)
narrowed_ptype = get_proper_type(narrowed_inner_type)
if (
is_uninhabited(inner_rest_type)
and isinstance(narrowed_ptype, Instance)
and (
narrowed_ptype.type.fullname == "builtins.dict"
or narrowed_ptype.type.fullname == "builtins.list"
)
):
# Can't narrow rest type to uninhabited
# if narrowed_type is dict or list.
# Those can be matched by Mapping or Sequence patterns.
Copy link
Collaborator

@A5rocks A5rocks Oct 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I... don't really follow. I don't know anything about the pattern matching checker, so I'm just going off inference/comments elsewhere, but isn't it right to say the rest is uninhabited? If I were to guess the problem is more with the | rather than the (_, {...})?

(sorry, you'll probably just have to explain why this is right when this comment does that. I just don't get it...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The rest type is calculated from the intersection of the subject inner type and the matched inner type. Let's consider an example. Say the subject type is int | str and the matched type is int, the rest type will then be str.

For dict and list this is a bit more complicated as those are match patterns itself. So if the subject is {"a": 1, "b": 2} inferred to dict[str, int] and the case {"a": 2} the pattern would not actually match. However the inferred matched type is still dict[str, int] and the intersection would be Never which isn't correct. A similar issue happens with list. So we have to ignore the intersection rest type for Mapping and Sequence sub-patterns for those.

--
I'm not sure if it might even affect other generic classes as well. So far though I haven't been able to trigger the issue with any other examples so I'd leave it at these two for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK thanks that makes more sense. In that case I'm surprised about the check that rest is uninhabited! But more importantly, it seems like we already have machinery already doing this? (or did I misunderstand your explanation):

x: dict[str, int] = {"b": 0}

match x:
    case {"a": 5}:
        pass
    case b:
        reveal_type(b)  # N: Revealed type is "builtins.dict[builtins.str, builtins.int]"

Copy link
Collaborator Author

@cdce8p cdce8p Oct 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

But more importantly, it seems like we already have machinery already doing this? [...]

x: dict[str, int] = {"b": 0}

match x:
    case {"a": 5}:
        pass
    case b:
        reveal_type(b)  # N: Revealed type is "builtins.dict[builtins.str, builtins.int]"

The issue only happens for Mapping or Sequence patterns inside a Sequence pattern. Furthermore you'll need a wildcard match. With that mypy currently thinks "oh it's a wildcard so it always matches, thus the rest should be never". That's only true though if the parent sequence pattern itself matches, so we shouldn't infer rest in those cases.

Copy link
Collaborator

@A5rocks A5rocks Oct 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I see. Is there a reason why this doesn't narrow the 2nd tuple element to Never?: (in comparison to if x is a dict, which I can see now and is I think what you are talking about)

x: str = "blah"

match (x, 4):
    case ("a", _):
        pass
    case b:
        reveal_type(b)  # N: Revealed type is "tuple[builtins.str, Literal[4]?]"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The rest type for x is inferred as str whereas with the dict example it's inferred as Never since technically both the subject and inner match types are identical dict[str, str].

m7: dict[str, str]

match (m7, m7):
    case ({"a": "1"}, _):
        ...
    case (_, {"a": "2"}):
        ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I forgot to respond. I'm concerned that the logic here is incomplete, does this PR fix this too?: (I'm specifically concerned about the comparison of rest to Never.)

x: dict[str, str] | str = {"blah": "blah"}

match (x, 4):
    case ({"b": "a"}, _):
        pass
    case b:
        reveal_type(b)  # N: Revealed type is "tuple[builtins.dict[builtins.str, builtins.str] | builtins.str, Never]"

Do you think it would be possible to instead have a marker that actually, if we specify a literal dict literal, it might not actually match? (unless it's empty) Otherwise maybe this is the best we can get...

inner_rest_types.append(narrowed_inner_type)
else:
inner_rest_types.append(inner_rest_type)
if all(not is_uninhabited(typ) for typ in narrowed_inner_types):
new_type = TupleType(narrowed_inner_types, current_type.partial_fallback)
else:
Expand Down
18 changes: 17 additions & 1 deletion test-data/unit/check-python310.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1525,6 +1525,7 @@ def f(value: Literal[1] | Literal[2]) -> int:
[typing fixtures/typing-medium.pyi]

[case testMatchSequencePatternNegativeNarrowing]
# flags: --warn-unreachable
from typing import Literal, Union, Sequence, Tuple

m1: Sequence[int | str]
Expand Down Expand Up @@ -1577,7 +1578,22 @@ match m6:
case _:
reveal_type(m6) # N: Revealed type is "tuple[Union[Literal[1], Literal[2]], Union[Literal['a'], Literal['b']]]"

[builtins fixtures/tuple.pyi]
m7: dict[str, str]

match (m7, m7):
case ({"a": "1"}, _):
reveal_type(m7) # N: Revealed type is "builtins.dict[builtins.str, builtins.str]"
case (_, {"a": "2"}):
reveal_type(m7) # N: Revealed type is "builtins.dict[builtins.str, builtins.str]"

m8: list[int]

match (m8, m8):
case ([1], _):
reveal_type(m8) # N: Revealed type is "builtins.list[builtins.int]"
case (_, [2]):
reveal_type(m8) # N: Revealed type is "builtins.list[builtins.int]"
[builtins fixtures/dict.pyi]

[case testMatchEnumSingleChoice]
from enum import Enum
Expand Down