Add decorators#35
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds comprehensive TC39 Stage 3 decorators support to the Goccia JavaScript evaluator, including parsing, AST representation, decorator evaluation phases, metadata handling, auto-accessor implementation, and extensive test coverage across lexer, parser, evaluator, and runtime components. Changes
Sequence DiagramsequenceDiagram
participant Lexer
participant Parser
participant AST as AST/Evaluator
participant DecEval as Decorator<br/>Evaluator
participant DecHlpr as Decorator<br/>Helpers
participant ClassVal as ClassValue
Lexer->>Parser: Tokenize @ and decorators
Parser->>Parser: ParseDecorators()
Parser->>AST: Create class with<br/>FDecorators list
AST->>DecEval: EvaluateClassDefinition<br/>with decorators
DecEval->>DecEval: Phase 1: Element<br/>decorators
DecEval->>DecHlpr: Create context for<br/>each element
DecHlpr->>DecEval: Call decorator<br/>with element & context
DecEval->>DecEval: Handle return values<br/>(replace/wrap/init)
DecEval->>DecEval: Phase 2: Initializers<br/>(methods/fields)
DecEval->>DecHlpr: Collect addInitializer<br/>callbacks
DecEval->>DecEval: Phase 3: Class<br/>decorator
DecEval->>DecEval: Call class decorator<br/>with context
DecEval->>DecEval: Handle class<br/>transformation
DecEval->>ClassVal: Store initializers &<br/>metadata
ClassVal->>ClassVal: Attach Symbol.metadata
Note over Lexer,ClassVal: During Instance Construction
ClassVal->>ClassVal: RunMethodInitializers()
ClassVal->>ClassVal: RunFieldInitializers()
ClassVal->>ClassVal: RunDecoratorFieldInitializers()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
Benchmark Results203 benchmarks · 🟢 9 improved · 🔴 64 regressed · 🆕 24 new · 106 unchanged · avg -3.5% arrays.js — 19 unchanged · avg -0.9%
classes.js — 🔴 5 regressed, 🆕 16 new, 10 unchanged · avg -2.9%
closures.js — 🔴 11 regressed · avg -12.3%
collections.js — 🔴 7 regressed, 5 unchanged · avg -8.6%
destructuring.js — 🔴 13 regressed, 🆕 8 new, 1 unchanged · avg -10.3%
fibonacci.js — 🔴 4 regressed, 2 unchanged · avg -8.9%
iterators.js — 🔴 18 regressed, 2 unchanged · avg -11.9%
json.js — 🔴 6 regressed, 14 unchanged · avg -4.1%
jsx.jsx — 21 unchanged · avg +1.8%
numbers.js — 🟢 1 improved, 10 unchanged · avg +2.8%
objects.js — 7 unchanged · avg -0.6%
promises.js — 🟢 5 improved, 7 unchanged · avg +6.2%
strings.js — 🟢 3 improved, 8 unchanged · avg +5.4%
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: 13
🧹 Nitpick comments (4)
tests/language/decorators/decorator-errors.js (1)
7-18: Consider using thetoThrow()matcher for cleaner error testing.The test framework supports a
toThrow()matcher that would simplify this test and provide better error messages on failure.♻️ Suggested refactor using toThrow()
- test("non-callable decorator throws", () => { - let threw = false; - try { - const notAFunction = 42; - - `@notAFunction` - class C {} - } catch (e) { - threw = true; - } - expect(threw).toBe(true); + test("non-callable decorator throws", () => { + expect(() => { + const notAFunction = 42; + + `@notAFunction` + class C {} + }).toThrow(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/language/decorators/decorator-errors.js` around lines 7 - 18, Replace the manual try/catch/flag pattern in the "non-callable decorator throws" test with Jest's toThrow matcher: wrap the class declaration that uses the non-callable decorator (the const notAFunction = 42 and the decorated class C) inside a function passed to expect, and assert .toThrow() on it (e.g., expect(() => { const notAFunction = 42; `@notAFunction` class C {} }).toThrow()); remove the threw variable and try/catch block.tests/language/decorators/decorator-order.js (1)
7-21: Rename test to match bottom-up application order.The assertion checks bottom-up application, so the description should reflect that.
♻️ Suggested rename
- test("decorators evaluated top-to-bottom", () => { + test("decorators applied bottom-up", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/language/decorators/decorator-order.js` around lines 7 - 21, Rename the test description string in the test(...) declaration so it accurately describes bottom-up application order (e.g., change "decorators evaluated top-to-bottom" to "decorators applied bottom-to-top"), leaving the test body, the order array and class C unchanged; update the test(...) call's first argument near the class C definition and the order/assertions to reflect the new descriptive text.tests/language/decorators/decorator-metadata.js (1)
1-60: Consider adding metadata inheritance edge-case coverage.The header mentions prototype-chain inheritance; a small test asserting subclass metadata inherits from the base class would round out edge-case coverage.
As per coding guidelines: “When implementing a new language feature, create JavaScript test files following existing patterns in
tests/. Tests should cover happy paths, edge cases, and error cases. Verify all tests pass before committing by running:./build.pas testrunner && ./build/TestRunner tests”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/language/decorators/decorator-metadata.js` around lines 1 - 60, Add a new test case that covers the prototype-chain inheritance behavior by creating a base class with a class-level decorator (e.g., reuse the existing track/first decorator pattern that writes to context.metadata) and then declare a subclass that extends the base; assert that the subclass’s Symbol.metadata either contains the inherited metadata entries from the base (or that the subclass metadata object is linked appropriately per spec), and also verify that modifying subclass metadata does not unexpectedly mutate the base metadata object; reference the existing test patterns using class C, context.metadata and Symbol.metadata to match style and placement in the same file.units/Goccia.Evaluator.Decorators.pas (1)
45-53: Consider pre-allocating or batching array growth.Calling
SetLengthfor each added initializer results in O(n²) reallocation cost. For classes with many decorated elements, this may impact performance.♻️ Alternative using capacity doubling
+private + FInitializers: array of TGocciaValue; + FCount: Integer; +public + constructor Create; + ... + +constructor TGocciaInitializerCollector.Create; +begin + FCount := 0; + SetLength(FInitializers, 4); // Initial capacity +end; + function TGocciaInitializerCollector.AddInitializer(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; begin if (AArgs.Length > 0) and AArgs.GetElement(0).IsCallable then begin - SetLength(FInitializers, Length(FInitializers) + 1); - FInitializers[High(FInitializers)] := AArgs.GetElement(0); + if FCount >= Length(FInitializers) then + SetLength(FInitializers, Length(FInitializers) * 2); + FInitializers[FCount] := AArgs.GetElement(0); + Inc(FCount); end; Result := TGocciaUndefinedLiteralValue.UndefinedValue; end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Evaluator.Decorators.pas` around lines 45 - 53, The AddInitializer method grows FInitializers by one each call causing quadratic reallocations; modify TGocciaInitializerCollector to track count and capacity (e.g., FInitializersCount and FInitializersCapacity) and change AddInitializer to check capacity before adding: when capacity is zero or full, grow capacity (e.g., double it or add a fixed chunk) with a single SetLength(FInitializers, FInitializersCapacity), then append the new initializer to FInitializers[FInitializersCount] and increment FInitializersCount; keep Result logic the same. Ensure any existing code that reads FInitializers uses the actual count (FInitializersCount) or resizes the array down when finalizing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/language/decorators/basic-field-decorator.js`:
- Around line 6-56: Add explicit edge/error-case tests to
tests/language/decorators/basic-field-decorator.js: add test cases that assert
(1) a decorator that returns a non-callable value throws a TypeError when
applied to a field (reference the existing noop/doubleInit patterns and class
C), (2) a decorator that throws synchronously causes class definition or
instantiation to propagate the error (use a decorator function that throws and
expect the error), and (3) invalid context usages (e.g., asserting
context.kind/name/static/private values for static/private fields) by defining
classes with static and private fields and verifying the decorator’s received
context; follow the existing test style (describe/test/expect) and ensure each
new test uses expect(...).toThrow or expect(...).toBe assertions consistent with
surrounding tests.
In `@tests/language/decorators/basic-method-decorator.js`:
- Around line 30-47: The decorator `double` currently returns an arrow function
which lexically binds `this`, causing `method.call(this)` to use the wrong
`this`; update `double` to return a regular function (e.g., function(...args) {
... }) so `this` is dynamically bound at call time, forward received arguments
to the original `method` (use method.apply or call with args), and return the
doubled result; update references in the diff to the `double` function and
ensure `Calculator.prototype.value` wrapping preserves method semantics.
In `@units/Goccia.Evaluator.Decorators.pas`:
- Around line 89-94: TGocciaAccessSetter.SetValue currently calls
AArgs.GetElement(0) without ensuring an argument exists; add a defensive check
(e.g., if AArgs.Length > 0) before calling AArgs.GetElement(0) and only call
TGocciaObjectValue(AThisValue).AssignProperty(FPropertyName,
AArgs.GetElement(0)) when the check passes; if no argument is available, avoid
calling GetElement(0) (either skip the assignment or handle the error/raise as
appropriate for the codebase) so the method no longer accesses out-of-bounds
arguments.
In `@units/Goccia.Evaluator.pas`:
- Around line 1964-2026: Replace all hardcoded decorator context/access string
literals in this block with the constants from Goccia.Constants.PropertyNames
(and add any missing PropertyNames such as kind, name, static, private,
metadata, access, get, set, init, addInitializer); specifically update calls
like ContextObject.AssignProperty('kind'...),
ContextObject.AssignProperty('name'...),
ContextObject.AssignProperty('metadata'...),
AccessObject.AssignProperty('get'...), AccessObject.AssignProperty('set'...),
and the addInitializer TGocciaNativeFunctionValue.CreateWithoutPrototype
invocations (StaticFieldCollector.AddInitializer, FieldCollector.AddInitializer,
MethodCollector.AddInitializer) to use PropertyNames constants instead of string
literals; keep the existing value constructors (TGocciaStringLiteralValue,
TGocciaBooleanLiteralValue, TGocciaObjectValue, TGocciaNativeFunctionValue) and
helper classes (TGocciaAccessGetter, TGocciaAccessSetter) unchanged, and add any
missing entries to Goccia.Constants.PropertyNames for the names referenced
elsewhere (also apply the same replacements in the other mentioned ranges).
- Around line 1898-1916: Add a spec-section annotation for the TC39 decorators
algorithm by inserting a comment in the required format (e.g. "// TC39
Decorators §X.Y.Z MethodName(params)") either immediately above the decorator
handling block shown (the loop that processes cekAccessor and calls
ClassValue.AddAutoAccessor) or on the surrounding routine (e.g.
EvaluateClassDefinition/whatever function contains this block); ensure the
proposal name "TC39 Decorators" is used and include the section number and
method signature per the coding guideline format.
- Around line 1956-2163: The code checks DecoratorFn.IsCallable and reports
errors but then proceeds to cast/use DecoratorFn and DecoratorResult which can
cause invalid-cast exceptions; after each AContext.OnError where a non-callable
decorator or invalid return is detected (e.g., the IsCallable check on
DecoratorFn, the "must return a function or undefined" checks in
cekMethod/cekGetter/cekSetter/cekField, and the "must return an object or
undefined" check in cekAccessor) add an early exit from that decorator handling
(e.g., continue to the next decorator) or raise so you do not perform
casts/Calls on invalid values; specifically guard/use early returns before
calling TGocciaFunctionBase(DecoratorFn).Call and before casting DecoratorResult
to TGocciaFunctionValue, TGocciaMethodValue or TGocciaObjectValue (references:
DecoratorFn, DecoratorArgs, TGocciaFunctionBase.Call, DecoratorResult,
ClassValue.AddPrivateMethod/AddPrivateGetter/AddPrivateSetter,
ClassValue.SetProperty/Prototype.AssignProperty, ClassValue.AddGetter/AddSetter,
ClassValue.AddFieldInitializer, and the accessor handling where DecoratorResult
must be TGocciaObjectValue).
- Around line 1921-2240: The decorator evaluation and application code creates
MetadataObject and collectors (MethodCollector, FieldCollector,
StaticFieldCollector, ClassCollector) but only frees/removes them at the end;
wrap the entire decorator phases (after MetadataObject creation through the
final static initializer loop) in a try..finally so
TGocciaGarbageCollector.Instance.RemoveTempRoot(MetadataObject) and Free on
MethodCollector, FieldCollector, StaticFieldCollector and ClassCollector always
run even if a decorator throws; ensure any temp objects created for each
decorator (e.g., ContextObject, AccessObject, DecoratorArgs) are still freed as
before but that the collectors and MetadataObject cleanup is moved into the
finally block.
- Around line 1898-1913: The initializer is being registered on the public
accessor name (Elem.Name) instead of the backing field, causing the instance
initializer to miss the backing storage used by the getter/setter; change the
call that registers the initializer so AddInstanceProperty is invoked with
AccessorBackingName (not Elem.Name) when Elem.FieldInitializer is assigned,
keeping the AddAutoAccessor(Elem.Name, AccessorBackingName, Elem.IsStatic) call
as-is so the accessor and backing-field initializer align (refer to
AClassDef.FElements loop, Elem.FieldInitializer, AccessorBackingName,
AddInstanceProperty and AddAutoAccessor).
- Around line 2190-2195: The code currently validates DecoratorResult with
IsCallable but then unsafely casts it to TGocciaClassValue
(TGocciaClassValue(DecoratorResult)), which will crash for non-class callables;
change this to accept any callable per TC39 by removing the direct cast and
keeping DecoratorResult as a generic TGocciaValue (or a new local TGocciaValue
variable) and update subsequent logic that used ClassValue to perform runtime
checks (e.g., "if DecoratorResult is TGocciaClassValue then" before calling
class-specific methods) or treat callables uniformly; ensure error reporting via
AContext.OnError remains correct and only reject non-callables (still treat
TGocciaUndefinedLiteralValue as before).
In `@units/Goccia.Values.AutoAccessor.pas`:
- Around line 55-60: The SetValue implementation in TGocciaAutoAccessorSetter
currently uses AArgs.GetElement(0) without verifying there is at least one
argument; update TGocciaAutoAccessorSetter.SetValue to check the argument count
(e.g. AArgs.Length or Count) before calling AArgs.GetElement(0), only calling
TGocciaObjectValue(AThisValue).AssignProperty(FBackingName, AArgs.GetElement(0))
when an argument exists, and otherwise return
TGocciaUndefinedLiteralValue.UndefinedValue (or handle the empty-argument case
consistently with other setters).
In `@units/Goccia.Values.ClassValue.pas`:
- Around line 44-51: MarkReferences currently doesn't mark the new TGocciaValue
initializer arrays so those values can be collected; update the class's
MarkReferences method to iterate and call Mark on each TGocciaValue in
FMethodInitializers and FFieldInitializers and also iterate
FDecoratorFieldInitializers calling Mark on each record's Initializer
(respecting IsPrivate/IsStatic only if relevant), ensuring all TGocciaValue
references are marked; locate the MarkReferences implementation and add loops
that call the existing MarkReferences/Mark helper on each TGocciaValue element
to prevent GC collection.
- Around line 551-565: TGocciaClassValue.RunMethodInitializers currently
unsafely casts entries of FMethodInitializers to TGocciaFunctionBase; modify the
loop to verify the type before calling by checking each FMethodInitializers[Idx]
with the "is" operator (or appropriate type-check) and only create
TGocciaArgumentsCollection and call TGocciaFunctionBase(Call) when the item is a
TGocciaFunctionBase, otherwise skip (or log/raise a controlled error) to avoid
runtime exceptions when non-callable values are present.
In `@units/Goccia.Values.SymbolValue.pas`:
- Around line 183-193: Add a spec-style comment above the
TGocciaSymbolValue.WellKnownMetadata function using the required format for TC39
proposals, e.g. "// TC39 proposal-decorator-metadata §X.Y.Z WellKnownMetadata",
so the method implementing the proposal is annotated; locate the function
(TGocciaSymbolValue.WellKnownMetadata) that initializes FWellKnownMetadata and
pins it via TGocciaGarbageCollector.Instance.PinValue and insert the single-line
comment immediately above the function declaration following the exact "// TC39
<Proposal> §X.Y.Z MethodName" pattern.
---
Nitpick comments:
In `@tests/language/decorators/decorator-errors.js`:
- Around line 7-18: Replace the manual try/catch/flag pattern in the
"non-callable decorator throws" test with Jest's toThrow matcher: wrap the class
declaration that uses the non-callable decorator (the const notAFunction = 42
and the decorated class C) inside a function passed to expect, and assert
.toThrow() on it (e.g., expect(() => { const notAFunction = 42; `@notAFunction`
class C {} }).toThrow()); remove the threw variable and try/catch block.
In `@tests/language/decorators/decorator-metadata.js`:
- Around line 1-60: Add a new test case that covers the prototype-chain
inheritance behavior by creating a base class with a class-level decorator
(e.g., reuse the existing track/first decorator pattern that writes to
context.metadata) and then declare a subclass that extends the base; assert that
the subclass’s Symbol.metadata either contains the inherited metadata entries
from the base (or that the subclass metadata object is linked appropriately per
spec), and also verify that modifying subclass metadata does not unexpectedly
mutate the base metadata object; reference the existing test patterns using
class C, context.metadata and Symbol.metadata to match style and placement in
the same file.
In `@tests/language/decorators/decorator-order.js`:
- Around line 7-21: Rename the test description string in the test(...)
declaration so it accurately describes bottom-up application order (e.g., change
"decorators evaluated top-to-bottom" to "decorators applied bottom-to-top"),
leaving the test body, the order array and class C unchanged; update the
test(...) call's first argument near the class C definition and the
order/assertions to reflect the new descriptive text.
In `@units/Goccia.Evaluator.Decorators.pas`:
- Around line 45-53: The AddInitializer method grows FInitializers by one each
call causing quadratic reallocations; modify TGocciaInitializerCollector to
track count and capacity (e.g., FInitializersCount and FInitializersCapacity)
and change AddInitializer to check capacity before adding: when capacity is zero
or full, grow capacity (e.g., double it or add a fixed chunk) with a single
SetLength(FInitializers, FInitializersCapacity), then append the new initializer
to FInitializers[FInitializersCount] and increment FInitializersCount; keep
Result logic the same. Ensure any existing code that reads FInitializers uses
the actual count (FInitializersCount) or resizes the array down when finalizing.
Comment on lines
+6
to
+56
| describe("field decorators", () => { | ||
| test("decorator receives undefined and context", () => { | ||
| let receivedValue; | ||
| let receivedContext; | ||
|
|
||
| const log = (value, context) => { | ||
| receivedValue = value; | ||
| receivedContext = context; | ||
| }; | ||
|
|
||
| class C { | ||
| @log | ||
| x = 42; | ||
| } | ||
|
|
||
| expect(receivedValue).toBe(undefined); | ||
| expect(receivedContext.kind).toBe("field"); | ||
| expect(receivedContext.name).toBe("x"); | ||
| expect(receivedContext.static).toBe(false); | ||
| expect(receivedContext.private).toBe(false); | ||
| }); | ||
|
|
||
| test("decorator can return initializer function", () => { | ||
| const doubleInit = (value, context) => { | ||
| return (initialValue) => { | ||
| return initialValue * 2; | ||
| }; | ||
| }; | ||
|
|
||
| class C { | ||
| @doubleInit | ||
| x = 21; | ||
| } | ||
|
|
||
| const c = new C(); | ||
| expect(c.x).toBe(42); | ||
| }); | ||
|
|
||
| test("decorator returning undefined keeps original initializer", () => { | ||
| const noop = (value, context) => { | ||
| return undefined; | ||
| }; | ||
|
|
||
| class C { | ||
| @noop | ||
| x = 42; | ||
| } | ||
|
|
||
| const c = new C(); | ||
| expect(c.x).toBe(42); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add edge/error-case coverage for field decorators.
These tests cover the happy path well; consider adding edge/error cases (e.g., decorator returning a non-callable initializer, throwing inside decorator, or invalid context usage) to complete coverage if not already covered elsewhere.
As per coding guidelines: “When implementing a new language feature, create JavaScript test files following existing patterns in tests/. Tests should cover happy paths, edge cases, and error cases. Verify all tests pass before committing by running: ./build.pas testrunner && ./build/TestRunner tests”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/language/decorators/basic-field-decorator.js` around lines 6 - 56, Add
explicit edge/error-case tests to
tests/language/decorators/basic-field-decorator.js: add test cases that assert
(1) a decorator that returns a non-callable value throws a TypeError when
applied to a field (reference the existing noop/doubleInit patterns and class
C), (2) a decorator that throws synchronously causes class definition or
instantiation to propagate the error (use a decorator function that throws and
expect the error), and (3) invalid context usages (e.g., asserting
context.kind/name/static/private values for static/private fields) by defining
classes with static and private fields and verifying the decorator’s received
context; follow the existing test style (describe/test/expect) and ensure each
new test uses expect(...).toThrow or expect(...).toBe assertions consistent with
surrounding tests.
Comment on lines
+30
to
+47
| test("decorator can replace method", () => { | ||
| const double = (method, context) => { | ||
| return (/* ...args */) => { | ||
| const result = method.call(this); | ||
| return result * 2; | ||
| }; | ||
| }; | ||
|
|
||
| class Calculator { | ||
| @double | ||
| value() { | ||
| return 21; | ||
| } | ||
| } | ||
|
|
||
| const calc = new Calculator(); | ||
| expect(calc.value()).toBe(42); | ||
| }); |
Contributor
There was a problem hiding this comment.
Incorrect this binding in decorator replacement function.
The double decorator returns an arrow function which captures this from its lexical scope (the module/global context), not from the method invocation. When calc.value() is called, method.call(this) will use the wrong this value.
To correctly wrap the method while preserving this binding, use a regular function expression or method shorthand.
💚 Proposed fix
test("decorator can replace method", () => {
const double = (method, context) => {
- return (/* ...args */) => {
- const result = method.call(this);
+ return function(/* ...args */) {
+ const result = method.call(this);
return result * 2;
};
};📝 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
| test("decorator can replace method", () => { | |
| const double = (method, context) => { | |
| return (/* ...args */) => { | |
| const result = method.call(this); | |
| return result * 2; | |
| }; | |
| }; | |
| class Calculator { | |
| @double | |
| value() { | |
| return 21; | |
| } | |
| } | |
| const calc = new Calculator(); | |
| expect(calc.value()).toBe(42); | |
| }); | |
| test("decorator can replace method", () => { | |
| const double = (method, context) => { | |
| return function(/* ...args */) { | |
| const result = method.call(this); | |
| return result * 2; | |
| }; | |
| }; | |
| class Calculator { | |
| `@double` | |
| value() { | |
| return 21; | |
| } | |
| } | |
| const calc = new Calculator(); | |
| expect(calc.value()).toBe(42); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/language/decorators/basic-method-decorator.js` around lines 30 - 47,
The decorator `double` currently returns an arrow function which lexically binds
`this`, causing `method.call(this)` to use the wrong `this`; update `double` to
return a regular function (e.g., function(...args) { ... }) so `this` is
dynamically bound at call time, forward received arguments to the original
`method` (use method.apply or call with args), and return the doubled result;
update references in the diff to the `double` function and ensure
`Calculator.prototype.value` wrapping preserves method semantics.
Comment on lines
+89
to
+94
| function TGocciaAccessSetter.SetValue(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; | ||
| begin | ||
| if Assigned(AThisValue) and (AThisValue is TGocciaObjectValue) then | ||
| TGocciaObjectValue(AThisValue).AssignProperty(FPropertyName, AArgs.GetElement(0)); | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| end; |
Contributor
There was a problem hiding this comment.
Missing bounds check before accessing argument.
SetValue accesses AArgs.GetElement(0) without verifying AArgs.Length > 0, similar to the auto-accessor setter issue.
🛡️ Proposed fix
function TGocciaAccessSetter.SetValue(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue;
begin
- if Assigned(AThisValue) and (AThisValue is TGocciaObjectValue) then
+ if Assigned(AThisValue) and (AThisValue is TGocciaObjectValue) and (AArgs.Length > 0) then
TGocciaObjectValue(AThisValue).AssignProperty(FPropertyName, AArgs.GetElement(0));
Result := TGocciaUndefinedLiteralValue.UndefinedValue;
end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Evaluator.Decorators.pas` around lines 89 - 94,
TGocciaAccessSetter.SetValue currently calls AArgs.GetElement(0) without
ensuring an argument exists; add a defensive check (e.g., if AArgs.Length > 0)
before calling AArgs.GetElement(0) and only call
TGocciaObjectValue(AThisValue).AssignProperty(FPropertyName,
AArgs.GetElement(0)) when the check passes; if no argument is available, avoid
calling GetElement(0) (either skip the assignment or handle the error/raise as
appropriate for the codebase) so the method no longer accesses out-of-bounds
arguments.
Comment on lines
+1898
to
+1916
| // TC39 proposal-decorators: Set up auto-accessors (creates backing field + getter + setter) | ||
| for I := 0 to High(AClassDef.FElements) do | ||
| begin | ||
| if AClassDef.FElements[I].Kind = cekAccessor then | ||
| begin | ||
| Elem := AClassDef.FElements[I]; | ||
| AccessorBackingName := '__accessor_' + Elem.Name; | ||
|
|
||
| if Assigned(Elem.FieldInitializer) then | ||
| ClassValue.AddInstanceProperty(Elem.Name, Elem.FieldInitializer) | ||
| else | ||
| ClassValue.AddInstanceProperty(Elem.Name, nil); | ||
|
|
||
| ClassValue.AddAutoAccessor(Elem.Name, AccessorBackingName, Elem.IsStatic); | ||
| end; | ||
| end; | ||
|
|
||
| // TC39 proposal-decorators: Apply decorators to class elements and class itself | ||
| HasDecorators := (Length(AClassDef.FDecorators) > 0) or (Length(AClassDef.FElements) > 0); |
Contributor
There was a problem hiding this comment.
Add TC39 Decorators spec annotations for this new algorithm.
The TC39 proposal-decorators headers don’t match the required // TC39 Decorators §X.Y.Z MethodName(params) format. Please add the spec-section annotation above the decorator handling block or on EvaluateClassDefinition. As per coding guidelines: When implementing ECMAScript-specified behavior, annotate each function or method with a comment referencing the relevant spec section in format // ESYYYY §X.Y.Z MethodName(params) where YYYY is the current edition year; for TC39 proposals, reference the proposal name instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Evaluator.pas` around lines 1898 - 1916, Add a spec-section
annotation for the TC39 decorators algorithm by inserting a comment in the
required format (e.g. "// TC39 Decorators §X.Y.Z MethodName(params)") either
immediately above the decorator handling block shown (the loop that processes
cekAccessor and calls ClassValue.AddAutoAccessor) or on the surrounding routine
(e.g. EvaluateClassDefinition/whatever function contains this block); ensure the
proposal name "TC39 Decorators" is used and include the section number and
method signature per the coding guideline format.
Comment on lines
+1898
to
+1913
| // TC39 proposal-decorators: Set up auto-accessors (creates backing field + getter + setter) | ||
| for I := 0 to High(AClassDef.FElements) do | ||
| begin | ||
| if AClassDef.FElements[I].Kind = cekAccessor then | ||
| begin | ||
| Elem := AClassDef.FElements[I]; | ||
| AccessorBackingName := '__accessor_' + Elem.Name; | ||
|
|
||
| if Assigned(Elem.FieldInitializer) then | ||
| ClassValue.AddInstanceProperty(Elem.Name, Elem.FieldInitializer) | ||
| else | ||
| ClassValue.AddInstanceProperty(Elem.Name, nil); | ||
|
|
||
| ClassValue.AddAutoAccessor(Elem.Name, AccessorBackingName, Elem.IsStatic); | ||
| end; | ||
| end; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "AddAutoAccessor" -C 4Repository: frostney/GocciaScript
Length of output: 2416
🏁 Script executed:
rg -n "__accessor_" -C 4Repository: frostney/GocciaScript
Length of output: 649
🏁 Script executed:
sed -n '535,600p' units/Goccia.Values.ClassValue.pas | cat -nRepository: frostney/GocciaScript
Length of output: 2724
🏁 Script executed:
rg -n "procedure AddInstanceProperty" -A 20 units/Goccia.Values.ClassValue.pasRepository: frostney/GocciaScript
Length of output: 1756
🏁 Script executed:
rg -n "FInstanceProperties" -C 3 | head -80Repository: frostney/GocciaScript
Length of output: 2700
🏁 Script executed:
rg -n "procedure.*AddInstanceProperty" units/Goccia.Values.ClassValue.pas -A 15Repository: frostney/GocciaScript
Length of output: 2251
🏁 Script executed:
rg -n "FInstancePropertyDefs" units/Goccia.Values.ClassValue.pas | head -20Repository: frostney/GocciaScript
Length of output: 447
🏁 Script executed:
rg -n "FInstancePropertyDefs" units/Goccia.Evaluator.pas | head -20Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
rg -n "RunFieldInitializers\|RunDecoratorFieldInitializers\|InstancePropertyDefs\|RunAutoAccessorInitializers" units/Goccia.Evaluator.pas | head -20Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
rg -n "for.*InstancePropertyDefs\|FInstancePropertyDefs" units/Goccia.Values.ClassValue.pasRepository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
rg -n "Instantiate\|CreateNativeInstance" units/Goccia.Values.ClassValue.pas -A 30 | head -100Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
sed -n '1880,1930p' units/Goccia.Evaluator.pas | cat -nRepository: frostney/GocciaScript
Length of output: 2658
🏁 Script executed:
rg -n "InstancePropertyDefs\|InitializeInstanceProperties" units/ -lRepository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
rg -n "for.*InstancePropertyOrder\|InstancePropertyDefs" units/Goccia.Values.ClassValue.pas -B 2 -A 10Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
grep -rn "CreateNativeInstance\|Instantiate" units/Goccia.Values.ClassValue.pas | head -5Repository: frostney/GocciaScript
Length of output: 620
🏁 Script executed:
rg -n "class TGocciaClassValue" units/Goccia.Values.ClassValue.pas -A 100 | grep -E "procedure|function" | head -20Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
rg -n "function.*Instantiate\|function.*CreateNativeInstance" units/Goccia.Values.ClassValue.pas -A 40 | head -120Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
wc -l units/Goccia.Values.ClassValue.pasRepository: frostney/GocciaScript
Length of output: 104
🏁 Script executed:
# Find line numbers where Instantiate and CreateNativeInstance are actually implemented
grep -n "^function.*Instantiate\|^function.*CreateNativeInstance" units/Goccia.Values.ClassValue.pasRepository: frostney/GocciaScript
Length of output: 1031
🏁 Script executed:
sed -n '648,720p' units/Goccia.Values.ClassValue.pas | cat -nRepository: frostney/GocciaScript
Length of output: 2609
🏁 Script executed:
rg -n "RunFieldInitializers\|RunDecoratorFieldInitializers" units/Goccia.Evaluator.pasRepository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
rg -rn "RunFieldInitializers\|RunDecoratorFieldInitializers" units/ --include="*.pas"Repository: frostney/GocciaScript
Length of output: 506
🏁 Script executed:
rg -n "RunFieldInitializers\|RunDecoratorFieldInitializers" units/Repository: frostney/GocciaScript
Length of output: 47
Initializer registration uses public accessor name, not backing field.
The initializer is registered via AddInstanceProperty(Elem.Name, Elem.FieldInitializer) before the accessor is created. Since AddAutoAccessor registers the getter/setter on the prototype for the same Elem.Name, the initializer and backing field initialization are misaligned. If instance property initializers are applied at construction time, the initializer on Elem.Name will either shadow the accessor or initialize the wrong location. The initializer should be registered on AccessorBackingName instead to match the backing field that the getter/setter manipulate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Evaluator.pas` around lines 1898 - 1913, The initializer is
being registered on the public accessor name (Elem.Name) instead of the backing
field, causing the instance initializer to miss the backing storage used by the
getter/setter; change the call that registers the initializer so
AddInstanceProperty is invoked with AccessorBackingName (not Elem.Name) when
Elem.FieldInitializer is assigned, keeping the AddAutoAccessor(Elem.Name,
AccessorBackingName, Elem.IsStatic) call as-is so the accessor and backing-field
initializer align (refer to AClassDef.FElements loop, Elem.FieldInitializer,
AccessorBackingName, AddInstanceProperty and AddAutoAccessor).
Comment on lines
+2190
to
+2195
| if (DecoratorResult <> nil) and not (DecoratorResult is TGocciaUndefinedLiteralValue) then | ||
| begin | ||
| if not DecoratorResult.IsCallable then | ||
| AContext.OnError('Class decorator must return a class/function or undefined', ALine, AColumn); | ||
| ClassValue := TGocciaClassValue(DecoratorResult); | ||
| end; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
TC39 decorators proposal class decorator return value specification requirements
💡 Result:
Under the current TC39 Stage 3 decorators proposal (tc39/proposal-decorators), a class decorator:
- Has the shape
type ClassDecorator = (value: Function, context: { kind: "class"; name: string | undefined; addInitializer(...) }) => Function | void. Returningvoid(i.e.,undefined) means “no replacement; keep the original class”. [1] - May replace the class only by returning a callable value (e.g., another class, function, or Proxy). If it returns a non-callable value, a runtime error is thrown (TypeError). [1]
Sources: [1] (github.com)
Citations:
🏁 Script executed:
cat -n units/Goccia.Evaluator.pas | sed -n '2180,2210p'Repository: frostney/GocciaScript
Length of output: 1757
🏁 Script executed:
rg -A 10 -B 10 "DecoratorResult" units/Goccia.Evaluator.pas | head -80Repository: frostney/GocciaScript
Length of output: 3674
Fix unsafe cast of decorator return value — class decorators per TC39 may return any callable, not just classes.
The code checks IsCallable correctly per the TC39 decorators spec, but then unsafely casts to TGocciaClassValue. Per TC39, class decorators can return any callable (function, class, Proxy, etc.), not just TGocciaClassValue. The direct cast at line 2194 will crash if a decorator returns a non-class callable.
The error message "Class decorator must return a class/function or undefined" already reflects spec compliance (any callable is valid), but the cast contradicts this. Either:
- Reject non-class callables (most conservative): Check
DecoratorResult is TGocciaClassValueand update the error message to "must return a class" — but this violates TC39 spec. - Accept any callable (spec-compliant): Remove the unsafe cast. Assign directly to a
TGocciaValuevariable, or refactor code that depends on class-specific methods to handle arbitrary callables.
Current implementation is inconsistent: accepts any callable in validation but requires a class in the cast.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Evaluator.pas` around lines 2190 - 2195, The code currently
validates DecoratorResult with IsCallable but then unsafely casts it to
TGocciaClassValue (TGocciaClassValue(DecoratorResult)), which will crash for
non-class callables; change this to accept any callable per TC39 by removing the
direct cast and keeping DecoratorResult as a generic TGocciaValue (or a new
local TGocciaValue variable) and update subsequent logic that used ClassValue to
perform runtime checks (e.g., "if DecoratorResult is TGocciaClassValue then"
before calling class-specific methods) or treat callables uniformly; ensure
error reporting via AContext.OnError remains correct and only reject
non-callables (still treat TGocciaUndefinedLiteralValue as before).
Comment on lines
+55
to
+60
| function TGocciaAutoAccessorSetter.SetValue(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; | ||
| begin | ||
| if Assigned(AThisValue) and (AThisValue is TGocciaObjectValue) then | ||
| TGocciaObjectValue(AThisValue).AssignProperty(FBackingName, AArgs.GetElement(0)); | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| end; |
Contributor
There was a problem hiding this comment.
Missing bounds check before accessing argument.
SetValue accesses AArgs.GetElement(0) without verifying AArgs.Length > 0. If called with no arguments, this may access an invalid index.
🛡️ Proposed fix to add bounds check
function TGocciaAutoAccessorSetter.SetValue(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue;
begin
- if Assigned(AThisValue) and (AThisValue is TGocciaObjectValue) then
+ if Assigned(AThisValue) and (AThisValue is TGocciaObjectValue) and (AArgs.Length > 0) then
TGocciaObjectValue(AThisValue).AssignProperty(FBackingName, AArgs.GetElement(0));
Result := TGocciaUndefinedLiteralValue.UndefinedValue;
end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Values.AutoAccessor.pas` around lines 55 - 60, The SetValue
implementation in TGocciaAutoAccessorSetter currently uses AArgs.GetElement(0)
without verifying there is at least one argument; update
TGocciaAutoAccessorSetter.SetValue to check the argument count (e.g.
AArgs.Length or Count) before calling AArgs.GetElement(0), only calling
TGocciaObjectValue(AThisValue).AssignProperty(FBackingName, AArgs.GetElement(0))
when an argument exists, and otherwise return
TGocciaUndefinedLiteralValue.UndefinedValue (or handle the empty-argument case
consistently with other setters).
Comment on lines
+44
to
+51
| FMethodInitializers: array of TGocciaValue; | ||
| FFieldInitializers: array of TGocciaValue; | ||
| FDecoratorFieldInitializers: array of record | ||
| Name: string; | ||
| Initializer: TGocciaValue; | ||
| IsPrivate: Boolean; | ||
| IsStatic: Boolean; | ||
| end; |
Contributor
There was a problem hiding this comment.
MarkReferences must mark initializer arrays to prevent GC collection.
The new fields FMethodInitializers, FFieldInitializers, and FDecoratorFieldInitializers hold TGocciaValue references that are not marked in MarkReferences. Per coding guidelines, each value type must override MarkReferences to mark all TGocciaValue references it holds.
🛡️ Proposed fix: Add marking for initializer arrays
Add to the existing MarkReferences method (after line 307):
// Mark static symbol descriptors
for SymPair in FStaticSymbolDescriptors do
begin
...
end;
+
+ // Mark method initializers
+ for I := 0 to High(FMethodInitializers) do
+ if Assigned(FMethodInitializers[I]) then
+ FMethodInitializers[I].MarkReferences;
+
+ // Mark field initializers
+ for I := 0 to High(FFieldInitializers) do
+ if Assigned(FFieldInitializers[I]) then
+ FFieldInitializers[I].MarkReferences;
+
+ // Mark decorator field initializers
+ for I := 0 to High(FDecoratorFieldInitializers) do
+ if Assigned(FDecoratorFieldInitializers[I].Initializer) then
+ FDecoratorFieldInitializers[I].Initializer.MarkReferences;
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 44 - 51, MarkReferences
currently doesn't mark the new TGocciaValue initializer arrays so those values
can be collected; update the class's MarkReferences method to iterate and call
Mark on each TGocciaValue in FMethodInitializers and FFieldInitializers and also
iterate FDecoratorFieldInitializers calling Mark on each record's Initializer
(respecting IsPrivate/IsStatic only if relevant), ensuring all TGocciaValue
references are marked; locate the MarkReferences implementation and add loops
that call the existing MarkReferences/Mark helper on each TGocciaValue element
to prevent GC collection.
Comment on lines
+551
to
+565
| procedure TGocciaClassValue.RunMethodInitializers(const AInstance: TGocciaValue); | ||
| var | ||
| Idx: Integer; | ||
| Args: TGocciaArgumentsCollection; | ||
| begin | ||
| for Idx := 0 to High(FMethodInitializers) do | ||
| begin | ||
| Args := TGocciaArgumentsCollection.Create; | ||
| try | ||
| TGocciaFunctionBase(FMethodInitializers[Idx]).Call(Args, AInstance); | ||
| finally | ||
| Args.Free; | ||
| end; | ||
| end; | ||
| end; |
Contributor
There was a problem hiding this comment.
Unsafe cast to TGocciaFunctionBase without type verification.
RunMethodInitializers casts initializers directly to TGocciaFunctionBase. If a non-callable value is incorrectly added to the array, this will cause a runtime exception.
🛡️ Proposed fix: Add type guard
procedure TGocciaClassValue.RunMethodInitializers(const AInstance: TGocciaValue);
var
Idx: Integer;
Args: TGocciaArgumentsCollection;
begin
for Idx := 0 to High(FMethodInitializers) do
begin
+ if not (FMethodInitializers[Idx] is TGocciaFunctionBase) then
+ Continue;
Args := TGocciaArgumentsCollection.Create;
try
TGocciaFunctionBase(FMethodInitializers[Idx]).Call(Args, AInstance);
finally
Args.Free;
end;
end;
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 TGocciaClassValue.RunMethodInitializers(const AInstance: TGocciaValue); | |
| var | |
| Idx: Integer; | |
| Args: TGocciaArgumentsCollection; | |
| begin | |
| for Idx := 0 to High(FMethodInitializers) do | |
| begin | |
| Args := TGocciaArgumentsCollection.Create; | |
| try | |
| TGocciaFunctionBase(FMethodInitializers[Idx]).Call(Args, AInstance); | |
| finally | |
| Args.Free; | |
| end; | |
| end; | |
| end; | |
| procedure TGocciaClassValue.RunMethodInitializers(const AInstance: TGocciaValue); | |
| var | |
| Idx: Integer; | |
| Args: TGocciaArgumentsCollection; | |
| begin | |
| for Idx := 0 to High(FMethodInitializers) do | |
| begin | |
| if not FMethodInitializers[Idx].IsCallable then | |
| Continue; | |
| Args := TGocciaArgumentsCollection.Create; | |
| try | |
| TGocciaFunctionBase(FMethodInitializers[Idx]).Call(Args, AInstance); | |
| finally | |
| Args.Free; | |
| end; | |
| end; | |
| 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 551 - 565,
TGocciaClassValue.RunMethodInitializers currently unsafely casts entries of
FMethodInitializers to TGocciaFunctionBase; modify the loop to verify the type
before calling by checking each FMethodInitializers[Idx] with the "is" operator
(or appropriate type-check) and only create TGocciaArgumentsCollection and call
TGocciaFunctionBase(Call) when the item is a TGocciaFunctionBase, otherwise skip
(or log/raise a controlled error) to avoid runtime exceptions when non-callable
values are present.
Comment on lines
+183
to
+193
| // TC39 proposal-decorator-metadata | ||
| class function TGocciaSymbolValue.WellKnownMetadata: TGocciaSymbolValue; | ||
| begin | ||
| if not Assigned(FWellKnownMetadata) then | ||
| begin | ||
| FWellKnownMetadata := TGocciaSymbolValue.Create('Symbol.metadata'); | ||
| if Assigned(TGocciaGarbageCollector.Instance) then | ||
| TGocciaGarbageCollector.Instance.PinValue(FWellKnownMetadata); | ||
| end; | ||
| Result := FWellKnownMetadata; | ||
| end; |
Contributor
There was a problem hiding this comment.
Add spec-style annotation for WellKnownMetadata.
The current comment doesn’t match the required // TC39 <Proposal> §X.Y.Z MethodName format. Please add the proposal section reference and method name above the function.
As per coding guidelines: “When implementing ECMAScript-specified behavior, annotate each function or method with a comment referencing the relevant spec section in format // ESYYYY §X.Y.Z MethodName(params)… For TC39 proposals not yet merged (Temporal, Iterator Helpers), reference the proposal name instead: // TC39 Temporal §5.5.3 MethodName”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Values.SymbolValue.pas` around lines 183 - 193, Add a spec-style
comment above the TGocciaSymbolValue.WellKnownMetadata function using the
required format for TC39 proposals, e.g. "// TC39 proposal-decorator-metadata
§X.Y.Z WellKnownMetadata", so the method implementing the proposal is annotated;
locate the function (TGocciaSymbolValue.WellKnownMetadata) that initializes
FWellKnownMetadata and pins it via TGocciaGarbageCollector.Instance.PinValue and
insert the single-line comment immediately above the function declaration
following the exact "// TC39 <Proposal> §X.Y.Z MethodName" pattern.
This was referenced Apr 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Symbol.metadatafor decorator metadata handlingaddInitializercontext method for decoratorsDocumentation
Symbol.metadataTests