Fix IteratorClose for abrupt for-of completion#488
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds iterator finalization to for-of loops: both interpreter and bytecode now invoke iterator Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler
participant BytecodeVM
participant IteratorObj
participant HostCode
HostCode->>Compiler: compile for-of (emit handler + OP_ITER_CLOSE)
Compiler-->>HostCode: bytecode with OP_ITER_CLOSE and pending-finally entry
HostCode->>BytecodeVM: execute bytecode (enter loop)
BytecodeVM->>IteratorObj: getIterator / next()
alt body completes abruptly (throw / break / return)
BytecodeVM->>BytecodeVM: invoke pending-finally handler
BytecodeVM->>IteratorObj: OP_ITER_CLOSE -> call iterator.return() if present (preserve original error)
BytecodeVM-->>HostCode: rethrow original abrupt completion
else iteration continues
BytecodeVM->>IteratorObj: continue loop / next()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/units/Goccia.Compiler.Statements.pas (1)
1653-1669:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove active pending entries before compiling
breakcleanup.
CompileBreakStatementemits each pending cleanup while the same entry is still present inGPendingFinally. If that cleanup block containsreturn/break/continue(for example,breakinsidetry { ... } finally { return ... }),EmitPendingCleanupsees the same entry again and recursively recompiles it. That can blow the compiler stack and also duplicate iterator/disposal cleanup. Mirror the snapshot/delete/restore flow you already use inCompileContinueStatement.Suggested fix
procedure CompileBreakStatement(const ACtx: TGocciaCompilationContext); var - I: Integer; + I, Count, Base: Integer; + Entries: array of TPendingFinallyEntry; Entry: TPendingFinallyEntry; begin if not Assigned(GBreakJumps) then Exit; - if Assigned(GPendingFinally) and (GPendingFinally.Count > GBreakFinallyBase) then + if Assigned(GPendingFinally) and (GPendingFinally.Count > GBreakFinallyBase) then + 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 + 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 := GPendingFinally.Count - 1 downto GBreakFinallyBase do - begin - Entry := GPendingFinally[I]; - EmitPendingEntryCleanup(ACtx, Entry, True); - end; GBreakJumps.Add(EmitJumpInstruction(ACtx, OP_JUMP, 0)); end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.Compiler.Statements.pas` around lines 1653 - 1669, CompileBreakStatement currently calls EmitPendingEntryCleanup while the same entries remain in GPendingFinally, which allows recursive recompilation; fix it by mirroring CompileContinueStatement's snapshot/delete/restore flow: when there are pending finally entries above GBreakFinallyBase, copy the range (GPendingFinally[GBreakFinallyBase..Count-1]) into a local list/array, remove those entries from GPendingFinally (truncate Count to GBreakFinallyBase), then iterate the local copy calling EmitPendingEntryCleanup(ACtx, Entry, True), and finally restore the removed entries back into GPendingFinally so state is preserved; keep the final GBreakJumps.Add(EmitJumpInstruction(ACtx, OP_JUMP, 0)) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@source/units/Goccia.Compiler.Statements.pas`:
- Around line 1653-1669: CompileBreakStatement currently calls
EmitPendingEntryCleanup while the same entries remain in GPendingFinally, which
allows recursive recompilation; fix it by mirroring CompileContinueStatement's
snapshot/delete/restore flow: when there are pending finally entries above
GBreakFinallyBase, copy the range (GPendingFinally[GBreakFinallyBase..Count-1])
into a local list/array, remove those entries from GPendingFinally (truncate
Count to GBreakFinallyBase), then iterate the local copy calling
EmitPendingEntryCleanup(ACtx, Entry, True), and finally restore the removed
entries back into GPendingFinally so state is preserved; keep the final
GBreakJumps.Add(EmitJumpInstruction(ACtx, OP_JUMP, 0)) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67a4c13d-d562-4851-b929-a356cdf2cfef
📒 Files selected for processing (6)
source/units/Goccia.Bytecode.OpCodeNames.passource/units/Goccia.Bytecode.passource/units/Goccia.Compiler.Statements.passource/units/Goccia.Evaluator.passource/units/Goccia.VM.pastests/language/for-of/for-of-iterators.js
# Conflicts: # source/units/Goccia.Bytecode.pas
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/units/Goccia.VM.pas (1)
7835-7862: ⚡ Quick winReuse the shared abrupt-close helper instead of open-coding iterator close
Line 7837 onward duplicates close behavior and can drift from the canonical IteratorClose paths. Using
CloseRawIteratorPreservingErrorhere keeps VM close semantics centralized while still preserving the original abrupt completion.♻️ Proposed refactor
OP_ITER_CLOSE: begin - try - if (FRegisters[A].Kind = grkObject) and - (FRegisters[A].ObjectValue is TGocciaIteratorValue) then - TGocciaIteratorValue(FRegisters[A].ObjectValue).Close - else if (FRegisters[A].Kind = grkObject) and - (FRegisters[A].ObjectValue is TGocciaObjectValue) then - begin - IterResult := FRegisters[A].ObjectValue; - NextMethod := IterResult.GetProperty(PROP_RETURN); - if Assigned(NextMethod) and - not (NextMethod is TGocciaUndefinedLiteralValue) and - not (NextMethod is TGocciaNullLiteralValue) and - NextMethod.IsCallable then - begin - CallArgs := AcquireArguments; - try - TGocciaFunctionBase(NextMethod).Call(CallArgs, IterResult); - finally - ReleaseArguments(CallArgs); - end; - end; - end; - except - // Iterator close errors must not replace the original abrupt completion. - end; + if FRegisters[A].Kind = grkObject then + CloseRawIteratorPreservingError(RegisterToValue(FRegisters[A])); end;Based on learnings:
CloseRawIterator/CloseRawIteratorPreservingErrorare the canonical normal/abrupt IteratorClose paths insource/units/Goccia.VM.pas.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/units/Goccia.VM.pas` around lines 7835 - 7862, The OP_ITER_CLOSE case is reimplementing iterator-close logic instead of using the canonical helper; replace the try/except block and open-coded close logic in the OP_ITER_CLOSE branch with a call to the shared helper CloseRawIteratorPreservingError (or CloseRawIterator for non-abrupt semantics) passing the iterator register (FRegisters[A] or its ObjectValue) so the VM uses the centralized IteratorClose path and preserves the original abrupt completion; remove the duplicated manual property lookup/CallArgs handling and ensure the call uses the same value/kind you currently check (e.g. when FRegisters[A].Kind = grkObject) so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@source/units/Goccia.VM.pas`:
- Around line 7835-7862: The OP_ITER_CLOSE case is reimplementing iterator-close
logic instead of using the canonical helper; replace the try/except block and
open-coded close logic in the OP_ITER_CLOSE branch with a call to the shared
helper CloseRawIteratorPreservingError (or CloseRawIterator for non-abrupt
semantics) passing the iterator register (FRegisters[A] or its ObjectValue) so
the VM uses the centralized IteratorClose path and preserves the original abrupt
completion; remove the duplicated manual property lookup/CallArgs handling and
ensure the call uses the same value/kind you currently check (e.g. when
FRegisters[A].Kind = grkObject) so behavior is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b9a5f63-f3fa-4118-9e14-b3d87594bd85
📒 Files selected for processing (3)
source/units/Goccia.Bytecode.passource/units/Goccia.Evaluator.passource/units/Goccia.VM.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- source/units/Goccia.Evaluator.pas
Contributor
Benchmark Results407 benchmarks Interpreted: 🟢 9 improved · 🔴 364 regressed · 34 unchanged · avg -7.6% arraybuffer.js — Interp: 🔴 12, 2 unch. · avg -8.4% · Bytecode: 🟢 4, 10 unch. · avg +1.1%
arrays.js — Interp: 🔴 18, 1 unch. · avg -9.9% · Bytecode: 🟢 11, 8 unch. · avg +2.4%
async-await.js — Interp: 🔴 4, 2 unch. · avg -6.4% · Bytecode: 🟢 2, 4 unch. · avg +1.9%
async-generators.js — Interp: 🔴 2 · avg -9.6% · Bytecode: 🟢 1, 1 unch. · avg +0.6%
base64.js — Interp: 🔴 10 · avg -7.0% · Bytecode: 🟢 3, 7 unch. · avg +1.5%
classes.js — Interp: 🔴 30, 1 unch. · avg -7.4% · Bytecode: 🟢 1, 🔴 1, 29 unch. · avg -0.3%
closures.js — Interp: 🔴 11 · avg -9.6% · Bytecode: 🟢 5, 6 unch. · avg +2.0%
collections.js — Interp: 🔴 11, 1 unch. · avg -9.1% · Bytecode: 🟢 3, 🔴 3, 6 unch. · avg +0.3%
csv.js — Interp: 🔴 12, 1 unch. · avg -8.7% · Bytecode: 🟢 1, 12 unch. · avg +0.5%
destructuring.js — Interp: 🔴 22 · avg -9.4% · Bytecode: 🟢 3, 🔴 3, 16 unch. · avg +0.1%
fibonacci.js — Interp: 🔴 7, 1 unch. · avg -7.8% · Bytecode: 🟢 4, 4 unch. · avg +1.2%
float16array.js — Interp: 🟢 3, 🔴 26, 3 unch. · avg -5.2% · Bytecode: 🟢 5, 🔴 12, 15 unch. · avg -5.6%
for-of.js — Interp: 🔴 7 · avg -5.4% · Bytecode: 🔴 4, 3 unch. · avg -5.5%
generators.js — Interp: 🔴 4 · avg -9.0% · Bytecode: 🔴 1, 3 unch. · avg -1.2%
iterators.js — Interp: 🔴 35, 7 unch. · avg -5.9% · Bytecode: 🟢 15, 🔴 9, 18 unch. · avg +1.3%
json.js — Interp: 🟢 1, 🔴 18, 1 unch. · avg -9.8% · Bytecode: 🟢 1, 19 unch. · avg +0.9%
jsx.jsx — Interp: 🔴 18, 3 unch. · avg -5.0% · Bytecode: 🟢 1, 🔴 5, 15 unch. · avg -1.9%
modules.js — Interp: 🔴 6, 3 unch. · avg -9.3% · Bytecode: 🟢 1, 8 unch. · avg +0.3%
numbers.js — Interp: 🔴 10, 1 unch. · avg -11.6% · Bytecode: 🟢 2, 9 unch. · avg +2.3%
objects.js — Interp: 🔴 7 · avg -8.8% · Bytecode: 7 unch. · avg +0.2%
promises.js — Interp: 🔴 10, 2 unch. · avg -6.5% · Bytecode: 🔴 3, 9 unch. · avg -1.8%
regexp.js — Interp: 🔴 11 · avg -10.3% · Bytecode: 🟢 5, 6 unch. · avg +3.0%
strings.js — Interp: 🔴 18, 1 unch. · avg -6.4% · Bytecode: 🟢 7, 12 unch. · avg +2.2%
tsv.js — Interp: 🔴 9 · avg -8.9% · Bytecode: 🔴 4, 5 unch. · avg -3.1%
typed-arrays.js — Interp: 🟢 5, 🔴 16, 1 unch. · avg -0.8% · Bytecode: 🟢 6, 🔴 6, 10 unch. · avg +9.4%
uint8array-encoding.js — Interp: 🔴 15, 3 unch. · avg -3.4% · Bytecode: 🔴 1, 17 unch. · avg -2.3%
weak-collections.js — Interp: 🔴 15 · avg -19.8% · Bytecode: 🟢 3, 🔴 7, 5 unch. · avg -5.4%
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,418 passed; bytecode: 8,418 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%
Non-blocking. 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 |
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
return()for abruptfor...ofcompletion in the interpreterFixes #482