GeezerVison is showing github.com — gh-95196: Fixed instances of types.ClassMethodDescriptorType not being pickled correctly by UltimateLobster · Pull Request #96162 · python/cpython
Conversation
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
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.
ghost
commented
Aug 21, 2022
•
edited by ghost
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
All commit authors signed the Contributor License Agreement. 
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
Sorry for missing this. I'll try to review before the end of the week.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry for missing this. I'll try to review before the end of the week.
Thank you :D
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Uh oh!
There was an error while loading. Please reload this page.
…unction for instances of types.ClassMethodDescriptorType in order to use it
|
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).
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
2 more reviewers
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
Milestone
No milestone
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.
|