GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-95196: Fixed instances of types.ClassMethodDescriptorType not being pickled correctly by UltimateLobster · Pull Request #96162 · python/cpython

Conversation

@UltimateLobster

@UltimateLobster UltimateLobster commented Aug 21, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

NOTE: I want to apologize for anything that is not up to par in this PR. This is my first time writing to cpython (and writing in python's C-API in general), I would love to learn from any mistakes done here and improve for the next time. I tried my best to look at other examples and guides and derive the correct conventions and workflow from there.

Previously, instances of types.ClassMethodDescriptorType were reduced
into a wrong set of instructions. Instead of the instructions retrieving
the descriptor itself (using direct access to the object's __dict__),
the instructions retrieved the underlying classmethod (using the normal getattr)
which resulted in different object upon unpickling.
This change has replaced the implementation of __reduce__ for such
instances so it will directly access the owner's __dict__.

Since there is no publicly available builtin function (That I could
find) to get an attribute directly from the object's __dict__, and in
order for the implmentation to work with older versions of python, I
unfortunately could not avoid the usage of the "eval" builtin in the
implementation.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@UltimateLobster
Fixed instances of types.ClassMethodDescriptorType not being pickled
6790f48
correctly

Previously, instances of types.ClassMethodDescriptorType were reduced
into a wrong set of instructions. Instead of instructions to retreive
the descriptor itself (using direct access to the owner's __dict__),
the instructions retrieved the underlying classmethod (using getattr).
This change has replaced the implementation of __reduce__ for these
instances so it will directly access the owner's __dict__.

Since there is no publicly available builtin function (That I could
find) to get an attribute directly from the object's __dict__, and in
order for the implmentation to work with older versions of python, I
unfortunately could not avoid the usage of the "eval" builtin in the
implementation.
@bedevere-bot bedevere-bot added the awaiting review label Aug 21, 2022
@ghost

ghost commented Aug 21, 2022 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner
Fidget-Spinner reviewed Aug 22, 2022
View reviewed changes
Comment thread Objects/descrobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@UltimateLobster
UltimateLobster requested a review from Fidget-Spinner August 22, 2022 18:16
@UltimateLobster UltimateLobster mentioned this pull request Aug 23, 2022
Incorrect pickling behavior in the c implemented classmethod descriptors #95196
Closed
UriyaHarpeness
UriyaHarpeness approved these changes Aug 25, 2022
View reviewed changes
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Aug 25, 2022
Pagajo
Pagajo approved these changes Aug 25, 2022
View reviewed changes
@Fidget-Spinner

Fidget-Spinner commented Aug 25, 2022

Copy link
Copy Markdown
Member

Sorry for missing this. I'll try to review before the end of the week.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@UltimateLobster

UltimateLobster commented Aug 25, 2022

Copy link
Copy Markdown
Author

Sorry for missing this. I'll try to review before the end of the week.

Thank you :D

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner
Fidget-Spinner reviewed Aug 28, 2022
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@serhiy-storchaka can you please help verify if this is the correct way to solve this problem?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Objects/descrobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka
serhiy-storchaka self-requested a review August 28, 2022 13:33
@serhiy-storchaka serhiy-storchaka linked an issue Aug 28, 2022 that may be closed by this pull request
Incorrect pickling behavior in the c implemented classmethod descriptors #95196
Closed
@UltimateLobster
Added _Py_ID() for the builtin eval function and changed the reduce f…
9d625ac
…unction for instances of types.ClassMethodDescriptorType in order to use it
@UltimateLobster
UltimateLobster requested review from Fidget-Spinner and removed request for serhiy-storchaka August 28, 2022 18:31
@Fidget-Spinner
Fidget-Spinner requested a review from serhiy-storchaka August 29, 2022 01:03
@serhiy-storchaka

serhiy-storchaka commented Aug 29, 2022

Copy link
Copy Markdown
Member

Using eval() here looks clever, but perhaps too clever. One of drawbacks of eval() is that it is much slower than simple attribute resolution and dict lookup. Other is that it looks suspicious (even if it is completely safe in this case) and can trigger alarms.

In any case, I think that it is better to just disable pickling of types.ClassMethodDescriptorType, for compatibility with Python implemented classmethod descriptors and with C and Python implemented staticmethod descriptors, and because there are no use common cases for this (otherwise we would get such report years ago).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka serhiy-storchaka closed this Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner
@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka
2 more reviewers
@UriyaHarpeness UriyaHarpeness UriyaHarpeness approved these changes
@Pagajo Pagajo Pagajo approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Incorrect pickling behavior in the c implemented classmethod descriptors

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@UltimateLobster @Fidget-Spinner @serhiy-storchaka @UriyaHarpeness @Pagajo @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.