-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
'in' can narrow TypedDict unions #13838
'in' can narrow TypedDict unions #13838
Conversation
e714a52
to
4642a9c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
c6f793e
to
4617bf8
Compare
4617bf8
to
ca87360
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@hauntsaninja I still need to actually respect BTW, does it strictly require class D(TypedDict):
foo: str
d: D
if 'foo' in d:
assert_type(d, D)
else:
assert_never() It doesn't make a good case for ruling out unions, though: class D1(TypedDict):
foo: str
class D2(TypedDict):
bar: str
d: D1 | D2
if 'foo' in d:
assert_type(d, Union[D1, D2])
else:
assert_type(d, D2) Might seem a bit complex, but we already have to deal with it for totality. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Update: Also discerning on final-ity now In the case of https://github.com/Rapptz/discord.py/blob/3bca40352ecfa5b219bae0527252ee5d296113e7/discord/client.py#L1755: |
This comment has been minimized.
This comment has been minimized.
@hauntsaninja wdyt? |
@sobolevn can this be merged as is? |
This comment has been minimized.
This comment has been minimized.
@hauntsaninja ping |
This comment has been minimized.
This comment has been minimized.
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.
Took a look at the tests and left some comments. I'll review the actual code too, but might take me a little bit to get to it. Will make sure to review before 1.0 though
@@ -2025,6 +2025,193 @@ v = {bad2: 2} # E: Extra key "bad" for TypedDict "Value" | |||
[builtins fixtures/dict.pyi] | |||
[typing fixtures/typing-typeddict.pyi] | |||
|
|||
[case testOperatorContainsNarrowsTypedDicts_unionWithList] |
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 test case seems completely duplicated in testOperatorContainsNarrowsTypedDicts_total, let's remove this?
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'll remove the relevant part from testOperatorContainsNarrowsTypedDicts_total
, as to keep test names descriptive.
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.
test-data/unit/check-typeddict.test
Outdated
else: | ||
assert_type(d_or_list, list[str]) |
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.
else: | |
assert_type(d_or_list, list[str]) | |
elif 'bar' in d_or_list: | |
assert_type(d_or_list, list[str]) | |
else: | |
assert_type(d_or_list, list[str]) |
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.
|
||
foo_or_invalid: Literal['foo', 'invalid'] | ||
if foo_or_invalid in d: | ||
assert_type(d, D1) |
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.
in theory this could narrow foo_or_invalid
as well, want to add an assert_type for that behaviour too?
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.
That'd have to be implemented. And I think it could be pretty neat, but would require a rework: I'd have to pass in the left expression, and return type maps (not "if_type" and "else_type"). Maybe in a follow-up?
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.
My current implementation is to make a form of "tagged TypeDicts" work, but I suspect a more generalized form of type narrowing should be possible. However, before we tackle the more contrived x in y
case, we should probably do it for x == y
, so that this example would see narrowing applied.
(I might be naive, though, and this might've been tried before and proven impossible.)
P.S. PyRight is similarly limited in this
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.
Oh sorry, to be clear my suggestion wasn't to implement this / I don't think it's a terribly important feature. I was just saying it's worth adding an assert_type
in this test to make this (lack of) behaviour clear to the reader.
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.
Gotcha, done: fb564eb
test-data/unit/check-typeddict.test
Outdated
value: int | ||
if 'foo' in arg: | ||
assert_type(d, Union[D1, D2]) # strangely enough it's seen as a union | ||
value = arg['foo'] # but acts here as D1 |
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 think this is because mypy processes these twice. Want to change the test to just do reveal_type(arg['foo'])
?
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.
…nst/mypy into typed-dict-in-type-narrowing
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Diff from mypy_primer, showing the effect of this PR on open source code: discord.py (https://github.com/Rapptz/discord.py)
- discord/client.py:1755: error: Value of "rpc_origins" has incompatible type "None"; expected "List[str]" [typeddict-item]
|
Could the PR description be modified to reflect the change made in f799344 ? Perhaps to class A(TypedDict)
foo: int
@final
class B(TypedDict):
bar: int
union: Union[A, B] = ...
value: int
if 'foo' in union:
# Cannot be a B as it is final and has no "foo" field, so must be an A
value = union['foo']
else:
# Cannot be an A as those went to the if branch
value = union['bar'] (Super excited to see this PR, thank you so much!) |
@alicederyn thanks for spotting this — updated with your proposed sample code |
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.
Thanks for improving this! This is a nice feature
in
could narrow unions of TypeDicts, e.g.