-
Notifications
You must be signed in to change notification settings - Fork 475
[eslint-miner] eslint: add require-fs-io-try-catch rule (statSync / readdirSync / copyFileSync / unlinkSync / renameSync) #47259
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.
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
Changes from all commits
86d99ed
3a1ac54
4d10906
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| import { RuleTester } from "eslint"; | ||
| import { describe, it } from "vitest"; | ||
| import { requireFsIoTryCatchRule } from "./require-fs-io-try-catch"; | ||
|
|
||
| const cjsRuleTester = new RuleTester({ | ||
| languageOptions: { | ||
| ecmaVersion: 2022, | ||
| sourceType: "commonjs", | ||
| }, | ||
| }); | ||
|
|
||
| describe("require-fs-io-try-catch", () => { | ||
| it("valid: fs.statSync inside try block passes", () => { | ||
| cjsRuleTester.run("require-fs-io-try-catch", requireFsIoTryCatchRule, { | ||
| valid: [ | ||
| `try { const s = fs.statSync(path); } catch (e) {}`, | ||
| `try { const entries = fs.readdirSync(dir); } catch (e) {}`, | ||
| `try { fs.copyFileSync(src, dest); } catch (e) {}`, | ||
| `try { fs.unlinkSync(path); } catch (e) {}`, | ||
| `try { fs.renameSync(oldPath, newPath); } catch (e) {}`, | ||
| ], | ||
| invalid: [], | ||
| }); | ||
| }); | ||
|
|
||
| it("valid: other fs methods not in scope are ignored", () => { | ||
| cjsRuleTester.run("require-fs-io-try-catch", requireFsIoTryCatchRule, { | ||
| valid: [`fs.existsSync(path);`, `fs.readFileSync(path, "utf8");`, `fs.writeFileSync(path, data);`, `mockFs.statSync(path);`, `storage.readdirSync(dir);`], | ||
| invalid: [], | ||
| }); | ||
| }); | ||
|
|
||
| it("invalid: fs.statSync outside try/catch is flagged", () => { | ||
| cjsRuleTester.run("require-fs-io-try-catch", requireFsIoTryCatchRule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: `fs.statSync(path);`, | ||
| errors: [{ messageId: "requireTryCatch", data: { method: "statSync", arg: "path" } }], | ||
| }, | ||
| ], | ||
| }); | ||
| }); | ||
|
|
||
| it("invalid: fs.readdirSync outside try/catch is flagged", () => { | ||
| cjsRuleTester.run("require-fs-io-try-catch", requireFsIoTryCatchRule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: `const entries = fs.readdirSync(dir);`, | ||
| errors: [{ messageId: "requireTryCatch", data: { method: "readdirSync", arg: "dir" } }], | ||
| }, | ||
| ], | ||
| }); | ||
| }); | ||
|
|
||
| it("invalid: fs.copyFileSync outside try/catch is flagged", () => { | ||
| cjsRuleTester.run("require-fs-io-try-catch", requireFsIoTryCatchRule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: `fs.copyFileSync(src, dest);`, | ||
| errors: [{ messageId: "requireTryCatch", data: { method: "copyFileSync", arg: "src" } }], | ||
| }, | ||
| ], | ||
| }); | ||
| }); | ||
|
|
||
| it("invalid: fs.unlinkSync outside try/catch is flagged", () => { | ||
| cjsRuleTester.run("require-fs-io-try-catch", requireFsIoTryCatchRule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: `fs.unlinkSync(outputFile);`, | ||
| errors: [{ messageId: "requireTryCatch", data: { method: "unlinkSync", arg: "outputFile" } }], | ||
| }, | ||
| ], | ||
| }); | ||
| }); | ||
|
|
||
| it("invalid: fs.renameSync outside try/catch is flagged", () => { | ||
| cjsRuleTester.run("require-fs-io-try-catch", requireFsIoTryCatchRule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: `fs.renameSync(tmpPath, finalPath);`, | ||
| errors: [{ messageId: "requireTryCatch", data: { method: "renameSync", arg: "tmpPath" } }], | ||
| }, | ||
| ], | ||
| }); | ||
| }); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The destructured-valid test covers 💡 Add missing destructured casesvalid: [
`const { statSync } = require("node:fs"); try { statSync(path); } catch (e) {}`,
`const { copyFileSync } = require("fs"); try { copyFileSync(src, dest); } catch (e) {}`,
`const { unlinkSync } = require("fs"); try { unlinkSync(path); } catch (e) {}`,
`const { renameSync } = require("fs"); try { renameSync(a, b); } catch (e) {}`,
],
invalid: [
{ code: `const { copyFileSync } = require("fs"); copyFileSync(src, dest);`, errors: [{ messageId: "requireTryCatch" }] },
{ code: `const { unlinkSync } = require("fs"); unlinkSync(path);`, errors: [{ messageId: "requireTryCatch" }] },
],@copilot please address this. |
||
|
|
||
| it("valid: node:fs destructured inside try block passes", () => { | ||
| cjsRuleTester.run("require-fs-io-try-catch", requireFsIoTryCatchRule, { | ||
| valid: [`const { statSync } = require("node:fs"); try { statSync(path); } catch (e) {}`, `const { readdirSync } = require("fs"); try { readdirSync(dir); } catch (e) {}`], | ||
| invalid: [], | ||
| }); | ||
| }); | ||
|
|
||
| it("invalid: destructured fs methods outside try/catch are flagged", () => { | ||
| cjsRuleTester.run("require-fs-io-try-catch", requireFsIoTryCatchRule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: `const { statSync } = require("node:fs"); statSync(path);`, | ||
| errors: [{ messageId: "requireTryCatch" }], | ||
| }, | ||
| ], | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| import { ESLintUtils } from "@typescript-eslint/utils"; | ||
| import { buildTryCatchSuggestion, createFsSyncMethodResolver, findEnclosingStatement, isInsideTryBlock } from "./try-catch-rule-utils"; | ||
|
|
||
| const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); | ||
|
|
||
| // fs methods beyond readFileSync/writeFileSync/appendFileSync that throw on I/O failure | ||
| // and appear frequently unguarded in actions/setup/js. | ||
| const FS_IO_METHODS = new Set(["statSync", "readdirSync", "copyFileSync", "unlinkSync", "renameSync"]); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/codebase-design] This rule is near-identical to 💡 Suggested refactorAdd a small factory in export function createFsMethodTryCatchRule(
name: string,
methods: Set<string>,
description: string
) {
return createRule({ name, meta: { ... description ... }, create(context) { /* shared visitor */ } });
}Both existing rules then become one-liners, and any future rule follows the same pattern at zero cost. @copilot please address this. |
||
|
|
||
| export const requireFsIoTryCatchRule = createRule({ | ||
| name: "require-fs-io-try-catch", | ||
|
|
||
| meta: { | ||
| type: "problem", | ||
| hasSuggestions: true, | ||
| docs: { | ||
| description: | ||
| "Require fs.statSync, fs.readdirSync, fs.copyFileSync, fs.unlinkSync, and fs.renameSync calls in actions/setup/js scripts to be wrapped in try/catch. " + | ||
| "These methods throw synchronously on missing files, permission errors, and other I/O failures; " + | ||
| "an unhandled throw crashes the action without surfacing a useful error message.", | ||
| }, | ||
| schema: [], | ||
| messages: { | ||
| requireTryCatch: "Wrap fs.{{method}}({{arg}}) in try/catch — synchronous fs methods throw on I/O errors " + "(missing file, permission denied, disk full) and will crash the action if unhandled.", | ||
| wrapInTryCatch: "Wrap in try { ... } catch { ... } and re-throw with { cause: err } to preserve context.", | ||
| }, | ||
| }, | ||
| defaultOptions: [], | ||
| create(context) { | ||
| const sourceCode = context.sourceCode; | ||
| const resolveFsIoMethod = createFsSyncMethodResolver(sourceCode, FS_IO_METHODS, { allowUnboundFsIdentifier: true }); | ||
|
|
||
| return { | ||
| CallExpression(node) { | ||
| const methodName = resolveFsIoMethod(node); | ||
|
|
||
| if (!methodName) return; | ||
|
|
||
| if (isInsideTryBlock(sourceCode, node)) return; | ||
|
|
||
| const argText = node.arguments.length > 0 ? sourceCode.getText(node.arguments[0]) : ""; | ||
| const method = methodName; | ||
| const stmt = findEnclosingStatement(sourceCode, node); | ||
|
|
||
| context.report({ | ||
| node, | ||
| messageId: "requireTryCatch", | ||
| data: { method, arg: argText }, | ||
| suggest: stmt | ||
| ? [ | ||
| { | ||
| messageId: "wrapInTryCatch", | ||
| fix(fixer) { | ||
| const stmtText = sourceCode.getText(stmt); | ||
| const startLine = stmt.loc?.start.line; | ||
| const stmtLine = startLine !== undefined ? (sourceCode.lines[startLine - 1] ?? "") : ""; | ||
| const indent = stmtLine.match(/^(\s*)/)?.[1] ?? ""; | ||
| return fixer.replaceText( | ||
| stmt, | ||
| buildTryCatchSuggestion(stmtText, { | ||
| indent, | ||
| todoComment: `TODO: handle I/O failure for this fs.${method} call.`, | ||
| errorPrefix: `fs.${method} failed: `, | ||
| }) | ||
| ); | ||
| }, | ||
| }, | ||
| ] | ||
| : [], | ||
| }); | ||
| }, | ||
| }; | ||
| }, | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] The
validtests group all five methods in oneitblock, but theinvalidtests use oneitper method. This asymmetry means a regression where a method is silently dropped from the valid set would only fail if you happened to test that one. Consider mirroring the structure — one combined invalid block, or one valid block per method — so coverage is symmetric.@copilot please address this.