From 7e752553bfc6555c001e61213539a2bdb2fdd3ef Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Mon, 3 Aug 2026 20:11:49 +0100 Subject: [PATCH 1/2] Fix double_up_as_factory when the wrapped object is passed by keyword `double_up_as_factory` decided between "wrap this" and "make a factory" by looking only at the first *positional* argument. So when the object to wrap was passed by keyword -- `wrap(func=foo)`, which is what happens whenever a caller forwards `**kwargs` -- it landed in `**kwargs`, `wrapped` stayed None, and the decorator silently returned a `functools.partial` instead of the wrapped object. No exception: the caller only found out much later, at call time, when the "wrapped" object behaved like the decorator. This affected every decorator built on the idiom, including `wrap`, `ch_names`, `include_exclude`, `rm_params` and `add_smart_defaults`. Fix: partialize the decorator's first-parameter name into `_double_up_as_factory` alongside `__decorator_func`, and, when nothing was given positionally, take the object to wrap from `kwargs` under that name. The lookup is skipped when a positional argument was given, so passing the object both ways still raises python's own "got multiple values for argument" TypeError rather than silently preferring one. The inner validator now returns that parameter name instead of True, so the signature is introspected once. Only the decorator's own first-parameter name is understood as "the object to wrap" -- `func=` is not special-cased -- so decorators that name it something else keep working and keep treating `func=` as an ordinary decorator argument. Tests: doctests on `double_up_as_factory` covering both call directions and the factory guard, plus parametrized tests in test_wrapper.py over the five affected decorators asserting `f(func=foo)` wraps rather than returning a partial, that the positional and keyword directions agree, and that the factory direction still returns a partial. Verified red before green: the new tests fail 12/18 against the previous implementation and pass 18/18 with the fix. i2 suite 690 -> 708 passed, 2 xfailed, no regressions. The 46 local dependents' suites were run before and after and are identical (32 pass / 14 fail, all 14 pre-existing and unrelated). Closes #64 Claude-Session: https://claude.ai/code/session_01Kug7UUbVeCQgruvNXUq63c --- i2/deco.py | 59 ++++++++++++++++---- i2/tests/test_wrapper.py | 113 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 161 insertions(+), 11 deletions(-) diff --git a/i2/deco.py b/i2/deco.py index ec85265b..8b89702d 100644 --- a/i2/deco.py +++ b/i2/deco.py @@ -210,17 +210,35 @@ def from_jdict(cls, jdict): return FuncFactory(**jdict) -def _double_up_as_factory(wrapped=None, *args, __decorator_func=None, **kwargs): - """Util for double_up_as_factory, ``__decorator_func`` to be partialized""" +def _double_up_as_factory( + wrapped=None, + *args, + __decorator_func=None, + __wrapped_param_name=None, + **kwargs, +): + """Util for double_up_as_factory; the ``__``-prefixed params are partialized in. + + ``__decorator_func`` is the decorator being doubled up, and + ``__wrapped_param_name`` is the name that decorator gave its first parameter -- + needed so that the object to wrap can be given by keyword as well as positionally. + """ if args: raise RuntimeError( f"You need to specify decorator arguments as keyword-only." f"You specified positional arguments: {args=}" ) + if wrapped is None: + # The object to wrap may have been given by keyword (``decorator(func=foo)``), + # in which case it landed in kwargs, under the decorator's first param name. + # Note we only look for it when nothing was given positionally: if it was given + # both ways, leaving kwargs alone lets python raise its own (clearer) + # "got multiple values for argument" TypeError. + wrapped = kwargs.pop(__wrapped_param_name, None) if wrapped is None: # then we want a factory return partial(__decorator_func, **kwargs) else: - return __decorator_func(wrapped, *args, **kwargs) + return __decorator_func(wrapped, **kwargs) def double_up_as_factory(decorator_func): @@ -246,7 +264,27 @@ def double_up_as_factory(decorator_func): >>> wrapped_foo = decorator(foo, multiplier=10) >>> wrapped_foo(2) 30 - >>> + + The object to wrap doesn't have to be given positionally: it can also be given by + keyword, under the name the decorator gave its first parameter (here, ``func``). + This matters because forwarding arguments through ``**kwargs`` is a very common way + to call a decorator, so ``decorator(func=foo)`` must mean what ``decorator(foo)`` + means: + + >>> decorator(func=foo, multiplier=10)(2) + 30 + >>> decorator(func=foo)(2) + 6 + + It is the *absence* of an object to wrap -- not the way it's passed -- that asks for + a factory: + + >>> from functools import partial + >>> isinstance(decorator(multiplier=3), partial) + True + >>> isinstance(decorator(func=foo), partial) + False + >>> multiply_by_3 = decorator(multiplier=3) >>> wrapped_foo = multiply_by_3(foo) >>> wrapped_foo(2) @@ -277,7 +315,8 @@ def double_up_as_factory(decorator_func): """ - def validate_decorator_func(decorator_func): + def validated_wrapped_param_name(decorator_func): + """Validate decorator_func, returning the name of its first parameter.""" first_param, *other_params = signature(decorator_func).parameters.values() assert first_param.default is None, ( f"First argument of the decorator function needs to default to None. " @@ -286,12 +325,14 @@ def validate_decorator_func(decorator_func): assert all( p.kind in {p.KEYWORD_ONLY, p.VAR_KEYWORD} for p in other_params ), f"All arguments (besides the first) need to be keyword-only" - return True - - validate_decorator_func(decorator_func) + return first_param.name return wraps(decorator_func)( - partial(_double_up_as_factory, __decorator_func=decorator_func) + partial( + _double_up_as_factory, + __decorator_func=decorator_func, + __wrapped_param_name=validated_wrapped_param_name(decorator_func), + ) ) diff --git a/i2/tests/test_wrapper.py b/i2/tests/test_wrapper.py index 07eba39c..9a5c9393 100644 --- a/i2/tests/test_wrapper.py +++ b/i2/tests/test_wrapper.py @@ -1,9 +1,20 @@ """Testing wrapper""" from collections.abc import Iterable -from i2.wrapper import wrap, mk_ingress_from_name_mapper, rm_params +from functools import partial + +import pytest + +from i2.wrapper import ( + wrap, + mk_ingress_from_name_mapper, + rm_params, + ch_names, + include_exclude, + add_smart_defaults, +) from i2.deco import FuncFactory -from i2.signatures import Sig +from i2.signatures import Sig, name_of_obj def _test_ingress(a, b: str, c="hi"): @@ -425,3 +436,101 @@ def egress(output): # No annotation assert sig.return_annotation is Parameter.empty # Test functionality assert wrapped(5) == 10 + + +# --------------------------------------------------------------------------------------- +# double_up_as_factory: the object to wrap can be given positionally OR by keyword +# See https://github.com/i2mint/i2/issues/64 + +#: The ``double_up_as_factory``-built decorators of ``i2.wrapper``. Each takes the object +#: to wrap as its first parameter (named ``func``) and every other parameter is +#: keyword-only with a default, so each must be usable in all three of these ways: +#: ``deco(func)``, ``deco(func=func)`` (keyword-forwarding) and ``deco(**params)(func)`` +#: (factory). +DOUBLED_UP_DECORATORS = (wrap, ch_names, include_exclude, rm_params, add_smart_defaults) + + +def _incr(x, y=1): + """Fixture function to be wrapped by the decorators under test.""" + return x + y + + +@pytest.mark.parametrize("decorator", DOUBLED_UP_DECORATORS, ids=name_of_obj) +def test_double_up_as_factory_accepts_wrapped_by_keyword(decorator): + """``deco(func=func)`` must wrap, not silently make a factory (i2mint/i2#64). + + The failure this guards against is silent: before the fix, passing the object to + wrap by keyword returned a ``functools.partial`` and the caller only found out much + later, at call time, when the "wrapped" object behaved like the decorator instead. + """ + wrapped = decorator(func=_incr) + assert not isinstance(wrapped, partial), ( + f"{name_of_obj(decorator)}(func=...) returned a factory instead of wrapping: " + f"{wrapped!r}" + ) + assert wrapped(2) == _incr(2) == 3 + + +@pytest.mark.parametrize("decorator", DOUBLED_UP_DECORATORS, ids=name_of_obj) +def test_double_up_as_factory_keyword_and_positional_agree(decorator): + """``deco(func)`` and ``deco(func=func)`` must produce equivalent wrappers.""" + from_positional, from_keyword = decorator(_incr), decorator(func=_incr) + assert type(from_positional) is type(from_keyword) + assert from_positional(2) == from_keyword(2) + assert Sig(from_positional) == Sig(from_keyword) + + +@pytest.mark.parametrize("decorator", DOUBLED_UP_DECORATORS, ids=name_of_obj) +def test_double_up_as_factory_still_makes_factories(decorator): + """Guard: the factory direction (no object to wrap) must keep returning a partial.""" + factory = decorator() + assert isinstance(factory, partial) + assert factory(_incr)(2) == 3 + + +def test_double_up_as_factory_with_decorator_params(): + """Guard: giving decorator params (and no wrapped object) still gives a factory.""" + from i2.deco import double_up_as_factory + + @double_up_as_factory + def multiply_result(func=None, *, multiplier=2): + return lambda x: func(x) * multiplier + + assert isinstance(multiply_result(multiplier=3), partial) + assert multiply_result(multiplier=3)(_incr)(2) == 9 + # ... and the two non-factory directions agree + assert multiply_result(_incr, multiplier=3)(2) == 9 + assert multiply_result(func=_incr, multiplier=3)(2) == 9 + + +def test_double_up_as_factory_honors_the_wrapped_params_name(): + """The keyword to use is whatever the decorator named its first param.""" + from i2.deco import double_up_as_factory + + @double_up_as_factory + def decorate(obj=None, *, suffix="!"): + return lambda: obj() + suffix + + hello = lambda: "hello" + assert decorate(obj=hello)() == "hello!" + assert decorate(hello)() == "hello!" + assert isinstance(decorate(suffix="?"), partial) + # ``func`` is NOT special: only the decorator's own first param name is understood + # as "the object to wrap", so ``func=`` stays an (here, unexpected) decorator arg, + # making a factory that complains only when it's used -- as it did before too. + unexpected_kwarg_factory = decorate(func=hello) + assert isinstance(unexpected_kwarg_factory, partial) + with pytest.raises(TypeError): + unexpected_kwarg_factory(hello) + + +def test_double_up_as_factory_rejects_duplicate_wrapped(): + """Giving the wrapped object both positionally and by keyword is an error.""" + from i2.deco import double_up_as_factory + + @double_up_as_factory + def decorate(func=None, *, multiplier=2): + return lambda x: func(x) * multiplier + + with pytest.raises(TypeError): + decorate(_incr, func=_incr) From 09c6f02a4498ab7520f4954d70558d726b195dff Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Mon, 3 Aug 2026 20:50:35 +0100 Subject: [PATCH 2/2] Document that a double_up_as_factory decorator's first param name is reserved The name of the decorator's first parameter always means "the object to wrap". For a decorator that also takes `**kwargs`, that name is therefore unusable as a decorator argument: `deco(func=)` cannot be expressed. This is a pre-existing limitation of the double-up idiom, not a regression -- but the failure mode changed with the keyword fix (it used to raise `TypeError: got multiple values for argument 'func'` when the factory was applied; now the factory call itself quietly returns nonsense), so it is worth stating explicitly. Adds doctests demonstrating both the working case and the unexpressible one. Claude-Session: https://claude.ai/code/session_01Kug7UUbVeCQgruvNXUq63c --- i2/deco.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/i2/deco.py b/i2/deco.py index 8b89702d..32ab3828 100644 --- a/i2/deco.py +++ b/i2/deco.py @@ -313,6 +313,34 @@ def double_up_as_factory(decorator_func): ... AssertionError: All arguments (besides the first) need to be keyword-only + Note also that the name of that first argument is effectively **reserved**: it always + means "the object to wrap". For a decorator that also takes ``**kwargs``, this means + a decorator argument can never share that name. Say a decorator's first parameter is + ``func`` and it renames parameters via ``**kwargs``: + + >>> @double_up_as_factory + ... def rename(func=None, **new_name_for_old_name): + ... return new_name_for_old_name # (stand-in for the real work) + + You can rename an ordinary parameter through the factory form: + + >>> rename(b='bee')(lambda a, b: None) + {'b': 'bee'} + + But you cannot use it to rename a parameter that happens to be called ``func``: + ``rename(func='callback')`` is read as "wrap the object ``'callback'``", not as + "rename ``func`` to ``callback``", so it returns nonsense rather than a factory: + + >>> rename(func='callback') + {} + + This is a pre-existing limitation of the double-up idiom -- there is no way to tell + the two intents apart -- and it is not specific to passing the object by keyword. + Before keyword-passing was supported the same call failed later and differently, + with ``TypeError: rename() got multiple values for argument 'func'``. If a decorator + needs an argument with the same name as its first parameter, don't use + ``double_up_as_factory``. + """ def validated_wrapped_param_name(decorator_func):