Improve type narrowing for walrus operator in conditional statements #9288
Conversation
|
Thanks for the PR! This addresses some important issues. I can review this later this week -- but if somebody else wants to review this, please go ahead! |
…g in if statements (#9297) This adds support for union narrowing in if statements when condition value has defined literal annotations in `__bool__` method. Value is narrowed based on the `__bool__` method return annotation and this works even if multiple instances defines the same literal value for `__bool__` method return type. This PR also works well with #9288 and makes below example to work as expected: ```python class A: def __bool__(self) -> Literal[True]: ... class B: def __bool__(self) -> Literal[False]: ... def get_thing() -> Union[A, B]: ... if x := get_thing(): reveal_type(x) # Revealed type is '__main__.A' else: reveal_type(x) # Revealed type is '__main__.B' ``` Partially fixes #9220
mypy/checker.py
Outdated
| if else_condition_map is not None: | ||
| else_map.update(else_condition_map) | ||
|
|
||
| return if_map, else_map |
There was a problem hiding this comment.
None values are special, but now we always return a non-None value. Should we sometimes still return a None value? The comment for TypeMap explains the distinction.
There was a problem hiding this comment.
Yes, we should return None value in case when condition_map is None. Basically there are three possibilities here:
assignment_map is Noneandcondition_map is None
def always_false() -> Literal[False]: ...
if x := always_false():
reveal_type(x) # E: Statement is unreachableassignment_map is not Noneandcondition_map is None
if (x := 0):
reveal_type(x) # E: Statement is unreachableassignment_map is not Noneandcondition_map is not None
if (is_str := maybe_str is not None):
reveal_type(is_str) # N: Revealed type is 'builtins.bool'
reveal_type(maybe_str) # N: Revealed type is 'builtins.str'I couldn't find any use case where assignment_map is None and condition_map is not None. I guess that there isn't actually such scenarios where this condition would be true. I added test cases for all above cases and corrected the return statement here.
Also, I modified slightly the behavior of find_isinstance_check_helper method to have a default mechanism for creating type maps (which was basically cut and pasted from the RefExpr case). This was actually needed to make use cases in testWalrusAssignmentAndConditionScopeForFunction test case work properly.
test-data/unit/check-python38.test
Outdated
|
|
||
| maybe_str: Optional[str] | ||
|
|
||
| if (is_str := maybe_str is not None): |
There was a problem hiding this comment.
Does this PR affect simple cases, such as if x := f()? If yes, it's worth testing these separately.
What about if (x := f()) is not None (note that parentheses are different from this test case)?
There was a problem hiding this comment.
I added tests for both if x := f() and if (x := f()) is not None cases.
Also, I found two odd behaviors:
- Type narrowing without
--strict-optionalflags seems odd because it can narrows theUnionoutside of conditional statement even though it is not known which value the variable actually holds. With--strict-optionalthe lastreveal_typein below example emitsUnion[builtins.str, None].
from typing import Optional
maybe_str: Optional[str]
if (x := maybe_str is not None):
reveal_type(maybe_str) # N: Revealed type is 'builtins.str'
else:
reveal_type(maybe_str) # N: Revealed type is 'None'
reveal_type(maybe_str) # N: Revealed type is 'builtins.str'- Below case is not handled properly and does not reflect the runtime behavior. Note that this is actually a different use case than the one I added in
testWalrusAssignmentAndConditionScopeForLiteraltest case because it does not fall intois_true_literaloris_false_literalchecks infind_isinstance_check_helpermethod.
if (x := 5):
reveal_type(x) # N: Revealed type is 'builtins.int'
else:
reveal_type(x) # E: Statement is unreachable
reveal_type(x) # N: Revealed type is 'builtins.int'
There was a problem hiding this comment.
if (x := maybe_str is not None):
Note that this is parsed as “x := (maybe_str is not None)“
|
@kprzybyla Do you have time to respond to the feedback above? If you are busy, maybe somebody else could finish this PR. |
|
@JukkaL Sorry, I had a really busy week, I should be able to take care of this soon this week or maybe even today. |
|
@JukkaL Please tell me if there's anything else I should do or correct. Once you have time of course 😃 |
There was a problem hiding this comment.
Thanks, and these new test cases are great!
The AssignmentExpr logic looks good to me, although technically I think if either if_assignment_map or if_condition_map is None, you'll want to return None. It's a little contrived, but e.g.:
x: None
if x := returns_any():
x = None
doesn't get flagged currently by --warn-unreachable. (Maybe a little less contrived thanks to your previous PR, since mypy is now smarter about classes whose __bool__ is a literal :-).
I don't have a good sense of potential consequences of now falling back to what was previously just the RefExpr case (having things under explicit if statements made reasoning about things somewhat easier for me), and you now have a small merge conflict, but otherwise looks good!
|
Any word on this? I can fix the merge conflict and put it up for review if that helps. |
|
Not sure why Github didn't like that. I opened a new PR at #11202 |
This improves type narrowing for walrus operator (AssignmentExpr) in conditional statements. Information about the both assignment and condition will be now passed correctly in the if and else map to the ConditionalTypeBinder for type narrowing. Also, literal checks are now correctly interpreted for AssignmentExpr.
Partially fixes #9220 and #9229