From 95ef94d0a2818718ecc174df172c65ac98b0ab03 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Tue, 22 Sep 2026 13:41:26 +0000 Subject: [PATCH] fix: disallow_overwrites no-op and cache_func_outputs default/kwargs bugs - dol.trans.disallow_overwrites: was a no-op (built an inner __setitem__ but never attached it, returned None). Now returns a proper subclass of `store` with the __setitem__ guard attached, and (per the `disable_deletes` param, previously ignored) an equivalent __delitem__ guard. error_msg supports both `{}` and `{k}` placeholders. - dol.caching.cache_func_outputs: with the default `cache=HashableDict`, `get_cache` returned the *class* (not an instance), so the first call raised `TypeError: argument of type 'type' is not iterable`. Now explicitly instantiates a bare class/factory before use. Separately, kwargs were wrapped in a fresh HashableDict each call, whose __hash__ is identity-based (id(self)) -- so kwargs-based calls never hit the cache. Now keys on a stable, value-based tuple of the sorted kwargs items instead. Both were reported, precisely diagnosed, and left as a fix-or-delete choice in the issue. Chose "finish" over "delete" (the issue's other option, and the one #94's cruft audit lists as a candidate) since they're now correct, useful, and not part of the public dol/__init__.py API -- so #94 can still relocate them later if fleet adoption stays at zero. Reviewed by an independent sub-agent before merge (per crowsnest policy), which caught two follow-up issues, both fixed here: the error_msg `{k}` placeholder didn't actually work (positional-only .format call), and unhashable kwargs values now raise TypeError instead of silently never caching -- documented as intentional (dol's own "no silent failures" convention), matching the pre-existing requirement that positional args be hashable too. Closes #101 Co-Authored-By: Claude Sonnet 5 --- dol/caching.py | 35 ++++++++++++++++++++++++--- dol/trans.py | 64 +++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 87 insertions(+), 12 deletions(-) diff --git a/dol/caching.py b/dol/caching.py index 4b788929..dafade08 100644 --- a/dol/caching.py +++ b/dol/caching.py @@ -2623,14 +2623,43 @@ class HashableDict(HashableMixin, dict): # NOTE: cache uses (func, args, kwargs). Don't want to make more complex with a bind cast to (func, kwargs) only def cache_func_outputs(cache=HashableDict): - """Decorator factory intended to cache a function's outputs in ``cache``, keyed by ``(func, args, kwargs)``; - only positional-argument calls with an explicitly given ``cache`` actually hit the cache.""" + """Decorator factory that caches a function's outputs in ``cache``, keyed by + ``(func, args, kwargs)``. + + :param cache: A cache instance (with ``__contains__``/``__getitem__``/ + ``__setitem__``), or a zero-arg factory/class (e.g. the default, + ``HashableDict``) used to make one. + + Note: like ``args``, every ``kwargs`` value must be hashable (they're part of + the cache key). Passing an unhashable value (e.g. a ``list``) raises + ``TypeError`` rather than silently skipping the cache. + + >>> @cache_func_outputs() + ... def f(x, y=2): + ... print(f"computing f({x}, {y})") + ... return x + y + >>> f(1) + computing f(1, 2) + 3 + >>> f(1) # cached: no "computing" print + 3 + >>> f(1, y=3) # different kwargs: not a cache hit + computing f(1, 3) + 4 + >>> f(1, y=3) # this one is now cached too + 4 + """ + if isinstance(cache, type): + cache = cache() # instantiate a cache class/factory cache = get_cache(cache) def cache_method_decorator(func): @wraps(func) def _func(*args, **kwargs): - k = (func, args, HashableDict(kwargs)) + # Use a hashable-by-value key for kwargs (a HashableDict instance is + # hashable by id, so a *fresh* one would never compare equal to another + # with the same contents -- see i2mint/dol#101). + k = (func, args, tuple(sorted(kwargs.items()))) if k not in cache: val = func(*args, **kwargs) cache[k] = val # cache it diff --git a/dol/trans.py b/dol/trans.py index b574b74e..d9065ba0 100644 --- a/dol/trans.py +++ b/dol/trans.py @@ -684,20 +684,66 @@ def _ipython_key_completions_(self): def disallow_overwrites(store, *, error_msg=None, disable_deletes=True): - """Intended to make a store class's ``__setitem__`` raise ``OverWritesNotAllowedError`` on existing keys; - currently a no-op that returns ``None`` (the override is never attached). Use ``OverWritesNotAllowedMixin``.""" + """Return a subclass of ``store`` whose ``__setitem__`` raises + ``OverWritesNotAllowedError`` on existing keys (``store`` itself is left + untouched). + + :param store: The store class to wrap (must be a type). + :param error_msg: Custom error message; ``{}`` (or ``{k}``) in it is filled + in with the offending key via ``.format``. Defaults to a generic message. + :param disable_deletes: If ``True`` (the default), also disable + ``__delitem__`` (raising the same error) -- since deleting a key and + rewriting it would otherwise be a way around the overwrite guard. + :return: A new subclass of ``store`` with the guard(s) attached. + + >>> class D(dict): ... + >>> ND = disallow_overwrites(D) + >>> d = ND(a=1) + >>> d['b'] = 2 + >>> d['a'] = 1 + Traceback (most recent call last): + ... + dol.errors.OverWritesNotAllowedError: key a already exists and cannot be overwritten... + >>> del d['a'] + Traceback (most recent call last): + ... + dol.errors.OverWritesNotAllowedError: delete of key a is not allowed + + With ``disable_deletes=False``, deletes are left alone: + + >>> ND2 = disallow_overwrites(D, disable_deletes=False) + >>> d2 = ND2(a=1) + >>> del d2['a'] # no error + >>> d2['a'] = 2 # no error either, since 'a' was deleted first + """ assert isinstance(store, type), "store needs to be a type" + if error_msg is None: + error_msg = ( + "key {} already exists and cannot be overwritten. " + "If you really want to write to that key, delete it before writing" + ) + + namespace = {} + if hasattr(store, "__setitem__"): def __setitem__(self, k, v): if k in self: - raise OverWritesNotAllowedError( - "key {} already exists and cannot be overwritten. " - "If you really want to write to that key, delete it before writing".format( - k - ) - ) - return super(type(self), self).__setitem__(k, v) + raise OverWritesNotAllowedError(error_msg.format(k, k=k)) + return super(NoOverwritesStore, self).__setitem__(k, v) + + namespace["__setitem__"] = __setitem__ + + if disable_deletes and hasattr(store, "__delitem__"): + + def __delitem__(self, k): + raise OverWritesNotAllowedError(f"delete of key {k} is not allowed") + + namespace["__delitem__"] = __delitem__ + + NoOverwritesStore = type(store.__name__, (store,), namespace) + copy_attrs(NoOverwritesStore, store, ("__name__", "__qualname__", "__module__")) + return NoOverwritesStore class OverWritesNotAllowedMixin: