Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix collections.UserList shallow copy #71328

Closed
bharel mannequin opened this issue May 27, 2016 · 18 comments
Closed

Fix collections.UserList shallow copy #71328

bharel mannequin opened this issue May 27, 2016 · 18 comments
Assignees
@serhiy-storchaka
Labels
3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@bharel
Copy link
Mannequin

bharel mannequin commented May 27, 2016

BPO 27141
Nosy @rhettinger, @pakal, @serhiy-storchaka, @bharel, @miss-islington, @iritkatriel
PRs
  • bpo-27141: Fix collections.UserList and UserDict shallow copy #4094
  • [3.7] bpo-27141: Fix collections.UserList and UserDict shallow copy. (GH-4094) #13423
    		•
    Files
  • UserList.patch
  • UserObj_tests.patch
  • issue27141_patch_rev1_opt1.patch
  • issue27141_patch_rev1_opt2.patch
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2020-10-10.12:44:11.119>
created_at = <Date 2016-05-27.21:29:02.248>
labels = ['3.7', 'type-bug', 'library']
title = 'Fix collections.UserList shallow copy'
updated_at = <Date 2020-10-10.12:44:11.119>
user = 'https://github.com/bharel'

    bugs.python.org fields:

    activity = <Date 2020-10-10.12:44:11.119>
actor = 'bar.harel'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2020-10-10.12:44:11.119>
closer = 'bar.harel'
components = ['Library (Lib)']
creation = <Date 2016-05-27.21:29:02.248>
creator = 'bar.harel'
dependencies = []
files = ['43032', '43036', '44832', '44833']
hgrepos = []
issue_num = 27141
keywords = ['patch']
message_count = 18.0
messages = ['266515', '266535', '266546', '266548', '266550', '269428', '277384', '277399', '277445', '277448', '278578', '304790', '304842', '304858', '304866', '342853', '342859', '378291']
nosy_count = 6.0
nosy_names = ['rhettinger', 'pakal', 'serhiy.storchaka', 'bar.harel', 'miss-islington', 'iritkatriel']
pr_nums = ['4094', '13423']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue27141'
versions = ['Python 2.7', 'Python 3.6', 'Python 3.7']
    @bharel
    Copy link
    Mannequin Author

    bharel mannequin commented May 27, 2016

    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.

    @bharel bharel mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels May 27, 2016
    @rhettinger
    Copy link
    Contributor

    Please add a test case.

    @rhettinger rhettinger self-assigned this May 28, 2016
    @serhiy-storchaka
    Copy link
    Member

    What about UserDict?

    @bharel
    Copy link
    Mannequin Author

    bharel mannequin commented May 28, 2016

    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.

    @bharel
    Copy link
    Mannequin Author

    bharel mannequin commented May 28, 2016

    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.

    @bharel
    Copy link
    Mannequin Author

    bharel mannequin commented Jun 28, 2016

    Are the patch and tests good?

    @rhettinger
    Copy link
    Contributor

    Yes. I will look at this shortly.

    @serhiy-storchaka
    Copy link
    Member

    UserList.copy() doesn't copy instance attributes, but copy.copy() should copy them.

    @bharel
    Copy link
    Mannequin Author

    bharel mannequin commented Sep 26, 2016

    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.

    @bharel bharel mannequin added the 3.7 (EOL) end of life label Sep 26, 2016
    @bharel
    Copy link
    Mannequin Author

    bharel mannequin commented Sep 26, 2016

    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.

    @bharel
    Copy link
    Mannequin Author

    bharel mannequin commented Oct 13, 2016

    Bumposaurus Rex

    @serhiy-storchaka
    Copy link
    Member

    I prefer issue27141_patch_rev1_opt1.patch. But now we use GitHub. Do you mind to create a pull request Bar?

    @bharel
    Copy link
    Mannequin Author

    bharel mannequin commented Oct 23, 2017

    Done :-)
    Seems like I forgot to edit the news though, I'll try to edit it.

    @rhettinger
    Copy link
    Contributor

    Either of the patches looks fine. I lean a bit towards the opt1 patch.

    @bharel
    Copy link
    Mannequin Author

    bharel mannequin commented Oct 24, 2017

    Alrighty then, opt1 passed all tests and waiting on GitHub for inclusion :-)

    @serhiy-storchaka
    Copy link
    Member

    New changeset f4e1bab by Serhiy Storchaka (Bar Harel) in branch 'master':
    bpo-27141: Fix collections.UserList and UserDict shallow copy. (GH-4094)
    f4e1bab

    @miss-islington
    Copy link
    Contributor

    New changeset 3645d29 by Miss Islington (bot) in branch '3.7':
    bpo-27141: Fix collections.UserList and UserDict shallow copy. (GH-4094)
    3645d29

    @iritkatriel
    Copy link
    Member

    This seems complete, can it be closed?

    @bharel bharel mannequin closed this as completed Oct 10, 2020
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @serhiy-storchaka serhiy-storchaka

    Labels
    3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @iritkatriel @rhettinger @serhiy-storchaka @miss-islington