gh-141510: Fix copy.deepcopy() for recursive frozendict by vstinner · Pull Request #145027 · python/cpython
This was referenced
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
After adding the pickle support we may be able to remove the special deepcopy code.
After adding the pickle support we may be able to remove the special deepcopy code.
I propose merging #144967 PR first (once it will be approved), and then rework this PR. See also @serhiy-storchaka's comment on the pickle PR:
If things are pickleable, they should also be deepcopyable. It is worth to have explicit deepcopy tests, because they can preserve additional invariants. For example, it should be possible to deepcopy a frozendict containing lambdas or modules.
We can add more tests in this deepcopy PR.
After adding the pickle support we may be able to remove the special deepcopy code.
I merged main into this branch to retrieve the pickle change. If I remove _deepcopy_frozendict(), test_copy.test_deepcopy_frozendict() fails:
FAIL: test_deepcopy_frozendict (test.test_copy.TestCopy.test_deepcopy_frozendict)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/vstinner/python/main/Lib/test/test_copy.py", line 442, in test_deepcopy_frozendict
self.assertIs(y['foo'][0], y)
~~~~~~~~~~~~~^^^^^^^^^^^^^^^^
AssertionError: frozendict({'foo': [frozendict({...})]}) is not frozendict({'foo': [frozendict({'foo': [...]})]})
If I remove
_deepcopy_frozendict(),test_copy.test_deepcopy_frozendict()fails:
Hmm, I thought it is just an optimization. I wonder if deepcopy() can be generalized to handle this. But this is a different issue.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. 👍 (But maybe add few more assertions?)
| x['foo'].append(x) | ||
| x = x['foo'] | ||
| y = copy.deepcopy(x) | ||
| self.assertIs(y[0]['foo'], y) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add also other assertions similar to the above ones:
self.assertEqual(y, x) self.assertIsNot(x, y) self.assertIsNot(x[0], y[0])
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.assertEqual(y, x) fails with RecursionError. I added the two other tests.
Merged, thanks for the exhaustive review.
