Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 5 commits into from
Jul 7, 2026
+125 −33
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8ab86db
Initial plan
Copilot Jul 7, 2026
b86b396
Fix catch-alias detection in require-error-cause-in-rethrow
Copilot Jul 7, 2026
cbd30f1
Harden catch alias collection in rethrow rule
Copilot Jul 7, 2026
12efdd5
Merge branch 'main' into copilot/eslint-factory-fix-require-error-cause
github-actions[bot] Jul 7, 2026
df8761c
fix: split catchVar message placeholder, add scope-safe alias binding…
Copilot Jul 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 30 additions & 6 deletions eslint-factory/src/rules/require-error-cause-in-rethrow.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: [],
});
Expand All @@ -55,7 +63,7 @@ describe("require-error-cause-in-rethrow", () => {
errors: [
{
messageId: "missingCause",
data: { catchVar: "err" },
data: { catchParam: "err", refName: "err" },
suggestions: [
{
messageId: "addCause",
Expand All @@ -70,7 +78,7 @@ describe("require-error-cause-in-rethrow", () => {
errors: [
{
messageId: "missingCause",
data: { catchVar: "err" },
data: { catchParam: "err", refName: "err" },
suggestions: [
{
messageId: "addCause",
Expand All @@ -85,7 +93,7 @@ describe("require-error-cause-in-rethrow", () => {
errors: [
{
messageId: "missingCause",
data: { catchVar: "error" },
data: { catchParam: "error", refName: "error" },
suggestions: [
{
messageId: "addCause",
Expand All @@ -101,7 +109,7 @@ describe("require-error-cause-in-rethrow", () => {
errors: [
{
messageId: "missingCause",
data: { catchVar: "err" },
data: { catchParam: "err", refName: "err" },
suggestions: [
{
messageId: "addCause",
Expand All @@ -117,7 +125,7 @@ describe("require-error-cause-in-rethrow", () => {
errors: [
{
messageId: "missingCause",
data: { catchVar: "err" },
data: { catchParam: "err", refName: "err" },
suggestions: [
{
messageId: "addCause",
Expand All @@ -133,7 +141,7 @@ describe("require-error-cause-in-rethrow", () => {
errors: [
{
messageId: "missingCause",
data: { catchVar: "err" },
data: { catchParam: "err", refName: "err" },
suggestions: [
{
messageId: "addCause",
Expand All @@ -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 }); }`,
},
],
},
],
},
],
});
});
Expand Down
122 changes: 95 additions & 27 deletions eslint-factory/src/rules/require-error-cause-in-rethrow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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>;
}

/**
Expand All @@ -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;

@github-actions github-actions Bot Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: collectCatchAliases only walks top-level const declarations — aliases declared in nested blocks (if, nested try, for) are silently ignored. This conservative boundary is correct, but a short comment explaining the intent would prevent future contributors from inadvertently widening the scope. Consider:\n\nts\n// Only tracks const aliases at the top level of the catch block;\n// aliases inside nested blocks are intentionally not followed.\n\n\n@copilot please address this.

@github-actions github-actions Bot Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

collectCatchAliases only scans top-level const declarations — aliases declared inside nested blocks (if, for, try) inside the catch body are silently missed, creating a false negative.

💡 False negative example 
try { doSomething(); } catch (deleteError) {
  if (condition) {
    const err = deleteError; // ← inside nested block, NOT at top-level
    throw new Error(`Failed: ${err}`); // ← rule does NOT flag this, but it should
  }
}

The check for (const statement of catchBody.body) iterates only the top-level statements of the BlockStatement. A const alias inside an if/for/while block body is never visited.

Whether this is intentional or not is worth documenting. If intentional (conservative design), a code comment should say so. If not intentional, the scan needs to recurse into nested blocks — though that significantly broadens what "alias" means and re-introduces the scope-collision problem noted in the other comment.

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;
}

/**
Expand Down Expand Up @@ -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: [],
Expand Down Expand Up @@ -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"() {
Expand All @@ -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) {
Expand All @@ -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) {
Comment on lines 241 to 249
// If there's already a second arg, replace it with an object that adds cause.
if (args.length >= 2) {
Expand All @@ -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} }`);
},
},
],
Expand Down
Loading