Skip to content

fix: preserve arguments.length for non-arrow wrapped functions#69

Closed
crysmags wants to merge 1 commit intonodejs:mainfrom
crysmags:fix/preserve-arguments-length-dispatch
Closed

fix: preserve arguments.length for non-arrow wrapped functions#69
crysmags wants to merge 1 commit intonodejs:mainfrom
crysmags:fix/preserve-arguments-length-dispatch

Conversation

@crysmags
Copy link
Copy Markdown
Contributor

Summary

PR #60 replaced every wrapped function's params with numbered placeholders (__apm$arg0…N, ...__apm$args) and rebuilt __apm$arguments from them. That rebuild pads unfilled named slots with undefined, so the array handed to .apply always has the declared arity rather than the caller's real arity. Wrapped functions that dispatch on arguments.length silently take the wrong branch on calls with fewer args than params.

Concrete breakage: graphql's execute(argsOrSchema, document, rootValue, contextValue, variableValues, operationName, fieldResolver, typeResolver) uses the classic dual-mode pattern

return arguments.length === 1
  ? executeImpl(argsOrSchema)                              // object form
  : executeImpl({ schema: argsOrSchema, document, ... })   // positional form

After the PR #60 rewrite, arguments.length inside the moved-out body is always 8 regardless of what the caller passed. graphql.execute({ schema, source }) now takes the positional branch, hands undefined to executeImpl as document, and throws "Must provide document." before any user code runs. Any function with a similar overload (lodash-style options-vs-positional dispatch, optional-trailing-arg detection, variadics that distinguish zero args from one explicit undefined) breaks the same way.

Fix

Two templates, split on whether the outer wrapper is an arrow:

  • Arrow outer — keep PR convert ArrowFunctionExpression to FunctionExpression in traceFunction #60's numbered-placeholder rebuild unchanged. Arrows have no own arguments (and ESM top-level arrows have no enclosing one), so they can't read real arity from arguments. The numbered-placeholder rebuild is the only approach that works uniformly for arrows in every context — nested in a function, at ESM top level, or at CJS top level. This is exactly the correctness guarantee PR convert ArrowFunctionExpression to FunctionExpression in traceFunction #60 was built to deliver, and it's preserved here.
  • Non-arrow outer (regular function, method, constructor) — leave the author's original params on the outer wrapper (preserves .length) and materialize arguments via Array.prototype.slice.call(arguments) in the preamble (preserves real caller arity). Non-arrow outer wrappers always have their own arguments object regardless of surrounding scope or module type, so reading it is safe in every case where this path is taken.

The Array.prototype.X.call(args-like) idiom matches existing usage elsewhere in the wrapper code (Array.prototype.at.call, Array.prototype.splice.call).

Why not unify the two templates

This re-introduces two templates, which a previous iteration of PR #60 was moved away from. Two things have changed:

  1. The earlier "two-template" iteration dropped .length to 0 on the arrow side because it used a bare ...rest param. This PR keeps PR convert ArrowFunctionExpression to FunctionExpression in traceFunction #60's numbered-placeholder approach for arrows, so .length stays correct on both paths.
  2. The single-template design PR convert ArrowFunctionExpression to FunctionExpression in traceFunction #60 landed on has a correctness bug for arguments.length-dispatching functions that cannot be fixed without either (a) reading arguments in the outer wrapper, which re-opens PR convert ArrowFunctionExpression to FunctionExpression in traceFunction #60's arrow and ESM bugs, or (b) detecting unfilled params at runtime, which is not possible in JS (f(undefined) is indistinguishable from f() once a param binding is established). Splitting on wrapper shape is the only way both correctness properties hold simultaneously.

Test plan

  • New test: tests/arguments_length_dispatch_cjs/ — regular function that dispatches on arguments.length, called with 1 arg (expects object-form branch) and with 2 args (expects positional branch), and asserts fn.length still equals the declared arity.
  • Full existing suite (42/42) still passes — arrow-path tests (arrow_expr_args_cjs, arrow_this_binding_cjs, arrow_this_property_args_cjs, object_property_this_cjs, object_property_named_cjs) confirm the PR convert ArrowFunctionExpression to FunctionExpression in traceFunction #60 fast path is unchanged.
  • npm run lint clean.

🤖 Generated with Claude Code

PR nodejs#60 replaced every wrapped function's params with numbered placeholders
and rebuilt `__apm$arguments` from them. That rebuild pads unfilled named
slots with `undefined`, so the array handed to `.apply` always has the
declared arity rather than the caller's real arity. Any wrapped function
that dispatches on `arguments.length` — e.g. graphql's
`execute(argsOrSchema, document, ...)` overload — silently takes the wrong
branch on calls with fewer args than params.

Arrow wrappers still need PR nodejs#60's numbered-placeholder rebuild (arrows
have no own `arguments`, and ESM top-level arrows have no enclosing one),
so the fix is scoped to non-arrow outers: keep the author's original
params on the outer wrapper (preserves `.length`) and materialize
`arguments` via `Array.prototype.slice.call(arguments)` in the preamble
(preserves real caller arity going into the inner body).

Adds a regression test mirroring graphql's `arguments.length === 1`
dispatch pattern.
@rochdev
Copy link
Copy Markdown
Contributor

rochdev commented Apr 22, 2026

I came up with a simpler alternative solution (#70) that also fixes generators when used with objectName+propertyName. That additional fix is actually why it's also simpler, because it makes the internal function consistent with the external function, avoiding the additional checks and conditions.

@crysmags crysmags closed this Apr 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants