◐ Shell
reader mode source ↗
Skip to content

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

Merged
serhiy-storchaka merged 45 commits into
python:mainfrom
eendebakpt:pickle_93627
Sep 10, 2023
Merged

gh-93627: Align Python implementation of pickle with C implementation of pickle#103035
serhiy-storchaka merged 45 commits into
python:mainfrom
eendebakpt:pickle_93627

Conversation

@eendebakpt

@eendebakpt eendebakpt commented Mar 25, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

In this PR we align the python and C implementation of pickle 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 to None, a TypeError 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 to None, instead of creating a method)

Notes:

  • Changes to align the copy module have been removed from the PR.

@bedevere-bot bedevere-bot mentioned this pull request Mar 25, 2023
Open
@eendebakpt eendebakpt mentioned this pull request Mar 27, 2023
Closed
@erlend-aasland erlend-aasland marked this pull request as draft March 28, 2023 08:15
@erlend-aasland erlend-aasland changed the title Draft: gh-93627: Align python implementation of pickle with c implementation of pickle Mar 28, 2023
@eendebakpt eendebakpt marked this pull request as ready for review April 2, 2023 19:33
@eendebakpt

eendebakpt commented Apr 9, 2023

Copy link
Copy Markdown
Contributor Author

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

@eendebakpt eendebakpt requested a review from Eclips4 April 27, 2023 19:41
Eclips4
Eclips4 approved these changes Apr 27, 2023
View reviewed changes

@Eclips4 Eclips4 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

LGTM

@JelleZijlstra JelleZijlstra self-requested a review April 28, 2023 13:21
@CAM-Gerlach CAM-Gerlach added the stdlib Standard Library Python modules in the Lib/ directory label Apr 28, 2023
26 hidden items Load more…
@serhiy-storchaka serhiy-storchaka self-requested a review September 10, 2023 06:47
serhiy-storchaka
serhiy-storchaka approved these changes Sep 10, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

LGTM.

@serhiy-storchaka serhiy-storchaka merged commit 85a5d3d into python:main Sep 10, 2023
@serhiy-storchaka

serhiy-storchaka commented Sep 10, 2023

Copy link
Copy Markdown
Member

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.

@eendebakpt

eendebakpt commented Sep 10, 2023

Copy link
Copy Markdown
Contributor Author

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

@eendebakpt eendebakpt mentioned this pull request Sep 16, 2023
Open
@eendebakpt

eendebakpt commented Sep 16, 2023

Copy link
Copy Markdown
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland erlend-aasland left review comments

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@Eclips4 Eclips4 Eclips4 approved these changes

Assignees

No one assigned

Labels

stdlib Standard Library Python modules in the Lib/ directory

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@eendebakpt @erlend-aasland @JelleZijlstra @serhiy-storchaka @Eclips4 @CAM-Gerlach @bedevere-bot