fix(hir): continue in for-await drivers skipped the iterator advance (spin)#6196
Conversation
…e (spin)
Every iterator-driver desugar that modeled `for await (x of src)` as
let __result = await __iter.next();
while (!__result.done) {
<bind x>; <user body>;
__result = await __iter.next(); // advance at the body TAIL
}
broke `continue`: it jumps to the `while` condition, skips the tail
advance, and re-processes the SAME result forever — an infinite spin
observed as a hang. Six copies of the shape existed:
- lower/stmt_loops.rs: the generic runtime for-await driver, the
recognized-generator-call driver, and the Web-ReadableStream
getReader()/read() driver;
- lower_decl/body_stmt.rs: the latter two again (function-body copies);
- lower_decl/body_stmt/for_await.rs: the function-body generic driver.
The array/lazy sync path (`lazy_iter_for_stmt`) already documented this
exact footgun and dodged it with `Stmt::For`'s update clause — which the
await-capable drivers can't use (no `await` in a `for` update). All six
now advance at the TOP:
let __result = undefined;
while (true) {
__result = await __iter.next();
if (__result.done) break;
<bind x>; <user body>;
}
`continue` falls to the `while (true)` condition and re-runs the
advance; `break`/`return`/`throw` behavior is unchanged (the synthetic
`if done break` is appended AFTER the abrupt-close rewrite over the user
body, so normal completion never runs a spurious IteratorClose).
Canonical real-world failure: an SSE consumer's
`for await (const ev of stream) { if (ev.event === "ping") continue; …}`
— a large esbuild-bundled CLI app hung on the FIRST real server ping
event (local mocks that never sent pings masked it; the ping event is
the only one that SDK skips via `continue`).
Tests: crates/perry/tests/for_await_continue.rs — continue inside
for-await in an async generator (the SSE shape), in a plain async fn,
over a custom `[Symbol.asyncIterator]` iterable (runtime GetAsyncIterator
driver), and over a Web ReadableStream (reader driver); plus a
break/nested-continue behavior guard. All node-oracle-verified, with a
30s hang guard (pre-fix these spin forever). perry-hir unit suites pass
(the one c262_parity failure — `o.x ??= 2` store lowering — fails
identically on pristine main; unrelated).
📝 WalkthroughWalkthroughThis PR fixes an infinite-spin bug in ChangesAdvance-at-top Iterator Driver Fix
Estimated code review effort: 4 (Complex) | ~50 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-hir/src/lower/stmt_loops.rs (1)
361-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider exposing
iter_driver_while_stmt(or a shared equivalent) so the other two files reuse it.The advance-at-top shape here is re-inlined verbatim in
lower_decl/body_stmt.rs(ReadableStream + iterator-protocol) andlower_decl/body_stmt/for_await.rs. Those copies even reference this helper in their comments. Since it was exactly this shape diverging across the "six copies" that produced the hang, promoting this to apub(crate)helper and calling it from all sites would prevent future drift. The implementation itself is correct.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-hir/src/lower/stmt_loops.rs` around lines 361 - 393, The advance-at-top iterator-driver loop is duplicated in other lowering paths, which risks the copies drifting again; expose iter_driver_while_stmt as a shared pub(crate) helper (or equivalent common helper) and update lower_decl/body_stmt.rs and lower_decl/body_stmt/for_await.rs to call it instead of inlining the shape. Keep the implementation in stmt_loops.rs as the single source of truth so the iterator-driver behavior stays consistent across all sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/perry-hir/src/lower/stmt_loops.rs`:
- Around line 361-393: The advance-at-top iterator-driver loop is duplicated in
other lowering paths, which risks the copies drifting again; expose
iter_driver_while_stmt as a shared pub(crate) helper (or equivalent common
helper) and update lower_decl/body_stmt.rs and lower_decl/body_stmt/for_await.rs
to call it instead of inlining the shape. Keep the implementation in
stmt_loops.rs as the single source of truth so the iterator-driver behavior
stays consistent across all sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bd80e267-7432-4997-aad7-fc91be287be5
📒 Files selected for processing (4)
crates/perry-hir/src/lower/stmt_loops.rscrates/perry-hir/src/lower_decl/body_stmt.rscrates/perry-hir/src/lower_decl/body_stmt/for_await.rscrates/perry/tests/for_await_continue.rs
Problem
Every iterator-driver desugar that modeled
for await (x of src)asbroke
continue: it jumps to thewhilecondition, skips the tail advance, and re-processes the same result forever — an infinite spin observed as a hang. Six copies of the shape existed (lower/stmt_loops.rs×3: generic for-await, recognized-generator-call, Web-ReadableStream reader;lower_decl/body_stmt.rs×2;lower_decl/body_stmt/for_await.rs×1).The array/lazy sync path (
lazy_iter_for_stmt) already documents this exact footgun — "awhilewith the advance at the body tail would skip it oncontinueand spin" — and dodges it withStmt::For's update clause, which the await-capable drivers can't use (noawaitin aforupdate).Ten-line repro (hangs pre-fix, node prints both values + done):
Canonical real-world failure: an SSE consumer's
for await (const ev of stream) { if (ev.event === "ping") continue; … }— a large esbuild-bundled CLI app hung on the first real server ping event. Local mocks that never sent pings masked it completely;pingis the only SSE event the client skips viacontinue.Fix
All six drivers now advance at the TOP:
continuefalls to thewhile (true)condition and re-runs the advance.break/return/throwbehavior is unchanged — the syntheticif done breakis appended after the abrupt-close rewrite over the user body, so normal completion never runs a spurious IteratorClose. The threestmt_loops.rssites share a newiter_driver_while_stmthelper; thebody_stmtcopies mirror it.Tests
New
crates/perry/tests/for_await_continue.rs(5 tests, node-oracle-verified, 30s hang guard since pre-fix they spin forever):continueinside for-await in an async generator (the SSE shape)[Symbol.asyncIterator]iterable (runtime GetAsyncIterator driver)(The pre-existing
c262_parity::logical_property_assignment_short_circuits_the_store_4586failure reproduces identically on pristine main — unrelated.)Summary by CodeRabbit
Bug Fixes
for awaitand iterator-based loop handling socontinuenow advances to the next item correctly instead of reusing the same completed result.breakand nested-loopcontinuecases.Tests
for awaitloops to verify correct output and prevent future loop-control regressions.