GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-103699: Add `__orig_bases__` to various typing classes by adriangb · Pull Request #103698 · python/cpython

Conversation

@adriangb

@adriangb adriangb commented Apr 22, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

  • Issue: Add __orig_bases__ to non-generic TypedDict #103699

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@adriangb
adriangb requested review from AlexWaygood, Fidget-Spinner, JelleZijlstra and gvanrossum as code owners April 22, 2023 22:31
@bedevere-bot bedevere-bot added the awaiting review label Apr 22, 2023
@adriangb adriangb changed the title Add to non-generic TypedDict Add __orig_bases__ to non-generic TypedDict Apr 22, 2023
@adriangb
Add __orig_bases__ to non-generic TypedDict
7a81c98
@adriangb
adriangb force-pushed the add-orig-bases-typeddict branch from dba5772 to 7a81c98 Compare April 22, 2023 22:31
@adriangb adriangb changed the title Add __orig_bases__ to non-generic TypedDict GH-103699: Add __orig_bases__ to non-generic TypedDict Apr 22, 2023
@bedevere-bot bedevere-bot mentioned this pull request Apr 22, 2023
Add __orig_bases__ to non-generic TypedDict #103699
Closed
blurb-it Bot and others added 2 commits April 22, 2023 22:37
@blurb-it
📜🤖 Added by blurb_it.
270cadf
@adriangb
Use double backticks
29b643d
AlexWaygood
AlexWaygood approved these changes Apr 22, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Apr 22, 2023
@AlexWaygood AlexWaygood 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
AlexWaygood
AlexWaygood reviewed Apr 22, 2023
View reviewed changes
Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum
gvanrossum approved these changes Apr 22, 2023
View reviewed changes
@AlexWaygood
remove whitespace
4037df1
@AlexWaygood AlexWaygood added the 🤖 automerge label Apr 22, 2023
JelleZijlstra
JelleZijlstra reviewed Apr 23, 2023
View reviewed changes
Comment thread Lib/test/test_typing.py Outdated
self.assertEqual(a, {'a': 1})

def test_orig_bases(self):
self.assertEqual(ChildTotalMovie.__orig_bases__, (ParentNontotalMovie,))

@JelleZijlstra JelleZijlstra Apr 23, 2023

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

adriangb reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@AlexWaygood AlexWaygood Apr 23, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

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

cpython/Lib/test/test_typing.py

Line 7017 in 4037df1

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@adriangb adriangb Apr 23, 2023

Copy link
Copy Markdown
Contributor Author

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 added quite a few :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@adriangb adriangb Apr 23, 2023

Copy link
Copy Markdown
Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@AlexWaygood AlexWaygood Apr 23, 2023

Copy link
Copy Markdown
Member

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.

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__

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra and adriangb reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@AlexWaygood AlexWaygood removed the 🤖 automerge label Apr 23, 2023
adriangb added 2 commits April 22, 2023 18:17
@adriangb
add more test cases
19569f9
@adriangb
one more test
b777373
JelleZijlstra
JelleZijlstra approved these changes Apr 23, 2023
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood and adriangb reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@AlexWaygood AlexWaygood added the 🤖 automerge label Apr 23, 2023
@AlexWaygood

AlexWaygood commented Apr 23, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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
>>> class Foo(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
>>> class Bar(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__'
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood AlexWaygood removed the 🤖 automerge label Apr 23, 2023
@AlexWaygood

AlexWaygood commented Apr 23, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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,)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@adriangb

adriangb commented Apr 23, 2023

Copy link
Copy Markdown
Contributor Author

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JelleZijlstra

JelleZijlstra commented Apr 23, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood

AlexWaygood commented Apr 23, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@adriangb

adriangb commented Apr 23, 2023

Copy link
Copy Markdown
Contributor Author

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood

AlexWaygood commented Apr 23, 2023

Copy link
Copy Markdown
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!

adriangb reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JelleZijlstra

JelleZijlstra commented Apr 23, 2023

Copy link
Copy Markdown
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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood

AlexWaygood commented Apr 23, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
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__:

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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@adriangb @AlexWaygood
Make call-based NamedTuples behave like call-based TypedDicts
4c9ee8c
Co-authored-by: Alex Waygood <[email protected]>
@AlexWaygood

AlexWaygood commented Apr 23, 2023

Copy link
Copy Markdown
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
>>> class Foo(c.namedtuple('Foo', {})): ...
...
>>> class Bar(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...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

adriangb added 2 commits April 22, 2023 23:07
@adriangb
Add (failing) tests
7224ddd
@adriangb
More tests
631d428
AlexWaygood
AlexWaygood reviewed Apr 23, 2023
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2023-04-22-22-37-39.gh-issue-103699.NizCjc.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@adriangb @AlexWaygood
Update Misc/NEWS.d/next/Library/2023-04-22-22-37-39.gh-issue-103699.N…
58701ac
…izCjc.rst

Co-authored-by: Alex Waygood <[email protected]>
@adriangb

adriangb commented Apr 23, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood AlexWaygood changed the title GH-103699: Add __orig_bases__ to non-generic TypedDict GH-103699: Add __orig_bases__ to various typing classes Apr 23, 2023
AlexWaygood
AlexWaygood approved these changes Apr 23, 2023
View reviewed changes

@AlexWaygood AlexWaygood left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

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
>>> class Foo(NamedTuple): ...
...
>>> class Bar(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")
>>> class Baz(Generic[T]): ...
...
>>> class Eggs(Baz): ...
...
>>> Eggs.__orig_bases__
(typing.Generic[~T],)

So I think it's fine.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@AlexWaygood
AlexWaygood requested a review from JelleZijlstra April 23, 2023 05:48
@AlexWaygood AlexWaygood added skip news and removed skip news labels Apr 23, 2023
JelleZijlstra
JelleZijlstra reviewed Apr 23, 2023
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2023-04-22-22-37-39.gh-issue-103699.NizCjc.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@adriangb @JelleZijlstra
Update Misc/NEWS.d/next/Library/2023-04-22-22-37-39.gh-issue-103699.N…
3fe75eb
…izCjc.rst

Co-authored-by: Jelle Zijlstra <[email protected]>
JelleZijlstra
JelleZijlstra approved these changes Apr 23, 2023
View reviewed changes
@AlexWaygood
AlexWaygood merged commit 0056701 into python:main Apr 23, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Apr 23, 2023
@adriangb adriangb mentioned this pull request Apr 23, 2023
Add __orig_bases__ to TypedDict python/typing_extensions#150
Merged
AlexWaygood added a commit to Gobot1234/cpython that referenced this pull request Apr 23, 2023
@AlexWaygood
More tests and docs following python#103698
cb21a65
@adriangb adriangb mentioned this pull request Jul 2, 2023
Fix validate_call schema generation and doc error for TypedDict pydantic/pydantic#6370
Merged
@adriangb adriangb mentioned this pull request Jul 13, 2023
Contradictory typing.TypedDict >=3.11 or >=3.12 support pydantic/pydantic#6645
Closed
1 task done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes
@gvanrossum gvanrossum gvanrossum approved these changes
@AlexWaygood AlexWaygood AlexWaygood approved these changes
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

3.12 only security fixes stdlib Standard Library Python modules in the Lib/ directory topic-typing type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@adriangb @AlexWaygood @JelleZijlstra @gvanrossum @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.