-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-125004: fix unpickling for dataclasses in a cycle via hashing #125005
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 4 commits
5b6ccea
6a982b0
e3b3d45
ef81c10
9bfd1f2
430e441
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1184,17 +1184,21 @@ def _process_class(cls, init, repr, eq, order, unsafe_hash, frozen, | |
| return cls | ||
|
|
||
|
|
||
| # _dataclass_getstate and _dataclass_setstate are needed for pickling frozen | ||
| # classes with slots. These could be slightly more performant if we generated | ||
| # _dataclass_reduce and _dataclass_produce are needed because there is a | ||
| # bug in pickle when there is a cycle in the object graph and the object | ||
| # serves as a key for a dict, set or frozenset in one of its descendants. | ||
| # cf. gh python/cpython#124937. | ||
| # | ||
| # These could be slightly more performant if we generated | ||
| # the code instead of iterating over fields. But that can be a project for | ||
| # another day, if performance becomes an issue. | ||
| def _dataclass_getstate(self): | ||
| return [getattr(self, f.name) for f in fields(self)] | ||
| def _dataclass_reduce(self): | ||
| return _dataclass_produce, ([getattr(self, f.name) for f in fields(self)],) | ||
|
|
||
|
|
||
| def _dataclass_setstate(self, state): | ||
| for field, value in zip(fields(self), state): | ||
| # use setattr because dataclass may be frozen | ||
| def _dataclass_produce(self, data): | ||
| for field, value in zip(fields(self), data): | ||
| # use `object.__setattr__` because dataclass may be frozen. | ||
| object.__setattr__(self, field.name, value) | ||
|
|
||
|
|
||
|
|
@@ -1305,12 +1309,8 @@ def _add_slots(cls, is_frozen, weakref_slot, defined_fields): | |
| if qualname is not None: | ||
| newcls.__qualname__ = qualname | ||
|
|
||
| if is_frozen: | ||
| # Need this for pickling frozen classes with slots. | ||
| if '__getstate__' not in cls_dict: | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this keep the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pondering... I wonder if always having a |
||
| cls.__reduce__ = _dataclass_reduce | ||
gpshead marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| # Fix up any closures which reference __class__. This is used to | ||
| # fix zero argument super so that it points to the correct class | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Fix unpickling for :mod:`dataclasses` with hash-based data structures in their | ||
| descendants in the presence of cycles. |
Uh oh!
There was an error while loading. Please reload this page.