fix(eslint): harden require-await-core-summary-write for compound expression forms#43507
Conversation
…ression forms - Add collectBareWriteCalls() helper to unwrap LogicalExpression (right operand), ConditionalExpression (both branches), and SequenceExpression (last element) before the CallExpression check - Detect .then()/.catch()/.finally() chains on write() calls and flag the outer expression (restricted to known Promise methods to avoid FPs) - Exempt void core.summary.write() as the idiomatic deliberate-discard marker - Add invalid tests for Logical / Conditional / Sequence / .then() forms - Add valid test for void exemption Closes #43326 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR strengthens the require-await-core-summary-write ESLint rule so it detects dropped Promise<Summary> values from core.summary.write() even when the call is embedded in compound expressions (logical/conditional/sequence expressions and Promise chain methods), and adds tests for these previously missed forms.
Changes:
- Add
collectBareWriteCalls(expr)helper to recursively find barewrite()calls inside several compound expression shapes. - Refactor the
ExpressionStatementvisitor to report (and suggestawaitfor) each matched call expression. - Add new tests covering compound-expression false negatives and
voiddeliberate-discard exemption.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/require-await-core-summary-write.ts | Adds recursive collector and refactors reporting/suggestions to catch compound-expression dropped write() promises. |
| eslint-factory/src/rules/require-await-core-summary-write.test.ts | Adds new valid/invalid cases for compound expressions and void exemption. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 4
- Review effort level: Low
| // Logical short-circuit: only the right operand can be the dropped promise | ||
| if (expr.type === "LogicalExpression") { | ||
| return collectBareWriteCalls(expr.right); | ||
| } |
| // Sequence: only the last expression is the "result" value. | ||
| // SequenceExpression always has ≥2 elements per the ECMAScript spec, but | ||
| // guard defensively since the type signature allows an empty array. | ||
| if (expr.type === "SequenceExpression") { | ||
| const { expressions } = expr; | ||
| if (expressions.length === 0) return []; | ||
| const last = expressions[expressions.length - 1]; | ||
| return collectBareWriteCalls(last); | ||
| } |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected). |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (2 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — overall a clean, well-structured fix with good coverage. Left a few suggestions around edge case documentation and missing test variants.
📋 Key Themes & Highlights
Key Themes
- Missing
||/??test coverage: TheLogicalExpressionhandler covers all three operators but only&&is tested. Adding||and??cases would lock in the behaviour. - Multi-level
.then().catch()chain: A doubly-chained form likewrite().then(f).catch(e)is not flagged by the current logic. Whether that's intentional (to avoid FPs) or an oversight should be documented or tested. voidcompound form:void (cond && write())is already exempt by the implementation, but no test locks in this behaviour.- ESLint suggestion independence: The double-error
ConditionalExpressioncase is tested correctly; a brief inline comment would make the single-at-a-time assumption explicit for future maintainers.
Positive Highlights
- ✅ Excellent recursive design for
collectBareWriteCalls— clean separation from the visitor - ✅
voidexemption is well-motivated and properly handled as a first-class escape hatch - ✅ The double-error conditional test is a great regression anchor for multiple-suggestion scenarios
- ✅ Defensive empty-array guard on
SequenceExpressionis a nice touch given the type signature - ✅ Promise chain restriction to
then/catch/finallyavoids false positives on arbitrary chained methods
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 68 AIC · ⌖ 5.81 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| // Logical short-circuit: only the right operand can be the dropped promise | ||
| if (expr.type === "LogicalExpression") { | ||
| return collectBareWriteCalls(expr.right); |
There was a problem hiding this comment.
[/diagnosing-bugs] The comment says "right operand only (the potential dropped value)" but doesn't explain why ||/?? left operands are safe to skip. For cond || core.summary.write() the right operand is only evaluated when cond is falsy, which is still a dropped Promise worth flagging — yet for cond ?? write() the left would never be a write call, so right-only is always correct. A brief comment clarifying the reasoning would prevent future readers from second-guessing this choice.
💡 Suggested clarification
// For all logical operators (&&, ||, ??), the right operand is the value that
// can be produced and immediately discarded as a bare ExpressionStatement.
// The left operand is always evaluated for its truthiness — it cannot itself
// be silently dropped — so right-only recursion is the correct conservative policy.@copilot please address this.
There was a problem hiding this comment.
Review: fix(eslint): harden require-await-core-summary-write for compound expression forms
The implementation is correct and well-reasoned. The new collectBareWriteCalls helper cleanly handles all four compound expression forms (logical, conditional, sequence, promise chains) and the tests thoroughly cover both the valid (void-exemption) and invalid cases including the multi-error conditional.
Highlights:
voidexemption is correctly handled as the outermost guard — deliberate-discard pattern won't produce false positives- Reporting against
callExpr(notnode) gives more precise location and makes theawait-insertion suggestion target the right node - Promise chain detection is conservatively restricted to
then/catch/finally— no false-positive risk on arbitrary method chains - The double-error test for
cond ? write() : write()validates independent suggestion application correctly
Non-blocking suggestions (inline comments):
- Line 201 — inline comment says "only the right operand can be the dropped promise" which is slightly misleading (left-side write() in
write() && condis an acknowledged gap, not because the right is the only possible drop). Suggest wording the comment to make the gap explicit. - Line 195 —
collectBareWriteCallshas no depth guard, unlikerootsSummaryOrAlias. A simpledepth > 32guard would make it consistent and prevent hypothetical deep-recursion in malformed ASTs. - Line 212 — Non-last sequence elements with
write()calls are silently ignored. The behavior is defensible, but a comment acknowledging this as intentional would improve readability.
All suggestions are non-blocking. The change is a net improvement in linter coverage. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 94.7 AIC · ⌖ 6.23 AIC · ⊞ 4.9K
|
|
||
| // Logical short-circuit: only the right operand can be the dropped promise | ||
| if (expr.type === "LogicalExpression") { | ||
| return collectBareWriteCalls(expr.right); |
There was a problem hiding this comment.
The comment says "only the right operand can be the dropped promise", but the left operand of && is also evaluated and its Promise is discarded when it is a bare write call:
core.summary.write() && doSomething() // left-side write() Promise silently dropped — not flaggedThis is a known gap (the PR description focuses on right-operand / result-value drops), but the inline comment is misleading. Consider rewording to:
// Logical: only the right operand is unwrapped (it carries the statement's result value).
// Left-side write() drops (e.g. write() && cond) are a known out-of-scope gap.Non-blocking suggestion. @copilot please address this.
|
@copilot please run the
|
…xpression elements Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Both issues from the review are addressed in commit eeb9b7d:
All 129 eslint-factory tests pass. The only test-unit failure ( |
|
@copilot please run the
|
All review feedback was addressed in commit eeb9b7d. Local validation is green: |
🤖 PR Triage — Run §28741077056
Rationale: ESLint rule fix capturing previously silently-missed Promise drops in compound expressions. 2× APPROVED. JS-only change with tests included.
|
|
@copilot please run the
|
|
🎉 This pull request is included in a new release. Release: |
The
require-await-core-summary-writerule only inspected directExpressionStatement → CallExpressionnodes, silently missingPromise<Summary>drops in compound expression forms:Changes
collectBareWriteCalls(expr)— new recursive helper insidecreate()that replaces the inline guard. Unwraps:LogicalExpression→ right operand onlyConditionalExpression→ both branchesSequenceExpression→ last element (with defensive empty-array guard)UnaryExpression[void]→ immediately returns[](deliberate-discard, exempt).then()/.catch()/.finally()chains on awrite()call → flags the outer expression; restricted to known Promise method names to avoid false positives on arbitrary chained callsExpressionStatementvisitor — refactored to callcollectBareWriteCallsand loop over results; theawait-insertion suggestion continues to work correctly for each flagged node within its compound context.Tests — new
itblocks forvoidexemption (valid) and all four compound forms (invalid), including a double-error case where both branches of a conditional callwrite().