fix(wrapper): stop Wrap mutating the ingress it is given - #84
Merged
Conversation
`preserve_signature` wrote `__signature__`/`__annotations__` onto the ingress object.
A decorator normally defines one ingress and reuses it for every function it wraps, so
that write corrupted the caller's function and made every later Wrap built from the same
ingress advertise the FIRST wrapped function's signature -- while still executing
correctly, so nothing surfaced the lie:
def shared_ingress(*args, **kwargs): return args, kwargs
def f(a: int) -> int: ...
def g(x: str, y: str) -> str: ...
Wrap(f, ingress=shared_ingress)
signature(shared_ingress) # (a: int) -> int <- caller's function mutated
Sig(Wrap(g, ingress=shared_ingress)) # (a: int) -> str <- wrong, but g still works
That is the worst shape of bug for this package: silent, and wrong in exactly the
introspection i2 exists to provide (meshed builds DAGs from these signatures).
Preserving means *the wrapper* presents func's interface, so read the signature from
func and never write it onto the ingress. Defaults now come from the same source as the
signature, so the two cannot disagree.
Also, while in here:
- `_get_return_annotation` had `x if x is not Parameter.empty else empty` three times.
`empty IS Parameter.empty`, so each was a no-op wrapped around a duplicated fallback.
Reduced to one "egress wins if annotated, else func" rule with doctests, including the
case that made the dead branch look meaningful (an unannotated func gives `empty`,
not `None` -- `None` is a real annotation meaning "returns None").
- Extracted `_is_generic_signature`, the actual predicate `'auto'` turns on, with
doctests for the near-misses ((*a) alone, (x, *a, **kw)).
- Dropped the unused `func` parameter from `_should_preserve_signature`.
- Named the `'auto'` sentinel `AUTO_PRESERVE_SIGNATURE` so it has one definition.
- Converted the numpydoc docstrings to the `:param:` style used everywhere else here.
Public API unchanged. Regression tests fail on the old code and pass on the new.
i2: 470 passed. meshed (biggest dependent): 60 passed. crude/o failures are pre-existing
and byte-identical with and without this change.
Claude-Session: https://claude.ai/code/session_01GsUw8ey8KikzNFQ1UaWWia
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while reviewing the abandoned
claude/implement-wrapper-improvements-*branch (thorwhalen/priv#38). That branch turned out to be already merged by another route — its only remaining difference from master is quote style. But reviewing it surfaced a real bug in the code that did land.The bug
preserve_signaturewrote__signature__and__annotations__onto the ingress object. A decorator normally defines one ingress and reuses it for every function it wraps, so that write corrupts the caller's function — and every laterWrapbuilt from the same ingress:Wrap(g, ...)still executes correctly (w2('a','b') == 'ab'), so nothing ever surfaced the lie. That is the worst shape this bug could take in this package: silently wrong in exactly the introspection i2 exists to provide, and meshed builds its DAGs from these signatures.The fix
Preserving means the wrapper presents
func's interface — so read the signature fromfunc, and never write it onto the ingress. Defaults are now read from the same source as the signature, so the two can't disagree.Cleanups in the same area
_get_return_annotationhadx if x is not Parameter.empty else emptythree times.emptyisParameter.empty, so each was a no-op around a duplicated fallback. Now one rule — egress wins if annotated, else func — with doctests covering the case that made the dead branch look meaningful: an unannotated func yieldsempty, notNone, sinceNoneis a real annotation meaning "returns None"._is_generic_signature(the predicate'auto'actually turns on), with doctests for the near-misses:(*a)alone,(x, *a, **kw).funcparameter from_should_preserve_signature.'auto'sentinelAUTO_PRESERVE_SIGNATUREso it has one definition.:param:style used elsewhere in this module.Public API unchanged.
Verification
i2.wrapper:extrude,front,larder,oapass;crudeandofail identically with and without this change (selenium, missinghum.sound) — pre-existing.Two hunks in the diff are ruff reformatting untouched
assertstatements — the publish job runsruff format .and pushes back anyway, so they'd land regardless.https://claude.ai/code/session_01GsUw8ey8KikzNFQ1UaWWia