gh-93627: Align Python implementation of pickle with C implementation of pickle #103035
Conversation
… copy implementation
erlend-aasland
changed the title
|
@serhiy-storchaka Will you be able to review the PR? |
Misc/NEWS.d/next/Core and Builtins/2023-03-26-19-11-10.gh-issue-93627.0UgwBL.rst
Outdated
Show resolved
Hide resolved
…e-93627.0UgwBL.rst Co-authored-by: Erlend E. Aasland <[email protected]>
Lib/test/test_pickle.py
Outdated
| def test_pickle_invalid_reduction_method(self): | ||
| c = C_None_reduce_ex() | ||
| with self.assertRaises(TypeError): | ||
| pickle.dumps(c) |
There was a problem hiding this comment.
What implementation does it test?
The tests in this module is organized in special way. There are abstract classes which test with the specified implementation specified by class attributes, and there are concrete subclasses which specialize tests for Python and C implementations. It would be better to rewrite your tests in conformance with existing style.
There was a problem hiding this comment.
@serhiy-storchaka I moved the tests to be part of AbstractPickleTests, AbstractPicklerUnpicklerObjectTests and AbstractDispatchTableTests. These are included in concrete tests both inside and outside the has_c_implementation section of test_pickle.py.
|
Thank you for your contribution @eendebakpt. Do you mind to align also the copy module with the pickle module? It is more complex and dangerous change, because we may need to change the order of some operations which is different in copy and pickle modules. |
|
@serhiy-storchaka If aligning the copy of pickle module is still blocking #91610 I will have a look at the alignment. However, Jelle Zijlstra remarked in #103035 (comment) that the alignment might break some existing code, so we reverted these changes for this PR. @JelleZijlstra was your remark only about the |
|
@serhiy-storchaka @JelleZijlstra I created #109498 to get a feeling for the impact of aligning the behaviour of the copy module with the pickle module. Maybe more needs to be aligned, but the PR seems quite straightforward so far. |
In this PR we align the python and C implementation of
picklemodule. In this PR we have chosen to modify all implementations to follow the c implementation of pickle conventions: if a method like__reduce_ex_or__reduce__is set toNone, aTypeErroris raised.Since the behaviour of the python and c implemenations is not equal, any PR aliging the two cannot be fully backwards compatible. The impact seems limited:
__reduce_ex__attribute of a class toNone, instead of creating a method)Notes:
copymodule have been removed from the PR.