diff --git a/eslint-factory/eslint.config.cjs b/eslint-factory/eslint.config.cjs index 2b60553305f..0cb81fe8524 100644 --- a/eslint-factory/eslint.config.cjs +++ b/eslint-factory/eslint.config.cjs @@ -43,6 +43,7 @@ module.exports = [ "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", + "gh-aw-custom/no-core-error-then-setfailed": "warn", }, }, { diff --git a/eslint-factory/src/index.ts b/eslint-factory/src/index.ts index fe84d35ab48..e5b276bf65a 100644 --- a/eslint-factory/src/index.ts +++ b/eslint-factory/src/index.ts @@ -29,6 +29,7 @@ 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"; +import { noCoreErrorThenSetFailedRule } from "./rules/no-core-error-then-setfailed"; const plugin = { meta: { @@ -67,6 +68,7 @@ const plugin = { "no-err-stack-then-string-fallback": noErrStackThenStringFallbackRule, "no-caught-error-interpolation": noCaughtErrorInterpolationRule, "require-fetch-try-catch": requireFetchTryCatchRule, + "no-core-error-then-setfailed": noCoreErrorThenSetFailedRule, }, }; diff --git a/eslint-factory/src/rules/no-core-error-then-setfailed.test.ts b/eslint-factory/src/rules/no-core-error-then-setfailed.test.ts new file mode 100644 index 00000000000..6f0c7044442 --- /dev/null +++ b/eslint-factory/src/rules/no-core-error-then-setfailed.test.ts @@ -0,0 +1,94 @@ +// Uses eslint's RuleTester rather than @typescript-eslint/rule-tester, matching the +// convention of all other rule tests in this package. The rule uses @typescript-eslint/utils +// internally but the standard eslint RuleTester is sufficient for all test scenarios here. +import { RuleTester } from "eslint"; +import { describe, it } from "vitest"; +import { noCoreErrorThenSetFailedRule } from "./no-core-error-then-setfailed"; + +const ruleTester = new RuleTester({ + languageOptions: { + ecmaVersion: "latest", + sourceType: "module", + }, +}); + +describe("no-core-error-then-setfailed", () => { + it("valid and invalid cases", () => { + ruleTester.run("no-core-error-then-setfailed", noCoreErrorThenSetFailedRule, { + valid: [ + // Only core.setFailed — no preceding core.error + `core.setFailed("something went wrong");`, + // Only core.error — no following setFailed + `core.error("something went wrong");`, + // core.error followed by something other than setFailed + `function f() { core.error("msg"); return; }`, + // core.warning followed by core.setFailed is allowed (different method) + `core.warning("msg"); core.setFailed("msg");`, + // core.error without adjacent setFailed (setFailed is non-adjacent) + `core.error("msg"); doSomething(); core.setFailed("msg");`, + // Different messages — core.error provides extra context not repeated by setFailed + `core.error("upload failed: " + filename); core.setFailed("action failed");`, + // Different template literals — messages differ in prefix + { + code: ` + try { + doSomething(); + } catch (err) { + core.error(\`Failed: \${err.message}\`); + core.setFailed(\`ERR: Failed: \${err.message}\`); + } + `, + }, + // core.error with annotation properties — carries extra diagnostic context + `core.error("msg", { title: "Upload error" }); core.setFailed("msg");`, + // Different core objects (cross-alias false-positive guard): + // c1 and c2 are different objects even if both are in CORE_ALIASES + `const c1 = core; const c2 = coreObj; c1.error("msg"); c2.setFailed("msg");`, + // Non-core alias is not flagged + `const c = notCore; c.error("msg"); c.setFailed("msg");`, + ], + invalid: [ + // Adjacent core.error then core.setFailed with same literal — has suggestion + { + code: `core.error("msg"); core.setFailed("msg");`, + errors: [{ messageId: "noCoreErrorThenSetFailed", suggestions: [{ messageId: "removeErrorCall", output: ` core.setFailed("msg");` }] }], + }, + // With an alias (const c = core) and matching messages — has suggestion + { + code: `const c = core; c.error("msg"); c.setFailed("msg");`, + errors: [{ messageId: "noCoreErrorThenSetFailed", suggestions: [{ messageId: "removeErrorCall", output: `const c = core; c.setFailed("msg");` }] }], + }, + // Computed property access with matching messages — has suggestion + { + code: `core["error"]("msg"); core["setFailed"]("msg");`, + errors: [{ messageId: "noCoreErrorThenSetFailed", suggestions: [{ messageId: "removeErrorCall", output: ` core["setFailed"]("msg");` }] }], + }, + // Same template literal in both calls — side-effect-free (identifier inside), has suggestion + { + code: `core.error(\`error: \${msg}\`); core.setFailed(\`error: \${msg}\`);`, + errors: [ + { + messageId: "noCoreErrorThenSetFailed", + suggestions: [{ messageId: "removeErrorCall", output: ` core.setFailed(\`error: \${msg}\`);` }], + }, + ], + }, + // Same call-expression argument — NOT side-effect-free: report but no suggestion + { + code: `core.error(nextMessage()); core.setFailed(nextMessage());`, + errors: [{ messageId: "noCoreErrorThenSetFailed", suggestions: [] }], + }, + // Inside a block with matching messages — has suggestion + { + code: `function run() { core.error("fatal"); core.setFailed("fatal"); }`, + errors: [ + { + messageId: "noCoreErrorThenSetFailed", + suggestions: [{ messageId: "removeErrorCall", output: `function run() { core.setFailed("fatal"); }` }], + }, + ], + }, + ], + }); + }); +}); diff --git a/eslint-factory/src/rules/no-core-error-then-setfailed.ts b/eslint-factory/src/rules/no-core-error-then-setfailed.ts new file mode 100644 index 00000000000..c41188071a6 --- /dev/null +++ b/eslint-factory/src/rules/no-core-error-then-setfailed.ts @@ -0,0 +1,198 @@ +import { AST_NODE_TYPES, ESLintUtils, TSESLint, TSESTree } from "@typescript-eslint/utils"; +import { CORE_ALIASES } from "./core-aliases"; +import { isCoreAliasIdentifier } from "./core-method-resolve"; + +const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); + +type SourceCode = Parameters[1]; + +function isCoreLikeIdentifier(name: string): boolean { + return CORE_ALIASES.has(name); +} + +/** + * Returns the first non-SpreadElement argument of a call, or null when + * there are no arguments or the first argument is a spread. + */ +function getFirstNonSpreadArg(call: TSESTree.CallExpression): TSESTree.Expression | null { + if (call.arguments.length === 0) return null; + const first = call.arguments[0]; + if (first.type === AST_NODE_TYPES.SpreadElement) return null; + return first as TSESTree.Expression; +} + +/** + * Returns true when the call has more than one argument (i.e. annotation + * properties are present, e.g. `core.error(msg, { title: "..." })`). + * Such calls carry diagnostic context not duplicated in setFailed and must + * not be flagged as redundant. + */ +function hasAnnotationProperties(call: TSESTree.CallExpression): boolean { + return call.arguments.length > 1; +} + +/** + * Returns true when the expression is provably side-effect-free: no call, + * new, or assignment expression at any nesting level. Conservatively returns + * false for any node type not listed here. + */ +function isSideEffectFree(node: TSESTree.Expression): boolean { + switch (node.type) { + case AST_NODE_TYPES.Literal: + case AST_NODE_TYPES.Identifier: + return true; + case AST_NODE_TYPES.TemplateLiteral: + return (node as TSESTree.TemplateLiteral).expressions.every(e => isSideEffectFree(e as TSESTree.Expression)); + case AST_NODE_TYPES.MemberExpression: { + const me = node as TSESTree.MemberExpression; + return isSideEffectFree(me.object as TSESTree.Expression) && (!me.computed || isSideEffectFree(me.property as TSESTree.Expression)); + } + case AST_NODE_TYPES.BinaryExpression: { + const be = node as TSESTree.BinaryExpression; + return isSideEffectFree(be.left as TSESTree.Expression) && isSideEffectFree(be.right as TSESTree.Expression); + } + case AST_NODE_TYPES.UnaryExpression: + return isSideEffectFree((node as TSESTree.UnaryExpression).argument as TSESTree.Expression); + default: + return false; + } +} + +/** + * Returns true when `node` is an expression statement containing a call to + * `.(...)` where the receiver is a known core alias + * (direct or assigned alias). Also returns the receiver identifier name via + * the `objectName` out-param so the caller can enforce same-object pairing. + */ +function isCoreMethodCallStatement(node: TSESTree.Statement, sourceCode: SourceCode, methodName: string): node is TSESTree.ExpressionStatement { + if (node.type !== AST_NODE_TYPES.ExpressionStatement) return false; + const expr = node.expression; + if (expr.type !== AST_NODE_TYPES.CallExpression) return false; + const callee = expr.callee; + if (callee.type !== AST_NODE_TYPES.MemberExpression) return false; + + const obj = callee.object; + const prop = callee.property; + const isNonComputed = !callee.computed && prop.type === AST_NODE_TYPES.Identifier && (prop as TSESTree.Identifier).name === methodName; + const isComputed = callee.computed && prop.type === AST_NODE_TYPES.Literal && (prop as TSESTree.Literal).value === methodName; + if (!isNonComputed && !isComputed) return false; + if (obj.type !== AST_NODE_TYPES.Identifier) return false; + + return isCoreLikeIdentifier((obj as TSESTree.Identifier).name) || isCoreAliasIdentifier(obj as TSESTree.Identifier, sourceCode); +} + +function isCoreErrorStatement(node: TSESTree.Statement, sourceCode: SourceCode): node is TSESTree.ExpressionStatement { + return isCoreMethodCallStatement(node, sourceCode, "error"); +} + +function isCoreSetFailedStatement(node: TSESTree.Statement, sourceCode: SourceCode): node is TSESTree.ExpressionStatement { + return isCoreMethodCallStatement(node, sourceCode, "setFailed"); +} + +/** + * Returns the receiver identifier name from a matched core-method call statement. + * Precondition: `isCoreErrorStatement` or `isCoreSetFailedStatement` returned true. + */ +function getCoreObjectName(stmt: TSESTree.ExpressionStatement): string { + const call = stmt.expression as TSESTree.CallExpression; + const callee = call.callee as TSESTree.MemberExpression; + return (callee.object as TSESTree.Identifier).name; +} + +export const noCoreErrorThenSetFailedRule = createRule({ + name: "no-core-error-then-setfailed", + meta: { + type: "suggestion", + hasSuggestions: true, + docs: { + description: + "Disallow the redundant pattern `core.error(msg); core.setFailed(msg)` in GitHub Actions scripts. " + + "`core.setFailed()` already logs the message as an error annotation and marks the action as failed. " + + "Preceding it with `core.error()` using the same message creates a duplicate error annotation " + + "in the GitHub Actions log, adding noise without benefit. Use `core.setFailed(msg)` alone.", + }, + schema: [], + messages: { + noCoreErrorThenSetFailed: "`core.error()` immediately before `core.setFailed()` with the same message is redundant: `core.setFailed()` already logs an error annotation and marks the action failed. Remove the `core.error()` call.", + removeErrorCall: "Remove the redundant `core.error()` call — `core.setFailed()` already logs an error annotation.", + }, + }, + defaultOptions: [], + create(context) { + const sourceCode = context.sourceCode; + + function checkStatements(stmts: readonly TSESTree.Statement[]): void { + for (let i = 0; i < stmts.length - 1; i++) { + const current = stmts[i]; + if (!isCoreErrorStatement(current, sourceCode)) continue; + + const next = stmts[i + 1]; + if (!isCoreSetFailedStatement(next, sourceCode)) continue; + + // Both calls must reference the same receiver identifier to avoid + // flagging `c1.error("x"); c2.setFailed("x")` where c1 and c2 are + // different objects that happen to both be in CORE_ALIASES. + if (getCoreObjectName(current) !== getCoreObjectName(next)) continue; + + const errorCall = (current as TSESTree.ExpressionStatement).expression as TSESTree.CallExpression; + const setFailedCall = (next as TSESTree.ExpressionStatement).expression as TSESTree.CallExpression; + + // Do not flag core.error calls that carry annotation properties (e.g. + // core.error(msg, { title: "..." })). The second argument provides + // diagnostic context that is not duplicated by setFailed. + if (hasAnnotationProperties(errorCall)) continue; + + // Only report when the message arguments are provably equivalent (same + // source text). Calls with different messages are not redundant — the + // core.error call may log extra context (file names, sizes, stack frames) + // that setFailed does not repeat. + const errorArg = getFirstNonSpreadArg(errorCall); + const setFailedArg = getFirstNonSpreadArg(setFailedCall); + if (errorArg === null || setFailedArg === null) continue; + if (sourceCode.getText(errorArg) !== sourceCode.getText(setFailedArg)) continue; + + // The auto-remove suggestion is semantics-preserving only when the shared + // argument is provably side-effect-free. For example, + // `core.error(nextMessage()); core.setFailed(nextMessage())` must not have + // the first call silently removed because that would drop a side-effectful + // function invocation. + const safeToFix = isSideEffectFree(errorArg); + + context.report({ + node: current, + messageId: "noCoreErrorThenSetFailed", + suggest: safeToFix + ? [ + { + messageId: "removeErrorCall", + fix(fixer: TSESLint.RuleFixer) { + return fixer.remove(current); + }, + }, + ] + : [], + }); + } + } + + return { + BlockStatement(node: TSESTree.BlockStatement) { + checkStatements(node.body); + }, + SwitchCase(node: TSESTree.SwitchCase) { + checkStatements(node.consequent); + }, + Program(node: TSESTree.Program) { + // Filter out all module declarations (ImportDeclaration, ExportAllDeclaration, + // ExportNamedDeclaration, ExportDefaultDeclaration) — they are not Statements + // and their type assertion would be incorrect. BlockStatement visitor handles + // the bodies of any exported function/class declarations separately. + const stmts = node.body.filter( + (s): s is TSESTree.Statement => + s.type !== AST_NODE_TYPES.ImportDeclaration && s.type !== AST_NODE_TYPES.ExportAllDeclaration && s.type !== AST_NODE_TYPES.ExportNamedDeclaration && s.type !== AST_NODE_TYPES.ExportDefaultDeclaration + ); + checkStatements(stmts); + }, + }; + }, +});