eslint-factory: detect catch-variable aliases in require-error-cause-in-rethrow#43966
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43966 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
This PR improves the require-error-cause-in-rethrow ESLint rule so it detects common catch-variable aliasing patterns and finds references hidden behind additional expression forms, preventing rethrows that drop causal chains.
Changes:
- Track direct
const <alias> = <catchParam>aliases insidecatchblocks and treat them as equivalent to the caught error for reference detection. - Expand message-expression traversal to include
LogicalExpressionandConditionalExpression. - Improve suggestion precision by inserting
{ cause: <identifier actually referenced> }(alias vs catch param) and add RuleTester coverage for these scenarios.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/require-error-cause-in-rethrow.ts | Adds catch-alias collection and expands expression reference scanning; updates reporting/suggestions to use the referenced identifier. |
| eslint-factory/src/rules/require-error-cause-in-rethrow.test.ts | Adds/updates RuleTester cases covering alias detection, logical-expression traversal, and non-tracked alias scenarios. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
| context.report({ | ||
| node, | ||
| messageId: "missingCause", | ||
| data: { catchVar: catchVarName }, | ||
| data: { catchVar: causeIdentifier }, | ||
| suggest: [ | ||
| { | ||
| messageId: "addCause" as const, | ||
| data: { catchVar: catchVarName }, | ||
| data: { catchVar: causeIdentifier }, | ||
| fix(fixer) { |
| function collectCatchAliases(catchBody: TSESTree.BlockStatement, catchVarName: string): Set<string> { | ||
| const aliases = new Set<string>(); | ||
| 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.add(decl.id.name); | ||
| } |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 83/100 — Excellent
📊 Metrics (4 tests)
Note: The test file is | Test | File | Classification | Issues | Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: eslint-factory — catch-variable alias detection
The changes are correct, well-scoped, and conservative. Core logic is sound:
- collectCatchAliases — correctly limits alias collection to top-level const declarations with a direct catchParam initializer. Unrelated initializers and let/reassigned variables are intentionally excluded.
- findTrackedIdentifierReference — cleanly refactored from boolean to name-returning. Correctly covers LogicalExpression and ConditionalExpression, closing the real-world miss described in the PR.
- causeIdentifier propagation — using the actual referenced identifier in error messages and fix suggestions produces accurate autofix output.
- Tests — cover the four key scenarios (unrelated alias valid, let/reassigned not tracked valid, alias+cause valid, alias+logical missing cause invalid with correct fix).
Two non-blocking suggestions left as inline comments.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 110.6 AIC · ⌖ 5.96 AIC · ⊞ 4.8K
| function collectCatchAliases(catchBody: TSESTree.BlockStatement, catchVarName: string): Set<string> { | ||
| const aliases = new Set<string>(); | ||
| for (const statement of catchBody.body) { | ||
| if (statement.type !== AST_NODE_TYPES.VariableDeclaration || statement.kind !== "const") continue; |
There was a problem hiding this comment.
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.
| return findTrackedIdentifierReference(node.right, trackedNames); | ||
| } | ||
| if (node.type === AST_NODE_TYPES.ConditionalExpression) { | ||
| const testMatch = findTrackedIdentifierReference(node.test, trackedNames); |
There was a problem hiding this comment.
Non-blocking: The ConditionalExpression handler traverses all three sub-expressions (test, consequent, alternate). Scanning test means a condition like err ? ... : ... would return err as the causeIdentifier, but the actual error object itself doesn't appear in the message text — only in the guard. This could produce a slightly misleading suggestion in rare patterns. Consider restricting traversal to consequent and alternate only, mirroring the semantics of LogicalExpression (where only the values, not the condition, matter for the message). If the intent is intentionally broad, a comment explaining why test is included would help.\n\n@copilot please address this.
There was a problem hiding this comment.
Verdict: two correctness issues need addressing before merge
The alias-tracking and expression-traversal additions are directionally correct, but the alias resolution is done by name only (no scope analysis), which creates a false positive when an inner-block const shadows the registered alias name. This can cause the rule to insert a wrong { cause: ... } suggestion that references a completely different object.
Blocking issues
-
Alias shadowing → false positive (line ~89):
collectCatchAliasesregisters a name globally for the entire catch scope.findTrackedIdentifierReferencethen matches that name anywhere in the throw message, including identifiers bound by inner-blockconstdeclarations that shadow the alias. The resulting suggestion inserts{ cause: innerErr }wheninnerErris not the caught error. Fix: use ESLint scope analysis to verify binding origin, or at minimum add a test case that proves the boundary is known. -
Nested-block alias → false negative (line ~83): Aliases declared inside
if/for/whileblocks within the catch body are never collected. If intentional (conservative design to avoid the scope-collision problem), document it with a comment so future maintainers don't "fix" it and reintroduce the collision.
Non-blocking observations
trackedNamesis constructed even whencatchVarNameis""(unreachable today due to the guard on line 189, but fragile). A comment or an assertion would make the invariant explicit.UnaryExpressionnodes (e.g.,${typeof err},${!err}) are not traversed byfindTrackedIdentifierReference. This is a false negative edge-case, not a correctness regression (the old code had the same gap).
🔎 Code quality review by PR Code Quality Reviewer · 145.4 AIC · ⌖ 6.75 AIC · ⊞ 5.4K
Comment /review to run again
| if (!decl.init || decl.init.type !== AST_NODE_TYPES.Identifier) continue; | ||
| if (decl.init.name !== catchVarName) continue; | ||
| if (decl.id.name === catchVarName) continue; | ||
| aliases.add(decl.id.name); |
There was a problem hiding this comment.
False positive: inner-block const shadowing a registered alias causes the rule to fire on unrelated variables.
collectCatchAliases scans only the top-level const alias = catchVar declaration and adds the alias name to trackedNames. But findTrackedIdentifierReference then matches that name anywhere in the throw message — including in expressions that reference a different inner-block const that shadows the alias.
💡 Concrete false positive example
try { doSomething(); } catch (deleteError) {
const err = deleteError; // ← 'err' registered as alias of deleteError
if (recoverable) {
const err = new Error('fallback'); // ← inner-block shadow — different variable
throw new Error(`Fallback: ${err}`); // ← rule incorrectly flags this
}
}The rule reports missingCause and suggests { cause: err }, but the err in the inner block is new Error('fallback'), not deleteError. There is no missing cause here.
The correct fix requires scope analysis: use ESLint's scope API to verify that the identifier's resolved binding actually points to the aliased declaration, not a shadowing one. Alternatively, document this gap with an explicit test case.
| if (!isInsideCatchBody(node)) return; | ||
|
|
||
| const catchVarName = frame.varName; | ||
| const trackedNames = new Set<string>([catchVarName, ...frame.aliases]); |
There was a problem hiding this comment.
Empty-string catchVarName is included in trackedNames, so any identifier named "" (impossible in practice, but also any future empty-sentinel path) would match legitimate identifiers — more importantly, every call with an empty-catch frame includes "" in the set, which is a wasted allocation.
More concretely: frame.varName is """" when the catch has no binding (catch {}or destructured). Line 189 already guards withif (!frame || !frame.varName) return;before reaching this line, so the empty-string-in-set is unreachable at runtime. But it is **fragile**: any future reordering of that guard would silently maketrackedNamescontain""`, which would match nothing harmful today but is not obviously correct.
💡 Suggested fix
Build the set only when catchVarName is non-empty, which is already guaranteed by the guard above. A comment noting the invariant would be clearest:
// frame.varName is guaranteed non-empty by the guard above.
const trackedNames = new Set<string>([catchVarName, ...frame.aliases]);Or assert it explicitly:
if (!catchVarName) return; // defensive guard (already checked via frame.varName)This documents the invariant and makes future refactors safe.
| function collectCatchAliases(catchBody: TSESTree.BlockStatement, catchVarName: string): Set<string> { | ||
| const aliases = new Set<string>(); | ||
| for (const statement of catchBody.body) { | ||
| if (statement.type !== AST_NODE_TYPES.VariableDeclaration || statement.kind !== "const") continue; |
There was a problem hiding this comment.
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.
|
@copilot run pr-finisher skill |
|
@copilot please run the
|
… verification
- Split `{{catchVar}}` into `{{catchParam}}` (catch clause name) and `{{refName}}`
(the referenced identifier), so diagnostics like "inside catch (deleteError)"
remain accurate when the reference is an alias `err`.
- Track alias `VariableDeclarator` nodes (Map instead of Set) and store
`catchNode: CatchClause` in each frame so bindings can be verified.
- `verifyIdentifier` walks up the scope chain via `sourceCode.getScope()` and
confirms the resolved `Variable.defs` points to the recorded catch-clause param
or alias declarator, preventing false positives from shadowed names in inner blocks.
- Update tests: use `{ catchParam, refName }` data keys; add valid case for
shadowed alias in nested block that must not be flagged.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Both review threads are addressed in the latest commit:
A test case for the shadowed alias scenario was added. All 159 tests pass. |
pr-finisher execution complete:
Actions taken: Validated all local checks, confirmed review fixes from df8761c are in place. Hand-off: CI must be re-triggered by a maintainer (close/reopen PR, workflow_dispatch, or push from maintainer) before merge. All local validation is green and ready for CI confirmation. |
🤖 PR Triage · Run §28868091227
Score breakdown: Impact 40 · Urgency 25 · Quality 15 Rationale: Fixes Labels:
|
require-error-cause-in-rethrowonly tracked the catch parameter identifier, so common normalization (const err = caughtError) bypassed detection and allowed rethrows that dropped causal chains. It also missed references hidden behind logical/conditional expressions, which masked real cases such as thecreate_pull_request.cjspattern.Catch alias tracking (conservative)
const <alias> = <catchParam>.Reference traversal expansion
LogicalExpressionandConditionalExpressionin addition to existing call/template/member/binary traversal.${message || String(err)}where the caught value is referenced indirectly.Cause suggestion precision
errvs catch param) so suggested fixes insert the correct{ cause: <id> }.Rule coverage updates
{ cause: err },