FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[match-case] Fix narrowing of class pattern with union-argument. by randolf-scholz · Pull Request #19473 · python/mypy · GitHub

Repository navigation

[match-case] Fix narrowing of class pattern with union-argument. - #19473

Closed
randolf-scholz wants to merge 3 commits into
python:masterfrom
randolf-scholz:fix_match_case_union_argument
Closed

randolf-scholz wants to merge 3 commits into
python:masterfrom
randolf-scholz:fix_match_case_union_argument

Conversation

randolf-scholz commented Jul 17, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #19468

I added factorization over UnionType inside the conditional_types function.
Then try_expanding_sum_type_to_union needed to move to the start of the function so that we expand current_type before attempting factorizating the union type.

I also reformatted the docstring for better legibility.

New Tests

randolf-scholz changed the title inital commit [match-case] Fix narrowing of class pattern with union-argument. Jul 17, 2025
Comment thread mypy/checker.py
current_type = try_expanding_sum_type_to_union(current_type, enum_name)

current_type = get_proper_type(current_type)
if isinstance(current_type, UnionType) and (default == current_type):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The default == current_type was necessary, otherwise quite a few tests break.
Adding or (default is None) and passing default=None in the recursive call also seems to break stuff. Not entirely sure what's going on there.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If I'm not mistaken, this only works by coincidence and is incorrect in general case.

The problem is that you try to union and restrict multiple results here. This is only reasonable when none of the ranges are upper bounds, otherwise this is unsound:

class Base: pass

class A1(Base): ...
class A2(Base): ...
class A3(Base): ...

ref: type[Base] = A1
x: Base | int

if isinstance(x, ref):
    reveal_type(x)  # Should be Base
else:
    reveal_type(x)  # Should be Base | int (e.g. `x = A2()`)

Applying the logic from this PR, you get Never | Base == Base as yes_type (correct), but then you get (Base | int) \ Base == int as no_type, which is wrong. Since this has nothing to do with default, likely this piece just happens to work on our testcases?

I can suggest only performing this expansion when not any(tr.is_upper_bound for tr in proposed_type_ranges), but maybe there's a better general solution similar to the logic below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I ran your test example and it gives the expected results.

$ mypy tmp.py 
tmp.py:11: note: Revealed type is "tmp.Base"
tmp.py:13: note: Revealed type is "tmp.Base | builtins.int"

I believe it depends on from where this is called. conditional_types is called in a few places:

  • refine_identity_comparison_expression without a default
  • conditional_types_with_intersection with default, which itself is called in 7 places:
    1. TypeChecker.find_type_equals_check with current_type=self.lookup_type(expr) and default=None
    2. TypeChecker.find_isinstance_check_helper with current_type=self.lookup_type(expr) and default=None
    3. TypeChecker.infer_issubclass_maps with current_type=vartype and default=None
    4. PatternChecker.visit_as_pattern with current_type=current_typeanddefault=current_type`
    5. PatternChecker.visit_value_pattern with current_type=current_typeanddefault=get_proper_type(typ)`
    6. PatternChecker.visit_singleton_pattern with current_type=current_typeanddefault=current_type`
    7. PatternChecker.visit_sequence_pattern with current_type=inner_typeanddefault=inner_type`
    8. PatternChecker.visit_sequence_pattern with current_type=new_tuple_typeanddefault=new_tuple_type`
    9. PatternChecker.narrow_sequence_child with current_type=outer_typeanddefault=outer_type`
    10. PatternChecker.visist_class_pattern with current_type=current_typeanddefault=current_type`

So, we can see for the isinstance calls, default is None, so this branch is actually never taken currently, but may be taken for all the call-sites within PatternChecker except possibly PatternChecker.visit_value_pattern

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If I try to replicate your example with match-case we get these results:

class Base: pass

class A1(Base): ...
class A2(Base): ...
class A3(Base): ...

ref: type[Base] = A1
x: Base | int

match x:
    case ref() as y: # E: expected type in class pattern; found "type[tmp.Base]"
        reveal_type(y)  # E: Statement is unreachable
    case other:
        reveal_type(other)  # Base | int

match x:
    case Base() as y:
        reveal_type(y)  # Base
    case other:
        reveal_type(other)  # int

match x:
    case A1() as y:
        reveal_type(y)  # A1
    case other:
        reveal_type(other)  # Base | int

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@sterliakov I integrated your suggestion into a separate branch #19517, and was able to relax the condition to

    if (
        isinstance(current_type, UnionType)
        and not any(tr.is_upper_bound for tr in proposed_type_ranges)
        and (default in (current_type, None))
    ):

so that it also is applied with regular isinstance branches when default=None

This comment has been minimized.

This comment has been minimized.

Comment thread mypy/checker.py
The second element is the type it would hold if it was not the proposed type, if any.
UninhabitedType means unreachable.
None means no new information can be inferred.
If default is set it is returned instead.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Same question as in #19471, but this will be conflicted anyway:)

Comment thread mypy/checker.py
current_type = try_expanding_sum_type_to_union(current_type, enum_name)

current_type = get_proper_type(current_type)
if isinstance(current_type, UnionType) and (default == current_type):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If I'm not mistaken, this only works by coincidence and is incorrect in general case.

The problem is that you try to union and restrict multiple results here. This is only reasonable when none of the ranges are upper bounds, otherwise this is unsound:

class Base: pass

class A1(Base): ...
class A2(Base): ...
class A3(Base): ...

ref: type[Base] = A1
x: Base | int

if isinstance(x, ref):
    reveal_type(x)  # Should be Base
else:
    reveal_type(x)  # Should be Base | int (e.g. `x = A2()`)

Applying the logic from this PR, you get Never | Base == Base as yes_type (correct), but then you get (Base | int) \ Base == int as no_type, which is wrong. Since this has nothing to do with default, likely this piece just happens to work on our testcases?

I can suggest only performing this expansion when not any(tr.is_upper_bound for tr in proposed_type_ranges), but maybe there's a better general solution similar to the logic below.

Copy link
Copy Markdown
Contributor

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

randolf-scholz marked this pull request as draft July 27, 2025 19:34

Copy link
Copy Markdown
Contributor Author

Converting to draft in favor of sister PR #19517

Copy link
Copy Markdown
Contributor Author

@sterliakov Mind taking a look at #19517 ? It's a modification of this PR based on your comment #19473 (comment)

hauntsaninja pushed a commit that referenced this pull request Oct 23, 2025
)

Fixes #19468

Based on earlier PR #19473

- refactored the `conditional_types` function. 
- return `UninhabitedType(), default` when no ranges are given. This
corresponds to `isinstance(x, ())`, with an empty tuple, which always
returns `False` at runtime.
- modified #19473 to change the proposed type, rather than directly
returning. This is essential to maintain the behavior of the unit test
`testIsinstanceWithOverlappingPromotionTypes`
- Added special casing in `restrict_subtype_away`: if the second
argument is a `TypeVar`, replace it with its upper bound (crucial to get
correct result in `testNarrowSelfType`)
- Allow `TypeChecker.get_isinstance_type` to return empty list (fixes
`isinstance(x, ())` behavior).


## Modified tests

- `testIsInstanceWithEmtpy2ndArg` now correctly infers unreachable for
`isinstance(x, ())`.
- `testNarrowingUnionMixins` now predicts the same results as pyright
playground

## New Tests

- `testMatchNarrowDownUnionUsingClassPattern`
(https://mypy-play.net/?mypy=1.17.0&python=3.12&gist=e9ec514f49903022bd32a82ae1774abd)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

[match-case] class pattern narrowing with Union-argument different from equivalent isinstance call.

2 participants


Back | FazBrowse Home | New Git URL