Emit OP_CLOSE_UPVALUE on break so closures pin per-iteration values#573
Merged
Conversation
…537) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughIntroduces a thread-local ChangesBreak Upvalue Closure Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/units/Goccia.Compiler.Statements.pas`:
- Around line 2969-2976: The close-upvalue loop uses GBreakScopeDepth to decide
which locals to keep, but CompileSwitchStatement doesn't initialize
GBreakScopeDepth so breaks from switch may use a stale value; fix by having
CompileSwitchStatement save the old GBreakScopeDepth, set GBreakScopeDepth to
the switch's intended break target depth (e.g. use ACtx.Scope.Depth or the scope
depth representing the switch's exit) before compiling case bodies / emitting
OP_BREAK, then restore the saved GBreakScopeDepth afterward so the for-loop in
the close-upvalue pass (the code iterating ACtx.Scope locals and checking
Local.Depth <= GBreakScopeDepth) sees the correct depth and won't close outer
captured locals mistakenly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8c66c9e-8557-4f8a-8639-494453c10eb1
📒 Files selected for processing (3)
source/units/Goccia.Compiler.Statements.pastests/language/for-loop/break-closure.jstests/language/for-of/for-of-break-closure.js
Contributor
Benchmark Results407 benchmarks Interpreted: 🟢 393 improved · 🔴 11 regressed · 3 unchanged · avg +38.7% arraybuffer.js — Interp: 🟢 14 · avg +40.8% · Bytecode: 🟢 9, 🔴 1, 4 unch. · avg +3.4%
arrays.js — Interp: 🟢 19 · avg +40.3% · Bytecode: 🟢 13, 6 unch. · avg +4.9%
async-await.js — Interp: 🟢 5, 1 unch. · avg +42.8% · Bytecode: 🟢 2, 4 unch. · avg +3.0%
async-generators.js — Interp: 🟢 2 · avg +47.5% · Bytecode: 🟢 1, 1 unch. · avg +1.3%
base64.js — Interp: 🟢 10 · avg +42.5% · Bytecode: 🟢 8, 2 unch. · avg +4.6%
classes.js — Interp: 🟢 31 · avg +36.0% · Bytecode: 🟢 12, 🔴 1, 18 unch. · avg +2.6%
closures.js — Interp: 🟢 11 · avg +40.7% · Bytecode: 🟢 9, 2 unch. · avg +7.8%
collections.js — Interp: 🟢 12 · avg +41.2% · Bytecode: 🟢 7, 5 unch. · avg +3.4%
csv.js — Interp: 🟢 13 · avg +41.9% · Bytecode: 🟢 12, 1 unch. · avg +8.6%
destructuring.js — Interp: 🟢 22 · avg +39.1% · Bytecode: 🟢 11, 🔴 1, 10 unch. · avg +3.6%
fibonacci.js — Interp: 🟢 8 · avg +42.1% · Bytecode: 🟢 4, 4 unch. · avg +6.6%
float16array.js — Interp: 🟢 32 · avg +33.2% · Bytecode: 🟢 11, 🔴 8, 13 unch. · avg -5.9%
for-of.js — Interp: 🟢 7 · avg +36.7% · Bytecode: 7 unch. · avg +3.2%
generators.js — Interp: 🟢 4 · avg +41.6% · Bytecode: 🟢 4 · avg +9.0%
iterators.js — Interp: 🟢 42 · avg +38.8% · Bytecode: 🟢 20, 🔴 4, 18 unch. · avg +2.4%
json.js — Interp: 🟢 20 · avg +43.8% · Bytecode: 🟢 15, 5 unch. · avg +7.9%
jsx.jsx — Interp: 🟢 21 · avg +31.0% · Bytecode: 🟢 10, 11 unch. · avg +4.2%
modules.js — Interp: 🟢 9 · avg +39.2% · Bytecode: 🟢 8, 1 unch. · avg +8.0%
numbers.js — Interp: 🟢 11 · avg +42.6% · Bytecode: 🟢 11 · avg +12.4%
objects.js — Interp: 🟢 7 · avg +35.5% · Bytecode: 🟢 5, 2 unch. · avg +4.7%
promises.js — Interp: 🟢 12 · avg +34.4% · Bytecode: 🟢 8, 4 unch. · avg +5.4%
regexp.js — Interp: 🟢 11 · avg +41.3% · Bytecode: 🟢 10, 1 unch. · avg +10.7%
strings.js — Interp: 🟢 19 · avg +35.6% · Bytecode: 🟢 14, 5 unch. · avg +6.3%
tsv.js — Interp: 🟢 9 · avg +45.4% · Bytecode: 🟢 8, 1 unch. · avg +10.5%
typed-arrays.js — Interp: 🟢 20, 🔴 1, 1 unch. · avg +34.1% · Bytecode: 🟢 10, 🔴 6, 6 unch. · avg -11.0%
uint8array-encoding.js — Interp: 🟢 11, 🔴 7 · avg +21.6% · Bytecode: 🟢 11, 🔴 2, 5 unch. · avg +7.2%
weak-collections.js — Interp: 🟢 11, 🔴 3, 1 unch. · avg +69.0% · Bytecode: 🟢 9, 🔴 4, 2 unch. · avg +14.0%
Deterministic profile diffDeterministic profile diff: no significant changes. Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
Suite TimingTest Runner (interpreted: 8,918 passed; bytecode: 8,918 passed)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Test runner worker shutdown frees thread-local heaps in bulk; that shutdown reclamation is not counted as GC collections or collected objects.
Benchmarks (interpreted: 407; bytecode: 407)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Benchmark runner performs explicit between-file collections, so collection and collected-object counts can be much higher than the test runner.
Measured on ubuntu-latest x64. |
Contributor
test262 Conformance
Areas closest to 100%
Per-test deltas (+1 / -0)Newly passing (1):
Steady-state failures are non-blocking; regressions vs the cached main baseline (lower total pass count, or any PASS → non-PASS transition) fail the conformance gate. Measured on ubuntu-latest x64, bytecode mode. Areas grouped by the first two test262 path components; minimum 25 attempted tests, areas already at 100% excluded. Δ vs main compares against the most recent cached |
…, try-finally Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/units/Goccia.Compiler.Statements.pas (2)
1956-2053:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
for await...ofstill skips iterator cleanup on abrupt exit.This path never mirrors the sync
CompileForOfStatementcleanup flow: there’s no pending iterator-close entry and no iterator-close emission onbreak/return/throw. So the new upvalue fix works, but async iterators with observablereturn()cleanup still leak that side effect when the loop exits abruptly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/units/Goccia.Compiler.Statements.pas` around lines 1956 - 2053, CompileForAwaitOfStatement is missing the iterator-close handling on abrupt exits; mirror the sync CompileForOfStatement flow by registering a pending finally entry for iterator cleanup before the loop and emitting an OP_ITER_CLOSE on abrupt exits. Specifically, in CompileForAwaitOfStatement add the same GPendingFinally push/entry (so GBreakFinallyBase/GContinueFinallyBase include the iterator close), ensure BreakJumps/ContinueJumps are routed through that finally region, and emit an OP_ITER_CLOSE (via EmitInstruction with EncodeABC(OP_ITER_CLOSE,...)) at the cleanup site (and/or in the finally trampoline) so breaks/returns/throws call the async iterator’s return() before leaving; use the existing ExitJump, LoopStart, BreakJumps, ContinueJumps, GPendingFinally and EmitJumpInstruction/ PatchJumpTarget locations to integrate this behavior.
2957-2982:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPop pending-finally entries before compiling them on
break.
EmitPendingCleanupandCompileContinueStatementsnapshot and remove pending entries before compiling cleanup, butCompileBreakStatementstill compiles them in-place. That means afinallycontaining its own abrupt completion can observe the same pending entry again and emit the cleanup twice.try { break; } finally { return 1; }is the minimal reproducer.Suggested shape
procedure CompileBreakStatement(const ACtx: TGocciaCompilationContext); var - I: Integer; + I, Count, Base: Integer; Entry: TPendingFinallyEntry; Local: TGocciaCompilerLocal; + Entries: array of TPendingFinallyEntry; begin if not Assigned(GBreakJumps) then Exit; if Assigned(GPendingFinally) and (GPendingFinally.Count > GBreakFinallyBase) then - for I := GPendingFinally.Count - 1 downto GBreakFinallyBase do + begin + Count := GPendingFinally.Count; + Base := GBreakFinallyBase; + SetLength(Entries, Count - Base); + for I := Base to Count - 1 do + Entries[I - Base] := GPendingFinally[I]; + for I := Count - 1 downto Base do begin - Entry := GPendingFinally[I]; + if GPendingFinally.Count > I then + GPendingFinally.Delete(I); + Entry := Entries[I - Base]; EmitPendingEntryCleanup(ACtx, Entry, True); end; + for I := 0 to Length(Entries) - 1 do + GPendingFinally.Insert(Base + I, Entries[I]); + end; for I := ACtx.Scope.LocalCount - 1 downto 0 do begin Local := ACtx.Scope.GetLocal(I);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/units/Goccia.Compiler.Statements.pas` around lines 2957 - 2982, In CompileBreakStatement the loop over GPendingFinally must operate on a snapshot and remove those entries first (like CompileContinueStatement/EmitPendingCleanup do) to avoid double-emitting; change the code so that you copy GPendingFinally entries from index GBreakFinallyBase..Count-1 into a local list/array, reduce/pop GPendingFinally to GBreakFinallyBase immediately, then iterate the snapshot and call EmitPendingEntryCleanup(ACtx, Entry, True) for each; keep the rest of CompileBreakStatement (local cleanup and GBreakJumps.Add(...)) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@source/units/Goccia.Compiler.Statements.pas`:
- Around line 1956-2053: CompileForAwaitOfStatement is missing the
iterator-close handling on abrupt exits; mirror the sync CompileForOfStatement
flow by registering a pending finally entry for iterator cleanup before the loop
and emitting an OP_ITER_CLOSE on abrupt exits. Specifically, in
CompileForAwaitOfStatement add the same GPendingFinally push/entry (so
GBreakFinallyBase/GContinueFinallyBase include the iterator close), ensure
BreakJumps/ContinueJumps are routed through that finally region, and emit an
OP_ITER_CLOSE (via EmitInstruction with EncodeABC(OP_ITER_CLOSE,...)) at the
cleanup site (and/or in the finally trampoline) so breaks/returns/throws call
the async iterator’s return() before leaving; use the existing ExitJump,
LoopStart, BreakJumps, ContinueJumps, GPendingFinally and EmitJumpInstruction/
PatchJumpTarget locations to integrate this behavior.
- Around line 2957-2982: In CompileBreakStatement the loop over GPendingFinally
must operate on a snapshot and remove those entries first (like
CompileContinueStatement/EmitPendingCleanup do) to avoid double-emitting; change
the code so that you copy GPendingFinally entries from index
GBreakFinallyBase..Count-1 into a local list/array, reduce/pop GPendingFinally
to GBreakFinallyBase immediately, then iterate the snapshot and call
EmitPendingEntryCleanup(ACtx, Entry, True) for each; keep the rest of
CompileBreakStatement (local cleanup and GBreakJumps.Add(...)) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9f17dc5-484d-42ea-8697-fbfda5f7e9fa
📒 Files selected for processing (2)
source/units/Goccia.Compiler.Statements.pastests/language/for-of/for-of-break-closure.js
… is not bypassed The switch compiler already routes break jumps through EndScope/OP_CLOSE_UPVALUE by patching breaks before EndScope. Setting GBreakScopeDepth after BeginScope ensures CompileBreakStatement does not emit a redundant inline close for the switch scope's own locals. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Mirror CompileContinueStatement's pattern: snapshot entries above GBreakFinallyBase, remove from GPendingFinally before compiling cleanup, then restore. Prevents double handler cleanup when a finally block contains a nested break. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
breakfrom loop bodies skipped per-iterationOP_CLOSE_UPVALUE, leaving closure cells attached to register slotsGBreakScopeDepththreadvar soCompileBreakStatementknows which scope depth to close down to, and emitsOP_CLOSE_UPVALUEfor any captured locals in intermediate scopes before the break jumpCompileCountedForOf,CompileForOfStatement,CompileForAwaitOfStatement,TryCompileCountedFor,CompileForStatementCloses #537
Test plan
tests/language/for-of/for-of-break-closure.js— 5 tests covering break+closure for general for-of, counted for-of, destructured binding, nested loopstests/language/for-loop/break-closure.js— 4 tests covering break+closure for traditional for-loops with let bindingsOP_CLOSE_UPVALUEnow fires on the break path (was 0, now 1)./format.pas --checkpasses🤖 Generated with Claude Code