From 55ab8fea58dd4fd1ef24012073669c46112f37a0 Mon Sep 17 00:00:00 2001 From: Charles Machalow Date: Thu, 6 Nov 2025 10:27:31 -0800 Subject: [PATCH 01/10] Make sure that frozenlists are pickle-able Add contributors / changes updates Add missing file --- CHANGES/718.bugfix.rst | 2 ++ CONTRIBUTORS.txt | 1 + docs/spelling_wordlist.txt | 1 + frozenlist/__init__.py | 44 +++++++++++++++++++++++++++++++++++++- frozenlist/_frozenlist.pyx | 10 +++++++++ tests/test_frozenlist.py | 18 ++++++++++++++++ 6 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 CHANGES/718.bugfix.rst diff --git a/CHANGES/718.bugfix.rst b/CHANGES/718.bugfix.rst new file mode 100644 index 00000000..656189b2 --- /dev/null +++ b/CHANGES/718.bugfix.rst @@ -0,0 +1,2 @@ +Fix bug where FrozenList could not be pickled/unpickled. +-- by :user:`csm10495`. diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index 0abae2ea..b7caf28f 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -1,6 +1,7 @@ - Contributors - ---------------- Andrew Svetlov +Charles Machalow Edgar Ramírez-Mondragón Marcin Konowalczyk Martijn Pieters diff --git a/docs/spelling_wordlist.txt b/docs/spelling_wordlist.txt index 1b7227a6..50ee5237 100644 --- a/docs/spelling_wordlist.txt +++ b/docs/spelling_wordlist.txt @@ -302,6 +302,7 @@ unicode unittest Unittest unix +unpickled unsets unstripped upstr diff --git a/frozenlist/__init__.py b/frozenlist/__init__.py index e83f9aa0..ff810758 100644 --- a/frozenlist/__init__.py +++ b/frozenlist/__init__.py @@ -1,11 +1,17 @@ +import copy import os import types from collections.abc import MutableSequence from functools import total_ordering +from typing import Any __version__ = "1.8.1.dev0" -__all__ = ("FrozenList", "PyFrozenList") # type: Tuple[str, ...] +__all__ = ( + "FrozenList", + "PyFrozenList", + "_reconstruct_pyfrozenlist", +) # type: Tuple[str, ...] NO_EXTENSIONS = bool(os.environ.get("FROZENLIST_NO_EXTENSIONS")) # type: bool @@ -73,7 +79,43 @@ def __hash__(self): else: raise RuntimeError("Cannot hash unfrozen list.") + def __deepcopy__(self, memo: dict[int, Any]): + obj_id = id(self) + # Return existing copy if already processed (circular reference) + if obj_id in memo: + return memo[obj_id] + + # Create new instance and register immediately + new_list = self.__class__([]) + memo[obj_id] = new_list + + # Deep copy items + new_list._items[:] = [copy.deepcopy(item, memo) for item in self._items] + + # Preserve frozen state + if self._frozen: + new_list.freeze() + + return new_list + + def __reduce__(self): + return ( + _reconstruct_pyfrozenlist, + (self._items, self._frozen), + ) + + +def _reconstruct_pyfrozenlist(items, frozen): + """Helper function to reconstruct the pure Python FrozenList during unpickling. + This function is needed since otherwise the class renaming confuses pickle.""" + fl = PyFrozenList(items) + if frozen: + fl.freeze() + return fl + + +# Store a reference to the pure Python implementation before it's potentially replaced PyFrozenList = FrozenList diff --git a/frozenlist/_frozenlist.pyx b/frozenlist/_frozenlist.pyx index a82d8c8f..f946fb7b 100644 --- a/frozenlist/_frozenlist.pyx +++ b/frozenlist/_frozenlist.pyx @@ -144,5 +144,15 @@ cdef class FrozenList: return new_list + def __reduce__(self): + return ( + self.__class__, + (self._items,), + {"_frozen": self._frozen.load()}, + ) + + def __setstate__(self, state): + self._frozen.store(state["_frozen"]) + MutableSequence.register(FrozenList) diff --git a/tests/test_frozenlist.py b/tests/test_frozenlist.py index c37f5c0d..61bf5d72 100644 --- a/tests/test_frozenlist.py +++ b/tests/test_frozenlist.py @@ -1,6 +1,7 @@ # FIXME: # mypy: disable-error-code="misc" +import pickle from collections.abc import MutableSequence from copy import deepcopy @@ -372,6 +373,23 @@ def test_deepcopy_multiple_references(self) -> None: assert len(copied[1]) == 3 # Should see the change assert len(shared) == 2 # Original unchanged + @pytest.mark.parametrize("freeze", [True, False]) + def test_picklability(self, freeze: bool) -> None: + # Test that the list can be pickled and unpickled successfully + orig = self.FrozenList([1, 2, 3]) + if freeze: + orig.freeze() + + assert orig.frozen == freeze + + pickled = pickle.dumps(orig) + unpickled = pickle.loads(pickled) + assert unpickled == orig + assert unpickled is not orig + assert list(unpickled) == list(orig) + + assert unpickled.frozen == freeze + class TestFrozenList(FrozenListMixin): FrozenList = FrozenList # type: ignore[assignment] # FIXME From 5536d0fa876ba905fb8ffacbf88e46adda5c2d44 Mon Sep 17 00:00:00 2001 From: Charles Machalow Date: Thu, 6 Nov 2025 17:12:45 -0800 Subject: [PATCH 02/10] Try to fix coverage/types --- frozenlist/__init__.py | 2 +- tests/test_frozenlist.py | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/frozenlist/__init__.py b/frozenlist/__init__.py index ff810758..197f4aeb 100644 --- a/frozenlist/__init__.py +++ b/frozenlist/__init__.py @@ -106,7 +106,7 @@ def __reduce__(self): ) -def _reconstruct_pyfrozenlist(items, frozen): +def _reconstruct_pyfrozenlist(items: list[Any], frozen: bool) -> "PyFrozenList": """Helper function to reconstruct the pure Python FrozenList during unpickling. This function is needed since otherwise the class renaming confuses pickle.""" fl = PyFrozenList(items) diff --git a/tests/test_frozenlist.py b/tests/test_frozenlist.py index 61bf5d72..e5ccfd1b 100644 --- a/tests/test_frozenlist.py +++ b/tests/test_frozenlist.py @@ -274,6 +274,20 @@ def test_deepcopy_frozen(self) -> None: with pytest.raises(RuntimeError): copied.append(4) + def test_deepcopy_frozen_circular(self) -> None: + orig = self.FrozenList([1, 2]) + orig.append(orig) # Create circular reference + orig.freeze() + copied = deepcopy(orig) + assert copied[0] == 1 + assert copied[1] == 2 + assert len(copied[2]) == 3 + assert copied[2][0] == 1 + assert copied[2][1] == 2 + assert len(copied[2][2]) == 3 + # ... and so on. Testing equality when a structure includes itself is tough. + assert orig.frozen + def test_deepcopy_nested(self) -> None: inner = self.FrozenList([1, 2]) orig = self.FrozenList([inner, 3]) From f87782fa851f3e88c23ebace659eeea4f879a8d9 Mon Sep 17 00:00:00 2001 From: Charles Machalow Date: Fri, 7 Nov 2025 16:20:52 -0800 Subject: [PATCH 03/10] Update frozenlist/__init__.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) --- frozenlist/__init__.py | 1 - 1 file changed, 1 deletion(-) diff --git a/frozenlist/__init__.py b/frozenlist/__init__.py index 197f4aeb..490b1adb 100644 --- a/frozenlist/__init__.py +++ b/frozenlist/__init__.py @@ -10,7 +10,6 @@ __all__ = ( "FrozenList", "PyFrozenList", - "_reconstruct_pyfrozenlist", ) # type: Tuple[str, ...] From 6187963c50e8fd15d865909fc8fc41fa904f86f0 Mon Sep 17 00:00:00 2001 From: Charles Machalow Date: Fri, 7 Nov 2025 16:20:59 -0800 Subject: [PATCH 04/10] Update frozenlist/__init__.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) --- frozenlist/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frozenlist/__init__.py b/frozenlist/__init__.py index 490b1adb..d3cc677c 100644 --- a/frozenlist/__init__.py +++ b/frozenlist/__init__.py @@ -78,7 +78,7 @@ def __hash__(self): else: raise RuntimeError("Cannot hash unfrozen list.") - def __deepcopy__(self, memo: dict[int, Any]): + def __deepcopy__(self, memo: dict[int, object]): obj_id = id(self) # Return existing copy if already processed (circular reference) From 76bbbcd39cebf7ef13832a910276b1ec2339f44c Mon Sep 17 00:00:00 2001 From: Charles Machalow Date: Fri, 7 Nov 2025 16:26:10 -0800 Subject: [PATCH 05/10] PR feedback --- CHANGES/718.bugfix.rst | 5 ++++- frozenlist/__init__.py | 3 +-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/CHANGES/718.bugfix.rst b/CHANGES/718.bugfix.rst index 656189b2..61e504a4 100644 --- a/CHANGES/718.bugfix.rst +++ b/CHANGES/718.bugfix.rst @@ -1,2 +1,5 @@ -Fix bug where FrozenList could not be pickled/unpickled. +Fixed a bug where FrozenList could not be pickled/unpickled. + +Pickling is a requirement for FrozenList to be able to be passed to multiprocessing Processes. Without this +fix, users receive a TypeError upon attempting to pickle a FrozenList. -- by :user:`csm10495`. diff --git a/frozenlist/__init__.py b/frozenlist/__init__.py index d3cc677c..b96ce5eb 100644 --- a/frozenlist/__init__.py +++ b/frozenlist/__init__.py @@ -3,7 +3,6 @@ import types from collections.abc import MutableSequence from functools import total_ordering -from typing import Any __version__ = "1.8.1.dev0" @@ -105,7 +104,7 @@ def __reduce__(self): ) -def _reconstruct_pyfrozenlist(items: list[Any], frozen: bool) -> "PyFrozenList": +def _reconstruct_pyfrozenlist(items: list[object], frozen: bool) -> "PyFrozenList": """Helper function to reconstruct the pure Python FrozenList during unpickling. This function is needed since otherwise the class renaming confuses pickle.""" fl = PyFrozenList(items) From e1a2e8b67a425ed20ebf5f823febf799563a7422 Mon Sep 17 00:00:00 2001 From: Charles Machalow Date: Fri, 7 Nov 2025 16:27:54 -0800 Subject: [PATCH 06/10] PR feedback: add human readble ids --- tests/test_frozenlist.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_frozenlist.py b/tests/test_frozenlist.py index e5ccfd1b..6ca3c318 100644 --- a/tests/test_frozenlist.py +++ b/tests/test_frozenlist.py @@ -387,7 +387,7 @@ def test_deepcopy_multiple_references(self) -> None: assert len(copied[1]) == 3 # Should see the change assert len(shared) == 2 # Original unchanged - @pytest.mark.parametrize("freeze", [True, False]) + @pytest.mark.parametrize("freeze", [True, False], ids=["frozen", "not frozen"]) def test_picklability(self, freeze: bool) -> None: # Test that the list can be pickled and unpickled successfully orig = self.FrozenList([1, 2, 3]) From 0b56ac48d209463ca492c66d9bc88b96b9a6256d Mon Sep 17 00:00:00 2001 From: Charles Machalow Date: Sat, 8 Nov 2025 17:28:04 -0800 Subject: [PATCH 07/10] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) --- CHANGES/718.bugfix.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGES/718.bugfix.rst b/CHANGES/718.bugfix.rst index 61e504a4..b23860a2 100644 --- a/CHANGES/718.bugfix.rst +++ b/CHANGES/718.bugfix.rst @@ -1,5 +1,5 @@ -Fixed a bug where FrozenList could not be pickled/unpickled. +Fixed a bug where :class:`~frozenlist.FrozenList` could not be pickled/unpickled. -Pickling is a requirement for FrozenList to be able to be passed to multiprocessing Processes. Without this -fix, users receive a TypeError upon attempting to pickle a FrozenList. +Pickling is a requirement for :class:`~frozenlist.FrozenList` to be able to be passed to :class:`multiprocessing.Process`\ es. Without this +fix, users receive a :exc:`TypeError` upon attempting to pickle a :class:`~frozenlist.FrozenList`. -- by :user:`csm10495`. From b1182649f2d00da83ebcefe1660cf4d196662013 Mon Sep 17 00:00:00 2001 From: Charles Machalow Date: Sat, 8 Nov 2025 17:46:24 -0800 Subject: [PATCH 08/10] More PR fixes --- frozenlist/__init__.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/frozenlist/__init__.py b/frozenlist/__init__.py index b96ce5eb..ea5e147d 100644 --- a/frozenlist/__init__.py +++ b/frozenlist/__init__.py @@ -6,10 +6,7 @@ __version__ = "1.8.1.dev0" -__all__ = ( - "FrozenList", - "PyFrozenList", -) # type: Tuple[str, ...] +__all__ = ("FrozenList", "PyFrozenList") # type: Tuple[str, ...] NO_EXTENSIONS = bool(os.environ.get("FROZENLIST_NO_EXTENSIONS")) # type: bool @@ -104,7 +101,11 @@ def __reduce__(self): ) -def _reconstruct_pyfrozenlist(items: list[object], frozen: bool) -> "PyFrozenList": +# Store a reference to the pure Python implementation before it's potentially replaced +PyFrozenList = FrozenList + + +def _reconstruct_pyfrozenlist(items: list[object], frozen: bool) -> PyFrozenList: """Helper function to reconstruct the pure Python FrozenList during unpickling. This function is needed since otherwise the class renaming confuses pickle.""" fl = PyFrozenList(items) @@ -113,10 +114,6 @@ def _reconstruct_pyfrozenlist(items: list[object], frozen: bool) -> "PyFrozenLis return fl -# Store a reference to the pure Python implementation before it's potentially replaced -PyFrozenList = FrozenList - - if not NO_EXTENSIONS: try: from ._frozenlist import FrozenList as CFrozenList # type: ignore From 1e048f4462afc542b0845e4710ed58ce5e6b1f24 Mon Sep 17 00:00:00 2001 From: Charles Machalow Date: Sun, 9 Nov 2025 14:19:08 -0800 Subject: [PATCH 09/10] PR feedback. Add pickability forward compat test. Remove dead code for memo checking in deepcopy, use better way for self-including check test --- frozenlist/__init__.py | 4 ---- tests/test_frozenlist.py | 43 +++++++++++++++++++++++++++++++++++----- 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/frozenlist/__init__.py b/frozenlist/__init__.py index ea5e147d..1a28b93c 100644 --- a/frozenlist/__init__.py +++ b/frozenlist/__init__.py @@ -77,10 +77,6 @@ def __hash__(self): def __deepcopy__(self, memo: dict[int, object]): obj_id = id(self) - # Return existing copy if already processed (circular reference) - if obj_id in memo: - return memo[obj_id] - # Create new instance and register immediately new_list = self.__class__([]) memo[obj_id] = new_list diff --git a/tests/test_frozenlist.py b/tests/test_frozenlist.py index 6ca3c318..39f18bdb 100644 --- a/tests/test_frozenlist.py +++ b/tests/test_frozenlist.py @@ -281,11 +281,8 @@ def test_deepcopy_frozen_circular(self) -> None: copied = deepcopy(orig) assert copied[0] == 1 assert copied[1] == 2 - assert len(copied[2]) == 3 - assert copied[2][0] == 1 - assert copied[2][1] == 2 - assert len(copied[2][2]) == 3 - # ... and so on. Testing equality when a structure includes itself is tough. + assert copied[2] is copied + assert copied is not orig assert orig.frozen def test_deepcopy_nested(self) -> None: @@ -404,6 +401,42 @@ def test_picklability(self, freeze: bool) -> None: assert unpickled.frozen == freeze + @pytest.mark.parametrize("freeze", [True, False], ids=["frozen", "not frozen"]) + def test_picklability_forward_compatible(self, freeze: bool) -> None: + orig = self.FrozenList([1, 2]) + if freeze: + orig.freeze() + + assert orig.frozen == freeze + + # 0 is the original pickle protocol. It's compatible with all supported Python versions. + pickled = pickle.dumps(orig, protocol=0) + + # If this test breaks, we've changed the frozenlist data structure in an incompatible way + # with previous pickled binaries. + if self.FrozenList is FrozenList: + if freeze: + assert ( + pickled + == b"cfrozenlist._frozenlist\nFrozenList\np0\n((lp1\nI1\naI2\natp2\nRp3\n(dp4\nV_frozen\np5\nI01\nsb." + ) + else: + assert ( + pickled + == b"cfrozenlist._frozenlist\nFrozenList\np0\n((lp1\nI1\naI2\natp2\nRp3\n(dp4\nV_frozen\np5\nI00\nsb." + ) + elif self.FrozenList is PyFrozenList: + if freeze: + assert ( + pickled + == b"cfrozenlist\n_reconstruct_pyfrozenlist\np0\n((lp1\nI1\naI2\naI01\ntp2\nRp3\n." + ) + else: + assert ( + pickled + == b"cfrozenlist\n_reconstruct_pyfrozenlist\np0\n((lp1\nI1\naI2\naI00\ntp2\nRp3\n." + ) + class TestFrozenList(FrozenListMixin): FrozenList = FrozenList # type: ignore[assignment] # FIXME From 861e60ddcbba155597bc90c2f9f671f9b7171f70 Mon Sep 17 00:00:00 2001 From: Charles Machalow Date: Sun, 9 Nov 2025 14:54:19 -0800 Subject: [PATCH 10/10] General cleanup. Reverse the class order to allowing pickling to work a bit more natively. Make the testing a bit smarter to auto skip CFrozenList tests if they don't apply. --- frozenlist/__init__.py | 22 +++++++++------------- frozenlist/_frozenlist.pyx | 4 ---- tests/test_frozenlist.py | 36 ++++++++++++++++++++++++++++-------- 3 files changed, 37 insertions(+), 25 deletions(-) diff --git a/frozenlist/__init__.py b/frozenlist/__init__.py index 1a28b93c..dc93dc6c 100644 --- a/frozenlist/__init__.py +++ b/frozenlist/__init__.py @@ -13,7 +13,7 @@ @total_ordering -class FrozenList(MutableSequence): +class PyFrozenList(MutableSequence): __slots__ = ("_frozen", "_items") __class_getitem__ = classmethod(types.GenericAlias) @@ -92,22 +92,18 @@ def __deepcopy__(self, memo: dict[int, object]): def __reduce__(self): return ( - _reconstruct_pyfrozenlist, - (self._items, self._frozen), + self.__class__, + (self._items,), + {"_frozen": self._frozen}, ) + def __setstate__(self, state: dict[str, object]): + self._frozen = state["_frozen"] -# Store a reference to the pure Python implementation before it's potentially replaced -PyFrozenList = FrozenList - -def _reconstruct_pyfrozenlist(items: list[object], frozen: bool) -> PyFrozenList: - """Helper function to reconstruct the pure Python FrozenList during unpickling. - This function is needed since otherwise the class renaming confuses pickle.""" - fl = PyFrozenList(items) - if frozen: - fl.freeze() - return fl +# Rename the pure Python implementation. The C extension (if available) will +# override this name. +FrozenList = PyFrozenList if not NO_EXTENSIONS: diff --git a/frozenlist/_frozenlist.pyx b/frozenlist/_frozenlist.pyx index f946fb7b..6a3a3e8a 100644 --- a/frozenlist/_frozenlist.pyx +++ b/frozenlist/_frozenlist.pyx @@ -127,10 +127,6 @@ cdef class FrozenList: cdef FrozenList new_list obj_id = id(self) - # Return existing copy if already processed (circular reference) - if obj_id in memo: - return memo[obj_id] - # Create new instance and register immediately new_list = self.__class__([]) memo[obj_id] = new_list diff --git a/tests/test_frozenlist.py b/tests/test_frozenlist.py index 39f18bdb..3a57c81f 100644 --- a/tests/test_frozenlist.py +++ b/tests/test_frozenlist.py @@ -9,6 +9,13 @@ from frozenlist import FrozenList, PyFrozenList +try: + from frozenlist import CFrozenList + + HAS_CFROZENLIST = True +except ImportError: + HAS_CFROZENLIST = False + class FrozenListMixin: FrozenList = NotImplemented @@ -414,33 +421,46 @@ def test_picklability_forward_compatible(self, freeze: bool) -> None: # If this test breaks, we've changed the frozenlist data structure in an incompatible way # with previous pickled binaries. - if self.FrozenList is FrozenList: + if self.FrozenList is PyFrozenList: if freeze: assert ( pickled - == b"cfrozenlist._frozenlist\nFrozenList\np0\n((lp1\nI1\naI2\natp2\nRp3\n(dp4\nV_frozen\np5\nI01\nsb." + == b"cfrozenlist\nPyFrozenList\np0\n((lp1\nI1\naI2\natp2\nRp3\n(dp4\nV_frozen\np5\nI01\nsb." ) else: assert ( pickled - == b"cfrozenlist._frozenlist\nFrozenList\np0\n((lp1\nI1\naI2\natp2\nRp3\n(dp4\nV_frozen\np5\nI00\nsb." + == b"cfrozenlist\nPyFrozenList\np0\n((lp1\nI1\naI2\natp2\nRp3\n(dp4\nV_frozen\np5\nI00\nsb." ) - elif self.FrozenList is PyFrozenList: + elif self.FrozenList is FrozenList: if freeze: assert ( pickled - == b"cfrozenlist\n_reconstruct_pyfrozenlist\np0\n((lp1\nI1\naI2\naI01\ntp2\nRp3\n." + == b"cfrozenlist._frozenlist\nFrozenList\np0\n((lp1\nI1\naI2\natp2\nRp3\n(dp4\nV_frozen\np5\nI01\nsb." ) else: assert ( pickled - == b"cfrozenlist\n_reconstruct_pyfrozenlist\np0\n((lp1\nI1\naI2\naI00\ntp2\nRp3\n." + == b"cfrozenlist._frozenlist\nFrozenList\np0\n((lp1\nI1\naI2\natp2\nRp3\n(dp4\nV_frozen\np5\nI00\nsb." ) + else: + pytest.fail("Unknown FrozenList implementation.") -class TestFrozenList(FrozenListMixin): - FrozenList = FrozenList # type: ignore[assignment] # FIXME +if HAS_CFROZENLIST: + # If we don't have CFrozenList, skip adding the test class specifically for it. + class TestFrozenList(FrozenListMixin): + FrozenList = CFrozenList # type: ignore[assignment] # FIXME class TestFrozenListPy(FrozenListMixin): + # All implementations will at least have the Python version. FrozenList = PyFrozenList # type: ignore[assignment] # FIXME + + +def test_frozenlist_aliasing() -> None: + """Test that FrozenList name points to the C extension if available, else to PyFrozenList.""" + if HAS_CFROZENLIST: + assert FrozenList is CFrozenList + else: + assert FrozenList is PyFrozenList