NotificationsYou must be signed in to change notification settings
Fork32.4k
Star67.9k

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

Bobronium wants to merge18 commits intopython:main

base:main

Choose a base branch

fromBobronium:issue-90562-dataclasses-zero-argument-super-fix

Open

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

Bobronium wants to merge18 commits intopython:mainfromBobronium:issue-90562-dataclasses-zero-argument-super-fix

+266 −13

Conversation

Copy link

Bobronium commentedSep 27, 2024•
edited
Loading

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

Bobronium requested a review fromericvsmith as acode owner

September 27, 2024 18:05

Copy link

ghost commentedSep 27, 2024•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.

bedevere-appbot added the awaiting review label

Sep 27, 2024

bedevere-appbot 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 forsuper() in dataclasses

3b65acf

sobolevn requested a review fromcarljm

September 27, 2024 18:10

Bobronium force-pushed theissue-90562-dataclasses-zero-argument-super-fix branch from2e0ca01 to3b65acfCompare

September 27, 2024 18:10

ericvsmith reviewed

Sep 27, 2024

View reviewed changes

Lib/dataclasses.py Outdated

		return True
		return False


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

Copy link

Member

Choose a reason for hiding this comment

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

Why the leading underscores on_seen and_depth?

Copy link

Author

Choose a reason for hiding this comment

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

Copy link

Member

Choose a reason for hiding this comment

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

Copy link

Author

Choose a reason for hiding this comment

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

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

Lib/dataclasses.py Outdated

		_seen.add(id(obj))

		_depth += 1
		if _depth > 2:

Copy link

Member

Choose a reason for hiding this comment

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

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.

Copy link

Author

Choose a reason for hiding this comment

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

Lib/dataclasses.py Outdated

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

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

Author

Choose a reason for hiding this comment

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

Hmm, there's already an explanation above:

cpython/Lib/dataclasses.py

Lines 1336 to 1339 in3b65acf

	# 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?

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

Author

Choose a reason for hiding this comment

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

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

Bobronium added3 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 OutdatedShow resolvedHide resolved

Bobronium added8 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

Copy link

Member

sobolevn left a comment

Choose a reason for hiding this comment

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

Great find, thanks a lot for your work! 👍

Lib/dataclasses.py Outdated

		return True
		return False


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

Copy link

Member

Choose a reason for hiding this comment

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

Lib/dataclasses.py OutdatedShow resolvedHide resolved

Lib/dataclasses.py Outdated

		return None

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

Copy link

Member

Choose a reason for hiding this comment

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

This case is always complex, sincegetattr 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.

Copy link

Author

Choose a reason for hiding this comment

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

Lib/test/test_dataclasses/__init__.py OutdatedShow resolvedHide resolved

Bobroniumand others added3 commits

September 28, 2024 13:03

Apply suggestions from code review

d33070f

Co-authored-by: sobolevn <mail@sobolevn.me>

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

Copy link

Member

carljm left a comment

Choose a reason for hiding this comment

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

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.pyShow resolvedHide resolved

Copy link

Member

ericvsmith commentedOct 2, 2024

I agree that the complexity is unfortunate.__classcell__ would be a good change. In the meantime, couldinspect.getattr_static orinspect.getmembers_static be used instead of_safe_get_attributes?

Bobronium added3 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

Copy link

Author

Bobronium commentedOct 5, 2024

In the meantime, couldinspect.getattr_static orinspect.getmembers_static be used instead of_safe_get_attributes?

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

Copy link

Author

Bobronium commentedNov 3, 2024

@carljm, friendly ping :)

Copy link

Member

carljm commentedNov 4, 2024

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

Copy link

Member

ericvsmith commentedNov 4, 2024

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

ZetaTwo mentioned this pull request

Dec 17, 2024

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

Merged

Labels

awaiting review

4 participants

Movatterモバイル変換

Uh oh!

gh-90562: Improve zero argument support forsuper() in dataclasses whenslots=True#124692

Are you sure you want to change the base?

gh-90562: Improve zero argument support forsuper() in dataclasses whenslots=True#124692

Conversation

Bobronium commentedSep 27, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

ghost commentedSep 27, 2024• edited by ghostLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ericvsmith commentedOct 2, 2024

Uh oh!

Bobronium commentedOct 5, 2024

Uh oh!

Bobronium commentedNov 3, 2024

Uh oh!

carljm commentedNov 4, 2024

Uh oh!

ericvsmith commentedNov 4, 2024

Uh oh!

Uh oh!

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 commentedSep 27, 2024•
edited
Loading

ghost commentedSep 27, 2024•
edited by ghost
Loading