diff --git a/lib/transforms.js b/lib/transforms.js index 7716c1d..967fd84 100644 --- a/lib/transforms.js +++ b/lib/transforms.js @@ -250,6 +250,15 @@ function traceFunction (state, node, program) { const type = isConstructor ? 'ArrowFunctionExpression' : node.type const params = node.params + // A derived constructor cannot read `this` in the wrapper, so record it at + // the `super()` call sites and point the wrapper at that binding instead. + // The name is namespaced by channel so that a second channel wrapping the + // same constructor writes to its own binding rather than shadowing ours. + const selfBinding = `${formatChannelVariable(state.channelName)}$self` + state.selfBinding = isConstructor && captureSelfAtSuper(node, selfBinding) + ? selfBinding + : null + node.body = wrap(state, { type, params, @@ -268,7 +277,13 @@ function traceFunction (state, node, program) { node.generator = false node.async = false - wrapSuper(state, node) + // An arrow function inherits `super` from its enclosing method, so a body + // moved into one keeps working and needs no rewrite. Rewriting it there is + // in fact wrong for a constructor: the capture it hoists reads `super.x` + // with `this` as the receiver, above the body's own `super()` call, where + // `this` is still unbound. Only the real `function` wrapper, which + // generators need, loses the binding. + if (type !== 'ArrowFunctionExpression') wrapSuper(state, node) } /** @@ -342,6 +357,7 @@ function traceInstanceMethod (state, node, program) { fn.params = [{ type: 'RestElement', argument: { type: 'Identifier', name: '__apm$args' } }] fn.async = operator === 'tracePromise' + state.selfBinding = null fn.body = wrap(state, { type: 'Identifier', name: savedBinding }, program) wrapSuper(state, fn) @@ -361,7 +377,7 @@ function traceInstanceMethod (state, node, program) { * @returns {import('estree').BlockStatement['body']} */ function wrap (state, node, program) { - const { operator, moduleVersion } = state + const { operator, moduleVersion, selfBinding } = state const { returnKind } = state.functionQuery const iterPatch = returnKind ? generateIterPatch(state, returnKind, program) : '' @@ -378,7 +394,10 @@ function wrap (state, node, program) { .map((_, i) => `__apm$arg${i}`).concat('...__apm$args').join(', ') const block = wrapper.body[0].body // Extract only block statement of function body. - const common = parse(node.type === 'ArrowFunctionExpression' + // Declared in the outer function scope so that the `super()` call sites in + // the moved body can write to it and the `finally` block can read it back. + const declareSelf = selfBinding ? `let ${selfBinding};` : '' + const common = parse(declareSelf + (node.type === 'ArrowFunctionExpression' ? ` const __apm$arguments = [${args}]; const __apm$ctx = { @@ -401,7 +420,7 @@ function wrap (state, node, program) { const __apm$wrapped = () => {}; return __apm$wrapped.apply(this, __apm$arguments); }; - `).body + `)).body block.body.unshift(...common) @@ -411,6 +430,75 @@ function wrap (state, node, program) { return block } +/** + * The expression a wrapper's `finally` block uses to fill in `message.self`. + * + * Most functions read `this` directly. A derived constructor cannot, so + * {@link captureSelfAtSuper} records `this` in a variable and the wrapper + * reads that variable instead. + * + * @param {{ selfBinding?: string|null }} state + * @returns {string} + */ +const selfExpression = ({ selfBinding }) => selfBinding || 'this' + +/** + * Rewrites each `super(...)` call in a constructor body so that it also + * records `this` in `binding`. + * + * A derived constructor's `this` stays unbound until `super()` returns, and + * the wrapper moves the original body into a nested arrow, so `super()` now + * runs while the wrapper's own `runStores` callback is on the stack. + * JavaScriptCore (Bun) loads `this` once, when a closure is entered, so that + * callback keeps the unbound value and throws when the `finally` block reads + * it. Assigning at the call site puts the read immediately after `super()`, + * where every engine agrees `this` is bound. + * + * `super(...)` already evaluates to the newly bound `this`, so the sequence + * expression returns the same value the call did. + * + * Nested classes and nested non-arrow functions are skipped: a `super()` call + * inside one of those belongs to that function, not to this constructor. + * + * @param {import('estree').Function} node - The constructor being wrapped. + * @param {string} binding - Name of the variable to record `this` in. + * @returns {boolean} `true` if at least one call was rewritten. + */ +function captureSelfAtSuper (node, binding) { + let found = false + + const visit = (parent, key) => { + const child = parent[key] + + if (child === null || typeof child !== 'object') return + + if (Array.isArray(child)) { + for (let i = 0; i < child.length; i++) visit(child, i) + return + } + + if (typeof child.type !== 'string') return + + if (child.type === 'ClassBody' || + child.type === 'FunctionExpression' || + child.type === 'FunctionDeclaration') return + + if (child.type === 'CallExpression' && child.callee.type === 'Super') { + const sequence = parse(`(0, ${binding} = this)`).body[0].expression + sequence.expressions[0] = child + parent[key] = sequence + found = true + return + } + + for (const name of Object.keys(child)) visit(child, name) + } + + visit(node, 'body') + + return found +} + /** * Rewrites `super.method(...)` calls inside a moved function body. * @@ -566,7 +654,7 @@ function wrapCallback (state, node, iterPatch = '') { ${channelVariable}.error.publish(__apm$ctx); throw err; } finally { - __apm$ctx.self ??= this; + __apm$ctx.self ??= ${selfExpression(state)}; ${channelVariable}.end.publish(__apm$ctx); } }); @@ -651,7 +739,7 @@ function wrapPromise (state, node, iterPatch = '') { ${channelVariable}.error.publish(__apm$ctx); throw err; } finally { - __apm$ctx.self ??= this; + __apm$ctx.self ??= ${selfExpression(state)}; ${channelVariable}.end.publish(__apm$ctx); } }); @@ -693,7 +781,7 @@ function wrapSync (state, node, iterPatch = '') { ${channelVariable}.error.publish(__apm$ctx); throw err; } finally { - __apm$ctx.self ??= this; + __apm$ctx.self ??= ${selfExpression(state)}; ${channelVariable}.end.publish(__apm$ctx); } return __apm$ctx.result; diff --git a/tests/constructor_self_cjs/mod.js b/tests/constructor_self_cjs/mod.js new file mode 100644 index 0000000..9ebd8ce --- /dev/null +++ b/tests/constructor_self_cjs/mod.js @@ -0,0 +1,19 @@ +/** + * Unless explicitly stated otherwise all files in this repository are licensed under the Apache-2.0 License. + * This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2025 Datadog, Inc. + **/ +class UndiciBase { + constructor (val) { + this.base = val + } +} + +class Undici extends UndiciBase { + constructor (val) { + if (val === 'boom') throw new Error('boom') + super(val) + this.val = val + } +} + +module.exports = Undici diff --git a/tests/constructor_self_cjs/test.js b/tests/constructor_self_cjs/test.js new file mode 100644 index 0000000..1a8f8e1 --- /dev/null +++ b/tests/constructor_self_cjs/test.js @@ -0,0 +1,30 @@ +/** + * Unless explicitly stated otherwise all files in this repository are licensed under the Apache-2.0 License. + * This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2025 Datadog, Inc. + **/ +const Undici = require('./instrumented.js') +const assert = require('node:assert') +const { tracingChannel } = require('node:diagnostics_channel') + +const ends = [] +tracingChannel('orchestrion:undici:Undici_constructor').subscribe({ + end (message) { + ends.push(message) + } +}) + +const undici = new Undici(42) +assert.strictEqual(undici.val, 42) +assert.strictEqual(undici.base, 42) +assert.strictEqual(ends.length, 1) +assert.strictEqual(ends[0].self, undici) + +// A throw before `super()` leaves `this` unbound, so the wrapper must not read +// it. If it does, the original error is replaced by a ReferenceError. +assert.throws(() => new Undici('boom'), (err) => { + assert.strictEqual(err.constructor, Error) + assert.strictEqual(err.message, 'boom') + return true +}) +assert.strictEqual(ends.length, 2) +assert.strictEqual(ends[1].self, undefined) diff --git a/tests/constructor_super_method_cjs/mod.js b/tests/constructor_super_method_cjs/mod.js new file mode 100644 index 0000000..6c784e9 --- /dev/null +++ b/tests/constructor_super_method_cjs/mod.js @@ -0,0 +1,22 @@ +/** + * Unless explicitly stated otherwise all files in this repository are licensed under the Apache-2.0 License. + * This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2025 Datadog, Inc. + **/ +class UndiciBase { + constructor (val) { + this.base = val + } + + greet () { + return `hi ${this.base}` + } +} + +class Undici extends UndiciBase { + constructor (val) { + super(val) + this.val = super.greet() + } +} + +module.exports = Undici diff --git a/tests/constructor_super_method_cjs/test.js b/tests/constructor_super_method_cjs/test.js new file mode 100644 index 0000000..828c2bf --- /dev/null +++ b/tests/constructor_super_method_cjs/test.js @@ -0,0 +1,19 @@ +/** + * Unless explicitly stated otherwise all files in this repository are licensed under the Apache-2.0 License. + * This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2025 Datadog, Inc. + **/ +const Undici = require('./instrumented.js') +const { assert, getContext } = require('../common/preamble.js') +const context = getContext('orchestrion:undici:Undici_constructor'); + +// `super.greet()` uses `this` as the receiver, so it only works after the +// body's own `super()` call. The wrapper must leave it where it was. +(() => { + const undici = new Undici(42) + assert.strictEqual(undici.base, 42) + assert.strictEqual(undici.val, 'hi 42') + assert.deepStrictEqual(context, { + start: true, + end: true + }) +})() diff --git a/tests/tests.test.mjs b/tests/tests.test.mjs index 6db2b7b..a0ce642 100644 --- a/tests/tests.test.mjs +++ b/tests/tests.test.mjs @@ -10,6 +10,20 @@ import { SourceMapConsumer } from 'source-map' const __dirname = dirname(fileURLToPath(import.meta.url)) +// Bun is not an officially supported engine, but people do run this code under +// it, and its JavaScriptCore engine is stricter than V8 about reading `this` in +// a derived constructor. Run every fixture under Bun as well when it is +// installed, so that engine's rules stay covered. +const hasBun = spawnSync('bun', ['--version'], { stdio: 'ignore' }).status === 0 + +function runEngine (engine, file, cwd) { + const result = spawnSync(engine, [file], { cwd, stdio: 'pipe' }) + if (result.status !== 0) { + const output = (result.stdout?.toString() || '') + (result.stderr?.toString() || '') + throw new Error(`${engine} ${file} exited with ${result.status}:\n${output}`) + } +} + const TEST_MODULE_NAME = 'undici' const TEST_MODULE_VERSION = '0.0.1' const TEST_MODULE_PATH = 'index.mjs' @@ -41,12 +55,8 @@ function runTest (testName, configs, { mjs = false, filePath = TEST_MODULE_PATH, // Injection failure — do not write instrumented file } - const result = spawnSync('node', [`test.${ext}`], { cwd: testDir, stdio: 'pipe' }) - if (result.status !== 0) { - const output = (result.stdout?.toString() || '') + (result.stderr?.toString() || '') - throw new Error(`node test.${ext} exited with ${result.status}:\n${output}`) - } - assert.equal(result.status, 0) + runEngine('node', `test.${ext}`, testDir) + if (hasBun) runEngine('bun', `test.${ext}`, testDir) } describe('arguments_mutation', () => { @@ -114,6 +124,30 @@ describe('constructor_cjs', () => { }) }) +describe('constructor_super_method_cjs', () => { + test('keeps super.method() working inside a derived constructor', () => { + runTest('constructor_super_method_cjs', [ + { + channelName: 'Undici_constructor', + module: { name: TEST_MODULE_NAME, versionRange: '>=0.0.1', filePath: TEST_MODULE_PATH }, + functionQuery: { className: 'Undici' }, + }, + ]) + }) +}) + +describe('constructor_self_cjs', () => { + test('reports the instance as message.self and does not mask a throw before super()', () => { + runTest('constructor_self_cjs', [ + { + channelName: 'Undici_constructor', + module: { name: TEST_MODULE_NAME, versionRange: '>=0.0.1', filePath: TEST_MODULE_PATH }, + functionQuery: { className: 'Undici' }, + }, + ]) + }) +}) + describe('constructor_mjs', () => { test('instruments class constructor (mjs)', () => { runTest('constructor_mjs', [