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.

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

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

bswck
Copy link
Contributor

@bswck bswck commented Jan 27, 2025

Fixes 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/

@bswck
Copy link
Contributor Author

bswck commented Jan 27, 2025

This needs a backport to 3.12 and 3.13.

@bswck
Copy link
Contributor Author

bswck commented Jan 28, 2025

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

@bswck
Copy link
Contributor Author

bswck commented Jan 28, 2025

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.

Doc/library/typing.rst Outdated Show resolved Hide resolved
@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!

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.

Lib/test/test_typing.py Show resolved Hide resolved
Doc/library/typing.rst Outdated Show resolved Hide resolved
Lib/collections/__init__.py Outdated Show resolved Hide resolved
@bswck
Copy link
Contributor Author

bswck commented Jan 28, 2025

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

Lib/collections/__init__.py Show resolved Hide resolved
Lib/collections/__init__.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
@AlexWaygood
Copy link
Member

oh, except some tests are failing

@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

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

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.

Doc/library/typing.rst Outdated Show resolved Hide resolved
@bswck
Copy link
Contributor Author

bswck commented Jan 29, 2025

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

__class__ not set defining 'X' as <class '__main__.X'>
5 participants