Skip to content

fix(transform): generator id scans must see closures in every Expr variant (#4851)#4854

Merged
proggeramlug merged 2 commits into
mainfrom
worktree-fix-4851
Jun 9, 2026
Merged

fix(transform): generator id scans must see closures in every Expr variant (#4851)#4854
proggeramlug merged 2 commits into
mainfrom
worktree-fix-4851

Conversation

@proggeramlug

Copy link
Copy Markdown
Contributor

Fixes #4851await fn() never resumed when fn applied Object.assign(promise, { ...≥2 function-valued props }), the exact shape Stripe's StripeMethod uses for its auto-pagination methods (so this blocked Stripe end-to-end even after #4849).

Root cause

A FuncId collision, not an await/promise bug. The generator transform allocates fresh ids for its synthesized closures starting at compute_max_func_id(module) + 1. That scan (generator/id_scan.rs) used a hand-rolled expression walker with a _ => {} catch-all, and Expr::ObjectAssign had no arm.

Object.assign(p, { a: () => 1, b: () => 2 }) lowers to

ObjectAssign { target, sources: [New { __AnonShape_…, args: [Closure(4), Closure(5)] }] }

so both arrows' FuncIds were invisible to the scan. With max miscomputed as 3 (the setTimeout arrow), the transform minted id 4 for an internal generator closure and id 5 for the __async_step closure of the awaiting caller — the same id as the b arrow. Codegen emits one LLVM function per FuncId, the arrow body won, and the caller's await continuation was silently dropped: no catch, no done, exit 0.

This also explains every variant in the issue's narrowing table:

  • one function prop → only the benign internal-closure collision (id 4 vs a), __async_step got a free id → worked
  • two number props → no closures in the literal at all → worked
  • bind-to-temp / top-level / .then → different caller shape, no __async_step minted at the colliding id → worked
  • runtime experiments (no-op Object.assign, can_reuse = false, GC off) couldn't help because the miscompile happens at transform time

Fix

Same refactor that async_to_generator.rs::scan_expr and inline/closure_analysis.rs::find_max_local_id already received after earlier instances of this bug class (#167/#169/#214; the func scan grew one-off arms in #393, #531, #1824): keep explicit arms only for the id-bearing fields an Expr owns directly (FuncRef/Closure; LocalGet/LocalSet/Update/array-mutator ids) and delegate all child descent to perry_hir::walker::walk_expr_children, which is exhaustively matched — a new Expr variant that forgets the walker is a compile error, so these scans can never silently undercount again. Net −231 lines.

Also hardened while there (same class, found by inspection):

  • the func-id stmt scan skipped loop/switch HEAD expressions (for (let g = () => 1;;), while ((() => f())())) that the local-id twin already covered
  • Stmt::PreallocateBoxes raw LocalIds are now maxed by the local scan

Validation

  • Issue's minimal repro + bind-first + single-function + mixed-props + async-callee + generator variants: all byte-match node --experimental-strip-types (before: no output, exit 0)
  • test-files/test_async*.ts (7 files): byte-match Node
  • Filtered parity sweeps (promise, gener, yield, iter): the only 3 mismatches (test_issue_3070_…, test_issue_3990_…, test_gap_yieldstar_inherited_iterator_this) fail byte-identically on a pre-fix baseline build — pre-existing, 0 regressions
  • cargo test -p perry-transform -p perry-hir: all pass; new unit test pins closures-inside-ObjectAssign visibility for both scans

Not addressed (separate issue from the report)

The "related" Promise-expando corruption (p.x = v writing into the Promise struct's state/reason fields) is real but orthogonal — it was explicitly ruled out as the hang cause and deserves its own fix (side-table expandos or a no-op guard like Date/TypedArray have). Worth its own issue if not already tracked.

No version bump / changelog per the maintainer-merges-metadata convention.

…riant (#4851)

The generator transform's max-FuncId/LocalId scans (compute_max_func_id /
compute_max_local_id in generator/id_scan.rs) used hand-rolled expression
walkers with a `_ => {}` catch-all. Any Expr variant without an explicit
arm silently hid the FuncIds/LocalIds of closures nested inside it.

`Object.assign(p, { a: () => 1, b: () => 2 })` lowers to
`ObjectAssign { sources: [New { __AnonShape, args: [Closure, Closure] }] }`
and ObjectAssign had no arm, so both arrows' FuncIds were invisible.
transform_generators then minted the same FuncId for the `__async_step`
closure it synthesizes for an async caller, codegen emitted one LLVM
function per FuncId (the arrow body won), and the caller's await
continuation was silently dropped: `await makeReq()` never resumed, the
program exited 0 with no output. This is the Stripe SDK's StripeMethod
shape (auto-pagination methods Object.assign'd onto the request promise),
so it blocked Stripe end-to-end even after #4849.

Fix: per the precedent already in async_to_generator.rs::scan_expr and
inline/closure_analysis.rs::find_max_local_id (refactored after the same
bug class in #167/#169/#214), keep explicit arms only for the id-bearing
fields an Expr owns directly (FuncRef/Closure, LocalGet/LocalSet/Update/
array-mutator ids) and delegate all child descent to
perry_hir::walker::walk_expr_children, which is exhaustively matched —
a new Expr variant that forgets the walker is a compile error, so this
scan can never silently undercount again. Also mirror the local-scan's
loop/switch-head expression coverage in the func stmt scan and max the
raw LocalIds carried by Stmt::PreallocateBoxes.

Verified: the #4851 minimal repro plus the bind-first / single-function /
mixed-props / async-callee / generator variants now byte-match Node;
test_async*.ts all match Node; perry-transform/perry-hir suites pass.
New unit test pins closures-inside-ObjectAssign visibility for both scans.
@proggeramlug

Copy link
Copy Markdown
Contributor Author

Note: the lint failure here is environmental — #4853 landed on main with formatting the current stable rustfmt rejects, so cargo fmt --all -- --check fails on every PR's merge ref (diffs are exclusively in files this PR doesn't touch). #4855 fixes it; lint here goes green once that merges and this re-runs.

proggeramlug added a commit that referenced this pull request Jun 9, 2026
…+ body_stmt.rs file-size split) (#4855)

* style: cargo fmt --all after #4853 (unblocks lint on every PR)

#4853 merged with formatting that the current stable rustfmt
(1.9.0 / rustc 1.96.0, what CI's dtolnay/rust-toolchain@stable
installs) rejects, so the required 'lint' check now fails on every
PR's merge ref regardless of its content (first seen on wt-clm-parity
at 18:51Z, then worktree-fix-4851/#4854). Formatting-only change,
no code edits.

* refactor(hir): split for-await target-detection helpers out of body_stmt.rs (file-size gate)

body_stmt.rs hit 2038 lines on main (limit 2000), so the lint job's
file-size gate fails every PR even after the fmt fix. Move the
self-contained for-await/for-of head-expression detection predicates
(ReadableStream / Node Readable / readline / fs.Dir) plus the
IteratorClose helpers into body_stmt/detect.rs, following the existing
body_stmt/for_await.rs split pattern. Pure code motion, no behavior
change; body_stmt.rs is now 1826 lines.

---------

Co-authored-by: Ralph Küpper <ralph@skelpo.com>
@proggeramlug proggeramlug merged commit 8ff2c81 into main Jun 9, 2026
1 check passed
@proggeramlug proggeramlug deleted the worktree-fix-4851 branch June 9, 2026 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

await of an Object.assign'd (>=2 function props) promise returned from a function never resumes (blocks Stripe SDK)

1 participant