gh-90562: Improve zero argument support for `super()` in dataclasses when `slots=True` #124692

Bobronium · 2024-09-27T18:05:27Z

Original PR: gh-90562: Support zero argument super with dataclasses when slots=True #124455

Added two tests for two cases that weren't covered in original PR:

Using custom descriptors
Wrapping function before making it a property

Issue: RFE: @dataclasses.dataclass(slots=True) doesn't support methods using zero-arg super() #90562

cpython-cla-bot · 2024-09-27T18:05:30Z

All commit authors signed the Contributor License Agreement.

ericvsmith · 2024-09-27T21:25:03Z

Lib/dataclasses.py

        return True
    return False


+def _find_inner_functions(obj, _seen=None, _depth=0):


Why the leading underscores on _seen and _depth?

This should indicate that these arguments are only used within the function (in recursive calls) and not outside of the function body.

I think that the whole function is private, so it can only be used inside CPython by us :)

Well, you've got a point. Removed the underscores.

ericvsmith · 2024-09-27T21:29:30Z

Lib/dataclasses.py

+    _seen.add(id(obj))
+
+    _depth += 1
+    if _depth > 2:


Why are depth 1 and 2 special? What is this test (and _depth in general) trying to accomplish? Please add comments explaining what's going on here, it's not at all clear from the code.

Added an explanation and few test cases for this behaviour. Caught two bugs in the process \o/

ericvsmith · 2024-09-27T21:37:16Z

Lib/dataclasses.py

+        ):
+            # we don't want to inspect custom descriptors just yet
+            # there's still a chance we'll encounter a pure function
+            # or a property


Please add a comment explaining why encountering a pure function or property means we don't have to inspect custom descriptors.

Hmm, there's already an explanation above:

cpython/Lib/dataclasses.py

Lines 1336 to 1339 in 3b65acf

# 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. First we try to find a pure function/properties,

# and then fallback to inspecting custom descriptors.

Would you like me to add clarification for this case specifically?

Ah, you're right. Maybe change the comment to "... fallback to inspecting custom descriptors if no pure function or property is found".

Makes sense. I've added clarification in both places to be extra clear.

Lib/dataclasses.py

sobolevn

Great find, thanks a lot for your work! 👍

sobolevn · 2024-09-28T05:47:14Z

Lib/dataclasses.py

        return True
    return False


+def _find_inner_functions(obj, _seen=None, _depth=0):


I think that the whole function is private, so it can only be used inside CPython by us :)

Lib/dataclasses.py

sobolevn · 2024-09-28T05:50:03Z

Lib/dataclasses.py

+        return None
+
+    for attr in dir(obj):
+        value = getattr(obj, attr, None)


This case is always complex, since getattr can evaluate properties and other objects. There are two ways of hanling this case:

Allow all exceptions to raise

Hide all exceptions from user here

I am not quite sure which one is better here.

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.

Lib/test/test_dataclasses/__init__.py

Co-authored-by: sobolevn <[email protected]>

carljm

This looks OK to me; I'll let @ericvsmith give it final approval/merge.

I don't love that we've opened the door to an infinite fractal of complexity here; at some point it would probably be better to bite the bullet and introduce a __classcell__ attribute on classes that allows reliably finding this cell in O(1) time.

Lib/dataclasses.py

ericvsmith · 2024-10-02T14:08:39Z

I agree that the complexity is unfortunate. __classcell__ would be a good change. In the meantime, could inspect.getattr_static or inspect.getmembers_static be used instead of _safe_get_attributes?

This uses builtins.dir, which will trigger custom __getattibute__ if any, and will trigger __get__ on __dict__ descriptor.

Bobronium · 2024-10-05T13:18:44Z

In the meantime, could inspect.getattr_static or inspect.getmembers_static be used instead of _safe_get_attributes?

Good idea. I've changed the implementation to use inspect.getmembers_static() instead.

Bobronium · 2024-11-03T13:39:04Z

@carljm, friendly ping :)

carljm · 2024-11-04T22:56:47Z

As I mentioned above, I would rather have @ericvsmith approve/merge this, since it was his review comments you addressed.

ericvsmith · 2024-11-04T23:26:57Z

I've been busy, but it's on my list of things to do.

Bobronium requested a review from ericvsmith as a code owner September 27, 2024 18:05

bedevere-app bot added the awaiting review label Sep 27, 2024

bedevere-app bot mentioned this pull request Sep 27, 2024

RFE: @dataclasses.dataclass(slots=True) doesn't support methods using zero-arg super() #90562

Closed

Improve zero argument support for super() in dataclasses

3b65acf

sobolevn requested a review from carljm September 27, 2024 18:10

Bobronium force-pushed the issue-90562-dataclasses-zero-argument-super-fix branch from 2e0ca01 to 3b65acf Compare September 27, 2024 18:10

ericvsmith reviewed Sep 27, 2024

View reviewed changes

Bobronium added 3 commits September 28, 2024 00:41

Fix NEWS.d formatting

efb6095

Add explanation for _depth use and tests

3654f51

Fix incorrect variable reference

19297bc

ericvsmith reviewed Sep 27, 2024

View reviewed changes

Lib/dataclasses.py Outdated Show resolved Hide resolved

Bobronium added 8 commits September 28, 2024 01:01

Correctly unwrap descriptor function

2606480

Add test with using partial as custom wrapper

42f9dc3

Rephrase comment explaining _depth argument

53c18d1

Add clarification for function search priority

96d5315

Ues consistent grammar throughout comments

22eeb8f

Fix breaking out from outer loop when handling property

e91c28b

Use generator expression with any() instead of classic loop

4941a3f

Merge branch 'main' into issue-90562-dataclasses-zero-argument-super-fix

6ef5317

sobolevn reviewed Sep 28, 2024

View reviewed changes

Bobronium and others added 3 commits September 28, 2024 13:03

Apply suggestions from code review

d33070f

Co-authored-by: sobolevn <[email protected]>

Remove leading underscores from seen and depth args

c26e978

Prevent user-defined code execution during attribute scanning

d0173d8

carljm reviewed Oct 1, 2024

View reviewed changes

Lib/dataclasses.py Show resolved Hide resolved

Bobronium added 3 commits October 5, 2024 14:41

Use inspect.getattr_static instead of _safe_get_attributes

c68b4cc

This uses builtins.dir, which will trigger custom __getattibute__ if any, and will trigger __get__ on __dict__ descriptor.

Put back the comment in _update_func_cell_for__class__

7d679f2

Rely on inspect.getmembers_static()

5bfea4d

ZetaTwo mentioned this pull request Dec 17, 2024

Use dataclass slots to decrease memory usage SiegeEngineers/genieutils-py#4

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-90562: Improve zero argument support for `super()` in dataclasses when `slots=True` #124692

gh-90562: Improve zero argument support for `super()` in dataclasses when `slots=True` #124692

Bobronium commented Sep 27, 2024 •

edited

Loading

cpython-cla-bot bot commented Sep 27, 2024 •

edited

Loading

ericvsmith Sep 27, 2024

Bobronium Sep 27, 2024

sobolevn Sep 28, 2024

Bobronium Sep 28, 2024

ericvsmith Sep 27, 2024

Bobronium Sep 27, 2024

ericvsmith Sep 27, 2024

Bobronium Sep 27, 2024

ericvsmith Sep 27, 2024

Bobronium Sep 27, 2024

sobolevn left a comment

sobolevn Sep 28, 2024

sobolevn Sep 28, 2024

Bobronium Sep 28, 2024

carljm left a comment

ericvsmith commented Oct 2, 2024

Bobronium commented Oct 5, 2024

Bobronium commented Nov 3, 2024

carljm commented Nov 4, 2024

ericvsmith commented Nov 4, 2024

	# 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. First we try to find a pure function/properties,
	# and then fallback to inspecting custom descriptors.

gh-90562: Improve zero argument support for super() in dataclasses when slots=True #124692

Are you sure you want to change the base?

gh-90562: Improve zero argument support for super() in dataclasses when slots=True #124692

Conversation

Bobronium commented Sep 27, 2024 • edited Loading

cpython-cla-bot bot commented Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobolevn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carljm left a comment

Choose a reason for hiding this comment

ericvsmith commented Oct 2, 2024

Bobronium commented Oct 5, 2024

Bobronium commented Nov 3, 2024

carljm commented Nov 4, 2024

ericvsmith commented Nov 4, 2024

gh-90562: Improve zero argument support for `super()` in dataclasses when `slots=True` #124692

gh-90562: Improve zero argument support for `super()` in dataclasses when `slots=True` #124692

Bobronium commented Sep 27, 2024 •

edited

Loading

cpython-cla-bot bot commented Sep 27, 2024 •

edited

Loading