eslint-factory: detect aliased/destructured/computed core.setFailed in require-return-after-core-setfailed #45334
+191
−18
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,14 @@ | ||
| import { AST_NODE_TYPES, AST_TOKEN_TYPES, ESLintUtils, TSESLint, TSESTree } from "@typescript-eslint/utils"; | ||
|
|
||
| const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); | ||
|
|
||
| // Known @actions/core binding names (same allow-list as require-await-core-summary-write). | ||
| // Only exact known aliases are matched — broad prefix matching (e.g. `/^core/i`) would | ||
| // silently flag unrelated objects that happen to start with "core". | ||
|
Contributor
There was a problem hiding this comment. [/tdd] @copilot please address this. |
||
| const CORE_ALIASES = new Set(["core", "coreObj"]); | ||
|
|
||
| function isCoreLikeIdentifier(name: string): boolean { | ||
| return CORE_ALIASES.has(name); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -51,10 +46,10 @@ function isControlTransfer(node: TSESTree.Statement): boolean { | |
| * Checks a list of sequential statements for `core.setFailed(...)` calls that | ||
| * are not immediately followed by a control-transfer statement. | ||
| */ | ||
| function checkStatementList(stmts: TSESTree.Statement[], isSetFailed: (node: TSESTree.Statement) => boolean, report: (node: TSESTree.Statement, next: TSESTree.Statement) => void): void { | ||
| for (let i = 0; i < stmts.length; i++) { | ||
| const stmt = stmts[i]; | ||
| if (!isSetFailed(stmt)) continue; | ||
| const next = stmts[i + 1]; | ||
| if (next && !isControlTransfer(next)) { | ||
| report(stmt, next); | ||
|
|
@@ -169,6 +164,95 @@ export const requireReturnAfterCoreSetFailedRule = createRule({ | |
| create(context) { | ||
| const sourceCode = context.sourceCode; | ||
|
|
||
| /** | ||
| * Checks whether an Identifier is a single-assignment alias for a core-like | ||
| * object (e.g., `const c = core`). Re-assigned let bindings are rejected. | ||
| * Local shadows (e.g., a parameter also named `c`) are excluded because they | ||
| * are found first in the scope chain and their definition type will not match. | ||
| */ | ||
| function isCoreAliasIdentifier(identifier: TSESTree.Identifier): boolean { | ||
| let currentScope: TSESLint.Scope.Scope | null = sourceCode.getScope(identifier); | ||
| while (currentScope !== null) { | ||
| const variable = currentScope.set.get(identifier.name); | ||
| if (variable !== undefined) { | ||
| if (variable.defs.length !== 1) return false; | ||
| const def = variable.defs[0]; | ||
| if (def.type !== "Variable") return false; | ||
| if (variable.references.some(ref => ref.isWrite() && !ref.init)) return false; | ||
| const declarator = def.node as TSESTree.VariableDeclarator; | ||
| if (!declarator.init) return false; | ||
| return declarator.id.type === AST_NODE_TYPES.Identifier && declarator.init.type === AST_NODE_TYPES.Identifier && isCoreLikeIdentifier(declarator.init.name); | ||
| } | ||
| currentScope = currentScope.upper; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Checks whether an Identifier is the destructured `setFailed` binding from | ||
| * a core-like object (e.g., `const { setFailed } = core` or | ||
| * `const { setFailed: sf } = core` where `sf` is the identifier). | ||
|
Contributor
There was a problem hiding this comment. [/codebase-design] 💡 Suggested refactorfunction resolveVariableDef(identifier: TSESTree.Identifier): TSESTree.VariableDeclarator | null {
let scope: TSESLint.Scope.Scope | null = sourceCode.getScope(identifier);
while (scope !== null) {
const variable = scope.set.get(identifier.name);
if (variable !== undefined) {
if (variable.defs.length !== 1) return null;
const def = variable.defs[0];
if (def.type !== "Variable") return null;
if (variable.references.some(ref => ref.isWrite() && !ref.init)) return null;
const declarator = def.node as TSESTree.VariableDeclarator;
return declarator.init ? declarator : null;
}
scope = scope.upper;
}
return null;
}Both helpers then call @copilot please address this. |
||
| * Re-assigned let bindings are rejected. Local `function setFailed()` or | ||
| * parameter shadows are excluded via the `def.type !== "Variable"` guard. | ||
| */ | ||
| function isDestructuredSetFailedIdentifier(identifier: TSESTree.Identifier): boolean { | ||
| let currentScope: TSESLint.Scope.Scope | null = sourceCode.getScope(identifier); | ||
| while (currentScope !== null) { | ||
| const variable = currentScope.set.get(identifier.name); | ||
| if (variable !== undefined) { | ||
| if (variable.defs.length !== 1) return false; | ||
| const def = variable.defs[0]; | ||
| if (def.type !== "Variable") return false; | ||
| if (variable.references.some(ref => ref.isWrite() && !ref.init)) return false; | ||
| const declarator = def.node as TSESTree.VariableDeclarator; | ||
| if (!declarator.init) return false; | ||
| if (declarator.id.type === AST_NODE_TYPES.ObjectPattern && declarator.init.type === AST_NODE_TYPES.Identifier && isCoreLikeIdentifier(declarator.init.name)) { | ||
| return declarator.id.properties.some(prop => { | ||
| if (prop.type !== AST_NODE_TYPES.Property || prop.computed) return false; | ||
| const keyIsSetFailed = prop.key.type === AST_NODE_TYPES.Identifier && prop.key.name === "setFailed"; | ||
| const valueIsAlias = prop.value.type === AST_NODE_TYPES.Identifier && prop.value.name === identifier.name; | ||
| return keyIsSetFailed && valueIsAlias; | ||
| }); | ||
| } | ||
| return false; | ||
| } | ||
| currentScope = currentScope.upper; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Returns true when the statement is a call to core.setFailed(...) in any | ||
| * recognized form: | ||
| * - Direct non-computed: core.setFailed(...) | ||
| * - Computed string literal: core["setFailed"](...) | ||
| * - Aliased object: const c = core; c.setFailed(...) | ||
| * - Destructured binding: const { setFailed } = core; setFailed(...) | ||
| */ | ||
| function isCoreSetFailedStatement(node: TSESTree.Statement): 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) { | ||
| const obj = callee.object; | ||
| const prop = callee.property; | ||
| const isNonComputedSetFailed = !callee.computed && prop.type === AST_NODE_TYPES.Identifier && prop.name === "setFailed"; | ||
| const isComputedSetFailed = callee.computed && prop.type === AST_NODE_TYPES.Literal && prop.value === "setFailed"; | ||
| if ((isNonComputedSetFailed || isComputedSetFailed) && obj.type === AST_NODE_TYPES.Identifier) { | ||
| if (isCoreLikeIdentifier(obj.name)) return true; | ||
| if (isCoreAliasIdentifier(obj)) return true; | ||
| } | ||
| } | ||
|
|
||
| if (callee.type === AST_NODE_TYPES.Identifier) { | ||
| if (isDestructuredSetFailedIdentifier(callee)) return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| function isInsideFunctionLike(node: TSESTree.Node): boolean { | ||
| const ancestors = sourceCode.getAncestors(node); | ||
| for (let i = ancestors.length - 1; i >= 0; i--) { | ||
|
|
@@ -246,7 +330,7 @@ export const requireReturnAfterCoreSetFailedRule = createRule({ | |
| return { | ||
| // Check statement blocks: if body, else body, while body, function body, etc. | ||
| BlockStatement(node: TSESTree.BlockStatement) { | ||
| checkStatementList(node.body, isCoreSetFailedStatement, report); | ||
|
|
||
| // Cross-block fall-through: when core.setFailed() is the last statement of | ||
| // this block, check whether any enclosing block has subsequent statements. | ||
|
|
@@ -263,10 +347,10 @@ export const requireReturnAfterCoreSetFailedRule = createRule({ | |
| // Handle single-statement arrow functions (no braces) — rare but safe to skip | ||
| // The main case is BlockStatement above. | ||
| SwitchCase(node: TSESTree.SwitchCase) { | ||
| checkStatementList(node.consequent, isCoreSetFailedStatement, report); | ||
| }, | ||
| Program(node: TSESTree.Program) { | ||
| checkStatementList(node.body.filter(isExecutableStatement), isCoreSetFailedStatement, report); | ||
| }, | ||
| }; | ||
| }, | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
[/tdd] The re-assigned binding guard (
variable.references.some(ref => ref.isWrite() && !ref.init)) is untested. A test withlet c = core; c = other;would confirm the rule does NOT flag it.💡 Suggested test
Without this test, a future refactor that accidentally drops the re-assignment guard would pass all existing tests.
@copilot please address this.