fix(transform): suspend on yields in nested generator loop headers - #10537
proggeramlug wants to merge 2 commits into
Conversation
A `yield` in a while/do-while condition or a for condition/update was only split into resume states when the loop was a direct statement of the generator body. `body_contains_yield` looked at loop bodies but never loop headers, so an enclosing `if`/`try`/`switch`/label/loop whose only yield sat in a nested loop header was emitted inline and the residual yield never suspended: `[...g()]` was empty and sent-value loops never terminated (lru-cache 11.5.2's minified `*#A`/`*#z` iterators). - body_contains_yield also checks loop conditions and for updates. - A for-init's own top-level yield (`for (let t = yield x; ...)`, `for (yield x; ...)`, and an async function's `for (let x = await p; ...)` after the await-to-yield rewrite) is hoisted ahead of the loop. - A yielding for update left in place by the header arm (a `continue` inside try/finally) is linearized in the loop's update state. - In an async generator, a header yield's operand is awaited like every other yield operand.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughGenerator transformation now detects yields in loop headers at nested depths, hoists ChangesGenerator loop-header yields
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant GeneratorBody
participant body_contains_yield
participant linearize_body
participant GeneratorState
GeneratorBody->>body_contains_yield: inspect loop condition or update
body_contains_yield-->>linearize_body: identify a yielding loop
linearize_body->>GeneratorState: normalize header and emit suspension state
GeneratorState-->>GeneratorBody: resume at header continuation
Merge Risk: ⚪ Minimal · up to The loop-header suspension changes have broad regression coverage and no identified merge-blocking issue. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 52.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 6 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
Landed via merge train #10559 (v0.5.1592). All source commits preserve authorship; merged main matches the validated train exactly. |
Summary
A generator never suspended on a
yieldin a loop header (while/do…whilecondition,forcondition / update) when the loop was nested insideif/else,try/catch/finally, aswitchcase, a label or another loop. #5933 fixed only loops that are direct statements of the generator body. The residual yield did not suspend, so[...g()]was empty, a sent-value loop never terminated, and.return()/.throw()hit a generator that had already finished. Async generators were affected the same way.Minifiers emit this shape. lru-cache 11.5.2 resolves
import+nodetodist/esm/node/index.min.js, where*#A/*#zareif(this.#n)for(let t=…;this.#V(t)&&(…(yield t),t!==this.#a);)t=…, so every iteration API came back empty.Root cause
body_contains_yieldfinds a suspend point in it. This decides theIf(linearize.rs:1355),Try(:1130),Labeled(:1594),Switch(:1665) and loop-body arms.body_contains_yield(crates/perry-transform/src/generator/break_continue.rs:276) looked at loop bodies only (While:326,DoWhile:332,For:338) and never at loop headers.linearize.rs:686/:729/:769) never ran on that loop.While covering every header position, three more gaps turned up. All three fail even for top-level loops on the baseline:
for (let t = yield x; …),for (yield x; …)):hoist_yields.rs:104hoisted only the yields nested inside the init. It left a top-level one in the init, and no arm splits it there. After the await→yield rewrite, plain async functions have the same shape:for (let x = await p; …)produced an empty loop.continueinsidetry/finally: the header arm skips this shape on purpose (the update must run after the finally). The body arm then copied the update intoupdate_statewithout splitting it (linearize.rs:956). If the body had no other yield, the loop fell through to the catch-all.await_async_generator_yield_operands(lower/yield_await.rs) runs before linearization and never looks inside loop headers. As a result,yield promisein a loop condition delivered the promise itself instead of its value.Fix (
crates/perry-transform/src/generator/)break_continue.rs:body_contains_yieldalso checksWhile/DoWhileconditions andForconditions and updates, usinghoist_yields::expr_contains_yield, which does not descend into nested closures.hoist_yields.rs: a for-init'slet-initializer or expression-statement is hoisted fully, including a top-level yield, ahead of the loop. The init runs once, so hoisting it is exact.linearize.rs:normalize_loop_header_stmts. It hoists yields, and in an async generator it also awaits each yield operand. The await is added only to the statements built from the header; loop bodies were already processed.Forbody arm now also fires when the update the header arm left in place contains a yield. That update is linearized starting atupdate_state, which is still thecontinuetarget.Tests
test-files/test_gap_10419_generator_loop_header_yield.tsmatches Node 26.5.1 byte-for-byte. It covers:while/do…while/ for-update / for-init loopsreturn()/throw()while suspended in a header yield (inner and outer catch)breakin a switchcontinue outerwith a yielding updatecontinueinsidetry/finallywith a yielding updatefor await(cond in if, while in try, do-while in switch, update in label, promise operand, sent values,return()mid-loop)for (let x = await p; …)Every loop calls a guard that throws after 1000 steps, and every consumer caps its pulls, so a regression prints a wrong line instead of hanging.
Parity Fail: 1.Parity Pass: 1, with and withoutPERRY_NO_AUTO_OPTIMIZE=1.crates/perry-transform/src/generator/loop_header_yield_tests.rs(5 unit tests):Expr::Yieldafter the full transform for 7 nested shapes × sync/async, with a check that each fixture starts with a yieldAgainst the baseline transform sources, 4 of 5 fail; the closure negative control passes either way, as it should.
Validation (perrybuilder, Linux x64)
cargo test --release -p perry-transform --testscargo test --release -p perry --test issue_5933_loop_condition_await --test issue_5868_switch_state_machine --test issue_5975_labeled_continue_in_yielding_switch --test issue_6709_async_generator_pending_await --test for_await_continue --test for_await_block_scope_leak --test issue_6067_generator_local_string_index_oob(separate target dir)./scripts/run_lint_gates.sh-D warningscheck, clippy, API-docs regen and drift). The one red gate,benchmarks/ci_public_baseline_check.py("public artifact benchmark inputs changed"), fails the same way on a pristine 7661bc0 tree, so it predates this change.PERRY_SKIP_BUILD=1 ./scripts/run_gap_tests.sh)GAP_EXIT=0, "Gap snapshot OK — 820 tests match". 814 pass, 6 fail (2159, 2514, json_lazy_defineproperty_index, perfhooks_3088…, prop_plan_cache_invalidation, v8_2_3680plus); the baseline 7661bc0 run fails the same 6. No new failures. The new test passes.python3 scripts/check_test_registration.pyPerformance (
perf stat -e instructions:u, 3 runs each, median; Node 26.5.1 wall median for context)range/while/ifgenerators, 6M steps)for/whilecond, 4M steps)benchmarks/tls-budget/asyncpipe.ts(async functions use the same linearizer)if/try, 4M steps)Emitted LLVM IR (
--trace llvm) for the first three workloads is byte-identical between the two compilers, except for the embedded entry-file path. The fix changes only which statements get linearized. Generators without header yields compile to the same code. A nested header-yield loop now costs about the same as the top-level form: 21.44B vs 20.41B instructions, with the extra ~5% coming from the enclosingtry/ifstates.In absolute terms, Perry generators are far slower than Node: about 4,900 instructions per generator step, or 3–4.5 s CPU on the first workload against Node's 173 ms wall. That gap was there before this change, including for top-level and body-only yields, and this PR neither causes nor fixes it.
Package check
lru-cache 11.5.2, default (minified) entry, compiled through
perry.compilePackageswith the audit script (keys/values/entries/rkeys/forEach/for…of/dump/clear()disposal /maxSize/ subclass): 14 diff lines vs Node on the baseline, 0 on this branch.Not verified / not addressed
keys()/values()/forEachover 10k entries take ~26 s CPU (349B instructions) against ~0.4 s in Node. The baseline already spent 193B instructions while yielding nothing. The profile is dominated by private-member access (js_object_get_own_field_or_undef+memcmp,ic_miss::private_instance_element_is_present, and aformat!of the private storage name on every access), not by the generator transform. A plain method doing 3M#field/#methodaccesses costs 57.5B instructions, against 83 ms in Node. I have not filed that separately.Fixes #10419
Summary by CodeRabbit
Bug Fixes
yieldexpressions inwhile,do…while, andforloop headers, including initializers, conditions, and updates.break/continue, and iterator cleanup.Tests