convert ArrowFunctionExpression to FunctionExpression in traceFunction#60
convert ArrowFunctionExpression to FunctionExpression in traceFunction#60crysmags wants to merge 2 commits intonodejs:mainfrom
Conversation
lib/transforms.js
Outdated
| // a regular FunctionExpression gives the wrapper its own `arguments` (the | ||
| // actual call arguments) and a dynamic `this` (the receiver at call time). | ||
| if (node.type === 'ArrowFunctionExpression') { | ||
| node.type = 'FunctionExpression' |
There was a problem hiding this comment.
Changing the original function type would break any existing references to this or arguments in the wrapped code.
There was a problem hiding this comment.
Good point — converting node.type would change the this semantics for any code in the original body that relied on the arrow's lexical binding. Instead we now replace the outer arrow's params with a rest element (...args → __apm$args) to capture call-site arguments, and pass the original params through to the inner __apm$wrapped function so the original body's parameter names remain valid. The preamble for the outer-arrow case uses __apm$args and preserves lexical this via .apply(this, __apm$args) — since the outer is still an arrow, this there is already the correct lexical binding from the enclosing scope.
| }, | ||
| ]) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Another test should be added to check that we're not breaking any this or arguments reference within the wrapped code (which was the case with the original code of this PR)
|
|
||
| const block = wrapper.body[0].body // Extract only block statement of function body. | ||
| const common = parse(node.type === 'ArrowFunctionExpression' | ||
| const common = parse(node.outerIsArrow |
There was a problem hiding this comment.
Let's just always use apply so that this check is not needed. I don't remember why they were separate to begin with as arrow functions also have a this that can be useful, especially in class methods defined as arrow functions.
| // __apm$args. The original params are preserved for the inner __apm$wrapped | ||
| // function so the original body's parameter names remain valid. | ||
| const originalParams = node.params | ||
| if (node.type === 'ArrowFunctionExpression') { |
There was a problem hiding this comment.
Let's just always do this to be consistent. If it's ever a problem, we could always append instead of replacing, and then carry over all params.
What does this PR do?
In
traceFunction(lib/transforms.js), converts anArrowFunctionExpressionnode to aFunctionExpressionbefore wrapping it:Why
Arrow functions have no own
argumentsbinding — they inherit it lexically from the enclosing scope. The generated wrapper preamble usesargumentsin two places:If the outer function stays as an
ArrowFunctionExpression,argumentsresolves to whatever the enclosing scope provides rather than the actual call-site arguments. This causes two failure modes:Silent data corruption — if the arrow function is inside a constructor (e.g.
this._query = async (sql) => {}),argumentsresolves to the constructor's arguments (e.g. connection options). The inner function receives wrong parameters and the span context carries wrong values.Hard crash in ESM — if the arrow function is at module scope,
argumentsis not defined and the instrumented module throwsReferenceError: arguments is not definedon load.Converting to
FunctionExpressiongives the wrapper its ownargumentsand a dynamicthis, making argument forwarding and span context correct for all call sites.Relation to #58
Directly motivated by #58 (
objectName+propertyName), which targets arrow functions assigned inside constructors — without this fix, #58's selector correctly finds and rewrites those functions but the instrumentation silently misbehaves at runtime. However the fix is broader — any query type targeting an arrow function (expressionNameincluded) has the same brokenargumentsbinding.