gh-116647: Fix recursive child in dataclasses#116790
Uh oh!
There was an error while loading. Please reload this page.
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
| # since python will call __eq__ and negate it. | ||
| cmp_fields= (fieldforfieldinfield_listiffield.compare) | ||
| terms= [f'self.{field.name}==other.{field.name}'forfieldincmp_fields] | ||
| terms= [f'(self.{field.name},)==(other.{field.name},)'forfieldincmp_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.
Jason-Y-Z left a comment
Jason-Y-Z left a comment There was a problem hiding this comment.
Overall LGTM, a small comment on the test case
| classTestEq(unittest.TestCase): | ||
| deftest_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!
Uh oh!
There was an error while loading. Please reload this page.
4 participants
#116647