Refine code style guide and auto-fix files#7
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughLarge, cross-cutting refactor: adds a Pascal formatter and lefthook pre-commit hook; renames and const-qualifies hundreds of parameters to an A-prefixed convention; reorders/grouped uses clauses; minor API additions (TGocciaToken.Column, TGocciaError properties, Freeze/IsFrozen) and CLI runner improvements. No major runtime behavior changes. Changes
Sequence Diagram(s)(omitted) 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 |
Contributor
Benchmark Results113 benchmarks · 113 unchanged · avg -1.5% arrays.js — 11 unchanged · avg -2.8%
classes.js — 10 unchanged · avg -0.2%
closures.js — 11 unchanged · avg -1.5%
collections.js — 12 unchanged · avg -0.9%
destructuring.js — 14 unchanged · avg -2.1%
fibonacci.js — 3 unchanged · avg -0.8%
json.js — 11 unchanged · avg -1.9%
numbers.js — 11 unchanged · avg -0.8%
objects.js — 7 unchanged · avg -1.5%
promises.js — 12 unchanged · avg -1.6%
strings.js — 11 unchanged · avg -1.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 (9)
units/Goccia.Values.TemporalDuration.pas (1)
528-536:⚠️ Potential issue | 🟠 MajorCalendar units (years/months) are silently ignored instead of throwing RangeError.
Per the TC39 Temporal specification,
Duration.prototype.total()requires arelativeTooption when the duration contains nonzero years or months; without it, the method must throw aRangeError. The current implementation omitsFYearsandFMonthsfrom the nanosecond calculation without validation, allowing such durations to silently produce incorrect results.Add a check to throw
RangeErrorwhen calendar units are present:Proposed fix
// Convert everything to nanoseconds first (ignoring calendar units for simplicity) + if (D.FYears <> 0) or (D.FMonths <> 0) then + ThrowRangeError('total() cannot be used on durations with years or months without relativeTo'); + TotalNs := D.FNanoseconds + D.FMicroseconds * 1000.0 +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalDuration.pas` around lines 528 - 536, The nanosecond aggregation silently ignores calendar units FYears and FMonths; before computing TotalNs in the duration total routine (the block that computes TotalNs from D.FNanoseconds...D.FWeeks), validate D.FYears and D.FMonths and throw a RangeError if either is nonzero so durations with calendar units require a relativeTo per the spec; place the check immediately before the TotalNs assignment and use the same exception type used elsewhere (RangeError) with a clear message.units/Goccia.SharedPrototype.pas (1)
27-37:⚠️ Potential issue | 🟡 MinorGuard PinValue for optional method host.
If
AMethodHostcan ever benil,PinValue(FMethodHost)will raise an AV becausePinValuedoes not nil-check before using the value as a dictionary key. Add a nil guard or explicitly document thatAMethodHostis required.🛡️ Suggested guard
if Assigned(TGocciaGC.Instance) then begin TGocciaGC.Instance.PinValue(FPrototype); + if Assigned(FMethodHost) then TGocciaGC.Instance.PinValue(FMethodHost); end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.SharedPrototype.pas` around lines 27 - 37, In TGocciaSharedPrototype.Create, guard the optional AMethodHost/FMethodHost before calling TGocciaGC.Instance.PinValue to avoid AVs: check if FMethodHost is assigned (e.g., if Assigned(FMethodHost) then TGocciaGC.Instance.PinValue(FMethodHost)) instead of always calling PinValue; update the constructor (TGocciaSharedPrototype.Create) to only pin FMethodHost when non-nil (or alternatively add doc comment to require AMethodHost non-nil) and ensure any other uses of FMethodHost in this unit similarly handle a nil value.units/Goccia.Builtins.GlobalString.pas (1)
47-51:⚠️ Potential issue | 🟠 MajorGuard against double-free of GC-managed value.
FStringConstructoris aTGocciaStringClassValue(aTGocciaValuesubclass) that auto-registers with the GC viaAfterConstructionwhen the GC is active. CallingFStringConstructor.Freedirectly in the destructor will cause a double-free when the GC attempts to free this value.The parent class
TGocciaBuiltincorrectly guards against this in its destructor (lines 49-51 ofGoccia.Builtins.Base.pas):if not Assigned(TGocciaGC.Instance) then FBuiltinObject.Free;Apply the same pattern here:
Proposed fix
destructor TGocciaGlobalString.Destroy; begin - FStringConstructor.Free; + if not Assigned(TGocciaGC.Instance) then + FStringConstructor.Free; inherited; end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.GlobalString.pas` around lines 47 - 51, In TGocciaGlobalString.Destroy, avoid directly calling FStringConstructor.Free because TGocciaStringClassValue auto-registers with the GC (AfterConstruction) and may be freed by TGocciaGC; instead wrap the free in the same guard used by TGocciaBuiltin (check Assigned(TGocciaGC.Instance)) so only free FStringConstructor when the GC is not active (i.e., if not Assigned(TGocciaGC.Instance) then FStringConstructor.Free); keep inherited call as-is.units/Goccia.Builtins.GlobalNumber.pas (2)
257-263:⚠️ Potential issue | 🟡 MinorType cast performed before type check.
At Line 257, you cast
AArgs.GetElement(0)toTGocciaNumberLiteralValuebefore checking whether it's actually that type at Line 259. If the argument is not aTGocciaNumberLiteralValue, this could cause undefined behavior or incorrect results.🐛 Proposed fix
function TGocciaGlobalNumber.NumberIsFinite(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; var NumberArg: TGocciaNumberLiteralValue; begin if AArgs.Length = 0 then begin Result := TGocciaBooleanLiteralValue.FalseValue; Exit; end; - NumberArg := TGocciaNumberLiteralValue(AArgs.GetElement(0)); - - if not (NumberArg is TGocciaNumberLiteralValue) then + if not (AArgs.GetElement(0) is TGocciaNumberLiteralValue) then begin Result := TGocciaBooleanLiteralValue.FalseValue; Exit; end; + + NumberArg := TGocciaNumberLiteralValue(AArgs.GetElement(0));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.GlobalNumber.pas` around lines 257 - 263, The code casts AArgs.GetElement(0) to TGocciaNumberLiteralValue in NumberArg before checking its type, risking invalid casts; change the order so you first check "if not (AArgs.GetElement(0) is TGocciaNumberLiteralValue) then ..." and return the FalseValue, and only after the type check assign NumberArg := TGocciaNumberLiteralValue(AArgs.GetElement(0)); this ensures TGocciaNumberLiteralValue, NumberArg and the type guard are used correctly without performing an unsafe cast.
308-314:⚠️ Potential issue | 🟡 MinorSame type cast issue as
NumberIsFinite.The same pattern of casting before type checking exists here in
NumberIsInteger.🐛 Proposed fix
function TGocciaGlobalNumber.NumberIsInteger(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; var NumberArg: TGocciaNumberLiteralValue; Value: Double; begin if AArgs.Length = 0 then begin Result := TGocciaBooleanLiteralValue.FalseValue; Exit; end; - NumberArg := TGocciaNumberLiteralValue(AArgs.GetElement(0)); - - if not (NumberArg is TGocciaNumberLiteralValue) then + if not (AArgs.GetElement(0) is TGocciaNumberLiteralValue) then begin Result := TGocciaBooleanLiteralValue.FalseValue; Exit; end; + + NumberArg := TGocciaNumberLiteralValue(AArgs.GetElement(0));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.GlobalNumber.pas` around lines 308 - 314, In NumberIsInteger fix the improper cast-before-check: stop assigning NumberArg by casting the argument (NumberArg := TGocciaNumberLiteralValue(AArgs.GetElement(0))) and instead retrieve the raw element first (via AArgs.GetElement(0)), check its type with "if not (NumberArg is TGocciaNumberLiteralValue) then" and only after that perform the cast to TGocciaNumberLiteralValue for further use; reference the NumberArg variable, the AArgs.GetElement(0) call, and TGocciaNumberLiteralValue in your change so the runtime type check precedes the cast.units/Goccia.Values.ClassValue.pas (1)
407-427:⚠️ Potential issue | 🟠 MajorRoot the instance during constructor dispatch.
Instanceis only referenced by Pascal code while the constructor runs; a GC cycle insideConstructorToCall.Callcould collect it. Add a temp root around the call (and addGoccia.GarbageCollectortousesif not already present).As per coding guidelines, "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
if Assigned(ConstructorToCall) then - // Call the constructor with the instance as this - ConstructorToCall.Call(AArguments, Instance); + // Call the constructor with the instance as this + TGocciaGC.Instance.AddTempRoot(Instance); + try + ConstructorToCall.Call(AArguments, Instance); + finally + TGocciaGC.Instance.RemoveTempRoot(Instance); + end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.ClassValue.pas` around lines 407 - 427, In TGocciaClassValue.Instantiate, the newly created Instance must be rooted before calling ConstructorToCall.Call to prevent GC from collecting it during the call; wrap the call with AddTempRoot(Instance) and RemoveTempRoot(Instance) (and add Goccia.GarbageCollector to the uses clause if missing), so: create Instance as now, determine ConstructorToCall, then if Assigned(ConstructorToCall) call AddTempRoot(Instance) before invoking ConstructorToCall.Call(AArguments, Instance) and ensure RemoveTempRoot(Instance) runs afterwards (use a try/finally to guarantee removal).units/Goccia.Builtins.Temporal.pas (1)
84-99:⚠️ Potential issue | 🟠 MajorProtect the Temporal namespace during registration.
FTemporalNamespaceis only held in Pascal until it’s bound into the scope; GC could collect it whileRegister*allocates. Add a temp root around registration and binding (and importGoccia.GarbageCollector).As per coding guidelines, "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
FTemporalNamespace := TGocciaObjectValue.Create; + TGocciaGC.Instance.AddTempRoot(FTemporalNamespace); + try RegisterDuration; RegisterInstant; RegisterPlainDate; RegisterPlainTime; RegisterPlainDateTime; RegisterNow; AScope.DefineLexicalBinding(AName, FTemporalNamespace, dtLet); + finally + TGocciaGC.Instance.RemoveTempRoot(FTemporalNamespace); + end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.Temporal.pas` around lines 84 - 99, TGocciaTemporalBuiltin.Create currently constructs FTemporalNamespace and calls RegisterDuration/RegisterInstant/RegisterPlainDate/RegisterPlainTime/RegisterPlainDateTime/RegisterNow before binding it into the scope, which risks GC collection; wrap the registration/binding sequence in a temporary root by calling AddTempRoot(FTemporalNamespace) immediately after creating FTemporalNamespace and RemoveTempRoot(FTemporalNamespace) after AScope.DefineLexicalBinding, and add the Goccia.GarbageCollector unit to the uses clause; ensure the AddTempRoot/RemoveTempRoot pair encloses all Register* calls and the DefineLexicalBinding so the namespace cannot be collected while being populated.units/Goccia.Values.TemporalPlainDate.pas (1)
447-457:⚠️ Potential issue | 🟠 MajorTemp-root the negated duration while calling
DateAdd.
NegatedDuris only referenced in Pascal whileDateAddexecutes; GC could collect it during allocations insideDateAdd. Add a temp root for the duration of the call.As per coding guidelines, "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
NegatedDur := TGocciaTemporalDurationValue.Create( -Dur.Years, -Dur.Months, -Dur.Weeks, -Dur.Days, -Dur.Hours, -Dur.Minutes, -Dur.Seconds, -Dur.Milliseconds, -Dur.Microseconds, -Dur.Nanoseconds); + TGocciaGC.Instance.AddTempRoot(NegatedDur); NewArgs := TGocciaArgumentsCollection.Create([NegatedDur]); try Result := DateAdd(NewArgs, AThisValue); finally NewArgs.Free; + TGocciaGC.Instance.RemoveTempRoot(NegatedDur); end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalPlainDate.pas` around lines 447 - 457, NegatedDur is only held in Pascal and must be temp-rooted while calling DateAdd; after creating NegatedDur (via TGocciaTemporalDurationValue.Create) call AddTempRoot(NegatedDur), then create NewArgs and call DateAdd, and in the finally block call RemoveTempRoot(NegatedDur) and then Free NewArgs (or Free NegatedDur if ownership semantics require) so the duration stays rooted for the duration of the DateAdd call; reference NegatedDur, TGocciaTemporalDurationValue.Create, NewArgs/TGocciaArgumentsCollection.Create, DateAdd, AddTempRoot and RemoveTempRoot.units/Goccia.Builtins.TestAssertions.pas (1)
709-736:⚠️ Potential issue | 🟠 Major
toHavePropertyhas inverted semantics—it checks property on the wrong argument.The implementation requires 2 arguments and checks the property on
AArgs[0], completely ignoringFActualValue(the actual value being tested). This contradicts the API (expect(value).toHaveProperty(name)) and the pattern used by all other matchers. Additionally, the unguarded cast ofAArgs[0]risksEInvalidCast. Change to take 1 argument (property name), validate thatFActualValueis an object, and check the property on it.🛠️ Suggested direction
-function TGocciaExpectationValue.ToHaveProperty(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; -var - Expected: TGocciaObjectValue; - HasProperty: Boolean; +function TGocciaExpectationValue.ToHaveProperty(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; +var + PropName: string; + HasProperty: Boolean; begin - TGocciaArgumentValidator.RequireExactly(AArgs, 2, 'toHaveProperty', FTestAssertions.ThrowError); - - Expected := AArgs.GetElement(0) as TGocciaObjectValue; - HasProperty := Expected.HasProperty(AArgs.GetElement(1).ToStringLiteral.Value); + TGocciaArgumentValidator.RequireExactly(AArgs, 1, 'toHaveProperty', FTestAssertions.ThrowError); + if not (FActualValue is TGocciaObjectValue) then + FTestAssertions.ThrowError('toHaveProperty expects the actual value to be an object', 0, 0); + + PropName := AArgs.GetElement(0).ToStringLiteral.Value; + HasProperty := TGocciaObjectValue(FActualValue).HasProperty(PropName); @@ - TGocciaTestAssertions(FTestAssertions).AssertionFailed('toHaveProperty', - 'Expected ' + FActualValue.ToStringLiteral.Value + ' not to have property ' + Expected.ToStringLiteral.Value) + TGocciaTestAssertions(FTestAssertions).AssertionFailed('toHaveProperty', + 'Expected ' + FActualValue.ToStringLiteral.Value + ' not to have property ' + PropName) @@ - TGocciaTestAssertions(FTestAssertions).AssertionFailed('toHaveProperty', - 'Expected ' + FActualValue.ToStringLiteral.Value + ' to have property ' + Expected.ToStringLiteral.Value); + TGocciaTestAssertions(FTestAssertions).AssertionFailed('toHaveProperty', + 'Expected ' + FActualValue.ToStringLiteral.Value + ' to have property ' + PropName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.TestAssertions.pas` around lines 709 - 736, TGocciaExpectationValue.ToHaveProperty currently reads the property name from AArgs[0] and checks it on AArgs[0] (and even casts it unsafely), but it should accept exactly one argument (the property name), validate that FActualValue is an object (cast FActualValue to TGocciaObjectValue safely, raising via FTestAssertions.ThrowError if not), then call the object's HasProperty with the string from AArgs.GetElement(0). Keep the FIsNegated handling and calls to TGocciaTestAssertions.AssertionPassed/AssertionFailed, but update the error messages to reference FActualValue.ToStringLiteral.Value and the property name; also change the RequireExactly call to expect 1 argument and remove the unsafe cast of AArgs[0] to TGocciaObjectValue.
🧹 Nitpick comments (11)
units/Goccia.Values.TemporalDuration.pas (1)
469-470: Remove unused local variables.The variables
NewYears,NewMonths,NewWeeks,NewDays,NewHours,NewMinutes,NewSeconds,NewMilliseconds,NewMicroseconds, andNewNanosecondsare declared but never used. The function directly passesGetFieldOrcalls to the constructor.♻️ Proposed fix
function TGocciaTemporalDurationValue.DurationWith(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; var D: TGocciaTemporalDurationValue; Obj: TGocciaObjectValue; V: TGocciaValue; - NewYears, NewMonths, NewWeeks, NewDays, NewHours, NewMinutes, NewSeconds, - NewMilliseconds, NewMicroseconds, NewNanoseconds: Int64;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalDuration.pas` around lines 469 - 470, Remove the unused local Int64 variables NewYears, NewMonths, NewWeeks, NewDays, NewHours, NewMinutes, NewSeconds, NewMilliseconds, NewMicroseconds, and NewNanoseconds from the procedure/function where they are declared (they are unused because the code passes GetFieldOr(...) results directly to the constructor), leaving only the needed parameters/locals and keeping the constructor call as-is (e.g., remove those identifiers from the var section surrounding the constructor invocation).units/TimingUtils.pas (1)
37-37: Verify:conston pointer parameter for external C function.The
const ATp: Pointerdeclaration means the pointer variable itself cannot be reassigned within Pascal code, but it doesn't affect how the C functionclock_gettimeuses the pointer (it still writes to the target memory). This should work correctly, but theconstqualifier on an external function's pointer parameter is atypical and may cause confusion sinceclock_gettimemodifies the data atATp.Consider removing
constfromATpsince the pointed-to data is mutated:Suggested change
-function clock_gettime(const AClk_id: cint; const ATp: Pointer): cint; cdecl; external 'c'; +function clock_gettime(const AClk_id: cint; ATp: Pointer): cint; cdecl; external 'c';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/TimingUtils.pas` at line 37, The declaration for the external C function clock_gettime currently uses a const on the pointer parameter (const ATp: Pointer) which is misleading because the C function writes to the memory pointed to by ATp; remove the const qualifier so the signature reads with ATp: Pointer (non-const) to match the mutating semantics and avoid confusion when calling clock_gettime from Pascal.units/Goccia.Lexer.pas (1)
39-39: Minor: ParameterCdoesn't follow A-prefix convention.For consistency with the project-wide naming convention, consider renaming
CtoAC.Suggested fix
- function IsValidIdentifierChar(const C: Char): Boolean; + function IsValidIdentifierChar(const AC: Char): Boolean;And update the implementation at Line 466-468:
-function TGocciaLexer.IsValidIdentifierChar(const C: Char): Boolean; +function TGocciaLexer.IsValidIdentifierChar(const AC: Char): Boolean; begin - Result := CharInSet(C, ValidIdentifierChars) or (Ord(C) > 127); + Result := CharInSet(AC, ValidIdentifierChars) or (Ord(AC) > 127); end;Based on learnings: "Use F prefix for private fields, A prefix for parameters".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Lexer.pas` at line 39, Rename the parameter C to AC in the function declaration IsValidIdentifierChar and in its implementation (previously referenced at the implementation block around the IsValidIdentifierChar routine) to follow the A-prefix parameter convention; update the function signature and all uses inside the function body, and adjust any external calls/overloads that pass or reference this parameter so they use the updated name.units/FileUtils.pas (1)
23-49: Consider callingFindCloseunconditionally.
FindCloseis currently only called after therepeat..untilloop completes. IfFindFirstsucceeds but an exception occurs during traversal, the search handle may not be released. Additionally, callingFindClosewhenFindFirstreturns non-zero is typically safe and ensures the handle is always cleaned up.Suggested fix using try/finally
if FindFirst(ADirectory + '/*', faAnyFile, SearchRec) = 0 then begin + try // If the file is a directory, add all the files in the directory repeat ... until FindNext(SearchRec) <> 0; + finally + FindClose(SearchRec); + end; - end; - FindClose(SearchRec); + end; Files.Sort; Result := Files; end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/FileUtils.pas` around lines 23 - 49, The code calls FindClose(SearchRec) only after the repeat..until loop so an exception during traversal can leak the search handle; wrap the FindFirst/loop/FindNext block in a try..finally that always calls FindClose(SearchRec) when FindFirst returns 0 (or simply call FindClose unconditionally in the finally), ensuring SearchRec is closed even if an exception occurs; update the function around the FindFirst/FindNext usage (affecting FindFirst, FindNext, FindClose, SearchRec, SubdirFiles, Files and the repeat..until block) to use try/finally semantics so resources are always released.format.pas (1)
192-203: Consider handlingin 'path'units specially for classification.When a uses clause contains
FileUtils in 'units/FileUtils.pas', the full string including the path is treated as the unit name. While sorting still works,ClassifyUnitwill classify based on the entire string, which may not match the intended category. This could causeGoccia.Something in 'path'to be classified as ucProject whileFileUtils in 'path'becomes ucRelative.This appears to work correctly for the current codebase, but documenting or handling this explicitly would improve robustness.
units/Goccia.Values.FunctionBase.pas (1)
186-237: Unused local variableCallArgsin FunctionApply.The variable
CallArgsis declared on line 188 but the code uses a different localCallArgswhich shadows it. This appears to be pre-existing, but worth noting for cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.FunctionBase.pas` around lines 186 - 237, The FunctionApply implementation declares a duplicated/shadowed local named CallArgs which makes the outer CallArgs unused; in TGocciaFunctionSharedPrototype.FunctionApply remove the inner shadowing declaration (or rename it) so the single CallArgs variable created with TGocciaArgumentsCollection.Create is the one populated and later freed in the finally block, ensuring CallArgs.Add and CallArgs.Free operate on the same variable.units/Goccia.Values.StringObjectValue.pas (5)
45-45: Minor formatting: extra space before semicolon.There's an inconsistent space before the semicolon in the
StringSubstringdeclaration.- function StringSubstring(const AArgs: TGocciaArgumentsCollection ; const AThisValue: TGocciaValue): TGocciaValue; + function StringSubstring(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.StringObjectValue.pas` at line 45, Remove the stray space before the semicolon in the function declaration for StringSubstring: adjust the declaration of function StringSubstring(const AArgs: TGocciaArgumentsCollection ; const AThisValue: TGocciaValue): TGocciaValue so the space before the semicolon after TGocciaArgumentsCollection is removed, ensuring the signature reads with consistent spacing around parameters and types (refer to the StringSubstring declaration and the TGocciaArgumentsCollection / TGocciaValue parameter names).
193-242: Remove unused variableTempNumberValue.The variable
TempNumberValueis declared at line 198 but never used inStringCharAt.🧹 Proposed fix
function TGocciaStringObjectValue.StringCharAt(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; var StringValue: string; Index: Integer; NumberValue: Double; - TempNumberValue: TGocciaNumberLiteralValue; Arg: TGocciaValue; begin🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.StringObjectValue.pas` around lines 193 - 242, The variable TempNumberValue declared in TGocciaStringObjectValue.StringCharAt is unused; remove the declaration of TempNumberValue (type TGocciaNumberLiteralValue) from the function's var section so only the actually used locals (StringValue, Index, NumberValue, Arg) remain, leaving the function logic unchanged.
244-293: Remove unused variableTempNumberValue.The variable
TempNumberValueis declared at line 249 but never used inStringCharCodeAt.🧹 Proposed fix
function TGocciaStringObjectValue.StringCharCodeAt(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; var StringValue: string; Index: Integer; NumberValue: Double; - TempNumberValue: TGocciaNumberLiteralValue; Arg: TGocciaValue; begin🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.StringObjectValue.pas` around lines 244 - 293, Remove the unused local variable TempNumberValue from the function TGocciaStringObjectValue.StringCharCodeAt; open the function and delete the declaration "TempNumberValue: TGocciaNumberLiteralValue;" so only the actually used locals (StringValue, Index, NumberValue, Arg) remain, leaving all logic unchanged.
528-549: Remove unused variablesStartPositionandFoundIndex.These variables are declared but never used in
StringStartsWith.🧹 Proposed fix
function TGocciaStringObjectValue.StringStartsWith(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; var StringValue, SearchValue: string; - StartPosition: Integer; - FoundIndex: Integer; begin🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.StringObjectValue.pas` around lines 528 - 549, The function TGocciaStringObjectValue.StringStartsWith declares unused local variables StartPosition and FoundIndex; remove these two declarations from the function's var section so only StringValue, SearchValue remain, ensuring no other code references them and compile after the cleanup.
551-572: Remove unused variablesStartPositionandFoundIndex.These variables are declared but never used in
StringEndsWith.🧹 Proposed fix
function TGocciaStringObjectValue.StringEndsWith(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; var StringValue, SearchValue: string; - StartPosition: Integer; - FoundIndex: Integer; begin🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.StringObjectValue.pas` around lines 551 - 572, In TGocciaStringObjectValue.StringEndsWith remove the unused local variables StartPosition and FoundIndex from the function's var declaration since they are never referenced; update the var section to only declare StringValue and SearchValue so the compiler no longer reports unused variables and the function remains unchanged otherwise.
🤖 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/build-system.md`:
- Around line 219-244: The docs incorrectly claim test files ("*.Test.pas") are
excluded; update the documentation to reflect actual behavior by removing or
correcting that sentence and describing the real Lefthook pattern and formatter
behavior (the lefthook.yml glob "*.{pas,dpr}" and the format.pas script do not
exclude test files). Edit the Auto-Formatter section to either (a) state that
test files are formatted like other .pas/.dpr files, or (b) if exclusion is
desired, add clear instructions referencing lefthook.yml and format.pas on how
to exclude "*.Test.pas" (e.g., by changing the glob or adding exclusion logic)
and note that stage_fixed: true auto-stages fixes.
In `@format.pas`:
- Around line 22-24: The decorative comment block has been accidentally
concatenated with the start of the function declaration; separate them so the
comment ends with a newline and the function declaration "function
IsUsesKeyword(const ALine: string): Boolean;" starts on its own line (i.e.,
break the line between the comment and the declaration and ensure proper
indentation for the IsUsesKeyword function header).
In `@lefthook.yml`:
- Around line 1-6: The current glob pattern under the pre-commit command
"format-pascal" only matches root .pas/.dpr files and misses nested files;
update lefthook.yml by either changing the "glob" for "format-pascal" to include
both root and nested patterns (use an array with "*.{pas,dpr}" and
"**/*.{pas,dpr}") or enable doublestar semantics at the top-level by adding
"glob_matcher: doublestar" and then use "**/*.{pas,dpr}" for the "glob"; adjust
the "format-pascal" entry accordingly and keep "run: ./format.pas
{staged_files}" and "stage_fixed: true" unchanged.
In `@units/Goccia.Evaluator.Assignment.pas`:
- Around line 48-55: AssignProperty currently checks Assigned(AOnError) but
always calls ThrowTypeError, so either invoke the callback or remove the check;
change the error path in AssignProperty to call AOnError with a descriptive
message and the ALine/ AColumn (or whatever parameters TGocciaThrowErrorCallback
expects) when Assigned(AOnError) is true, and only call ThrowTypeError when
AOnError is nil — this ensures AOnError is used for reporting while falling back
to ThrowTypeError, and keep the successful path using AObj.SetProperty
unchanged.
---
Outside diff comments:
In `@units/Goccia.Builtins.GlobalNumber.pas`:
- Around line 257-263: The code casts AArgs.GetElement(0) to
TGocciaNumberLiteralValue in NumberArg before checking its type, risking invalid
casts; change the order so you first check "if not (AArgs.GetElement(0) is
TGocciaNumberLiteralValue) then ..." and return the FalseValue, and only after
the type check assign NumberArg :=
TGocciaNumberLiteralValue(AArgs.GetElement(0)); this ensures
TGocciaNumberLiteralValue, NumberArg and the type guard are used correctly
without performing an unsafe cast.
- Around line 308-314: In NumberIsInteger fix the improper cast-before-check:
stop assigning NumberArg by casting the argument (NumberArg :=
TGocciaNumberLiteralValue(AArgs.GetElement(0))) and instead retrieve the raw
element first (via AArgs.GetElement(0)), check its type with "if not (NumberArg
is TGocciaNumberLiteralValue) then" and only after that perform the cast to
TGocciaNumberLiteralValue for further use; reference the NumberArg variable, the
AArgs.GetElement(0) call, and TGocciaNumberLiteralValue in your change so the
runtime type check precedes the cast.
In `@units/Goccia.Builtins.GlobalString.pas`:
- Around line 47-51: In TGocciaGlobalString.Destroy, avoid directly calling
FStringConstructor.Free because TGocciaStringClassValue auto-registers with the
GC (AfterConstruction) and may be freed by TGocciaGC; instead wrap the free in
the same guard used by TGocciaBuiltin (check Assigned(TGocciaGC.Instance)) so
only free FStringConstructor when the GC is not active (i.e., if not
Assigned(TGocciaGC.Instance) then FStringConstructor.Free); keep inherited call
as-is.
In `@units/Goccia.Builtins.Temporal.pas`:
- Around line 84-99: TGocciaTemporalBuiltin.Create currently constructs
FTemporalNamespace and calls
RegisterDuration/RegisterInstant/RegisterPlainDate/RegisterPlainTime/RegisterPlainDateTime/RegisterNow
before binding it into the scope, which risks GC collection; wrap the
registration/binding sequence in a temporary root by calling
AddTempRoot(FTemporalNamespace) immediately after creating FTemporalNamespace
and RemoveTempRoot(FTemporalNamespace) after AScope.DefineLexicalBinding, and
add the Goccia.GarbageCollector unit to the uses clause; ensure the
AddTempRoot/RemoveTempRoot pair encloses all Register* calls and the
DefineLexicalBinding so the namespace cannot be collected while being populated.
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 709-736: TGocciaExpectationValue.ToHaveProperty currently reads
the property name from AArgs[0] and checks it on AArgs[0] (and even casts it
unsafely), but it should accept exactly one argument (the property name),
validate that FActualValue is an object (cast FActualValue to TGocciaObjectValue
safely, raising via FTestAssertions.ThrowError if not), then call the object's
HasProperty with the string from AArgs.GetElement(0). Keep the FIsNegated
handling and calls to TGocciaTestAssertions.AssertionPassed/AssertionFailed, but
update the error messages to reference FActualValue.ToStringLiteral.Value and
the property name; also change the RequireExactly call to expect 1 argument and
remove the unsafe cast of AArgs[0] to TGocciaObjectValue.
In `@units/Goccia.SharedPrototype.pas`:
- Around line 27-37: In TGocciaSharedPrototype.Create, guard the optional
AMethodHost/FMethodHost before calling TGocciaGC.Instance.PinValue to avoid AVs:
check if FMethodHost is assigned (e.g., if Assigned(FMethodHost) then
TGocciaGC.Instance.PinValue(FMethodHost)) instead of always calling PinValue;
update the constructor (TGocciaSharedPrototype.Create) to only pin FMethodHost
when non-nil (or alternatively add doc comment to require AMethodHost non-nil)
and ensure any other uses of FMethodHost in this unit similarly handle a nil
value.
In `@units/Goccia.Values.ClassValue.pas`:
- Around line 407-427: In TGocciaClassValue.Instantiate, the newly created
Instance must be rooted before calling ConstructorToCall.Call to prevent GC from
collecting it during the call; wrap the call with AddTempRoot(Instance) and
RemoveTempRoot(Instance) (and add Goccia.GarbageCollector to the uses clause if
missing), so: create Instance as now, determine ConstructorToCall, then if
Assigned(ConstructorToCall) call AddTempRoot(Instance) before invoking
ConstructorToCall.Call(AArguments, Instance) and ensure RemoveTempRoot(Instance)
runs afterwards (use a try/finally to guarantee removal).
In `@units/Goccia.Values.TemporalDuration.pas`:
- Around line 528-536: The nanosecond aggregation silently ignores calendar
units FYears and FMonths; before computing TotalNs in the duration total routine
(the block that computes TotalNs from D.FNanoseconds...D.FWeeks), validate
D.FYears and D.FMonths and throw a RangeError if either is nonzero so durations
with calendar units require a relativeTo per the spec; place the check
immediately before the TotalNs assignment and use the same exception type used
elsewhere (RangeError) with a clear message.
In `@units/Goccia.Values.TemporalPlainDate.pas`:
- Around line 447-457: NegatedDur is only held in Pascal and must be temp-rooted
while calling DateAdd; after creating NegatedDur (via
TGocciaTemporalDurationValue.Create) call AddTempRoot(NegatedDur), then create
NewArgs and call DateAdd, and in the finally block call
RemoveTempRoot(NegatedDur) and then Free NewArgs (or Free NegatedDur if
ownership semantics require) so the duration stays rooted for the duration of
the DateAdd call; reference NegatedDur, TGocciaTemporalDurationValue.Create,
NewArgs/TGocciaArgumentsCollection.Create, DateAdd, AddTempRoot and
RemoveTempRoot.
---
Nitpick comments:
In `@units/FileUtils.pas`:
- Around line 23-49: The code calls FindClose(SearchRec) only after the
repeat..until loop so an exception during traversal can leak the search handle;
wrap the FindFirst/loop/FindNext block in a try..finally that always calls
FindClose(SearchRec) when FindFirst returns 0 (or simply call FindClose
unconditionally in the finally), ensuring SearchRec is closed even if an
exception occurs; update the function around the FindFirst/FindNext usage
(affecting FindFirst, FindNext, FindClose, SearchRec, SubdirFiles, Files and the
repeat..until block) to use try/finally semantics so resources are always
released.
In `@units/Goccia.Lexer.pas`:
- Line 39: Rename the parameter C to AC in the function declaration
IsValidIdentifierChar and in its implementation (previously referenced at the
implementation block around the IsValidIdentifierChar routine) to follow the
A-prefix parameter convention; update the function signature and all uses inside
the function body, and adjust any external calls/overloads that pass or
reference this parameter so they use the updated name.
In `@units/Goccia.Values.FunctionBase.pas`:
- Around line 186-237: The FunctionApply implementation declares a
duplicated/shadowed local named CallArgs which makes the outer CallArgs unused;
in TGocciaFunctionSharedPrototype.FunctionApply remove the inner shadowing
declaration (or rename it) so the single CallArgs variable created with
TGocciaArgumentsCollection.Create is the one populated and later freed in the
finally block, ensuring CallArgs.Add and CallArgs.Free operate on the same
variable.
In `@units/Goccia.Values.StringObjectValue.pas`:
- Line 45: Remove the stray space before the semicolon in the function
declaration for StringSubstring: adjust the declaration of function
StringSubstring(const AArgs: TGocciaArgumentsCollection ; const AThisValue:
TGocciaValue): TGocciaValue so the space before the semicolon after
TGocciaArgumentsCollection is removed, ensuring the signature reads with
consistent spacing around parameters and types (refer to the StringSubstring
declaration and the TGocciaArgumentsCollection / TGocciaValue parameter names).
- Around line 193-242: The variable TempNumberValue declared in
TGocciaStringObjectValue.StringCharAt is unused; remove the declaration of
TempNumberValue (type TGocciaNumberLiteralValue) from the function's var section
so only the actually used locals (StringValue, Index, NumberValue, Arg) remain,
leaving the function logic unchanged.
- Around line 244-293: Remove the unused local variable TempNumberValue from the
function TGocciaStringObjectValue.StringCharCodeAt; open the function and delete
the declaration "TempNumberValue: TGocciaNumberLiteralValue;" so only the
actually used locals (StringValue, Index, NumberValue, Arg) remain, leaving all
logic unchanged.
- Around line 528-549: The function TGocciaStringObjectValue.StringStartsWith
declares unused local variables StartPosition and FoundIndex; remove these two
declarations from the function's var section so only StringValue, SearchValue
remain, ensuring no other code references them and compile after the cleanup.
- Around line 551-572: In TGocciaStringObjectValue.StringEndsWith remove the
unused local variables StartPosition and FoundIndex from the function's var
declaration since they are never referenced; update the var section to only
declare StringValue and SearchValue so the compiler no longer reports unused
variables and the function remains unchanged otherwise.
In `@units/Goccia.Values.TemporalDuration.pas`:
- Around line 469-470: Remove the unused local Int64 variables NewYears,
NewMonths, NewWeeks, NewDays, NewHours, NewMinutes, NewSeconds, NewMilliseconds,
NewMicroseconds, and NewNanoseconds from the procedure/function where they are
declared (they are unused because the code passes GetFieldOr(...) results
directly to the constructor), leaving only the needed parameters/locals and
keeping the constructor call as-is (e.g., remove those identifiers from the var
section surrounding the constructor invocation).
In `@units/TimingUtils.pas`:
- Line 37: The declaration for the external C function clock_gettime currently
uses a const on the pointer parameter (const ATp: Pointer) which is misleading
because the C function writes to the memory pointed to by ATp; remove the const
qualifier so the signature reads with ATp: Pointer (non-const) to match the
mutating semantics and avoid confusion when calling clock_gettime from Pascal.
Comment on lines
+48
to
55
| procedure AssignProperty(const AObj: TGocciaValue; const APropertyName: string; const AValue: TGocciaValue; const AOnError: TGocciaThrowErrorCallback; const ALine, AColumn: Integer); | ||
| begin | ||
| if (Obj is TGocciaInstanceValue) or (Obj is TGocciaObjectValue) or | ||
| (Obj is TGocciaClassValue) or (Obj is TGocciaArrayValue) then | ||
| Obj.SetProperty(PropertyName, Value) | ||
| else if Assigned(OnError) then | ||
| if (AObj is TGocciaInstanceValue) or (AObj is TGocciaObjectValue) or | ||
| (AObj is TGocciaClassValue) or (AObj is TGocciaArrayValue) then | ||
| AObj.SetProperty(APropertyName, AValue) | ||
| else if Assigned(AOnError) then | ||
| ThrowTypeError('Cannot set property on non-object'); | ||
| end; |
Contributor
There was a problem hiding this comment.
AOnError is checked but not used for reporting the error.
The code checks if Assigned(AOnError) but then calls ThrowTypeError directly instead of invoking AOnError. This appears to be dead code—either the error should be reported via AOnError(...) or the check should be removed.
Proposed fix
procedure AssignProperty(const AObj: TGocciaValue; const APropertyName: string; const AValue: TGocciaValue; const AOnError: TGocciaThrowErrorCallback; const ALine, AColumn: Integer);
begin
if (AObj is TGocciaInstanceValue) or (AObj is TGocciaObjectValue) or
(AObj is TGocciaClassValue) or (AObj is TGocciaArrayValue) then
AObj.SetProperty(APropertyName, AValue)
else if Assigned(AOnError) then
- ThrowTypeError('Cannot set property on non-object');
+ AOnError('Cannot set property on non-object', ALine, AColumn);
end;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| procedure AssignProperty(const AObj: TGocciaValue; const APropertyName: string; const AValue: TGocciaValue; const AOnError: TGocciaThrowErrorCallback; const ALine, AColumn: Integer); | |
| begin | |
| if (Obj is TGocciaInstanceValue) or (Obj is TGocciaObjectValue) or | |
| (Obj is TGocciaClassValue) or (Obj is TGocciaArrayValue) then | |
| Obj.SetProperty(PropertyName, Value) | |
| else if Assigned(OnError) then | |
| if (AObj is TGocciaInstanceValue) or (AObj is TGocciaObjectValue) or | |
| (AObj is TGocciaClassValue) or (AObj is TGocciaArrayValue) then | |
| AObj.SetProperty(APropertyName, AValue) | |
| else if Assigned(AOnError) then | |
| ThrowTypeError('Cannot set property on non-object'); | |
| end; | |
| procedure AssignProperty(const AObj: TGocciaValue; const APropertyName: string; const AValue: TGocciaValue; const AOnError: TGocciaThrowErrorCallback; const ALine, AColumn: Integer); | |
| begin | |
| if (AObj is TGocciaInstanceValue) or (AObj is TGocciaObjectValue) or | |
| (AObj is TGocciaClassValue) or (AObj is TGocciaArrayValue) then | |
| AObj.SetProperty(APropertyName, AValue) | |
| else if Assigned(AOnError) then | |
| AOnError('Cannot set property on non-object', ALine, AColumn); | |
| end; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Evaluator.Assignment.pas` around lines 48 - 55, AssignProperty
currently checks Assigned(AOnError) but always calls ThrowTypeError, so either
invoke the callback or remove the check; change the error path in AssignProperty
to call AOnError with a descriptive message and the ALine/ AColumn (or whatever
parameters TGocciaThrowErrorCallback expects) when Assigned(AOnError) is true,
and only call ThrowTypeError when AOnError is nil — this ensures AOnError is
used for reporting while falling back to ThrowTypeError, and keep the successful
path using AObj.SetProperty unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
units/Goccia.Values.StringObjectValue.pas (1)
650-755:⚠️ Potential issue | 🟠 MajorProtect
ResultArraywith AddTempRoot/RemoveTempRoot while building.
Line 686 onward builds aTGocciaArrayValuethat’s only held by Pascal code until return. GC can run during allocations inside the loop, so the array should be temporarily rooted.🔧 Proposed fix
- ResultArray := TGocciaArrayValue.Create; + ResultArray := TGocciaArrayValue.Create; + if Assigned(TGocciaGC.Instance) then + TGocciaGC.Instance.AddTempRoot(ResultArray); + try ... - Result := ResultArray; + Result := ResultArray; + finally + if Assigned(TGocciaGC.Instance) then + TGocciaGC.Instance.RemoveTempRoot(ResultArray); + end; end;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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.StringObjectValue.pas` around lines 650 - 755, The function TGocciaStringObjectValue.StringSplit builds a TGocciaArrayValue (ResultArray) that lives only in Pascal stack and must be protected from the GC; after creating ResultArray assign it a temp root via the runtime's AddTempRoot and ensure every exit path calls RemoveTempRoot before returning (including the early returns for Limit=0, empty-string cases, and after final Result assignment); wrap all code that mutates ResultArray (the character loop, manual split loop, and the branch that adds the original string) between AddTempRoot(ResultArray) and a matching RemoveTempRoot(ResultArray) so the array is rooted while you create TGocciaStringLiteralValue elements and only unroot immediately before returning the Result value.units/Goccia.Builtins.TestAssertions.pas (1)
298-306:⚠️ Potential issue | 🟠 MajorFix NaN detection to avoid false negatives.
Math.IsNaN(FActualValue.ToNumberLiteral.Value)can miss NaN if special values store0.0internally. Use the value-object’sIsNaNinstead.Proposed fix
- if FActualValue is TGocciaNumberLiteralValue then - IsNaNValue := TGocciaNumberLiteralValue(FActualValue).IsNaN - else - IsNaNValue := Math.IsNaN(FActualValue.ToNumberLiteral.Value); + IsNaNValue := FActualValue.ToNumberLiteral.IsNaN;Based on learnings: In Pascal tests, use Value.ToNumberLiteral.IsNaN (not Math.IsNaN) for NaN checks; special values store 0.0 internally.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.TestAssertions.pas` around lines 298 - 306, The NaN check in TGocciaExpectationValue.ToBeNaN currently falls back to Math.IsNaN(FActualValue.ToNumberLiteral.Value) which can miss NaN for special value objects that store 0.0; replace that fallback with the value-object check by calling FActualValue.ToNumberLiteral.IsNaN (i.e., use the ToNumberLiteral.IsNaN method instead of Math.IsNaN) and keep the existing branch that uses TGocciaNumberLiteralValue(...).IsNaN so all code paths rely on the value object's IsNaN to avoid false negatives.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@format.pas`:
- Around line 394-475: ReplaceWordInLine currently replaces identifiers even
when qualified (e.g., Self.Value → Self.AValue); update the replacement check
inside ReplaceWordInLine (the block that tests CompareText(Copy(ALine, I,
OldLen), AOld) and IsIdentChar) to skip the replacement when the identifier is
qualified by a dot: ensure the character immediately before position I is not
'.' (taking into account I=1) before accepting the replacement; adjust the same
logic used by FixParamNames so qualified references are left untouched (refer to
symbols ALine, I, OldLen, AOld, ANew, IsIdentChar and the replacement
conditional).
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 709-718: TGocciaExpectationValue.ToHaveProperty currently
validates two arguments and checks the first argument object; change it to
require exactly 1 argument and test the property on FActualValue. Specifically,
update TGocciaExpectationValue.ToHaveProperty to call
TGocciaArgumentValidator.RequireExactly(AArgs, 1, 'toHaveProperty',
FTestAssertions.ThrowError), cast FActualValue to TGocciaObjectValue (instead of
using AArgs.GetElement(0) as TGocciaObjectValue), and set HasProperty :=
(FActualValue as
TGocciaObjectValue).HasProperty(AArgs.GetElement(0).ToStringLiteral.Value);
remove the unused Expected variable and ensure the method returns the correct
TGocciaValue result based on HasProperty.
In `@units/Goccia.Values.StringObjectValue.pas`:
- Around line 77-85: In TGocciaStringObjectValue.ExtractStringValue detect
Symbol values and throw a TypeError instead of falling through to
ToStringLiteral: check for TGocciaSymbolValue (or the symbol value class used
elsewhere) when inspecting AValue and raise the VM type exception (e.g.
EGocciaTypeError) with a message like 'Cannot convert a Symbol value to a
string' rather than returning AValue.ToStringLiteral.Value; keep the existing
branches for TGocciaStringLiteralValue and TGocciaStringObjectValue intact and
only add the symbol check before the ToStringLiteral fallback.
---
Outside diff comments:
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 298-306: The NaN check in TGocciaExpectationValue.ToBeNaN
currently falls back to Math.IsNaN(FActualValue.ToNumberLiteral.Value) which can
miss NaN for special value objects that store 0.0; replace that fallback with
the value-object check by calling FActualValue.ToNumberLiteral.IsNaN (i.e., use
the ToNumberLiteral.IsNaN method instead of Math.IsNaN) and keep the existing
branch that uses TGocciaNumberLiteralValue(...).IsNaN so all code paths rely on
the value object's IsNaN to avoid false negatives.
In `@units/Goccia.Values.StringObjectValue.pas`:
- Around line 650-755: The function TGocciaStringObjectValue.StringSplit builds
a TGocciaArrayValue (ResultArray) that lives only in Pascal stack and must be
protected from the GC; after creating ResultArray assign it a temp root via the
runtime's AddTempRoot and ensure every exit path calls RemoveTempRoot before
returning (including the early returns for Limit=0, empty-string cases, and
after final Result assignment); wrap all code that mutates ResultArray (the
character loop, manual split loop, and the branch that adds the original string)
between AddTempRoot(ResultArray) and a matching RemoveTempRoot(ResultArray) so
the array is rooted while you create TGocciaStringLiteralValue elements and only
unroot immediately before returning the Result value.
---
Duplicate comments:
In `@units/Goccia.Evaluator.Assignment.pas`:
- Around line 48-59: AssignProperty currently calls ThrowTypeError without
source location, so the thrown TGocciaThrowValue lacks ALine/AColumn context;
update AssignProperty to pass the ALine and AColumn to ThrowTypeError (or call
the overload that accepts location) when invoking it in the else branch so the
JavaScript-level TypeError includes source position; if no such overload exists,
add one (or modify ThrowTypeError signature) and propagate ALine/AColumn through
to the TGocciaThrowValue creation, keeping the existing AOnError check and
comment about TGocciaRuntimeError vs TGocciaThrowValue.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
format.pas (2)
843-899: Minor:InBlockcounter may mishandle nested{characters inside comments.Pascal block comments don't nest, so
{ foo { bar }ends at the first}. The counter-based approach at lines 871-879 would incorrectly track this case, leavingInBlock > 0when the comment has actually ended.For practical purposes, nested
{inside comments is rare, but using a boolean instead of a counter would be more accurate:💡 Suggested fix
var I, J, SpaceStart: Integer; Line: string; InStr, InLineComment: Boolean; - InBlock: Integer; + InBlock: Boolean; begin Result := False; - InBlock := 0; + InBlock := False; for I := 0 to ALines.Count - 1 do begin Line := ALines[I]; ... - if InBlock > 0 then + if InBlock then begin if Line[J] = '}' then - Dec(InBlock); + InBlock := False; ... else if Line[J] = '{' then - Inc(InBlock) + InBlock := True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@format.pas` around lines 843 - 899, The InBlock counter in FixStraySpaces can mis-handle nested '{' inside comments; change it to a boolean (e.g., InBlockComment: Boolean) initialized False, set True when you hit '{' (and not in a string or line comment) and set False when you hit the first '}' (do not increment/decrement), replace all checks of "InBlock > 0" with "InBlockComment", and remove Inc(InBlock)/Dec(InBlock) uses so the comment state toggles only on the first open and first close brace.
133-145: Minor:StripLineCommentdoes not account for{inside string literals.The function strips content after
{without checking if it's inside a string literal. This could cause issues if a uses clause contains a string with{(rare in practice but possible within 'path{name}.pas').For uses-clause parsing, this is unlikely to cause real issues, but worth noting if the function is reused elsewhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@format.pas` around lines 133 - 145, The current StripLineComment naively finds '//' and '{' with Pos, which fails when those appear inside single-quoted string literals; replace the logic with a single character-by-character scan of ALine that tracks when you're inside a single-quote string (handle doubled single quotes as escaped quotes), ignore '//' and '{' while in a string, and stop at the first '//' or '{' encountered outside a string then return the substring up to that position; update the function StripLineComment (and its local P/loop) to use this stateful scan so both '//' and '{' are correctly ignored when inside quoted literals.
🤖 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.StringObjectValue.pas`:
- Around line 738-754: The StringRepeat implementation must validate the repeat
count before calling DupeString: read the numeric count into a floating value
(e.g., RawCount := AArgs.GetElement(0).ToNumberLiteral.Value), check if RawCount
is negative or infinite (use IsInfinite/IsNan checks appropriate for your
platform) and if so throw a JavaScript RangeError via the engine's normal
error/exception helper; only after validation convert/truncate RawCount to an
integer Count and call DupeString(StringValue, Count) to create the
TGocciaStringLiteralValue. Ensure these checks are added in
TGocciaStringObjectValue.StringRepeat and use existing helpers
(ExtractStringValue, AArgs.GetElement, DupeString) and the runtime's RangeError
creation mechanism.
---
Nitpick comments:
In `@format.pas`:
- Around line 843-899: The InBlock counter in FixStraySpaces can mis-handle
nested '{' inside comments; change it to a boolean (e.g., InBlockComment:
Boolean) initialized False, set True when you hit '{' (and not in a string or
line comment) and set False when you hit the first '}' (do not
increment/decrement), replace all checks of "InBlock > 0" with "InBlockComment",
and remove Inc(InBlock)/Dec(InBlock) uses so the comment state toggles only on
the first open and first close brace.
- Around line 133-145: The current StripLineComment naively finds '//' and '{'
with Pos, which fails when those appear inside single-quoted string literals;
replace the logic with a single character-by-character scan of ALine that tracks
when you're inside a single-quote string (handle doubled single quotes as
escaped quotes), ignore '//' and '{' while in a string, and stop at the first
'//' or '{' encountered outside a string then return the substring up to that
position; update the function StripLineComment (and its local P/loop) to use
this stateful scan so both '//' and '{' are correctly ignored when inside quoted
literals.
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.StringObjectValue.pas (1)
207-234:⚠️ Potential issue | 🟠 MajorDon’t swallow numeric‑coercion errors in charAt/charCodeAt
The blanket try/except defaultsIndexto 0, which masksToNumberLiteralTypeErrors (e.g., Symbol or user‑defined valueOf errors). These should propagate per the engine’s numeric‑coercion rules.Based on learnings: "Symbol coercion: TGocciaSymbolValue.ToNumberLiteral throws TypeError; ToStringLiteral returns "Symbol(description)"; implicit string coercion (template literals, + operator, concat) must check for symbols and throw TypeError at operator level."🔧 Suggested fix (apply to both methods)
- try - Arg := AArgs.GetElement(0); - - // Handle special values according to ECMAScript spec: - // - undefined/null convert to 0 - // - NaN converts to 0 - // - Infinity/-Infinity are treated as out-of-bounds (use very large number) - if (Arg is TGocciaUndefinedLiteralValue) or (Arg is TGocciaNullLiteralValue) then - Index := 0 - else if (Arg is TGocciaNumberLiteralValue) then - begin - if TGocciaNumberLiteralValue(Arg).IsNaN then - Index := 0 - else if TGocciaNumberLiteralValue(Arg).IsInfinity or TGocciaNumberLiteralValue(Arg).IsNegativeInfinity then - Index := MaxInt // Force out-of-bounds - else - Index := Trunc(TGocciaNumberLiteralValue(Arg).Value); - end - else - begin - NumberValue := Arg.ToNumberLiteral.Value; - Index := Trunc(NumberValue); - end; - except - Index := 0; // Default to 0 if conversion fails - end; + Arg := AArgs.GetElement(0); + + // Handle special values according to ECMAScript spec: + // - undefined/null convert to 0 + // - NaN converts to 0 + // - Infinity/-Infinity are treated as out-of-bounds (use very large number) + if (Arg is TGocciaUndefinedLiteralValue) or (Arg is TGocciaNullLiteralValue) then + Index := 0 + else if (Arg is TGocciaNumberLiteralValue) then + begin + if TGocciaNumberLiteralValue(Arg).IsNaN then + Index := 0 + else if TGocciaNumberLiteralValue(Arg).IsInfinity or TGocciaNumberLiteralValue(Arg).IsNegativeInfinity then + Index := MaxInt // Force out-of-bounds + else + Index := Trunc(TGocciaNumberLiteralValue(Arg).Value); + end + else + begin + NumberValue := Arg.ToNumberLiteral.Value; + Index := Trunc(NumberValue); + end;Also applies to: 258-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.StringObjectValue.pas` around lines 207 - 234, The current broad try/except in the charAt/charCodeAt conversion block swallows numeric‑coercion exceptions (e.g., errors thrown by Arg.ToNumberLiteral for Symbol or faulty valueOf) — remove or narrow that blanket exception handling so TypeError (or any exception thrown by ToNumberLiteral) is allowed to propagate; specifically, in the methods implementing charAt/charCodeAt, stop defaulting Index := 0 inside a general except block and instead perform the Arg checks and numeric conversions directly (use TGocciaNumberLiteralValue checks for NaN/Infinity and call Arg.ToNumberLiteral.Value without catching its exceptions), or if you must catch, re-raise any TypeError exceptions thrown by ToNumberLiteral rather than swallowing them; apply the same change to the other identical conversion block in the file.
🤖 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.StringObjectValue.pas`:
- Around line 207-234: The current broad try/except in the charAt/charCodeAt
conversion block swallows numeric‑coercion exceptions (e.g., errors thrown by
Arg.ToNumberLiteral for Symbol or faulty valueOf) — remove or narrow that
blanket exception handling so TypeError (or any exception thrown by
ToNumberLiteral) is allowed to propagate; specifically, in the methods
implementing charAt/charCodeAt, stop defaulting Index := 0 inside a general
except block and instead perform the Arg checks and numeric conversions directly
(use TGocciaNumberLiteralValue checks for NaN/Infinity and call
Arg.ToNumberLiteral.Value without catching its exceptions), or if you must
catch, re-raise any TypeError exceptions thrown by ToNumberLiteral rather than
swallowing them; apply the same change to the other identical conversion block
in the file.
This was referenced Feb 18, 2026
Merged
Merged
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
Documentation
Development Tooling
Public API
Tests