eslint-factory: detect catch-variable aliases in require-error-cause-in-rethrow (#43966)

Copilot · web-flow · commit 0121fe66e84b · 2026-07-07T07:18:49.000-07:00
diff --git a/eslint-factory/src/rules/require-error-cause-in-rethrow.test.ts b/eslint-factory/src/rules/require-error-cause-in-rethrow.test.ts
@@ -41,6 +41,14 @@ describe("require-error-cause-in-rethrow", () => {
         `try { doSomething(); } catch (err) { const e = new Error(\`msg: \${getErrorMessage(err)}\`); log(e); }`,
         // Existing cause property with wrapped expression should not be flagged.
         `try { doSomething(); } catch (err) { throw new Error("Failed: " + getErrorMessage(err), { cause: new Error(err.message), code: 500 }); }`,
+        // Alias initialized from unrelated value should not be treated as catch alias.
+        `try { doSomething(); } catch (deleteError) { const err = getFallbackError(); throw new Error(\`Failed to delete: \${String(err)}\`); }`,
+        // Reassigned alias is intentionally not tracked (too complex to follow safely).
+        `try { doSomething(); } catch (deleteError) { let err = deleteError; err = normalizeError(err); throw new Error(\`Failed to delete: \${String(err)}\`); }`,
+        // Alias + cause should pass.
+        `try { doSomething(); } catch (deleteError) { const err = deleteError; throw new Error(\`Failed to delete existing remote branch: \${message || String(err)}\`, { cause: err }); }`,
+        // Alias name shadowed in nested block — inner `err` is unrelated, should not flag.
+        `try { doSomething(); } catch (deleteError) { const err = deleteError; { const err = getFallbackError(); throw new Error(String(err)); } }`,
       ],
       invalid: [],
     });
@@ -55,7 +63,7 @@ describe("require-error-cause-in-rethrow", () => {
           errors: [
             {
               messageId: "missingCause",
-              data: { catchVar: "err" },
+              data: { catchParam: "err", refName: "err" },
               suggestions: [
                 {
                   messageId: "addCause",
@@ -70,7 +78,7 @@ describe("require-error-cause-in-rethrow", () => {
           errors: [
             {
               messageId: "missingCause",
-              data: { catchVar: "err" },
+              data: { catchParam: "err", refName: "err" },
               suggestions: [
                 {
                   messageId: "addCause",
@@ -85,7 +93,7 @@ describe("require-error-cause-in-rethrow", () => {
           errors: [
             {
               messageId: "missingCause",
-              data: { catchVar: "error" },
+              data: { catchParam: "error", refName: "error" },
               suggestions: [
                 {
                   messageId: "addCause",
@@ -101,7 +109,7 @@ describe("require-error-cause-in-rethrow", () => {
           errors: [
             {
               messageId: "missingCause",
-              data: { catchVar: "err" },
+              data: { catchParam: "err", refName: "err" },
               suggestions: [
                 {
                   messageId: "addCause",
@@ -117,7 +125,7 @@ describe("require-error-cause-in-rethrow", () => {
           errors: [
             {
               messageId: "missingCause",
-              data: { catchVar: "err" },
+              data: { catchParam: "err", refName: "err" },
               suggestions: [
                 {
                   messageId: "addCause",
@@ -133,7 +141,7 @@ describe("require-error-cause-in-rethrow", () => {
           errors: [
             {
               messageId: "missingCause",
-              data: { catchVar: "err" },
+              data: { catchParam: "err", refName: "err" },
               suggestions: [
                 {
                   messageId: "addCause",
@@ -143,6 +151,22 @@ describe("require-error-cause-in-rethrow", () => {
             },
           ],
         },
+        {
+          // Aliased catch var + logical expression branch should be detected.
+          code: `try { doSomething(); } catch (deleteError) { const err = deleteError; throw new Error(\`Failed to delete existing remote branch: \${message || String(err)}\`); }`,
+          errors: [
+            {
+              messageId: "missingCause",
+              data: { catchParam: "deleteError", refName: "err" },
+              suggestions: [
+                {
+                  messageId: "addCause",
+                  output: `try { doSomething(); } catch (deleteError) { const err = deleteError; throw new Error(` + `\`Failed to delete existing remote branch: \${message || String(err)}\`, { cause: err }); }`,
+                },
+              ],
+            },
+          ],
+        },
       ],
     });
   });
diff --git a/eslint-factory/src/rules/require-error-cause-in-rethrow.ts b/eslint-factory/src/rules/require-error-cause-in-rethrow.ts
@@ -1,9 +1,11 @@
-import { AST_NODE_TYPES, ESLintUtils, TSESTree } from "@typescript-eslint/utils";
+import { AST_NODE_TYPES, ESLintUtils, TSESLint, TSESTree } from "@typescript-eslint/utils";
 
 const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`);
 
 interface CatchFrame {
   varName: string;
+  catchNode: TSESTree.CatchClause | null;
+  aliases: Map<string, TSESTree.VariableDeclarator>;
 }
 
 /**
@@ -20,39 +22,83 @@ function getErrorMessageArg(node: TSESTree.CallExpression): string | null {
 }
 
 /**
- * Returns true when the expression tree references the given identifier name.
- * Used to detect whether the catch variable appears somewhere in the Error
- * message (directly or via getErrorMessage(catchVar)).
+ * Returns the first identifier name from `trackedNames` referenced in `node`,
+ * or null if none are found. Used to detect whether the Error message
+ * references the caught variable (or a direct alias of it).
+ * `verifyIdentifier` is called on each name-matched identifier node to confirm
+ * the binding resolves to the expected catch variable or alias (scope safety).
  */
-function expressionReferencesCatchVar(node: TSESTree.Expression, varName: string): boolean {
-  if (node.type === AST_NODE_TYPES.Identifier && node.name === varName) return true;
+function findTrackedIdentifierReference(node: TSESTree.Expression, trackedNames: ReadonlySet<string>, verifyIdentifier: (name: string, identNode: TSESTree.Identifier) => boolean): string | null {
+  if (node.type === AST_NODE_TYPES.Identifier && trackedNames.has(node.name)) {
+    return verifyIdentifier(node.name, node) ? node.name : null;
+  }
   if (node.type === AST_NODE_TYPES.CallExpression) {
     const gem = getErrorMessageArg(node);
-    if (gem === varName) return true;
+    if (gem && trackedNames.has(gem)) {
+      // getErrorMessageArg returns the name; verify via the actual argument identifier node
+      const firstArg = node.arguments[0];
+      if (firstArg && firstArg.type === AST_NODE_TYPES.Identifier && verifyIdentifier(gem, firstArg)) return gem;
+    }
     // Recurse into all arguments
     for (const arg of node.arguments) {
-      if (arg.type !== "SpreadElement" && expressionReferencesCatchVar(arg, varName)) return true;
+      if (arg.type === "SpreadElement") continue;
+      const match = findTrackedIdentifierReference(arg, trackedNames, verifyIdentifier);
+      if (match) return match;
     }
   }
   if (node.type === AST_NODE_TYPES.TemplateLiteral) {
     for (const expr of node.expressions) {
-      if (expressionReferencesCatchVar(expr, varName)) return true;
+      const match = findTrackedIdentifierReference(expr, trackedNames, verifyIdentifier);
+      if (match) return match;
     }
   }
   if (node.type === AST_NODE_TYPES.BinaryExpression) {
     const left = node.left;
     const right = node.right;
-    const leftResult = left.type !== AST_NODE_TYPES.PrivateIdentifier && expressionReferencesCatchVar(left, varName);
-    return leftResult || expressionReferencesCatchVar(right, varName);
+    if (left.type !== AST_NODE_TYPES.PrivateIdentifier) {
+      const leftMatch = findTrackedIdentifierReference(left, trackedNames, verifyIdentifier);
+      if (leftMatch) return leftMatch;
+    }
+    return findTrackedIdentifierReference(right, trackedNames, verifyIdentifier);
+  }
+  if (node.type === AST_NODE_TYPES.LogicalExpression) {
+    const leftMatch = findTrackedIdentifierReference(node.left, trackedNames, verifyIdentifier);
+    if (leftMatch) return leftMatch;
+    return findTrackedIdentifierReference(node.right, trackedNames, verifyIdentifier);
+  }
+  if (node.type === AST_NODE_TYPES.ConditionalExpression) {
+    const testMatch = findTrackedIdentifierReference(node.test, trackedNames, verifyIdentifier);
+    if (testMatch) return testMatch;
+    const consequentMatch = findTrackedIdentifierReference(node.consequent, trackedNames, verifyIdentifier);
+    if (consequentMatch) return consequentMatch;
+    return findTrackedIdentifierReference(node.alternate, trackedNames, verifyIdentifier);
   }
   if (node.type === AST_NODE_TYPES.MemberExpression) {
-    if (node.object.type !== AST_NODE_TYPES.Super && expressionReferencesCatchVar(node.object, varName)) return true;
+    if (node.object.type !== AST_NODE_TYPES.Super) {
+      const objectMatch = findTrackedIdentifierReference(node.object, trackedNames, verifyIdentifier);
+      if (objectMatch) return objectMatch;
+    }
     if (node.computed) {
-      return expressionReferencesCatchVar(node.property as TSESTree.Expression, varName);
+      return findTrackedIdentifierReference(node.property as TSESTree.Expression, trackedNames, verifyIdentifier);
     }
-    return false;
+    return null;
   }
-  return false;
+  return null;
+}
+
+function collectCatchAliases(catchBody: TSESTree.BlockStatement, catchVarName: string): Map<string, TSESTree.VariableDeclarator> {
+  const aliases = new Map<string, TSESTree.VariableDeclarator>();
+  for (const statement of catchBody.body) {
+    if (statement.type !== AST_NODE_TYPES.VariableDeclaration || statement.kind !== "const") continue;
+    for (const decl of statement.declarations) {
+      if (decl.id.type !== AST_NODE_TYPES.Identifier) continue;
+      if (!decl.init || decl.init.type !== AST_NODE_TYPES.Identifier) continue;
+      if (decl.init.name !== catchVarName) continue;
+      if (decl.id.name === catchVarName) continue;
+      aliases.set(decl.id.name, decl);
+    }
+  }
+  return aliases;
 }
 
 /**
@@ -86,8 +132,8 @@ export const requireErrorCauseInRethrowRule = createRule({
     },
     schema: [],
     messages: {
-      missingCause: "`new Error(...)` inside catch ({{catchVar}}) references {{catchVar}} but omits `{ cause: {{catchVar}} }` — the original stack trace will be lost. Add `{ cause: {{catchVar}} }` as the second argument.",
-      addCause: "Add `{ cause: {{catchVar}} }` as the second argument to preserve the original error chain.",
+      missingCause: "`new Error(...)` inside catch ({{catchParam}}) references {{refName}} but omits `{ cause: {{refName}} }` — the original stack trace will be lost. Add `{ cause: {{refName}} }` as the second argument.",
+      addCause: "Add `{ cause: {{refName}} }` as the second argument to preserve the original error chain.",
     },
   },
   defaultOptions: [],
@@ -131,10 +177,10 @@ export const requireErrorCauseInRethrowRule = createRule({
         const param = node.param;
         if (!param || param.type !== AST_NODE_TYPES.Identifier) {
           // Bare catch {} or destructured — push empty sentinel so CatchClause:exit still pops
-          catchStack.push({ varName: "" });
+          catchStack.push({ varName: "", catchNode: null, aliases: new Map() });
           return;
         }
-        catchStack.push({ varName: param.name });
+        catchStack.push({ varName: param.name, catchNode: node, aliases: collectCatchAliases(node.body, param.name) });
       },
 
       "CatchClause:exit"() {
@@ -149,18 +195,40 @@ export const requireErrorCauseInRethrowRule = createRule({
         if (callee.type !== AST_NODE_TYPES.Identifier || callee.name !== "Error") return;
 
         const frame = innermostCatch();
-        if (!frame || !frame.varName) return;
+        if (!frame || !frame.varName || !frame.catchNode) return;
         if (!isInsideCatchBody(node)) return;
 
         const catchVarName = frame.varName;
+        const catchNode = frame.catchNode;
+        const trackedNames = new Set<string>([catchVarName, ...frame.aliases.keys()]);
         const args = node.arguments;
 
+        // Build a scope-aware verifier: confirm the identifier actually resolves to the
+        // catch parameter or a recorded alias, not a shadowed variable in a nested block.
+        const verifyIdentifier = (name: string, identNode: TSESTree.Identifier): boolean => {
+          let currentScope: TSESLint.Scope.Scope | null = sourceCode.getScope(identNode);
+          while (currentScope !== null) {
+            const variable = currentScope.set.get(name);
+            if (variable !== undefined) {
+              if (name === catchVarName) {
+                return variable.defs.some(def => def.type === TSESLint.Scope.DefinitionType.CatchClause && def.node === catchNode);
+              }
+              const aliasDeclarator = frame.aliases.get(name);
+              if (aliasDeclarator === undefined) return false;
+              return variable.defs.some(def => def.type === TSESLint.Scope.DefinitionType.Variable && def.node === aliasDeclarator);
+            }
+            currentScope = currentScope.upper;
+          }
+          return false;
+        };
+
         // Must have at least a message argument that references the catch variable.
         if (args.length === 0) return;
         const msgArg = args[0];
         if (msgArg.type === "SpreadElement") return;
 
-        if (!expressionReferencesCatchVar(msgArg, catchVarName)) return;
+        const causeIdentifier = findTrackedIdentifierReference(msgArg, trackedNames, verifyIdentifier);
+        if (!causeIdentifier) return;
 
         // If a second argument exists, check that it contains { cause: catchVar }.
         if (args.length >= 2) {
@@ -173,11 +241,11 @@ export const requireErrorCauseInRethrowRule = createRule({
         context.report({
           node,
           messageId: "missingCause",
-          data: { catchVar: catchVarName },
+          data: { catchParam: catchVarName, refName: causeIdentifier },
           suggest: [
             {
               messageId: "addCause" as const,
-              data: { catchVar: catchVarName },
+              data: { refName: causeIdentifier },
               fix(fixer) {
                 // If there's already a second arg, replace it with an object that adds cause.
                 if (args.length >= 2) {
@@ -186,17 +254,17 @@ export const requireErrorCauseInRethrowRule = createRule({
                   if (secondArg.type === AST_NODE_TYPES.ObjectExpression) {
                     // Add cause property to the existing object
                     if (secondArg.properties.length === 0) {
-                      return fixer.replaceText(secondArg, `{ cause: ${catchVarName} }`);
+                      return fixer.replaceText(secondArg, `{ cause: ${causeIdentifier} }`);
                     }
                     const firstProp = secondArg.properties[0];
-                    return fixer.insertTextBefore(firstProp, `cause: ${catchVarName}, `);
+                    return fixer.insertTextBefore(firstProp, `cause: ${causeIdentifier}, `);
                   }
                   return null;
                 }
-                // No second argument — append `, { cause: catchVar }` before closing paren
+                // No second argument — append `, { cause: causeIdentifier }` before closing paren
                 const lastArg = args[args.length - 1];
                 if (!lastArg || lastArg.type === "SpreadElement") return null;
-                return fixer.insertTextAfter(lastArg, `, { cause: ${catchVarName} }`);
+                return fixer.insertTextAfter(lastArg, `, { cause: ${causeIdentifier} }`);
               },
             },
           ],
