Skip to content

fix(hir): preserve class captures across loop iterations (#9528) - #9548

Closed
proggeramlug wants to merge 2 commits into
PerryTS:mainfrom
proggeramlug:fix/9528-loop-class-captures
Closed

fix(hir): preserve class captures across loop iterations (#9528)#9548
proggeramlug wants to merge 2 commits into
PerryTS:mainfrom
proggeramlug:fix/9528-loop-class-captures

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Fixes lexical classic-for class captures that shared the final loop value across every evaluated class instance.

What changed

  • Freshen the one-element shared-mutable capture cell at the ECMAScript per-iteration environment boundary, before the update expression runs.
  • Leave var-headed loops on their existing shared-binding path.
  • Add a structural HIR regression test and a Node-differential gap fixture covering immediate and delayed methods, field initializers, for-of, and the var control.

Verification

  • Demonstrated the fixture on unfixed main: delayed classic-loop methods returned method-3 three times while Node returned method-0, method-1, method-2.
  • cargo test -p perry-hir --lib: 376 passed, 0 failed, 1 ignored.
  • Focused parity harness: 1 passed, 0 failed; output byte-identical to Node.
  • Test registration checker passed.

Closes #9528

Summary by CodeRabbit

  • Bug Fixes

    • Fixed class closures in for (let …) loops so each iteration retains its own captured value.
    • Preserved shared binding behavior for for (var …) loops.
    • Corrected per-iteration captures across classic for and for-of loops, including delayed methods and field initializers.
  • Tests

    • Added regression coverage for lexical and var loop capture behavior.

@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 8c04899b-a138-43c7-b305-fd2cd8f93c75

📥 Commits

Reviewing files that changed from the base of the PR and between 3b9c9de and b93fc8a.

📒 Files selected for processing (4)
  • changelog.d/9548-loop-class-captures.md
  • crates/perry-hir/src/lower/shared_mutable_capture.rs
  • crates/perry-hir/src/lower/tests.rs
  • test-files/test_gap_9528_loop_class_capture.ts

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


📝 Walkthrough

Walkthrough

The loop lowering pass now creates fresh capture cells for lexical classic-for iterations before updates. Tests cover class captures in classic for, for-of, and var loops, including delayed methods and field initializers.

Changes

Loop class capture freshening

Layer / File(s) Summary
Capture-cell lowering
crates/perry-hir/src/lower/shared_mutable_capture.rs
Stmt::For records shared lexical bindings and prepends a LocalSet that copies the current value into a fresh capture cell before the update.
Capture regression coverage
crates/perry-hir/src/lower/tests.rs, test-files/test_gap_9528_loop_class_capture.ts, changelog.d/9548-loop-class-captures.md
Tests verify fresh lexical cells, retained class values, for-of captures, and shared var behavior. The changelog documents the fix.

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

Merge Risk: ⚪ Minimal · up to b93fc

The change corrects lexical class captures across classic-for loop iterations while preserving var-loop behavior, preventing methods and initializers from observing the wrong loop value. No actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: thehypnoo

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main HIR fix for preserving class captures across loop iterations.
Description check ✅ Passed The description explains the problem, lists the implementation and test changes, identifies issue #9528, and records verification results. It omits the formal checklist and optional screenshots sectio…
Linked Issues check ✅ Passed The changes address issue #9528 by freshening lexical classic-for capture cells, preserving var sharing, and adding coverage for classic for, for-of, immediate and delayed methods, field initializers,…
Out of Scope Changes check ✅ Passed The changelog entry, lowering change, structural HIR test, and regression fixture are directly related to the linked issue and stated pull request objectives. No unrelated changes are evident.
Full details: Description check

Explanation

The description explains the problem, lists the implementation and test changes, identifies issue #9528, and records verification results. It omits the formal checklist and optional screenshots section, but the required content is substantially present.

Full details: Linked Issues check

Explanation

The changes address issue #9528 by freshening lexical classic-for capture cells, preserving var sharing, and adding coverage for classic for, for-of, immediate and delayed methods, field initializers, retained instances, and the var control case.

Full details: Docstring Coverage

Explanation

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

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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
proggeramlug marked this pull request as ready for review September 2, 2026 16:21
@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed via merge train # (rebase-merge, your authorship preserved).

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Correction: the landing PR is #9549 — merged (rebase-merge, authorship preserved).

@proggeramlug
proggeramlug deleted the fix/9528-loop-class-captures branch September 2, 2026 17:42
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.

A class declared in a loop body captures stale loop variables: methods see the LAST iteration's values (c3,c3,c3 vs node c0,c1,c2)

1 participant