gh-90562: Improve zero argument support for `super()` in dataclasses when `slots=True` by Bobronium · Pull Request #124692 · python/cpython
All commit authors signed the Contributor License Agreement.
| _seen.add(id(obj)) | ||
|
|
||
| _depth += 1 | ||
| if _depth > 2: |
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.
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/
| ): | ||
| # we don't want to inspect custom descriptors just yet | ||
| # there's still a chance we'll encounter a pure function | ||
| # or a property |
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.
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:
| # 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?
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".
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.
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! 👍
| return None | ||
|
|
||
| for attr in dir(obj): | ||
| value = getattr(obj, attr, None) |
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, 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.
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.
Co-authored-by: sobolevn <mail@sobolevn.me>
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.
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.
In the meantime, could
inspect.getattr_staticorinspect.getmembers_staticbe used instead of_safe_get_attributes?
Good idea. I've changed the implementation to use inspect.getmembers_static() instead.
As I mentioned above, I would rather have @ericvsmith approve/merge this, since it was his review comments you addressed.
I've been busy, but it's on my list of things to do.
This PR is stale because it has been open for 30 days with no activity.