De-couple control flow from exception flow#45
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces exception-based return/break signaling with a bit-packed TGocciaControlFlow record; many statement-level evaluator functions now return TGocciaControlFlow and callers propagate non-normal kinds (cfkReturn/cfkBreak) by early exit. New unit Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (Interpreter / Function)
participant Evaluator as Evaluator
participant CF as TGocciaControlFlow
participant Throw as TGocciaThrowValue
Caller->>Evaluator: EvaluateStatement(node, ctx)
Evaluator->>Evaluator: execute statement logic
alt normal completion
Evaluator-->>CF: Normal(value)
Evaluator-->>Caller: TGocciaControlFlow(kind: cfkNormal, value)
Caller->>Caller: if kind = cfkNormal then continue (use value)
else return encountered
Evaluator-->>CF: Return(value)
Evaluator-->>Caller: TGocciaControlFlow(kind: cfkReturn, value)
Caller->>Caller: if kind = cfkReturn then early exit with value
else break encountered
Evaluator-->>CF: Break(nil)
Evaluator-->>Caller: TGocciaControlFlow(kind: cfkBreak, nil)
Caller->>Caller: if kind = cfkBreak then handle loop/switch break
end
Note right of Throw: TGocciaThrowValue still propagates as an exception for JS throws
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Benchmark Results254 benchmarks · 🟢 72 improved · 182 unchanged · avg +5.8% arraybuffer.js — 14 unchanged · avg +4.4%
arrays.js — 🟢 3 improved, 16 unchanged · avg +3.0%
async-await.js — 🟢 4 improved, 2 unchanged · avg +7.1%
classes.js — 🟢 3 improved, 28 unchanged · avg +4.7%
closures.js — 🟢 2 improved, 9 unchanged · avg +6.8%
collections.js — 🟢 2 improved, 10 unchanged · avg +5.2%
destructuring.js — 🟢 10 improved, 12 unchanged · avg +7.1%
fibonacci.js — 🟢 3 improved, 5 unchanged · avg +7.3%
for-of.js — 🟢 7 improved · avg +9.7%
iterators.js — 🟢 18 improved, 2 unchanged · avg +11.5%
json.js — 🟢 8 improved, 12 unchanged · avg +7.2%
jsx.jsx — 21 unchanged · avg +3.5%
numbers.js — 🟢 2 improved, 9 unchanged · avg +5.2%
objects.js — 🟢 1 improved, 6 unchanged · avg +4.3%
promises.js — 🟢 7 improved, 5 unchanged · avg +6.9%
strings.js — 🟢 2 improved, 9 unchanged · avg +4.2%
typed-arrays.js — 22 unchanged · avg +3.3%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/architecture.md (1)
200-216:⚠️ Potential issue | 🟡 MinorThe evaluator section still describes a
TGocciaValue-only contract.These additions introduce
TGocciaControlFlow, but the nearby prose and the Script Execution diagram still say the evaluator returnsTGocciaValue. Please update those spots too so the docs stay internally consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/architecture.md` around lines 200 - 216, Update the evaluator documentation and the Script Execution diagram to reflect that evaluator routines return TGocciaControlFlow (which can carry a TGocciaValue) instead of a TGocciaValue-only contract; change prose that references "returns TGocciaValue" to "returns TGocciaControlFlow" and mention how TGocciaControlFlow encodes cfkNormal/cfkReturn/cfkBreak plus an optional value, and update any examples or labels that reference EvaluateStatements, EvaluateBlock, EvaluateTry, or the Script Execution diagram to use TGocciaControlFlow semantics consistently.units/Goccia.Evaluator.pas (1)
1638-1651:⚠️ Potential issue | 🟠 MajorRoute
try/catch/finallybodies throughEvaluateBlock.These paths now execute raw
.Nodeslists, solet/ destructuring declarations insidetry,catch, orfinallyskip the normal block-scope handling and leak into the surrounding scope. Atry { let x = 1 } finally {}will currently bindxoutside thetry.♻️ Suggested fix
- Result := EvaluateStatements(ATryStatement.CatchBlock.Nodes, CatchContext); + Result := EvaluateBlock(ATryStatement.CatchBlock, CatchContext); ... - Result := EvaluateStatements(ATryStatement.CatchBlock.Nodes, AContext); + Result := EvaluateBlock(ATryStatement.CatchBlock, AContext); ... - Result := EvaluateStatements(ATryStatement.Block.Nodes, AContext); + Result := EvaluateBlock(ATryStatement.Block, AContext); ... - FinallyCF := EvaluateStatements(ATryStatement.FinallyBlock.Nodes, AContext); + FinallyCF := EvaluateBlock(ATryStatement.FinallyBlock, AContext);Also applies to: 1678-1727
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Evaluator.pas` around lines 1638 - 1651, The try/catch/finally bodies currently call EvaluateStatements with raw .Nodes which bypasses block-scoped handling and causes let/destructuring to leak; change calls to EvaluateStatements(… .Nodes, …) to call EvaluateBlock with the Block node itself so block scope is applied: e.g. replace EvaluateStatements(ATryStatement.CatchBlock.Nodes, CatchContext) with EvaluateBlock(ATryStatement.CatchBlock, CatchContext) (and similarly for the main try block and finally block), keeping the existing CatchScope creation/assignment (TGocciaCatchScope, CatchContext.Scope) and return handling otherwise unchanged; apply the same replacement for the other occurrence handling the try/finally paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.ControlFlow.pas`:
- Around line 13-18: TGocciaControlFlow currently holds a TGocciaValue in a
plain record so a Pascal-only reference can be reclaimed by the GC; fix this by
making the record self-managing: add class operator Initialize(var Dest:
TGocciaControlFlow) and Finalize(var Dest: TGocciaControlFlow) and in Finalize
call RemoveTempRoot(Dest.Value) when Dest.Value is non-nil, and in the factory
methods Normal(const AValue: TGocciaValue) and Return(const AValue:
TGocciaValue) call AddTempRoot(AValue) before assigning Result.Value (ensure
Break sets Result.Value to a nil/empty value), so every control-flow instance
roots its payload for its lifetime and releases it when the record is finalized.
In `@units/Goccia.Evaluator.pas`:
- Around line 1691-1739: The pending thrown value (ThrownValue) must be
temp-rooted while the finally block runs to prevent GC collecting it; modify the
try/finally handling in the evaluator so that if HasUnhandledThrow is true you
call AddTempRoot(ThrownValue) immediately before
EvaluateStatements(ATryStatement.FinallyBlock.Nodes, AContext) and ensure
RemoveTempRoot(ThrownValue) is called on every control-flow path after the
finally runs (i.e. before you Exit when FinallyCF.Kind <> cfkNormal and before
re-raising the TGocciaThrowValue at the end); reference ThrownValue,
HasUnhandledThrow, EvaluateStatements, FinallyCF, AddTempRoot and RemoveTempRoot
when locating where to add these calls.
- Around line 1886-1909: The unconditional Continue when CaseClause.Test is nil
causes the default clause to be skipped even if a prior branch matched; change
the logic in the case evaluation loop so DefaultIndex := I is still recorded but
only skip to next iteration when no prior match exists (i.e. only Continue if
not Matched). Locate the loop using symbols CaseClause.Test, DefaultIndex,
Matched, Discriminant, CaseTest, EvaluateExpression and IsStrictEqual and adjust
the early-branch handling so default can participate in fallthrough after a
match while preserving recording of the default index.
In `@units/Goccia.Values.FunctionValue.pas`:
- Around line 224-240: The function body evaluator (in
TGocciaFunctionValue.ExecuteBody where FBodyStatements are iterated and CF :=
Evaluate(...) is used) currently treats cfkBreak by silently Exit which converts
stray break statements into an undefined return; change the cfkBreak branch so
that if a break reaches this function boundary you raise a syntax/runtime error
instead (e.g. raise a SyntaxError/EGocciaSyntaxError with message "Illegal break
statement") rather than exiting; locate the cfkBreak handling in the loop that
checks CF.Kind and replace the Exit with a thrown error so invalid standalone
breaks (evaluated to cfkBreak by TGocciaBreakStatement) are reported.
---
Outside diff comments:
In `@docs/architecture.md`:
- Around line 200-216: Update the evaluator documentation and the Script
Execution diagram to reflect that evaluator routines return TGocciaControlFlow
(which can carry a TGocciaValue) instead of a TGocciaValue-only contract; change
prose that references "returns TGocciaValue" to "returns TGocciaControlFlow" and
mention how TGocciaControlFlow encodes cfkNormal/cfkReturn/cfkBreak plus an
optional value, and update any examples or labels that reference
EvaluateStatements, EvaluateBlock, EvaluateTry, or the Script Execution diagram
to use TGocciaControlFlow semantics consistently.
In `@units/Goccia.Evaluator.pas`:
- Around line 1638-1651: The try/catch/finally bodies currently call
EvaluateStatements with raw .Nodes which bypasses block-scoped handling and
causes let/destructuring to leak; change calls to EvaluateStatements(… .Nodes,
…) to call EvaluateBlock with the Block node itself so block scope is applied:
e.g. replace EvaluateStatements(ATryStatement.CatchBlock.Nodes, CatchContext)
with EvaluateBlock(ATryStatement.CatchBlock, CatchContext) (and similarly for
the main try block and finally block), keeping the existing CatchScope
creation/assignment (TGocciaCatchScope, CatchContext.Scope) and return handling
otherwise unchanged; apply the same replacement for the other occurrence
handling the try/finally paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ecf6c619-b757-4a9d-8683-3cc0cbf52745
📒 Files selected for processing (10)
AGENTS.mddocs/architecture.mddocs/code-style.mddocs/design-decisions.mdunits/Goccia.ControlFlow.pasunits/Goccia.Evaluator.pasunits/Goccia.Interpreter.pasunits/Goccia.Values.Error.pasunits/Goccia.Values.FunctionValue.Test.pasunits/Goccia.Values.FunctionValue.pas
- Temp-root ThrownValue during finally block execution in EvaluateTry to prevent GC from collecting the pending throw value - Throw SyntaxError for illegal break statements that escape to ExecuteBody instead of silently returning undefined Made-with: Cursor
Owner
Author
Acknowledged. The benchmark regressions are significant and need investigation. The likely cause is the overhead of returning TGocciaControlFlow records (16 bytes) from Evaluate instead of TGocciaValue pointers (8 bytes) — this affects every AST evaluation, not just control flow. The record creation in TGocciaControlFlow.Normal() adds per-expression overhead. This needs profiling to determine whether FPC is inlining the record factory or generating unnecessary memory operations. The correctness improvements (temp-rooting ThrownValue, SyntaxError for illegal break) are in 6281579. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
units/Goccia.Evaluator.pas (1)
1893-1916:⚠️ Potential issue | 🔴 Critical
defaultclause does not participate in fallthrough after a prior match.The unconditional
Continueat line 1896 skips thedefaultclause even whenMatchedis already true from an earlier case. Forswitch(1) { case 1: ...; default: ... }without a break, thedefaultbody is never executed when1matches.♻️ Suggested fix
if not Assigned(CaseClause.Test) then begin DefaultIndex := I; - Continue; - end; - - if not Matched then + if not Matched then + Continue; + end + else if not Matched then begin CaseTest := EvaluateExpression(CaseClause.Test, AContext); if IsStrictEqual(Discriminant, CaseTest) then - Matched := True; - end; + Matched := True + else + Continue; + end; if Matched then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Evaluator.pas` around lines 1893 - 1916, The default clause is skipped unconditionally because the code sets DefaultIndex and always does Continue when CaseClause.Test is nil; modify the logic so when CaseClause.Test is nil you still set DefaultIndex := I but only do Continue if not Matched (i.e., change the block around CaseClause.Test to: if not Assigned(CaseClause.Test) then begin DefaultIndex := I; if not Matched then Continue; end) so that a prior Matched value allows fallthrough into the default Consequent; keep the rest of the evaluation flow (EvaluateExpression, IsStrictEqual, Matched, Consequent loop, CF.Kind checks) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@units/Goccia.Evaluator.pas`:
- Around line 1893-1916: The default clause is skipped unconditionally because
the code sets DefaultIndex and always does Continue when CaseClause.Test is nil;
modify the logic so when CaseClause.Test is nil you still set DefaultIndex := I
but only do Continue if not Matched (i.e., change the block around
CaseClause.Test to: if not Assigned(CaseClause.Test) then begin DefaultIndex :=
I; if not Matched then Continue; end) so that a prior Matched value allows
fallthrough into the default Consequent; keep the rest of the evaluation flow
(EvaluateExpression, IsStrictEqual, Matched, Consequent loop, CF.Kind checks)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6343151b-7769-4cd0-88b9-1c1bc55eb8ab
📒 Files selected for processing (2)
units/Goccia.Evaluator.pasunits/Goccia.Values.FunctionValue.pas
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on April 11. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
units/Goccia.Evaluator.pas (1)
1403-1404:⚠️ Potential issue | 🟠 MajorUse
ThrowTypeErrorhere, not a rawTGocciaTypeError.This branch bypasses the normal JS-error path and can surface a Pascal exception instead of a catchable
TGocciaThrowValue.EvaluateForAwaitOfalready usesThrowTypeError('Value is not iterable')for the same condition.♻️ Suggested change
- if Iterator = nil then - raise TGocciaTypeError.Create('Value is not iterable', AForOfStatement.Line, AForOfStatement.Column, '', nil); + if Iterator = nil then + ThrowTypeError('Value is not iterable');As per coding guidelines: all error construction must use centralized helpers in
Goccia.Values.ErrorHelper.pas.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Evaluator.pas` around lines 1403 - 1404, Replace the direct TGocciaTypeError.Create construction in the branch where Iterator = nil with the centralized helper ThrowTypeError('Value is not iterable') so the runtime raises a catchable TGocciaThrowValue instead of a raw Pascal exception; locate the check around Iterator and AForOfStatement (the current raise TGocciaTypeError.Create call) and swap it to call ThrowTypeError (as used by EvaluateForAwaitOf) from Goccia.Values.ErrorHelper.pas to conform to the error handling guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Evaluator.pas`:
- Around line 1428-1430: The loop evaluators currently only handle cfkBreak and
cfkReturn, discarding the normal completion value (CF.Value) so loops end up as
undefined; to fix, after calling EvaluateStatement(AForOfStatement.Body,
IterContext) assign the body's completion to the overall Result (i.e., add
Result := CF) before checking/handling cfkBreak and cfkReturn so normal
completions are preserved; apply this same change in the other for-await-of body
loops as well (locations using EvaluateStatement, AForOfStatement.Body,
IterContext and testing CF.Kind against cfkBreak/cfkReturn).
In `@units/Goccia.Values.FunctionValue.pas`:
- Around line 204-226: The fast-paths for expression bodies and single-return
bodies evaluate and exit before Context.OnError is copied onto the call scope,
causing closures to capture ACallScope.OnError = nil; before doing any early
Exit in the method, ensure you propagate the error handler by setting
ACallScope.OnError := Context.OnError when Assigned(Context.OnError) and not
Assigned(ACallScope.OnError) (same logic currently at the end) — do this
immediately after setting Context.Scope := ACallScope and before calling
EvaluateExpression or returning TGocciaUndefinedLiteralValue so that closures
created during the FIsExpressionBody and TGocciaReturnStatement fast-paths
receive the correct OnError.
---
Outside diff comments:
In `@units/Goccia.Evaluator.pas`:
- Around line 1403-1404: Replace the direct TGocciaTypeError.Create construction
in the branch where Iterator = nil with the centralized helper
ThrowTypeError('Value is not iterable') so the runtime raises a catchable
TGocciaThrowValue instead of a raw Pascal exception; locate the check around
Iterator and AForOfStatement (the current raise TGocciaTypeError.Create call)
and swap it to call ThrowTypeError (as used by EvaluateForAwaitOf) from
Goccia.Values.ErrorHelper.pas to conform to the error handling guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c47eea1-63ce-4458-8198-18d6055d227a
📒 Files selected for processing (3)
units/Goccia.ControlFlow.pasunits/Goccia.Evaluator.pasunits/Goccia.Values.FunctionValue.pas
Move Context.Scope and OnError propagation before the expression-body and single-return fast paths so closures created during those paths inherit the error handler correctly. Made-with: Cursor
Owner
Author
Acknowledged. |
Owner
Author
Acknowledged. The tagged-pointer optimization (7b37eab) reduced TGocciaControlFlow from 16 bytes to 8 bytes (single register), recovering the regression from the initial refactoring. CI now shows avg +0.3% with 17 improvements and 7 regressions — effectively at parity with the original exception-based approach. |
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)
units/Goccia.Evaluator.pas (1)
1644-1651:⚠️ Potential issue | 🔴 CriticalTemp-root the caught value before constructing the catch scope.
Line 1646 allocates
TGocciaCatchScopebeforeAErrorValueis stored in any GocciaScript scope. At that point the thrown JS value exists only in Pascal code, so a GC during scope creation can drop the catch parameter before the handler runs.Based on learnings: Values held only by Pascal code (not in any GocciaScript scope) must be protected with `AddTempRoot`/`RemoveTempRoot` for the duration they are needed.🔒 Proposed fix
function ExecuteCatchBlock(const ATryStatement: TGocciaTryStatement; const AErrorValue: TGocciaValue; const AContext: TGocciaEvaluationContext): TGocciaControlFlow; var CatchScope: TGocciaScope; CatchContext: TGocciaEvaluationContext; + GarbageCollector: TGocciaGarbageCollector; begin if ATryStatement.CatchParam <> '' then begin - CatchScope := TGocciaCatchScope.Create(AContext.Scope, ATryStatement.CatchParam); - try - CatchScope.DefineLexicalBinding(ATryStatement.CatchParam, AErrorValue, dtParameter); - CatchContext := AContext; - CatchContext.Scope := CatchScope; - Result := EvaluateStatements(ATryStatement.CatchBlock.Nodes, CatchContext); - finally - CatchScope.Free; - end; + GarbageCollector := TGocciaGarbageCollector.Instance; + if Assigned(GarbageCollector) and Assigned(AErrorValue) then + GarbageCollector.AddTempRoot(AErrorValue); + try + CatchScope := TGocciaCatchScope.Create(AContext.Scope, ATryStatement.CatchParam); + try + CatchScope.DefineLexicalBinding(ATryStatement.CatchParam, AErrorValue, dtParameter); + CatchContext := AContext; + CatchContext.Scope := CatchScope; + Result := EvaluateStatements(ATryStatement.CatchBlock.Nodes, CatchContext); + finally + CatchScope.Free; + end; + finally + if Assigned(GarbageCollector) and Assigned(AErrorValue) then + GarbageCollector.RemoveTempRoot(AErrorValue); + end; end else Result := EvaluateStatements(ATryStatement.CatchBlock.Nodes, AContext); end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Evaluator.pas` around lines 1644 - 1651, The thrown value AErrorValue must be temp-rooted before creating TGocciaCatchScope to prevent GC from collecting it; call AddTempRoot(AErrorValue) immediately before constructing CatchScope (or before any allocation that can trigger GC) and use a try/finally to call RemoveTempRoot in the finally block after you have moved AErrorValue into the new scope via CatchScope.DefineLexicalBinding (so the binding owns the value). Update the block that creates CatchScope / calls DefineLexicalBinding / EvaluateStatements to surround scope creation and binding with AddTempRoot/RemoveTempRoot and ensure RemoveTempRoot runs even on exceptions.
🤖 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 `@units/Goccia.Evaluator.pas`:
- Around line 1644-1651: The thrown value AErrorValue must be temp-rooted before
creating TGocciaCatchScope to prevent GC from collecting it; call
AddTempRoot(AErrorValue) immediately before constructing CatchScope (or before
any allocation that can trigger GC) and use a try/finally to call RemoveTempRoot
in the finally block after you have moved AErrorValue into the new scope via
CatchScope.DefineLexicalBinding (so the binding owns the value). Update the
block that creates CatchScope / calls DefineLexicalBinding / EvaluateStatements
to surround scope creation and binding with AddTempRoot/RemoveTempRoot and
ensure RemoveTempRoot runs even on exceptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46c8b881-fba3-41fa-8d2c-ca2154fd3f2b
📒 Files selected for processing (2)
units/Goccia.Evaluator.pasunits/Goccia.Values.FunctionValue.pas
This was referenced Apr 12, 2026
This was referenced Apr 23, 2026
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 by CodeRabbit