[eslint-miner] eslint: add no-core-error-then-setfailed rule

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",
     },
   },
   {

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,
   },
 };
 

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"); }` }],
+            },
+          ],
+        },
+      ],
+    });
+  });
+});

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<typeof isCoreAliasIdentifier>[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
+ * `<coreObj>.<methodName>(...)` 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);
+      },
+    };
+  },
+});