Repository navigation
Preserve overload arrow arity with stored call wrappers - #318
Merged
Merged
Conversation
Computed class-field arrows need a per-instance key capture so their inferred name survives engines without field-key inference (Node 18): the field key stays a plain coerced expression while the value routes through an inner object wrapper. The plain path now gates on overload presence instead of arity, so rest-only arrows capture too. Adds key-spelling, carrier-collision, and evaluation-order spec files.
…ad paths
overloadSignature held only return {tuple, templates} with one caller and
a spec that pinned construction: stubbing it broke nothing the suite
could name, so the literal goes inline in overloadSignatures. The same
bar applies to overloadInvocation (string concat inlined at the guarded
call site), overloadImplParams (null guard plus parseJSDoc forward), and
dummyArityNames (index loop folded into dummyArityHead, which owns the
collision renaming the loop feeds).
ArrowFunctionExpression and generateTypeChecks shed their overload
blocks into overloadArrowHead and emitOverloadChecks: same file, each
branch now fits on one screen. ClassProperty and ObjectProperty keep
their duplicated capture bodies for per-site overrides.
AGENTS.md gains the extraction bar that drove this: a helper file must
capture a meaning that can fail on its own, adjudicated by the stub
test, with this commit as the deletion precedent and dummyArityHead
plus overloadNameKey as the keeping precedents.
Emptying templates passed test_runtime silently: no unit spec pinned template content, so only the overloads fixture caught the joint NoInfer regression. testOwnTemplateSurvivesMapping fails when the mapping drops the section binding, testOwnTemplateShadowsScope fails when scope merge order flips or scope-only names are lost.
Bare single-line ifs hide branches from review and from diffs, so the Test style section now requires braces on every control body. The curly eslint rule stays off, review enforces. The branch's own emitOverloadChecks guard complies.
validateTuple braces its two bare optional guards; the inferred-name specs build their sources as multiline templates; the eraser specs build tuples through overloadToTuple and fnNodes through parse, per the spec input rules.
The rest-only wrapper always inferred length zero, so emission compensated with a runtime setOverloadArrowLength(fn, arity) call. The dummy-head rewrite declares real leading slots, so length is right without a call: zero hits across both regenerated outputs while length stays pinned directly off emitted functions. Removes the header import, the runChecks harness slots, the helper, its spec, the barrel export, and the test wiring.
dummyArityHead split its glued prefix into open, store, and entry parts; ArrowFunctionExpression places each on its own indented line through numSpaces and this.spaces instead of concatenating a single line. Regenerates the overload fixtures under the readable emission.
The keepName wrapper opened with inline '({' glue and closed with a
glued '})' suffix, and both capture builders concatenated their brace
tails inline, hiding nesting from the eye and from diffs. Each part now
rides numSpaces and this.spaces on its own line; the brace-heavy capture
tails bind to a named close first, since '}}' inside a template literal
reads as interpolation close plus one literal brace and silently drops
the other. Regenerates the overload fixtures under the readable
emission.
The dummy-head churn around async and generator marks touched the wrapper entry both engines execute, so the suite now pins each shape end to end: async arrows await through the wrapper while the check fires first, extra arguments forward through the stored rest carrier, block-body async functions keep lexical this, async generators keep their protocol, and sync generator declarations warn before the first yield without consuming the iterable. dummyArityHead gains the joined async-plus-generator entry pin. Each case fails when its mark is removed: killing asyncMark throws await SyntaxError, killing generatorMark fails the entry pin.
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.
Fixes #42
Primer
A function with
@overloaddocs promises several call shapes, and the transpiler turns each documented shape into a positional tuple plus template bindings so the runtime can try every signature with independent inference and report one failure when no shape accepts the call. The check must run before the body, so the emitter wraps the function: the wrapper head carries the observablelength,name,async, and generator behavior while the inner function keeps the declared parameters and the inspection call reads the actual arguments. Computed keys complicate this because the key expression evaluates once per object or instance creation, and Node 18 does not infer a functionnamefrom a computed key where Node 24 does, so the emission captures the coerced key outside the function and routes the value through an inner object literal that restores the name on both engines. A full worked pass over a one-overload arrow plus a computed class field closes this file, and a deep dive on the name-preserving shell comes next: it introduces the shape piece by piece, shows which engine gap each piece covers, and justifies why the ugly wrapper is needed at all.Why the shell exists
The shell is the
({["action"]: WRAPPER})["action"]object literal around the dummy-head wrapper, emitted only whenkeepNameholds (src-transpiler/Asserter.jsoverloadArrowHead: a wrapper exists,arrowInferredNamereturned non-null, and no captured key already covers the name). It looks redundant next toconst action = WRAPPER, because a plain binding infersnameon both engines. The probes below show where that intuition fails and why each piece of the shape is load-bearing.namefrom its syntactic position, and the inference table has holes. A probe over one arrow per class-field shape reportsnameon Node 24 versus Node 18: identifier fields infer"action"on both, private fields infer"#priv"on both, but string-literal fields infer"literal"on 24 andundefinedon 18, numeric fields infer"0"on 24 andundefinedon 18, and computed fields infer"dynamic"on 24 andundefinedon 18. Object-literal string keys infer on both engines, so the hole is specific to class fields with literal or computed keys on Node 18.["literal"] = (rtiParam0, ...) => ..., and the Node 18 probe readsname === undefinedoff the live field. The original arrow already lost its name at that position; wrapping it in one more anonymous arrow cannot bring the name back. The shell fixes this by giving the wrapper a position that infers on every engine: an object literal with a quoted key.({["literal"]: WRAPPER})["literal"]evaluates the inner arrow at a["literal"]property, both engines setnameto"literal", and the trailing["literal"]lookup hands the named function to the field. A probe of the exact shelled shape reports"literal"and"0"on both engines.overloadNameKeyquotes throughJSON.stringify, so keys that are not valid identifiers survive:"lit-key"keeps its hyphen,"0"keeps its numeric spelling, and"#privateAction"keeps the hash the private field needs (see the#privateActionfixture attest/typechecking/overload-fields-output.mjs:73-99). A bare identifier key would be a syntax error for"lit-key"and would name the private accessor wrong. The trailing lookup spells the same key so evaluation order stays single-shot: the key expression appears once in the property and once in the lookup for static names, and for computed keys it appears as the pre-coercedoverloadPropertyKeycall, never re-evaluated.keepNameis false whennode.rtiOverloadKeyNameis set, which is exactly the dynamic case:capturedClassFieldand theObjectPropertyspread already route the arrow through[rtiOverloadKey]: arrowinside a capture that evaluates the key once per creation. Adding the static shell there would double the wrapper: an outer({[key]: ...})[key]around an inner(({k} => ({[k]: arrow}))(key))[key], with two key evaluations and two object allocations per instance. The collision and reentrancy specs (overloadedComputedClassFieldCollisions,overloadedComputedClassFieldEvaluation) pin the single-evaluation behavior, so the shell stays off where the capture already restores the name.arrowInferredNamereturns""(a callback argument with no binding to infer from), the emitter still wraps as({[""]: WRAPPER})[""]: the probe maps[1, 2]through it and gets[2, 4]back on both engines. Dropping the shell only for""would add a branch tooverloadArrowHeadfor a shape that already works, and the branch would need its own spec to prove it never fires for a named position. The uniform rule (shell whenever no capture covers the name) is one condition instead of two.keepNameto false turns the#privateActionfixture into#privateAction = (rtiParam0, ...) => ..., and the hostile spectestOverloadedArrowMetadatafails on Node 18 withname === undefinedwhere it expects"#privateAction". The failure is engine-specific by construction: the same emission passes on Node 24, which is why the gate runstest_runtime.json both binaries.Demonstration
An overloaded arrow with one parameter documents its contract and keeps its metadata:
The emitter wraps the arrow and checks the store before the body runs. The object shell survives only for static names, where it restores the quoted key on engines that would otherwise drop it:
At runtime
action(1)passes silently,action('bad')reportsarguments: Argument of type ["bad"] is not assignable to parameter of type [value: number], andaction.lengthis 1 withaction.namestillaction, because the dummy slots keep arity in syntax and the quoted key restores the name.A plain function declaration needs no wrapper at all: the check inlines into the body, so there is no head, no store, and no shell:
At runtime
take(1)passes andtake('bad')warns, withtake.lengthandtake.nameuntouched: the wrapper machinery exists only for arrows, whose expression position carries no name of its own.A computed class field adds key capture around the same wrapper:
The capture carries the coerced key, so the arrow needs no shell of its own: the inner object that holds the key already restores the name. This is the no-shell emission:
At runtime
fields.computed(1)passes,fields.computed('bad')warns, andfields.computed.lengthis 1 withfields.computed.nameequal tocomputedon both Node 24 and Node 18, because each instance coerces its own key and the inner carrier object restores the name where the engine would not. Cutting the arrow-level shell for captured keys is what removed the doubled inner wrapper.What changed
src-runtime/inspectOverloads.jstries each{tuple, templates}signature with fresh template bindings and reports a single failure when none accepts the arguments,src-runtime/overloadPropertyKey.jscoerces computed keys for capture, andsrc-runtime/validateTuple.jsvalidates positional members including optional and variadic slots: before there was no overload runtime path, after overloaded calls validate against the union of documented tuples with per-signature inference; the proof istest/typechecking/overloads-errors.jsonpinning every throwing call plustest/jsdoc-expect/run.mjsagreeing 44/44 and thesumfixture acceptingsum('x', 1, 2)while warning onsum('x', 1, 'bad')andsum(1, 2).src-transpiler/collectOverloadBlocks.js,src-transpiler/splitOverloadSections.js,src-transpiler/overloadToTuple.js,src-transpiler/overloadSignatures.js,src-transpiler/overloadArity.js,src-transpiler/arrowArity.js,src-transpiler/arrowInferredName.js,src-transpiler/dummyArityHead.js,src-transpiler/overloadInspectionArgs.js,src-transpiler/overloadNameKey.js,src-transpiler/overloadIgnoreSet.js,src-transpiler/eraseIgnoredOverloadParams.js,src-transpiler/freshName.js, andsrc-transpiler/freshKeyName.jsform the overload pipeline from comment blocks to runtime signatures: beforeAsserter.jsbuilt arities, names, and signatures inline, after each branch is a named helper with its own spec, barrel export, andtest_runtime.jswiring; the proof issrc-transpiler/overloadSignatures.spec.jsfailing when template bindings are dropped (including theNoInferjoint-inference case that warns twice more) andsrc-transpiler/dummyArityHead.spec.jsfailing when collision renaming breaks.src-transpiler/Asserter.jsemits the dummy-head plus stored-list wrapper throughoverloadArrowHeadand the check block throughemitOverloadChecks, with computed object and class-field keys captured per creation through the shared key shuttle: before overloaded arrows lostlengthand computed field arrows lostnameon Node 18, aftertest/typechecking/overload-fields-output.mjsshows the stored-list checks for instance, static, private, and computed fields with per-instance key reads of 1 to 3 and 1 to 5 plus reentrant 2, 6, and 8; the proof istestOverloadedComputedClassFieldChecks,testOverloadedComputedClassFieldSecondInstance, and the collisions plus evaluation hostile specs, all green on Node 24 and Node 18.test/typechecking/overloads-input.mjs,test/typechecking/overloads-output.mjs,test/typechecking/overload-fields-input.mjs,test/typechecking/overload-fields-output.mjs, and their*-errors.jsonfiles pin the runtime contracts including variadic rest, template shadowing, and theNoInferjoint-inference case; the proof is thejsdoc-expectrunner failing on any missing or extra throw and the 65/65 parity harness agreeing with the tsc oracle.AGENTS.mdrecords the extraction bar (a helper file must capture a meaning that can fail on its own, judged by the stub test) and the always-brace control-body rule, andsrc-runtime/validateTuple.js,src-transpiler/arrowInferredName.spec.js, plussrc-transpiler/eraseIgnoredOverloadParams.spec.jscomply with the snippet and brace rules with producer-built inputs: before the rules were unwritten and the specs hand-built shapes, after the rules name this branch as precedent and the specs build throughoverloadToTupleandparse; the proof isnpm run lintclean with the new specs failing under the corresponding mutations.Latent holes the work exposed
Asserter.jskeepPlainPropertystill gates its capture branch on a shape that never fires for overloaded arrows: plain objects infer on both engines (verified identifier plus coercing keys on Node 24 and 18), so the dead branch is tolerated deliberately with that parity analysis as cover.expandTypeerases, with a poison{number[]} ...valuesshape and a silentdotDotdrop for bare and optional rest: the re-scan hid the loss, the tsc parity probes intmp/jsdoc-parsing.md(git-ignored) exposed it; deferred to its own branch so this PR stays emission plus cleanup with byte-identical outputs.