✨ Added new Error to TypedDict #14225

JoaquimEsteves · 2022-11-30T12:59:06Z

This allows the following code to trigger the error typeddict-unknown-key

A = T.TypedDict("A", {"x": int})

def f(x: A) -> None:
    ...

f({"x": 1, "y": "foo"})  # err: typeddict-unknown-key
f({"y": "foo"})  # err: typeddict-unknown-key & typeddict-item
f({"x": 'err', "y": "foo"})  # err: typeddict-unknown-key & typeddict-item

a: A = { 'x': 1 }

# You can set extra attributes
a['extra'] = 'extra' # err: typeddict-unknown-key
# Reading them produces the normal item error
err = a['does not exist'] # err: typeddict-item

The user can then safely ignore this specific error at their disgression.

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.

JoaquimEsteves · 2022-11-30T13:04:00Z

TODO:

Add a small unit-test
Ensure I don't trample over previous tests*

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.
2 - A few errors like: Expected TypedDict key "x" but found. There was an early return previously whenever we found extra keys. I'll have to add it back if there's missing or extra, or change the tests

~~Will work on it on the weekend :)~~
Done

We now return two instead of one

ilevkivskyi

Thanks! I have few review comments

ilevkivskyi · 2022-12-01T23:44:09Z

test-data/unit/check-errorcodes.test

@@ -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]


Could you please add a test verifying that values for known keys are still type-checked if unknown keys are present?

Good idea, will add it over the weekend

Done!

I left them on the 'check-errorcodes' file but can come them to the typeddict specific file.

test-data/unit/check-errorcodes.test

test-data/unit/check-typeddict.test

ilevkivskyi · 2022-12-01T23:49:02Z

mypy/errorcodes.py

@@ -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"
+)


As mentioned on the issue, for consistency this error code should be also used for errors in case like:

d["unknown"] d["unknown"] = 42

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.

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

I think that's very reasonable. Will ping you when I have that sorted.

Done!

Not sure if the solution is pretty. Added a test for it on the typeddict file.

Co-authored-by: Ivan Levkivskyi <[email protected]>

🧑‍🔬 Added test to check this behaviour

ilevkivskyi

Looks better now, I have few more comments.

ilevkivskyi · 2022-12-07T23:23:59Z

mypy/checkexpr.py

-        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())):


Do you really need to convert these to sets? IIRC dictionary .keys() already behave like sets.

You're quite right. I had no idea.

Fixed in f7a7f4f

ilevkivskyi · 2022-12-07T23:27:34Z

mypy/checkexpr.py

            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


This comment contradicts the condition, the if condition specifically checks that found_set is a subset of required_keys.

Done: f7a7f4f#diff-42eda644ce5003e33ea0766d7601fcfb4934ad106429abfe115954a1fccddd07R804

ilevkivskyi · 2022-12-07T23:35:16Z

mypy/messages.py

+            a value.
+            Setting a value on a TypedDict is an 'unknown-key' error,
+            whereas reading it is the more serious/general 'item' error.
+        """


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. """

Fixed in: 06a3866

ilevkivskyi · 2022-12-07T23:37:51Z

test-data/unit/check-errorcodes.test

-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


This looks wrong. The whole idea is that if an error is suppressed, the related notes should be suppressed as well.

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.

We don't need to convert dict_keys/items to set. Also fixed the comment saying literally the opposite of what was happening

ilevkivskyi

Thanks, looks almost ready! Couple more small comments.

ilevkivskyi · 2022-12-24T13:02:15Z

mypy/checkexpr.py

            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:


This condition is unreachable, there is callee.required_keys <= actual_keys in the enclosing if statement.

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).

Oh yeah, right, I have missed the not.

mypy/messages.py

ilevkivskyi · 2022-12-24T13:17:16Z

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).

Co-authored-by: Ivan Levkivskyi <[email protected]>

ilevkivskyi · 2023-01-23T13:50:49Z

@JoaquimEsteves could you please re-run black . locally, commit and push? It looks like black test is failing.

JoaquimEsteves · 2023-01-23T13:54:22Z

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 messages.py; however I don't have this problem on my local and my commit does not produce the changes shown.

Please review the text for the error_codes.rst.
aspell didn't point out any spelling mistakes; but if you have any idea on how to improbe the text I'm all ears.

AlexWaygood · 2023-01-23T14:01:11Z

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(

JoaquimEsteves · 2023-01-23T14:20:18Z

Thanks @AlexWaygood; I was confused as it looked like my message.py was different from the one shown on the CD (turns out I needed to pull from the python:mypy).

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 : )

JoaquimEsteves · 2023-01-23T15:20:47Z

@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)

AlexWaygood · 2023-01-23T15:26:44Z

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 :)

JoaquimEsteves · 2023-01-25T15:21:54Z

Sorry to bother you @AlexWaygood and @ilevkivskyi,

I seem to have hit a wall here with the run_primer; the action seems to run 4 times and fail twice, and at this stage I don't know what to do 😅

P.S. Apologies if pinging is rude - I don't think requesting another review was the appropriate action

AlexWaygood · 2023-01-25T15:46:41Z

I seem to have hit a wall here with the run_primer; the action seems to run 4 times and fail twice, and at this stage I don't know what to do 😅

Looks like it was just a random fluke — closing and reopening the PR fixed it :)

ilevkivskyi

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.

docs/source/error_code_list.rst

ilevkivskyi · 2023-01-25T16:32:27Z

Could you re-consider #14225 (comment)? IMO it makes sense to keep the "Did you mean" for when the text is really similar.

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 # type: ignore[attr-defined] suppresses both error and the related suggestion). We can easily change this later if people will complain.

github-actions · 2023-01-25T16:48:20Z

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

JoaquimEsteves added 2 commits November 30, 2022 13:45

🐛 Fixed 'missing keys' not being reported

f19a7f7

JoaquimEsteves added 2 commits November 30, 2022 14:42

🐛 Early return in case there's missing or extra keys

2a4220c

🧑‍🔬 Fixed simple error-code tests

fcf4909

We now return two instead of one

JoaquimEsteves force-pushed the master branch from 16339ce to fcf4909 Compare November 30, 2022 13:45

ilevkivskyi reviewed Dec 1, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

JoaquimEsteves and others added 2 commits December 2, 2022 12:14

Update test-data/unit/check-typeddict.test

deb6543

Co-authored-by: Ivan Levkivskyi <[email protected]>

Update test-data/unit/check-errorcodes.test

222869d

Co-authored-by: Ivan Levkivskyi <[email protected]>

This comment has been minimized.

Sign in to view

JoaquimEsteves added 2 commits December 5, 2022 11:09

✨ We now typecheck despite having an extra-key

70c0a18

🧑‍🔬 Added test to check this behaviour

🧹 Setting an extra value on a TypedDict now has the correct error code

eda0c8b

This comment has been minimized.

Sign in to view

JoaquimEsteves added 2 commits December 5, 2022 12:44

✨ We can now set an item and get the correct error code

25f3dbc

🧑‍🔬 Redid the IgnotErrorCodeTypedDictNoteIgnore test

c7c687f

JoaquimEsteves requested a review from ilevkivskyi December 6, 2022 15:05

ilevkivskyi reviewed Dec 7, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

JoaquimEsteves added 4 commits December 8, 2022 12:02

🐈‍⬛ Black

a275c86

🐛 Added Review Suggestions

f7a7f4f

We don't need to convert dict_keys/items to set. Also fixed the comment saying literally the opposite of what was happening

📝 Fixed docstring not being in the correct format.

06a3866

🐛 Fixed ignoring unknown-key not silencing the 'Did you mean' message

780e10d

JoaquimEsteves requested a review from ilevkivskyi December 8, 2022 12:59

AlexWaygood added the topic-typed-dict label Dec 11, 2022

ilevkivskyi approved these changes Dec 24, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

Review Feedback - Better Docstrings

fb98524

Co-authored-by: Ivan Levkivskyi <[email protected]>

This comment has been minimized.

Sign in to view

📝 Added unknown-key to the error_code_list

708d322

This comment has been minimized.

Sign in to view

JoaquimEsteves added 2 commits January 23, 2023 15:10

🐛 Black and sphinx bugs

e4b82b6

Merge branch 'master' into master

1c9a623

Merge branch 'master' into master

f4f0cd0

This comment has been minimized.

Sign in to view

AlexWaygood closed this Jan 25, 2023

AlexWaygood reopened this Jan 25, 2023

This comment has been minimized.

Sign in to view

ilevkivskyi approved these changes Jan 25, 2023

View reviewed changes

Apply suggestions from code review (docs)

957ba17

ilevkivskyi merged commit 81efd6e into python:master Jan 27, 2023

reneleonhardt mentioned this pull request Sep 21, 2024

Use extra keys for TypedDict Delgan/loguru#1208

Closed

✨ Added new Error to TypedDict #14225

✨ Added new Error to TypedDict #14225

Conversation

JoaquimEsteves commented Nov 30, 2022 • edited Loading

JoaquimEsteves commented Nov 30, 2022 • edited Loading

ilevkivskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoaquimEsteves Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ilevkivskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

ilevkivskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi commented Dec 24, 2022

This comment has been minimized.

This comment has been minimized.

ilevkivskyi commented Jan 23, 2023

JoaquimEsteves commented Jan 23, 2023

AlexWaygood commented Jan 23, 2023 • edited Loading

This comment has been minimized.

JoaquimEsteves commented Jan 23, 2023

This comment has been minimized.

JoaquimEsteves commented Jan 23, 2023

AlexWaygood commented Jan 23, 2023 • edited Loading

JoaquimEsteves commented Jan 25, 2023

This comment has been minimized.

AlexWaygood commented Jan 25, 2023

ilevkivskyi left a comment

Choose a reason for hiding this comment

ilevkivskyi commented Jan 25, 2023

github-actions bot commented Jan 25, 2023

JoaquimEsteves commented Nov 30, 2022 •

edited

Loading

JoaquimEsteves commented Nov 30, 2022 •

edited

Loading

JoaquimEsteves Dec 2, 2022 •

edited

Loading

AlexWaygood commented Jan 23, 2023 •

edited

Loading

AlexWaygood commented Jan 23, 2023 •

edited

Loading