convert ArrowFunctionExpression to FunctionExpression in traceFunction#60
convert ArrowFunctionExpression to FunctionExpression in traceFunction#60crysmags wants to merge 6 commits intonodejs:mainfrom
Conversation
| // 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.
There was a problem hiding this comment.
Using apply won't work when a class is extended from another class. In those cases this can't be accessed until after super() has been called and apply(this, arguments) evaluates first.
| // __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.
There was a problem hiding this comment.
If we always replace all params with a rest element the length drops to zero because rest elements don't count toward .length, so (...__apm$args) gives length 0.
Appending preserves .length since named params still count, but __apm$args would be empty for a normal call — it only captures overflow arguments beyond the named ones. So we can't use it to forward all arguments to __apm$wrapped, which makes it unusable for our purposes.
What does this PR do?
Fixes how the rewriter handles arrow function arguments. Arrow functions have no own arguments binding — they
inherit it from the enclosing scope. The generated wrapper preamble previously used arguments directly, which
resolved to the wrong scope for arrow functions. This caused silent data corruption (wrong arguments forwarded)
or hard crashes in ESM (ReferenceError: arguments is not defined).
Approach
The initial approach was to convert ArrowFunctionExpression nodes to FunctionExpression before wrapping, giving
the wrapper its own arguments. However this breaks lexical this for code that relies on it — arrow functions
capture this from the enclosing scope, and converting the node type changes that binding.
The next attempt used rest parameters (...__apm$args) on arrow functions only, keeping other function types
unchanged. This preserved this but introduced inconsistency between arrow and non-arrow code paths, and dropped
the function's .length to 0.
The final approach replaces params on all function types with numbered placeholders (__apm$arg0, __apm$arg1,
...__apm$args) via a shared wrapParams helper. The preamble reconstructs all arguments into __apm$arguments
from the placeholders. This:
methods, and constructors
For constructors in classes that extend another class, apply(null, ...) is used instead of apply(this, ...)
because this can't be accessed before super() has been called.
For traceInstanceMethod, the original method's params aren't known at instrumentation time (the method is
assigned dynamically at runtime), so .length is restored via Object.defineProperty after capturing the
original.
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
(expressionName included) had the same broken arguments binding.