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.

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

gh-123777: Allow __orig_class__ to be accessed in __init__ #123878

Closed
wants to merge 4 commits into from
+34 −1
Closed

gh-123777: Allow __orig_class__ to be accessed in __init__ #123878

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e6a84cc
Allow __orig_class__ to be used in the __init__ of pure-Python types.
ZeroIntensity Sep 6, 2024
8cc02fe
Add NEWS entry.
ZeroIntensity Sep 9, 2024
0084601
Switch check to inspect.isfunction rather than checking for PyType_Ge…
ZeroIntensity Sep 9, 2024
19c0b1e
Add tests and fix implementation.
ZeroIntensity Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion Lib/typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1290,13 +1290,30 @@ def __call__(self, *args, **kwargs):
if not self._inst:
raise TypeError(f"Type {self._name} cannot be instantiated; "
f"use {self.__origin__.__name__}() instead")
result = self.__origin__(*args, **kwargs)
clas = self.__origin__
is_python_type = clas.__new__ is type.__new__
ZeroIntensity marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to do what you want it to do:

>>> class A: pass
... 
>>> A.__new__ is type.__new__
False

This PR doesn't include tests; to get it merged we'll definitely need tests.

I'm also not convinced we should change this at all, since it feels like a risky change at this point.

Copy link
Member

Choose a reason for hiding this comment

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

You may want to compare against object.__new__ instead, but that still doesn't directly check for "Python types", and in any case I'm not sure it's a good idea to branch on whether it's a Python type.

>>> from typing import TypeVar
>>> TypeVar.__new__
<built-in method __new__ of type object at 0x14c817e30>
>>> A.__new__ is object.__new__
True

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, Alex had the same concern, I just pushed a better fix instead of type.__new__.

I'm also not convinced we should change this at all, since it feels like a risky change at this point.

Very much agreeing there, but there does seem to be some demand from the typing users. If this causes a regression somewhere, we could always roll back the changes!

Copy link
Member

Choose a reason for hiding this comment

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

If this causes a regression somewhere, we could always roll back the changes!

If we release this in 3.14.0, then backward compatibility will again mean we're stuck with the change. CPython is deep enough in the stack that we can't afford to just go back and forth.

Copy link
Member Author

Choose a reason for hiding this comment

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

You may want to compare against object.__new__ instead, but that still doesn't directly check for "Python types", and in any case I'm not sure it's a good idea to branch on whether it's a Python type.

>>> from typing import TypeVar
>>> TypeVar.__new__
<built-in method __new__ of type object at 0x14c817e30>
>>> A.__new__ is object.__new__
True

Oh yeah, that sounds like the right approach, my bad!

If we release this in 3.14.0, then backward compatibility will again mean we're stuck with the change. CPython is deep enough in the stack that we can't afford to just go back and forth.

Hmm, maybe we should bring this to Discourse.

Copy link
Member

Choose a reason for hiding this comment

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

Very much agreeing there, but there does seem to be some demand from the typing users.

Just because there's a problem doesn't mean that this is the best fix. __orig_class__ is completely undocumented and the last time we discussed potentially documenting it we decided not to because it sometimes has a counterintuitive value. For other typing-specific dunders our approach has generally been to expose public introspection helpers so that we're easily able to refactor internals later without breaking any public interface (e.g. get_protocol_members(), types.get_original_bases, etc.).


if is_python_type:
# GH-123777: Some types try and access __orig_class__ in their __init__
result = clas.__new__(clas, *args, **kwargs)
else:
# This is a C type with a custom __new__
#
# I'm worried that trying to set attributes on C types before they've been
# fully initialized will be problematic, so let's just disallow them
# from using the __orig_class__ in the initializer for now.
result = clas(*args, **kwargs)

try:
result.__orig_class__ = self
# Some objects raise TypeError (or something even more exotic)
# if you try to set attributes on them; we guard against that here
except Exception:
pass

if is_python_type:
Copy link
Member

Choose a reason for hiding this comment

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

There can be C types with __init__ as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

As in, there are C types that might want to access their own __orig_class__?

result.__init__(*args, **kwargs)

return result

def __mro_entries__(self, bases):
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2024-09-09-16-21-12.gh-issue-123777.sdyP3n.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Allowed the ``__orig_class__`` attribute to be used in the ``__init__`` of
pure-Python types.
Loading