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.

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

Sixth element of tuple from __reduce__(), inconsistency between pickle and copy #90494

Closed
levbishop mannequin opened this issue Jan 10, 2022 · 3 comments · Fixed by #93609
Closed

Sixth element of tuple from __reduce__(), inconsistency between pickle and copy #90494

levbishop mannequin opened this issue Jan 10, 2022 · 3 comments · Fixed by #93609
Assignees
Labels
3.9 3.10 3.11 stdlib type-bug

Comments

@levbishop
Copy link
Mannequin

@levbishop levbishop mannequin commented Jan 10, 2022

BPO 46336
Nosy @rhettinger, @pitrou, @serhiy-storchaka, @iritkatriel
Files
  • reduce.py
  • 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 = None
    closed_at = None
    created_at = <Date 2022-01-10.21:41:04.524>
    labels = ['type-bug', 'library', '3.9', '3.10', '3.11']
    title = 'Sixth element of tuple from __reduce__(), inconsistency between pickle and copy'
    updated_at = <Date 2022-01-10.22:33:13.389>
    user = 'https://bugs.python.org/levbishop'

    bugs.python.org fields:

    activity = <Date 2022-01-10.22:33:13.389>
    actor = 'iritkatriel'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2022-01-10.21:41:04.524>
    creator = 'lev.bishop'
    dependencies = []
    files = ['50554']
    hgrepos = []
    issue_num = 46336
    keywords = []
    message_count = 2.0
    messages = ['410256', '410262']
    nosy_count = 5.0
    nosy_names = ['rhettinger', 'pitrou', 'serhiy.storchaka', 'iritkatriel', 'lev.bishop']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = None
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue46336'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @levbishop
    Copy link
    Mannequin Author

    @levbishop levbishop mannequin commented Jan 10, 2022

    As discussed in discord thread https://discuss.python.org/t/sixth-element-of-tuple-from-reduce-inconsistency-between-pickle-and-copy/12902 where guido suggested to open this issue.

    Both the pickle and copy modules of the standard library make use of a class’s __reduce__() method for customizing their pickle/copy process. They seem to have a consistent view of the first 5 elements of the returned tuple:
    (func, args, state, listiter, dictiter) but the 6th element seems different. For pickle it’s state_setter , a callable with signature state_setter(obj, state)->None , but for copy it’s deepcopy with signature deepcopy(arg: T, memo) -> T .

    This seems to be unintentional, since the pickle documentation states:

    As we shall see, pickle does not use directly the methods described
    above. In fact, these methods are part of the copy protocol which
    implements the __reduce__() special method. The copy protocol provides
    a unified interface for retrieving the data necessary for pickling
    and copying objects

    It seems like in order to make a class definition for __reduce__() returning all 6 elements, then the __reduce__() would have to do something very awkward like examining its call stack in order to determine if it is being called in pickle or copy context in order to return an appropriate callable? (Naively providing the same callable in both contexts would cause errors for one or the other).

    I attach a test file which defines two classes making use of a __reduce__() returning a 6 element tuple. One class Pickleable can be duplicated via pickling, but not deepcopied. The converse is true for the Copyable class.

    Other than the 6th element of the tuple returned from __reduce__() the classes are identical.

    Guido dug into the history and found that:

    it looks like these are independent developments:

    the 6th arg for deepcopy was added 6 years ago via bpo-26167: Improve copy.copy speed for built-in types (list/set/dict) - Python tracker
    the 6th arg for pickle was adde 3 years ago via bpo-35900: Add pickler hook for the user to customize the serialization of user defined functions and types. - Python tracker

    I’m guessing the folks doing the latter weren’t aware that deepcopy already uses the 6th arg. Sorting this out will be painful.

    @levbishop levbishop mannequin added stdlib type-bug labels Jan 10, 2022
    @iritkatriel
    Copy link
    Member

    @iritkatriel iritkatriel commented Jan 10, 2022

    I added Serhiy as the author of the deepcopy optimization. Although it was the first to use the 6th item, it is not documented so I wonder if it's the easier of the two to change.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @serhiy-storchaka serhiy-storchaka self-assigned this Jun 8, 2022
    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jun 8, 2022

    The code was written with expectation that __reduce__() returns 2 to 5 items. The deepcopy parameter was not designed to be overridden, it was a pure microoptimization (access to a global). The simplest correct solution is either to make it keyword-only, or remove it at all.

    There is only one vague test for the 6th item of __reduce__(), so I hesitate to implement its support for copy.

    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jun 8, 2022
    copy.copy() and copy.deepcopy() now always raise a TypeError if
    __reduce__() returns a tuple with length 6 instead of silently ignore
    the 6th item or produce incorrect result.
    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jun 8, 2022
    copy.copy() and copy.deepcopy() now always raise a TypeError if
    __reduce__() returns a tuple with length 6 instead of silently ignore
    the 6th item or produce incorrect result.
    serhiy-storchaka added a commit that referenced this issue Jun 9, 2022
    copy.copy() and copy.deepcopy() now always raise a TypeError if
    __reduce__() returns a tuple with length 6 instead of silently ignore
    the 6th item or produce incorrect result.
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 9, 2022
    …H-93609)
    
    copy.copy() and copy.deepcopy() now always raise a TypeError if
    __reduce__() returns a tuple with length 6 instead of silently ignore
    the 6th item or produce incorrect result.
    (cherry picked from commit a365dd6)
    
    Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 9, 2022
    …H-93609)
    
    copy.copy() and copy.deepcopy() now always raise a TypeError if
    __reduce__() returns a tuple with length 6 instead of silently ignore
    the 6th item or produce incorrect result.
    (cherry picked from commit a365dd6)
    
    Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 3.10 3.11 stdlib type-bug
    Projects
    Status: Done
    Development

    Successfully merging a pull request may close this issue.

    3 participants