From fd781ef110c1fdd61196ab04cc099eee271d933e Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 27 Jul 2026 10:09:00 +0000 Subject: [PATCH 1/3] eslint: add no-core-error-then-setfailed rule Add a new custom ESLint rule that flags the redundant pattern of calling core.error(msg) immediately before core.setFailed(msg). Since core.setFailed() already logs an error annotation and marks the action as failed, the preceding core.error() call creates a duplicate annotation in the GitHub Actions log. The rule detects this in BlockStatement, SwitchCase, and Program bodies. It provides an auto-fix suggestion to remove the redundant core.error() call. Evidence: 7 violations found across actions/setup/js/*.cjs in the current codebase (check_permissions.cjs, unlock-issue.cjs, add_reaction_and_edit_comment.cjs, add_reaction.cjs, push_repo_memory.cjs, start_mcp_gateway.cjs x3, validate_memory_files.cjs). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- eslint-factory/eslint.config.cjs | 1 + eslint-factory/src/index.ts | 2 + .../no-core-error-then-setfailed.test.ts | 73 +++++++++++ .../src/rules/no-core-error-then-setfailed.ts | 124 ++++++++++++++++++ 4 files changed, 200 insertions(+) create mode 100644 eslint-factory/src/rules/no-core-error-then-setfailed.test.ts create mode 100644 eslint-factory/src/rules/no-core-error-then-setfailed.ts 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..25e0eebd294 --- /dev/null +++ b/eslint-factory/src/rules/no-core-error-then-setfailed.test.ts @@ -0,0 +1,73 @@ +import { RuleTester } from "@typescript-eslint/rule-tester"; +import { noCoreErrorThenSetFailedRule } from "./no-core-error-then-setfailed"; + +const ruleTester = new RuleTester(); + +ruleTester.run("no-core-error-then-setfailed", noCoreErrorThenSetFailedRule, { + valid: [ + // Only core.setFailed — no preceding core.error + { + code: `core.setFailed("something went wrong");`, + }, + // Only core.error — no following setFailed + { + code: `core.error("something went wrong");`, + }, + // core.error followed by something other than setFailed + { + code: `core.error("msg"); return;`, + }, + // core.warning followed by core.setFailed is allowed (different method) + { + code: `core.warning("msg"); core.setFailed("msg");`, + }, + // core.error without adjacent setFailed (setFailed is non-adjacent) + { + code: `core.error("msg"); doSomething(); core.setFailed("msg");`, + }, + ], + invalid: [ + // Adjacent core.error then core.setFailed — direct + { + code: `core.error("msg"); core.setFailed("msg");`, + errors: [{ messageId: "noCoreErrorThenSetFailed" }], + }, + // Inside a catch block + { + code: ` + try { + doSomething(); + } catch (err) { + core.error(\`Failed: \${err.message}\`); + core.setFailed(\`ERR: Failed: \${err.message}\`); + } + `, + errors: [{ messageId: "noCoreErrorThenSetFailed" }], + }, + // With an alias (c = core) + { + code: `const c = core; c.error("msg"); c.setFailed("msg");`, + errors: [{ messageId: "noCoreErrorThenSetFailed" }], + }, + // Computed property access + { + code: `core["error"]("msg"); core["setFailed"]("msg");`, + errors: [{ messageId: "noCoreErrorThenSetFailed" }], + }, + // Has suggestion to remove core.error call + { + code: `core.error("msg"); core.setFailed("msg");`, + errors: [ + { + messageId: "noCoreErrorThenSetFailed", + suggestions: [ + { + messageId: "removeErrorCall", + output: ` core.setFailed("msg");`, + }, + ], + }, + ], + }, + ], +}); 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..96ce9e6effa --- /dev/null +++ b/eslint-factory/src/rules/no-core-error-then-setfailed.ts @@ -0,0 +1,124 @@ +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 true when `node` is an expression statement containing a call to + * `core.error(...)` (direct, computed, or aliased). + */ +function isCoreErrorStatement(node: TSESTree.Statement, sourceCode: SourceCode): 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 isErrorNonComputed = !callee.computed && prop.type === AST_NODE_TYPES.Identifier && prop.name === "error"; + const isErrorComputed = callee.computed && prop.type === AST_NODE_TYPES.Literal && prop.value === "error"; + if (!isErrorNonComputed && !isErrorComputed) return false; + if (obj.type !== AST_NODE_TYPES.Identifier) return false; + + return isCoreLikeIdentifier(obj.name) || isCoreAliasIdentifier(obj, sourceCode); +} + +/** + * Returns true when `node` is an expression statement containing a call to + * `core.setFailed(...)` (direct, computed, or aliased). + */ +function isCoreSetFailedStatement(node: TSESTree.Statement, sourceCode: SourceCode): 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 isSetFailedNonComputed = !callee.computed && prop.type === AST_NODE_TYPES.Identifier && prop.name === "setFailed"; + const isSetFailedComputed = callee.computed && prop.type === AST_NODE_TYPES.Literal && prop.value === "setFailed"; + if (!isSetFailedNonComputed && !isSetFailedComputed) return false; + if (obj.type !== AST_NODE_TYPES.Identifier) return false; + + return isCoreLikeIdentifier(obj.name) || isCoreAliasIdentifier(obj, sourceCode); +} + +function hasSingleNonSpreadArgument(call: TSESTree.CallExpression): boolean { + return call.arguments.length === 1 && call.arguments[0].type !== AST_NODE_TYPES.SpreadElement; +} + +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 or similar 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()` 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; + + const errorCall = current.expression as TSESTree.CallExpression; + const safeToFix = hasSingleNonSpreadArgument(errorCall); + + 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) { + const stmts = node.body.filter((s): s is TSESTree.Statement => s.type !== AST_NODE_TYPES.ImportDeclaration && s.type !== AST_NODE_TYPES.ExportAllDeclaration); + checkStatements(stmts); + }, + }; + }, +}); From 84e01ec345107dfcaa611ecfee0347286729b5bc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 27 Jul 2026 12:30:07 +0000 Subject: [PATCH 2/3] fix: address review feedback on no-core-error-then-setfailed rule - Only flag when core.error() and core.setFailed() use provably equivalent message arguments (same source text); different messages are no longer reported since core.error may carry extra diagnostic context - Exclude core.error() calls with annotation properties (second argument) as they provide context not duplicated by setFailed - Restrict auto-remove suggestion to side-effect-free arguments; function call arguments are no longer silently dropped - Replace @typescript-eslint/rule-tester with RuleTester from eslint plus Vitest describe/it, matching the convention of all sibling rules - Update test cases to reflect the new message-equivalence requirement Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .../no-core-error-then-setfailed.test.ts | 142 ++++++++++-------- .../src/rules/no-core-error-then-setfailed.ts | 83 ++++++++-- 2 files changed, 152 insertions(+), 73 deletions(-) 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 index 25e0eebd294..538983a43b8 100644 --- a/eslint-factory/src/rules/no-core-error-then-setfailed.test.ts +++ b/eslint-factory/src/rules/no-core-error-then-setfailed.test.ts @@ -1,73 +1,89 @@ -import { RuleTester } from "@typescript-eslint/rule-tester"; +// 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(); +const ruleTester = new RuleTester({ + languageOptions: { + ecmaVersion: "latest", + sourceType: "module", + }, +}); -ruleTester.run("no-core-error-then-setfailed", noCoreErrorThenSetFailedRule, { - valid: [ - // Only core.setFailed — no preceding core.error - { - code: `core.setFailed("something went wrong");`, - }, - // Only core.error — no following setFailed - { - code: `core.error("something went wrong");`, - }, - // core.error followed by something other than setFailed - { - code: `core.error("msg"); return;`, - }, - // core.warning followed by core.setFailed is allowed (different method) - { - code: `core.warning("msg"); core.setFailed("msg");`, - }, - // core.error without adjacent setFailed (setFailed is non-adjacent) - { - code: `core.error("msg"); doSomething(); core.setFailed("msg");`, - }, - ], - invalid: [ - // Adjacent core.error then core.setFailed — direct - { - code: `core.error("msg"); core.setFailed("msg");`, - errors: [{ messageId: "noCoreErrorThenSetFailed" }], - }, - // Inside a catch block - { - code: ` - try { - doSomething(); - } catch (err) { - core.error(\`Failed: \${err.message}\`); - core.setFailed(\`ERR: Failed: \${err.message}\`); - } - `, - errors: [{ messageId: "noCoreErrorThenSetFailed" }], - }, - // With an alias (c = core) - { - code: `const c = core; c.error("msg"); c.setFailed("msg");`, - errors: [{ messageId: "noCoreErrorThenSetFailed" }], - }, - // Computed property access - { - code: `core["error"]("msg"); core["setFailed"]("msg");`, - errors: [{ messageId: "noCoreErrorThenSetFailed" }], - }, - // Has suggestion to remove core.error call - { - code: `core.error("msg"); core.setFailed("msg");`, - errors: [ +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");`, + ], + 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 { - messageId: "noCoreErrorThenSetFailed", - suggestions: [ + code: `function run() { core.error("fatal"); core.setFailed("fatal"); }`, + errors: [ { - messageId: "removeErrorCall", - output: ` core.setFailed("msg");`, + 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 index 96ce9e6effa..48da21ba5a3 100644 --- a/eslint-factory/src/rules/no-core-error-then-setfailed.ts +++ b/eslint-factory/src/rules/no-core-error-then-setfailed.ts @@ -10,6 +10,54 @@ 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 * `core.error(...)` (direct, computed, or aliased). @@ -52,10 +100,6 @@ function isCoreSetFailedStatement(node: TSESTree.Statement, sourceCode: SourceCo return isCoreLikeIdentifier(obj.name) || isCoreAliasIdentifier(obj, sourceCode); } -function hasSingleNonSpreadArgument(call: TSESTree.CallExpression): boolean { - return call.arguments.length === 1 && call.arguments[0].type !== AST_NODE_TYPES.SpreadElement; -} - export const noCoreErrorThenSetFailedRule = createRule({ name: "no-core-error-then-setfailed", meta: { @@ -65,14 +109,12 @@ export const noCoreErrorThenSetFailedRule = createRule({ 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 or similar message creates a duplicate error annotation " + + "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()` is redundant: `core.setFailed()` already logs " + - "an error annotation and marks the action failed. Remove the `core.error()` call.", + 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.", }, }, @@ -88,8 +130,29 @@ export const noCoreErrorThenSetFailedRule = createRule({ const next = stmts[i + 1]; if (!isCoreSetFailedStatement(next, sourceCode)) continue; - const errorCall = current.expression as TSESTree.CallExpression; - const safeToFix = hasSingleNonSpreadArgument(errorCall); + 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, From 5da2a72e8bdb0179c34e3fb13e5c56cfdf0c1106 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 27 Jul 2026 13:05:01 +0000 Subject: [PATCH 3/3] fix: address github-actions bot review feedback on no-core-error-then-setfailed - Add same-object identifier check: c1.error("x"); c2.setFailed("x") with different aliases no longer triggers a false positive - Fix Program body filter to exclude all module declarations: adds ExportNamedDeclaration and ExportDefaultDeclaration to the exclusion list - Unify isCoreErrorStatement/isCoreSetFailedStatement into a single isCoreMethodCallStatement helper, eliminating the near-identical duplication - Add getCoreObjectName helper to extract receiver name from a matched call - Add test cases: mixed-receiver alias pair (valid) and non-core alias (valid) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../no-core-error-then-setfailed.test.ts | 5 ++ .../src/rules/no-core-error-then-setfailed.ts | 61 +++++++++++-------- 2 files changed, 41 insertions(+), 25 deletions(-) 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 index 538983a43b8..6f0c7044442 100644 --- a/eslint-factory/src/rules/no-core-error-then-setfailed.test.ts +++ b/eslint-factory/src/rules/no-core-error-then-setfailed.test.ts @@ -41,6 +41,11 @@ describe("no-core-error-then-setfailed", () => { }, // 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 diff --git a/eslint-factory/src/rules/no-core-error-then-setfailed.ts b/eslint-factory/src/rules/no-core-error-then-setfailed.ts index 48da21ba5a3..c41188071a6 100644 --- a/eslint-factory/src/rules/no-core-error-then-setfailed.ts +++ b/eslint-factory/src/rules/no-core-error-then-setfailed.ts @@ -33,7 +33,7 @@ function hasAnnotationProperties(call: TSESTree.CallExpression): boolean { /** * Returns true when the expression is provably side-effect-free: no call, - * new, or assignment expression at any nesting level. Conservatively returns + * new, or assignment expression at any nesting level. Conservatively returns * false for any node type not listed here. */ function isSideEffectFree(node: TSESTree.Expression): boolean { @@ -60,9 +60,11 @@ function isSideEffectFree(node: TSESTree.Expression): boolean { /** * Returns true when `node` is an expression statement containing a call to - * `core.error(...)` (direct, computed, or aliased). + * `.(...)` 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 isCoreErrorStatement(node: TSESTree.Statement, sourceCode: SourceCode): node is TSESTree.ExpressionStatement { +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; @@ -71,33 +73,30 @@ function isCoreErrorStatement(node: TSESTree.Statement, sourceCode: SourceCode): const obj = callee.object; const prop = callee.property; - const isErrorNonComputed = !callee.computed && prop.type === AST_NODE_TYPES.Identifier && prop.name === "error"; - const isErrorComputed = callee.computed && prop.type === AST_NODE_TYPES.Literal && prop.value === "error"; - if (!isErrorNonComputed && !isErrorComputed) return false; + 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.name) || isCoreAliasIdentifier(obj, sourceCode); + return isCoreLikeIdentifier((obj as TSESTree.Identifier).name) || isCoreAliasIdentifier(obj as TSESTree.Identifier, sourceCode); } -/** - * Returns true when `node` is an expression statement containing a call to - * `core.setFailed(...)` (direct, computed, or aliased). - */ -function isCoreSetFailedStatement(node: TSESTree.Statement, sourceCode: SourceCode): 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; +function isCoreErrorStatement(node: TSESTree.Statement, sourceCode: SourceCode): node is TSESTree.ExpressionStatement { + return isCoreMethodCallStatement(node, sourceCode, "error"); +} - const obj = callee.object; - const prop = callee.property; - const isSetFailedNonComputed = !callee.computed && prop.type === AST_NODE_TYPES.Identifier && prop.name === "setFailed"; - const isSetFailedComputed = callee.computed && prop.type === AST_NODE_TYPES.Literal && prop.value === "setFailed"; - if (!isSetFailedNonComputed && !isSetFailedComputed) return false; - if (obj.type !== AST_NODE_TYPES.Identifier) return false; +function isCoreSetFailedStatement(node: TSESTree.Statement, sourceCode: SourceCode): node is TSESTree.ExpressionStatement { + return isCoreMethodCallStatement(node, sourceCode, "setFailed"); +} - return isCoreLikeIdentifier(obj.name) || isCoreAliasIdentifier(obj, sourceCode); +/** + * 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({ @@ -130,6 +129,11 @@ export const noCoreErrorThenSetFailedRule = createRule({ 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; @@ -179,7 +183,14 @@ export const noCoreErrorThenSetFailedRule = createRule({ checkStatements(node.consequent); }, Program(node: TSESTree.Program) { - const stmts = node.body.filter((s): s is TSESTree.Statement => s.type !== AST_NODE_TYPES.ImportDeclaration && s.type !== AST_NODE_TYPES.ExportAllDeclaration); + // 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); }, };