8000 gh-85795: Add support for `super()` in `NamedTuple` subclasses by bswck · Pull Request #129352 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-85795: Add support for super() in NamedTuple subclasses #129352

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
Closed
wants to merge 16 commits into from
Closed

gh-85795: Add support for super() in NamedTuple subclasses #129352

wants to merge 16 commits into from

Conversation

bswck
Copy link
Contributor
@bswck bswck commented Jan 27, 2025 •
edited
Loading

Fi​xes gh-85795.

A private parameter _classcell was added to collections.namedtuple() and its wrappers in typing in order to deliver it to the targeted type.__new__.

It's the easiest approach here.

__classcell__ is popped from the original class namespace, as it would otherwise leak to the end class (metaclasses should clean up the __classcell__ attribute).

It is expected not to be added to typeshed, as the parameter is intended for internal use only.

After this PR, please consider a follow-up issue gh-129343 and the PR associated with it.

📚 Documentation preview 📚: https://cpython-previews--129352.org.readthedocs.build/

@bedevere-app bedevere-app bot mentioned this pull request Jan 27, 2025
Closed
@bswck
Copy link
Contributor Author
bswck commented Jan 27, 2025

This needs a backport to 3.12 and 3.13.

@bswck bswck mentioned this pull request Jan 27, 2025
Closed
@ZeroIntensity ZeroIntensity self-requested a review January 28, 2025 03:21
@bswck
Copy link
Contributor Author
bswck commented Jan 28, 2025 •
edited
Loading

Changed my mind, let's not backport it :)
Thanks @ZeroIntensity and @JelleZijlstra for convincing me this.

  1. If we backport this, users can start 8000 relying on super() working in typed namedtuples in versions ~=3.12.9 and ~=3.13.2 in libraries. That would cause other users (>=3.12.0,<3.12.9; >=3.13.0,<3.13.2) to get hit with this strange error message, and no hint on how to fix it. A way to navigate this is distribute the bugfix with typing_extensions.NamedTuple to allow gradual introduction, but library maintainers would have to know it up front to not rely on typing.NamedTuple directly in 3.12-3.13 and instead use typing_extensions.NamedTuple, which kills the point of backporting this if we can just add this functionality to typing_extensions and not backport anything in cpython itself.

  2. A potential to break a vague usage we don't know of. I don't see it an appealing argument, but OTOH I see peace of mind a good argument.

@rhettinger rhettinger self-assigned this Jan 28, 2025
@bswck
Copy link
Contributor Author
bswck commented Jan 28, 2025 •
edited
Loading

CPython disallows

class Foo:
    __classcell__ = None

but this implementation allows

from typing import NamedTuple

class Foo(NamedTuple):
    __classcell__ = None

should I pop from the origin class namespace with a sentinel object fallback instead of None?
So that we let the error propagate if someone sets __classcell__ to None explicitly.

Errors should never pass silently, unless explicitly silenced.

AlexWaygood
AlexWaygood reviewed Jan 28, 2025
View reviewed changes
@AlexWaygood
Copy link
Member

Ideally I feel like this fix would not involve adding a new parameter to collections.namedtuple(), since this issue only really affects typing.NamedTuple. However, I can't see another solution to the problem. I suppose the other option would be just to document that super() doesn't work inside methods on typing.NamedTuples created using the class syntax.

should I pop from the origin class namespace with a sentinel object fallback instead of None?
So that we let the error propagate if someone sets __classcell__ to None explicitly.

It seems like an unlikely situation, but I suppose this would be strictly-speaking more correct!

ZeroIntensity
ZeroIntensity reviewed Jan 28, 2025
View reviewed changes
Copy link
Member
@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, but I have a few thoughts.

@bswck
Copy link
Contributor Author
bswck commented Jan 28, 2025 •
edited
Loading

I like @ZeroIntensity's idea of moving collections.namedtuple to collections._namedtuple, adding the param to collections._namedtuple, and then wrapping it in a new collections.namedtuple with the original signature preserved, without the _classcell param.

@bswck bswck force-pushed the fix-namedtuple-classcell-bug branch from 20bfaeb to 074df4c Compare January 29, 2025 10:04
AlexWaygood
AlexWaygood approved these changes Jan 29, 2025
View reviewed changes
Copy link
Member
@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Nice. This LGTM now.

@AlexWaygood
Copy link
Member

oh, except some tests are failing

bswck and others added 2 commits January 29, 2025 13:30
@bswck @AlexWaygood
Add a comment discussing reasons for __classcell__ manipulation
998474d 
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@bswck @AlexWaygood
Reformat for optimal line length
ee93519 
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@bswck bswck force-pushed the fix-namedtuple-classcell-bug branch from 0d6dcbb to ee93519 Compare January 29, 2025 12:36
@bswck
Copy link
Contributor Author
bswck commented Jan 29, 2025 •
edited
Loading

oh, except some tests are failing

Hah! How ignorant of me.

I added another frame by wrapping the function and didn't fixup the existing frame refs (getframe(1) should become getframe(2)) :)

ZeroIntensity
ZeroIntensity approved these changes Jan 29, 2025
View reviewed changes
Copy link
Member
@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

LGTM, with one tiny suggestion.

@bswck @ZeroIntensity
Use next version ref instead of 3.14
c2a0c50 
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@bswck
< 9E88 details class="details-overlay details-reset position-relative d-inline-block"> Copy link
Contributor Author
bswck commented Jan 29, 2025 •
edited
Loading

Thanks @AlexWaygood and @ZeroIntensity for working through this and finding a better approach!

One last thought is whether I should test __classcell__ nonsense overwrites errors are preserved in typing.NamedTuple subclasses, as in

cpython/Lib/test/test_super.py

Lines 262 to 273 in c931d75

# See issue #23722
# Overwriting __classcell__ with nonsense is explicitly prohibited
class Meta(type):
def __new__(cls, name, bases, namespace, cell):
namespace['__classcell__'] = cell
return super().__new__(cls, name, bases, namespace)
for bad_cell in (None, 0, "", object()):
with self.subTest(bad_cell=bad_cell):
with self.assertRaises(TypeError):
class A(metaclass=Meta, cell=bad_cell):
pass

I'm leaving this decision to @rhettinger since he self-assigned this. I personally believe a possible regression allowing nonsense overwrites of __classcell__ in typing.NamedTuple subclasses is not harmful by any means and niche enough that and I wouldn't care about that case.

@rhettinger Please review. :)

@rhettinger
Copy link
Contributor
rhettinger commented Feb 6, 2025 •
edited
Loading

I don't think this is worth it and instead prefer Alex's suggestion "to document that super() doesn't work inside methods on typing.NamedTuples created using the class syntax".

Named tuples are thin classes, there isn't much in the way of reusable code. Likely, that is why overriding rather than extending is the norm. Also dataclasses seem to be preferred when people are trying to do anything more interesting.

I do admire the analysis and effort that was put into this PR. The code looks correct. However, I think almost no one needs this. The edit is a really interesting (and brilliant) but slightly icky magic trick that isn't that easy to explain. Neither _classcell nor stackoffset is intrinsic to what namedtuple is trying to do. I prefer that they were not there.

FWIW, I'm only -0 on this one. The thought is to avoid janky code and not skate onto thin ice when you don't have to.
.

@rhettinger rhettinger removed their assignment Feb 6, 2025
@bswck
Copy link
Contributor Author
bswck commented Feb 6, 2025 •
edited
Loading

@rhettinger Thank you. I must say I agree with you for the most part, I also don't feel too strong about the general necessity for this to work, especially since NamedTuple subclasses cannot be inherited from. What I'd like to stress though is that IMO we can't just document that super() is unsupported in typing.NamedTuple subclasses—the error message is very strange upon using one of the basic features of Python:
__class__ not set defining 'Foo' as <class 'Foo'>. Was __classcell__ propagated to type.__new__?

How about I make a PR to detect classcells and raise a proper, easy to comprehend error message like "using __class__ or super() in typed namedtuples is not supported"?
That would touch only typing.NamedTuple, addressing the root cause and allowing others to save time and confusion.

@rhettinger
Copy link
Contributor

How about I make a PR to detect classcells and raise a proper, easy to comprehend error message like "using class or super() in typed namedtuples is not supported"?

That would touch only typing.NamedTuple, addressing the root cause and allowing others to save time and confusion.

+1 That would tremendously improve the user experience.

@bswck
Copy link
Contributor Author
bswck commented Feb 6, 2025

I'm therefore closing this PR and will create another one with the improved error message.

Thank y'all for reviews, this bug was very fun to work on, so no regrets though it didn't make a cut :)

@bswck bswck closed this Feb 6, 2025
@bswck bswck mentioned this pull request Feb 13, 2025
Merged
@bswck bswck deleted the fix-namedtuple-classcell-bug branch February 14, 2025 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZeroIntensity ZeroIntensity ZeroIntensity approved these changes

@AlexWaygood AlexWaygood AlexWaygood approved these changes

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner

Assignees
No one assigned
Labels
awaiting merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bswck @AlexWaygood @rhettinger @JelleZijlstra @ZeroIntensity
0