eslint: no-core-error-then-process-exitcode — forward scan + top-level autofix #47240
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,6 +83,56 @@ function isProcessExitCodeNonZero(node: TSESTree.Statement): node is TSESTree.Ex | |
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Returns true when `node` is an expression statement containing a call to | ||
| * `core.setFailed(...)` (direct, computed, or aliased). | ||
| * Accepts `sourceCode` for alias resolution via `isCoreAliasIdentifier`; contrast | ||
| * with `isControlTransferStatement` which is a pure syntax check and needs no source-code context. | ||
| */ | ||
| function isCoreSetFailedStatement(node: TSESTree.Statement, sourceCode: SourceCode): boolean { | ||
| 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); | ||
| } | ||
|
|
||
| /** | ||
| * Returns true when `node` is a control-transfer statement that definitively | ||
| * exits the current block: return, throw, break, continue, or process.exit(...). | ||
| */ | ||
| function isControlTransferStatement(node: TSESTree.Statement): boolean { | ||
| // prettier-ignore | ||
| if ( | ||
| node.type === AST_NODE_TYPES.ReturnStatement || | ||
| node.type === AST_NODE_TYPES.ThrowStatement || | ||
| node.type === AST_NODE_TYPES.BreakStatement || | ||
| node.type === AST_NODE_TYPES.ContinueStatement | ||
| ) { | ||
| return true; | ||
| } | ||
| // process.exit(...) — any call, regardless of exit code; handles both dot and computed access | ||
| if (node.type === AST_NODE_TYPES.ExpressionStatement && node.expression.type === AST_NODE_TYPES.CallExpression) { | ||
| const callee = node.expression.callee; | ||
| if (callee.type === AST_NODE_TYPES.MemberExpression && callee.object.type === AST_NODE_TYPES.Identifier && callee.object.name === "process") { | ||
| const prop = callee.property; | ||
| const isExitDot = !callee.computed && prop.type === AST_NODE_TYPES.Identifier && prop.name === "exit"; | ||
| const isExitComputed = callee.computed && prop.type === AST_NODE_TYPES.Literal && prop.value === "exit"; | ||
| if (isExitDot || isExitComputed) return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| function hasSingleNonSpreadArgument(call: TSESTree.CallExpression): boolean { | ||
| return call.arguments.length === 1 && call.arguments[0].type !== AST_NODE_TYPES.SpreadElement; | ||
| } | ||
|
|
@@ -98,51 +148,84 @@ export const noCoreErrorThenProcessExitCodeRule = createRule({ | |
| hasSuggestions: true, | ||
| docs: { | ||
| description: | ||
| "Disallow the pattern `core.error(msg); ... ; process.exitCode = nonzero` in GitHub Actions scripts. " + | ||
| "`core.error()` annotates the log but does not mark the action as failed. " + | ||
| "Prefer `core.setFailed(msg)` which correctly marks the action as failed and allows post-action " + | ||
| "cleanup hooks to run. Unlike `process.exit(1)`, `process.exitCode = 1` does not immediately halt " + | ||
| "execution, so subsequent code still runs in the failed state. " + | ||
| "The rule scans forward from `core.error(...)` for a later `process.exitCode = nonzero`, " + | ||
| "stopping at `core.setFailed(...)` or a control-transfer statement.", | ||
| }, | ||
| schema: [], | ||
| messages: { | ||
| noCoreErrorThenProcessExitCode: | ||
| "Avoid `core.error()` followed by `process.exitCode = nonzero`. Prefer `core.setFailed(msg)` to signal " + | ||
| "action failure; it marks the action failed and allows post-action cleanup hooks to run. " + | ||
| "Unlike `process.exit(1)`, `process.exitCode = 1` does not halt execution immediately.", | ||
| replaceWithSetFailed: "Replace `core.error(msg); process.exitCode = nonzero` with `core.setFailed(msg)` (at module top level) or `core.setFailed(msg); return;` (inside main()).", | ||
| }, | ||
| }, | ||
| defaultOptions: [], | ||
| create(context) { | ||
| const sourceCode = context.sourceCode; | ||
|
|
||
| function checkStatements(stmts: readonly TSESTree.Statement[]): void { | ||
| // Track which process.exitCode nodes have already been reported so that two consecutive | ||
| // core.error() calls before the same exitCode do not each fire their own diagnostic | ||
| // (which could produce conflicting autofixers on the same node). | ||
| const reported = new WeakSet<TSESTree.Statement>(); | ||
| for (let i = 0; i < stmts.length - 1; i++) { | ||
| const current = stmts[i]; | ||
| if (!isCoreErrorStatement(current, sourceCode)) continue; | ||
|
|
||
| // Scan forward for process.exitCode = nonzero, stopping at setFailed or control-transfer. | ||
| // Adjacent (j === i+1) keeps autofix; non-adjacent reports without suggestion. | ||
| for (let j = i + 1; j < stmts.length; j++) { | ||
|
Contributor
There was a problem hiding this comment. Potential duplicate reports when two The inner core.error("a"); core.error("b"); process.exitCode = 1;both Consider tracking already-reported @copilot please address this.
Contributor
Author
There was a problem hiding this comment. Fixed in 069ebdb. Added a
Contributor
There was a problem hiding this comment. [/tdd] Two consecutive 💡 Scenario + suggested fixcore.error("a");
core.error("b");
process.exitCode = 1;The outer Track which exitCode nodes have already been claimed, or add a test that documents the intended behavior for this pattern: const claimedExitCodes = new Set<TSESTree.Statement>();
// inside the j loop, before reporting:
if (claimedExitCodes.has(candidate)) break;
claimedExitCodes.add(candidate);@copilot please address this.
Contributor
Author
There was a problem hiding this comment. Fixed in 069ebdb. Added the |
||
| const candidate = stmts[j]; | ||
|
|
||
| if (isProcessExitCodeNonZero(candidate)) { | ||
| if (!reported.has(candidate)) { | ||
| reported.add(candidate); | ||
| const isAdjacent = j === i + 1; | ||
| // The autofix suggestion is only safe when the pair is at module top level or directly | ||
| // inside a `main()` entrypoint. Inside helper functions, `return;` only exits the helper | ||
| // and lets the caller continue. For non-adjacent pairs we omit the suggestion to avoid | ||
| // a fixer that leaves intervening statements between a deleted exitCode and the new setFailed. | ||
| const enclosingFn = getImmediateEnclosingFunction(current, sourceCode); | ||
| const errorCall = current.expression as TSESTree.CallExpression; | ||
| const safeToFix = isAdjacent && (enclosingFn === null || isFunctionNamedMain(enclosingFn)) && hasSingleNonSpreadArgument(errorCall); | ||
|
|
||
| context.report({ | ||
| node: current, | ||
| messageId: "noCoreErrorThenProcessExitCode", | ||
| suggest: safeToFix | ||
| ? [ | ||
| { | ||
| messageId: "replaceWithSetFailed", | ||
| fix(fixer: TSESLint.RuleFixer) { | ||
| const args = errorCall.arguments.map(a => sourceCode.getText(a)).join(", "); | ||
| const callee = errorCall.callee as TSESTree.MemberExpression; | ||
| const objectName = sourceCode.getText(callee.object); | ||
|
|
||
| // At module top-level (enclosingFn === null) there is nothing to `return` from, | ||
| // so we just replace with setFailed. Inside main() we append `return;` to exit | ||
| // the entrypoint cleanly. | ||
| const replacement = enclosingFn !== null ? `${objectName}.setFailed(${args}); return;` : `${objectName}.setFailed(${args});`; | ||
|
|
||
| return [fixer.replaceText(current, replacement + "\n"), fixer.remove(candidate)]; | ||
| }, | ||
| }, | ||
| ] | ||
| : [], | ||
| }); | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| // Stop scanning if setFailed already handles the failure or a control-transfer exits the block. | ||
|
Contributor
There was a problem hiding this comment. [/tdd] The autofix fixer removes 💡 Test scaffold{
// Two core.error() calls both adjacent to the same process.exitCode
// — expected: one report per core.error(), only the closest one gets autofix
code: `core.error("a"); core.error("b"); process.exitCode = 1;`,
errors: [
{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [] },
{ messageId: "noCoreErrorThenProcessExitCode", suggestions: [{ messageId: "replaceWithSetFailed", output: "..." }] },
],
}@copilot please address this.
Contributor
Author
There was a problem hiding this comment. Added in 069ebdb. The new invalid test case in the test file documents the deduplication invariant: |
||
| if (isCoreSetFailedStatement(candidate, sourceCode) || isControlTransferStatement(candidate)) { | ||
| break; | ||
|
Contributor
There was a problem hiding this comment. Consecutive 💡 Details and suggested fixExample that currently produces two separate reports: core.error("a");
core.error("b");
process.exitCode = 1; // reported by BOTH i=0 and i=1When Simplest fix — deduplicate by tracking already-reported function checkStatements(stmts: readonly TSESTree.Statement[]): void {
const reported = new WeakSet<TSESTree.Statement>();
for (let i = 0; i < stmts.length - 1; i++) {
...
if (isProcessExitCodeNonZero(candidate)) {
if (!reported.has(candidate)) {
reported.add(candidate);
context.report({ ... });
}
break;
}
}
} |
||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -155,13 +238,19 @@ export const noCoreErrorThenProcessExitCodeRule = createRule({ | |
| checkStatements(node.consequent); | ||
| }, | ||
| Program(node: TSESTree.Program) { | ||
| // At module top level, export declarations act as segment boundaries (they separate | ||
| // "regions" of the module). We split the program body at export/import declarations | ||
| // so that an export between core.error and process.exitCode breaks the scan. | ||
| let segment: TSESTree.Statement[] = []; | ||
| for (const stmt of node.body) { | ||
| if (isProgramStatement(stmt)) { | ||
| segment.push(stmt); | ||
| } else { | ||
| checkStatements(segment); | ||
| segment = []; | ||
| } | ||
| } | ||
| checkStatements(segment); | ||
| }, | ||
| }; | ||
| }, | ||
|
|
||
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.
[/codebase-design]
isCoreSetFailedStatementacceptssourceCode: SourceCodesolely for the alias check, whileisControlTransferStatementtakes no such parameter. The asymmetry is fine functionally, but a brief comment noting why the signatures differ (alias resolution vs. pure syntax) would help the next reader who wonders if the omission was intentional.@copilot please address this.
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.
Fixed in 069ebdb. Updated the JSDoc for
isCoreSetFailedStatementto note: "AcceptssourceCodefor alias resolution viaisCoreAliasIdentifier; contrast withisControlTransferStatementwhich is a pure syntax check and needs no source-code context."