fix(hir): materialize the implicit undefined for init-less let/const (#6871) - #6877
Merged
Conversation
…6871) A `let x;` declared inside a loop body kept the previous iteration's value instead of being reset to undefined, whenever the assignment happened inside a NESTED loop: for (let i = 0; i < src.length; i++) { let f: string[] | undefined; for (const n of src[i]) { if (!f) f = []; f.push(n); } // node: a, undefined, b // perry: a, a, ab } Executing a lexical declaration initializes the binding to undefined (ECMA-262 CreateMutableBinding + InitializeBinding), and re-executing it creates a FRESH binding — so each iteration must start from undefined. HIR faithfully emitted `Stmt::Let { init: None }`, but codegen allocates the slot once in the entry block and emits no store for a None init, so the stale value survived the back-edge. Assignment directly in the loop body was already correct, which is what kept this hidden. Fixing it in codegen is not possible without new information: `Stmt::Let` carries no var/let distinction and `ctx.var_hoisted_ids` never reaches perry-codegen. Lowering is where `is_var_decl` is known, so materialize the initializer there instead. `var` is deliberately excluded — it is function-scoped and hoisted, so re-executing `var x;` keeps the prior value, and the test locks that in. Found compiling the Milo compiler (32k lines of TS) with Perry. Its parser collects declaration attributes with exactly this shape: let attrs: Attribute[] | undefined; while (this.at(TokenKind.At)) { if (!attrs) attrs = []; ... } so a struct's `@derive(...)` leaked onto every following declaration and the compiled compiler rejected valid programs with spurious errors ("'@derive' is not supported on functions"). Changing that one declaration to `= undefined` fixed it, confirming the cause.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe compiler now explicitly initializes uninitialized ChangesUninitialized let reset
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6871.
Problem
A
let x;declared inside a loop body kept the previous iteration's valueinstead of starting from
undefined, whenever the assignment happened inside anested loop:
["a"] / undefined / ["b"]["a"] / ["a"] / ["a","b"]Executing a lexical declaration initializes the binding to
undefined(ECMA-262
CreateMutableBinding+InitializeBinding), and re-executing itcreates a fresh binding — so every iteration must start from
undefined.Assignment placed directly in the loop body was already correct, and writing
= undefinedexplicitly was already correct. That combination is what keptthis hidden.
Cause
HIR faithfully emitted
Stmt::Let { init: None }. Codegen allocates the slotonce in the entry block and emits no store for a
Noneinit, so the stalevalue survived the loop back-edge.
This cannot be fixed in codegen without new information:
Stmt::Letcarries novar/let distinction, and
ctx.var_hoisted_idsnever reachesperry-codegen.Lowering is where
is_var_declis known, so the implicit initializer ismaterialized there instead.
varis deliberately excluded — it is function-scoped and hoisted, sore-executing
var x;keeps the prior value.Test
test-files/test_gap_uninit_let_loop_reset.ts— nestedfor-of, nestedwhile, nestedfor, scalar and string accumulators, an outerwhile, anested loop that never runs, plus the two forms that were already correct
(explicit
= undefined, assignment directly in the body) to lock them in.Byte-identical to
node --experimental-strip-types.Regression check
This touches every
letdeclaration, so beyond the unit tests I ran adifferential sweep of 120
test_gap_*files againstnode --experimental-strip-types: 114 pass, 2 fail, 4 skipped. Bothfailures (
test_gap_2159_defineproperty_class_prototype,test_gap_2514_settracesigint) are already listed intest-parity/known_failures.json.cargo test -p perry-hirmatchesmainexactly (18 passed / 1 failed —logical_property_assignment_short_circuits_the_store_4586fails identicallyon
origin/main, verified by reverting just this hunk).Related
While testing I found the mirror-image defect in
var:var x;in a loop isreset each iteration when it should keep its value. That is pre-existing
(reproduces on released 0.5.1239, before this change) and is filed separately
as #6876. This PR neither fixes nor worsens it; the test documents the case in
a comment rather than asserting it, so no known failure gets baked in.
How this was found
Compiling the Milo compiler (https://github.com/milo-language/milo) with Perry.
Its parser collects declaration attributes with exactly this shape:
so a struct's
@derive(...)leaked onto every following declaration and thecompiled compiler rejected valid programs ("'@derive' is not supported on
functions — 'arenaNew'"). Changing that one declaration to
= undefinedfixedit, confirming the cause.
Summary by CodeRabbit
Bug Fixes
letvariables retaining values from previous loop iterations.letdeclarations reset toundefinedcorrectly, including in nested loops.Tests