-
-
Notifications
You must be signed in to change notification settings - Fork 32.7k
gh-137840: Implement PEP 728 (closed and extra_items in typing.TypedDict) #137933
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
base: main
Are you sure you want to change the base?
Conversation
cf0ac68
to
c338766
Compare
TypeError, | ||
"Cannot combine closed=True and extra_items" | ||
): | ||
class TD(TypedDict, closed=True, extra_items=range): |
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.
Is range
the builtin here?
That's a bit strange.
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 agree it looks a bit odd as an arbitrary extra_items
value, but it's valid, since it just means extra keys must have values assignable to range. For context, this test is copied from typing_extensions
, where it was used to cover the same case. We could edit it for readability, but I kind of like it as a reminder for maintainers that extra_items
accepts any type, not just the obvious ones.
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 could be valid for a regular test that validates stored values.
this however is a negative test, isn’t 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.
It's still valid/correct as a negative test. By "valid" do you mean "correct" or "readable?" Passing range into extra_items is valid, and passing closed=True at the same time should raise a runtime 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.
Sorry I was being terse.
I'm trying to say that this test:
class Child(Base, extra_items=int):
a: str
self.assertEqual(Child.__required_keys__, frozenset({'a'}))
self.assertEqual(Child.__optional_keys__, frozenset({}))
self.assertEqual(Child.__readonly_keys__, frozenset({}))
self.assertEqual(Child.__mutable_keys__, frozenset({'a'}))
self.assertEqual(Child.__annotations__, {"a": str})
self.assertIs(Child.__extra_items__, int)
self.assertIsNone(Child.__closed__)
Could have an extra_items=range
counterpart.
That would make a solid test, both understandable and useful.
Meanwhile, the negative test, class TD(TypedDict, closed=True, extra_items=range): --> error
would be better served with a simpler, more straightforward extra_items=int
argument.
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 clarifying. I thought about it some more.
I don't think introducing an extra_items=range
counterpart would actually widen the test coverage, since it wouldn't be exercising any new behavior. PEP 728 splits responsibilities between runtime and type checker behavior. While extra_items
is only supposed to accept a valid type expression, validating that is the type checker's job (e.g. MyPy's valid-type error), not the runtime's. The runtime just stores whatever is passed in.
So the real subject under test is simply:
Child.__extra_items__
must correctly store the value passed intoextra_items
.
We don't need multiple values to prove that behavior.
And although range
is a less obvious type, I think it makes sense in the negative test. That test is specifically asserting the error message “Cannot combine closed=True
and extra_items
”. Using range
highlights that the failure comes from the combination, not from range
itself being an invalid value of extra_items
.
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 tend to use range
sometimes as it's a builtin type that isn't generic (like list
) and doesn't participate in promotion weirdness (like float
and historically bytes
), so it's a good basic type to test with.
Plus, people who forget that range
is a type get to learn something :)
Implemented _Py_NoExtraItemsStruct and _Py_NoExtraItemsStruct
They now both accept `closed` and `extra_items` as parameters. For introspection, these arguments are also mapped to 2 new attributes: `__closed__` and `__extra_items__`.
c1395ca
to
e2297b6
Compare
e2297b6
to
1f23caa
Compare
Ready for review! @PIG208 @JelleZijlstra |
@@ -123,6 +123,63 @@ PyTypeObject _PyNoDefault_Type = { | |||
|
|||
PyObject _Py_NoDefaultStruct = _PyObject_HEAD_INIT(&_PyNoDefault_Type); | |||
|
|||
/* NoExtraItems: a marker object for TypedDict extra_items when it's unset. */ |
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.
Is this implemented in C just for analogy with NoDefault? NoDefault is in C because TypeVar uses it and TypeVar has to be in C because it has native syntax, but TypedDict is all Python code, so I'd rather keep NoExtraItems also just Python.
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.
Also if we get PEP 661 implemented in 3.15 (I hope we will), this can and should use it.
"--\n\n" | ||
"The type of the NoExtraItems singleton."); | ||
|
||
PyTypeObject _PyNoExtraItems_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.
I agree with Jelle that this probably doesn't need to be a C singleton, but if we do keep it, this needs to be added to the static types array in object.c
.
.tp_methods = noextraitems_methods, | ||
.tp_new = noextraitems_new, |
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.
Again moot if we don't keep the singleton, but otherwise we probably want a tp_hash
and tp_richcompare
.
@@ -3266,14 +3274,41 @@ class DatabaseUser(TypedDict): | |||
id: ReadOnly[int] # the "id" key must not be modified | |||
username: str # the "username" key can be changed | |||
|
|||
The closed argument controls whether the TypedDict allows additional | |||
non-required items during inheritance and assignability checks. | |||
If closed=True, the TypedDict is closed to additional items:: |
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.
If closed=True, the TypedDict is closed to additional items:: | |
If closed=True, the TypedDict does not allow additional items:: |
@@ -123,6 +123,63 @@ PyTypeObject _PyNoDefault_Type = { | |||
|
|||
PyObject _Py_NoDefaultStruct = _PyObject_HEAD_INIT(&_PyNoDefault_Type); | |||
|
|||
/* NoExtraItems: a marker object for TypedDict extra_items when it's unset. */ |
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.
Also if we get PEP 661 implemented in 3.15 (I hope we will), this can and should use it.
Features:
TypedDict
:closed
andextra_items
.TypeError
when both parameters are specified together.__closed__
and__extra_items__
.typing.NoExtraItems
is now a C-level immortal singleton, co-located withNoDefault
intypevarobject.c
.TypedDict
.Not included in this PR:
Doc/library/typing.rst