classification
Title: Fix collections.UserList shallow copy
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.7, Python 3.6, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: bar.harel, miss-islington, pakal, rhettinger, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2016-05-27 21:29 by bar.harel, last changed 2019-05-19 14:26 by miss-islington.

Files
File name Uploaded Description Edit
UserList.patch bar.harel, 2016-05-27 21:29 review
UserObj_tests.patch bar.harel, 2016-05-28 12:10 review
issue27141_patch_rev1_opt1.patch bar.harel, 2016-09-26 18:32 review
issue27141_patch_rev1_opt2.patch bar.harel, 2016-09-26 18:32 review
Pull Requests
URL Status Linked Edit
PR 4094 merged python-dev, 2017-10-23 21:08
PR 13423 merged miss-islington, 2019-05-19 13:58
Messages (17)
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) * (Python committer) Date: 2016-05-28 05:40
Please add a test case.
msg266546 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) 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) * (Python committer) Date: 2016-09-25 16:48
Yes.  I will look at this shortly.
msg277399 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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
History
Date User Action Args
2019-05-19 14:26:40miss-islingtonsetnosy: + miss-islington
messages: + msg342859
2019-05-19 13:58:09miss-islingtonsetpull_requests: + pull_request13333
2019-05-19 13:57:19serhiy.storchakasetmessages: + msg342853
2017-10-24 07:08:38bar.harelsetmessages: + msg304866
2017-10-24 04:30:11rhettingersetassignee: rhettinger -> serhiy.storchaka
messages: + msg304858
2017-10-23 21:11:18bar.harelsetmessages: + msg304842
2017-10-23 21:08:25python-devsetstage: needs patch -> patch review
pull_requests: + pull_request4065
2017-10-23 10:21:22serhiy.storchakasetstage: needs patch
messages: + msg304790
versions: - Python 3.2, Python 3.3, Python 3.4, Python 3.5
2016-10-13 14:32:30bar.harelsetmessages: + msg278578
2016-09-26 18:35:01bar.harelsetmessages: + msg277448
2016-09-26 18:32:37bar.harelsetfiles: + issue27141_patch_rev1_opt2.patch
2016-09-26 18:32:17bar.harelsetfiles: + issue27141_patch_rev1_opt1.patch

messages: + msg277445
versions: + Python 3.7
2016-09-25 21:44:51serhiy.storchakasetmessages: + msg277399
2016-09-25 16:48:22rhettingersetmessages: + msg277384
2016-07-02 15:01:57pakalsetnosy: + pakal
2016-06-28 09:39:07bar.harelsetmessages: + msg269428
2016-05-28 12:10:24bar.harelsetfiles: + UserObj_tests.patch

messages: + msg266550
2016-05-28 11:47:16bar.harelsetmessages: + msg266548
2016-05-28 07:58:49serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg266546
2016-05-28 05:40:49rhettingersetassignee: rhettinger

messages: + msg266535
nosy: + rhettinger
2016-05-27 21:29:02bar.harelcreate