Skip to content

gh-112509: Fix keys being present in both required_keys and optional_keys in TypedDict #112512

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 29, 2023
Merged

gh-112509: Fix keys being present in both required_keys and optional_keys in TypedDict #112512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc3095e
gh-112509: Fix keys being present in both required_keys and optional_…
JelleZijlstra Nov 29, 2023
af8a732
thanks lint
JelleZijlstra Nov 29, 2023
2c83a11
Update Lib/typing.py
JelleZijlstra Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions Lib/test/test_typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7769,6 +7769,46 @@ class Cat(Animal):
'voice': str,
})

def test_keys_inheritance_with_same_name(self):
class NotTotal(TypedDict, total=False):
a: int

class Total(NotTotal):
a: int

self.assertEqual(NotTotal.__required_keys__, frozenset())
self.assertEqual(NotTotal.__optional_keys__, frozenset(['a']))
self.assertEqual(Total.__required_keys__, frozenset(['a']))
self.assertEqual(Total.__optional_keys__, frozenset())

class Base(TypedDict):
a: NotRequired[int]
b: Required[int]

class Child(Base):
a: Required[int]
b: NotRequired[int]

self.assertEqual(Base.__required_keys__, frozenset(['b']))
self.assertEqual(Base.__optional_keys__, frozenset(['a']))
self.assertEqual(Child.__required_keys__, frozenset(['a']))
self.assertEqual(Child.__optional_keys__, frozenset(['b']))

def test_multiple_inheritance_with_same_key(self):
class Base1(TypedDict):
a: NotRequired[int]

class Base2(TypedDict):
a: Required[str]

class Child(Base1, Base2):
pass

# Last base wins
Copy link
Member

@AlexWaygood AlexWaygood Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it correct that the last base should win? Doesn't that go against how multiple inheritance in Python usually works, where earlier bases in the __bases__ tuple generally have priority?

Python 3.13.0a2+ (heads/main:e9d1360c9a, Nov 24 2023, 11:23:45) [MSC v.1932 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> class Foo:
...     x = 1
...
>>> class Bar:
...     x = 2
...
>>> class Baz(Foo, Bar): pass
...
>>> Baz.x
1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's arguably not correct as you say, but it's the current behavior for __annotations__ and changing that behavior seems difficult.

>>> class A(TypedDict):
...     a: int
... 
>>> class B(TypedDict):
...     a: str
... 
>>> class C(A, B): pass
... 
>>> C.__annotations__
{'a': <class 'str'>}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, that seems unfortunate. As you say, though, better to be internally consistent for now, I guess!

self.assertEqual(Child.__annotations__, {'a': Required[str]})
self.assertEqual(Child.__required_keys__, frozenset(['a']))
self.assertEqual(Child.__optional_keys__, frozenset())

def test_required_notrequired_keys(self):
self.assertEqual(NontotalMovie.__required_keys__,
frozenset({"title"}))
Expand Down
25 changes: 20 additions & 5 deletions Lib/typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2884,8 +2884,14 @@ def __new__(cls, name, bases, ns, total=True):

for base in bases:
annotations.update(base.__dict__.get('__annotations__', {}))
required_keys.update(base.__dict__.get('__required_keys__', ()))
optional_keys.update(base.__dict__.get('__optional_keys__', ()))

base_required = base.__dict__.get('__required_keys__', set())
required_keys |= base_required
optional_keys -= base_required

base_optional = base.__dict__.get('__optional_keys__', set())
required_keys -= base_optional
optional_keys |= base_optional

annotations.update(own_annotations)
for annotation_key, annotation_type in own_annotations.items():
Expand All @@ -2897,14 +2903,23 @@ def __new__(cls, name, bases, ns, total=True):
annotation_origin = get_origin(annotation_type)

if annotation_origin is Required:
required_keys.add(annotation_key)
is_required = True
elif annotation_origin is NotRequired:
optional_keys.add(annotation_key)
elif total:
is_required = False
else:
is_required = total

if is_required:
required_keys.add(annotation_key)
optional_keys.discard(annotation_key)
else:
optional_keys.add(annotation_key)
required_keys.discard(annotation_key)

assert required_keys.isdisjoint(optional_keys), (
f"Required keys overlap with optional keys in {name}:"
f" {required_keys=}, {optional_keys=}"
)
tp_dict.__annotations__ = annotations
tp_dict.__required_keys__ = frozenset(required_keys)
tp_dict.__optional_keys__ = frozenset(optional_keys)
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2023-11-28-20-01-33.gh-issue-112509.QtoKed.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix edge cases that could cause a key to be present in both the
``__required_keys__`` and ``__optional_keys__`` attributes of a
:class:`typing.TypedDict`. Patch by Jelle Zijlstra.