-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
gh-90562: Improve zero argument support for super()
in dataclasses when slots=True
#124692
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
base: main
Are you sure you want to change the base?
Changes from 12 commits
3b65acf
efb6095
3654f51
19297bc
2606480
42f9dc3
53c18d1
96d5315
22eeb8f
e91c28b
4941a3f
6ef5317
d33070f
c26e978
d0173d8
c68b4cc
7d679f2
5bfea4d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1222,11 +1222,6 @@ def _get_slots(cls): | |
|
||
|
||
def _update_func_cell_for__class__(f, oldcls, newcls): | ||
# Returns True if we update a cell, else False. | ||
if f is None: | ||
# f will be None in the case of a property where not all of | ||
# fget, fset, and fdel are used. Nothing to do in that case. | ||
return False | ||
try: | ||
idx = f.__code__.co_freevars.index("__class__") | ||
except ValueError: | ||
|
@@ -1235,13 +1230,38 @@ def _update_func_cell_for__class__(f, oldcls, newcls): | |
# Fix the cell to point to the new class, if it's already pointing | ||
# at the old class. I'm not convinced that the "is oldcls" test | ||
# is needed, but other than performance can't hurt. | ||
closure = f.__closure__[idx] | ||
if closure.cell_contents is oldcls: | ||
closure.cell_contents = newcls | ||
cell = f.__closure__[idx] | ||
if cell.cell_contents is oldcls: | ||
cell.cell_contents = newcls | ||
return True | ||
return False | ||
|
||
|
||
def _find_inner_functions(obj, _seen=None, _depth=0): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the leading underscores on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should indicate that these arguments are only used within the function (in recursive calls) and not outside of the function body. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that the whole function is private, so it can only be used inside CPython by us :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, you've got a point. Removed the underscores. |
||
if _seen is None: | ||
_seen = set() | ||
if id(obj) in _seen: | ||
return None | ||
_seen.add(id(obj)) | ||
|
||
_depth += 1 | ||
# Normally just inspection of a descriptor object itself should be enough, | ||
# and we should encounter the function as its attribute, | ||
# but in case function was wrapped (e.g. functools.partition was used), | ||
Bobronium marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# we want to dive at least one level deeper. | ||
if _depth > 2: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are depth 1 and 2 special? What is this test (and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added an explanation and few test cases for this behaviour. Caught two bugs in the process \o/ |
||
return None | ||
|
||
for attr in dir(obj): | ||
value = getattr(obj, attr, None) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This case is always complex, since
I am not quite sure which one is better here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for bringing this to my attention! After some hacking, I've decided to go with the third option: removing any possibility of user defined code execution. |
||
if value is None: | ||
continue | ||
if isinstance(value, types.FunctionType): | ||
yield inspect.unwrap(value) | ||
return | ||
yield from _find_inner_functions(value, _seen, _depth) | ||
|
||
|
||
def _create_slots(defined_fields, inherited_slots, field_names, weakref_slot): | ||
# The slots for our class. Remove slots from our base classes. Add | ||
# '__weakref__' if weakref_slot was given, unless it is already present. | ||
|
@@ -1317,19 +1337,41 @@ def _add_slots(cls, is_frozen, weakref_slot, defined_fields): | |
# (the newly created one, which we're returning) and not the | ||
# original class. We can break out of this loop as soon as we | ||
# make an update, since all closures for a class will share a | ||
# given cell. | ||
# given cell. First we try to find a pure function or a property, | ||
# and then fallback to inspecting custom descriptors | ||
# if no pure function or property is found. | ||
|
||
custom_descriptors_to_check = [] | ||
for member in newcls.__dict__.values(): | ||
# If this is a wrapped function, unwrap it. | ||
member = inspect.unwrap(member) | ||
|
||
if isinstance(member, types.FunctionType): | ||
if _update_func_cell_for__class__(member, cls, newcls): | ||
break | ||
elif isinstance(member, property): | ||
if (_update_func_cell_for__class__(member.fget, cls, newcls) | ||
or _update_func_cell_for__class__(member.fset, cls, newcls) | ||
or _update_func_cell_for__class__(member.fdel, cls, newcls)): | ||
break | ||
elif isinstance(member, property) and ( | ||
any( | ||
# Unwrap once more in case function | ||
# was wrapped before it became property. | ||
_update_func_cell_for__class__(inspect.unwrap(f), cls, newcls) | ||
for f in (member.fget, member.fset, member.fdel) | ||
if f is not None | ||
) | ||
): | ||
break | ||
elif hasattr(member, "__get__") and not inspect.ismemberdescriptor( | ||
member | ||
): | ||
# We don't want to inspect custom descriptors just yet | ||
# there's still a chance we'll encounter a pure function | ||
# or a property and won't have to use slower recursive search. | ||
custom_descriptors_to_check.append(member) | ||
else: | ||
# Now let's ensure custom descriptors won't be left out. | ||
for descriptor in custom_descriptors_to_check: | ||
for f in _find_inner_functions(descriptor): | ||
if _update_func_cell_for__class__(f, cls, newcls): | ||
break | ||
|
||
return newcls | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Modify dataclasses to enable zero argument support for ``super()`` when ``slots=True`` is | ||
specified and custom descriptor is used or ``property`` function is wrapped. |
Uh oh!
There was an error while loading. Please reload this page.