Refactor built-in object model registration#136
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new object-model registration framework (units: Changes
Sequence Diagram(s)sequenceDiagram
participant Engine as TGocciaEngine
participant Registrar as RegisterTypeDefinition
participant Scope as TGocciaScope
participant Constructor as TGocciaClassValue
participant Prototype as PrototypeProvider / SharedPrototype
participant StaticSrc as StaticSource (TGocciaObjectValue)
Engine->>Registrar: Build TGocciaTypeDefinition (name, kind, ClassValueClass, PrototypeProvider/ExposePrototype, StaticSource, PrototypeParent, AddSpeciesGetter)
Registrar->>Scope: Create TGocciaClassValue in Scope
alt ExposePrototype provided
Registrar->>Prototype: Call ExposePrototype(Constructor)
else PrototypeProvider provided
Registrar->>Prototype: Prototype := PrototypeProvider()
Registrar->>Constructor: Set prototype from Prototype
end
Registrar->>Constructor: Optionally set PrototypeParent
alt StaticSource provided
Registrar->>StaticSrc: CopyOwnProperties(StaticSource -> Constructor)
end
alt AddSpeciesGetter
Registrar->>Constructor: Define [Symbol.species] accessor
end
Registrar->>Scope: Register Constructor as lexical const (ConstructorName)
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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results263 benchmarks Interpreted: 🟢 5 improved · 258 unchanged · avg +0.6% arraybuffer.js — Interp: 🟢 1, 13 unch. · avg +2.2% · Bytecode: 14 unch. · avg +1.5%
arrays.js — Interp: 19 unch. · avg +1.3% · Bytecode: 🟢 1, 18 unch. · avg +3.2%
async-await.js — Interp: 6 unch. · avg +1.4% · Bytecode: 6 unch. · avg +1.9%
classes.js — Interp: 🟢 1, 30 unch. · avg +1.5% · Bytecode: 31 unch. · avg +1.1%
closures.js — Interp: 11 unch. · avg +0.8% · Bytecode: 11 unch. · avg +0.5%
collections.js — Interp: 12 unch. · avg +0.5% · Bytecode: 🔴 1, 11 unch. · avg -2.8%
destructuring.js — Interp: 🟢 1, 21 unch. · avg +2.6% · Bytecode: 🔴 1, 21 unch. · avg -1.1%
fibonacci.js — Interp: 8 unch. · avg -0.9% · Bytecode: 🔴 1, 7 unch. · avg -1.7%
for-of.js — Interp: 7 unch. · avg -1.7% · Bytecode: 🔴 1, 6 unch. · avg -2.2%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 20 unch. · avg -0.4% · Bytecode: 🔴 2, 18 unch. · avg -3.2%
json.js — Interp: 20 unch. · avg -0.2% · Bytecode: 🔴 3, 17 unch. · avg -0.8%
jsx.jsx — Interp: 21 unch. · avg -2.1% · Bytecode: 21 unch. · avg +0.2%
modules.js — Interp: 🟢 1, 8 unch. · avg +4.2% · Bytecode: 9 unch. · avg -2.5%
numbers.js — Interp: 11 unch. · avg -0.2% · Bytecode: 11 unch. · avg -1.6%
objects.js — Interp: 7 unch. · avg -0.4% · Bytecode: 🟢 1, 6 unch. · avg +2.5%
promises.js — Interp: 12 unch. · avg +0.0% · Bytecode: 🟢 1, 🔴 2, 9 unch. · avg -2.5%
strings.js — Interp: 🟢 1, 10 unch. · avg -0.6% · Bytecode: 🔴 1, 10 unch. · avg -2.8%
typed-arrays.js — Interp: 22 unch. · avg +1.1% · Bytecode: 22 unch. · avg +1.1%
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: 9
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.BooleanObjectValue.pas (1)
59-78:⚠️ Potential issue | 🟠 MajorPin the shared Boolean prototype before registering members.
FSharedBooleanPrototypeandFPrototypeMethodHostare only held in class vars until the finalPinObjectcalls. If GC runs during member collection/registration, both objects are still collectible and the registered callbacks can end up bound to a freed host.Suggested fix
FSharedBooleanPrototype := TGocciaObjectValue.Create; FPrototypeMethodHost := Self; + if Assigned(TGarbageCollector.Instance) then + begin + TGarbageCollector.Instance.PinObject(FSharedBooleanPrototype); + TGarbageCollector.Instance.PinObject(FPrototypeMethodHost); + end; if Length(FPrototypeMembers) = 0 then begin Members := TGocciaMemberCollection.Create; try Members.AddMethod(BooleanValueOf, 0); @@ end; RegisterMemberDefinitions(FSharedBooleanPrototype, FPrototypeMembers); - - if Assigned(TGarbageCollector.Instance) then - begin - TGarbageCollector.Instance.PinObject(FSharedBooleanPrototype); - TGarbageCollector.Instance.PinObject(FPrototypeMethodHost); - end; end;As per coding guidelines, "Values held only by Pascal code (not in any GocciaScript scope) must be protected with
AddTempRoot/RemoveTempRootfor the duration they are needed" and "shared prototype singletons must be pinned in theirInitializePrototypemethod".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.BooleanObjectValue.pas` around lines 59 - 78, Pin the shared prototype and method host before you build/register members to prevent GC from collecting them during member collection: in the InitializePrototype flow for FSharedBooleanPrototype/FPrototypeMethodHost, call the GC pin (e.g. TGarbageCollector.Instance.PinObject or AddTempRoot) immediately after creating FSharedBooleanPrototype and setting FPrototypeMethodHost, then perform Members.AddMethod(...) and RegisterMemberDefinitions(...), and finally unpin/remove the temp root only if you used AddTempRoot/RemoveTempRoot; ensure the PinObject calls happen before any Members.AddMethod or RegisterMemberDefinitions calls so callbacks cannot be bound to a freed host.
🧹 Nitpick comments (8)
units/Goccia.Values.TemporalPlainTime.pas (1)
156-161: Use property-name constants for the new accessors.The new registrations reintroduce raw property strings (
'hour','minute', etc.) even though this unit already importsGoccia.Constants.PropertyNames. Please register these via the split constants instead of literals.As per coding guidelines, "Use constant names from split constant units instead of hardcoded string literals:
Goccia.Constants.PropertyNamesfor property names".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalPlainTime.pas` around lines 156 - 161, Replace the hardcoded property name strings used when registering accessors with the appropriate constants from Goccia.Constants.PropertyNames: update the Members.AddAccessor calls that reference 'hour','minute','second','millisecond','microsecond','nanosecond' (the lines registering GetHour, GetMinute, GetSecond, GetMillisecond, GetMicrosecond, GetNanosecond) to use the corresponding PropertyNames constants instead of string literals so the unit follows the split-constants guideline.units/Goccia.Values.FunctionBase.pas (1)
156-158: Use property constants for the Function prototype registrations.The new member definitions hardcode
'call','apply', and'bind'instead of going throughGoccia.Constants.PropertyNames. Keeping these names centralized matters here because they are looked up across multiple runtime paths.As per coding guidelines, "Use constant names from split constant units instead of hardcoded string literals:
Goccia.Constants.PropertyNamesfor property names".🤖 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 156 - 158, Replace the hardcoded property name strings when registering Function prototype members with the central constants from Goccia.Constants.PropertyNames: use the PropertyNames.Call, PropertyNames.Apply, and PropertyNames.Bind constants instead of 'call', 'apply', 'bind' in the Members[...] assignments that call DefineNamedMethod (the registrations using DefineNamedMethod with handlers FunctionCall, FunctionApply, FunctionBind) so look up and use those constant symbols rather than string literals.units/Goccia.Builtins.GlobalString.pas (1)
17-18: MakeFStaticMembersactually static, or keep it local.The new class var reads like a cache, but Lines 40-44 overwrite it on every
TGocciaGlobalStringconstruction. Either guard the build withLength(FStaticMembers) = 0or drop the class var so the lifetime/ownership is explicit.Also applies to: 40-48
🤖 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 17 - 18, FStaticMembers is declared as a class var on TGocciaGlobalString but is being rebuilt on every construction; either make it a true shared cache by populating it only once (guard the population with Length(FStaticMembers)=0 inside the constructor/initializer that currently sets it — e.g., the TGocciaGlobalString constructor block that fills members), or remove the class var and use a local/instance variable so lifetime/ownership is explicit; update the code paths that currently write FStaticMembers (the block around lines 40–48) to follow one of these two approaches.units/Goccia.Values.StringObjectValue.pas (1)
183-184: Use shared property-name constants for these explicit method names.
replaceandreplaceAllare now hardcoded in a table that otherwise comes from shared runtime metadata. Please route them throughGoccia.Constants.PropertyNameshere (adding constants if needed) so this registration block stays aligned with the rest of the runtime.
As per coding guidelines,Use constant names from split constant units instead of hardcoded string literals: Goccia.Constants.PropertyNames for property names...🤖 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 183 - 184, Replace the hardcoded 'replace' and 'replaceAll' literal strings passed to Members.AddNamedMethod with shared constants from Goccia.Constants.PropertyNames (add two constants there if missing, e.g., Replace and ReplaceAll or similarly named identifiers used throughout the runtime). Update the calls Members.AddNamedMethod('replace', StringReplaceMethod, ...) and Members.AddNamedMethod('replaceAll', StringReplaceAllMethod, ...) to use Goccia.Constants.PropertyNames.<YourReplaceConstant> and Goccia.Constants.PropertyNames.<YourReplaceAllConstant> so the registration uses the shared property-name constants.units/Goccia.Values.TemporalPlainDateTime.pas (1)
192-224: Replace the new member-name literals with shared constants.This block adds a large batch of raw runtime names (
'calendarId','monthCode','toJSON', etc.) even though the unit already importsGoccia.Constants.PropertyNames. Please move these names into the constant units before the same literals start diverging across other Temporal types.As per coding guidelines, "Use constant names from split constant units instead of hardcoded string literals:
Goccia.Constants.PropertyNamesfor property names,Goccia.Constants.TypeNamesfor type names,Goccia.Constants.ErrorNamesfor error names,Goccia.Constants.ConstructorNamesfor constructor names,Goccia.Constants.SymbolNamesfor symbol names, andGoccia.Constantsfor literal value strings".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TemporalPlainDateTime.pas` around lines 192 - 224, The large block in the constructor/register code uses raw string literals for runtime property names in calls like Members.AddAccessor('calendarId', GetCalendarId, ...) and should use the shared constants from Goccia.Constants.PropertyNames instead; replace each literal ('calendarId','year','month','monthCode','day',...,'nanosecond') with the corresponding PropertyNames.* constant (e.g. PropertyNames.CalendarId, PropertyNames.Year, PropertyNames.MonthCode, etc.), keeping the same accessor methods (GetCalendarId, GetYear, GetMonthCode, GetNanosecond) and existing flags, and ensure any property-like names used elsewhere in this unit follow the same pattern so names stay consistent across Temporal types.units/Goccia.Values.IteratorValue.pas (1)
147-147: Use the existing property-name constants here.
PROP_NEXTandPROP_FROMare already imported and used later in this unit. Hardcoding those names in the new registration path makes this file drift from the repo’s constant-name convention.♻️ Suggested cleanup
- Members.AddNamedMethod('next', IteratorNext, 0, gmkPrototypeMethod, [gmfNoFunctionPrototype]); + Members.AddNamedMethod(PROP_NEXT, IteratorNext, 0, gmkPrototypeMethod, [gmfNoFunctionPrototype]); - Members.AddNamedMethod('from', FPrototypeMethodHost.IteratorFrom, 1, gmkStaticMethod, [gmfNoFunctionPrototype]); + Members.AddNamedMethod(PROP_FROM, FPrototypeMethodHost.IteratorFrom, 1, gmkStaticMethod, [gmfNoFunctionPrototype]);As per coding guidelines, "Use constant names from split constant units instead of hardcoded string literals:
Goccia.Constants.PropertyNamesfor property names,Goccia.Constants.TypeNamesfor type names,Goccia.Constants.ErrorNamesfor error names,Goccia.Constants.ConstructorNamesfor constructor names,Goccia.Constants.SymbolNamesfor symbol names, andGoccia.Constantsfor literal value strings."Also applies to: 187-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.IteratorValue.pas` at line 147, Replace the hardcoded property-name string literals in the iterator registration with the shared property-name constants: change the AddNamedMethod call that uses 'next' (Members.AddNamedMethod('next', IteratorNext, ...)) to use PROP_NEXT, and likewise replace any other hardcoded property names like 'from' with PROP_FROM (these constants are already imported in this unit), ensuring the AddNamedMethod/registration paths use PROP_NEXT and PROP_FROM instead of string literals.units/Goccia.ObjectModel.pas (2)
157-158: Consider adding a named type alias for the global list.Per coding guidelines,
TObjectList<T>should use named type aliases. This could be:type TGocciaPublishedGetterHostList = TObjectList<TGocciaPublishedGetterHost>; var GPublishedGetterHosts: TGocciaPublishedGetterHostList;This improves type safety and clarity, though the current implementation works correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.ObjectModel.pas` around lines 157 - 158, Introduce a named type alias for the global list instead of declaring GPublishedGetterHosts as TObjectList<TObject>; add a type like TGocciaPublishedGetterHostList = TObjectList<TGocciaPublishedGetterHost> and change the declaration of GPublishedGetterHosts to use TGocciaPublishedGetterHostList so the global variable references the concrete element type (refer to GPublishedGetterHosts and the use of TObjectList<TObject> to find where to update). Ensure the new type is declared in the same unit scope before the var section and update any code that constructs or frees GPublishedGetterHosts to use the new alias.
624-629: Consider validatingDef.Symbolis aTGocciaSymbolValueinValidateMemberDefinition.The validation checks that
Symbolis assigned but doesn't verify it's actually aTGocciaSymbolValue. If a caller mistakenly passes a non-symbol value, the unchecked cast on line 672 (TGocciaSymbolValue(Def.Symbol)) would produce incorrect behavior.🛡️ Defensive validation
gmkSymbolMethod, gmkSymbolProperty, gmkSymbolAccessor: + begin if not Assigned(ADefinition.Symbol) then raise EGocciaObjectModelError.CreateFmt( 'Invalid %s "%s": symbol members require a symbol value', [KindToString(ADefinition.Kind), ADefinition.ExposedName]); + if not (ADefinition.Symbol is TGocciaSymbolValue) then + raise EGocciaObjectModelError.CreateFmt( + 'Invalid %s "%s": symbol members require a TGocciaSymbolValue, got %s', + [KindToString(ADefinition.Kind), ADefinition.ExposedName, + ADefinition.Symbol.ClassName]); + end;Also applies to: 671-676
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.ObjectModel.pas` around lines 624 - 629, In ValidateMemberDefinition, the code only checks Assigned(ADefinition.Symbol) but not its runtime type; add a check that ADefinition.Symbol is actually a TGocciaSymbolValue (e.g. using the "is" operator) and raise EGocciaObjectModelError with a clear message if not, so the later cast TGocciaSymbolValue(Def.Symbol) is safe; reference ValidateMemberDefinition, ADefinition.Symbol and TGocciaSymbolValue when making this 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/adding-built-in-types.md`:
- Around line 105-133: Replace the deprecated early-exit guard in
TGocciaYourValue.InitializePrototype by inverting the condition: instead of "if
Assigned(FShared) then Exit" use "if not Assigned(FShared) then" to wrap the
prototype creation and registration logic (including the FShared :=
TGocciaSharedPrototype.Create(Self), the FPrototypeMembers initialization block,
and RegisterMemberDefinitions). Ensure you still check and populate
FPrototypeMembers only when its Length = 0 and preserve the Members try/finally
logic and the gmfNoFunctionPrototype flag on FPrototypeMembers[0]; remove the
bare early return so the singleton prototype is created exactly once.
In `@units/Goccia.Builtins.GlobalNumber.pas`:
- Around line 60-65: The Number.* methods are registered with incorrect .length
values; update the Members.AddMethod calls so NumberParseInt is registered with
length 2 (not 1) and the NumberIsFinite, NumberIsNaN, NumberIsInteger,
NumberIsSafeInteger methods are registered with length 1 (not 0). Locate the
Members.AddMethod invocations for NumberParseInt, NumberParseFloat,
NumberIsFinite, NumberIsNaN, NumberIsInteger, and NumberIsSafeInteger and change
their arity arguments to reflect the actual signatures (NumberParseInt -> 2;
NumberParseFloat stays 1; NumberIs* helpers -> 1). Ensure only the numeric arity
parameter (the middle integer) is updated so .length exposes the correct values.
In `@units/Goccia.Builtins.GlobalObject.pas`:
- Around line 79-80: The Object.create and Object.hasOwn methods are registered
with incorrect arity; update the Members.AddMethod calls that register
ObjectCreate and ObjectHasOwn to use length 2 instead of 1 so Function.length
metadata matches the ECMAScript spec. Locate the Members.AddMethod calls that
reference ObjectCreate and ObjectHasOwn and change their second argument from 1
to 2 (keeping gmkStaticMethod unchanged); this will align with ObjectCreate's
optional Properties parameter and ObjectHasOwn's RequireExactly(AArgs, 2, ...)
requirement.
In `@units/Goccia.ObjectModel.Engine.pas`:
- Around line 55-79: The loop currently copies static values via
ATypeDefinition.StaticSource.GetProperty into AConstructor.SetProperty which
flattens accessors/flags; instead, iterate
ATypeDefinition.StaticSource.GetAllPropertyNames and for each Key fetch the full
property descriptor (e.g. StaticSource.GetPropertyDescriptor or equivalent) and
define that descriptor on the constructor (e.g. AConstructor.DefineProperty /
AssignPropertyDescriptor) so getters/setters and attributes
(writable/enumerable/configurable) are preserved; update the code paths around
AConstructor.SetProperty and ATypeDefinition.StaticSource.GetProperty to use the
descriptor-based API to copy descriptors intact.
In `@units/Goccia.ObjectModel.pas`:
- Around line 433-448: Replace the Pascal exception raised for runtime type
mismatches in Invoke with a JS-catchable error: add Goccia.Values.ErrorHelper to
the implementation uses clause and replace the raise
EGocciaObjectModelError.CreateFmt(...) that checks (AThisValue is FTargetClass)
with a call to ThrowTypeError (include the same formatted message), preserving
assignment to TargetObject when valid; leave the property-not-found raise for
GetPropInfo untouched since it represents engine misconfiguration.
In `@units/Goccia.Values.ArrayValue.pas`:
- Around line 260-263: Replace the accessor registration for PROP_LENGTH with a
data property on the shared array prototype: stop calling
Members.AddAccessor(PROP_LENGTH, GetLengthProperty, ...) and instead define
PROP_LENGTH as a data property with numeric value 0 and attributes
writable=true, enumerable=false, configurable=false (use the equivalent
Members.AddDataProperty / Members.AddProperty method available in
TGocciaMemberCollection). Remove reliance on GetLengthProperty for the prototype
(it casts the receiver to TGocciaArrayValue and will fail for
FSharedArrayPrototype which is a TGocciaObjectValue); keep GetLengthProperty for
actual array instances only. Leave ArrayMap registration unchanged.
- Around line 284-285: The registered arities for the Array prototype methods
are wrong: change the Members.AddMethod calls that register ArrayToSorted and
ArrayToSpliced so their length metadata match the spec — call
Members.AddMethod(ArrayToSorted, 1, gmkPrototypeMethod,
[gmfNoFunctionPrototype]) and Members.AddMethod(ArrayToSpliced, 2,
gmkPrototypeMethod, [gmfNoFunctionPrototype]) so Array.prototype.toSorted.length
=== 1 and Array.prototype.toSpliced.length === 2.
In `@units/Goccia.Values.EnumValue.pas`:
- Around line 68-74: The TGocciaStringLiteralValue created for
Symbol.toStringTag (via TGocciaStringLiteralValue.Create(AEnum.Name)) is only
temporarily held in the local Members array before
RegisterMemberDefinitions(AEnum, Members) and can be collected; protect it by
calling AddTempRoot on the created string value immediately after creation and
call RemoveTempRoot after RegisterMemberDefinitions returns (or alternatively
call DefineSymbolDataProperty on AEnum immediately after creating the string
literal so it becomes reachable without relying on Pascal-only references).
Ensure you reference the created value passed to DefineSymbolDataProperty and
use AddTempRoot/RemoveTempRoot around the registration window to satisfy the GC
rooting guideline.
---
Outside diff comments:
In `@units/Goccia.Values.BooleanObjectValue.pas`:
- Around line 59-78: Pin the shared prototype and method host before you
build/register members to prevent GC from collecting them during member
collection: in the InitializePrototype flow for
FSharedBooleanPrototype/FPrototypeMethodHost, call the GC pin (e.g.
TGarbageCollector.Instance.PinObject or AddTempRoot) immediately after creating
FSharedBooleanPrototype and setting FPrototypeMethodHost, then perform
Members.AddMethod(...) and RegisterMemberDefinitions(...), and finally
unpin/remove the temp root only if you used AddTempRoot/RemoveTempRoot; ensure
the PinObject calls happen before any Members.AddMethod or
RegisterMemberDefinitions calls so callbacks cannot be bound to a freed host.
---
Nitpick comments:
In `@units/Goccia.Builtins.GlobalString.pas`:
- Around line 17-18: FStaticMembers is declared as a class var on
TGocciaGlobalString but is being rebuilt on every construction; either make it a
true shared cache by populating it only once (guard the population with
Length(FStaticMembers)=0 inside the constructor/initializer that currently sets
it — e.g., the TGocciaGlobalString constructor block that fills members), or
remove the class var and use a local/instance variable so lifetime/ownership is
explicit; update the code paths that currently write FStaticMembers (the block
around lines 40–48) to follow one of these two approaches.
In `@units/Goccia.ObjectModel.pas`:
- Around line 157-158: Introduce a named type alias for the global list instead
of declaring GPublishedGetterHosts as TObjectList<TObject>; add a type like
TGocciaPublishedGetterHostList = TObjectList<TGocciaPublishedGetterHost> and
change the declaration of GPublishedGetterHosts to use
TGocciaPublishedGetterHostList so the global variable references the concrete
element type (refer to GPublishedGetterHosts and the use of TObjectList<TObject>
to find where to update). Ensure the new type is declared in the same unit scope
before the var section and update any code that constructs or frees
GPublishedGetterHosts to use the new alias.
- Around line 624-629: In ValidateMemberDefinition, the code only checks
Assigned(ADefinition.Symbol) but not its runtime type; add a check that
ADefinition.Symbol is actually a TGocciaSymbolValue (e.g. using the "is"
operator) and raise EGocciaObjectModelError with a clear message if not, so the
later cast TGocciaSymbolValue(Def.Symbol) is safe; reference
ValidateMemberDefinition, ADefinition.Symbol and TGocciaSymbolValue when making
this change.
In `@units/Goccia.Values.FunctionBase.pas`:
- Around line 156-158: Replace the hardcoded property name strings when
registering Function prototype members with the central constants from
Goccia.Constants.PropertyNames: use the PropertyNames.Call, PropertyNames.Apply,
and PropertyNames.Bind constants instead of 'call', 'apply', 'bind' in the
Members[...] assignments that call DefineNamedMethod (the registrations using
DefineNamedMethod with handlers FunctionCall, FunctionApply, FunctionBind) so
look up and use those constant symbols rather than string literals.
In `@units/Goccia.Values.IteratorValue.pas`:
- Line 147: Replace the hardcoded property-name string literals in the iterator
registration with the shared property-name constants: change the AddNamedMethod
call that uses 'next' (Members.AddNamedMethod('next', IteratorNext, ...)) to use
PROP_NEXT, and likewise replace any other hardcoded property names like 'from'
with PROP_FROM (these constants are already imported in this unit), ensuring the
AddNamedMethod/registration paths use PROP_NEXT and PROP_FROM instead of string
literals.
In `@units/Goccia.Values.StringObjectValue.pas`:
- Around line 183-184: Replace the hardcoded 'replace' and 'replaceAll' literal
strings passed to Members.AddNamedMethod with shared constants from
Goccia.Constants.PropertyNames (add two constants there if missing, e.g.,
Replace and ReplaceAll or similarly named identifiers used throughout the
runtime). Update the calls Members.AddNamedMethod('replace',
StringReplaceMethod, ...) and Members.AddNamedMethod('replaceAll',
StringReplaceAllMethod, ...) to use
Goccia.Constants.PropertyNames.<YourReplaceConstant> and
Goccia.Constants.PropertyNames.<YourReplaceAllConstant> so the registration uses
the shared property-name constants.
In `@units/Goccia.Values.TemporalPlainDateTime.pas`:
- Around line 192-224: The large block in the constructor/register code uses raw
string literals for runtime property names in calls like
Members.AddAccessor('calendarId', GetCalendarId, ...) and should use the shared
constants from Goccia.Constants.PropertyNames instead; replace each literal
('calendarId','year','month','monthCode','day',...,'nanosecond') with the
corresponding PropertyNames.* constant (e.g. PropertyNames.CalendarId,
PropertyNames.Year, PropertyNames.MonthCode, etc.), keeping the same accessor
methods (GetCalendarId, GetYear, GetMonthCode, GetNanosecond) and existing
flags, and ensure any property-like names used elsewhere in this unit follow the
same pattern so names stay consistent across Temporal types.
In `@units/Goccia.Values.TemporalPlainTime.pas`:
- Around line 156-161: Replace the hardcoded property name strings used when
registering accessors with the appropriate constants from
Goccia.Constants.PropertyNames: update the Members.AddAccessor calls that
reference 'hour','minute','second','millisecond','microsecond','nanosecond' (the
lines registering GetHour, GetMinute, GetSecond, GetMillisecond, GetMicrosecond,
GetNanosecond) to use the corresponding PropertyNames constants instead of
string literals so the unit follows the split-constants guideline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 609c25f5-dbe5-4433-9ffe-4210cf4b67fc
📒 Files selected for processing (42)
AGENTS.mddocs/adding-built-in-types.mddocs/architecture.mddocs/code-style.mdunits/Goccia.Builtins.Console.pasunits/Goccia.Builtins.GlobalArray.pasunits/Goccia.Builtins.GlobalArrayBuffer.pasunits/Goccia.Builtins.GlobalNumber.pasunits/Goccia.Builtins.GlobalObject.pasunits/Goccia.Builtins.GlobalPromise.pasunits/Goccia.Builtins.GlobalString.pasunits/Goccia.Builtins.GlobalSymbol.pasunits/Goccia.Builtins.Globals.pasunits/Goccia.Builtins.JSON.pasunits/Goccia.Builtins.Math.pasunits/Goccia.Builtins.Performance.pasunits/Goccia.Builtins.Temporal.pasunits/Goccia.Engine.pasunits/Goccia.ObjectModel.Engine.pasunits/Goccia.ObjectModel.Types.pasunits/Goccia.ObjectModel.pasunits/Goccia.Values.ArrayBufferValue.pasunits/Goccia.Values.ArrayValue.pasunits/Goccia.Values.BooleanObjectValue.pasunits/Goccia.Values.EnumValue.pasunits/Goccia.Values.FunctionBase.pasunits/Goccia.Values.IteratorValue.pasunits/Goccia.Values.MapValue.pasunits/Goccia.Values.NumberObjectValue.pasunits/Goccia.Values.ObjectValue.pasunits/Goccia.Values.PromiseValue.pasunits/Goccia.Values.SetValue.pasunits/Goccia.Values.SharedArrayBufferValue.pasunits/Goccia.Values.StringObjectValue.pasunits/Goccia.Values.SymbolValue.pasunits/Goccia.Values.TemporalDuration.pasunits/Goccia.Values.TemporalInstant.pasunits/Goccia.Values.TemporalPlainDate.pasunits/Goccia.Values.TemporalPlainDateTime.pasunits/Goccia.Values.TemporalPlainTime.pasunits/Goccia.Values.TypedArrayValue.pasunits/Goccia.inc
👮 Files not reviewed due to content moderation or server errors (7)
- units/Goccia.inc
- docs/architecture.md
- units/Goccia.Builtins.Temporal.pas
- docs/code-style.md
- AGENTS.md
- units/Goccia.Values.SharedArrayBufferValue.pas
- units/Goccia.Values.NumberObjectValue.pas
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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)
385-393:⚠️ Potential issue | 🟠 Major
ThrowErrormethods bypassErrorHelperand create uncatchable errors.These private helper methods use
TGocciaError.Createdirectly instead of theGoccia.Values.ErrorHelperfunctions. Per coding guidelines, all runtime errors must useThrowTypeError,ThrowRangeError, etc., to ensure errors are proper JS Error objects with correct prototype chains, catchable from JavaScripttry...catch.The current implementation creates errors that may not be catchable from JavaScript code.
Suggested refactor
Replace calls to
ThrowError(...)throughout the file with the appropriateThrowTypeError(...)calls fromGoccia.Values.ErrorHelper, then remove these private methods:-procedure TGocciaArrayValue.ThrowError(const AMessage: string; const AArgs: array of const); -begin - raise TGocciaError.Create(Format(AMessage, AArgs), 0, 0, '', nil); -end; - -procedure TGocciaArrayValue.ThrowError(const AMessage: string); -begin - ThrowError(AMessage, []); -end;Then update call sites like:
- ThrowError('Array.%s called on non-array', [AMethodName]); + ThrowTypeError(Format('Array.%s called on non-array', [AMethodName]));As per coding guidelines: "Always call error construction through
Goccia.Values.ErrorHelperfunctions to ensure proper error type prototype chain and catchability from JavaScript."🤖 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 385 - 393, The ThrowError overloads in TGocciaArrayValue create TGocciaError directly and must be replaced by calls to the ErrorHelper functions (e.g., ThrowTypeError, ThrowRangeError) so JS errors are proper and catchable; update all call sites that use TGocciaArrayValue.ThrowError to call the appropriate Goccia.Values.ErrorHelper function (choose ThrowTypeError/ThrowRangeError based on context) and then remove the private ThrowError methods from TGocciaArrayValue; ensure Format(AMessage, AArgs) is passed into the chosen ErrorHelper so the message formatting is preserved.
🧹 Nitpick comments (1)
units/Goccia.Values.ArrayValue.pas (1)
98-98:GetLengthPropertyappears to be dead code.This method was previously used as the accessor getter for
Array.prototype.length, but now thatlengthis registered as a data property viaAddDataProperty(line 262), this method is no longer referenced. Consider removing it.🤖 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 98, Remove the now-unused GetLengthProperty function as it is dead code: locate the declaration/implementation of function GetLengthProperty(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue and delete it (and any related forward declaration) because length is registered as a data property via AddDataProperty and the getter is no longer referenced; ensure no other code references GetLengthProperty before removing, and run a build to confirm no unresolved symbols remain.
🤖 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 289: The Array.prototype.unshift method is registered with length -1
(variadic convention) but per spec its length must be 1; update the
Members.AddMethod call that registers ArrayUnshift so its length argument is 1
(i.e., replace the -1 with 1) while keeping the same flags (gmkPrototypeMethod
and [gmfNoFunctionPrototype]) so ArrayUnshift.length reports 1 as required by
ECMAScript §23.1.3.37.
---
Outside diff comments:
In `@units/Goccia.Values.ArrayValue.pas`:
- Around line 385-393: The ThrowError overloads in TGocciaArrayValue create
TGocciaError directly and must be replaced by calls to the ErrorHelper functions
(e.g., ThrowTypeError, ThrowRangeError) so JS errors are proper and catchable;
update all call sites that use TGocciaArrayValue.ThrowError to call the
appropriate Goccia.Values.ErrorHelper function (choose
ThrowTypeError/ThrowRangeError based on context) and then remove the private
ThrowError methods from TGocciaArrayValue; ensure Format(AMessage, AArgs) is
passed into the chosen ErrorHelper so the message formatting is preserved.
---
Nitpick comments:
In `@units/Goccia.Values.ArrayValue.pas`:
- Line 98: Remove the now-unused GetLengthProperty function as it is dead code:
locate the declaration/implementation of function GetLengthProperty(const AArgs:
TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue and
delete it (and any related forward declaration) because length is registered as
a data property via AddDataProperty and the getter is no longer referenced;
ensure no other code references GetLengthProperty before removing, and run a
build to confirm no unresolved symbols remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8321010-d79d-49a8-8b72-3edab2146d0e
📒 Files selected for processing (16)
tests/built-ins/Array/constructor.jstests/built-ins/Array/prototype/toSorted.jstests/built-ins/Array/prototype/toSpliced.jstests/built-ins/Number/isFinite.jstests/built-ins/Number/isInteger.jstests/built-ins/Number/isNaN.jstests/built-ins/Number/isSafeInteger.jstests/built-ins/Number/parseInt.jstests/built-ins/Object/create.jstests/built-ins/Object/hasOwn.jsunits/Goccia.Builtins.GlobalNumber.pasunits/Goccia.Builtins.GlobalObject.pasunits/Goccia.ObjectModel.Engine.pasunits/Goccia.ObjectModel.pasunits/Goccia.Values.ArrayValue.pasunits/Goccia.Values.EnumValue.pas
✅ Files skipped from review due to trivial changes (10)
- tests/built-ins/Number/parseInt.js
- tests/built-ins/Array/constructor.js
- tests/built-ins/Array/prototype/toSorted.js
- tests/built-ins/Object/create.js
- tests/built-ins/Number/isFinite.js
- tests/built-ins/Object/hasOwn.js
- tests/built-ins/Number/isSafeInteger.js
- tests/built-ins/Array/prototype/toSpliced.js
- tests/built-ins/Number/isInteger.js
- tests/built-ins/Number/isNaN.js
🚧 Files skipped from review as they are similar to previous changes (4)
- units/Goccia.Builtins.GlobalNumber.pas
- units/Goccia.ObjectModel.Engine.pas
- units/Goccia.Builtins.GlobalObject.pas
- units/Goccia.ObjectModel.pas
This was referenced Apr 4, 2026
Merged
This was referenced Apr 10, 2026
This was referenced Apr 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Summary by CodeRabbit
Refactor
Documentation
Tests