-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Do not treat match value patterns as isinstance checks
#20146
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
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
|
I'm frankly not sure what part of the core team to ping here - reviewers of the original PR, @JukkaL? @randolf-scholz please also take a look if you have time, IIRC some recent PRs of yours were related to pattern matching? Primer finds nothing, apparently |
Well, many popular projects still support 3.9, but match-case requires at least 3.10, so that's not surprising. |
|
One thing that might be worth testing is if it interacts correctly with unions types and union patterns, e.g. from typing import Literal
def test1(x: Literal[1,2,3]) -> None:
match x:
case 1:
reveal_type(x)
case other:
reveal_type(x)
def test2(x: Literal[1,2,3]) -> None:
match x:
case 1:
reveal_type(x)
case 2:
reveal_type(x)
case 3:
reveal_type(x)
case other:
assert_never(x)
def test3(x: Literal[1,2,3]) -> None:
match x:
case 1 | 3:
reveal_type(x)
case other:
reveal_type(x)and possibly the same with enum arguments / enum patterns. |
|
I don't want to add any enum tests here, they won't be representative due to #19594 - I'd prefer to add |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
| # like `if 1 < len(x) < 4: ...` | ||
| return reduce_conditional_maps(self.find_tuple_len_narrowing(node), use_meet=True) | ||
|
|
||
| def equality_type_narrowing_helper( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see why you split this function, but it doesn't seem necessary?
| match m: | ||
| case b.b: | ||
| reveal_type(m) # N: Revealed type is "__main__.<subclass of "__main__.A" and "b.B">" | ||
| reveal_type(m) # N: Revealed type is "__main__.A" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was confusing for me until I realized that's just how equality works!
| def visit_value_pattern(self, o: ValuePattern) -> PatternType: | ||
| current_type = self.type_context[-1] | ||
| typ = self.chk.expr_checker.accept(o.expr) | ||
| typ = coerce_to_literal(typ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this coerce_to_literal still necessary now that we use narrow_type_by_equality rather than conditional_types_with_intersection?
As discovered by @randolf-scholz in #20142,
mypytreats value patterns in match statement in a completely wrong way.From PEP 622:
Existing tests for the feature are invalid: for example,
must reveal
object, and testtestMatchValuePatternNarrowsasserts the opposite. Here's a runtime example:I have updated the existing tests accordingly.
The idea is that value patterns are essentially equivalent to
if foo == SomeValuechecks, notisinstancechecks modelled byconditional_types_wit_intersection.The original implementation was introduced in #10191.