-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
✨ Added new Error to TypedDict #14225
Conversation
See: [python#4617](python#4617) This allows the following code to trigger the error `typeddict-unknown-key` ```python A = T.TypedDict("A", {"x": int}) def f(x: A) -> None: ... f({"x": 1, "y": "foo"}) ``` The user can then safely ignore this specific error at their disgression.
TODO:
mypy used to only report extra keys and not missing keys. We now report both (I suspect the CI will yell at me and I'll try to correct those tests) edit: Having run the tests on my own terminal I can see a few that fail. 1 - as expected we now have two error codes instead of just the one. Will need to fix these.
|
We now return two instead of one
16339ce
to
fcf4909
Compare
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! I have few review comments
test-data/unit/check-errorcodes.test
Outdated
@@ -455,7 +455,7 @@ class E(TypedDict): | |||
y: int | |||
|
|||
a: D = {'x': ''} # E: Incompatible types (expression has type "str", TypedDict item "x" has type "int") [typeddict-item] | |||
b: D = {'y': ''} # E: Extra key "y" for TypedDict "D" [typeddict-item] | |||
b: D = {'y': ''} # E: Missing key "x" for TypedDict "D" [typeddict-item] # E: Extra key "y" for TypedDict "D" [typeddict-unknown-key] |
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.
Could you please add a test verifying that values for known keys are still type-checked if unknown keys are present?
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.
Good idea, will add it over the weekend
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.
Done!
I left them on the 'check-errorcodes' file but can come them to the typeddict specific file.
@@ -67,6 +67,9 @@ def __str__(self) -> str: | |||
TYPEDDICT_ITEM: Final = ErrorCode( | |||
"typeddict-item", "Check items when constructing TypedDict", "General" | |||
) | |||
TYPPEDICT_UNKNOWN_KEY: Final = ErrorCode( | |||
"typeddict-unknown-key", "Check unknown keys when constructing TypedDict", "General" | |||
) |
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.
As mentioned on the issue, for consistency this error code should be also used for errors in case like:
d["unknown"]
d["unknown"] = 42
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.
Indeed, that makes sense, but I'm afraid I don't like it. I'm worried about what would happen when the user would ignore this error. For example
Running mypy --disable-error-code=typeddict-unknown-key
on the snippet:
A = T.TypedDict("A", {"x": int})
def f(x: A) -> None:
X["obvious error"]
f({"x": 1, "y": "foo"}) # I want this to be OK
Would result in an obvious error being completely ignored by mypy.
So to my view we have to options:
1 - rename the unknown-key to something that better reflects the code (unknown-anon-key
for example)
2 - Create another error code for this usecase.
Open to suggestions.
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.
OK, reading an unknown key may be too much, but setting it (i.e. d["unknown"] = 42
) should definitely use the same error code. I don't see a difference between:
d: TD = {"known": 1, "unknown": 2}
and
d: TD = {"known": 1}
d["unknown"] = 2
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 that's very reasonable. Will ping you when I have that sorted.
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.
Done!
Not sure if the solution is pretty. Added a test for it on the typeddict file.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Ivan Levkivskyi <[email protected]>
Co-authored-by: Ivan Levkivskyi <[email protected]>
This comment has been minimized.
This comment has been minimized.
🧑🔬 Added test to check this behaviour
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.
Looks better now, I have few more comments.
mypy/checkexpr.py
Outdated
if not (callee.required_keys <= set(kwargs.keys()) <= set(callee.items.keys())): | ||
actual_keys = kwargs.keys() | ||
found_set = set(actual_keys) | ||
if not (callee.required_keys <= found_set <= set(callee.items.keys())): |
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.
Do you really need to convert these to sets? IIRC dictionary .keys()
already behave like sets.
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.
You're quite right. I had no idea.
Fixed in f7a7f4f
mypy/checkexpr.py
Outdated
self.msg.unexpected_typeddict_keys( | ||
callee, expected_keys=expected_keys, actual_keys=list(actual_keys), context=context | ||
) | ||
return AnyType(TypeOfAny.from_error) | ||
if callee.required_keys > found_set: | ||
# found_set is not a sub-set of the required_keys |
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 comment contradicts the condition, the if
condition specifically checks that found_set
is a subset of required_keys
.
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.
a value. | ||
Setting a value on a TypedDict is an 'unknown-key' error, | ||
whereas reading it is the more serious/general 'item' error. | ||
""" |
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.
Please use common convention for docstrings. It should be like this.
def func() -> None:
"""Short description in form of "do something".
After empty line, long description indented with function body.
The closing quotes n a separate line.
"""
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.
Fixed in: 06a3866
test-data/unit/check-errorcodes.test
Outdated
a['other_commonpart'] = 3 # type: ignore[typeddict-item] | ||
a['other_commonpart'] = 3 # type: ignore[typeddict-unknown-key] \ | ||
# N: Did you mean "one_commonpart" or "two_commonparts"? \ | ||
# N: Error code "typeddict-item" not covered by "type: ignore" comment |
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 looks wrong. The whole idea is that if an error is suppressed, the related notes should be suppressed as well.
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've implemented your suggestion in commit 780e10d
Originally, I did it on purpose as I liked the 'Did you mean' feature. It would be helpful for situations like so:
# Assume we're running mypy while ignoring the typpedict-unknown-key
F = TypedDict('F', { 'foo':int })
foo: F = { 'foo': 1 }
foo['bar'] = 2
# OOPS! I made a typo
foo['fooo'] = 2
My rationale being that it would be quite rare for a user to both want to add extra keys AND have those keys be very similar looking.
This comment has been minimized.
This comment has been minimized.
We don't need to convert dict_keys/items to set. Also fixed the comment saying literally the opposite of what was happening
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, looks almost ready! Couple more small comments.
self.msg.unexpected_typeddict_keys( | ||
callee, expected_keys=expected_keys, actual_keys=list(actual_keys), context=context | ||
) | ||
return AnyType(TypeOfAny.from_error) | ||
if callee.required_keys > actual_keys: |
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 condition is unreachable, there is callee.required_keys <= actual_keys
in the enclosing if statement.
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.
Hello!
I believe this is correct as the callee.required_keys <= actual_keys
has a not preceeding it.
Indeed, removing this return wil fail the test [case TestCannotCreateTypedDictInstanceWithMissingItems]
Let's imagine the following:
assert callee.items.keys == {1,2,3,4}
assert callee.required_keys == {1,2,3}
actual_keys = {1,2}
if not (required <= actual_keys <= callee.items.keys ):
# call unexpected_typeddict_keys on the actual vs expected
if required > actual:
return AnyType
Will return AnyType and print out a single error message: "missing required key 3"
Whereas with actual_keys = {1,2,3, 'unrelated'}
will print out "extra key 'unrelated' but not return AnyType; ie it will continue to check the TypedDict for any other errors (For example, the actual keys might all be accounted for but have the wrong type).
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 yeah, right, I have missed the not
.
Oh wait, there is one more thing, you need to add an entry in the docs to the list of error codes (otherwise it will be hard for people to find out the solution). |
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Ivan Levkivskyi <[email protected]>
This comment has been minimized.
This comment has been minimized.
@JoaquimEsteves could you please re-run |
Hello, apologies for the delay! Went off on vacation and then real life™️ got in the way. Hopefully this'll be OK to merge now. A couple of three things: Could you re-consider this comment? IMO it makes sense to keep the "Did you mean" for when the text is really similar. I can't seem to get Black to collaborate with me. The CI/CD was complains that Black was returning an error on Please review the text for the |
The error message in CI says that black wants you to apply the following diff: --- mypy/messages.py 2023-01-23 13:51:23.667146 +0000
+++ mypy/messages.py 2023-01-23 13:52:16.792518 +0000
@@ -1700,13 +1700,11 @@
f'TypedDict {format_type(typ)} has no key "{item_name}"', context, code=err_code
)
matches = best_matches(item_name, typ.items.keys(), n=3)
if matches:
self.note(
- "Did you mean {}?".format(pretty_seq(matches, "or")),
- context,
- code=err_code,
+ "Did you mean {}?".format(pretty_seq(matches, "or")), context, code=err_code
)
def typeddict_context_ambiguous(self, types: list[TypedDictType], context: Context) -> None:
formatted_types = ", ".join(list(format_type_distinctly(*types)))
self.fail( |
This comment has been minimized.
This comment has been minimized.
Thanks @AlexWaygood; I was confused as it looked like my The newest commit should have fixed all of the docs+black issues. Do submit feedback about my writting on the docs; Otherwise I'll just press the commit button : ) |
This comment has been minimized.
This comment has been minimized.
@AlexWaygood or @ilevkivskyi as a first-time contributor it seems I need one of you to request running the CD pipeline before approval (I foolishly merged master into this branch again, thinking that it would re-run the workflow) |
I'm afraid I'm not a maintainer, so I don't have the power to run the CI. I'm just a triager and contributor :) |
Sorry to bother you @AlexWaygood and @ilevkivskyi, I seem to have hit a wall here with the P.S. Apologies if pinging is rude - I don't think requesting another review was the appropriate action |
This comment has been minimized.
This comment has been minimized.
Looks like it was just a random fluke — closing and reopening the PR fixed it :) |
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! Docs look good, I just made a bunch of suggestions on formatting/naming to make them more consistent. I am going to apply them now.
I would propose to keep the PR as is (i.e. with consistent use of error codes for error and note). We don't have a precedent for such inconsistency (e.g. IIRC |
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
Fixes #4617
This allows the following code to trigger the error
typeddict-unknown-key
The user can then safely ignore this specific error at their disgression.