fix(hir): keep the pure first declarator in For::init — restores the versioned loop clones (#9106) - #9116
Conversation
…PerryTS#9106) PerryTS#9062 fixed source-order semantics for multi-declarator lexical for heads by hoisting EVERY declarator into the loop-scoped prelude and leaving `For::init = None`. That silently demoted every versioned counted-loop fast clone whose head is spelled the classic way — `for (let i = 0, length = arr.length; i < length; i++)` — because all of codegen's loop matchers (stable_packed, packed_f64, range, class-field, element-shape) identify the counter through the `For::init == Let { id, Integer(0) }` slot. The wolf-ecs nested `Query`/`Archetype` scans in issue_8690_loop_versioned_arraylike lost all three `js_packed_arraylike_loop_guard` admissions (3 -> 0); output stayed correct, only the fast clones vanished. Carve-out: when hoisting the tail declarators around the first one is provably unobservable — the first declarator is a plain identifier bound to a pure literal, and no tail declarator mentions that identifier in its pattern or initializer — keep the first declarator in `For::init` and hoist only the tail. Under those conditions the reorder can observe neither the value nor the TDZ state of the first binding, and no closure can capture it early. Every order-observable head (including both PerryTS#9052 regression fixtures, whose tails read `i`) stays on the PerryTS#9062 prelude path, and hoisted tail bindings keep their `classic_for_lexical_bindings` per-iteration capture semantics. Validation: issue_8690_loop_versioned_arraylike 3/3, issue_8773_closure_capture_packed_loops 4/4, issue_9052_for_lexical_declarators 2/2, perry-codegen --lib 1347, native_proof_regressions 284, perry-hir --lib 360 (incl. a new pin that the safe head keeps its init slot), cargo fmt clean. Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP
|
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 (6)
Included review availability: Your plan provides up to 8 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe PR restores ChangesFor-loop init-slot lowering
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to The change restores optimized lowering for eligible multi-declarator loops, but a tail initializer using direct eval may observe lexical bindings in the wrong order and change program behavior. Merge should wait for an explicit safeguard or owner acceptance of this bounded correctness risk. Sequence Diagram(s)sequenceDiagram
participant ForHeadLowering
participant InitSlotHelper
participant HIRFor
participant LexicalBindings
ForHeadLowering->>InitSlotHelper: Check first declarator and tail references
InitSlotHelper-->>ForHeadLowering: Return safe init-slot decision
ForHeadLowering->>HIRFor: Keep literal counter in For::init
ForHeadLowering->>LexicalBindings: Hoist tail declarators
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is mostly complete. It explains the regression, mechanism, contained fix, affected lowering sites, regression coverage, related issue, and validation results. It does not use the template headings or include the checklist, but the missing items are non-critical for understanding and reviewing this change. Full details: Docstring CoverageExplanation Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 5 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 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 |
|
Merged. Fixes #9106. The attribution correction is the most valuable part of this PR, and I confirmed it: #9062 is the culprit, and #9070/#9084/#9091 are innocent. Reproduced on main (
with Your note that the bisect "walked to a CI-only commit, an impossible verdict that exposed the window as wrong" is worth keeping in the changelog fragment. A bisect landing on a commit that cannot affect the subject is evidence about the window, not about the commit — easy to rationalize away instead of acting on. The carve-out is correctly conditioned. The risk here is obvious — #9062 removed this reordering precisely because it was observable — so the question is whether the three conditions are jointly sufficient. First declarator a plain identifier, initializer a pure I tested the ways it could leak — 18 shapes, byte-identical to node, and #9062's own
Cases 4, 5 and 7 are the ones that would fail if the "no tail mentions it" check were weak, and 12/13 confirm the non-literal path is untouched. Two process notes from validating this:
Validation: hir 360 passed ( |
1 participant
Fixes #9106. The culprit is #9062, not the issue's stated window — the bisect (fast IR-grep oracle: count
js_packed_arraylike_loop_guardinscan()'s IR) walked to a CI-only commit, an impossible verdict that exposed the window as wrong; the claimed-good endpoint 8b40634 already fails on this machine. #9070/#9084/#9091 are innocent.Mechanism: #9062 correctly fixed out-of-order initialization of multi-declarator lexical for heads by hoisting every declarator into a pre-loop prelude — leaving
For::init = None. But every versioned counted-loop matcher identifies the counter exclusively throughFor::init(stable_packed_loop.rs'smatch_candidatereturns None immediately). The canonical wolf-ecs idiomfor (let j = 0, length = current.length; j < length; j++)is a multi-declarator head, so the nested scan loops silently fell to generic lowering: 0 guard sites where 3 are expected, correct output, fast clones gone.Fix (HIR, contained): new predicate
for_head_first_decl_keeps_init_slot— when the FIRST declarator is a plain identifier bound to a pure literal and no tail declarator mentions it (swc Visit scan over tail patterns + initializers), the tail-only hoist is provably unobservable, so the first declarator stays inFor::initand only the tail hoists. Order-observable heads — including both #9052 fixtures, whose tails readi— stay on #9062's prelude path; hoisted tails keep theclassic_for_lexical_bindingscapture semantics. Applied at both twin lowering sites; HIR regression test pins the carve-out; changelog fragment included.Validation (at tip, post-fix):
issue_8690_loop_versioned_arraylike3/3 (guard sites 3 again),issue_8773_closure_capture_packed_loops4/4,issue_9052_for_lexical_declarators2/2 (the culprit's own wins intact), perry-codegen lib 1347/0, native_proof_regressions 284/0, perry-hir lib 360/0, fmt clean.Goal-relevant: this restores the loop family's beat-node standing (the ECS 2.3×/1.5× shape ran at generic speed on main). Implemented by a subagent in an isolated worktree; reviewed and shipped by the coordinating session.
Summary by CodeRabbit
Performance Improvements
forloops, such asfor (let i = 0, len = arr.length; i < len; i++).Bug Fixes
Tests
forloops.