Skip to content

Conversation

@adriangb
Copy link
Contributor

@adriangbadriangb commented Apr 22, 2023

As discussed at the typing summit, non generic TypedDict's completely loose all inheritance information. Funny enough, generic TypedDict's actually preserve it via __orig_bases__ because Generic does that. So this just brings non-generic TypedDicts to be the same.

@adriangbadriangb changed the title Add to non-generic TypedDictAdd __orig_bases__ to non-generic TypedDictApr 22, 2023
@adriangbadriangbforce-pushed the add-orig-bases-typeddict branch from dba5772 to 7a81c98CompareApril 22, 2023 22:31
@adriangbadriangb changed the title Add __orig_bases__ to non-generic TypedDictGH-103699: Add __orig_bases__ to non-generic TypedDictApr 22, 2023
Copy link
Member

@AlexWaygoodAlexWaygood left a comment

Choose a reason for hiding this comment

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

Thank you! Will wait for a second thumbs-up from a typing maintainer before merging, but LGTM

@AlexWaygoodAlexWaygood added type-feature A feature request or enhancement topic-typing 3.12 only security fixes stdlib Standard Library Python modules in the Lib/ directory labels Apr 22, 2023
self.assertEqual(a,{'a': 1})

deftest_orig_bases(self):
self.assertEqual(ChildTotalMovie.__orig_bases__, (ParentNontotalMovie,))
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some more tests? Suggestions: a TypedDict with no bases, one with multiple bases, a generic TypedDict.

Copy link
Member

@AlexWaygoodAlexWaygoodApr 23, 2023

Choose a reason for hiding this comment

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

__orig_bases__ on generic TypedDicts are already tested quite extensively, e.g.

self.assertEqual(A.__orig_bases__, (TypedDict, Generic[T]))

Tests for TypedDicts with no bases and multiple bases sound worth adding, though

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I added quite a few :)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Should I remove the checks on generic TypedDicts then? Or move them all here?

Copy link
Member

Choose a reason for hiding this comment

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

Should I remove the checks on generic TypedDicts then? Or move them all here?

I think it's probably fine as is, actually. A little bit of duplication in tests isn't a massive issue, and it makes sense to both:

1). Test __orig_bases__ in the test method for testing generic TypedDicts
2). Test generic TypedDicts in the test method for testing __orig_bases__

Copy link
Member

@JelleZijlstraJelleZijlstra left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

I might want documentation for this attribute, but we have another PR open to add typing.get_original_bases, so we can cover TypedDicts there too.

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 23, 2023

Hmm, one edge case that isn't currently covered in the tests is call-based TypedDicts. I'm not sure if this behaviour is correct:

>>> from typing import TypedDict >>> classFoo(TypedDict): ... ... >>> Foo2 = TypedDict("Foo2",{}) >>> Foo.__orig_bases__ (<function TypedDict at 0x000002796B986410>,) >>> Foo2.__orig_bases__ ()

Here's the current behaviour with call-based NamedTuples, which is the closest stdlib analogue:

>>> from typing import NamedTuple >>> classBar(NamedTuple): ... ... >>> Bar2 = NamedTuple("Bar2",{}) >>> Bar.__orig_bases__ (<function NamedTuple at 0x000002796B986200>,) >>> Bar2.__orig_bases__ Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: type object 'Bar2' has no attribute '__orig_bases__'

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 23, 2023

We could match the behaviour of NamedTuple here (adding __orig_bases__ only for class-based TypedDicts -- which I think is probably the correct thing to do?) by applying this diff to your PR branch:

diff --git a/Lib/typing.py b/Lib/typing.py index 8408edc49a..660e89cb30 100644 --- a/Lib/typing.py+++ b/Lib/typing.py@@ -3107,7 +3107,9 @@ class body be required. # Setting correct module is necessary to make typed dict classes pickleable. ns['__module__'] = module - return _TypedDictMeta(typename, (), ns, total=total)+ td = _TypedDictMeta(typename, (), ns, total=total)+ del td.__orig_bases__+ return td _TypedDict = type.__new__(_TypedDictMeta, 'TypedDict', (),{}) TypedDict.__mro_entries__ = lambda bases: (_TypedDict,)

@adriangb
Copy link
ContributorAuthor

I'm happy to do that, but I think the new behavior is better. Even better would be if the call approaches matched the class-based approaches, but certainly the attribute always existing is better than it only existing sometimes, right?

@JelleZijlstra
Copy link
Member

I think the conclusion from the in-person discussion was that we should make it return (TypedDict,) for call-based TypedDicts, right?

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 23, 2023

I think the conclusion from the in-person discussion was that we should make it return (TypedDict,) for call-based TypedDicts, right?

Yes, that sounds good. But we should fix it for call-based NamedTuples as well, so that it's consistent between call-based NamedTuples and call-based TypedDicts. (That could be done in a separate PR, but I also wouldn't mind doing it in the same PR.)

@adriangb
Copy link
ContributorAuthor

Yup that’s what I recall as well.

Alex you did bring up the point about inline TypedDicts (which is being discussed in the mailing list and it seems like has a good chance of moving to a PEP), and how it might not make sense to have TypedDict in the bases for inline TypedDicts. Philosophically, I agree with that. But in practice, for the things that are going to be digging into the bases of a TypedDict, it is not a problem to have or not have TypedDict itself in the bases. So I think we should move forward with this keeping it as is to minimize changes and deal with inline TypedDicts if/when they show up since even if they behave different it wouldn’t really be a huge deal (and we can probably just get them to behave the same if we want to).

@AlexWaygood
Copy link
Member

Alex you did bring up the point about inline TypedDicts

I think @hauntsaninja brought up the point about inline TypedDicts, but I agree with your conclusion here!

@JelleZijlstra
Copy link
Member

I don't know exactly how inline TypedDicts will work (for one, they won't have a name); that'll be up to Nikita to specify in the PEP. But yeah, we can discuss that when the time comes to actually implement it.

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 23, 2023

Okay, applying this patch to your PR branch appears to make call-based TypedDicts and NamedTuples consistent with class-based TypedDicts and NamedTuples when it comes to __orig_bases__:

diff --git a/Lib/typing.py b/Lib/typing.py index 8408edc49a..354bc80eb3 100644 --- a/Lib/typing.py+++ b/Lib/typing.py@@ -2962,7 +2962,9 @@ class Employee(NamedTuple): elif kwargs: raise TypeError("Either list of fields or keywords" " can be provided to NamedTuple, not both") - return _make_nmtuple(typename, fields, module=_caller())+ nt = _make_nmtuple(typename, fields, module=_caller())+ nt.__orig_bases__ = (NamedTuple,)+ return nt _NamedTuple = type.__new__(NamedTupleMeta, 'NamedTuple', (),{}) @@ -3107,7 +3109,9 @@ class body be required. # Setting correct module is necessary to make typed dict classes pickleable. ns['__module__'] = module - return _TypedDictMeta(typename, (), ns, total=total)+ td = _TypedDictMeta(typename, (), ns, total=total)+ td.__orig_bases__ = (TypedDict,)+ return td _TypedDict = type.__new__(_TypedDictMeta, 'TypedDict', (),{}) TypedDict.__mro_entries__ = lambda bases: (_TypedDict,)

(Needs tests as well)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@AlexWaygood
Copy link
Member

Okay, applying this patch to your PR branch appears to make call-based TypedDicts and NamedTuples consistent with class-based TypedDicts and NamedTuples when it comes to __orig_bases__:

This, uh, would introduce a discrepancy between call-based collections.namedtuple and call-based typing.NamedTuple:

>>> import collections as c, typing as t >>> classFoo(c.namedtuple('Foo',{})): ... ... >>> classBar(t.NamedTuple('Bar',{})): ... ... >>> Foo.__orig_bases__ Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: type object 'Foo' has no attribute '__orig_bases__' >>> Bar.__orig_bases__ (<function NamedTuple at 0x000001426A124730>,)

But I think we can live with that...

…izCjc.rst Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@adriangb
Copy link
ContributorAuthor

adriangb commented Apr 23, 2023

I added tests for call-based TypedDict and various NamedTuple scenarios in 631d428, your patch works great, thank you. I previously pushed 7224ddd but then remembered that NamedTuple inheritance is not really a thing.

But I think we can live with that...

I agree, especially given that (1) it can probably be fixed there as well and (2) I think we all agree that the new behavior is better.

@AlexWaygoodAlexWaygood changed the title GH-103699: Add __orig_bases__ to non-generic TypedDictGH-103699: Add __orig_bases__ to various typing classesApr 23, 2023
Copy link
Member

@AlexWaygoodAlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM.

It surprises me that Bar.__orig_bases__ in this case is (NamedTuple,), since the "original bases" of Bar are not (NamedTuple,) (they're (Foo,)):

>>> from typing import NamedTuple >>> classFoo(NamedTuple): ... ... >>> classBar(Foo): ... ... >>> Bar.__orig_bases__ (<function NamedTuple at 0x0000016E1F4ACEA0>,)

However, this behaviour is perfectly consistent with how inheriting from Generic[T] works:

>>> from typing import Generic, TypeVar >>> T = TypeVar("T") >>> classBaz(Generic[T]): ... ... >>> classEggs(Baz): ... ... >>> Eggs.__orig_bases__ (typing.Generic[~T],)

So I think it's fine.

…izCjc.rst Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3.12only security fixesstdlibStandard Library Python modules in the Lib/ directorytopic-typingtype-featureA feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@adriangb@AlexWaygood@JelleZijlstra@gvanrossum@bedevere-bot