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
getappmap · Aug 2, 2024 · 05af269 · 05af269
1 parent 6f9be83
commit 05af269
Show file tree

Hide file tree

Showing 4 changed files with 116 additions and 28 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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();