From 5c9ae58fcbdd9f5c396ad915e0694002c16fb359 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Sun, 5 Jul 2026 08:19:55 +0200 Subject: [PATCH 1/2] Add wrapped_self() to fix Issue #18 (self is unwrapped in delegation-wrapped stores) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dol wraps by delegation (has-a): a method defined on a wrap_kvs-decorated class runs bound to the inner, unwrapped store, so `self[k]` inside it bypasses the transform pipeline (#18). This adds `wrapped_self(self)` — an additive, opt-in accessor that recovers the outer, transform-applying store, so authors write `wrapped_self(self)[k]`. - id-keyed weakref registry populated in Store.__init__ (covers class- AND instance-wraps) and re-populated in Store.__setstate__ (unpickle bypasses __init__); guarded compare-and-pop finalizer. - climbs to the OUTERMOST wrapper for stacked/Pipe wraps; safe no-op on direct Store subclasses and plain objects. - Zero default behavior change: dependents test-gate (top 15) stays green, 0 PASS->FAIL. 7 regression tests + a runnable doctest. Design rationale and the deferred structural fix (is-a wrapping, which would also close #6) are in misc/docs/dol_issue18_design.md. Refs #18 Claude-Session: https://claude.ai/code/session_01H8PmV6xmMhzV64zi1Twraw --- CLAUDE.md | 2 +- dol/__init__.py | 1 + dol/base.py | 98 +++++++++++++++++++++++++++++++++ dol/tests/base_test.py | 120 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 220 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index b2f703a6..81497e94 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -215,7 +215,7 @@ Run tests: `pytest dol/tests/` ## Known Limitations / Gotchas -- **`wrap_kvs` + `self` inside methods**: When a `wrap_kvs`-decorated class uses `self[k]` in its own methods, `self` is the unwrapped instance. Re-apply the wrapper to `self` if transforms are needed (Issue #18, still open — delegation architecture). +- **`wrap_kvs` + `self` inside methods**: When a `wrap_kvs`-decorated class uses `self[k]` in its own methods, `self` is the unwrapped inner store, so transforms are bypassed (Issue #18 — delegation architecture). Blessed fix: `from dol import wrapped_self` and write `wrapped_self(self)[k]` to reach the outer, transform-applying store (climbs to the outermost wrapper for stacked/`Pipe` wraps; a safe no-op on direct `Store` subclasses). The older `sq(self)[k]` re-wrap still works. A structural fix (is-a wrapping) is proposed in `misc/docs/dol_issue18_design.md`. - **`clear()` is disabled** on `KvPersister`. Call `ensure_clear_to_kv_store(store)` to re-enable. - **No async support** in core. Use synchronous wrappers for async backends (thread pool, etc.). - **Transforms wanting the store**: a transform is called `f(self, data)` only if its first param is named `self`/`store`/`mapping` **and** it has ≥2 required params; otherwise `f(data)`. Mark explicitly with `FirstArgIsMapping(f)`. (`bytes.decode` as `obj_of_data` now works — Issue #9 fixed.) diff --git a/dol/__init__.py b/dol/__init__.py index 92967055..486df78d 100644 --- a/dol/__init__.py +++ b/dol/__init__.py @@ -57,6 +57,7 @@ def ihead(store, n=1): Persister, # TODO: deprecate? (now KvPersister) kv_walk, # walk a kv store Store, # base class for stores (adds hooks for key and value transforms) + wrapped_self, # recover the outer (transform-applying) store inside a wrapped method ) diff --git a/dol/base.py b/dol/base.py index 022f5161..95f5a155 100644 --- a/dol/base.py +++ b/dol/base.py @@ -26,6 +26,7 @@ from functools import partial, update_wrapper import copyreg +import weakref from collections.abc import Collection as CollectionABC from collections.abc import Mapping, MutableMapping from collections.abc import ( @@ -285,6 +286,93 @@ def __delete__(self, instance): delattr(getattr(instance, self.delegate_name), self.attr_name) +# --------------------------------------------------------------------------------------- +# wrapped_self: recover the outer (transform-applying) store from inside a wrapped +# class's own method. See Issue #18 and misc/docs/dol_issue18_design.md. +# +# dol wraps by delegation (has-a): ``wrap_kvs(SomeClass)`` returns a ``Store`` subclass +# ``Wrap`` that HOLDS an instance of ``SomeClass`` in ``self.store``. A method defined on +# ``SomeClass`` is copied onto ``Wrap`` as a ``DelegatedAttribute`` and runs bound to the +# INNER, unwrapped ``self.store`` -- so ``self[k]`` inside such a method bypasses the +# transform pipeline (Issue #18). ``wrapped_self(self)`` maps that inner store back to the +# outer wrapper so ``self[k]`` can be routed through the transforms as +# ``wrapped_self(self)[k]``. +# +# The mapping is kept in a module-global registry of weak references, keyed by ``id`` of +# the inner store, and populated once per ``Store`` construction (in ``Store.__init__`` and +# re-populated in ``Store.__setstate__``, since unpickling bypasses ``__init__``). We key by +# ``id`` -- rather than setting an attribute on the inner store -- because inner stores are +# often plain ``dict``/C backends that reject instance attributes and whose pickling a stray +# weakref would break. +_wrapper_backrefs: "dict[int, weakref.ReferenceType]" = {} + + +def _register_wrapper_backref(inner: Any, wrapper: "Store") -> None: + """Record that ``wrapper`` holds ``inner`` as its delegated store. + + Registers ``id(inner) -> weakref(wrapper)``. When ``wrapper`` is garbage-collected the + weakref callback removes the entry, but only if it still points at *this* wrapper + (guarded compare-and-pop) -- so a later registration for a reused ``id`` is never + clobbered by an earlier wrapper's finalizer. + """ + inner_id = id(inner) + + def _cleanup(dead_ref: "weakref.ReferenceType") -> None: + if _wrapper_backrefs.get(inner_id) is dead_ref: + _wrapper_backrefs.pop(inner_id, None) + + try: + _wrapper_backrefs[inner_id] = weakref.ref(wrapper, _cleanup) + except TypeError: + # wrapper not weak-referenceable (not expected for a Store) -- skip silently: + # wrapped_self simply falls back to returning the inner store unchanged. + pass + + +def wrapped_self(obj: Any) -> Any: + """Return the outermost transform-applying store wrapping ``obj``, else ``obj`` itself. + + Use this inside a method DEFINED ON a class that was wrapped by dol's delegation + machinery (``wrap_kvs`` / ``filt_iter`` / ``cached_keys`` / ``mk_relative_path_store`` / + ``Store.wrap`` applied to the *class*). There, ``self`` is bound to the inner, unwrapped + store, so ``self[k]`` bypasses the transforms (Issue #18). Writing + ``wrapped_self(self)[k]`` recovers the outer, transform-applying view. + + On a direct ``Store``/``KvReader`` subclass, or a plain object that never went through + the delegation machinery, there is no registered wrapper and ``obj`` is returned + unchanged -- a safe no-op. If the store was wrapped in several layers (e.g. via + ``Pipe``), this climbs to the OUTERMOST wrapper. + + >>> import math + >>> from dol import wrap_kvs, wrapped_self + >>> sq = wrap_kvs(data_of_obj=lambda x: x * x, obj_of_data=lambda x: math.sqrt(x)) + >>> @sq + ... class S(dict): + ... def via_self(self, k): + ... return self[k] # self is the INNER store: NOT transformed + ... def via_wrapped_self(self, k): + ... return wrapped_self(self)[k] # outer store: transform applied + >>> s = S() + >>> s['2'] = 2 + >>> s['2'] # external access is transformed + 2.0 + >>> s.via_self('2') # Issue #18: bypasses the transform + 4 + >>> s.via_wrapped_self('2') # wrapped_self recovers the transformed value + 2.0 + """ + seen = set() + cur = obj + while True: + ref = _wrapper_backrefs.get(id(cur)) + nxt = ref() if ref is not None else None + if nxt is None or id(nxt) in seen: + break + seen.add(id(nxt)) + cur = nxt + return cur + + Decorator = Callable[[Callable], Any] # TODO: Look up typing protocols @@ -588,6 +676,10 @@ def __init__(self, store=dict): self.store = store + # Record inner-store -> self, so wrapped_self(inner) can recover this wrapper from + # inside a delegated method whose ``self`` is the inner store (Issue #18). + _register_wrapper_backref(store, self) + if hasattr(self.store, "KeysView"): self.KeysView = self.store.KeysView @@ -731,6 +823,12 @@ def __setstate__(self, state: dict): for attr in Store._state_attrs: if attr in state: setattr(self, attr, state[attr]) + # Unpickling reconstructs via copyreg (bypassing __init__), so re-register the + # inner-store -> self back-reference here too (else wrapped_self would silently + # revert to the Issue #18 raw behavior after a pickle round-trip). + inner = getattr(self, "store", None) + if inner is not None: + _register_wrapper_backref(inner, self) # Store.register(dict) # TODO: Would this be a good idea? To make isinstance({}, Store) be True (though missing head()) diff --git a/dol/tests/base_test.py b/dol/tests/base_test.py index 4be55c28..b2082dde 100644 --- a/dol/tests/base_test.py +++ b/dol/tests/base_test.py @@ -1,5 +1,7 @@ """Testing base.py objects""" +import math +import pickle from typing import KT, VT, Tuple from collections.abc import Iterable import pytest @@ -9,6 +11,7 @@ wrap_kvs, filt_iter, cached_keys, + wrapped_self, ) from dol.base import BaseItemsView, BaseKeysView, BaseValuesView from dol.trans import take_everything @@ -195,3 +198,120 @@ def hi(): if errors: first_error, *_ = errors raise first_error + + +# --------------------------------------------------------------------------------------- +# wrapped_self / Issue #18 ("self is the unwrapped inner store" in delegation-wrapped +# classes). See misc/docs/dol_issue18_design.md. +# +# Module-level named transforms + a non-rebound base class so the wrapped store is +# picklable (lambdas and name-rebound classes are not — pre-existing dol limitations). + +def _square(x): + return x * x + + +def _sqrt(x): + return math.sqrt(x) + + +_square_wrap = wrap_kvs(data_of_obj=_square, obj_of_data=_sqrt) + + +class _SquareStoreBase(dict): + """A store whose stored values are the square of the value written.""" + + def via_self(self, k): + return self[k] # self is the INNER store -> Issue #18: NOT transformed + + def via_wrapped_self(self, k): + return wrapped_self(self)[k] # outer store -> transform applied + + +# NOTE: assign to a NEW name (do not rebind _SquareStoreBase) so the __reduce__ pickle +# path can still resolve the original wrapped class by qualified name. +_SquareStore = _square_wrap(_SquareStoreBase) + + +def test_issue18_reproduced_and_wrapped_self_fixes_it(): + """The #18 bug is real, and wrapped_self(self)[k] recovers the transformed value.""" + s = _SquareStore() + s["2"] = 2 + assert s["2"] == 2.0 # external access is transformed (sqrt of stored 4) + assert s.via_self("2") == 4 # Issue #18: inner-bound self bypasses the transform + assert s.via_wrapped_self("2") == 2.0 # wrapped_self recovers the transformed value + + +def test_wrapped_self_climbs_to_outermost_when_nested(): + """Under stacked/Pipe wrapping, wrapped_self returns the OUTERMOST wrapper.""" + outer_wrap = wrap_kvs(obj_of_data=lambda x: x + 100, data_of_obj=lambda x: x - 100) + + @outer_wrap + @_square_wrap + class Nested(dict): + def via_wrapped_self(self, k): + return wrapped_self(self)[k] + + s = Nested() + s["2"] = 2 + # via_wrapped_self must match the fully-transformed external read, not a partial one + assert s.via_wrapped_self("2") == s["2"] + + +def test_wrapped_self_is_noop_on_direct_store_subclass(): + """A direct Store subclass already has self == the store, so wrapped_self is a no-op.""" + + class Direct(Store): + def _obj_of_data(self, data): + return data * 10 + + def via_wrapped_self(self, k): + return wrapped_self(self)[k] + + d = Direct() + d.store["a"] = 5 + assert d["a"] == 50 + assert d.via_wrapped_self("a") == 50 # wrapped_self(self) is self here + assert wrapped_self(d) is d + + +def test_wrapped_self_is_noop_on_plain_objects(): + class Plain: + pass + + p = Plain() + assert wrapped_self(p) is p + assert wrapped_self(123) == 123 + assert wrapped_self("hello") == "hello" + + +def test_wrapped_self_covers_instance_wrap(): + """wrap_kvs(instance) also registers (via Store.__init__), so wrapped_self resolves.""" + inner = {"a": 1} + s = wrap_kvs(inner, obj_of_data=lambda x: x * 10) + assert s["a"] == 10 + assert wrapped_self(s.store) is s # inner store climbs back to the wrapper + + +def test_wrapped_self_survives_pickle_round_trip(): + """__setstate__ re-registers the backref (unpickling bypasses __init__).""" + s = _SquareStore() + s["2"] = 2 + assert s.via_wrapped_self("2") == 2.0 + + s2 = pickle.loads(pickle.dumps(s)) + assert s2["2"] == 2.0 + # Without the __setstate__ re-registration this would silently revert to 4 (Issue #18): + assert s2.via_wrapped_self("2") == 2.0 + + +def test_wrapped_self_registration_is_zero_behavior_change(): + """A store that never calls wrapped_self behaves exactly as before.""" + s = wrap_kvs(dict(), key_of_id=str.upper, id_of_key=str.lower) + s["foo"] = 1 + s["bar"] = 2 + # key_of_id=str.upper => iterating yields upper-cased keys; id_of_key=str.lower on write + assert dict(s.items()) == {"FOO": 1, "BAR": 2} + assert list(s) == ["FOO", "BAR"] + assert "foo" in s and "nope" not in s # __contains__ lowercases the key on lookup + assert len(s) == 2 From 8388cd5de0b1e2df4e82147b91dc82f23b619569 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Sun, 5 Jul 2026 09:25:16 +0200 Subject: [PATCH 2/2] Harden wrapped_self per adversarial review (multi-valued registry + identity guard) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review found a real defect: copy.copy of a wrapped store shares the inner; the copy re-registered id(inner) and, when GC'd, its finalizer popped the entry — silently reverting the still-alive original to raw #18 behavior. Root cause: single-slot dict[id(inner) -> weakref] can't represent one-inner:N-wrappers. Fixes (all contained to base.py): - Registry is now dict[id -> list[weakref]]; register appends (idempotent per live wrapper), the finalizer removes only its OWN ref and drops the id when empty. GC of a transient copy can no longer evict a live sibling. - wrapped_self climb adds an identity guard: follow a back-ref only if the wrapper still holds `cur` as its .store — rejects stale entries from post-init .store reassignment (and id-reuse false positives), and disambiguates shared inners. - Documented the residual one-inner:N-live-wrappers-with-different-transforms ambiguity (returns a valid wrapper, never raw; doesn't arise for class-wraps or copy.copy). Renamed an incidental `wrapped_self` local in DelegatedAttribute. 4 new regression tests (copy.copy+GC, deepcopy, no-leak over 1000 wrappers, shared-inner). Full dol suite green (122 passed). Claude-Session: https://claude.ai/code/session_01H8PmV6xmMhzV64zi1Twraw --- dol/base.py | 83 ++++++++++++++++++++++++++++++------------ dol/tests/base_test.py | 57 +++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 23 deletions(-) diff --git a/dol/base.py b/dol/base.py index 95f5a155..b6b030d3 100644 --- a/dol/base.py +++ b/dol/base.py @@ -263,17 +263,17 @@ def __get__(self, instance, owner): # self.__wrapped__ would make it hard to debug and # self would fail with unbound methods (why?) # So doing a check here, but would like to find a better solution. - wrapped_self = getattr(self, "__wrapped__", None) - if is_classmethod(wrapped_self) or is_unbound_method(wrapped_self): - return wrapped_self + wrapped_callable = getattr(self, "__wrapped__", None) + if is_classmethod(wrapped_callable) or is_unbound_method(wrapped_callable): + return wrapped_callable else: return self - # wrapped_self = getattr(self, '__wrapped__', None) - # if not is_classmethod(wrapped_self): + # wrapped_callable = getattr(self, '__wrapped__', None) + # if not is_classmethod(wrapped_callable): # return self # else: - # return wrapped_self + # return wrapped_callable else: # i.e. return instance.delegate.attr return getattr(getattr(instance, self.delegate_name), self.attr_name) @@ -299,34 +299,52 @@ def __delete__(self, instance): # ``wrapped_self(self)[k]``. # # The mapping is kept in a module-global registry of weak references, keyed by ``id`` of -# the inner store, and populated once per ``Store`` construction (in ``Store.__init__`` and -# re-populated in ``Store.__setstate__``, since unpickling bypasses ``__init__``). We key by -# ``id`` -- rather than setting an attribute on the inner store -- because inner stores are -# often plain ``dict``/C backends that reject instance attributes and whose pickling a stray -# weakref would break. -_wrapper_backrefs: "dict[int, weakref.ReferenceType]" = {} +# the inner store, populated in ``Store.__init__`` and re-populated in ``Store.__setstate__`` +# (unpickling bypasses ``__init__``). We key by ``id`` -- rather than setting an attribute on +# the inner store -- because inner stores are often plain ``dict``/C backends that reject +# instance attributes and whose pickling a stray weakref would break. +# +# Each id maps to a *list* of weakrefs, not a single one, so that objects legitimately +# sharing an inner store (most importantly a shallow ``copy.copy`` of a wrapped store, whose +# copy shares ``self.store`` with the original) coexist: a finalizer removes only its own +# weakref, never a live sibling's. ``wrapped_self`` then picks a live wrapper that still +# actually holds the queried object as its ``.store`` (an identity guard). +_wrapper_backrefs: "dict[int, list[weakref.ReferenceType]]" = {} def _register_wrapper_backref(inner: Any, wrapper: "Store") -> None: """Record that ``wrapper`` holds ``inner`` as its delegated store. - Registers ``id(inner) -> weakref(wrapper)``. When ``wrapper`` is garbage-collected the - weakref callback removes the entry, but only if it still points at *this* wrapper - (guarded compare-and-pop) -- so a later registration for a reused ``id`` is never - clobbered by an earlier wrapper's finalizer. + Appends ``weakref(wrapper)`` to the list registered under ``id(inner)`` (idempotent for + an already-registered live wrapper). When ``wrapper`` is garbage-collected the weakref + callback removes *only its own* entry from that list, and drops the ``id(inner)`` key once + the list is empty -- so garbage-collecting one wrapper (e.g. a transient ``copy.copy``) + never evicts a still-live sibling that shares the same inner. """ inner_id = id(inner) + existing = _wrapper_backrefs.get(inner_id) + if existing is not None and any(ref() is wrapper for ref in existing): + return # this live wrapper is already registered for this inner + def _cleanup(dead_ref: "weakref.ReferenceType") -> None: - if _wrapper_backrefs.get(inner_id) is dead_ref: + refs = _wrapper_backrefs.get(inner_id) + if refs is None: + return + try: + refs.remove(dead_ref) + except ValueError: + pass + if not refs: _wrapper_backrefs.pop(inner_id, None) try: - _wrapper_backrefs[inner_id] = weakref.ref(wrapper, _cleanup) + ref = weakref.ref(wrapper, _cleanup) except TypeError: # wrapper not weak-referenceable (not expected for a Store) -- skip silently: # wrapped_self simply falls back to returning the inner store unchanged. - pass + return + _wrapper_backrefs.setdefault(inner_id, []).append(ref) def wrapped_self(obj: Any) -> Any: @@ -343,6 +361,13 @@ def wrapped_self(obj: Any) -> Any: unchanged -- a safe no-op. If the store was wrapped in several layers (e.g. via ``Pipe``), this climbs to the OUTERMOST wrapper. + Limitation: if the *same inner store instance* is wrapped by several live wrappers with + different transforms (e.g. calling ``wrap_kvs(shared_instance, ...)`` twice), a method + bound to that shared inner cannot know which wrapper it was reached through, so one of + the wrappers is returned (ambiguous but never raw/None). This does not arise for + class-wraps (each construction builds its own inner) nor for ``copy.copy`` (whose copies + are transform-equivalent, so either answer is correct). + >>> import math >>> from dol import wrap_kvs, wrapped_self >>> sq = wrap_kvs(data_of_obj=lambda x: x * x, obj_of_data=lambda x: math.sqrt(x)) @@ -361,12 +386,24 @@ def wrapped_self(obj: Any) -> Any: >>> s.via_wrapped_self('2') # wrapped_self recovers the transformed value 2.0 """ - seen = set() + seen = {id(obj)} cur = obj while True: - ref = _wrapper_backrefs.get(id(cur)) - nxt = ref() if ref is not None else None - if nxt is None or id(nxt) in seen: + nxt = None + for ref in _wrapper_backrefs.get(id(cur), ()): + candidate = ref() + # Identity guard: only follow a back-reference whose wrapper STILL holds ``cur`` + # as its ``.store``. This rejects stale entries left by a post-construction + # ``.store`` reassignment (and the id-reuse false positives they enable), and + # picks a genuinely-wrapping candidate when several are registered for one id. + if ( + candidate is not None + and id(candidate) not in seen + and getattr(candidate, "store", None) is cur + ): + nxt = candidate + break + if nxt is None: break seen.add(id(nxt)) cur = nxt diff --git a/dol/tests/base_test.py b/dol/tests/base_test.py index b2082dde..966aa856 100644 --- a/dol/tests/base_test.py +++ b/dol/tests/base_test.py @@ -315,3 +315,60 @@ def test_wrapped_self_registration_is_zero_behavior_change(): assert list(s) == ["FOO", "BAR"] assert "foo" in s and "nope" not in s # __contains__ lowercases the key on lookup assert len(s) == 2 + + +def test_wrapped_self_copy_copy_preserves_original(): + """A shallow copy shares the inner store; GC of the copy must NOT break the original. + + Regression for the single-slot id-registry clobber found in adversarial review: the + copy re-registered id(inner) then its finalizer popped the entry, silently reverting the + still-alive original to raw Issue #18 behavior. + """ + import copy + import gc + + s = _SquareStore() + s["2"] = 2 + c = copy.copy(s) + assert c.store is s.store # shallow copy shares the inner store + assert c.via_wrapped_self("2") == 2.0 + assert s.via_wrapped_self("2") == 2.0 + del c + gc.collect() + assert s.via_wrapped_self("2") == 2.0 # would have been 4 before the multi-valued fix + + +def test_wrapped_self_deepcopy_is_independent(): + import copy + + s = _SquareStore() + s["2"] = 2 + d = copy.deepcopy(s) + assert d.store is not s.store # deepcopy builds a fresh inner + assert d.via_wrapped_self("2") == 2.0 + assert s.via_wrapped_self("2") == 2.0 + + +def test_wrapped_self_registry_does_not_leak(): + import gc + from dol.base import _wrapper_backrefs + + gc.collect() + before = len(_wrapper_backrefs) + for _ in range(1000): + t = _SquareStore() + t["1"] = 1 + assert t.via_wrapped_self("1") == 1.0 + gc.collect() + after = len(_wrapper_backrefs) + assert after - before <= 5 # short-lived wrappers' entries are cleaned up + + +def test_wrapped_self_shared_inner_returns_a_valid_wrapper(): + """Two live wrappers over one shared inner: ambiguous, but never returns the raw inner.""" + base = {} + a = wrap_kvs(base, obj_of_data=lambda x: x * 2, data_of_obj=lambda x: x // 2) + b = wrap_kvs(base, obj_of_data=lambda x: x * 3, data_of_obj=lambda x: x // 3) + resolved = wrapped_self(base) + assert resolved is a or resolved is b # a real wrapper... + assert resolved is not base # ...never the raw inner