Fix double_up_as_factory when the wrapped object is passed by keyword

[thorwhalen]

Closes #64

The bug

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 it landed in **kwargs, wrapped stayed None, and the decorator silently returned a functools.partial instead of the wrapped object:

>>> wrap(foo)
<i2.Wrap foo(x)>
>>> wrap(func=foo)
functools.partial(<function wrap ...>, func=<function foo ...>)   # before
<i2.Wrap foo(x)>                                                  # after

There was no exception — the caller only found out much later, at call time, when the "wrapped" object behaved like the decorator. That makes it nasty: decorator(**kwargs) forwarding is a very common way to call a decorator, and double_up_as_factory is the canonical decorator idiom here, so this was latent in anything building on it.

Confirmed to affect wrap, ch_names, include_exclude, rm_params and add_smart_defaults.

The 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.

Two deliberate details:

• The kwargs 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.
• Only the decorator's own first-parameter name is understood as "the object to wrap" — func= is not special-cased. Decorators that name it something else keep working and keep treating func= as an ordinary decorator argument.

The inner validator now returns that parameter name instead of True, so the signature is introspected once rather than twice.

Public signatures of all five decorators are byte-identical to before.

Documented limitation (second commit)

The decorator's first parameter name is effectively reserved — it always means "the object to wrap". For a decorator whose first parameter is func and which also takes **kwargs, that name can therefore never be used as a decorator argument: deco(func=<not-the-wrapped-object>) is unexpressible.

This is pre-existing and not a regression, but the failure mode changed, so double_up_as_factory's docstring now records it (with doctests):

@double_up_as_factory
def rename(func=None, **new_name_for_old_name): ...

rename(b='bee')(lambda a, b: None)   # fine
rename(func='callback')              # cannot mean "rename func to callback"

• before: TypeError: rename() got multiple values for argument 'func', raised when the factory was applied
• after: the factory call itself quietly returns nonsense

If a decorator needs an argument named the same as its first parameter, it shouldn't use double_up_as_factory.

Tests

• Doctests on double_up_as_factory covering both call directions, the factory guard, and the reserved-name limitation above.
• Parametrized tests in test_wrapper.py over the five affected decorators: f(func=foo) wraps rather than returning a partial; the positional and keyword directions agree (same type, same result, same Sig); and the factory direction still returns a partial.
• Guard tests that a decorator naming its first param something other than func behaves correctly, and that giving the object both ways raises.

Red before green — the new tests fail 12/18 against the previous implementation and pass 18/18 with the fix. The new doctests were also verified red against the old logic.

Test results

i2: 696 → 714 passed, 2 xfailed (pytest --doctest-modules with CI's option flags and ignores). No regressions.

Dependents gate — corrected

An earlier version of this description said the dependents gate was clean — that all 46 dependents' results were "identical: 32 pass / 14 fail" with no regressions. That was wrong, and this section replaces it. The claim is not being deleted, it is being corrected.

The gate compared only each dependent package's overall pass/fail status. That is blind to a new failure inside a package that was already red: the package reads "fail" both before and after, and a status-level diff shows nothing. meshed is exactly that case — it carries 3 pre-existing doctest failures on its own master (meshed/dag.py, meshed/makers.py, meshed/scrap/cached_dag.py), so it was already "fail" and its regression was invisible to the gate.

Re-run at per-test (node id) granularity across i2 plus all 46 local dependents (1549 test ids before, 1567 after), the true result is:

1 pass → fail regression: meshed/tests/test_ch_funcs.py::test_ch_funcs_no_change, failing with TypeError: missing a required argument: 'a'. Deterministic, A/B-verified: that file is 3 passed against i2 master and 1 failed, 2 passed against this branch.

That test is a fossil of the very bug this PR fixes, not a defect introduced by it. meshed.dag.ch_funcs is built on double_up_as_factory with first parameter func_nodes, so before this fix ch_funcs(func_nodes=..., func_mapping=...) wrongly returned a factory — and the test compensated with a trailing () to get its DAG. With the fix it returns the DAG directly, so that () calls the DAG instead. The fix is correct; the test was written against the bug.

Remedied by i2mint/meshed#76, which should land before (or together with) this PR. That PR is written to be safe to merge first: its new regression pin feature-detects this fix rather than pinning an unreleased i2 version, so it skips on today's i2 and starts enforcing the moment this lands.

With i2mint/meshed#76 applied, the per-test diff across i2 + 46 dependents is:

• 0 pass → fail
• 19 tests added, all passing (18 here, plus meshed's regression pin)
• 0 tests removed, 0 other outcome or status changes

Reproducing the per-test gate. Same discovery and pytest invocation as priv.dep_graph.run_dependents_tests (--doctest-modules, -o doctest_optionflags=ELLIPSIS IGNORE_EXCEPTION_DETAIL, each repo's exclude_paths as --ignores), plus -v --tb=no -p no:randomly; each <nodeid> <OUTCOME> line is parsed into {package: {nodeid: outcome}} and the two maps are diffed — instead of diffing per-package status. Run once with i2 at master and once at this branch.

Survey for other fossils of this shape

Two independent passes over the local ecosystem, looking for other call sites that pass the wrapped object by keyword to a double_up_as_factory-built decorator and then call the result:

• Static. Enumerated every double_up_as_factory-built decorator and its first parameter name — i2: wrap, ch_names, include_exclude, rm_params, add_smart_defaults, _conditional_arg_trans (all func); meshed: ch_funcs, ch_names (func_nodes), code_to_dag, code_to_fnodes (src); front: prepare_for_dispatch, store_on_output, prepare_for_crude_dispatch, inject_enum_annotations (func); larder: store_on_output (save_name_or_func), prepare_for_crude_dispatch (func); plus one WIP package's azure_wrap (func). Then searched every .py, .ipynb, .md and .rst in the ecosystem for calls giving that first parameter by keyword — including multi-line calls — and for **-splat calls into those decorators. Also checked for aliased imports (none).
• Dynamic. Temporarily instrumented _double_up_as_factory to log every call that takes the new kwargs.pop(...) path, and ran the whole dependents sweep under it. This catches call sites grep cannot see, e.g. a func= arriving through **kwargs forwarding.

Both passes converge on the same live call site: meshed/tests/test_ch_funcs.py:34 — the only one in code that any test suite executes. Every other probe hit was one of this PR's own new tests or doctests.

Correction (found in re-review). The static pass missed a second occurrence of the same fossil: meshed/scrap/notebook.ipynb, a 2023 scratch notebook whose saved output literally shows the bug (functools.partial(<function ch_funcs ...>, func_nodes=...)). It is dead code — scrap/ is in meshed's CI paths-to-ignore, no notebook plugin collects it, and it produced no test change in any gate run — but re-running that notebook after this lands would raise. It is deliberately left as-is: rewriting its source without re-executing it would desynchronise the cells from their stored outputs. So the accurate statement is one affected call site in executed code, plus one in an unexecuted scratch notebook.

Not affected, checked explicitly: mongodol.track_method_calls and dol's own decorators use dol's separate copy of double_up_as_factory; DAG.ch_funcs and Sig.ch_names are ordinary methods that merely share a name with these decorators.

Caveat, stated plainly: the dynamic pass only sees code the dependents' suites actually execute, and the static pass only sees checked-in local source. A dynamic func= forward in untested code, or in code outside this ecosystem, would be caught by neither.

Other notes

An apparent mongodol regression during the original gate turned out to be leftover state in a local test database, not this change — mongodol uses i2 only for Sig, and its store_layers/wrap_kvs come from dol's own separate double_up_as_factory. With a clean database, mongodol is green both with and without this change.

dol carries an independent copy of double_up_as_factory in dol/trans.py (not vendored from i2/deco.py), so this fix is i2-local. That copy looks like it has the same bug and may be worth a separate issue.

Note for whoever merges

This repo's squash-merge default composes the merge commit body from the branch's commit
messages (squash_merge_commit_message = COMMIT_MESSAGES). The first commit's message
still contains the retracted claim that the dependents' suites were "identical ... all
pre-existing and unrelated" with no regressions. That sentence is wrong — see
"Dependents gate — corrected" above for what actually happened. Please either amend that
commit message or edit the squash body at merge time, so the false claim does not land in
master's history. It was not amended here because rewriting already-pushed history is
outside what this session is permitted to do.

https://claude.ai/code/session_01Kug7UUbVeCQgruvNXUq63c

[thorwhalen]

Independent verification — one correction to the dependents claim

I re-ran everything on this branch. The fix itself holds up:

• Premise reproduces on master: all five decorators return a partial for deco(func=...).
• Suite: master 690 passed / 2 xfailed → branch 708 passed / 2 xfailed. Reproduced.
• Red-before-green reproduced exactly: reverting only deco.py and keeping the new tests gives 12 failed / 6 passed; restoring the fix gives 18 passed. The new doctests also fail against the old logic (got multiple values for argument 'func').
• Public signatures of all five decorators are unchanged. Verified programmatically.

Correction: the PR body says the dependents gate is "identical: 32 pass / 14 fail. All 14 failures are pre-existing and unrelated." The 32/14 status split is right, but that granularity hides a real regression, because a package that was already red stays "fail" no matter how many more tests break inside it.

Re-running the full 47-suite gate on both sides and diffing per-test rather than per-package, there is exactly one new failure:

meshed: 3 failed, 157 passed  (master)
meshed: 4 failed, 156 passed  (this branch)
NEW: meshed/tests/test_ch_funcs.py::test_ch_funcs_no_change

It is causal and deterministic (A/B'd against this branch vs master), and meshed is red on both sides — it was never "green both ways".

The cause is benign and actually confirms the fix is right. That test does:

new_dag = ch_funcs(func_nodes=nodes, func_mapping=dummy_mapping)
new_nodes = new_dag().func_nodes   # <- the extra call compensated for the bug

meshed.dag.ch_funcs is built on double_up_as_factory with first parameter func_nodes, so pre-fix the keyword form returned a factory and the author had to call it to get the DAG. Post-fix it returns the DAG directly, so the trailing () now tries to call the DAG. The test is a fossil of the bug.

The one-line companion fix, verified locally against this branch (3 passed):

new_nodes = new_dag.func_nodes

DAG.ch_funcs (the method) passes the object positionally, so it is unaffected — the blast radius is this one call site. Nothing else in the ecosystem passes the wrapped object by keyword to a double_up_as_factory decorator; I grepped for it.

Suggest landing a companion meshed PR before or with this one, so the gate is genuinely clean.

One pre-existing wrinkle worth noting (not introduced here): for decorators with **kwargs whose first parameter is func (ch_names, add_smart_defaults, _conditional_arg_trans, ensure_iterable_args), the first parameter's name is effectively reserved in the factory direction. ch_names(func='new_name') was already broken before this change (got multiple values for argument 'func') and is still broken after, just with a different message ('str' object is not callable). No regression, but it may deserve a docstring note.