diff --git a/.claude/skills/dol-dev-wrap-kvs/SKILL.md b/.claude/skills/dol-dev-wrap-kvs/SKILL.md new file mode 100644 index 00000000..3db1ab1e --- /dev/null +++ b/.claude/skills/dol-dev-wrap-kvs/SKILL.md @@ -0,0 +1,145 @@ +--- +name: dol-dev-wrap-kvs +description: "Understand and safely modify dol's core wrapping machinery — wrap_kvs, store_decorator, Store.wrap, and how transforms are applied. Use when touching dol/trans.py or dol/base.py; when changing how key/value transforms (key_of_id, id_of_key, obj_of_data, data_of_obj, postget, preset) are called; when a transform passed to wrap_kvs behaves unexpectedly (called with/without the store as first arg); when working Issues #9/#12/#18/#6/#5; or when a wrap_kvs change could ripple through the ~32 ecosystem packages that use it. Covers the signature-conditioning rule (name AND arity), the FirstArgIsMapping marker, the delegation (has-a) architecture and its 'self is unwrapped' trap, the subclass-signature-freeze trap, and the mandatory dependents test-gate. For end-user store-building, see the consumer skills; for Windows path issues, see dol-dev-portability." +--- + +# dol wrapping machinery (wrap_kvs / store_decorator / Store.wrap) + +`wrap_kvs` is the heart of dol and its highest-blast-radius surface: **32 of dol's 76 +local dependents import it** (`Files` 24, `KvReader` 22, `Store` 12 follow). Any change +here can ripple across the ecosystem, so changes are **breaking-change-class** and gated +by a dependents test run (below). This file maps the machinery and its traps. For the +structural big picture see `misc/docs/dol_architecture_map.md` §5; for the code-verified +issue analysis see `misc/docs/dol_issues_report.md`. + +## The one thing to internalize first: dol wraps by **delegation (has-a)**, not inheritance + +`wrap_kvs(SomeClass)` does **not** return a subclass of `SomeClass`. It returns a `Store` +subclass (`Wrap`) that holds an *instance* of `SomeClass` in `self.store` and forwards to +it. `Store.wrap = classmethod(partial(delegator_wrap, delegation_attr="store"))` +(`base.py`); the actual subclass is built by `delegate_to` (`base.py`), which: +- makes `class Wrap(Store)` whose `__init__(*args, **kwargs)` builds `delegate = wrapped(*args, **kwargs)` and stores it, and +- copies each *public* attribute/method of the wrapped class onto `Wrap` as a + `DelegatedAttribute` that forwards to `self.store.`. + +The transform hooks (`_key_of_id`, `_id_of_key`, `_obj_of_data`, `_data_of_obj`) are the +`Store` methods that `Store.__getitem__`/`__iter__`/`__setitem__` call; `wrap_kvs` +overrides them (via `_wrap_outcoming`/`_wrap_ingoing`). Two consequences are the traps +below (#18, #6). + +## The signature-conditioning rule (Issue #9/#12) — how a transform is called + +A transform can be written two ways and dol must decide which: +```python +obj_of_data=lambda data: ... # f(data) — no store +obj_of_data=lambda self, data: ... # f(self, data) — wants the store +``` +The decision lives in `_has_unbound_self(func)` (`dol/trans.py`) and is used at **four +call sites**: `_wrap_outcoming`, `_wrap_ingoing`, and the `postget`/`preset` blocks in +`_wrap_kvs`. The rule (post-#9 fix) is: + +> **wants_self ⟺ first parameter name ∈ `self_names = {"self","store","mapping"}` +> AND the function has ≥ 2 *required* positional params** (`_num_required_positional_params`). + +The arity clause is the #9 fix. Without it, unary callables whose first param merely +*happens* to be named `self` — e.g. `bytes.decode`, `str.upper` (method descriptors) — +were mis-called as `f(store, data)` → `TypeError: descriptor 'decode' ... doesn't apply to +a 'Store' object`. **Never reintroduce a name-only check.** When editing, verify with: +```python +_has_unbound_self(bytes.decode) # -> False (1 required positional) +_has_unbound_self(lambda self, d: d) # -> True (2 required) +``` + +### The explicit escape hatch: `FirstArgIsMapping` +When the heuristic can't (or shouldn't) infer intent, callers opt in explicitly: +```python +from dol import wrap_kvs, FirstArgIsMapping +wrap_kvs(store, obj_of_data=FirstArgIsMapping(lambda self, data: self.root / data)) +``` +`FirstArgIsMapping(LiteralVal)` (`trans.py`) marks a transform as wanting the store, +*regardless* of names/arity. All four call sites resolve it through the single helper +**`_resolve_self_convention(trans_func) -> (func, wants_self)`** — use that helper for any +NEW call site; do not re-implement the check. It (a) unwraps the marker and forces +`wants_self=True`, else (b) falls back to `_has_unbound_self`. Note `postget`/`preset` +run their `num_of_args(...) < 2` validation on the **already-unwrapped** function. + +This is the intended long-term direction (explicit marker) — the name heuristic is kept +for backward-compat. See `misc/docs/dol_issues_report.md` §Wave-1. + +## Trap 1 — `self` inside a wrapped class's methods is the INNER (unwrapped) store (Issue #18) + +Because methods are delegated (has-a), a method defined on the wrapped class runs bound to +`self.store`, not the `Wrap` instance: +```python +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 dict -> NO transform +s = S(); s['2'] = 2 +s['2'] # 2.0 (transformed, via Store.__getitem__) +s.via_self('2') # 4 (untransformed! self is the inner store) +``` +This is **not** the same bug as #9 and the #9 fix does not touch it. Workaround: re-wrap +`self` inside the method (`sq(self)[k]`). A general fix requires rethinking method +delegation — out of scope for conditioning changes; do not conflate. + +## Trap 2 — `Store.wrap` freezes a subclass's `__init__` signature (Issue #6) + +`delegator_wrap` sets `wrap.__signature__ = Sig(obj)` (`base.py`) so the wrapper advertises +the wrapped class's signature (its own `__init__` is `*args,**kwargs`). But `__signature__` +is a plain class attribute, so **subclasses inherit the frozen signature** and lose their +own params: +```python +@Store.wrap +class A: + def __init__(self, a=1): ... +class B(A): + def __init__(self, a=1, b=2): ... +signature(B) # (a=1) — B's b=2 is lost; B.__dict__ has no __signature__ +``` +A fix belongs in the delegation/signature machinery (recompute per-subclass, e.g. via +`__init_subclass__`), independent of the conditioning work. + +## `store_decorator`: why wrappers work 4 ways + +`wrap_kvs`, `filt_iter`, `cached_keys`, etc. are decorated with `@store_decorator`, which +makes each usable as: class decorator (`@wrap_kvs(...) class S(dict)`), instance wrapper +(`wrap_kvs(a_dict, ...)`), bare-class factory (`wrap_kvs(dict, ...)` → a class), and +parameter-only factory (`wrap_kvs(**opts)` → a decorator). Passing a **type** returns a +class; passing an **instance** returns a wrapped instance. Keep this contract when adding +wrappers, and test all four forms (see `tests/test_trans.py::test_redirect_getattr_to_getitem`). + +## Where things live / how to test + +- Machinery: `dol/trans.py` (`_has_unbound_self`, `_num_required_positional_params`, + `_resolve_self_convention`, `FirstArgIsMapping`, `_wrap_outcoming`, `_wrap_ingoing`, + `_wrap_kvs`, `store_decorator`) and `dol/base.py` (`Store`, `delegator_wrap`, + `delegate_to`, `DelegatedAttribute`). +- Tests: `dol/tests/test_trans.py`. Doctests in `trans.py` are primary docs — keep runnable. + (`dol.trans.double_up_as_factory` has a KNOWN pre-existing doctest failure, unrelated.) +- Run: `python -m pytest dol/tests/ -q` and + `python -m pytest --doctest-modules dol/trans.py -q`. + +## MANDATORY: the dependents test-gate (any wrap_kvs/base change) + +dol is editably installed across the local ecosystem, so **dependents already import your +working-tree dol** — no reinstall needed. Before landing a wrap_kvs/Store.wrap change: +1. Measure blast radius statically: AST-scan dependents for transforms passed to + wrap_kvs-family whose first param ∈ self_names (see `misc/data/` scan scripts; + the reusable pattern found 0 breaking + 12 safe self-convention sites). +2. Run the dependents test-gate **baseline vs modified**: run each key dependent's suite + with your change, then `git stash` your dol edits, run again (baseline), `git stash pop`; + a `PASS→FAIL` delta is a real regression (env `FAIL→FAIL` is not). Helpers and the + gate order live in `misc/data/` (gitignored). Prioritize `config2py → graze → ju → + tabled → oa` (highest transitive fan-out) plus the self-convention users + (`py2store, sqldol, tec, unbox, mongodol`). + +## Checklist for a PR touching wrap_kvs / Store.wrap + +- [ ] All self-vs-data decisions go through `_resolve_self_convention` (no ad-hoc name checks). +- [ ] `_has_unbound_self` keeps BOTH conditions (name AND ≥2 required); `bytes.decode` stays unary. +- [ ] `FirstArgIsMapping` honored in every affected slot (key/value, in/out, postget, preset). +- [ ] All 4 `store_decorator` usage forms still work. +- [ ] Pickling of a wrapped store still round-trips. +- [ ] Blast radius measured + dependents test-gate run (baseline vs modified) → no PASS→FAIL. +- [ ] Didn't conflate #18 (delegation/self) or #6 (subclass signature) with the conditioning fix. diff --git a/dol/__init__.py b/dol/__init__.py index 0cf4b37d..92967055 100644 --- a/dol/__init__.py +++ b/dol/__init__.py @@ -121,6 +121,7 @@ def ihead(store, n=1): from dol.trans import ( wrap_kvs, # transform store key and/or value + FirstArgIsMapping, # mark a transform whose first arg is the store (self), not data filt_iter, # filter store keys (and contains ready to use filters as attributes) cached_keys, # cache store keys add_decoder, # add a decoder (i.e. outcomming value transformer) to a store diff --git a/dol/tests/test_trans.py b/dol/tests/test_trans.py index 6494f1ca..cbd19028 100644 --- a/dol/tests/test_trans.py +++ b/dol/tests/test_trans.py @@ -7,9 +7,109 @@ filter_regex, filter_suffixes, redirect_getattr_to_getitem, + wrap_kvs, + FirstArgIsMapping, + _has_unbound_self, + _resolve_self_convention, ) +# --------------------------------------------------------------------------- +# Signature-based conditioning of wrap_kvs transforms (Issues #9, #12, #18) +# --------------------------------------------------------------------------- + + +def test_wrap_kvs_unary_builtin_transform_issue_9(): + """A unary callable whose first param happens to be named ``self`` must be + applied as ``f(data)``, not ``f(self, data)`` (Issue #9). + + ``bytes.decode`` is the canonical case: it is effectively unary (one required + positional), but its first parameter is named ``self``. The old name-only + heuristic mis-called it as ``bytes.decode(store, data)`` -> TypeError. + """ + S = wrap_kvs(dict, obj_of_data=bytes.decode) + s = S({"k": b"hello"}) + assert s["k"] == "hello" + + # str.upper, str.split — same shape, must also be treated as unary + assert wrap_kvs({"k": "hi"}, obj_of_data=str.upper)["k"] == "HI" + + # The lambda form, which always worked, must keep working + assert wrap_kvs({"k": b"hi"}, obj_of_data=lambda x: x.decode())["k"] == "hi" + + +def test_wrap_kvs_self_convention_still_works(): + """Transforms genuinely using the ``(self, data)`` convention (>=2 required + positional params, first named self/store/mapping) must keep receiving the + store instance. Must not regress the ~12 real ecosystem usages.""" + + def obj_of_data(self, data): + return f"{getattr(self, 'p', '?')}:{data}" + + S = wrap_kvs(dict, obj_of_data=obj_of_data) + s = S({"a": "x"}) + s.p = "ns" + assert s["a"] == "ns:x" + + # first param named 'store' works too + def key_of_id(store, _id): + return _id.upper() + + s2 = wrap_kvs({"a": 1, "b": 2}, key_of_id=key_of_id) + assert sorted(s2) == ["A", "B"] + + +def test_first_arg_is_mapping_explicit_marker_issue_12(): + """FirstArgIsMapping forces the ``(self, data)`` convention regardless of the + transform's parameter names, and unwraps to the underlying callable.""" + + def needs_store(x, data): # first param 'x' -> heuristic alone says no-self + return f"{getattr(x, 'p', '?')}/{data}" + + S = wrap_kvs(dict, obj_of_data=FirstArgIsMapping(needs_store)) + s = S({"a": "v"}) + s.p = "NS" + assert s["a"] == "NS/v" + + # Marker resolves to (underlying_func, wants_self=True) + func, wants_self = _resolve_self_convention(FirstArgIsMapping(needs_store)) + assert func is needs_store and wants_self is True + + +def test_postget_preset_self_conventions(): + """postget/preset honor both the no-self and self conventions, and the + FirstArgIsMapping marker.""" + + def postget_self(self, k, v): + return f"{k}={v}" + + assert wrap_kvs({"a": 1}, postget=postget_self)["a"] == "a=1" + + def postget_plain(k, v): + return v * 2 + + assert wrap_kvs({"a": 5}, postget=postget_plain)["a"] == 10 + + def preset_self(self, k, v): + return v + 1 + + S = wrap_kvs(dict, preset=preset_self) + s = S() + s["a"] = 10 + assert dict(s) == {"a": 11} + + +def test_has_unbound_self_heuristic_units(): + """Unit-level checks of the (name AND >=2 required) heuristic.""" + assert _has_unbound_self(lambda self, data: data) is True + assert _has_unbound_self(lambda store, data: data) is True + assert _has_unbound_self(lambda data: data) is False + assert _has_unbound_self(bytes.decode) is False # first param 'self', 1 required + assert _has_unbound_self(str.upper) is False + # first param self-ish but 2nd is optional -> only 1 required -> no-self + assert _has_unbound_self(lambda self, data=None: data) is False + + def test_filter_regex_is_os_independent(): """Regex filters must compile as REGEXES, not as path templates. diff --git a/dol/trans.py b/dol/trans.py index beb883f8..0759b129 100644 --- a/dol/trans.py +++ b/dol/trans.py @@ -419,15 +419,41 @@ def _first_param_is_an_instance_param(params): return len(params) > 0 and list(params)[0] in self_names -# TODO: Add validation of func: That all but perhaps 1 argument (not counting self) -# has a default -def _has_unbound_self(func): +def _num_required_positional_params(params): + """Number of positional parameters with no default value. + + >>> from inspect import signature + >>> _num_required_positional_params(signature(lambda a, b, c=1: 0).parameters) + 2 """ + return sum( + 1 + for p in params.values() + if p.default is Parameter.empty + and p.kind in (Parameter.POSITIONAL_ONLY, Parameter.POSITIONAL_OR_KEYWORD) + ) + + +def _has_unbound_self(func): + """Guess whether ``func`` expects the store instance as its first argument. + + A transform passed to ``wrap_kvs`` (and friends) may take either the form + ``f(data)`` or ``f(self, data)``. We decide by two conditions that must *both* + hold: the first parameter is named like an instance param + (``self``/``store``/``mapping`` -- see ``self_names``) **and** the function has at + least two required (no-default) positional parameters. + + The second condition avoids misclassifying *unary* callables whose first + parameter merely happens to be named ``self`` -- e.g. builtin method descriptors + such as ``bytes.decode`` -- which was the root cause of Issue #9. When the + heuristic can't infer intent, wrap the function in :class:`FirstArgIsMapping` to + opt in explicitly (see :func:`_resolve_self_convention`). Args: - func: + func: a candidate transform callable Returns: + bool: True iff ``func`` should be called as ``func(self, ...)``. >>> def f1(x): ... >>> assert _has_unbound_self(f1) == 0 @@ -452,6 +478,13 @@ def _has_unbound_self(func): >>> _has_unbound_self(A.foo) 0 >>> + + Issue #9 regression: a unary callable whose first parameter is named ``self`` + (only one required positional) is NOT treated as wanting the store: + + >>> assert _has_unbound_self(bytes.decode) is False + >>> assert _has_unbound_self(str.upper) is False + >>> assert _has_unbound_self(lambda self, data=None: data) is False """ try: params = signature(func).parameters @@ -467,12 +500,54 @@ def _has_unbound_self(func): not isinstance(func, type) and not _is_bound(func) and _first_param_is_an_instance_param(params) + and _num_required_positional_params(params) >= 2 ): return True else: return False +def _resolve_self_convention(trans_func): + """Resolve how a transform wants to be called: ``(func, wants_self)``. + + An explicit :class:`FirstArgIsMapping` marker always wins (unwrapped, wants_self + True); otherwise fall back to the :func:`_has_unbound_self` heuristic. This is the + single source of truth used by every wrap_kvs call site (Issues #9, #12). + + >>> _resolve_self_convention(bytes.decode)[1] + False + >>> f = lambda self, data: data + >>> _resolve_self_convention(f)[1] + True + >>> g = FirstArgIsMapping(bytes.decode) # opt in explicitly + >>> func, wants_self = _resolve_self_convention(g) + >>> wants_self, func is bytes.decode + (True, True) + """ + if isinstance(trans_func, FirstArgIsMapping): + return trans_func.get_val(), True + return trans_func, _has_unbound_self(trans_func) + + +class FirstArgIsMapping(LiteralVal): + """Mark a transform so its first argument is the store (mapping), not the data. + + Use this to explicitly opt a transform function into the ``f(self, data)`` + calling convention in wrappers such as ``wrap_kvs`` -- instead of relying on the + name/arity heuristic (:func:`_has_unbound_self`). This is the escape hatch for + functions the heuristic can't (or shouldn't) infer, and the recommended, explicit + alternative to naming a transform's first parameter ``self``/``store``/``mapping``. + + >>> from dol import wrap_kvs, FirstArgIsMapping + >>> def prefix_with_name(self, data): + ... return f"{getattr(self, 'name', '?')}:{data}" + >>> S = wrap_kvs(dict, obj_of_data=FirstArgIsMapping(prefix_with_name)) + >>> s = S({'a': 'x'}); s.name = 'ns' + >>> s['a'] + 'ns:x' + """ + + def transparent_key_method(self, k): return k @@ -1760,9 +1835,10 @@ def _wrap_outcoming( [7, 14] """ if trans_func is not None: + trans_func, wants_self = _resolve_self_convention(trans_func) wrapped_func = getattr(store_cls, wrapped_method) - if not _has_unbound_self(trans_func): + if not wants_self: # print(f"00000: {store_cls}: {wrapped_method}, {trans_func}, {wrapped_func}, {wrap_arg_idx}") @wraps(wrapped_func) def new_method(self, x): @@ -1791,9 +1867,10 @@ def new_method(self, x): def _wrap_ingoing(store_cls, wrapped_method: str, trans_func: Callable | None = None): if trans_func is not None: + trans_func, wants_self = _resolve_self_convention(trans_func) wrapped_func = getattr(store_cls, wrapped_method) - if not _has_unbound_self(trans_func): + if not wants_self: @wraps(wrapped_func) def new_method(self, x): @@ -2110,17 +2187,6 @@ def add_decoder(store_cls=None, *, decoder: Callable = None, name=None): return wrap_kvs(store_cls, obj_of_data=decoder, name=name) -class FirstArgIsMapping(LiteralVal): - """A Literal class to mark a function as being one where the first argument is - a mapping (store). This is intended to be used in wrappers such as ``wrap_kvs`` - to indicate when the first argument of a transformer function ``trans`` like - ``key_of_id``, ``preset``, etc. is the store itself, therefore should be applied as - ``trans(store, ...)`` instead of ``trans(...)``. - """ - - # TODO: Use this for it's intent! - - def _wrap_kvs( store_cls: type, *, @@ -2154,12 +2220,13 @@ def _wrap_kvs( # Should only count args with no defaults or partial won't be able to be used to make postget/preset funcs # TODO: Extract postget and preset patterns? if postget is not None: + postget, postget_wants_self = _resolve_self_convention(postget) if num_of_args(postget) < 2: raise ValueError( "A postget function needs to have (key, value) or (self, key, value) arguments" ) - if not _has_unbound_self(postget): + if not postget_wants_self: def __getitem__(self, k): return postget(k, super(store_cls, self).__getitem__(k)) @@ -2172,12 +2239,13 @@ def __getitem__(self, k): store_cls.__getitem__ = __getitem__ if preset is not None: + preset, preset_wants_self = _resolve_self_convention(preset) if num_of_args(preset) < 2: raise ValueError( "A preset function needs to have (key, value) or (self, key, value) arguments" ) - if not _has_unbound_self(preset): + if not preset_wants_self: def __setitem__(self, k, v): return super(store_cls, self).__setitem__(k, preset(k, v))