Skip to content

fix(codegen): initialize missing cells in reused boxed declarations - #10049

Closed
proggeramlug wants to merge 4 commits into
mainfrom
fix/generator-preallocation-10048
Closed

proggeramlug wants to merge 4 commits into
mainfrom
fix/generator-preallocation-10048

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Closes #10048.

Problem and fix

An async-generator delegated loop emits the local callback's declaration in
more than one continuation. The first emitted declaration allocates its box;
the ordinary existing-local / hoisted-var reuse guard routes later copies
straight through LocalSet. On a resumed path the stack slot exists but still
holds the entry undefined sentinel, so initialization and capture use no cell.

Before a reused, non-preallocated boxed declaration evaluates its initializer,
check whether the pointer slot still contains that sentinel. Materialize the
missing cell when needed and preserve an existing live cell unchanged. This
keeps hoisted-var/parameter binding identity and handles self-capturing
initializers. Module globals, preallocated cells, and ordinary unboxed slots
stay on their existing paths; specialized async control cell types are kept.
No environment switch or runtime ABI change is introduced.

Independent branch from main 603b074ac. Neither #10042 nor #10046 nor any
application bundle, extraction, credentials, or network is needed for testing.

Validation

  • Pinned Node 26.5.1 passes both independent gap fixtures.
  • Frozen before-fix native reproduction exits 1 with the callback TypeError.
  • The initial preallocation-only candidate eb52a581e was rejected after
    native O0 failed under both default and compact GC. That implementation has
    been removed; the original PreallocateBoxes behavior is restored.
  • The revised tests exercise the actual ordinary declaration reuse path,
    with and without an initializer, requiring conditional allocation into the
    same entry-initialized slot.
  • 1,463 codegen unit tests pass, no failures, one ignored, with the revised
    implementation: normal debug profile, one thread, 16.01s execution.
  • 12 native cases pass: both standalone fixtures at O0/Os/Oz under default
    and compact GC, Wasm host enabled, byte-for-byte against pinned Node.
    The compiler and all nine coherent provider archives are frozen at
    81b47b47cace5b150edde32059ff272a3a19eca5. Compiler build: 10m30s;
    runtime graph: 9m56s; each had a 15-minute bound.
  • Current head f56ef0f9638e333be994db6de8bdac00f90b587e changes only Rust
    comments and test setup relative to that native toolchain. Its exact fixtures
    were rerun with the frozen pair (12 passes). This does not claim a newly built
    compiler at the test-only head. The compiler implementation is unchanged.
  • The initial broad control exposed a separate existing repeated-loop TDZ
    failure, now independently reported with a reproducer in codegen: captured let loses its TDZ on the second loop iteration #10051. A six-case
    A/B reproduces identical failure at O0/Oz on old main 53df2c671, pre-fix
    f3e8d5d1a, and fixed 81b47b47c. This PR's TDZ/no-initializer/recursion
    control uses separate invocations; retained per-iteration callbacks and
    shared hoisted-var bindings remain covered. No failing behavior is claimed fixed.
  • Final full script-tier lint: 75 pass, one proven base-main benchmark freshness
    failure, two CI-only skips
    . Compile-tier lint is explicitly skipped locally.
    The two-line file-cap overflow has been corrected with a comment-only move
    (let_stmt.rs: 1,998 lines); formatting and file-cap checks pass.
  • Additional local moving-GC witness passes at Oz under both rooting modes:
    insert an allocating synchronous loop between getter creation, wrapper
    capture, and later invocation. Each run verifies 224 loop polls, 231 copying
    minors, and 13,778 moved objects
    , with from-space protection and evacuation
    verification enabled. This uses the same frozen 81b47b47c toolchain; it is
    supplementary local evidence, not a claim that CI runs this extra witness.

Completed current-head Linux CI

Run 34576434651
at exact head f56ef0f9638e333be994db6de8bdac00f90b587e is complete. Both new
generator fixtures executed and passed (shards 2 and 5). Check, warnings,
scoped E2E, gap shards 1/6, and GC stress passed. The GC matrix reports
429 PASS / 159 UNVERIFIED / 0 FAIL; unverified cells are not passes.

CI is not all green. A single attribution pass against the exact base-main
603b074ace01464bc66fc07cc8d532f26ccf5a0f
push run 34565492075
finds the same failures:

  • Lint: inherited public benchmark freshness failure.
  • Cargo-test: native_stack::tests::stack_top_respects_custom_thread_stack_sizes,
    the same bound must belong to this worker assertion at native_stack.rs:53
    and unwrap at :60; 3,504 passed / 1 failed / 4 ignored.
  • Gap shards 2/3/4/5: all 10 printed failure verdicts and diagnostics match
    base main, including the webcrypto threadpool timeout. Seven are snapshot
    regressions and three are already baseline-accepted parity failures. Comparison
    covers reported exit codes and first/last printed output lines, not full stdout.
  • pr-gate is the fan-in of those failed jobs, not an additional test failure.

No baseline or tests were suppressed and no CI rerun was requested. This remains
ready for maintainer review; the separate full application rebuild is pending.

Native fixtures cover delegated loops, retained iteration callbacks, empty
delegates, ordinary yield/await, recursion, TDZ, and shared hoisted-var bindings.
This is not a claim that the full application works or meets its size target.
The merge train can perform the patch-version bump.

@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 01b7d514-935a-4604-9825-4b48966b8015

📥 Commits

Reviewing files that changed from the base of the PR and between 603b074 and f56ef0f.

📒 Files selected for processing (8)
  • changelog.d/10049-generator-continuation-preallocation.md
  • crates/perry-codegen/src/stmt/boxed_continuation_tests.rs
  • crates/perry-codegen/src/stmt/boxed_local_init.rs
  • crates/perry-codegen/src/stmt/let_stmt.rs
  • crates/perry-codegen/src/stmt/mod.rs
  • crates/perry-codegen/src/stmt/prealloc_module_global_tests.rs
  • test-files/test_gap_generator_delegated_local_capture.ts
  • test-files/test_gap_generator_preallocated_capture_controls.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The code generator now initializes missing boxed cells when a continuation reuses a boxed local slot. New code-generation tests and async generator fixtures cover delegation, closures, control flow, TDZ behavior, recursion, and shared bindings.

Changes

Generator continuation box initialization

Layer / File(s) Summary
Reused boxed-cell initialization
crates/perry-codegen/src/stmt/boxed_local_init.rs, crates/perry-codegen/src/stmt/let_stmt.rs, crates/perry-codegen/src/stmt/mod.rs
The hoisted-variable reuse path now creates a missing boxed cell before lowering its initializer. The helper preserves preallocated and module-global paths.
Code-generation regression coverage
crates/perry-codegen/src/stmt/boxed_continuation_tests.rs, crates/perry-codegen/src/stmt/prealloc_module_global_tests.rs
Tests verify conditional allocations, shared lexical slots, entry sentinels, and initialized and uninitialized branch cases.
Async generator parity fixtures
test-files/test_gap_generator_delegated_local_capture.ts, test-files/test_gap_generator_preallocated_capture_controls.ts, changelog.d/10049-generator-continuation-preallocation.md
Fixtures cover delegated loops, empty and non-empty delegates, yield and await controls, retained closures, TDZ, recursion, hoisted bindings, and result validation. The changelog records the behavior and coverage.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to f56ef

The reused boxed-cell initialization path and its async-generator regression coverage do not leave an identified merge-blocking risk.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 26.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 7 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #10048 requires a missing cell before a reused boxed declaration initializer or nested capture executes. lower_let calls ensure_reused_box_is_initialized on the existing-local reuse path. Th…
Out of Scope Changes check ✅ Passed The changed files implement or test Issue #10048. The changelog documents the fix. The pub(super) visibility change enables reuse of existing test compile options. No unrelated production behavior, …
Title check ✅ Passed The title is concise, specific, and accurately identifies the main code-generation fix: initializing missing cells in reused boxed declarations.
Description check ✅ Passed The description provides a detailed problem statement, fix summary, related issue, implementation scope, validation results, and CI status. It does not use every template heading or checklist item, bu…
Full details: Docstring Coverage

Explanation

Docstring coverage is 26.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 7 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/generator-preallocation-10048

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Native validation found the initial eb52a581e implementation insufficient:
both default and compact O0 runs still exit 1 with the original callback
TypeError. The earlier 1,463 passing codegen tests do not override that result.
This PR remains drafted; no application rebuild has started.

The small fixture's retained LLVM identifies the actual path:
stmt/let_stmt.rs's general existing-local / hoisted-var reuse guard, before
the later boxed declaration arm. The second emitted getter declaration routes
through LocalSet while its shared stack slot still holds the entry undefined
sentinel. The initial PreallocateBoxes-only patch does not execute there.

The follow-up restores the original preallocation implementation and guards
the ordinary reused boxed-declaration path: initialize a missing cell before
lowering the initializer, but keep existing live cells unchanged. This avoids
changing real hoisted-var or parameter binding identity. Module globals and
preallocated cells stay on their existing paths. Updated tests target this
actual declaration guard (with and without initializers). Full unit and native
verification of the follow-up are pending, not claimed green.

@proggeramlug proggeramlug changed the title fix(codegen): initialize captured cells on cloned generator continuations fix(codegen): initialize missing cells in reused boxed declarations Sep 11, 2026
@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed with preserved authorship via merge train #10047 (main f6c6879; exact validated tree 48d71715f50fce6d4ec1c64b6ba5388778889aaf).

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.

codegen: async-generator yield* loop loses a captured local callback

1 participant