Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-85795: Add support forsuper() inNamedTuple 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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
bswck wants to merge16 commits intopython:mainfrombswck:fix-namedtuple-classcell-bug

Conversation

@bswck
Copy link
Contributor

@bswckbswck commentedJan 27, 2025
edited
Loading

Fi​xesgh-85795.

A private parameter_classcell was added tocollections.namedtuple() and its wrappers intyping in order to deliver it to the targetedtype.__new__.

It's the easiest approach here.

__classcell__ ispopped 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 issuegh-129343 and the PR associated with it.


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

j-g00da reacted with thumbs up emoji
@bswck
Copy link
ContributorAuthor

This needs a backport to 3.12 and 3.13.

@bswck
Copy link
ContributorAuthor

bswck commentedJan 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 relying onsuper() 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 withtyping_extensions.NamedTuple to allow gradual introduction, but library maintainers would have to know it up front to not rely ontyping.NamedTuple directly in 3.12-3.13 and instead usetyping_extensions.NamedTuple, which kills the point of backporting this if we can just add this functionality totyping_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 seepeace of mind a good argument.

@bswck
Copy link
ContributorAuthor

bswck commentedJan 28, 2025
edited
Loading

CPython disallows

classFoo:__classcell__=None

but this implementation allows

fromtypingimportNamedTupleclassFoo(NamedTuple):__classcell__=None

should I pop from the origin class namespace with a sentinel object fallback instead ofNone?
So that we let the error propagate if someone sets__classcell__ toNone explicitly.

Errors should never pass silently, unless explicitly silenced.

@AlexWaygood
Copy link
Member

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

should I pop from the origin class namespace with a sentinel object fallback instead ofNone?
So that we let the error propagate if someone sets__classcell__ toNone explicitly.

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

Copy link
Member

@ZeroIntensityZeroIntensity 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
ContributorAuthor

bswck commentedJan 28, 2025
edited
Loading

I like@ZeroIntensity's idea of movingcollections.namedtuple tocollections._namedtuple, adding the param tocollections._namedtuple, and then wrapping it in a newcollections.namedtuple with the original signature preserved, without the_classcell param.

AlexWaygood reacted with thumbs up emoji

@bswckbswckforce-pushed thefix-namedtuple-classcell-bug branch from20bfaeb to074df4cCompareJanuary 29, 2025 10:04
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.

Nice. This LGTM now.

@AlexWaygood
Copy link
Member

oh, except some tests are failing

bswckand others added2 commitsJanuary 29, 2025 13:30
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@bswckbswckforce-pushed thefix-namedtuple-classcell-bug branch from0d6dcbb toee93519CompareJanuary 29, 2025 12:36
@bswck
Copy link
ContributorAuthor

bswck commentedJan 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 becomegetframe(2)) :)

AlexWaygood reacted with heart emoji

Copy link
Member

@ZeroIntensityZeroIntensity 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.

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@bswck
Copy link
ContributorAuthor

bswck commentedJan 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 intyping.NamedTuple subclasses, as in

# See issue #23722
# Overwriting __classcell__ with nonsense is explicitly prohibited
classMeta(type):
def__new__(cls,name,bases,namespace,cell):
namespace['__classcell__']=cell
returnsuper().__new__(cls,name,bases,namespace)
forbad_cellin (None,0,"",object()):
withself.subTest(bad_cell=bad_cell):
withself.assertRaises(TypeError):
classA(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__ intyping.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 commentedFeb 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 norstackoffset is intrinsic to whatnamedtuple 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.
.

@rhettingerrhettinger removed their assignmentFeb 6, 2025
@bswck
Copy link
ContributorAuthor

bswck commentedFeb 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 sinceNamedTuple subclasses cannot be inherited from. What I'd like to stress though is that IMO we can'tjust document thatsuper() is unsupported intyping.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__ orsuper() in typed namedtuples is not supported"?
That would touch onlytyping.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 "usingclass 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
ContributorAuthor

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@ZeroIntensityZeroIntensityZeroIntensity approved these changes

@AlexWaygoodAlexWaygoodAlexWaygood approved these changes

@JelleZijlstraJelleZijlstraJelleZijlstra approved these changes

@rhettingerrhettingerAwaiting requested review from rhettingerrhettinger is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@bswck@AlexWaygood@rhettinger@JelleZijlstra@ZeroIntensity

[8]ページ先頭

©2009-2025 Movatter.jp