gh-125004: fix unpickling for dataclasses in a cycle via hashing #125005
Conversation
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
workaround for gh python#124937, following the recommended approach at python#124937 (comment).
charles-cooper
force-pushed
the
fix/dataclasses-reduce
branch
from
185c720 to
5b6ccea
Compare
charles-cooper
requested review from
brettcannon,
a team,
encukou,
ethanfurman,
gvanrossum,
JelleZijlstra,
AlexWaygood,
terryjreedy,
rhettinger,
berkerpeksag,
gpshead,
erlend-aasland,
cjw296,
isidentical,
pablogsal,
lysnikolaou,
pganssle,
abalkin,
ericsnowcurrently,
ncoghlan and
warsaw
as code owners
Eclips4
requested review from
carljm and
sobolevn
and removed request for
gvanrossum,
ethanfurman,
markshannon,
pablogsal,
erlend-aasland,
terryjreedy,
lysnikolaou,
ezio-melotti,
brandtbucher,
isidentical and
AlexWaygood
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Misc/NEWS.d/next/Library/2024-10-05-10-24-48.gh-issue-125004.noUctj.rst
Outdated
Show resolved
Hide resolved
…oUctj.rst Co-authored-by: Peter Bierma <[email protected]>
| newcls.__getstate__ = _dataclass_getstate | ||
| if '__setstate__' not in cls_dict: | ||
| newcls.__setstate__ = _dataclass_setstate | ||
| if '__reduce__' not in cls_dict: |
There was a problem hiding this comment.
Shouldn't this keep the is_frozen condition? If not, can you explain why not? (nothing uses that arg any more if not, so remove the arg and update the caller if so)
There was a problem hiding this comment.
no, i don't think so, because we need the new reduce function no matter if the dataclass is frozen or not.
however, looking at the tests, it looks like maybe __getstate__ and __setstate__ are required if the dataclass has slots?
There was a problem hiding this comment.
Pondering... I wonder if always having a __reduce__ has meaningful performance implications?
gpshead
changed the title
__hash__
gpshead
changed the title
__hash__
|
FYI - I marked this PR as a Draft as tons of tests are failing. |
Yep, makes sense. I wanted to get some feedback on the approach first. Also this is my first pull request to the repository, I don't have the build or test suite set up. What is the recommended way here to run tests locally? |
Check out our devguide, it should in theory answer everything like that. |
workaround for gh #124937 specific to dataclasses, following the recommended approach at
#124937 (comment).
fixes #125004
to reviewers:
i branched off of 3.11 since that is what i have on my machine. if you would like me to branch off of main instead, please let me know.EDIT: never mind, i changed the base branch to main: #125005 (comment)