diff --git a/eslint-factory/eslint.config.cjs b/eslint-factory/eslint.config.cjs index 63ce614345b..2b60553305f 100644 --- a/eslint-factory/eslint.config.cjs +++ b/eslint-factory/eslint.config.cjs @@ -41,6 +41,7 @@ module.exports = [ "gh-aw-custom/require-fs-io-try-catch": "warn", "gh-aw-custom/no-setfailed-then-exit-zero": "warn", "gh-aw-custom/no-err-stack-then-string-fallback": "warn", + "gh-aw-custom/no-caught-error-interpolation": "warn", "gh-aw-custom/require-fetch-try-catch": "warn", }, }, diff --git a/eslint-factory/src/index.ts b/eslint-factory/src/index.ts index 59114b0244c..fe84d35ab48 100644 --- a/eslint-factory/src/index.ts +++ b/eslint-factory/src/index.ts @@ -27,6 +27,7 @@ import { requireExecFileSyncTryCatchRule } from "./rules/require-execfilesync-tr import { requireFsIoTryCatchRule } from "./rules/require-fs-io-try-catch"; import { noSetFailedThenExitZeroRule } from "./rules/no-setfailed-then-exit-zero"; import { noErrStackThenStringFallbackRule } from "./rules/no-err-stack-then-string-fallback"; +import { noCaughtErrorInterpolationRule } from "./rules/no-caught-error-interpolation"; import { requireFetchTryCatchRule } from "./rules/require-fetch-try-catch"; const plugin = { @@ -64,6 +65,7 @@ const plugin = { "require-fs-io-try-catch": requireFsIoTryCatchRule, "no-setfailed-then-exit-zero": noSetFailedThenExitZeroRule, "no-err-stack-then-string-fallback": noErrStackThenStringFallbackRule, + "no-caught-error-interpolation": noCaughtErrorInterpolationRule, "require-fetch-try-catch": requireFetchTryCatchRule, }, }; diff --git a/eslint-factory/src/rules/no-caught-error-interpolation.test.ts b/eslint-factory/src/rules/no-caught-error-interpolation.test.ts new file mode 100644 index 00000000000..71b3e538089 --- /dev/null +++ b/eslint-factory/src/rules/no-caught-error-interpolation.test.ts @@ -0,0 +1,332 @@ +import { RuleTester } from "eslint"; +import { describe, it } from "vitest"; +import { noCaughtErrorInterpolationRule } from "./no-caught-error-interpolation"; + +const cjsRuleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: "commonjs", + }, +}); + +const esmRuleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: "module", + }, +}); + +describe("no-caught-error-interpolation", () => { + it("valid: non-caught variable interpolation is not flagged", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [`const err = new Error("test"); const msg = \`Error: \${err}\`;`, `const name = "world"; const s = \`Hello \${name}\`;`, `fetch(url).then(r => \`status: \${r}\`);`], + invalid: [], + }); + }); + + it("valid: getErrorMessage(err) interpolation is not flagged", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [ + `try { f(); } catch (err) { console.log(\`failed: \${getErrorMessage(err)}\`); }`, + `try { f(); } catch (err) { core.setFailed(\`Error: \${getErrorMessage(err)}\`); }`, + `p.catch(err => { log(\`failed: \${getErrorMessage(err)}\`); });`, + ], + invalid: [], + }); + }); + + it("valid: String(err) interpolation is not flagged", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [`try { f(); } catch (err) { console.log(\`failed: \${String(err)}\`); }`, `p.catch(err => \`error: \${String(err)}\`);`], + invalid: [], + }); + }); + + it("valid: err.message interpolation is not flagged (covered by no-unsafe-catch-error-property)", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [`try { f(); } catch (err) { console.log(\`failed: \${err.message}\`); }`, `try { f(); } catch (err) { console.log(\`stack: \${err.stack}\`); }`], + invalid: [], + }); + }); + + it("valid: err.toString() interpolation is not flagged", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [`try { f(); } catch (err) { console.log(\`failed: \${err.toString()}\`); }`], + invalid: [], + }); + }); + + it("valid: tagged template expression is not flagged (tag receives raw values)", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [ + // Tagged templates pass values to the tag as-is; no unsafe coercion occurs + `try { f(); } catch (err) { tag\`msg: \${err}\`; }`, + `p.catch(err => format\`error: \${err}\`);`, + ], + invalid: [], + }); + }); + + it("valid: .catch() with zero params is not flagged", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [`fetch(url).catch(() => { log("caught"); });`], + invalid: [], + }); + }); + + it("valid: destructured catch param elements are not flagged", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [ + // `message` is already a string extracted from the error object + `try { f(); } catch ({ message }) { log(\`failed: \${message}\`); }`, + `try { f(); } catch ({ message, code }) { log(\`\${code}: \${message}\`); }`, + ], + invalid: [], + }); + }); + + it("invalid: bare catch variable in template literal is flagged (try/catch)", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [], + invalid: [ + { + code: `try { f(); } catch (err) { console.log(\`failed: \${err}\`); }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "err" }, + output: `try { f(); } catch (err) { console.log(\`failed: \${String(err)}\`); }`, + }, + ], + }, + ], + }, + { + code: `try { f(); } catch (error) { core.setFailed(\`Action failed: \${error}\`); }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "error" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "error" }, + output: `try { f(); } catch (error) { core.setFailed(\`Action failed: \${String(error)}\`); }`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: bare catch variable flagged with getErrorMessage suggestion when in scope", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [], + invalid: [ + { + code: `const { getErrorMessage } = require("./error_helpers.cjs"); try { f(); } catch (err) { console.log(\`failed: \${err}\`); }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useGetErrorMessage", + data: { errorVar: "err" }, + output: `const { getErrorMessage } = require("./error_helpers.cjs"); try { f(); } catch (err) { console.log(\`failed: \${getErrorMessage(err)}\`); }`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: bare .catch() rejection handler variable is flagged", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [], + invalid: [ + { + code: `fetch(url).catch(err => { log(\`network error: \${err}\`); });`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "err" }, + output: `fetch(url).catch(err => { log(\`network error: \${String(err)}\`); });`, + }, + ], + }, + ], + }, + { + code: `p.then(null, err => { log(\`rejected: \${err}\`); });`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "err" }, + output: `p.then(null, err => { log(\`rejected: \${String(err)}\`); });`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: multiple bare error interpolations in same template are all flagged", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [], + invalid: [ + { + code: `try { f(); } catch (err) { log(\`got \${err} at step, original: \${err}\`); }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "err" }, + output: `try { f(); } catch (err) { log(\`got \${String(err)} at step, original: \${err}\`); }`, + }, + ], + }, + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "err" }, + output: `try { f(); } catch (err) { log(\`got \${err} at step, original: \${String(err)}\`); }`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: closure inside catch body still flags outer catch variable", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [], + invalid: [ + { + code: `try { f(); } catch (err) { setTimeout(function() { log(\`msg: \${err}\`); }, 0); }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "err" }, + output: `try { f(); } catch (err) { setTimeout(function() { log(\`msg: \${String(err)}\`); }, 0); }`, + }, + ], + }, + ], + }, + { + code: `try { f(); } catch (err) { [1].forEach(x => { log(\`\${err}\`); }); }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "err" }, + output: `try { f(); } catch (err) { [1].forEach(x => { log(\`\${String(err)}\`); }); }`, + }, + ], + }, + ], + }, + { + // FunctionDeclaration inside catch — err resolves to the catch binding, + // so the interpolation is unsafe and must be flagged + code: `try { f(); } catch (err) { function helper() { return \`msg: \${err}\`; } }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "err" }, + output: `try { f(); } catch (err) { function helper() { return \`msg: \${String(err)}\`; } }`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: ESM import of getErrorMessage triggers getErrorMessage suggestion", () => { + esmRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [], + invalid: [ + { + code: `import { getErrorMessage } from "./error_helpers.js"; try { f(); } catch (err) { log(\`failed: \${err}\`); }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useGetErrorMessage", + data: { errorVar: "err" }, + output: `import { getErrorMessage } from "./error_helpers.js"; try { f(); } catch (err) { log(\`failed: \${getErrorMessage(err)}\`); }`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: ESM import context is also flagged", () => { + esmRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [], + invalid: [ + { + code: `try { f(); } catch (e) { throw new Error(\`wrapper: \${e}\`); }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "e" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "e" }, + output: `try { f(); } catch (e) { throw new Error(\`wrapper: \${String(e)}\`); }`, + }, + ], + }, + ], + }, + ], + }); + }); +}); diff --git a/eslint-factory/src/rules/no-caught-error-interpolation.ts b/eslint-factory/src/rules/no-caught-error-interpolation.ts new file mode 100644 index 00000000000..9d7c6c3a6b7 --- /dev/null +++ b/eslint-factory/src/rules/no-caught-error-interpolation.ts @@ -0,0 +1,163 @@ +import { AST_NODE_TYPES, ESLintUtils, TSESLint, TSESTree } from "@typescript-eslint/utils"; + +const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); + +/** + * Returns true when the function node is an inline rejection handler passed to + * a promise method (.catch(fn) or .then(onFulfilled, onRejected)). + */ +function isInlineRejectionHandler(node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression): boolean { + const parent = node.parent; + if (!parent || parent.type !== AST_NODE_TYPES.CallExpression) return false; + const callee = parent.callee; + if (callee.type !== AST_NODE_TYPES.MemberExpression || callee.computed) return false; + const prop = callee.property; + if (prop.type !== AST_NODE_TYPES.Identifier) return false; + if (prop.name === "catch" && parent.arguments[0] === node) return true; + if (prop.name === "then" && parent.arguments[1] === node) return true; + return false; +} + +/** + * Returns true when `node` is a bare `Identifier` expression — no member + * access, no call, no unary/binary operation, no nullish coercion. Used to + * identify direct `${someVar}` interpolations as opposed to safe forms such + * as `${someVar.message}` or `${fn(someVar)}`. + */ +function isBareIdentifierExpression(node: TSESTree.Expression): node is TSESTree.Identifier { + return node.type === AST_NODE_TYPES.Identifier; +} + +/** + * Returns true when the variable definition represents a caught error binding + * that should not be interpolated directly: + * - A try/catch clause with a simple identifier param (not destructured). + * - A parameter of an inline promise rejection handler (.catch(fn) / + * .then(_, fn)). + */ +function isCaughtErrorVariableDef(def: TSESLint.Scope.Definition): boolean { + // try/catch binding — only flag simple identifier params, not destructured ones + // (e.g. `catch ({ message })` introduces string properties, not raw error objects) + if (def.type === "CatchClause") { + const catchNode = def.node as TSESTree.CatchClause; + return catchNode.param?.type === AST_NODE_TYPES.Identifier; + } + + // Inline rejection handler parameter (.catch(err => ...) / .then(_, err => ...)) + // def.node is the function node for Parameter definitions + if (def.type === "Parameter") { + const fn = def.node as TSESTree.Node; + if (fn.type !== AST_NODE_TYPES.ArrowFunctionExpression && fn.type !== AST_NODE_TYPES.FunctionExpression) { + return false; + } + return isInlineRejectionHandler(fn as TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression); + } + + return false; +} + +export const noCaughtErrorInterpolationRule = createRule({ + name: "no-caught-error-interpolation", + meta: { + type: "suggestion", + hasSuggestions: true, + docs: { + description: + "Disallow directly interpolating a caught error variable in a template literal (e.g. `${err}`). " + + "For Error objects this produces the redundant 'Error: message' prefix; for non-Error throws (plain objects, strings, etc.) " + + "it silently produces '[object Object]' or another useless string. " + + "Use getErrorMessage(err) for consistent, safe formatting, or String(err) when getErrorMessage is unavailable. " + + "Detected scopes: try/catch bindings, .catch(fn) inline callbacks, and .then(onFulfilled, onRejected) inline rejection handlers.", + }, + schema: [], + messages: { + bareErrorInterpolation: + "Directly interpolating caught error '{{errorVar}}' in a template literal is unsafe — " + + "for Error objects it produces 'Error: message' (redundant prefix); for non-Error throws it produces '[object Object]'. " + + "Use ${getErrorMessage({{errorVar}})} if it is available, or ${String({{errorVar}})} as an import-free alternative.", + useGetErrorMessage: "Replace \\${{{errorVar}}} with \\${getErrorMessage({{errorVar}})} — ensure getErrorMessage is imported from error_helpers.cjs.", + useStringFallback: "Replace \\${{{errorVar}}} with \\${String({{errorVar}})} — getErrorMessage is not in scope; String() is a safe import-free alternative.", + }, + }, + defaultOptions: [], + create(context) { + const sourceCode = context.sourceCode; + type Scope = ReturnType; + + function isDefinitionAvailableAtNode(definition: TSESLint.Scope.Definition, node: TSESTree.Node): boolean { + if (definition.type === "ImportBinding" || definition.type === "FunctionName") { + return true; + } + const definitionNode = definition.name ?? definition.node; + // range is absent on synthetic/virtual nodes; treat as unavailable rather than throwing + if (!definitionNode?.range || !node.range) return false; + return definitionNode.range[0] < node.range[0]; + } + + function hasResolvableLocalBinding(node: TSESTree.Node, name: string): boolean { + let scope: Scope | null = sourceCode.getScope(node); + while (scope) { + const variable = scope.set.get(name); + if (variable && variable.defs.some(def => isDefinitionAvailableAtNode(def, node))) { + return true; + } + scope = scope.upper; + } + return false; + } + + return { + TemplateLiteral(node) { + // Tagged templates pass values to the tag function as-is; they are not + // string-coerced by interpolation, so flagging them would be incorrect. + if (node.parent?.type === AST_NODE_TYPES.TaggedTemplateExpression) return; + + for (const expr of node.expressions) { + if (!isBareIdentifierExpression(expr)) continue; + + // Resolve the identifier through the full scope chain. This correctly + // handles closures: an identifier inside a nested function can still + // resolve to an outer catch binding. + let scope: Scope | null = sourceCode.getScope(expr); + let variable: TSESLint.Scope.Variable | null = null; + while (scope) { + const v = scope.set.get(expr.name); + if (v) { + variable = v; + break; + } + scope = scope.upper; + } + + if (!variable || !variable.defs.some(isCaughtErrorVariableDef)) continue; + + const errorVar = expr.name; + const getErrorMessageAvailable = hasResolvableLocalBinding(node, "getErrorMessage"); + + context.report({ + node: expr, + messageId: "bareErrorInterpolation", + data: { errorVar }, + suggest: [ + getErrorMessageAvailable + ? ({ + messageId: "useGetErrorMessage" as const, + data: { errorVar }, + fix(fixer) { + return fixer.replaceText(expr, `getErrorMessage(${errorVar})`); + }, + } as const) + : ({ + messageId: "useStringFallback" as const, + data: { errorVar }, + fix(fixer) { + return fixer.replaceText(expr, `String(${errorVar})`); + }, + } as const), + ], + }); + } + }, + }; + }, +});