| msg266515 - (view) |
Author: Bar Harel (bar.harel) * |
Date: 2016-05-27 21:29 |
| I have encountered a weird behavior in collections.UserList. Using copy.copy() on an instance results in a new instance of UserList but with the same underlying list. Seems like self.copy() works great but __copy__ was not overridden to allow copy.copy to work too. The patch just assigns __copy__ to self.copy triggering the correct behavior. |
|
|
| msg266535 - (view) |
Author: Raymond Hettinger (rhettinger) *  |
Date: 2016-05-28 05:40 |
| Please add a test case. |
|
|
| msg266546 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-05-28 07:58 |
| What about UserDict? |
|
|
| msg266548 - (view) |
Author: Bar Harel (bar.harel) * |
Date: 2016-05-28 11:47 |
| I thought about UserDict, but adding this simple patch to UserDict will result in infinite recursion (due to how copy is implemented in there). We will have to change the implementation of UserDict's copy method. |
|
|
| msg266550 - (view) |
Author: Bar Harel (bar.harel) * |
Date: 2016-05-28 12:10 |
| Added UserDict and UserList tests. Keep in mind I am currently skipping UserDict's tests until we will implement the correct mechanism. We do not need the same tests or functionality for UserString as UserString is immutable. |
|
|
| msg269428 - (view) |
Author: Bar Harel (bar.harel) * |
Date: 2016-06-28 09:39 |
| Are the patch and tests good? |
|
|
| msg277384 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2016-09-25 16:48 |
| Yes. I will look at this shortly. |
|
|
| msg277399 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-09-25 21:44 |
| UserList.copy() doesn't copy instance attributes, but copy.copy() should copy them. |
|
|
| msg277445 - (view) |
Author: Bar Harel (bar.harel) * |
Date: 2016-09-26 18:32 |
| Alright. 2 patches are available. opt_1 makes the use of __copy__. Advantages: - Clean - Does not affect pickling at all - More memory efficient Disadvantages: - Might be missing something from the normal copy mechanism (e.g. UserList doesn't implement __slots__ but it somewhat interferes with future implementation) - Doesn't call __reduce__, __getstate__, ... while people might rely on it for some reason during copy, thus it might not be entirely backwards compatible opt_2 makes use of __reduce__. Advantages: - Lowest in the chain. Shouldn't cause any backwards compatibility issues as if the user manually defined __getstate__ or __reduce_ex__ himself, the code as far as he's concerned did not change. - Uses the default mechanism for copying. Changes in the protocol will not cause any bug in here. Disadvantages: - Affects pickling, messes up with the __reduce__ protocol. - Takes more memory during pickling as it recreates the dict. - Uglier as a personal opinion. __getstate__ was not attempted as it will break backwards compatibility for sure if someone wrote a __reduce__ method (as it won't be called), but it's also a viable option. Both patches contain tests and both fix the bug in UserDict and UserList. |
|
|
| msg277448 - (view) |
Author: Bar Harel (bar.harel) * |
Date: 2016-09-26 18:35 |
| I personally prefer the __copy__ mechanism as I think a bugfix shouldn't be 100000% backwards compatible, chances of issues are low, and it's cleaner, more efficient and how things should be. |
|
|
| msg278578 - (view) |
Author: Bar Harel (bar.harel) * |
Date: 2016-10-13 14:32 |
| Bumposaurus Rex |
|
|
| msg304790 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2017-10-23 10:21 |
| I prefer issue27141_patch_rev1_opt1.patch. But now we use GitHub. Do you mind to create a pull request Bar? |
|
|
| msg304842 - (view) |
Author: Bar Harel (bar.harel) * |
Date: 2017-10-23 21:11 |
| Done :-) Seems like I forgot to edit the news though, I'll try to edit it. |
|
|
| msg304858 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2017-10-24 04:30 |
| Either of the patches looks fine. I lean a bit towards the opt1 patch. |
|
|
| msg304866 - (view) |
Author: Bar Harel (bar.harel) * |
Date: 2017-10-24 07:08 |
| Alrighty then, opt1 passed all tests and waiting on GitHub for inclusion :-) |
|
|
| msg342853 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2019-05-19 13:57 |
| New changeset f4e1babf44792bdeb0c01da96821ba0800a51fd8 by Serhiy Storchaka (Bar Harel) in branch 'master': bpo-27141: Fix collections.UserList and UserDict shallow copy. (GH-4094) https://github.com/python/cpython/commit/f4e1babf44792bdeb0c01da96821ba0800a51fd8 |
|
|
| msg342859 - (view) |
Author: miss-islington (miss-islington) |
Date: 2019-05-19 14:26 |
| New changeset 3645d29a1dc2102fdb0f5f0c0129ff2295bcd768 by Miss Islington (bot) in branch '3.7': bpo-27141: Fix collections.UserList and UserDict shallow copy. (GH-4094) https://github.com/python/cpython/commit/3645d29a1dc2102fdb0f5f0c0129ff2295bcd768 |
|
|
| msg378291 - (view) |
Author: Irit Katriel (iritkatriel) * |
Date: 2020-10-08 23:09 |
| This seems complete, can it be closed? |
|
|