Skip to content

fix(eslint-factory): treat try/finally without catch as unprotected in isInsideTryBlock #44256

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 2 commits into from
Jul 8, 2026
+77 −2
Merged

fix(eslint-factory): treat try/finally without catch as unprotected in isInsideTryBlock #44256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d49661
Initial plan
Copilot Jul 8, 2026
89f074c
fix: require catch handler in isInsideTryBlock for fs-sync and json-p…
Copilot Jul 8, 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
45 changes: 45 additions & 0 deletions eslint-factory/src/rules/require-fs-sync-try-catch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,51 @@ describe("require-fs-sync-try-catch", () => {
});
});

it("valid: try/catch/finally is treated as protective", () => {
cjsRuleTester.run("require-fs-sync-try-catch", requireFsSyncTryCatchRule, {
valid: [`try { fs.writeFileSync(path, data); } catch (e) {} finally { cleanup(); }`, `try { fs.readFileSync(path, "utf8"); } catch (e) { handleErr(e); } finally { releaseLock(); }`],
invalid: [],
});
});

it("invalid: try/finally without catch is not protective", () => {
cjsRuleTester.run("require-fs-sync-try-catch", requireFsSyncTryCatchRule, {
valid: [],
invalid: [
{
code: `try { fs.writeFileSync(path, data); } finally { cleanup(); }`,
errors: [
{
messageId: "requireTryCatch",
data: { method: "writeFileSync", arg: "path" },
suggestions: [
{
messageId: "wrapInTryCatch",
output: `try { try {\n fs.writeFileSync(path, data);\n} catch (err) {\n // TODO: handle I/O failure for this fs.writeFileSync call.\n throw new Error(\n "fs.writeFileSync failed: " + (err instanceof Error ? err.message : String(err)),\n { cause: err },\n );\n} } finally { cleanup(); }`,
},
],
},
],
},
{
code: `try { fs.readFileSync(path, "utf8"); } finally { releaseLock(); }`,
errors: [
{
messageId: "requireTryCatch",
data: { method: "readFileSync", arg: "path" },
suggestions: [
{
messageId: "wrapInTryCatch",
output: `try { try {\n fs.readFileSync(path, "utf8");\n} catch (err) {\n // TODO: handle I/O failure for this fs.readFileSync call.\n throw new Error(\n "fs.readFileSync failed: " + (err instanceof Error ? err.message : String(err)),\n { cause: err },\n );\n} } finally { releaseLock(); }`,
},
],
},
],
},
],
});
});

it("valid: synchronous callbacks inside try block are protected", () => {
cjsRuleTester.run("require-fs-sync-try-catch", requireFsSyncTryCatchRule, {
valid: [
Expand Down
2 changes: 1 addition & 1 deletion eslint-factory/src/rules/require-fs-sync-try-catch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export const requireFsSyncTryCatchRule = createRule({
crossedDeferredBoundary = true;
}

if (ancestor.type === "TryStatement" && !crossedDeferredBoundary) {
if (ancestor.type === "TryStatement" && !crossedDeferredBoundary && ancestor.handler != null) {

@github-actions github-actions Bot Jul 8, 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.

[/diagnosing-bugs] The handler != null guard is correct, but the function's existing comment doesn't document the catch-clause contract. A future reader seeing isInsideTryBlock return false for a try/finally block may be confused without this context.

💡 Suggested comment addition 
// Walk from innermost ancestor outward. Stop marking a try as protective once a deferred
// callback boundary has been crossed (the callback fires after the try has returned).
// NOTE: only TryStatements with a catch clause (handler != null) are treated as protective;
// a bare try/finally does not catch exceptions — the throw continues propagating after finally.
let crossedDeferredBoundary = false;

This makes the protection contract self-documenting and prevents future contributors from widening the check back to handler == null cases.

@copilot please address this.

const block = ancestor.block;
if (node.range != null && block.range != null && node.range[0] >= block.range[0] && node.range[1] <= block.range[1]) {
return true;
Expand Down
30 changes: 30 additions & 0 deletions eslint-factory/src/rules/require-json-parse-try-catch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,36 @@ describe("require-json-parse-try-catch", () => {
});
});

it("valid: try/catch/finally is treated as protective", () => {
cjsRuleTester.run("require-json-parse-try-catch", requireJsonParseTryCatchRule, {
valid: [`try { const x = JSON.parse(str); } catch (e) {} finally { cleanup(); }`, `try { JSON.parse(str); } catch (e) { handleErr(e); } finally { release(); }`],
invalid: [],
});
});

it("invalid: try/finally without catch is not protective", () => {
cjsRuleTester.run("require-json-parse-try-catch", requireJsonParseTryCatchRule, {
valid: [],
invalid: [
{
code: `try { const x = JSON.parse(str); } finally { cleanup(); }`,
errors: [
{
messageId: "requireTryCatch",
data: { arg: "str" },
suggestions: [
{
messageId: "useHelper",
output: `try { try {\n const x = JSON.parse(str);\n} catch (err) {\n // TODO: handle parse failure for this code path.\n throw new Error(\n "Failed to parse JSON: " + (err instanceof Error ? err.message : String(err)),\n { cause: err },\n );\n} } finally { cleanup(); }`,
},
],
},
],
},
],
});
});

it("valid: synchronous callbacks inside try block are protected", () => {
cjsRuleTester.run("require-json-parse-try-catch", requireJsonParseTryCatchRule, {
valid: [
Expand Down
2 changes: 1 addition & 1 deletion eslint-factory/src/rules/require-json-parse-try-catch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export const requireJsonParseTryCatchRule = createRule({
crossedDeferredBoundary = true;
}

if (ancestor.type === "TryStatement" && !crossedDeferredBoundary) {
if (ancestor.type === "TryStatement" && !crossedDeferredBoundary && ancestor.handler != null) {

@github-actions github-actions Bot Jul 8, 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.

[/diagnosing-bugs] Same handler != null fix as the fs rule — identical concern about the missing comment. Consider adding the same note here to keep both implementations self-consistent in their documentation.

💡 Suggested comment addition 
// Walk from innermost ancestor outward. If we cross a deferred function boundary
// (e.g., a .then/.on/setTimeout callback), a try statement further out does NOT
// protect the node — the callback runs after the try has already returned.
// NOTE: only TryStatements with a catch clause (handler != null) are treated as protective;
// a bare try/finally does not catch exceptions — the throw continues propagating after finally.
let crossedDeferredBoundary = false;

@copilot please address this.

const block = ancestor.block;
if (node.range[0] >= block.range[0] && node.range[1] <= block.range[1]) {
return true;
Expand Down
Loading