Skip to content

eslint-factory: flag core.error + core.setFailed(prefix + msg) as redundant #49496

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 6 commits into from
Aug 1, 2026
+138 −16
Merged

eslint-factory: flag core.error + core.setFailed(prefix + msg) as redundant #49496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
119c4a3
Initial plan
Copilot Aug 1, 2026
efa3729
plan: relax no-core-error-then-setfailed equivalence check for prefix…
Copilot Aug 1, 2026
2e7ecf3
fix: relax no-core-error-then-setfailed to flag prefixed-message pairs
Copilot Aug 1, 2026
286f850
Merge branch 'main' into copilot/fix-eslint-no-core-error
github-actions[bot] Aug 1, 2026
bf40d45
fix: add BinaryExpression prefix support, null-cooked guards, and edg…
Copilot Aug 1, 2026
69163ce
Merge branch 'main' into copilot/fix-eslint-no-core-error
pelikhan Aug 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 42 additions & 11 deletions eslint-factory/src/rules/no-core-error-then-setfailed.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,8 @@ describe("no-core-error-then-setfailed", () => {
`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}\`);
}
`,
},
// Suffix added to setFailed message — not a prefix-match shape, not flagged
"core.error(`Failed: ${msg}`); core.setFailed(`Failed: ${msg} [extra context]`);",
// 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):
Expand Down Expand Up @@ -88,6 +79,46 @@ describe("no-core-error-then-setfailed", () => {
},

@github-actions github-actions Bot Aug 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New prefix-matching branches lack tests for zero-expression error templates and null-cooked quasis, leaving two guard conditions unverified.

💡 Details

isSetFailedArgPrefixedVersion has at least two branches with no covering test case:

  1. errTpl.expressions.length === 0 (a plain literal-only core.error(\Failed`)) paired with a setFailedtemplate that has a prefix expression, e.g.core.setFailed(`${ERR_CODE}: Failed`). This exercises prefixExprCount > 0with the loop overerrTpl.expressionsnever running — a distinct code path from the two invalid tests added, which both useerrTpl.expressions.length === 1`.
  2. The cooked === null guard (if (junctionCooked === null || errFirstCooked === null) return false;) — reachable when a template literal contains an invalid escape sequence — has no test, so a regression that inverts or removes this null check would go undetected.

Adding two more invalid/valid cases would close this gap and lock in the intended behavior for both edge cases.

],
},
// Literal prefix in setFailed template literal — same message with error-code text prefix
{
code: "core.error(`Failed: ${err.message}`); core.setFailed(`ERR: Failed: ${err.message}`);",
errors: [
{
messageId: "noCoreErrorThenSetFailed",
suggestions: [{ messageId: "removeErrorCall", output: " core.setFailed(`ERR: Failed: ${err.message}`);" }],
},
],
},
// Expression prefix in setFailed template literal — mirrors add_reaction.cjs:184-185
{
code: "core.error(`Failed to add reaction: ${errorMessage}`); core.setFailed(`${ERR_API}: Failed to add reaction: ${errorMessage}`);",
errors: [
{
messageId: "noCoreErrorThenSetFailed",
suggestions: [{ messageId: "removeErrorCall", output: " core.setFailed(`${ERR_API}: Failed to add reaction: ${errorMessage}`);" }],
},
],
},
// Expression-first (empty leading quasi) in errorArg — errorArg starts with an expression
{
code: "core.error(`${msg} failed`); core.setFailed(`ERR: ${msg} failed`);",
errors: [
{
messageId: "noCoreErrorThenSetFailed",
suggestions: [{ messageId: "removeErrorCall", output: " core.setFailed(`ERR: ${msg} failed`);" }],
},
],
},
// BinaryExpression concatenation: "prefix" + templateLiteral matching errorArg
{
code: 'core.error(`Failed: ${msg}`); core.setFailed("ERR: " + `Failed: ${msg}`);',
errors: [
{
messageId: "noCoreErrorThenSetFailed",
suggestions: [{ messageId: "removeErrorCall", output: ' core.setFailed("ERR: " + `Failed: ${msg}`);' }],
},
],
},
],
});
});

@github-actions github-actions Bot Aug 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Missing edge-case: errorArg starts with an expression (empty leading quasi). When errTpl.quasis[0].value.cooked === '' and the junction quasi ends with '', endsWith('') is always true, so the prefix guard at line 85 becomes the only safeguard. Add a test to document the expected behaviour:

💡 Suggested test case 
// Expression-first error arg with expression prefix in setFailed
{
  code: 'core.error(`${msg}`); core.setFailed(`ERR: ${msg}`);',
  errors: [{ messageId: 'noCoreErrorThenSetFailed', ... }],
},

This exercises the empty-leading-quasi path and confirms that prefixExprCount > 0 correctly drives the non-empty-prefix guard when errFirstCooked === ''.

@copilot please address this.

Expand Down
101 changes: 96 additions & 5 deletions eslint-factory/src/rules/no-core-error-then-setfailed.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,88 @@ function hasAnnotationProperties(call: TSESTree.CallExpression): boolean {
return call.arguments.length > 1;
}

/**
* Returns true when `setFailedArg` is a TemplateLiteral whose content ends
* with the full content of `errorArg` (also a TemplateLiteral), with a
* non-empty prefix prepended. The prefix may contain template expressions.
* This captures the common pattern:
* core.error(`Failed: ${msg}`)
* core.setFailed(`${ERR_CODE}: Failed: ${msg}`)
* where setFailed emits the same annotation text with only an error-code
* prefix added in front.
*
* A match requires:
* 1. Both args are TemplateLiterals.
* 2. setFailed has at least as many expressions as error.
* 3. The last N expressions of setFailed match error's expressions in
* order (same source text), where N = error.expressions.length.
* 4. The quasis of setFailed at offsets [P+1 .. end] (P = prefix expr
* count) equal error's quasis at [1 .. end] (same cooked value).
* 5. The "junction" quasi in setFailed (at offset P) ends with error's
* first quasi (cooked value).
* 6. The prefix is non-empty (to avoid re-matching exact-equal pairs).
*/
function isSetFailedArgPrefixedVersion(errorArg: TSESTree.Expression, setFailedArg: TSESTree.Expression, sourceCode: SourceCode): boolean {
if (errorArg.type !== AST_NODE_TYPES.TemplateLiteral) return false;
if (setFailedArg.type !== AST_NODE_TYPES.TemplateLiteral) return false;
Comment on lines +55 to +57

const errTpl = errorArg as TSESTree.TemplateLiteral;
const sfTpl = setFailedArg as TSESTree.TemplateLiteral;

if (sfTpl.expressions.length < errTpl.expressions.length) return false;

const prefixExprCount = sfTpl.expressions.length - errTpl.expressions.length;

// Last errTpl.expressions.length expressions of sfTpl must match errTpl's expressions.
for (let i = 0; i < errTpl.expressions.length; i++) {
const sfExpr = sfTpl.expressions[prefixExprCount + i] as TSESTree.Expression;

@github-actions github-actions Bot Aug 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expression-prefix matching relies purely on textual source comparison, which can be fooled by non-obvious cases.

💡 Details

Both exactMatch and isSetFailedArgPrefixedVersion compare sourceCode.getText(expr) for equality. This was already the approach for the exact-match case, but the new prefix-matching path widens the blast radius: it now also compares multiple expressions per call pair (all of errTpl.expressions) and glues cooked quasi text together, so more real-world call sites will be auto-matched and their core.error() calls silently removed by the removeErrorCall suggestion.

Textual equality does not guarantee semantic equality — e.g. a shadowed err in nested scope, or an expression whose evaluation has a side effect visible only through equality-of-text but not equality-of-runtime-value (rare, but the existing isSideEffectFree check only protects against side effects in the removed call, not against divergent runtime values between the two call sites).

Suggested hardening: since this is a suggestion (not autofix), reviewers already vet it manually, so the risk is moderate rather than critical. Still worth documenting this limitation explicitly in the function doc comment so future maintainers don't assume textual equality implies semantic equality.

const errExpr = errTpl.expressions[i] as TSESTree.Expression;
if (sourceCode.getText(sfExpr) !== sourceCode.getText(errExpr)) return false;
}

// Quasis at offsets [prefixExprCount+1 .. end] of sfTpl must equal errTpl quasis [1 .. end].
// Guard against null cooked values (produced by invalid Unicode escape sequences).
for (let i = 1; i < errTpl.quasis.length; i++) {
const sfCooked = sfTpl.quasis[prefixExprCount + i].value.cooked;
const errCooked = errTpl.quasis[i].value.cooked;
if (sfCooked === null || errCooked === null) return false;
if (sfCooked !== errCooked) return false;
}

// The junction quasi (sfTpl.quasis[prefixExprCount]) must end with errTpl's first quasi.
const junctionCooked = sfTpl.quasis[prefixExprCount].value.cooked;
const errFirstCooked = errTpl.quasis[0].value.cooked;
if (junctionCooked === null || errFirstCooked === null) return false;
if (!junctionCooked.endsWith(errFirstCooked)) return false;

// There must be an actual prefix (not an exact-equal pair).
return prefixExprCount > 0 || junctionCooked.length > errFirstCooked.length;
}

/**
* Returns true when `setFailedArg` is a BinaryExpression of the form
* `<string> + <expr>` where the string literal is non-empty (the prefix)
* and the right operand is identical in source text to `errorArg`.
* This captures:
* core.error(`Failed: ${msg}`)
* core.setFailed("ERR: " + `Failed: ${msg}`)
*/
function isSetFailedArgStringConcatPrefixedVersion(errorArg: TSESTree.Expression, setFailedArg: TSESTree.Expression, sourceCode: SourceCode): boolean {
if (setFailedArg.type !== AST_NODE_TYPES.BinaryExpression) return false;
const be = setFailedArg as TSESTree.BinaryExpression;
if (be.operator !== "+") return false;

// Left side must be a non-empty string literal (the prefix).
const left = be.left as TSESTree.Expression;
if (left.type !== AST_NODE_TYPES.Literal) return false;
if (typeof (left as TSESTree.Literal).value !== "string") return false;
if ((left as TSESTree.Literal).value === "") return false;

// Right side must be identical in source to errorArg.
const right = be.right as TSESTree.Expression;
return sourceCode.getText(right) === sourceCode.getText(errorArg);
}

/**
* Returns true when the expression is provably side-effect-free: no call,
* new, or assignment expression at any nesting level. Conservatively returns
Expand Down Expand Up @@ -142,14 +224,23 @@ export const noCoreErrorThenSetFailedRule = createRule({
// 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.
// Only report when the message arguments are provably equivalent.
// Two shapes are recognised:
// 1. Exact source-text match — the classic identical-message case.
// 2. Prefixed match — setFailedArg is a TemplateLiteral whose
// content ends with the full content of errorArg (also a
// TemplateLiteral), with only a non-empty prefix added in front
// (e.g. `${ERR_CODE}: ${msg}` vs `${msg}`). This is the dominant
// anti-pattern in the codebase: core.error duplicates the same
// annotation that core.setFailed already emits, just without the
// leading error-code prefix.
// Calls where setFailed adds suffix or interleaved context are NOT
// flagged, since they may carry information not duplicated by setFailed.
const errorArg = getFirstNonSpreadArg(errorCall);
const setFailedArg = getFirstNonSpreadArg(setFailedCall);
if (errorArg === null || setFailedArg === null) continue;
if (sourceCode.getText(errorArg) !== sourceCode.getText(setFailedArg)) continue;
const exactMatch = sourceCode.getText(errorArg) === sourceCode.getText(setFailedArg);
if (!exactMatch && !isSetFailedArgPrefixedVersion(errorArg, setFailedArg, sourceCode) && !isSetFailedArgStringConcatPrefixedVersion(errorArg, setFailedArg, sourceCode)) continue;

// The auto-remove suggestion is semantics-preserving only when the shared
// argument is provably side-effect-free. For example,
Expand Down