fix: preserve forward closure initializers and TDZ names - #9762
fix: preserve forward closure initializers and TDZ names#9762proggeramlug wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (13)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change preserves forward-captured ChangesTDZ handling for forward captures
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change targets forward-captured const initialization and named TDZ errors. No concrete merge-blocking issue remains in the supplied record. Sequence Diagram(s)sequenceDiagram
participant Compiler
participant GeneratedCode
participant Runtime
Compiler->>Compiler: Collect TDZ binding names
Compiler->>GeneratedCode: Emit named box reads
GeneratedCode->>Runtime: Call named TDZ getter
Runtime-->>GeneratedCode: Return value or throw named ReferenceError
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 58.82% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 12 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
Holding this one — it regresses a pre-existing codegen regression test.
No PR in today's queue touches that test file, so this is a behaviour change underneath it rather than a stale expectation. The shape of the assertion suggests a pre-declaration Everything else in the 20-PR train validated together (64/64 gates, five suites), so this is the only thing held back; the other 19 are landing now. Happy to take it as soon as either the fold is gated on the binding being past its TDZ, or you can show the test's expectation is itself wrong — I did not want to assume the latter and update the test out from under it. |
|
Correction — I was wrong above. This is not a regression, and I've fixed the test instead. I claimed the fold had lost the TDZ check. It hasn't. The emitted IR for that fixture is: %r5 = call i64 @js_box_get_bits_named(i64 %r3, double %r4)The read still goes through the box, so the check is intact — this PR routes it through the named variant so the thrown My A/B was sound (passes on Fixed on my side in the train: the assertion now checks the property rather than the spelling — either helper satisfies it — and I added the assertion it was missing, that the read is not folded to the later value: assert!(
ir.contains("call i64 @js_box_get_bits(i64 ")
|| ir.contains("call i64 @js_box_get_bits_named(i64 "),
"the pre-declaration read must retain the TDZ box check:\n{ir}"
);
assert!(
!ir.contains("double 4.200000e+01"),
"the pre-declaration read must NOT be constant-folded to its later value:\n{ir}"
);I sabotage-checked that: with the box call stripped and the fold substituted in, it still fails. So the guarantee is unchanged — only the accepted spelling widened. This is going into the next train. Nothing needed from you. |
|
Landed on |
1 participant
A closure-local optimization checked only uses after a declaration. For mutually recursive
constfunctions, it could inline a later call and delete the initializer while an earlier closure still captured that binding's box. The box stayed TDZ-poisoned, so a legal call after initialization threw. The pass now checks earlier uses too and preserves initializers that earlier captures, reads, writes, or calls need.Genuine TDZ failures now name the source binding. Codegen collects names for TDZ-capable locals and passes their existing string-pool values through named checked/trusted box readers. The trusted inline load keeps its current hot path and passes the name on the cold TDZ arm. Box layout, sentinel representation, and suppression semantics are unchanged; the runtime copies the name before allocating the error.
Validation:
typeof, updates, nested shadows, Unicode names, and subsequent successful reads.claude install) #9718), default-parameter TDZ, and class-forward-capture (class methods referencing a const declared after the class: capture omitted (ReferenceError) or spurious TDZ at class definition — kills Next.js standalone boot (bundled semver) #6523) parity fixtures pass.PERRY_CONCAT_SITE_CACHE(fixed separately by fix(cache): register concat switch and explain codegen inputs #9748); 1,083 compiler tests pass.Fixes #9721.
Summary by CodeRabbit
constfunctions and forward-referenced closures.ReferenceErrormessages now identify the affected variable, including captured, updated, nested, and Unicode bindings.