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-111874: Call __set_name__ on objects that define the method inside a typing.NamedTuple class dictionary as part of the creation of that class #111876

Merged
merged 17 commits into from Nov 27, 2023

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Nov 9, 2023

…d inside a `typing.NamedTuple` class dictionary as part of the creation of that class
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM, but I have few minor suggestions.

Lib/typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Show resolved Hide resolved
@AlexWaygood
Copy link
Member Author

Thanks @serhiy-storchaka! I applied your suggestions; would you mind taking another look? I discovered another interesting thing in the process: __set_name__ was also not being called for the values of annotated fields (which become namedtuple members). I changed that so that __set_name__ is called on these field values also. It's consistent with what e.g. dataclasses does:

Python 3.13.0a1+ (heads/main:e5dfcc2b6e, Nov 15 2023, 17:23:51) [MSC v.1932 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from dataclasses import dataclass
>>> class Vanilla:
...     def __set_name__(self, owner, name):
...         self.name = name
...
>>> @dataclass
... class Foo:
...     v: Vanilla = Vanilla()
...
>>> f = Foo()
>>> f.v.name
'v'

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

I am not sure that __set_name__ should be called for annotated fields.

@AlexWaygood
Copy link
Member Author

I am not sure that __set_name__ should be called for annotated fields.

Yes, I could be persuaded either way there.

@AlexWaygood AlexWaygood removed needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Nov 16, 2023
Lib/test/test_typing.py Outdated Show resolved Hide resolved
setattr(nm_tpl, key, val)
try:
set_name = type(val).__set_name__
except AttributeError:
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth adding a test for the case where this yields some other exception (e.g. due to a weird metaclass)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Seems like for normal classes, if any strange exception is raised when trying to lookup __set_name__, the interpreter just swallows it and moves on:

>>> class Meta(type):
...     def __getattribute__(self, attr):
...         if attr == "__set_name__":
...             raise BaseException("NO")
...
>>> class VeryAnnoying(metaclass=Meta): pass
...
>>> class Foo:
...     attr = VeryAnnoying()
...
>>>

We should do the same here (though I don't want to swallow BaseExceptions, since that would catch KeyboardInterrupt, SystemExit, etc. -- I think I'll just use except Exception).

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 1909fd5

Copy link
Member

Choose a reason for hiding this comment

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

Seems like for normal classes, if any strange exception is raised when trying to lookup __set_name__, the interpreter just swallows it and moves on:

It looks like a bug. I do not think it should be reproduced here. It is better to fix it for normal classes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I opened #112453, and will revert the changes made here that reproduce the bug

Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

Few minor suggestions which you can ignore.

Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
@AlexWaygood
Copy link
Member Author

Thanks for the reviews!

@AlexWaygood AlexWaygood enabled auto-merge (squash) November 27, 2023 16:20
@AlexWaygood AlexWaygood merged commit 22e411e into python:main Nov 27, 2023
27 checks passed
@AlexWaygood AlexWaygood deleted the namedtuple-setname branch November 27, 2023 16:34
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Dec 11, 2023
# Release 4.9.0 (December 9, 2023)

This feature release adds `typing_extensions.ReadOnly`, as specified
by PEP 705, and makes various other improvements, especially to
`@typing_extensions.deprecated()`.

There are no changes since 4.9.0rc1.

# Release 4.9.0rc1 (November 29, 2023)

- Add support for PEP 705, adding `typing_extensions.ReadOnly`. Patch
  by Jelle Zijlstra.
- All parameters on `NewType.__call__` are now positional-only. This means that
  the signature of `typing_extensions.NewType.__call__` now exactly matches the
  signature of `typing.NewType.__call__`. Patch by Alex Waygood.
- Fix bug with using `@deprecated` on a mixin class. Inheriting from a
  deprecated class now raises a `DeprecationWarning`. Patch by Jelle Zijlstra.
- `@deprecated` now gives a better error message if you pass a non-`str`
  argument to the `msg` parameter. Patch by Alex Waygood.
- `@deprecated` is now implemented as a class for better introspectability.
  Patch by Jelle Zijlstra.
- Exclude `__match_args__` from `Protocol` members.
  Backport of python/cpython#110683 by Nikita Sobolev.
- When creating a `typing_extensions.NamedTuple` class, ensure `__set_name__`
  is called on all objects that define `__set_name__` and exist in the values
  of the `NamedTuple` class's class dictionary. Patch by Alex Waygood,
  backporting python/cpython#111876.
- Improve the error message when trying to call `issubclass()` against a
  `Protocol` that has non-method members. Patch by Alex Waygood (backporting
  python/cpython#112344, by Randolph Scholz).
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
…d inside a `typing.NamedTuple` class dictionary as part of the creation of that class (python#111876)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
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.

Objects in NamedTuple class namespaces don't have __set_name__ called on them
4 participants