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

pickle.py's load_appends should call append() on objects other than lists #61920

Closed
avassalotti opened this issue Apr 13, 2013 · 10 comments
Closed
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@avassalotti
Copy link
Member

BPO 17720
Nosy @pitrou, @avassalotti, @serhiy-storchaka
Files
  • loads_appends.patch
  • fix_loads_appends.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/avassalotti'
    closed_at = <Date 2017-01-25.09:56:40.837>
    created_at = <Date 2013-04-13.18:04:49.880>
    labels = ['type-bug', 'library']
    title = "pickle.py's load_appends should call append() on objects other than lists"
    updated_at = <Date 2017-01-26.10:52:58.321>
    user = 'https://github.com/avassalotti'

    bugs.python.org fields:

    activity = <Date 2017-01-26.10:52:58.321>
    actor = 'serhiy.storchaka'
    assignee = 'alexandre.vassalotti'
    closed = True
    closed_date = <Date 2017-01-25.09:56:40.837>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2013-04-13.18:04:49.880>
    creator = 'alexandre.vassalotti'
    dependencies = []
    files = ['29810', '29846']
    hgrepos = []
    issue_num = 17720
    keywords = ['patch']
    message_count = 10.0
    messages = ['186774', '186799', '186800', '186885', '187454', '261188', '286028', '286219', '286238', '286305']
    nosy_count = 4.0
    nosy_names = ['pitrou', 'alexandre.vassalotti', 'python-dev', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue17720'
    versions = ['Python 3.3', 'Python 3.4', 'Python 3.5']

    @avassalotti
    Copy link
    Member Author

    In pickle.py, load_appends is currently defined as

        def load_appends(self):
            stack = self.stack
            mark = self.marker()
            list = stack[mark - 1]
            list.extend(stack[mark + 1:])
            del stack[mark:]

    However, according to the spec of APPENDS, it should actually be:

        def load_appends(self):
            obj = stack[mark - 1]
            items = stack[mark + 1:]
            if isinstance(obj, list):
                obj.extend(items)
            else:
                for item in items:
                    obj.append(item)

    This will match with the current behaviour of _pickle.

    @avassalotti avassalotti self-assigned this Apr 13, 2013
    @avassalotti avassalotti added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Apr 13, 2013
    @pitrou
    Copy link
    Member

    pitrou commented Apr 13, 2013

    Your patch is bogus (duplicated load_appends definition).
    Also, I think you want to add a test if you want to avoid further regressions.

    @serhiy-storchaka
    Copy link
    Member

    Yes, test is needed. Current tests don't cover this case.

    I suggest to move obj.append out of the tight loop, as in C implementation.

                append = obj.append
                for item in items:
                    append(item)

    @avassalotti
    Copy link
    Member Author

    Alright alright! Here's a less bogus patch. :)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 20, 2013

    New changeset 37139694aed0 by Alexandre Vassalotti in branch '3.3':
    Isuse bpo-17720: Fix APPENDS handling in the Python implementation of Unpickler
    http://hg.python.org/cpython/rev/37139694aed0

    @serhiy-storchaka
    Copy link
    Member

    Why this patch was not applied to 2.7? What is the spec of APPENDS?

    @serhiy-storchaka
    Copy link
    Member

    From PEP-7:

    listitems    Optional, and new in this PEP.
                 If this is not None, it should be an iterator (not a
                 sequence!) yielding successive list items.  These list
                 items will be pickled, and appended to the object using
                 either obj.append(item) or obj.extend(list_of_items).
                 This is primarily used for list subclasses, but may
                 be used by other classes as long as they have append()
                 and extend() methods with the appropriate signature.
                 (Whether append() or extend() is used depends on which
                 pickle protocol version is used as well as the number
                 of items to append, so both must be supported.)
    

    Both append() or extend() must be supported, therefore old code was correct. C implementation can be optimized by using extend().

    @avassalotti
    Copy link
    Member Author

    I don't recall where I picked up the referred spec. Looking at PEP-307 and pickle docs 1, it does look like that objects which provides the listitems field of the reduce tuple should support extend() too.

    It might best to try both in the implementation to avoid breaking existing code that relies on the previous behavior.

    @serhiy-storchaka
    Copy link
    Member

    Thanks. Opened bpo-29368 for this.

    @serhiy-storchaka
    Copy link
    Member

    From PEP-7:

    Sorry, I meant PEP-307 of course.

    @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
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants