python / cpython

The Python programming language
https://www.python.org/
Other
59.98k stars 29.03k forks source link

__reduce__ does not work as documented #36308

Closed nascheme closed 22 years ago

nascheme commented 22 years ago
BPO 533291
Nosy @gvanrossum, @freddrake, @nascheme, @rhettinger
Files
  • copy_reduce.diff
  • pickle.diff: Patch deprecating None form of reduce
  • 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: ```python assignee = 'https://github.com/rhettinger' closed_at = created_at = labels = ['docs'] title = '__reduce__ does not work as documented' updated_at = user = 'https://github.com/nascheme' ``` bugs.python.org fields: ```python activity = actor = 'gvanrossum' assignee = 'rhettinger' closed = True closed_date = None closer = None components = ['Documentation'] creation = creator = 'nascheme' dependencies = [] files = ['382', '383'] hgrepos = [] issue_num = 533291 keywords = [] message_count = 9.0 messages = ['9891', '9892', '9893', '9894', '9895', '9896', '9897', '9898', '9899'] nosy_count = 4.0 nosy_names = ['gvanrossum', 'fdrake', 'nascheme', 'rhettinger'] pr_nums = [] priority = 'normal' resolution = 'fixed' stage = None status = 'closed' superseder = None type = None url = 'https://bugs.python.org/issue533291' versions = ['Python 2.3'] ```

    nascheme commented 22 years ago

    The documentation for __reduce__ says that the second element of the returned tuple can either be a tuple of arguments or None. The copy module does not handle the None case:

      class C(object):
        def __reduce__(self):
            return (C, None)
    
      import copy
      copy.copy(C())
    nascheme commented 22 years ago

    Logged In: YES user_id=35752

    Added trivial patch to fix copy.py module.

    gvanrossum commented 22 years ago

    Logged In: YES user_id=6380

    I'm not sure it's that simple. While pickling a C instance indeed succeed, unpickling it raises a strange exception:

    >>> pickle.loads(pickle.dumps(C()))
    Traceback (most recent call last):
      File "<stdin>", line 1, in ?
      File "/home/guido/trunk/Lib/pickle.py", line 982, in loads
        return Unpickler(file).load()
      File "/home/guido/trunk/Lib/pickle.py", line 593, in load
        dispatch[key](self)
      File "/home/guido/trunk/Lib/pickle.py", line 842, in
    load_reduce
        value = callable.__basicnew__()
    AttributeError: type object 'C' has no attribute
    '__basicnew__'
    >>>

    It appears that an argument tuple of None signals some special Jim-Fulton-only behavior. __basicnew is a feature of ExtensionClasses (similar to __new in Python 2.2), and while ExtensionClasses work in 2.2, they're being deprecated: Zope 2.x will continue to use Python 2.1.x, and Zope 3 will require Python 2.2 or higher.

    The copy module has never worked for ExtensionClass instances (unless maybe the class defines a __copy__ method).

    Maybe the right thing to do is to document 'None' as a special case that one shouldn't use, and deprecate __basicnew__?

    (Hm, OTOH why don't I just approve your fix so we can stop thinking about this. :-)

    nascheme commented 22 years ago

    Logged In: YES user_id=35752

    I would prefer that the None version be deprecated. The documentation for __reduce__ is confusing enough as it is.

    gvanrossum commented 22 years ago

    Logged In: YES user_id=6380

    So be it.

    rhettinger commented 22 years ago

    Logged In: YES user_id=80475

    See attached documentation and code patch for deprecating the None version. If approved, re-assign to Raymond for commit.

    freddrake commented 22 years ago

    Logged In: YES user_id=3066

    Issues with the doc portion the patch:

    Once appropriate corrections are made, I'm fine with the patch.

    rhettinger commented 22 years ago

    Logged In: YES user_id=80475

    Committed libpickle.tex 1.36 and pickle.py 1.62. Closing patch.

    gvanrossum commented 22 years ago

    Logged In: YES user_id=6380

    Wouldn't it be better if the warning came on the pickling side instead of on the unpickling side?