Skip to content
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

Jump to bottom

gh-109543: Remove unnecessary hasattr check #109544

Merged
merged 2 commits into from
Sep 20, 2023
Merged

gh-109543: Remove unnecessary hasattr check #109544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5821520
gh-109543: Remove unnecessary hasattr check
JelleZijlstra Sep 18, 2023
5ccf8ff
Merge branch 'main' into tdtotal
AlexWaygood Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 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 @@ -7586,6 +7586,17 @@ def test_total(self):
self.assertEqual(Options.__required_keys__, frozenset())
self.assertEqual(Options.__optional_keys__, {'log_level', 'log_path'})

def test_total_inherits_non_total(self):
class TD1(TypedDict, total=False):
a: int

self.assertIs(TD1.__total__, False)

class TD2(TD1):
b: str

self.assertIs(TD2.__total__, True)
Copy link
Member

Choose a reason for hiding this comment

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

I know you're not changing behaviour here, but are the precise semantics here specified anywhere? Mypy has some interesting behaviour here -- it treats all the keys present in TD2 as required, but all the keys present in TD1 as not required. That implies to me that TD2 should not be seen as a "total" TypedDict.

https://mypy-play.net/?mypy=latest&python=3.11&gist=e7ef88dd8c2b5697297c4739d471ac45

The __total__ attribute seems to be pretty meaningless these days, though, in a post-PEP-655 world:

>>> from typing import *
>>> class Foo(TypedDict):
...     x: Required[int]
...     y: NotRequired[str]
...
>>> Foo.__total__
True

Maybe we should treat that as a bug and make it meaningful. Or maybe we should just be clearer in the documentation that __total__ doesn't indicate whether or not all keys in the TypedDict are required or not, it just literally indicates whether that specific TypedDict was constructed using total=True (but I'm not sure why that would be useful to anybody).

Copy link
Member Author

Choose a reason for hiding this comment

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

The mypy behavior is right; that was the way to mix Required and NotRequired keys before PEP 655.

Agree that __total__ isn't that useful and you should generally look at __required_keys__ and __optional_keys__. However, I think the current behavior is right. If you do want to use __total__ for introspection, you should look not just at the current class's attribute, but also those of base classes.

Copy link
Member

Choose a reason for hiding this comment

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

It's strange to me that we iterate through the base classes as part of the class construction in order to make sure we have accurate __required_keys__, __optional_keys__ and __annotations__ attributes on the produced class, but we don't do the same for the __total__ attribute. (We probably shouldn't be doing that for the __annotations__ attribute, since that's not the way __annotations__ works on any other class... but we do it anyway.) It seems very inconsistent to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Submitted #109547 with some enhancements to the docs.


def test_optional_keys(self):
class Point2Dor3D(Point2D, total=False):
z: int
Expand Down
3 changes: 1 addition & 2 deletions Lib/typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2886,8 +2886,7 @@ def __new__(cls, name, bases, ns, total=True):
tp_dict.__annotations__ = annotations
tp_dict.__required_keys__ = frozenset(required_keys)
tp_dict.__optional_keys__ = frozenset(optional_keys)
if not hasattr(tp_dict, '__total__'):
tp_dict.__total__ = total
tp_dict.__total__ = total
return tp_dict

__call__ = dict # static method
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2023-09-18-07-43-22.gh-issue-109543.1tOGoV.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Remove unnecessary :func:`hasattr` check during :data:`typing.TypedDict`
creation.