fix: restore pickling of FrozenList in the C extension - #835
Conversation
for more information, see https://pre-commit.ci
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Confidence Score: 3/5The PR is not yet safe to merge because extension subclasses can still lose instance state and can fail to unpickle when their constructors do not accept the base-class item argument. The reducer now preserves dynamic class identity, but it serializes no subclass state and reconstructs through cls(items), so valid subclasses can either silently lose attributes or raise during pickle.loads. Files Needing Attention: frozenlist/_frozenlist.pyx and tests/test_frozenlist.py Reviews (4): Last reviewed commit: "style(pyx): satisfy cython-lint blank-li..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #835 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 383 402 +19
Branches 9 10 +1
=========================================
+ Hits 383 402 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The FrozenListMixin subclasses assign class-level aliases (FrozenList = FrozenList / PyFrozenList) and subclass those aliases directly, which mypy flags as [assignment]/[valid-type]/[type-arg]. The pattern is intentional runtime dispatch, so annotate with targeted ignores instead of restructuring. Fixes the red lint/Linter job on this PR.
|
The red Pushed b956d6d with targeted
Everything else in the run was already green (ruff check/format pass locally too). |
| # atomic[bint] `_frozen` member, so pickle the state explicitly. | ||
| return ( | ||
| _unpickle_frozen_list, | ||
| (type(self), list(self._items), bool(self._frozen.load())), |
There was a problem hiding this comment.
When an extension-backed FrozenList subclass with instance attributes is pickled, this reducer serializes only its class, items, and frozen flag, so reconstruction creates a fresh subclass instance without restoring those attributes. The round-trip therefore silently loses subclass-specific state, unlike the pure-Python implementation.
Context Used: CLAUDE.md (source)
…zen_list E303 (3 blank lines before the helper) and E305 (1 line after it) were introduced together with the helper itself; normalize to 2/2.
|
Follow-up to the lint fix: CI also flagged cython-lint (E303/E305 around Local verification on f7fa46a: full |
|
|
||
|
|
||
| def _unpickle_frozen_list(cls, items, frozen): | ||
| fl = cls(items) |
There was a problem hiding this comment.
When an extension-backed FrozenList subclass defines an __init__ that does not accept one positional iterable, _unpickle_frozen_list calls cls(items), causing pickle.loads to raise TypeError; constructors with side effects are also executed again during reconstruction.
2 participants
Fixes #834
Root cause
_frozenlist.pyxstores the frozen flag ascdef atomic[bint] _frozen(free-threading support). Cython's auto-generated__reduce_cython__cannot convert alibcpp.atomic.atomic[bint]member into a Python object, sopickle.dumps()on anyFrozenListraises:The pure-Python implementation pickles fine on the same interpreter — the two implementations diverged, and there were no pickle tests to notice.
Fix
An explicit
__reduce__on the cdef class serializes(items, frozen)through a module-level_unpickle_frozen_list(items, frozen)constructor. The atomic stays in place (no free-threading regression); only the serialization path changes.Testing
TestPickleC/TestPicklePycovering frozen and unfrozen round-trips for both implementations.TypeError; with it, the whole suite passes:pytest tests/ -q→ 222 passed (was 112 passed / collection without the new cases; both implementations exercised via the existing mixin pattern).deepcopy, equality and hash behavior unchanged.CHANGES/834.bugfix.rst).