From 05af269fcfa66b2987acd8e6b2fe1f831ed0dfad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Rzepecki?= Date: Thu, 1 Aug 2024 13:06:55 +0200 Subject: [PATCH] fix: Use a collision-resistant name for lambda argument capture Lambda argument captures used to use generic (...args) capture which could shadow locals named 'args' in instrumented code closures. This lead to unexpected behaviour when the code tried accessing its own 'args' but ended up accessing AppMap's argument capture instead. Changing to a collision-resistant name ($appmap$args) avoids this issue. Fixes #161 --- src/hooks/__tests__/instrument.test.ts | 24 +++--- src/hooks/instrument.ts | 2 +- test/__snapshots__/simple.test.ts.snap | 104 +++++++++++++++++++++---- test/simple/index.js | 14 ++++ 4 files changed, 116 insertions(+), 28 deletions(-) diff --git a/src/hooks/__tests__/instrument.test.ts b/src/hooks/__tests__/instrument.test.ts index 456b1aea..5032399d 100644 --- a/src/hooks/__tests__/instrument.test.ts +++ b/src/hooks/__tests__/instrument.test.ts @@ -175,29 +175,29 @@ describe(instrument.transform, () => { }, ]; - const outer = (...args) => global.AppMapRecordHook.call( + const outer = (...$appmap$args) => global.AppMapRecordHook.call( undefined, (arg) => arg + 42, - args, + $appmap$args, __appmapFunctionRegistry[0], ); - export const testFun = (...args) => + export const testFun = (...$appmap$args) => global.AppMapRecordHook.call( undefined, (arg) => { var s42 = y => y - 42; let s43 = y => y - 43; - const inner = (...args) => + const inner = (...$appmap$args) => global.AppMapRecordHook.call( undefined, (x) => s42(x) * 2, - args, + $appmap$args, __appmapFunctionRegistry[1], ); return inner(arg); }, - args, + $appmap$args, __appmapFunctionRegistry[2], ); `, @@ -255,20 +255,20 @@ describe(instrument.transform, () => { }, ]; - exports.testFun = (...args) => + exports.testFun = (...$appmap$args) => global.AppMapRecordHook.call( undefined, (arg) => { - const inner = (...args) => + const inner = (...$appmap$args) => global.AppMapRecordHook.call( undefined, (x) => x * 2, - args, + $appmap$args, __appmapFunctionRegistry[0], ); return inner(arg); }, - args, + $appmap$args, __appmapFunctionRegistry[1], ); `, @@ -304,8 +304,8 @@ describe(instrument.transform, () => { "static": true }]; - const testFun = (...args) => global.AppMapRecordHook.call(undefined, x => x * 2, - args, __appmapFunctionRegistry[0]); + const testFun = (...$appmap$args) => global.AppMapRecordHook.call(undefined, x => x * 2, + $appmap$args, __appmapFunctionRegistry[0]); exports.testFun = testFun; `, diff --git a/src/hooks/instrument.ts b/src/hooks/instrument.ts index dabba530..c6c68e3b 100644 --- a/src/hooks/instrument.ts +++ b/src/hooks/instrument.ts @@ -176,7 +176,7 @@ function wrapLambda( lambda: ESTree.ArrowFunctionExpression, functionInfo: FunctionInfo, ): ESTree.ArrowFunctionExpression { - const args = identifier("args"); + const args = identifier("$appmap$args"); return { type: "ArrowFunctionExpression", async: lambda.async, diff --git a/test/__snapshots__/simple.test.ts.snap b/test/__snapshots__/simple.test.ts.snap index 00d03c66..b960e3c0 100644 --- a/test/__snapshots__/simple.test.ts.snap +++ b/test/__snapshots__/simple.test.ts.snap @@ -285,26 +285,38 @@ exports[`mapping a simple script 1`] = ` { "children": [ { - "location": "index.js:15", + "location": "index.js:10", + "name": "promised", + "static": true, + "type": "function", + }, + { + "location": "index.js:16", "name": "immediatePromise", "static": true, "type": "function", }, { - "location": "index.js:19", + "location": "index.js:20", "name": "throws", "static": true, "type": "function", }, { - "location": "index.js:4", - "name": "foo", + "location": "index.js:29", + "name": "argsInClosure", "static": true, "type": "function", }, { - "location": "index.js:9", - "name": "promised", + "location": "index.js:34", + "name": "incr", + "static": true, + "type": "function", + }, + { + "location": "index.js:5", + "name": "foo", "static": true, "type": "function", }, @@ -355,7 +367,7 @@ exports[`mapping a simple script 1`] = ` "defined_class": "index", "event": "call", "id": 1, - "lineno": 19, + "lineno": 20, "method_id": "throws", "parameters": [], "path": "index.js", @@ -380,7 +392,7 @@ exports[`mapping a simple script 1`] = ` "defined_class": "index", "event": "call", "id": 3, - "lineno": 4, + "lineno": 5, "method_id": "foo", "parameters": [ { @@ -408,7 +420,7 @@ exports[`mapping a simple script 1`] = ` "defined_class": "index", "event": "call", "id": 5, - "lineno": 4, + "lineno": 5, "method_id": "foo", "parameters": [ { @@ -436,7 +448,7 @@ exports[`mapping a simple script 1`] = ` "defined_class": "index", "event": "call", "id": 7, - "lineno": 9, + "lineno": 10, "method_id": "promised", "parameters": [], "path": "index.js", @@ -459,7 +471,7 @@ exports[`mapping a simple script 1`] = ` "defined_class": "index", "event": "call", "id": 9, - "lineno": 9, + "lineno": 10, "method_id": "promised", "parameters": [ { @@ -488,7 +500,7 @@ exports[`mapping a simple script 1`] = ` "defined_class": "index", "event": "call", "id": 11, - "lineno": 15, + "lineno": 16, "method_id": "immediatePromise", "parameters": [], "path": "index.js", @@ -511,7 +523,69 @@ exports[`mapping a simple script 1`] = ` "defined_class": "index", "event": "call", "id": 13, - "lineno": 19, + "lineno": 29, + "method_id": "argsInClosure", + "parameters": [], + "path": "index.js", + "static": true, + "thread_id": 0, + }, + { + "defined_class": "index", + "event": "call", + "id": 14, + "lineno": 34, + "method_id": "incr", + "parameters": [], + "path": "index.js", + "static": true, + "thread_id": 0, + }, + { + "elapsed": 31.337, + "event": "return", + "id": 15, + "parent_id": 14, + "return_value": { + "class": "Number", + "value": "0", + }, + "thread_id": 0, + }, + { + "defined_class": "index", + "event": "call", + "id": 16, + "lineno": 34, + "method_id": "incr", + "parameters": [], + "path": "index.js", + "static": true, + "thread_id": 0, + }, + { + "elapsed": 31.337, + "event": "return", + "id": 17, + "parent_id": 16, + "return_value": { + "class": "Number", + "value": "1", + }, + "thread_id": 0, + }, + { + "elapsed": 31.337, + "event": "return", + "id": 18, + "parent_id": 13, + "thread_id": 0, + }, + { + "defined_class": "index", + "event": "call", + "id": 19, + "lineno": 20, "method_id": "throws", "parameters": [], "path": "index.js", @@ -528,8 +602,8 @@ exports[`mapping a simple script 1`] = ` "object_id": 5, }, ], - "id": 14, - "parent_id": 13, + "id": 20, + "parent_id": 19, "thread_id": 0, }, ], diff --git a/test/simple/index.js b/test/simple/index.js index a8612bd8..d800b329 100644 --- a/test/simple/index.js +++ b/test/simple/index.js @@ -1,4 +1,5 @@ /* eslint-disable @typescript-eslint/no-var-requires */ +const assert = require("assert"); const { setTimeout } = require("timers/promises"); function foo(x) { @@ -25,6 +26,18 @@ function skipped() { console.log("skipped"); } +function argsInClosure() { + // make sure "args" local is not shadowed by the instrumentation + + let args = 0; + + const incr = () => args++; + + incr(); + incr(); + assert.equal(args, 2); +} + try { throws(); console.log(foo(43)); @@ -36,3 +49,4 @@ console.log(foo(42)); promised().then(console.log); promised(false).catch(console.log); immediatePromise().then(console.log); +argsInClosure();