Add stack property for errors#23
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a process-global CallStack singleton and integrates call-frame push/pop into the evaluator so Error objects receive a formatted Changes
Sequence Diagram(s)sequenceDiagram
participant Evaluator
participant CallStack as CallStack (singleton)
participant ErrorHelper as ErrorHelper
participant ErrorObj as Error (object)
Evaluator->>CallStack: Push(calleeName, file, line)
Evaluator->>ErrorHelper: CreateErrorObject(name, message, skipTop)
ErrorHelper->>CallStack: CaptureStackTrace(name, message, skipTop)
CallStack-->>ErrorHelper: formattedStack
ErrorHelper->>ErrorObj: attach stack property
ErrorObj-->>Evaluator: return Error object
Evaluator->>CallStack: Pop()
sequenceDiagram
participant User
participant Evaluator
participant CallStack as CallStack (singleton)
participant Function as Callee
User->>Evaluator: invoke function
Evaluator->>CallStack: Push(calleeName, file, line)
Evaluator->>Function: call
alt throws
Function-->>Evaluator: raises error
Evaluator->>ErrorHelper: CreateErrorObject(...)
ErrorHelper->>CallStack: CaptureStackTrace(...)
else returns
Function-->>Evaluator: result
end
Evaluator->>CallStack: Pop()
Evaluator-->>User: result or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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)
Comment |
Contributor
Benchmark Results172 benchmarks · 🔴 1 regressed · 171 unchanged · avg -2.8% arrays.js — 12 unchanged · avg -2.0%
classes.js — 15 unchanged · avg -2.5%
closures.js — 11 unchanged · avg -3.9%
collections.js — 12 unchanged · avg -1.8%
destructuring.js — 14 unchanged · avg -2.5%
fibonacci.js — 6 unchanged · avg -2.7%
iterators.js — 20 unchanged · avg -1.4%
json.js — 20 unchanged · avg -2.0%
jsx.jsx — 21 unchanged · avg -3.7%
numbers.js — 🔴 1 regressed, 10 unchanged · avg -3.9%
objects.js — 7 unchanged · avg -3.1%
promises.js — 12 unchanged · avg -3.7%
strings.js — 11 unchanged · avg -4.2%
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: 3
🧹 Nitpick comments (2)
units/Goccia.Builtins.Globals.pas (1)
145-154: Avoid double stack capture inBuildErrorObject.
CreateErrorObjectnow attachesstack, and this block captures again withSkipTop=1, so error constructors compute the stack twice. Consider adding an optionalASkipTop(orCaptureStackflag) toCreateErrorObjectand call it with1here (and0elsewhere). This also keepsAggregateErrorconsistent.♻️ Proposed refactor
- Result := CreateErrorObject(AName, Message); - Result.Prototype := AProto; - - { Capture stack trace at construction site, skipping the Error constructor frame } - if Assigned(TGocciaCallStack.Instance) then - Result.AssignProperty('stack', - TGocciaStringLiteralValue.Create( - TGocciaCallStack.Instance.CaptureStackTrace(AName, Message, 1))); + Result := CreateErrorObject(AName, Message, 1); + Result.Prototype := AProto;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.Globals.pas` around lines 145 - 154, BuildErrorObject is double-capturing the stack because CreateErrorObject already sets the 'stack' property; add an optional parameter to CreateErrorObject (e.g., ASkipTop: Integer = 0 or ACaptureStack: Boolean = True) so callers can control stack capture, update this call-site in BuildErrorObject to call CreateErrorObject with ASkipTop = 1 (or ACaptureStack = True and ASkipTop = 1) and remove the subsequent manual Result.AssignProperty('stack', ...) block, and update other callers (including AggregateError creation paths) to pass ASkipTop = 0 (or leave default) so only one stack capture occurs per error construction.units/Goccia.CallStack.pas (1)
51-54: Consider defensive nil check or lazy initialization.
InstancereturnsFInstancedirectly, which could benilifInitializehasn't been called. Callers that assume a non-nil result risk null pointer dereferences.If intentional (relying on engine startup order), consider adding an assertion or at minimum documenting this precondition. Alternatively, lazy initialization could make this more robust:
♻️ Optional: Lazy initialization approach
class function TGocciaCallStack.Instance: TGocciaCallStack; begin + if not Assigned(FInstance) then + FInstance := TGocciaCallStack.Create; Result := FInstance; end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.CallStack.pas` around lines 51 - 54, The TGocciaCallStack.Instance currently returns FInstance directly which may be nil; update Instance to guard against that by either (a) performing lazy initialization (if FInstance is nil call TGocciaCallStack.Initialize or create a new TGocciaCallStack instance and assign FInstance before returning) or (b) adding a defensive check that raises/asserts with a clear message when FInstance is nil (use Assert or raise an exception) so callers won't silently dereference nil; reference TGocciaCallStack.Instance, FInstance and Initialize when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/built-ins.md`:
- Line 322: The inline code span for the `stack` property contains leading
spaces (the string showing lines like " at functionName
(filePath:line:col)"), which violates MD038; remove the leading indentation
inside the inline code span or replace that example with a fenced code block
showing the multi-line stack trace. Update the text around the `stack` property
in the sentence that describes the formatted string so it either uses a properly
trimmed inline code span (no leading spaces) or a fenced code block
demonstrating the first line plus the "at functionName (filePath:line:col)"
frames.
In `@units/Goccia.CallStack.pas`:
- Around line 108-110: Clamp negative ASkipTop before computing EffectiveCount
to avoid out-of-bounds reads: ensure ASkipTop is treated as at least 0 (e.g.,
set LocalSkipTop := Max(0, ASkipTop) or explicitly if ASkipTop < 0 then
LocalSkipTop := 0), then compute EffectiveCount := FCount - LocalSkipTop and
clamp EffectiveCount to >= 0; update the block referencing
EffectiveCount/FCount/ASkipTop so any negative ASkipTop cannot cause
EffectiveCount to exceed FCount or produce negative indices.
In `@units/Goccia.Evaluator.pas`:
- Around line 934-953: The evaluator currently mutates the global singleton
TGocciaCallStack via TGocciaCallStack.Instance.Push/Pop inside EvaluateCall (and
EvaluateNewExpression), causing side effects; instead add a CallStack (or
ICallStack) reference to TGocciaEvaluationContext and remove any
TGocciaCallStack.Instance usage from evaluator units—have EvaluateCall and
EvaluateNewExpression accept/use AContext.CallStack to Push/Pop (or better,
expose PushCall/PopCall on TGocciaEvaluationContext) and move the responsibility
for initializing/owning the call stack to the interpreter (which instantiates
and supplies the CallStack on the context), ensuring evaluators remain pure and
no longer touch the singleton.
---
Nitpick comments:
In `@units/Goccia.Builtins.Globals.pas`:
- Around line 145-154: BuildErrorObject is double-capturing the stack because
CreateErrorObject already sets the 'stack' property; add an optional parameter
to CreateErrorObject (e.g., ASkipTop: Integer = 0 or ACaptureStack: Boolean =
True) so callers can control stack capture, update this call-site in
BuildErrorObject to call CreateErrorObject with ASkipTop = 1 (or ACaptureStack =
True and ASkipTop = 1) and remove the subsequent manual
Result.AssignProperty('stack', ...) block, and update other callers (including
AggregateError creation paths) to pass ASkipTop = 0 (or leave default) so only
one stack capture occurs per error construction.
In `@units/Goccia.CallStack.pas`:
- Around line 51-54: The TGocciaCallStack.Instance currently returns FInstance
directly which may be nil; update Instance to guard against that by either (a)
performing lazy initialization (if FInstance is nil call
TGocciaCallStack.Initialize or create a new TGocciaCallStack instance and assign
FInstance before returning) or (b) adding a defensive check that raises/asserts
with a clear message when FInstance is nil (use Assert or raise an exception) so
callers won't silently dereference nil; reference TGocciaCallStack.Instance,
FInstance and Initialize when making the change.
Comment on lines
+934
to
+953
| if Assigned(TGocciaCallStack.Instance) then | ||
| TGocciaCallStack.Instance.Push(CalleeName, AContext.CurrentFilePath, | ||
| ACallExpression.Line, ACallExpression.Column); | ||
| try | ||
| if Callee is TGocciaNativeFunctionValue then | ||
| Result := TGocciaNativeFunctionValue(Callee).Call(Arguments, ThisValue) | ||
| else if Callee is TGocciaFunctionValue then | ||
| Result := TGocciaFunctionValue(Callee).Call(Arguments, ThisValue) | ||
| else if Callee is TGocciaBoundFunctionValue then | ||
| Result := TGocciaBoundFunctionValue(Callee).Call(Arguments, ThisValue) | ||
| else if Callee is TGocciaClassValue then | ||
| Result := TGocciaClassValue(Callee).Call(Arguments, ThisValue) | ||
| else | ||
| begin | ||
| SafeOnError(AContext, Format('%s is not a function', [Callee.TypeName]), ACallExpression.Line, ACallExpression.Column); | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| end; | ||
| finally | ||
| if Assigned(TGocciaCallStack.Instance) then | ||
| TGocciaCallStack.Instance.Pop; |
Contributor
There was a problem hiding this comment.
Evaluator purity breach: CallStack mutations inside evaluator functions.
Pushing/popping the singleton introduces global side effects in EvaluateCall/EvaluateNewExpression, which conflicts with the evaluator purity rule. Please move stack tracking to the interpreter or a value/context-owned object (e.g., pass a CallStack instance via TGocciaEvaluationContext) so evaluator logic remains side-effect free.
Based on learnings: Evaluator functions in Goccia.Evaluator.pas and its sub-modules must be pure functions with no side effects. State changes happen through the scope and value objects passed via TGocciaEvaluationContext, never through evaluator-internal state.
Also applies to: 1592-1684
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Evaluator.pas` around lines 934 - 953, The evaluator currently
mutates the global singleton TGocciaCallStack via
TGocciaCallStack.Instance.Push/Pop inside EvaluateCall (and
EvaluateNewExpression), causing side effects; instead add a CallStack (or
ICallStack) reference to TGocciaEvaluationContext and remove any
TGocciaCallStack.Instance usage from evaluator units—have EvaluateCall and
EvaluateNewExpression accept/use AContext.CallStack to Push/Pop (or better,
expose PushCall/PopCall on TGocciaEvaluationContext) and move the responsibility
for initializing/owning the call stack to the interpreter (which instantiates
and supplies the CallStack on the context), ensuring evaluators remain pure and
no longer touch the singleton.
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.Builtins.Globals.pas (1)
241-241:⚠️ Potential issue | 🟡 MinorInconsistent
ASkipTopforAggregateError.
BuildErrorObjectpassesASkipTop=1to exclude the constructor frame, butAggregateErrorConstructorcallsCreateErrorObjectdirectly without this parameter. This meansAggregateErrorstack traces will include an extra constructor frame compared to other error types.🔧 Proposed fix for consistency
{ Step 2: Let O = OrdinaryCreateFromConstructor } - ErrorObj := CreateErrorObject('AggregateError', Message); + ErrorObj := CreateErrorObject('AggregateError', Message, 1); ErrorObj.Prototype := FAggregateErrorProto;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.Globals.pas` at line 241, AggregateErrorConstructor currently calls CreateErrorObject without the ASkipTop parameter, causing inconsistent stack traces vs. BuildErrorObject which uses ASkipTop=1; update AggregateErrorConstructor to call CreateErrorObject with ASkipTop=1 (or otherwise propagate the same skip behavior) so the constructor frame is excluded and AggregateError stack traces match other error types.
🧹 Nitpick comments (1)
docs/built-ins.md (1)
324-328: Add language identifier to fenced code block.The fenced code block lacks a language identifier, triggering MD040. Since this is a plain text example, adding
textorplaintextwould satisfy the linter.📝 Proposed fix
-``` +```text ErrorName: message at functionName (filePath:line:col) at outerFunction (filePath:line:col)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/built-ins.mdaround lines 324 - 328, The fenced code block in
docs/built-ins.md showing the stack trace lacks a language identifier which
triggers MD040; update that fenced block (the triple-backtick block that
contains "ErrorName: message" and the subsequent "at functionName..." lines) to
include a plain text language identifier such as text or plaintext (e.g.,
replacewithtext) so the linter recognizes it as plain text.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In@units/Goccia.Builtins.Globals.pas:
- Line 241: AggregateErrorConstructor currently calls CreateErrorObject without
the ASkipTop parameter, causing inconsistent stack traces vs. BuildErrorObject
which uses ASkipTop=1; update AggregateErrorConstructor to call
CreateErrorObject with ASkipTop=1 (or otherwise propagate the same skip
behavior) so the constructor frame is excluded and AggregateError stack traces
match other error types.
Nitpick comments:
In@docs/built-ins.md:
- Around line 324-328: The fenced code block in docs/built-ins.md showing the
stack trace lacks a language identifier which triggers MD040; update that fenced
block (the triple-backtick block that contains "ErrorName: message" and the
subsequent "at functionName..." lines) to include a plain text language
identifier such as text or plaintext (e.g., replacewithtext) so the
linter recognizes it as plain text.</details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
This was referenced Feb 20, 2026
Merged
This was referenced Apr 12, 2026
3 tasks
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
New Features
Documentation
Tests