NotificationsYou must be signed in to change notification settings
Fork32k
Star67.2k

gh-93627: Align Python implementation of pickle with C implementation of pickle#103035

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

serhiy-storchaka merged 45 commits intopython:mainfromeendebakpt:pickle_93627

Sep 10, 2023

Merged

gh-93627: Align Python implementation of pickle with C implementation of pickle#103035

serhiy-storchaka merged 45 commits intopython:mainfromeendebakpt:pickle_93627

Sep 10, 2023

Conversation

Copy link

Contributor

eendebakpt commentedMar 25, 2023•
edited
Loading

In this PR we align the python and C implementation ofpickle module. 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, aTypeError is 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:

No changes are required to the tests (although a few tests are added to verify the new behaviour)
The changes impact cases that seem rare (e.g. setting the__reduce_ex__ attribute of a class toNone, instead of creating a method)

Notes:

Changes to align thecopy module have been removed from the PR.

Issue:Inconsistency in handling None handlers in the Python and C implementations of pickle #93627

align python implementation of pickle, c implementation of pickle and…

9939a1c

… copy implementation

bedevere-bot mentioned this pull request

Mar 25, 2023

Inconsistency in handling None handlers in the Python and C implementations of pickle#93627

Open

bedevere-bot added the awaiting review label

Mar 25, 2023

Eclips4 reviewed

Mar 26, 2023

View reviewed changes

Lib/copy.py OutdatedShow resolvedHide resolved

eendebakptand others added8 commits

March 26, 2023 21:03

review comments

29445b9

review comments

95cbf77

Merge branch 'main' into pickle_93627

31d3b05

Merge branch 'pickle_93627' of github.com:eendebakpt/cpython into pic…

e928112

…kle_93627

📜🤖 Added by blurb_it.

fa2a741

Merge branch 'pickle_93627' of github.com:eendebakpt/cpython into pic…

dfc4ef2

…kle_93627

format news entry

3e6e4eb

remove unused exception

92362c1

eendebakpt mentioned this pull request

Mar 27, 2023

gh-72793: C implementation of parts of copy.deepcopy#91610

Open

erlend-aasland marked this pull request as draft

March 28, 2023 08:15

erlend-aasland changed the title~~Draft: gh-93627: Align python implementation of pickle with c implementation of pickle~~gh-93627: Align Python implementation of pickle with C implementation of pickle

Mar 28, 2023

erlend-aasland requested a review fromserhiy-storchaka

March 28, 2023 08:16

eendebakpt added3 commits

April 2, 2023 20:32

Merge branch 'main' into pickle_93627

bab3dab

Merge branch 'pickle_93627' of github.com:eendebakpt/cpython into pic…

3a6c8c5

…kle_93627

align C and python pickle implementations

9c5cd38

eendebakpt marked this pull request as ready for review

April 2, 2023 19:33

Merge branch 'main' into pickle_93627

a8a797c

Copy link

ContributorAuthor

eendebakpt commentedApr 9, 2023

@serhiy-storchaka Will you be able to review the PR?

Merge branch 'main' into pickle_93627

1f1b2ff

eendebakpt requested a review fromEclips4

April 27, 2023 19:41

Eclips4 approved these changes

Apr 27, 2023

View reviewed changes

Copy link

Member

Eclips4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM

bedevere-bot added awaiting core review and removed awaiting review labels

Apr 27, 2023

JelleZijlstra self-requested a review

April 28, 2023 13:21

CAM-Gerlach added the stdlibPython modules in the Lib dir label

Apr 28, 2023

Merge branch 'main' into pickle_93627

ec8d1c7

eendebakpt added2 commits

August 2, 2023 11:46

Merge branch 'main' into pickle_93627

3a88607

Merge branch 'main' into pickle_93627

dc7d33e

erlend-aasland reviewed

Aug 21, 2023

View reviewed changes

Modules/_pickle.c OutdatedShow resolvedHide resolved

eendebakpt added3 commits

August 21, 2023 13:37

revert spelling mistake in exception message as it is part of the pub…

d365a44

…lic api

revert adding comment to pickle c implementation

1bf64c0

Merge branch 'main' into pickle_93627

f275ede

erlend-aasland reviewed

Aug 22, 2023

View reviewed changes

Misc/NEWS.d/next/Core and Builtins/2023-03-26-19-11-10.gh-issue-93627.0UgwBL.rst OutdatedShow resolvedHide resolved

Lib/pickle.pyShow resolvedHide resolved

eendebakptand others added10 commits

August 22, 2023 08:59

Update Misc/NEWS.d/next/Core and Builtins/2023-03-26-19-11-10.gh-issu…

3548dc3

…e-93627.0UgwBL.rstCo-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

add unit test for None value of __setstate__

250accc

Merge branch 'main' into pickle_93627

1ea9a1b

fix tests

e58f4a1

add test for reduced_override

b1bd83c

add test for dispatch table

f565513

whitespace

4a928aa

fix test

39ab1ed

move docstrings

f694005

Merge branch 'main' into pickle_93627

833b2fb

serhiy-storchaka reviewed

Aug 26, 2023

View reviewed changes

Lib/test/test_pickle.py OutdatedShow resolvedHide resolved

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)

Copy link

Member

serhiy-storchakaAug 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

ContributorAuthor

eendebakptAug 28, 2023•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@serhiy-storchaka I moved the tests to be part ofAbstractPickleTests,AbstractPicklerUnpicklerObjectTests andAbstractDispatchTableTests. These are included in concrete tests both inside and outside thehas_c_implementation section oftest_pickle.py.

eendebakpt added4 commits

August 28, 2023 13:31

refactor tests

c803480

fix typo

7f3ee96

whitespace

1f02ab2

Merge branch 'main' into pickle_93627

f9dd613

serhiy-storchaka self-requested a review

September 10, 2023 06:47

serhiy-storchaka approved these changes

Sep 10, 2023

View reviewed changes

Copy link

Member

serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM.

serhiy-storchaka merged commit85a5d3d intopython:main

Sep 10, 2023

bedevere-bot removed the awaiting merge label

Sep 10, 2023

Copy link

Member

serhiy-storchaka commentedSep 10, 2023

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.

Copy link

ContributorAuthor

eendebakpt commentedSep 10, 2023

@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__copy__ or also about theNone/NoValue for the other attributes (e.g.__reduce_ex__,__reduce__)?

eendebakpt mentioned this pull request

Sep 16, 2023

gh-93627: Align copy module behaviour with pickle module#109498

Open

Copy link

ContributorAuthor

eendebakpt commentedSep 16, 2023

@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.