Create BindingMap for Scope and pre-typed array callbacks#29
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds callback-argument collection types and makes argument-collection methods overridable; introduces a new lexically-scoped binding map and migrates scope storage to it; routes array/map/set callbacks through a typed function fast-path with fallbacks; and adds function execution fast-paths for simple bodies. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![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)
units/Goccia.Values.ArrayValue.pas (1)
1700-1711:⚠️ Potential issue | 🔴 CriticalAdd
is TGocciaFunctionBaseguard before casting inArraySortandArrayToSorted.At lines 1708 and 1486, the callback is cast to
TGocciaFunctionBaseafter only checkingIsCallable. However,TGocciaClassValueis callable (overridesIsCallable) but inherits fromTGocciaValue, notTGocciaFunctionBase. If a class value is passed as the comparator, the cast fails.Other array methods (e.g., lines 465, 505, 559) properly guard with
if Callback is TGocciaFunctionBasebefore accessing typed callback state. Apply the same guard here:Suggested pattern
if CustomSortFunction.IsCallable then begin if CustomSortFunction is TGocciaFunctionBase then begin // safe to cast QuickSortElements(Arr.Elements, TGocciaFunctionBase(CustomSortFunction), ...); end else ThrowError('Custom sort function must be a function'); end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.ArrayValue.pas` around lines 1700 - 1711, The ArraySort/ArrayToSorted code currently only checks CustomSortFunction.IsCallable before casting to TGocciaFunctionBase and calling QuickSortElements; add a type guard to verify CustomSortFunction is TGocciaFunctionBase (e.g., "if CustomSortFunction is TGocciaFunctionBase") before performing the cast and calling QuickSortElements, and otherwise call ThrowError('Custom sort function must be a function'); apply the same pattern in both ArraySort and ArrayToSorted so the cast to TGocciaFunctionBase is only done when the callback truly inherits TGocciaFunctionBase.
🧹 Nitpick comments (2)
units/Goccia.Scope.BindingMap.pas (1)
170-173: Consider adding bounds validation or documenting the precondition.
GetValueAtlacks bounds checking, relying on callers to ensure0 <= AIndex < FCount. While this is acceptable for theinlinehot path inMarkReferences, the publicValues[AIndex]property could be misused.💡 Option: Add debug-only assertion
function TGocciaBindingMap.GetValueAt(const AIndex: Integer): TGocciaValue; begin + Assert((AIndex >= 0) and (AIndex < FCount), 'BindingMap index out of bounds'); Result := FValues[AIndex]; end;This provides safety during development without release build overhead (assertions are typically stripped in release mode).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Scope.BindingMap.pas` around lines 170 - 173, TGocciaBindingMap.GetValueAt currently returns FValues[AIndex] with no bounds validation; add a debug-only bounds assertion (e.g. Assert((AIndex >= 0) and (AIndex < FCount))) at the start of TGocciaBindingMap.GetValueAt to catch misuse during development without release overhead, or alternatively raise an ERangeError for public usage; also update the Values property or method comment to document the precondition (0 <= AIndex < FCount) and ensure callers like MarkReferences remain inline/hot-path safe.units/Goccia.Arguments.Callbacks.pas (1)
48-51: Constructors don't callinherited Create, leavingFArgsuninitialized.Both constructors (
TGocciaArrayCallbackArgs.CreateandTGocciaReduceCallbackArgs.Create) skip callinginherited Create, soFArgs(fromTGocciaArgumentsCollection) remainsnil. This is intentional for the allocation-free design, but could cause issues if any inherited methods are called that accessFArgs(e.g.,Add,SetElement,Slice).Since these classes only override
GetElementandGetLength, and are used exclusively for callback invocation, this is acceptable—but consider adding a brief comment documenting this design choice.📝 Add documentation comment
constructor TGocciaArrayCallbackArgs.Create(const AThisArray: TGocciaValue); begin + // Intentionally skip inherited Create to avoid FArgs allocation. + // This class overrides GetElement/GetLength with fixed-structure access. FThisArray := AThisArray; end;Also applies to: 71-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Arguments.Callbacks.pas` around lines 48 - 51, The constructors TGocciaArrayCallbackArgs.Create and TGocciaReduceCallbackArgs.Create intentionally do not call inherited Create (so FArgs from TGocciaArgumentsCollection stays nil); add a brief inline documentation comment above each constructor explaining this deliberate allocation-free design and that it's safe because these classes only override GetElement and GetLength and are used solely for callback invocation, referencing FArgs and TGocciaArgumentsCollection to make the rationale clear to future maintainers.
🤖 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.Values.ArrayValue.pas`:
- Around line 1700-1711: The ArraySort/ArrayToSorted code currently only checks
CustomSortFunction.IsCallable before casting to TGocciaFunctionBase and calling
QuickSortElements; add a type guard to verify CustomSortFunction is
TGocciaFunctionBase (e.g., "if CustomSortFunction is TGocciaFunctionBase")
before performing the cast and calling QuickSortElements, and otherwise call
ThrowError('Custom sort function must be a function'); apply the same pattern in
both ArraySort and ArrayToSorted so the cast to TGocciaFunctionBase is only done
when the callback truly inherits TGocciaFunctionBase.
---
Nitpick comments:
In `@units/Goccia.Arguments.Callbacks.pas`:
- Around line 48-51: The constructors TGocciaArrayCallbackArgs.Create and
TGocciaReduceCallbackArgs.Create intentionally do not call inherited Create (so
FArgs from TGocciaArgumentsCollection stays nil); add a brief inline
documentation comment above each constructor explaining this deliberate
allocation-free design and that it's safe because these classes only override
GetElement and GetLength and are used solely for callback invocation,
referencing FArgs and TGocciaArgumentsCollection to make the rationale clear to
future maintainers.
In `@units/Goccia.Scope.BindingMap.pas`:
- Around line 170-173: TGocciaBindingMap.GetValueAt currently returns
FValues[AIndex] with no bounds validation; add a debug-only bounds assertion
(e.g. Assert((AIndex >= 0) and (AIndex < FCount))) at the start of
TGocciaBindingMap.GetValueAt to catch misuse during development without release
overhead, or alternatively raise an ERangeError for public usage; also update
the Values property or method comment to document the precondition (0 <= AIndex
< FCount) and ensure callers like MarkReferences remain inline/hot-path safe.
Contributor
Benchmark Results179 benchmarks · 🟢 130 improved · 🔴 4 regressed · 45 unchanged · avg +21.7% arrays.js — 🟢 15 improved, 🔴 3 regressed, 1 unchanged · avg +68.2%
classes.js — 🟢 7 improved, 8 unchanged · avg +8.0%
closures.js — 🟢 11 improved · avg +21.7%
collections.js — 🟢 9 improved, 🔴 1 regressed, 2 unchanged · avg +21.3%
destructuring.js — 🟢 11 improved, 3 unchanged · avg +14.3%
fibonacci.js — 🟢 5 improved, 1 unchanged · avg +18.6%
iterators.js — 🟢 13 improved, 7 unchanged · avg +16.3%
json.js — 🟢 14 improved, 6 unchanged · avg +11.3%
jsx.jsx — 🟢 21 improved · avg +27.7%
numbers.js — 🟢 8 improved, 3 unchanged · avg +21.6%
objects.js — 🟢 3 improved, 4 unchanged · avg +7.0%
promises.js — 🟢 3 improved, 9 unchanged · avg +3.6%
strings.js — 🟢 10 improved, 1 unchanged · avg +18.4%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.Values.ArrayValue.pas`:
- Line 1706: The cast to TGocciaFunctionBase when calling QuickSortElements is
unsafe—before passing CustomSortFunction into QuickSortElements, check whether
CustomSortFunction actually supports TGocciaFunctionBase (e.g., using an "is" or
TryCast-style check) and only cast if valid; if the check fails, handle it
safely (either call a fallback comparison, raise a controlled error, or wrap the
callable in an adapter) so QuickSortElements receives a valid
TGocciaFunctionBase instance; update the call site where
QuickSortElements(Arr.Elements, TGocciaFunctionBase(CustomSortFunction),
CallArgs, AThisValue, 0, ...) is made to perform this type check and safe
handling (mirror the fix used in ArrayToSorted).
- Line 1484: The call to QuickSortElements casts CustomSortFunction to
TGocciaFunctionBase without checking its actual runtime type, which will crash
when CustomSortFunction is a TGocciaClassValue or another callable; before
casting, check the concrete type (e.g. use "is" to test for TGocciaFunctionBase)
and only pass a TGocciaFunctionBase instance to QuickSortElements, otherwise
handle the alternative callable (for example call a different comparator wrapper
or raise a clear error) so ResultArray.Elements sorting never performs an unsafe
cast.
| CallArgs := TGocciaArgumentsCollection.Create([nil, nil]); | ||
| try | ||
| QuickSortElements(ResultArray.Elements, CustomSortFunction, CallArgs, AThisValue, 0, ResultArray.Elements.Count - 1); | ||
| QuickSortElements(ResultArray.Elements, TGocciaFunctionBase(CustomSortFunction), CallArgs, AThisValue, 0, ResultArray.Elements.Count - 1); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get context around line 1484 in the ArrayValue.pas file
echo "=== Context around line 1484 (the suspect code) ==="
sed -n '1475,1495p' units/Goccia.Values.ArrayValue.pas
echo ""
echo "=== Line 1479 to check the IsCallable check ==="
sed -n '1477,1486p' units/Goccia.Values.ArrayValue.pas
echo ""
echo "=== Check QuickSortElements signature ==="
sed -n '209,215p' units/Goccia.Values.ArrayValue.pasRepository: frostney/GocciaScript
Length of output: 1570
🏁 Script executed:
#!/bin/bash
# Find all classes and check IsCallable
echo "=== Classes that define IsCallable property ==="
rg -n "procedure.*IsCallable|function.*IsCallable" -A 2 units/Goccia.Values*.pas 2>/dev/null | head -50
echo ""
echo "=== Check if TGocciaFunctionBase and related classes are defined ==="
rg -n "class.*TGoccia.*Function" units/Goccia.Values.FunctionBase.pas 2>/dev/null | head -20
echo ""
echo "=== Check VMT base class and its IsCallable ==="
fd -e pas . units/ | xargs grep -l "virtual.*IsCallable" 2>/dev/null | head -5Repository: frostney/GocciaScript
Length of output: 1638
🏁 Script executed:
#!/bin/bash
# Search for where IsCallable is set to true to understand what types are callable
echo "=== Types that set IsCallable to True ==="
rg -n "IsCallable.*:=.*true|IsCallable.*:=.*True" -i units/Goccia.Values*.pas -B 5 | head -100
echo ""
echo "=== Check the value base class hierarchy ==="
rg -n "TGocciaValue\s*=\s*class" units/Goccia.Values.pas -A 20 | head -50Repository: frostney/GocciaScript
Length of output: 233
🏁 Script executed:
#!/bin/bash
# Verify exact hierarchy - search for class definitions related to functions
echo "=== Class definitions inheriting from TGocciaValue or TGocciaFunctionBase ==="
ast-grep --pattern 'class $NAME($BASE)' units/Goccia.Values.FunctionBase.pas | head -20
echo ""
echo "=== Check TGocciaValue.IsCallable implementation ==="
rg -n "IsCallable" units/Goccia.Values.pas -B 2 -A 5 | head -40Repository: frostney/GocciaScript
Length of output: 277
🏁 Script executed:
#!/bin/bash
# Check TGocciaClassValue class hierarchy
echo "=== TGocciaClassValue definition and inheritance ==="
rg -n "class TGocciaClassValue" units/ -A 5
echo ""
echo "=== TGocciaFunctionBase definition and inheritance ==="
rg -n "class TGocciaFunctionBase" units/ -A 5
echo ""
echo "=== All classes that override IsCallable ==="
rg -n "function IsCallable.*override" units/ -B 3Repository: frostney/GocciaScript
Length of output: 906
🏁 Script executed:
#!/bin/bash
# Check if TGocciaClassValue is anywhere inherited from TGocciaFunctionBase
echo "=== Check full inheritance chain ==="
rg -n "TGocciaClassValue" units/Goccia.Values.ClassValue.pas | head -10
echo ""
echo "=== Search for all classes that may have IsCallable = True ==="
rg -n "procedure.*ObjectValue\|class.*Native.*Function" units/ | head -20Repository: frostney/GocciaScript
Length of output: 734
Unsafe cast to TGocciaFunctionBase without type check.
At line 1484, the code assumes that IsCallable guarantees a TGocciaFunctionBase type, but this assumption is incorrect. Both TGocciaFunctionBase and TGocciaClassValue independently return true for IsCallable, yet TGocciaClassValue does not derive from TGocciaFunctionBase. The direct cast TGocciaFunctionBase(CustomSortFunction) will fail at runtime if the callable is a TGocciaClassValue or another callable type unrelated to TGocciaFunctionBase.
Proposed fix: Add type check before cast
if not CustomSortFunction.IsCallable then
ThrowError('Custom sort function must be a function');
+ if not (CustomSortFunction is TGocciaFunctionBase) then
+ ThrowError('Sort comparator must be a function');
+
CallArgs := TGocciaArgumentsCollection.Create([nil, nil]);
try
QuickSortElements(ResultArray.Elements, TGocciaFunctionBase(CustomSortFunction), CallArgs, AThisValue, 0, ResultArray.Elements.Count - 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Values.ArrayValue.pas` at line 1484, The call to
QuickSortElements casts CustomSortFunction to TGocciaFunctionBase without
checking its actual runtime type, which will crash when CustomSortFunction is a
TGocciaClassValue or another callable; before casting, check the concrete type
(e.g. use "is" to test for TGocciaFunctionBase) and only pass a
TGocciaFunctionBase instance to QuickSortElements, otherwise handle the
alternative callable (for example call a different comparator wrapper or raise a
clear error) so ResultArray.Elements sorting never performs an unsafe cast.
| CallArgs := TGocciaArgumentsCollection.Create([nil, nil]); | ||
| try | ||
| QuickSortElements(Arr.Elements, CustomSortFunction, CallArgs, AThisValue, 0, Arr.Elements.Count - 1); | ||
| QuickSortElements(Arr.Elements, TGocciaFunctionBase(CustomSortFunction), CallArgs, AThisValue, 0, Arr.Elements.Count - 1); |
Contributor
There was a problem hiding this comment.
Same unsafe cast issue as in ArrayToSorted.
This line has the same problem as line 1484 - the cast to TGocciaFunctionBase may fail if CustomSortFunction is a callable that doesn't inherit from TGocciaFunctionBase.
Proposed fix: Add type check before cast
if not CustomSortFunction.IsCallable then
ThrowError('Custom sort function must be a function');
+ if not (CustomSortFunction is TGocciaFunctionBase) then
+ ThrowError('Sort comparator must be a function');
+
CallArgs := TGocciaArgumentsCollection.Create([nil, nil]);
try
QuickSortElements(Arr.Elements, TGocciaFunctionBase(CustomSortFunction), CallArgs, AThisValue, 0, Arr.Elements.Count - 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Values.ArrayValue.pas` at line 1706, The cast to
TGocciaFunctionBase when calling QuickSortElements is unsafe—before passing
CustomSortFunction into QuickSortElements, check whether CustomSortFunction
actually supports TGocciaFunctionBase (e.g., using an "is" or TryCast-style
check) and only cast if valid; if the check fails, handle it safely (either call
a fallback comparison, raise a controlled error, or wrap the callable in an
adapter) so QuickSortElements receives a valid TGocciaFunctionBase instance;
update the call site where QuickSortElements(Arr.Elements,
TGocciaFunctionBase(CustomSortFunction), CallArgs, AThisValue, 0, ...) is made
to perform this type check and safe handling (mirror the fix used in
ArrayToSorted).
This was referenced Feb 22, 2026
This was referenced Apr 7, 2026
This was referenced Apr 20, 2026
4 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