fix(eslint-factory): require-fs-sync-try-catch catches destructured and aliased fs bindings #44240
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,9 @@ const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh | |
| // now to keep FP risk low on the first iteration. | ||
| const FS_SYNC_METHODS = new Set(["readFileSync", "writeFileSync", "appendFileSync"]); | ||
|
|
||
| // fs module specifiers recognised as the Node.js built-in file system module. | ||
| const FS_MODULE_SPECIFIERS = new Set(["fs", "node:fs"]); | ||
|
|
||
| export const requireFsSyncTryCatchRule = createRule({ | ||
| name: "require-fs-sync-try-catch", | ||
| meta: { | ||
|
|
@@ -29,6 +32,7 @@ export const requireFsSyncTryCatchRule = createRule({ | |
| defaultOptions: [], | ||
| create(context) { | ||
| const sourceCode = context.sourceCode; | ||
| type SourceCodeScope = ReturnType<typeof sourceCode.getScope>; | ||
|
Contributor
There was a problem hiding this comment. [/tdd] The Consider using the more explicit inline type @copilot please address this. |
||
|
|
||
| function isInsideTryBlock(node: TSESTree.Node): boolean { | ||
| const ancestors = sourceCode.getAncestors(node); | ||
|
|
@@ -65,23 +69,130 @@ export const requireFsSyncTryCatchRule = createRule({ | |
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Returns true if `node` is a `require("fs")` or `require("node:fs")` call. | ||
| */ | ||
| function isRequireFsCall(node: TSESTree.Node | null | undefined): boolean { | ||
| if (!node) return false; | ||
| return ( | ||
| node.type === AST_NODE_TYPES.CallExpression && | ||
| node.callee.type === AST_NODE_TYPES.Identifier && | ||
| node.callee.name === "require" && | ||
| node.arguments.length >= 1 && | ||
| node.arguments[0].type === AST_NODE_TYPES.Literal && | ||
| FS_MODULE_SPECIFIERS.has(node.arguments[0].value as string) | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Resolves the original fs sync method name for an Identifier callee via scope analysis. | ||
| * Handles two binding shapes: | ||
| * 1. Destructured: `const { appendFileSync } = require("fs")` | ||
| * or `const { appendFileSync: alias } = require("fs")` | ||
| * 2. Member-expression alias: `const alias = fs.appendFileSync` | ||
| * | ||
| * Returns the canonical method name (e.g. "appendFileSync") or null if the identifier | ||
| * does not trace back to an in-scope fs sync method. | ||
| */ | ||
| function resolveFsSyncMethodFromIdentifier(node: TSESTree.CallExpression): string | null { | ||
| const callee = node.callee; | ||
| if (callee.type !== AST_NODE_TYPES.Identifier) return null; | ||
|
|
||
| let scope: SourceCodeScope | null = sourceCode.getScope(node); | ||
| while (scope) { | ||
| const variable = scope.set.get(callee.name); | ||
| if (variable && variable.defs.length > 0) { | ||
| for (const def of variable.defs) { | ||
| if (def.type !== "Variable") continue; | ||
| const declarator = def.node as TSESTree.VariableDeclarator; | ||
|
|
||
| // Shape 1: `const { appendFileSync } = require("fs")` | ||
| // or `const { appendFileSync: alias } = require("fs")` | ||
| if (declarator.id.type === AST_NODE_TYPES.ObjectPattern && isRequireFsCall(declarator.init)) { | ||
| for (const prop of declarator.id.properties) { | ||
| if (prop.type !== AST_NODE_TYPES.Property) continue; | ||
| if (prop.key.type !== AST_NODE_TYPES.Identifier) continue; | ||
| if (!FS_SYNC_METHODS.has(prop.key.name)) continue; | ||
| // prop.value is the bound identifier (same as key for shorthand) | ||
| const boundName = prop.value.type === AST_NODE_TYPES.Identifier ? prop.value.name : null; | ||
| if (boundName === callee.name) { | ||
| return prop.key.name; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Shape 2: `const alias = fs.appendFileSync` | ||
| // Only matches when the `fs` identifier is itself bound to require("fs") / require("node:fs"). | ||
| if (declarator.id.type === AST_NODE_TYPES.Identifier && declarator.init?.type === AST_NODE_TYPES.MemberExpression) { | ||
| const init = declarator.init; | ||
| if (init.object.type === AST_NODE_TYPES.Identifier && isIdentifierBoundToFsModule(init.object.name, init.object)) { | ||
| const methodName = getFsSyncMethodFromProperty(init); | ||
| if (methodName !== null) return methodName; | ||
| } | ||
| } | ||
| } | ||
| // Variable is locally defined but not as an fs sync method binding — stop searching. | ||
|
Contributor
There was a problem hiding this comment. [/diagnosing-bugs] Scope-walk stops at the first definition block of a variable regardless of whether it contains any This is an edge case unlikely in practice, but worth a comment or a test to document the intent. 💡 Low-risk clarifying comment// If variable is found but has no defs (e.g. global built-in), skip to outer scope.
// This is intentional: we want to walk up to find a require("fs") binding.Alternatively, a test like: // valid: a globally-named `appendFileSync` that is not from require("fs")
`appendFileSync(filePath, data);` // should NOT be flagged (no binding found at all)@copilot please address this. |
||
| return null; | ||
| } | ||
| scope = scope.upper; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the fs sync method name from a MemberExpression property, or null if the property | ||
| * is not one of the in-scope fs sync methods. Handles both direct and computed string-literal access. | ||
| */ | ||
| function getFsSyncMethodFromProperty(memberExpr: TSESTree.MemberExpression): string | null { | ||
| const property = memberExpr.property; | ||
| if (!memberExpr.computed && property.type === AST_NODE_TYPES.Identifier && FS_SYNC_METHODS.has(property.name)) { | ||
| return property.name; | ||
| } | ||
| if (memberExpr.computed && property.type === AST_NODE_TYPES.Literal && typeof property.value === "string" && FS_SYNC_METHODS.has(property.value)) { | ||
| return property.value; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Returns true if the named identifier is bound to `require("fs")` or `require("node:fs")` | ||
| * anywhere in the scope chain visible from `scopeNode`. | ||
| */ | ||
| function isIdentifierBoundToFsModule(identifierName: string, scopeNode: TSESTree.Node): boolean { | ||
| let scope: SourceCodeScope | null = sourceCode.getScope(scopeNode); | ||
| while (scope) { | ||
| const variable = scope.set.get(identifierName); | ||
| if (variable && variable.defs.length > 0) { | ||
| for (const def of variable.defs) { | ||
| if (def.type !== "Variable") continue; | ||
| const declarator = def.node as TSESTree.VariableDeclarator; | ||
| if (declarator.id.type === AST_NODE_TYPES.Identifier && isRequireFsCall(declarator.init)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; // Identifier found but not bound to require("fs") | ||
| } | ||
| scope = scope.upper; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| return { | ||
| CallExpression(node) { | ||
| const callee = node.callee; | ||
| let methodName: string | null = null; | ||
|
|
||
| if (callee.type === AST_NODE_TYPES.MemberExpression) { | ||
| // Object may be `fs` or any identifier bound to require("fs") / require("node:fs"). | ||
| if (callee.object.type !== AST_NODE_TYPES.Identifier) return; | ||
| if (callee.object.name !== "fs" && !isIdentifierBoundToFsModule(callee.object.name, callee.object)) return; | ||
|
|
||
| // Accept both direct property access (fs.readFileSync) and computed string-literal access | ||
| // (fs["readFileSync"]). Dynamic computed access (fs[varName]) is excluded. | ||
| methodName = getFsSyncMethodFromProperty(callee); | ||
| } else if (callee.type === AST_NODE_TYPES.Identifier) { | ||
| // Destructured or aliased fs binding — resolve via scope analysis. | ||
| methodName = resolveFsSyncMethodFromIdentifier(node); | ||
| } | ||
|
|
||
| if (!methodName) return; | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] Missing false-positive edge case: no test verifies that a non-fs identifier with the same name as an fs method is not flagged.
If
appendFileSyncis destructured from a non-fs library, the scope-walk will returnnull— but without a test, a future refactor could silently introduce false positives.💡 Suggested valid test case
@copilot please address this.