-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Fix type inference in pattern matching by positional argument #13618
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
Conversation
7204ab1 to
2f4bd29
Compare
This comment has been minimized.
This comment has been minimized.
|
I am quite surprised! |
|
@sobolevn a good surprise indeed! Tests look comprehensive enough. I wonder, does it also run correctly with inheritance? (Note the extra class Base(Generic[T]):
...
@dataclass # problem also occurs with non-dataclass equivalent
class A(Base[T]):
x: T
a: Base[str] = A("foo")
reveal_type(a) # Base[str] (correct)
match a:
case A(b):
reveal_type(b) # Any (incorrect! Should be builtins.str)It might be a good addition to the tests, because inheritance here breaks |
|
I will add a test case for subclasses and dataclasses. Thanks! |
|
Ok, this indeed does not work for Let's open a new issue for it. Can you, @ariebovenberg? 🙂 |
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
…python#13618)" This reverts commit b031f1c.
|
@thomkeh hi, can you please share any context on your commit? Is there something wrong? |
|
Oh sorry, I clicked the "revert" button accidentally! I didn't know this was possible. Everything is fine as far as I know. |
Oh, this was very interesting.
During the debug sessions I learned a lot about how pattern matching and its analysis do work.
But, the problem was that
expand_typedid not preserve.last_known_valuefor some reason.I used
.copy_modifiedto preserve everything we know aboutInstance.However, I expect that some tests might fail now. This code even has something similar in
TODOsome lines above:mypy/mypy/expandtype.py
Lines 144 to 148 in 88aed94
Let's see what will happen.
Closes #13612