gh-116647: Fix recursive child in dataclasses #116790
Conversation
ghost
requested a review
from 
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Lib/dataclasses.py
Outdated
| @@ -1073,7 +1073,7 @@ def _process_class(cls, init, repr, eq, order, unsafe_hash, frozen, | |||
| # Create __eq__ method. There's no need for a __ne__ method, | |||
| # since python will call __eq__ and negate it. | |||
| cmp_fields = (field for field in field_list if field.compare) | |||
| terms = [f'self.{field.name}==other.{field.name}' for field in cmp_fields] | |||
| terms = [f'(self.{field.name},)==(other.{field.name},)' for field in cmp_fields] | |||
There was a problem hiding this comment.
Is this an improvement over just reverting to the code before 18cfc1e?
There was a problem hiding this comment.
I think this is just reverting to the code before 18cfc1e
due to (object_a, ) == (object_b) will first compare ids of objects, if it's not same then call eq of the object, that may not cause RecursionError: maximum recursion depth exceeded when the objects pointers are the same
but object_a == object_b directly calls object.eq whether the pointers is same or not, so i think remove 18cfc1e will solve the errors and provides better performance when the child have the same pointers will cause early return without calling eq of objects
There was a problem hiding this comment.
Thanks for the comment, I've improved the efficiency base on 18cfc1e now it'll compare id(obj) first to make sure if the child point to themselves will not cause maximum recursion depth and it'll be faster when compare exact same objects.
| @@ -2471,6 +2471,15 @@ def __repr__(self): | |||
|
|
|||
|
|
|||
| class TestEq(unittest.TestCase): | |||
| def test_recursive_eq(self): | |||
There was a problem hiding this comment.
Thanks for the change! Per discussions in the issue, maybe also worth adding a test case for mutual recursive classes (given that's intended to be included in the scope of this PR)?
There was a problem hiding this comment.
Thanks for the comment, but I don't think mutual recursive classes can be implement with good performance, I've think ways like maintenance dict like id(obj) as key to check if it's recursive but it'll cost spaces and times to make compare like that.
There was a problem hiding this comment.
Make sense, thanks for the clarification!
#116647