-
Notifications
You must be signed in to change notification settings - Fork 448
fix(eslint): detect dropped await on local core.summary aliases #43404
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.
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
Changes from all commits
7dbd123
952f20d
3383f46
0327783
bf628b7
72d11d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,6 +129,67 @@ describe("require-await-core-summary-write", () => { | |
| }); | ||
| }); | ||
|
|
||
| it("invalid: local alias `const summary = core.summary` is flagged", () => { | ||
| cjsRuleTester.run("require-await-core-summary-write", requireAwaitCoreSummaryWriteRule, { | ||
| valid: [ | ||
| // Awaited alias calls are safe | ||
| `async function f() { const summary = core.summary; await summary.write(); }`, | ||
| // Re-assigned let — alias source is unknown after reassignment; not flagged | ||
| `async function f() { let summary = core.summary; summary = other; summary.write(); }`, | ||
| // Non-core initializer — not a core.summary alias | ||
| `async function f() { const summary = fs.summary; summary.write(); }`, | ||
| // Alias without .write() — addRaw alone must NOT be flagged | ||
| `async function f() { const { summary } = core; summary.addRaw("x"); }`, | ||
| // Chained init: core.summary.addRaw(x) still returns Summary — alias is valid; awaited call is safe | ||
| `async function f() { const s = core.summary.addRaw("x"); await s.write(); }`, | ||
| // Initializer is core.summary.write() — write() returns Promise<Summary>, not Summary; not treated as alias | ||
| `async function f() { const s = core.summary.write(); s.write(); }`, | ||
| ], | ||
| invalid: [ | ||
| // Corpus pattern 1: const summary = core.summary; summary.write(); | ||
| { | ||
| code: `async function f() { const summary = core.summary; summary.write(); }`, | ||
| errors: [{ messageId: "requireAwait", suggestions: [{ messageId: "addAwait", output: `async function f() { const summary = core.summary; await summary.write(); }` }] }], | ||
| }, | ||
| // let variant (e.g. check_workflow_timestamp_api.cjs) | ||
| { | ||
| code: `async function f() { let summary = core.summary; summary.write(); }`, | ||
| errors: [{ messageId: "requireAwait", suggestions: [{ messageId: "addAwait", output: `async function f() { let summary = core.summary; await summary.write(); }` }] }], | ||
| }, | ||
| // Corpus pattern 2: const { summary } = core; summary.addRaw(x).write(); | ||
| { | ||
| code: `async function f() { const { summary } = core; summary.addRaw(x).write(); }`, | ||
| errors: [{ messageId: "requireAwait", suggestions: [{ messageId: "addAwait", output: `async function f() { const { summary } = core; await summary.addRaw(x).write(); }` }] }], | ||
| }, | ||
| // Destructure with rename: const { summary: s } = core; s.write(); | ||
| { | ||
| code: `async function f() { const { summary: s } = core; s.write(); }`, | ||
| errors: [{ messageId: "requireAwait", suggestions: [{ messageId: "addAwait", output: `async function f() { const { summary: s } = core; await s.write(); }` }] }], | ||
| }, | ||
| // coreObj alias: const summary = coreObj.summary; summary.write(); | ||
| { | ||
| code: `async function f() { const summary = coreObj.summary; summary.write(); }`, | ||
| errors: [{ messageId: "requireAwait", suggestions: [{ messageId: "addAwait", output: `async function f() { const summary = coreObj.summary; await summary.write(); }` }] }], | ||
| }, | ||
| // coreObj destructuring: const { summary } = coreObj; summary.write(); | ||
| { | ||
| code: `async function f() { const { summary } = coreObj; summary.write(); }`, | ||
| errors: [{ messageId: "requireAwait", suggestions: [{ messageId: "addAwait", output: `async function f() { const { summary } = coreObj; await summary.write(); }` }] }], | ||
| }, | ||
| // Cross-scope: alias declared in outer scope, write() called inside inner async function | ||
| { | ||
| code: `async function outer() {\n const summary = core.summary;\n return (async function inner() { summary.write(); })();\n}`, | ||
| errors: [{ messageId: "requireAwait", suggestions: [{ messageId: "addAwait", output: `async function outer() {\n const summary = core.summary;\n return (async function inner() { await summary.write(); })();\n}` }] }], | ||
| }, | ||
| // Outside async: flagged, no suggestion | ||
| { | ||
| code: `function f() { const summary = core.summary; summary.write(); }`, | ||
| errors: [{ messageId: "requireAwait", suggestions: [] }], | ||
| }, | ||
| ], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] No test verifies what happens when the destructured alias is called directly without 💡 Suggested test// Using alias for addRaw only (no write) — must NOT be flagged
`async function f() { const { summary } = core; summary.addRaw("x"); }`@copilot please address this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 72d11d2. Added a valid test: |
||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new test block covers {
code: `async function f() { const summary = coreObj.summary; summary.write(); }`,
errors: [{ messageId: "requireAwait", suggestions: [{ messageId: "addAwait", output: `async function f() { const summary = coreObj.summary; await summary.write(); }` }] }],
},would guard against a future regression there. @copilot please address this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 72d11d2. Added an invalid test for the |
||
| }); | ||
|
|
||
| it("suggestion: inserts 'await ' before the expression", () => { | ||
| cjsRuleTester.run("require-await-core-summary-write", requireAwaitCoreSummaryWriteRule, { | ||
| valid: [], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,6 +92,88 @@ export const requireAwaitCoreSummaryWriteRule = createRule({ | |
| }, | ||
| defaultOptions: [], | ||
| create(context) { | ||
| /** | ||
| * Checks whether an Identifier in the current scope is a single-assignment | ||
| * local alias for `core.summary` (or any CORE_ALIASES member's `.summary`). | ||
| * | ||
| * Accepted initializer patterns: | ||
| * - `const/let summaryVar = core.summary;` | ||
| * - `const { summary } = core;` or `const { summary: alias } = core;` | ||
| * | ||
| * Re-assigned `let` bindings are rejected (conservative: source unknown after | ||
| * reassignment). | ||
| */ | ||
| function isCoreSummaryAlias(identifier: TSESTree.Identifier): boolean { | ||
| let currentScope: TSESLint.Scope.Scope | null = context.sourceCode.getScope(identifier); | ||
| while (currentScope !== null) { | ||
| const variable = currentScope.set.get(identifier.name); | ||
| if (variable !== undefined) { | ||
| // Only handle single-declaration bindings (no overloads / duplicate lets). | ||
| if (variable.defs.length !== 1) return false; | ||
| const def = variable.defs[0]; | ||
|
|
||
| // Must be a VariableDeclarator (not a function parameter, import, etc.). | ||
| if (def.type !== "Variable") return false; | ||
|
|
||
| // Reject let bindings that are re-assigned after initialisation. | ||
| // ref.init === true marks the write that comes from the initialiser itself. | ||
| if (variable.references.some(ref => ref.isWrite() && !ref.init)) return false; | ||
|
|
||
| const declarator = def.node as TSESTree.VariableDeclarator; | ||
| if (!declarator.init) return false; | ||
|
|
||
| // Pattern 1: const/let summaryVar = core.summary; | ||
| // rootsSummary covers `core.summary` and chained inits like | ||
| // `core.summary.addRaw(x)` — addRaw returns Summary, so the alias | ||
| // still wraps a Summary object. | ||
| // .write() is explicitly rejected: write() returns Promise<Summary>, | ||
| // not Summary, so `const s = core.summary.write()` must not be | ||
| // treated as a Summary alias. | ||
| if (declarator.id.type === "Identifier") { | ||
| if (declarator.init.type === "CallExpression" && declarator.init.callee.type === "MemberExpression" && isWriteProperty(declarator.init.callee)) return false; | ||
| return rootsSummary(declarator.init); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnosing-bugs] Pattern 1 in 💡 Suggested clarifying comment + optional testAdd to the existing comment block: // Note: rootsSummary also accepts chained initialisers like `core.summary.addRaw(x)`
// because addRaw returns Summary — the alias still wraps a Summary object.Optional extra valid-case test: `async function f() { const s = core.summary.addRaw("x"); await s.write(); }`@copilot please address this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in 72d11d2. Pattern 1 now has a comment block explicitly documenting that |
||
| } | ||
|
|
||
| // Pattern 2: const { summary } = core; or const { summary: alias } = core; | ||
| if (declarator.id.type === "ObjectPattern" && declarator.init.type === "Identifier" && isCoreLikeIdentifier(declarator.init.name)) { | ||
| return declarator.id.properties.some(prop => { | ||
| if (prop.type !== "Property" || prop.computed) return false; | ||
| const keyIsSummary = prop.key.type === "Identifier" && prop.key.name === "summary"; | ||
| const valueIsAlias = prop.value.type === "Identifier" && prop.value.name === identifier.name; | ||
| return keyIsSummary && valueIsAlias; | ||
| }); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| currentScope = currentScope.upper; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Extended version of rootsSummary that also handles local aliases of | ||
| * `core.summary` (e.g. `const summary = core.summary; summary.write()`). | ||
| * The depth guard prevents unbounded recursion on pathologically deep | ||
| * call chains (> 32 levels); real source code is well below this limit. | ||
| */ | ||
| function rootsSummaryOrAlias(node: TSESTree.Node, depth = 0): boolean { | ||
| if (depth > 32) return false; | ||
| // Fast path: already handles core.summary and chained core.summary.*() calls. | ||
| if (rootsSummary(node)) return true; | ||
|
|
||
| // Direct alias: `summary.write()` — callee.object is an Identifier. | ||
| if (node.type === "Identifier") return isCoreSummaryAlias(node); | ||
|
|
||
| // Chained alias: `summary.addRaw(x).write()` — callee.object is a CallExpression | ||
| // rooted on the alias identifier. | ||
| if (node.type === "CallExpression" && node.callee.type === "MemberExpression") { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnosing-bugs] 💡 Note on risk and mitigationIn practice, ESLint AST depths for real source code are shallow (< ~20 levels), so a stack overflow is not a real-world risk. However:
function rootsSummaryOrAlias(node: TSESTree.Node, depth = 0): boolean {
if (depth > 32) return false; // guard against extreme nesting
if (rootsSummary(node)) return true;
if (node.type === "Identifier") return isCoreSummaryAlias(node);
if (node.type === "CallExpression" && node.callee.type === "MemberExpression") {
return rootsSummaryOrAlias(node.callee.object, depth + 1);
}
return false;
}@copilot please address this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 72d11d2. |
||
| return rootsSummaryOrAlias(node.callee.object, depth + 1); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| return { | ||
| ExpressionStatement(node) { | ||
| const expr = node.expression; | ||
|
|
@@ -107,8 +189,8 @@ export const requireAwaitCoreSummaryWriteRule = createRule({ | |
| // Property must be `write` (direct or computed string-literal access) | ||
| if (!isWriteProperty(callee)) return; | ||
|
|
||
| // Object must trace back through a `.summary` member access | ||
| if (!rootsSummary(callee.object)) return; | ||
| // Object must trace back through a `.summary` member access (or an alias) | ||
| if (!rootsSummaryOrAlias(callee.object)) return; | ||
|
|
||
| // Only offer the `await` suggestion when already inside an async function — | ||
| // applying `await` outside an async context would produce a syntax error. | ||
|
|
||
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] No test for alias used across a nested function scope — the scope-walk (
currentScope.upper) path inisCoreSummaryAliasis never exercised.💡 Suggested test case to add inside the valid/invalid blocks
Without this, the
while (currentScope !== null) { currentScope = currentScope.upper; }loop is only tested with a single-scope setup and a regression here would go undetected.@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 72d11d2. Added a cross-scope invalid test: alias declared in the outer async function,
write()called inside a nested inner async function. The scope-walk inisCoreSummaryAliasfinds the variable viacurrentScope.upperand the call is correctly flagged with theawaitsuggestion.