eslint-factory: extend no-empty-catch-block to empty Promise .catch handlers - #52979
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
.catch handlers
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed 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 does not have the implementation label and has 0 new lines of code in business logic directories.
|
There was a problem hiding this comment.
Pull request overview
Extends the ESLint rule to detect undocumented empty Promise rejection handlers.
Changes:
- Adds
.catch(() => {})and function-handler detection. - Shares intentional-ignore comment handling across catch forms.
- Adds valid and invalid rule tests.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/no-empty-catch-block.ts |
Implements Promise catch detection and comment exemptions. |
eslint-factory/src/rules/no-empty-catch-block.test.ts |
Covers Promise handler diagnostics and exemptions. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
| const ancestors = sourceCode.getAncestors(node); | ||
| for (let i = ancestors.length - 1; i >= 0; i -= 1) { | ||
| const ancestor = ancestors[i]; | ||
| if (ancestor.type === AST_NODE_TYPES.Program) break; | ||
| if (ancestor.type.endsWith("Statement") && hasAdjacentIntentionalIgnoreComment(ancestor)) { | ||
| return true; | ||
| } | ||
| } |
| `run().catch((err) => { core.warning(getErrorMessage(err)); });`, | ||
| `run().catch(function (err) { core.warning(getErrorMessage(err)); });`, | ||
| `run().catch(() => { /* best-effort cleanup */ });`, | ||
| `// Non-fatal: errors are silently swallowed.\nif (require.main === module) {\n run().catch(() => {});\n}`, |
| messageId: "noEmptyCatch", | ||
| }); | ||
| }, | ||
| "CallExpression[callee.type='MemberExpression'][callee.property.type='Identifier'][callee.property.name='catch']"(node: TSESTree.CallExpression) { |
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This broadens the rule usefully, but the new ancestor-comment exemption makes it too easy for unrelated enclosing comments to silence genuinely undocumented empty handlers.
Blocking theme
- The rule now treats comments on enclosing statements as intent for nested empty
catch/.catchhandlers, which creates false negatives and weakens the lint rule's core guarantee.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 3.82 AIC · ⌖ 6.75 AIC · ⊞ 6.9K
Comment /review to run again
| if (hasAdjacentIntentionalIgnoreComment(block)) return true; | ||
| if (hasAdjacentIntentionalIgnoreComment(node)) return true; | ||
|
|
||
| const ancestors = sourceCode.getAncestors(node); |
There was a problem hiding this comment.
This comment-ancestor walk is too broad: a generic // Non-fatal... comment on any enclosing statement now suppresses diagnostics for nested empty catch {} blocks and .catch(() => {}), so the rule can be bypassed accidentally by unrelated control-flow comments.
💡 Why this is a real correctness hole
hasIntentionalIgnoreComment() climbs every ancestor whose type ends with Statement and accepts any adjacent comment there. That means code like this now passes even though the empty handler itself is undocumented:
// Non-fatal: continue startup if optional step fails
if (ready) {
try {
risky();
} catch {}
}The comment is about the if statement, not necessarily the empty catch body, but the rule treats it as authorization to swallow any nested error. That weakens the lint signal exactly where it is supposed to be strict.
Please tighten the exemption to comments attached to the handler itself (inside the block, immediately before the catch clause / callback, or another deliberately narrow scope), and add a regression test proving an enclosing statement comment does not exempt a nested empty handler.
There was a problem hiding this comment.
The extension looks correct and well-tested. The new AST selector accurately targets empty .catch handlers, the comment-based escape hatch works for both inline and ancestor-level comments, and all new test cases align with the implementation. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 12.5 AIC · ⌖ 7 AIC · ⊞ 5.6K
There was a problem hiding this comment.
Ponytail review (over-engineering only).
net: -6 lines possible.
Generated by ✂️ Ponytail Reviewer for #52979 · auto · 14.4 AIC · ⌖ 3.14 AIC · ⊞ 7.2K
Comment /ponytail to run again
| context.report({ | ||
| node: node.body, | ||
| messageId: "noEmptyCatch", | ||
| }); |
There was a problem hiding this comment.
L64-71: yagni: unbounded ancestor walk up to Program just to support one test case (comment above wrapping if). Check only the immediate parent statement instead of looping through all ancestors.
Test Quality Sentinel 🧪 — PR #52979Test Quality Score: ✅ 100/100 ExcellentSummaryThis PR extends the Key FindingsTest Coverage: 6 new test cases (4 valid, 2 invalid)
Test Inflation Ratio: 0.27:1 (12 test lines / 44 prod lines)
Assertion Quality: All tests use RuleTester's messageId assertions to verify rule acceptance/rejection Detailed Test Classification
RationaleThe new test cases directly verify the feature requirement: the rule now rejects empty Promise ✅ Recommendation: APPROVE
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on two correctness gaps and one test coverage gap.
📋 Key Themes & Highlights
Key Themes
- False positive risk (correctness): The new
CallExpressionselector is not scoped to Promise-returning calls. Any object with a.catchmethod (cache, event emitter, test double) will trigger the rule incorrectly. - Ancestor-walk scope creep (correctness): The intent-comment ancestor walk is unbounded — a comment above a distant ancestor statement can exempt an unrelated inner catch block. Should be capped at a defined ancestor depth.
- Missing cross-path test coverage (test coverage): The new regex vocabulary (
non-fatal,silently swallow*) was added to the shared helper but is only tested in the Promise.catchpath.try/catchvalid-case tests for the new terms are absent.
Positive Highlights
- ✅ Clean extraction of comment-handling logic into shared helpers — good deep-module design
- ✅ Solid invalid-case tests for the new Promise path
- ✅ Backward-compatible: no changes to existing try/catch error messages or rule ID
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 34.9 AIC · ⌖ 7.71 AIC · ⊞ 7.7K
Comment /matt to run again
| }); | ||
| }, | ||
| "CallExpression[callee.type='MemberExpression'][callee.property.type='Identifier'][callee.property.name='catch']"(node: TSESTree.CallExpression) { | ||
| const [handler] = node.arguments; |
There was a problem hiding this comment.
[/tdd] The selector matches any .catch(handler) call, not just on Promises. Objects with a custom catch method (e.g. a Cache or test double) will produce false positives.
💡 Suggested guard
Document the known limitation, and add a valid fixture covering a non-Promise object:
// valid – non-Promise object with its own .catch method
`const myObj = { catch: (fn) => fn() }; myObj.catch(() => {})`,If that should be flagged, that’s fine — but the rule docs should say so. If not, narrow the selector or add an explicit exclusion.
@copilot please address this.
| if (ancestor.type.endsWith("Statement") && hasAdjacentIntentionalIgnoreComment(ancestor)) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
[/codebase-design] hasIntentionalIgnoreComment walks ancestors and breaks on any *Statement node with a preceding intent comment, but the loop stops at Program. A chained call deep inside a function body could legitimately pick up an intent comment on a sibling IfStatement that was meant for a different block. The proximity guard is only applied to the adjacent line, so a comment two lines above an unrelated sibling would still trigger the exemption.
💡 Consider a tighter ancestor scope
Consider limiting the ancestor walk to the nearest enclosing ExpressionStatement (the direct parent of the call) rather than any ancestor *Statement. That matches the documented exemption pattern:
// Non-fatal: errors are silently swallowed.
if (require.main === module) {
run().catch(() => {}); // <-- intent comment is on the IfStatement
}Walking up to the IfStatement is deliberate here, but it should be bounded so that an intent comment on a distant ancestor ForStatement is not accidentally honoured.
@copilot please address this.
| @@ -27,14 +56,26 @@ export const noEmptyCatchBlockRule = createRule({ | |||
| // An explicit intentional-ignore comment inside the otherwise empty | |||
| // braces documents intent, e.g.: | |||
There was a problem hiding this comment.
[/tdd] The original CatchClause handler now delegates to hasIntentionalIgnoreComment, but the old test cases for try/catch only covered /* intentional */ and /* best-effort */ patterns. The expanded regex now also matches non-fatal and silently swallow* — there are no tests asserting that those new patterns also exempt a try/catch block.
💡 Missing test cases to add
// valid – new vocabulary in try/catch context
`try { risky(); } catch { /* non-fatal: optional step */ }`,
`try { risky(); } catch { /* errors are silently swallowed */ }`,Without these, a future regex change could silently break the try/catch path.
@copilot please address this.
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if needed, address the remaining maintainer-facing follow-up, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
|
🎉 This pull request is included in a new release. Release: |
no-empty-catch-blockpreviously only analyzedCatchClausenodes, so empty Promise terminal handlers like.catch(() => {})could silently swallow rejections with no diagnostics. This closes that gap while preserving explicit intent-based exemptions for documented non-fatal swallowing patterns.Rule scope expansion
CallExpressionselector for.catch(...)member calls..catch(() => {}).catch(function () {})Shared intent-comment exemption logic
try/catchand Promise.catch.non-fatalandsilently swallowed(in addition to existing intentional/best-effort forms), so documented entrypoint swallow sites remain valid.Coverage updates
invalidcases for undocumented empty Promise catch handlers.validcases for: