Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds first-class async/await and async-iteration support across parser, AST, evaluator, runtime values, builtins (Array.fromAsync, Symbol.asyncIterator), test assertions, benchmark API/outputs, constants, and many tests; integrates microtask/thenable handling, for-of / for-await-of semantics, and benchmark setup/run/teardown timing. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant Evaluator
participant AsyncFn as AsyncFunctionValue
participant Promise as TGocciaPromiseValue
participant Microtask as MicrotaskQueue
Parser->>Evaluator: EvaluateExpression(async function call)
Evaluator->>AsyncFn: Call(args, this)
AsyncFn->>Promise: Create & return Promise
AsyncFn->>Evaluator: Execute body in new call scope
alt body returns value
Evaluator-->>AsyncFn: Result value
AsyncFn->>Promise: Resolve(result)
else body throws TGocciaThrowValue
Evaluator-->>AsyncFn: TGocciaThrowValue
AsyncFn->>Promise: Reject(error)
end
AsyncFn->>Microtask: Schedule microtask reactions
Microtask-->>Promise: Process queued promise reactions
sequenceDiagram
participant Parser
participant Evaluator
participant Source as IterableSource
participant Iterator
participant Microtask as MicrotaskQueue
participant Binding as LoopBinding
Parser->>Evaluator: EvaluateStatement(for-await-of)
Evaluator->>Source: Get(Symbol.asyncIterator) or fallback Symbol.iterator
Source-->>Evaluator: Iterator object
loop each iteration
Evaluator->>Iterator: Call next()
Iterator-->>Evaluator: {value, done} or Promise
alt next() returned Promise
Evaluator->>Microtask: Drain & await resolution
end
Evaluator->>Binding: Bind loop variable (const/let)
Evaluator->>Evaluator: Execute loop body
alt break encountered
Evaluator-->>Evaluator: Exit loop
end
end
Evaluator-->>Parser: Loop completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
> [!CAUTION]
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)units/Goccia.Builtins.GlobalSymbol.pas (1)
68-75:⚠️ Potential issue | 🟡 MinorReplace hardcoded
asyncIteratorwith a property-name constant.Please add a
PROP_ASYNC_ITERATORconstant inGoccia.Constants.PropertyNamesand use it here to avoid hardcoded property strings.As per coding guidelines "Use split constant units instead of hardcoded string literals for property names, type names, error names, and constructor names:
Goccia.Constants.PropertyNames,Goccia.Constants.TypeNames,Goccia.Constants.ErrorNames,Goccia.Constants.ConstructorNames,Goccia.Constants".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.GlobalSymbol.pas` around lines 68 - 75, Add a PROP_ASYNC_ITERATOR constant to the Goccia.Constants.PropertyNames unit and replace the hardcoded 'asyncIterator' literal in units/Goccia.Builtins.GlobalSymbol.pas by using that constant (i.e. change the FSymbolFunction.RegisterConstant call that currently passes 'asyncIterator' to pass the PropertyNames.PROP_ASYNC_ITERATOR symbol instead); also update the uses clause in Goccia.Builtins.GlobalSymbol.pas to include the PropertyNames constants unit so the new constant is visible.
♻️ Duplicate comments (4)
tests/language/for-of/for-await-of.js (1)
6-94: Loop restriction conflict for for-await-of tests.for-await-of is still a traditional loop form; please update the guideline or add an exception if this is now supported.
As per coding guidelines, "GocciaScript does not support
eval(), theargumentsobject, automatic semicolon insertion (semicolons are required), thewithstatement, or traditional loops (for,while,do...while). Use array methods instead of loops."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/language/for-of/for-await-of.js` around lines 6 - 94, The test suite uses traditional for-await-of loops in the "for-await-of" describe block and its tests (e.g., the async fn functions that perform "for await (const item of ...)" and the "break in for-await-of" test); update the project linting/guideline configuration or add an explicit exception so these tests are allowed to use for-await-of (either whitelist this describe/test group in the linter, add a rule exception for "for-await-of" traditional loops, or update the guideline text to state that for-await-of is supported), and ensure the test runner/linter respects that change so the tests under describe("for-await-of") are not flagged as using prohibited traditional loops.tests/language/for-of/for-of-iterators.js (1)
6-46: Loop restriction conflict for for...of tests.This uses for...of loops, which are currently disallowed by test guidelines. Please update the rule or add a dedicated exception.
As per coding guidelines, "GocciaScript does not support
eval(), theargumentsobject, automatic semicolon insertion (semicolons are required), thewithstatement, or traditional loops (for,while,do...while). Use array methods instead of loops."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/language/for-of/for-of-iterators.js` around lines 6 - 46, The tests inside the describe block "for...of with iterators" (tests "iterates over Set", "iterates over Map", "iterates over custom iterable") use for...of loops which violate the project guideline banning traditional loops; either update the lint/test rule to explicitly allow for...of iterations for iterator-related tests, or rewrite these tests to avoid for...of by using allowed constructs (e.g., convert iterables to arrays with Array.from and use array methods like forEach/map) so the behavior is preserved without using for...of.tests/language/for-of/for-of-break.js (1)
6-28: Loop restriction conflict for for...of tests.These loops conflict with the current “no traditional loops” test rule; please update the rule or add an explicit exception for for...of.
As per coding guidelines, "GocciaScript does not support
eval(), theargumentsobject, automatic semicolon insertion (semicolons are required), thewithstatement, or traditional loops (for,while,do...while). Use array methods instead of loops."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/language/for-of/for-of-break.js` around lines 6 - 28, The tests under describe("for...of break") (specifically test("break exits the loop early") and test("break in nested for...of")) are being flagged by the project rule banning traditional loops; either relax that rule to permit ES6 for...of loops or add an explicit exception for these test files: update the loop restriction rule (the "no traditional loops" / no-traditional-loops lint/test rule or test harness filter) to exclude for...of constructs or add a whitelist entry for tests/language/for-of/* so for...of is allowed, ensuring the tests can use for...of semantics with break without changing the test code.tests/language/for-of/basic-for-of.js (1)
6-47: Loop restriction conflict for for...of tests.These tests use traditional loops, which the current test guidelines forbid. Please update the rule or add a for...of exception.
As per coding guidelines, "GocciaScript does not support
eval(), theargumentsobject, automatic semicolon insertion (semicolons are required), thewithstatement, or traditional loops (for,while,do...while). Use array methods instead of loops."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/language/for-of/basic-for-of.js` around lines 6 - 47, The tests in describe("basic for...of") use for...of loops which the current "no-traditional-loops"/GocciaScript loop restriction blocks; update the rule or linter/configuration to allow ForOfStatement while still forbidding traditional for/while/do-while loops. Concretely, alter the rule that enforces "no-traditional-loops" (or the ESLint no-restricted-syntax/no-loops setting) so it excludes ForOfStatement, or add a test-suite-specific override for the tests with titles like "iterates over array elements", "iterates over string characters", and "const binding per iteration" to permit for...of usage. Ensure only for...of is allowed (keep forbidding ForStatement, WhileStatement, DoWhileStatement).
🧹 Nitpick comments (5)
tests/built-ins/Array/fromAsync-edge-cases.js (1)
179-193: Remove the duplicatenextmethod in the async iterator literal.
The earliernext()is immediately overwritten, which is confusing and unnecessary.♻️ Suggested cleanup
- return { - next() { - return Promise.resolve({ value: "async", done: false }); - }, - count: 0, - next() { + return { + count: 0, + next() { this.count = this.count + 1; if (this.count <= 1) { return Promise.resolve({ value: "async", done: false }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/Array/fromAsync-edge-cases.js` around lines 179 - 193, The iterator object returned by the [Symbol.asyncIterator] method contains a duplicate next() definition (the first next() is immediately overwritten by the second), so remove the redundant earlier next() and keep the intended async iterator implementation that uses this.count and returns Promise.resolve({ value: ..., done: ... }); update the iterator literal inside the [Symbol.asyncIterator]() function to have a single next method (the one that increments this.count and signals done).units/Goccia.Parser.pas (4)
367-374: Add ECMAScript spec annotation for await expression.Per coding guidelines, ECMAScript-specified behavior should include spec annotations.
📝 Suggested spec annotation
+ // ES2017 §14.6.2 AwaitExpression // await expression (only valid inside async functions) if (FInAsyncFunction > 0) and Check(gttIdentifier) and (Peek.Lexeme = KEYWORD_AWAIT) then beginBased on learnings: "If the feature implements ECMAScript-specified behavior, add
// ESYYYY §X.Y.Zspec annotations above each function body and at key algorithm steps within the body"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Parser.pas` around lines 367 - 374, Add an ECMAScript spec annotation comment for the await-expression handling: above the await-handling block that checks FInAsyncFunction and creates TGocciaAwaitExpression (the code that advances the 'await' token, sets Right := Unary and constructs TGocciaAwaitExpression) add a comment like "// ES2017 §14.8.2" (or the correct ES year/section) and, if helpful, add inline step annotations for the operator consumption and node creation to reflect the spec steps; ensure the chosen section number matches the ECMAScript spec for Await Expressions.
669-676: InitializeParameters[0].Patterntonilfor consistency.Other parameter initialization code (e.g., line 1040) explicitly sets
Pattern := nilwhenIsPatternisFalse. This should be consistent to avoid potential uninitialized memory issues depending on compiler settings.♻️ Proposed fix
SetLength(Parameters, 1); Parameters[0].Name := Name; Parameters[0].DefaultValue := nil; Parameters[0].IsPattern := False; + Parameters[0].Pattern := nil; Parameters[0].IsRest := False; Parameters[0].IsOptional := False; Parameters[0].TypeAnnotation := '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Parser.pas` around lines 669 - 676, The Parameters[0] record is missing initialization of its Pattern field which other initialization sites set to nil when IsPattern is False; update the constructor/initializer that sets Parameters[0].Name, DefaultValue, IsPattern, IsRest, IsOptional and TypeAnnotation so it also sets Parameters[0].Pattern := nil when IsPattern is False to match the behavior of the other initialization path (i.e., ensure Parameters[0].Pattern is explicitly nil).
653-653: UseCheckNextfor consistency and bounds safety.Direct array access
FTokens[FCurrent + 1].TokenTypebypasses bounds checking. UseCheckNext(gttArrow)for consistency with the rest of the codebase (e.g., lines 207-212).♻️ Proposed fix
- else if (Name = KEYWORD_ASYNC) and Check(gttIdentifier) and (FTokens[FCurrent + 1].TokenType = gttArrow) then + else if (Name = KEYWORD_ASYNC) and Check(gttIdentifier) and CheckNext(gttArrow) then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Parser.pas` at line 653, The condition uses direct access FTokens[FCurrent + 1].TokenType which can go out of bounds; replace that check with CheckNext(gttArrow) for safety and consistency: keep the existing (Name = KEYWORD_ASYNC) and Check(gttIdentifier) parts but change the final clause to CheckNext(gttArrow) so the parser uses the same bounds-checked helper instead of directly indexing FTokens/FCurrent.
1555-1556: Add ECMAScript spec annotations for for-of and for-await-of statements.Per coding guidelines, these ECMAScript-specified iteration statements should have spec annotations.
📝 Suggested spec annotations
+// ES2015 §13.7.5 ForInOfStatement (for-of) +// ES2018 §13.7.5 ForInOfStatement (for-await-of) function TGocciaParser.ForStatement: TGocciaStatement;Based on learnings: "If the feature implements ECMAScript-specified behavior, add
// ESYYYY §X.Y.Zspec annotations above each function body and at key algorithm steps within the body"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Parser.pas` around lines 1555 - 1556, Add ECMAScript spec annotations to the for-statement parser: place a comment above the TGocciaParser.ForStatement function body referencing the relevant ECMAScript clauses for for-of and for-await-of (for-of: // ES2015 §13.7.5 (or the exact clause in your ECMAScript reference); for-await-of: // ES2018 §13.7.6), and add inline spec comments at key algorithmic steps inside the function where the parser distinguishes between classic for, for-of, and for-await-of heads (e.g., token detection of "await", evaluation of iterable, and iterator/async-iterator handling) using the same // ESYYYY §X.Y.Z format to map code to the spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/for-of.js`:
- Around line 54-62: The benchmark callback is not awaiting the async work:
change the bench callback to be async and return the promise from the for-await
loop so the runner waits for completion; specifically, update the bench
invocation named "for-await-of with sync array" to use an async callback
(instead of a plain function) and either return fn() or inline the async body
(referencing fn and smallArray) so the promise produced by the for-await-of is
awaited by the BenchmarkRunner.
In `@tests/language/async-await/expect-resolves-rejects.js`:
- Around line 85-90: The test named "rejects.toThrow without type argument" uses
await expect(asyncFn()).rejects.toThrow() without an explicit error constructor;
update the assertion to specify the expected error constructor (e.g., use
rejects.toThrow(Error)) so the call becomes await
expect(asyncFn()).rejects.toThrow(Error); locate the asyncFn and the
expect(...).rejects.toThrow() invocation in the test and replace the bare
toThrow() with an explicit constructor reference.
In `@units/Goccia.Builtins.GlobalArray.pas`:
- Around line 348-519: The async iteration code can have GC collect Pascal-only
values (Promise, ResultArray, iterator objects and related temporaries) while
awaiting; add temp roots for each value that is only held in Pascal across
awaits: call AddTempRoot(Promise) immediately after creating Promise and
RemoveTempRoot in the surrounding finally/exit, AddTempRoot(ResultArray) after
creating ResultArray and RemoveTempRoot when done, and similarly
AddTempRoot/RemoveTempRoot for IteratorObj, Iterator (TGocciaIteratorValue/
TGocciaGenericIteratorValue/ TGocciaStringIteratorValue), IteratorMethod,
NextMethod, IterResult, NextResult and MapArgs around the sections that call
AwaitValue or invoke iterator .next (ensure roots are registered before the
first await and removed in the corresponding finally blocks or before each
exit/Reject/Resolve); ensure every AddTempRoot has a matching RemoveTempRoot
even on exceptions so values remain rooted during DrainQueue processing.
- Around line 384-388: Array.fromAsync currently always constructs the result
with TGocciaArrayValue.Create instead of using the receiver constructor
AThisValue; update Array.fromAsync to follow the ArrayOf pattern: check if
AThisValue is a constructor (IsConstructor/IsCallable as used in ArrayOf), and
if so call Instantiate(AThisValue, [length]) to create the result instance,
otherwise fall back to TGocciaArrayValue.Create; ensure you replace the direct
TGocciaArrayValue.Create usage in the Array.fromAsync implementation and use the
same instantiation path/arguments as ArrayOf so subclassed constructors (e.g.,
MyArray.fromAsync) produce instances of the subclass.
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 1200-1227: RunCallbacks currently calls TGocciaFunctionBase.Call
and then runs TGocciaMicrotaskQueue.Instance.DrainQueue while CallbackResult
(and Promise) are only held in local variables, so they can be GC'd during the
drain; to fix, wrap the microtask drain with temporary rooting: after assigning
CallbackResult, call the global root manager's AddTempRoot(CallbackResult) (and
AddTempRoot(Promise) if you detect a promise), then call
TGocciaMicrotaskQueue.Instance.DrainQueue, and finally RemoveTempRoot in a
try/finally so CallbackResult and Promise remain protected while DrainQueue
executes; update TGocciaTestAssertions.RunCallbacks to use
AddTempRoot/RemoveTempRoot around the DrainQueue call for CallbackResult (and
the casted TGocciaPromiseValue) to prevent collection.
In `@units/Goccia.Evaluator.pas`:
- Around line 1298-1299: The ES spec comment above EvaluateAwait is missing the
required method signature; update the spec-annotation to the coding guideline
format by including the method name and parameters (e.g., change "// ES2026
§27.7.5.3 Await" to "// ES2026 §27.7.5.3 EvaluateAwait(const AAwaitExpression:
TGocciaAwaitExpression; const AContext: TGocciaEvaluationContext): TGocciaValue"
) and apply the same fix to the other two spec annotations referenced (the
comments above the corresponding methods at the other locations) so each ES
section line includes the MethodName(params) signature exactly matching the
implemented function/method names.
- Around line 1298-1551: EvaluateAwait and the for-of/for-await-of handlers are
exposing locally-held Promise and iterator objects to GC across microtask
drains; protect those values with temporary roots. Wrap the Promise variable in
EvaluateAwait with AddTempRoot(Promise) immediately after creating/obtaining it
and ensure RemoveTempRoot(Promise) in a finally block after the code that may
call TGocciaMicrotaskQueue.Instance.DrainQueue; similarly, in
EvaluateForOf/EvaluateForAwaitOf add temp roots for Iterator, IteratorObj,
GenericNextResult (and CurrentValue/NextResult when they can be Promise/iterator
objects) before any DrainQueue/await-like operations and remove them in the
corresponding finally blocks so all paths (normal, break/exception) call
RemoveTempRoot. Ensure uses of AddTempRoot/RemoveTempRoot are paired and scoped
around the microtask drain sections in EvaluateAwait, EvaluateForOf, and
EvaluateForAwaitOf.
- Around line 1396-1399: The destructuring branch for for-of/for-await-of
creates mutable bindings because AssignIdentifierPattern currently hardcodes
dtLet; modify AssignPattern (used in the for-of/for-await-of blocks) to accept
and propagate the TDeclarationType (or add an overload) and pass the current
DeclarationType from the loop site into AssignPattern so identifier bindings
inside patterns use the correct declaration kind; update AssignIdentifierPattern
to use the passed-in DeclarationType (so it calls CreateImmutableBinding for
const) and ensure both the for-of (AssignPattern call) and for-await-of sites
mirror this threaded declaration type.
- Around line 1388-1391: Replace all hardcoded iterator property accesses and
direct TGocciaBooleanLiteralValue casts with the constant-based VMT method
calls: use IterResult.GetProperty(PROP_DONE).ToBooleanLiteral.Value to test
iterator completion (instead of
TGocciaBooleanLiteralValue(IterResult.GetProperty('done')).Value) and use
PROP_VALUE (IterResult.GetProperty(PROP_VALUE)) for reading the value; apply
this same change to the other occurrences referenced (the iterator handling,
spread/destructuring areas) so you consistently use PROP_DONE, PROP_VALUE and
.ToBooleanLiteral.Value rather than string literals and direct casts.
In `@units/Goccia.Values.AsyncFunctionValue.pas`:
- Around line 45-46: Add the full ES spec annotation with the method signature
immediately above the implementation of the async function call: replace or
extend the existing "// ES2026 §27.7.5.1 AsyncFunctionStart" comment to the
required format "// ES2026 §27.7.5.1 AsyncFunctionStart
TGocciaAsyncFunctionValue.Call(const AArguments: TGocciaArgumentsCollection;
const AThisValue: TGocciaValue): TGocciaValue" so the comment includes the
method signature exactly; apply the same change for the other two occurrences
referenced (around the implementations at the locations noted, e.g., the methods
found at lines ~94-95 and ~144-145) so every ECMAScript-implemented method has a
spec comment in the format "// ESYYYY §X.Y.Z MethodName(params)" immediately
above its body.
In `@units/Goccia.Values.SymbolValue.pas`:
- Around line 185-194: Add the ECMAScript spec annotation above
TGocciaSymbolValue.WellKnownAsyncIterator by inserting a comment in the format
"// ESYYYY §X.Y.Z Symbol.asyncIterator" immediately above the function
implementation and, if the implementation corresponds to multi-step algorithm
steps, add inline step annotations (e.g., "// Step N: ...") next to the relevant
lines that create FWellKnownAsyncIterator and pin it via
TGocciaGarbageCollector.Instance.PinValue; reference the method name
TGocciaSymbolValue.WellKnownAsyncIterator, the static field
FWellKnownAsyncIterator, and the call to
TGocciaGarbageCollector.Instance.PinValue when adding the annotations.
---
Outside diff comments:
In `@units/Goccia.Builtins.GlobalSymbol.pas`:
- Around line 68-75: Add a PROP_ASYNC_ITERATOR constant to the
Goccia.Constants.PropertyNames unit and replace the hardcoded 'asyncIterator'
literal in units/Goccia.Builtins.GlobalSymbol.pas by using that constant (i.e.
change the FSymbolFunction.RegisterConstant call that currently passes
'asyncIterator' to pass the PropertyNames.PROP_ASYNC_ITERATOR symbol instead);
also update the uses clause in Goccia.Builtins.GlobalSymbol.pas to include the
PropertyNames constants unit so the new constant is visible.
---
Duplicate comments:
In `@tests/language/for-of/basic-for-of.js`:
- Around line 6-47: The tests in describe("basic for...of") use for...of loops
which the current "no-traditional-loops"/GocciaScript loop restriction blocks;
update the rule or linter/configuration to allow ForOfStatement while still
forbidding traditional for/while/do-while loops. Concretely, alter the rule that
enforces "no-traditional-loops" (or the ESLint no-restricted-syntax/no-loops
setting) so it excludes ForOfStatement, or add a test-suite-specific override
for the tests with titles like "iterates over array elements", "iterates over
string characters", and "const binding per iteration" to permit for...of usage.
Ensure only for...of is allowed (keep forbidding ForStatement, WhileStatement,
DoWhileStatement).
In `@tests/language/for-of/for-await-of.js`:
- Around line 6-94: The test suite uses traditional for-await-of loops in the
"for-await-of" describe block and its tests (e.g., the async fn functions that
perform "for await (const item of ...)" and the "break in for-await-of" test);
update the project linting/guideline configuration or add an explicit exception
so these tests are allowed to use for-await-of (either whitelist this
describe/test group in the linter, add a rule exception for "for-await-of"
traditional loops, or update the guideline text to state that for-await-of is
supported), and ensure the test runner/linter respects that change so the tests
under describe("for-await-of") are not flagged as using prohibited traditional
loops.
In `@tests/language/for-of/for-of-break.js`:
- Around line 6-28: The tests under describe("for...of break") (specifically
test("break exits the loop early") and test("break in nested for...of")) are
being flagged by the project rule banning traditional loops; either relax that
rule to permit ES6 for...of loops or add an explicit exception for these test
files: update the loop restriction rule (the "no traditional loops" /
no-traditional-loops lint/test rule or test harness filter) to exclude for...of
constructs or add a whitelist entry for tests/language/for-of/* so for...of is
allowed, ensuring the tests can use for...of semantics with break without
changing the test code.
In `@tests/language/for-of/for-of-iterators.js`:
- Around line 6-46: The tests inside the describe block "for...of with
iterators" (tests "iterates over Set", "iterates over Map", "iterates over
custom iterable") use for...of loops which violate the project guideline banning
traditional loops; either update the lint/test rule to explicitly allow for...of
iterations for iterator-related tests, or rewrite these tests to avoid for...of
by using allowed constructs (e.g., convert iterables to arrays with Array.from
and use array methods like forEach/map) so the behavior is preserved without
using for...of.
---
Nitpick comments:
In `@tests/built-ins/Array/fromAsync-edge-cases.js`:
- Around line 179-193: The iterator object returned by the
[Symbol.asyncIterator] method contains a duplicate next() definition (the first
next() is immediately overwritten by the second), so remove the redundant
earlier next() and keep the intended async iterator implementation that uses
this.count and returns Promise.resolve({ value: ..., done: ... }); update the
iterator literal inside the [Symbol.asyncIterator]() function to have a single
next method (the one that increments this.count and signals done).
In `@units/Goccia.Parser.pas`:
- Around line 367-374: Add an ECMAScript spec annotation comment for the
await-expression handling: above the await-handling block that checks
FInAsyncFunction and creates TGocciaAwaitExpression (the code that advances the
'await' token, sets Right := Unary and constructs TGocciaAwaitExpression) add a
comment like "// ES2017 §14.8.2" (or the correct ES year/section) and, if
helpful, add inline step annotations for the operator consumption and node
creation to reflect the spec steps; ensure the chosen section number matches the
ECMAScript spec for Await Expressions.
- Around line 669-676: The Parameters[0] record is missing initialization of its
Pattern field which other initialization sites set to nil when IsPattern is
False; update the constructor/initializer that sets Parameters[0].Name,
DefaultValue, IsPattern, IsRest, IsOptional and TypeAnnotation so it also sets
Parameters[0].Pattern := nil when IsPattern is False to match the behavior of
the other initialization path (i.e., ensure Parameters[0].Pattern is explicitly
nil).
- Line 653: The condition uses direct access FTokens[FCurrent + 1].TokenType
which can go out of bounds; replace that check with CheckNext(gttArrow) for
safety and consistency: keep the existing (Name = KEYWORD_ASYNC) and
Check(gttIdentifier) parts but change the final clause to CheckNext(gttArrow) so
the parser uses the same bounds-checked helper instead of directly indexing
FTokens/FCurrent.
- Around line 1555-1556: Add ECMAScript spec annotations to the for-statement
parser: place a comment above the TGocciaParser.ForStatement function body
referencing the relevant ECMAScript clauses for for-of and for-await-of (for-of:
// ES2015 §13.7.5 (or the exact clause in your ECMAScript reference);
for-await-of: // ES2018 §13.7.6), and add inline spec comments at key
algorithmic steps inside the function where the parser distinguishes between
classic for, for-of, and for-await-of heads (e.g., token detection of "await",
evaluation of iterable, and iterator/async-iterator handling) using the same //
ESYYYY §X.Y.Z format to map code to the spec.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
AGENTS.mdbenchmarks/async-await.jsbenchmarks/for-of.jsdocs/built-ins.mddocs/language-restrictions.mddocs/testing.mddocs/value-system.mdtests/built-ins/Array/fromAsync-edge-cases.jstests/built-ins/Array/fromAsync.jstests/built-ins/Symbol/asyncIterator.jstests/language/async-await/async-class-methods.jstests/language/async-await/async-error-handling.jstests/language/async-await/async-hooks.jstests/language/async-await/async-object-methods.jstests/language/async-await/async-test-callbacks.jstests/language/async-await/await-edge-cases.jstests/language/async-await/await-expression.jstests/language/async-await/basic-async.jstests/language/async-await/expect-resolves-rejects.jstests/language/for-of/basic-for-of.jstests/language/for-of/for-await-of-edge-cases.jstests/language/for-of/for-await-of.jstests/language/for-of/for-of-break.jstests/language/for-of/for-of-destructuring.jstests/language/for-of/for-of-edge-cases.jstests/language/for-of/for-of-iterators.jsunits/Goccia.AST.Expressions.pasunits/Goccia.AST.Statements.pasunits/Goccia.Builtins.GlobalArray.pasunits/Goccia.Builtins.GlobalSymbol.pasunits/Goccia.Builtins.TestAssertions.pasunits/Goccia.Constants.PropertyNames.pasunits/Goccia.Evaluator.pasunits/Goccia.Keywords.Contextual.pasunits/Goccia.Parser.pasunits/Goccia.Values.AsyncFunctionValue.pasunits/Goccia.Values.SymbolValue.pasvitest.config.js
Comment on lines
+185
to
+194
| class function TGocciaSymbolValue.WellKnownAsyncIterator: TGocciaSymbolValue; | ||
| begin | ||
| if not Assigned(FWellKnownAsyncIterator) then | ||
| begin | ||
| FWellKnownAsyncIterator := TGocciaSymbolValue.Create('Symbol.asyncIterator'); | ||
| if Assigned(TGocciaGarbageCollector.Instance) then | ||
| TGocciaGarbageCollector.Instance.PinValue(FWellKnownAsyncIterator); | ||
| end; | ||
| Result := FWellKnownAsyncIterator; | ||
| end; |
Contributor
There was a problem hiding this comment.
Add ES spec annotation for WellKnownAsyncIterator.
This implements an ECMAScript-specified well‑known symbol; add the required // ESYYYY §X.Y.Z Symbol.asyncIterator comment above the method and inline step annotations if applicable.
As per coding guidelines "When implementing ECMAScript-specified behavior, annotate each function or method with a comment referencing the relevant spec section using the format // ESYYYY §X.Y.Z MethodName(params) immediately above the function body in the implementation section. For multi-step algorithms, annotate individual steps inline."
🤖 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 185 - 194, Add the
ECMAScript spec annotation above TGocciaSymbolValue.WellKnownAsyncIterator by
inserting a comment in the format "// ESYYYY §X.Y.Z Symbol.asyncIterator"
immediately above the function implementation and, if the implementation
corresponds to multi-step algorithm steps, add inline step annotations (e.g.,
"// Step N: ...") next to the relevant lines that create FWellKnownAsyncIterator
and pin it via TGocciaGarbageCollector.Instance.PinValue; reference the method
name TGocciaSymbolValue.WellKnownAsyncIterator, the static field
FWellKnownAsyncIterator, and the call to
TGocciaGarbageCollector.Instance.PinValue when adding the annotations.
Contributor
Benchmark Results216 benchmarks · 🟢 37 improved · 🔴 63 regressed · 🆕 13 new · 103 unchanged · avg +42.3% arrays.js — 🟢 11 improved, 8 unchanged · avg +75.9%
async-await.js — 🆕 6 new
classes.js — 🔴 2 regressed, 29 unchanged · avg -4.6%
closures.js — 🔴 6 regressed, 5 unchanged · avg -6.5%
collections.js — 🟢 11 improved, 1 unchanged · avg +513.3%
destructuring.js — 🟢 5 improved, 🔴 2 regressed, 15 unchanged · avg +42.5%
fibonacci.js — 🔴 1 regressed, 5 unchanged · avg -2.0%
for-of.js — 🆕 7 new
iterators.js — 🟢 5 improved, 🔴 2 regressed, 13 unchanged · avg +23.4%
json.js — 🟢 2 improved, 🔴 1 regressed, 17 unchanged · avg +9.6%
jsx.jsx — 🔴 18 regressed, 3 unchanged · avg -9.3%
numbers.js — 🔴 7 regressed, 4 unchanged · avg -8.3%
objects.js — 🟢 3 improved, 🔴 4 regressed · avg +21.4%
promises.js — 🔴 12 regressed · avg -13.3%
strings.js — 🔴 8 regressed, 3 unchanged · avg -8.3%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
units/Goccia.Values.SymbolValue.pas (1)
185-187: Add the method signature to the ES annotation.Line 185’s spec comment still omits the
MethodName(params)portion; please include the fullTGocciaSymbolValue.WellKnownAsyncIteratorsignature.✍️ Suggested comment update
-// ES2026 §20.1.2.1 Symbol.asyncIterator +// ES2026 §20.1.2.1 Symbol.asyncIterator TGocciaSymbolValue.WellKnownAsyncIterator: TGocciaSymbolValueAs per coding guidelines "When implementing ECMAScript-specified behavior, annotate each function or method with a comment referencing the relevant spec section using the format
// ESYYYY §X.Y.Z MethodName(params)immediately above the function body in the implementation section."🤖 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 185 - 187, Update the ES annotation comment above the TGocciaSymbolValue.WellKnownAsyncIterator implementation to include the full method signature (MethodName(params)) — change the existing "// ES2026 §20.1.2.1 Symbol.asyncIterator" to include the method signature, e.g. "// ES2026 §20.1.2.1 TGocciaSymbolValue.WellKnownAsyncIterator()" so the comment follows the required `// ESYYYY §X.Y.Z MethodName(params)` format and directly annotates the TGocciaSymbolValue.WellKnownAsyncIterator class function.units/Goccia.Values.AsyncFunctionValue.pas (1)
45-46: ES annotations for async Call overrides need full method signatures.The AsyncFunctionStart comments still omit the required
MethodName(params)format on all threeCalloverrides.✍️ Example update (apply to all three)
-// ES2026 §27.7.5.1 AsyncFunctionStart(promiseCapability, asyncFunctionBody) +// ES2026 §27.7.5.1 AsyncFunctionStart TGocciaAsyncFunctionValue.Call(const AArguments: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValueAs per coding guidelines "When implementing ECMAScript-specified behavior, annotate each function or method with a comment referencing the relevant spec section using the format
// ESYYYY §X.Y.Z MethodName(params)immediately above the function body in the implementation section."Also applies to: 94-95, 144-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.AsyncFunctionValue.pas` around lines 45 - 46, Update the ES annotation above each TGocciaAsyncFunctionValue.Call override to include the full method signature in the required format; locate the three Call method implementations (the overrides at the top, near lines ~45-46, ~94-95 and ~144-145) and replace the current comment with the spec reference using the MethodName(params) form, e.g. "// ES2026 §27.7.5.1 AsyncFunctionStart(promiseCapability, asyncFunctionBody)" immediately above each function implementation.units/Goccia.Builtins.TestAssertions.pas (1)
236-240: 🛠️ Refactor suggestion | 🟠 MajorUse PropertyNames constants for
resolves/rejects.These new property keys should come from
Goccia.Constants.PropertyNamesinstead of hardcoded strings; please add constants (e.g.,PROP_RESOLVES,PROP_REJECTS) and use them here.♻️ Example usage (after adding constants)
- DefineProperty('resolves', TGocciaPropertyDescriptorAccessor.Create( + DefineProperty(PROP_RESOLVES, TGocciaPropertyDescriptorAccessor.Create( TGocciaNativeFunctionValue.Create(GetResolves, 'resolves', 0), nil, [pfConfigurable])); - DefineProperty('rejects', TGocciaPropertyDescriptorAccessor.Create( + DefineProperty(PROP_REJECTS, TGocciaPropertyDescriptorAccessor.Create( TGocciaNativeFunctionValue.Create(GetRejects, 'rejects', 0), nil, [pfConfigurable]));As per coding guidelines "Use split constant units instead of hardcoded string literals for property names, type names, error names, and constructor names:
Goccia.Constants.PropertyNames,Goccia.Constants.TypeNames,Goccia.Constants.ErrorNames,Goccia.Constants.ConstructorNames,Goccia.Constants."🤖 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 236 - 240, Replace the hardcoded 'resolves' and 'rejects' property keys with constants from Goccia.Constants.PropertyNames: add PROP_RESOLVES and PROP_REJECTS to that unit and use them in the DefineProperty calls instead of the string literals; update the two DefineProperty invocations that create TGocciaPropertyDescriptorAccessor wrapping TGocciaNativeFunctionValue(Create(GetResolves, 'resolves', 0) ...) and Create(GetRejects, 'rejects', 0) to reference the new constants (e.g., PropertyNames.PROP_RESOLVES and PropertyNames.PROP_REJECTS) so the property names follow the project-wide constant usage convention.units/Goccia.Builtins.GlobalArray.pas (1)
511-531:⚠️ Potential issue | 🟠 MajorRoot sync iterators while
AwaitValuecan drain microtasks.In the sync-iterator fallback,
Iteratoris only held in a local whileAwaitValuecan drain the microtask queue, so GC could collect it mid-loop. Add a temp root for the iterator for the duration of the loop.🛡️ Suggested fix
if Assigned(Iterator) then begin + if Assigned(GC) then + GC.AddTempRoot(Iterator); + try K := 0; IterResult := Iterator.AdvanceNext; while not IterResult.GetProperty(PROP_DONE).ToBooleanLiteral.Value do begin KValue := IterResult.GetProperty(PROP_VALUE); KValue := AwaitValue(KValue); @@ IterResult := Iterator.AdvanceNext; end; + finally + if Assigned(GC) then + GC.RemoveTempRoot(Iterator); + end; if UseConstructor and not (ResultObj is TGocciaArrayValue) then ResultObj.SetProperty(PROP_LENGTH, TGocciaNumberLiteralValue.SmallInt(K)); endAs 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.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.GlobalArray.pas` around lines 511 - 531, The sync-iterator loop can lose the Iterator to GC because AwaitValue can run microtasks; wrap the Iterator in a temporary root for the loop lifetime by calling AddTempRoot(Iterator) before entering the while loop and RemoveTempRoot(Iterator) after the loop (ensure both on all control paths), keeping all existing uses of Iterator, IterResult, AwaitValue, MapArgs, MapCallback and AddElement unchanged; this ensures the Iterator remains rooted while AwaitValue is called.units/Goccia.Evaluator.pas (1)
1298-1299:⚠️ Potential issue | 🟡 MinorSpec annotations must include the Pascal method signature.
These ES annotations still omit the required
MethodName(params)format. Please include the concrete function signature in each comment.♻️ Suggested update
-// ES2026 §27.7.5.3 Await(value) +// ES2026 §27.7.5.3 EvaluateAwait(const AAwaitExpression: TGocciaAwaitExpression; const AContext: TGocciaEvaluationContext): TGocciaValue @@ -// ES2026 §14.7.5.6 ForIn/OfBodyEvaluation(lhs, stmt, iteratorRecord, iterationKind, lhsKind) +// ES2026 §14.7.5.6 EvaluateForOf(const AForOfStatement: TGocciaForOfStatement; const AContext: TGocciaEvaluationContext): TGocciaValue @@ -// ES2026 §14.7.5.6 ForIn/OfBodyEvaluation — for-await-of variant +// ES2026 §14.7.5.6 EvaluateForAwaitOf(const AForAwaitOfStatement: TGocciaForAwaitOfStatement; const AContext: TGocciaEvaluationContext): TGocciaValueAs per coding guidelines, “When implementing ECMAScript-specified behavior, annotate each function or method with a comment referencing the relevant spec section using the format
// ESYYYY §X.Y.Z MethodName(params)immediately above the function body in the implementation section.”Also applies to: 1370-1371, 1430-1431
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Evaluator.pas` around lines 1298 - 1299, The ES spec comment above EvaluateAwait is missing the required Pascal method signature format; change the annotation to include the concrete function signature (for example: "// ES2026 §27.7.5.3 Await(value) EvaluateAwait(const AAwaitExpression: TGocciaAwaitExpression; const AContext: TGocciaEvaluationContext): TGocciaValue") and apply the same fix to the other two annotations around EvaluateYield/other functions referenced at the similar locations (lines noted in the review: near 1370-1371 and 1430-1431) so every ECMAScript spec comment uses "Section MethodName(params)" with the exact Pascal method declaration above the corresponding function implementation.
🧹 Nitpick comments (1)
benchmarks/for-of.js (1)
30-52: Hoist collection setup to isolate iteration cost.Creating
Set,Map, andpairsinside the bench mixes allocation with iteration, which can skew the iteration benchmark.♻️ Suggested refactor
suite("for...of", () => { const smallArray = Array.from({ length: 10 }, (_, i) => i); const mediumArray = Array.from({ length: 100 }, (_, i) => i); + const smallSet = new Set(smallArray); + const smallMap = new Map(smallArray.map((v) => [v, v * 2])); + const pairs = smallArray.map((v) => [v, v * 10]); bench("for...of with Set (10 elements)", () => { - const s = new Set(smallArray); let sum = 0; - for (const item of s) { + for (const item of smallSet) { sum = sum + item; } }); bench("for...of with Map entries (10 entries)", () => { - const m = new Map(smallArray.map((v) => [v, v * 2])); let sum = 0; - for (const [k, v] of m) { + for (const [k, v] of smallMap) { sum = sum + k + v; } }); bench("for...of with destructuring", () => { - const pairs = smallArray.map((v) => [v, v * 10]); let sum = 0; for (const [a, b] of pairs) { sum = sum + a + b; } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/for-of.js` around lines 30 - 52, The benchmark currently creates collections inside each bench callback (see bench(...) blocks using new Set(smallArray), new Map(...), and pairs = smallArray.map(...)), which mixes allocation cost with iteration; move the setup out of the bench bodies so only the iteration runs in the timed closure: hoist creation of s = new Set(smallArray), m = new Map(...), and pairs = smallArray.map(...) to before the respective bench(...) calls and leave each bench callback containing only the for...of loops (for (const item of s), for (const [k,v] of m), for (const [a,b] of pairs)) and the sum accumulation to isolate iteration cost.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 306-312: The markdown list under “Runtime constants” has one extra
leading space causing MD005; fix it by normalizing the sub-list indentation so
all bullet lines under that heading start at the same column; update the lines
listing Goccia.Constants.PropertyNames (PROP_LENGTH, PROP_NAME, ...),
Goccia.Constants.TypeNames (OBJECT_TYPE_NAME, ...), Goccia.Constants.ErrorNames,
Goccia.Constants.ConstructorNames, Goccia.Constants.SymbolNames and
Goccia.Constants literals to remove the stray space before the hyphen so every
list item is aligned consistently with the other bullets.
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 1058-1088: The promise object can be collected during the
microtask drain because it is only referenced by Pascal locals; in
TGocciaExpectationValue.GetResolves (and likewise GetRejects) wrap the promise
root with AddTempRoot before calling TGocciaMicrotaskQueue.Instance.DrainQueue
and RemoveTempRoot immediately after to protect FActualValue/Promise;
specifically, call AddTempRoot(FActualValue) (or AddTempRoot(Promise) after
casting) prior to DrainQueue and ensure RemoveTempRoot is called in all control
paths (fulfilled, rejected, pending) so the temporary root is always removed.
In `@units/Goccia.Parser.pas`:
- Around line 1556-1558: Update the ES annotation above the ForStatement
implementation to include the method signature; replace the current "// ES2026
§14.7.5 ForIn/OfStatement" with a single-line comment in the required format
that names the method and its signature, e.g. "// ES2026 §14.7.5
ForIn/OfStatement ForStatement(): TGocciaStatement", placed immediately above
the function TGocciaParser.ForStatement so the annotation matches the function
declaration.
- Around line 1576-1581: The parser currently allows `await` in the `for
await...of` header unconditionally; change the guard so `await` is only accepted
when inside an async context by checking `FInAsyncFunction > 0` before treating
`Peek.Lexeme = KEYWORD_AWAIT` as valid. In the `for-await-of` parsing block that
sets IsAwait and calls Advance, add the same async-context check used in the
`UnaryExpression` code path (i.e. require `FInAsyncFunction > 0` in addition to
`Check(gttIdentifier)` and `Peek.Lexeme = KEYWORD_AWAIT`); if the check fails,
do not set IsAwait and instead emit the existing invalid-syntax handling so
non-async contexts reject `await` properly.
In `@units/Goccia.Values.AsyncFunctionValue.pas`:
- Around line 81-89: The CloneWithNewScope method
(TGocciaAsyncFunctionValue.CloneWithNewScope) and the other clone path around
lines 130-138 re-specialize TObjectList<TGocciaASTNode>; replace that local
specialization with the shared alias defined in the AST unit (e.g., the named
alias for TObjectList<TGocciaASTNode]) so both the variable declaration and the
Create call use that alias instead of TObjectList<TGocciaASTNode>; keep the same
ownership flag (False) and the existing Add loop to copy references, and add the
AST unit to the uses clause if not already present so the alias is available.
---
Duplicate comments:
In `@units/Goccia.Builtins.GlobalArray.pas`:
- Around line 511-531: The sync-iterator loop can lose the Iterator to GC
because AwaitValue can run microtasks; wrap the Iterator in a temporary root for
the loop lifetime by calling AddTempRoot(Iterator) before entering the while
loop and RemoveTempRoot(Iterator) after the loop (ensure both on all control
paths), keeping all existing uses of Iterator, IterResult, AwaitValue, MapArgs,
MapCallback and AddElement unchanged; this ensures the Iterator remains rooted
while AwaitValue is called.
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 236-240: Replace the hardcoded 'resolves' and 'rejects' property
keys with constants from Goccia.Constants.PropertyNames: add PROP_RESOLVES and
PROP_REJECTS to that unit and use them in the DefineProperty calls instead of
the string literals; update the two DefineProperty invocations that create
TGocciaPropertyDescriptorAccessor wrapping
TGocciaNativeFunctionValue(Create(GetResolves, 'resolves', 0) ...) and
Create(GetRejects, 'rejects', 0) to reference the new constants (e.g.,
PropertyNames.PROP_RESOLVES and PropertyNames.PROP_REJECTS) so the property
names follow the project-wide constant usage convention.
In `@units/Goccia.Evaluator.pas`:
- Around line 1298-1299: The ES spec comment above EvaluateAwait is missing the
required Pascal method signature format; change the annotation to include the
concrete function signature (for example: "// ES2026 §27.7.5.3 Await(value)
EvaluateAwait(const AAwaitExpression: TGocciaAwaitExpression; const AContext:
TGocciaEvaluationContext): TGocciaValue") and apply the same fix to the other
two annotations around EvaluateYield/other functions referenced at the similar
locations (lines noted in the review: near 1370-1371 and 1430-1431) so every
ECMAScript spec comment uses "Section MethodName(params)" with the exact Pascal
method declaration above the corresponding function implementation.
In `@units/Goccia.Values.AsyncFunctionValue.pas`:
- Around line 45-46: Update the ES annotation above each
TGocciaAsyncFunctionValue.Call override to include the full method signature in
the required format; locate the three Call method implementations (the overrides
at the top, near lines ~45-46, ~94-95 and ~144-145) and replace the current
comment with the spec reference using the MethodName(params) form, e.g. "//
ES2026 §27.7.5.1 AsyncFunctionStart(promiseCapability, asyncFunctionBody)"
immediately above each function implementation.
In `@units/Goccia.Values.SymbolValue.pas`:
- Around line 185-187: Update the ES annotation comment above the
TGocciaSymbolValue.WellKnownAsyncIterator implementation to include the full
method signature (MethodName(params)) — change the existing "// ES2026 §20.1.2.1
Symbol.asyncIterator" to include the method signature, e.g. "// ES2026 §20.1.2.1
TGocciaSymbolValue.WellKnownAsyncIterator()" so the comment follows the required
`// ESYYYY §X.Y.Z MethodName(params)` format and directly annotates the
TGocciaSymbolValue.WellKnownAsyncIterator class function.
---
Nitpick comments:
In `@benchmarks/for-of.js`:
- Around line 30-52: The benchmark currently creates collections inside each
bench callback (see bench(...) blocks using new Set(smallArray), new Map(...),
and pairs = smallArray.map(...)), which mixes allocation cost with iteration;
move the setup out of the bench bodies so only the iteration runs in the timed
closure: hoist creation of s = new Set(smallArray), m = new Map(...), and pairs
= smallArray.map(...) to before the respective bench(...) calls and leave each
bench callback containing only the for...of loops (for (const item of s), for
(const [k,v] of m), for (const [a,b] of pairs)) and the sum accumulation to
isolate iteration cost.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
AGENTS.mdbenchmarks/for-of.jstests/built-ins/Array/fromAsync-edge-cases.jstests/language/async-await/expect-resolves-rejects.jsunits/Goccia.Builtins.GlobalArray.pasunits/Goccia.Builtins.GlobalSymbol.pasunits/Goccia.Builtins.TestAssertions.pasunits/Goccia.Constants.SymbolNames.pasunits/Goccia.Evaluator.pasunits/Goccia.Parser.pasunits/Goccia.Values.AsyncFunctionValue.pasunits/Goccia.Values.SymbolValue.pas
Comment on lines
+1556
to
1558
| // ES2026 §14.7.5 ForIn/OfStatement | ||
| function TGocciaParser.ForStatement: TGocciaStatement; | ||
| var |
Contributor
There was a problem hiding this comment.
Include the method signature in the ForStatement ES annotation.
Line 1556 should follow the // ESYYYY §X.Y.Z MethodName(params) format.
✍️ Suggested comment update
-// ES2026 §14.7.5 ForIn/OfStatement
+// ES2026 §14.7.5 ForIn/OfStatement TGocciaParser.ForStatement: TGocciaStatementAs per coding guidelines "When implementing ECMAScript-specified behavior, annotate each function or method with a comment referencing the relevant spec section using the format // ESYYYY §X.Y.Z MethodName(params) immediately above the function body in the implementation section."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Parser.pas` around lines 1556 - 1558, Update the ES annotation
above the ForStatement implementation to include the method signature; replace
the current "// ES2026 §14.7.5 ForIn/OfStatement" with a single-line comment in
the required format that names the method and its signature, e.g. "// ES2026
§14.7.5 ForIn/OfStatement ForStatement(): TGocciaStatement", placed immediately
above the function TGocciaParser.ForStatement so the annotation matches the
function declaration.
Comment on lines
+81
to
+89
| function TGocciaAsyncFunctionValue.CloneWithNewScope(const ANewScope: TGocciaScope): TGocciaFunctionValue; | ||
| var | ||
| ClonedStatements: TObjectList<TGocciaASTNode>; | ||
| I: Integer; | ||
| begin | ||
| ClonedStatements := TObjectList<TGocciaASTNode>.Create(False); | ||
| for I := 0 to FBodyStatements.Count - 1 do | ||
| ClonedStatements.Add(FBodyStatements[I]); | ||
| Result := TGocciaAsyncFunctionValue.Create(FParameters, ClonedStatements, ANewScope, FName); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the shared AST node list alias instead of re‑specializing TObjectList<TGocciaASTNode>.
Both clone paths re-specialize the generic list. Please use the alias defined in the AST unit (or add one there) and reference it here to comply with the generic-list guideline.
As per coding guidelines "When a generic list specialization TObjectList<T> is used across multiple units, define a named type alias in the unit that declares T (e.g., TGocciaValueList = TObjectList<TGocciaValue>). All consumers must use the alias — never re-specialize locally."
Also applies to: 130-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Values.AsyncFunctionValue.pas` around lines 81 - 89, The
CloneWithNewScope method (TGocciaAsyncFunctionValue.CloneWithNewScope) and the
other clone path around lines 130-138 re-specialize TObjectList<TGocciaASTNode>;
replace that local specialization with the shared alias defined in the AST unit
(e.g., the named alias for TObjectList<TGocciaASTNode]) so both the variable
declaration and the Create call use that alias instead of
TObjectList<TGocciaASTNode>; keep the same ownership flag (False) and the
existing Add loop to copy references, and add the AST unit to the uses clause if
not already present so the alias is available.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
units/Goccia.Parser.pas (1)
1556-1557:⚠️ Potential issue | 🟡 MinorAdd the method signature to the ES annotation above
ForStatement.✍️ Suggested comment update
-// ES2026 §14.7.5 ForIn/OfStatement +// ES2026 §14.7.5 ForIn/OfStatement TGocciaParser.ForStatement: TGocciaStatementAs per coding guidelines "When implementing ECMAScript-specified behavior, annotate each function or method with a comment referencing the relevant spec section using the format
// ESYYYY §X.Y.Z MethodName(params)immediately above the function body in theimplementationsection."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Parser.pas` around lines 1556 - 1557, The ES annotation above the ForStatement function is missing the method signature; update the comment to follow the project's spec-format by adding the signature after the spec reference (e.g. "// ES2026 §14.7.5 ForIn/OfStatement TGocciaParser.ForStatement: TGocciaStatement") immediately above the ForStatement implementation in the implementation section so it matches the required `// ESYYYY §X.Y.Z MethodName(params)` style and references TGocciaParser.ForStatement.units/Goccia.Evaluator.pas (1)
1304-1306:⚠️ Potential issue | 🟡 MinorSpec annotations should include the Pascal method signature.
These ES comments still omit the
MethodName(params)portion required by the guideline. Please update all three.✍️ Suggested format
-// ES2026 §27.7.5.3 Await(value) +// ES2026 §27.7.5.3 EvaluateAwait(const AAwaitExpression: TGocciaAwaitExpression; const AContext: TGocciaEvaluationContext): TGocciaValue -// ES2026 §14.7.5.6 ForIn/OfBodyEvaluation(lhs, stmt, iteratorRecord, iterationKind, lhsKind) +// ES2026 §14.7.5.6 EvaluateForOf(const AForOfStatement: TGocciaForOfStatement; const AContext: TGocciaEvaluationContext): TGocciaValue -// ES2026 §14.7.5.6 ForIn/OfBodyEvaluation — for-await-of variant +// ES2026 §14.7.5.6 EvaluateForAwaitOf(const AForAwaitOfStatement: TGocciaForAwaitOfStatement; const AContext: TGocciaEvaluationContext): TGocciaValueAs per coding guidelines, "When implementing ECMAScript-specified behavior, annotate each function or method with a comment referencing the relevant spec section using the format
// ESYYYY §X.Y.Z MethodName(params)immediately above the function body in the implementation section."Also applies to: 1376-1378, 1436-1438
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Evaluator.pas` around lines 1304 - 1306, The ES comment above EvaluateAwait (and the two other ECMAScript-annotated functions noted) is missing the Pascal method signature; update each ES spec annotation immediately above the function implementation to include the full Pascal method signature (function name with parameter list and return type) — e.g. replace or augment "// ES2026 §27.7.5.3 ..." with a line that includes the method signature for EvaluateAwait (EvaluateAwait(const AAwaitExpression: TGocciaAwaitExpression; const AContext: TGocciaEvaluationContext): TGocciaValue) and do the same for the other two functions referenced in the review so each spec comment follows the required "ESYYYY §X.Y.Z MethodName(params)" format.
🧹 Nitpick comments (7)
AGENTS.md (1)
419-420: Async callback support forbench()not mentioned in the benchmark section.Line 481 states that "benchmark callbacks (
bench()) also supportasyncfunctions withawait", but thebench()API description here only documents the synchronous{ setup?, run, teardown? }shape. An agent reading only the Built-in Objects section would have no indication that these callbacks can beasync.📝 Suggested addition
The `bench()` API takes a name and an options object: `bench(name, { setup?, run, teardown? })`. The `setup` function runs once before warmup and its return value is passed to `run` and `teardown`. All three phases are independently timed and reported (`setupMs`, `teardownMs`). +All three callback phases (`setup`, `run`, `teardown`) support `async` functions — `await` works inside each phase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 419 - 420, The docs for BenchmarkRunner/ggBenchmark describe bench(name, { setup?, run, teardown? }) but omit that the callbacks (setup, run, teardown) may be async and can use await; update the BenchmarkRunner/ggBenchmark section to explicitly state that bench() supports async functions for setup, run, and teardown, that the value returned/resolved by async setup is passed to run/teardown, and that each phase (setupMs, runMs, teardownMs) times the awaited completion; reference the bench() API, the setup/run/teardown callbacks, and BenchmarkRunner/ggBenchmark in the text so readers know where async behavior is supported.units/Goccia.Builtins.GlobalArray.pas (2)
20-21: StandardizefromAsyncargument checks withTGocciaArgumentValidator.
This keeps arity/type errors consistent across built-ins and centralizes validation logic.As per coding guidelines "Use the
TGocciaArgumentValidatorfromGoccia.Arguments.Validator.pasfor standardized argument count and type validation."Also applies to: 315-398
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.GlobalArray.pas` around lines 20 - 21, The ArrayFromAsync and ArrayOf built-ins currently perform ad-hoc argument/arity checks; replace those checks with the centralized TGocciaArgumentValidator usage from Goccia.Arguments.Validator.pas to standardize errors and behavior. Update the implementations of ArrayFromAsync and ArrayOf (and similarly the other built-ins in the 315–398 region) to construct a TGocciaArgumentValidator, call its ValidateCount/ValidateType (or equivalent) for the expected arity and types, and use its validated/accessor methods to obtain arguments instead of manual checks; ensure you add the unit reference for Goccia.Arguments.Validator and preserve existing semantics while delegating validation to the validator so error messages and arity handling are consistent.
55-58: Use a property-name constant forfromAsync.
Avoid hardcoded built-in property names when registering natives.As per coding guidelines "Use split constant units instead of hardcoded string literals for property names, type names, error names, and constructor names:
Goccia.Constants.PropertyNames,Goccia.Constants.TypeNames,Goccia.Constants.ErrorNames,Goccia.Constants.ConstructorNames,Goccia.Constants."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.GlobalArray.pas` around lines 55 - 58, Replace the hardcoded 'fromAsync' property string when registering the native method in FBuiltinObject.RegisterNativeMethod: instead of TGocciaNativeFunctionValue.Create(ArrayFromAsync, 'fromAsync', 1) use the property-name constant from Goccia.Constants.PropertyNames (e.g., PropertyNames.FromAsync) and update the unit uses clause to include Goccia.Constants so the call becomes TGocciaNativeFunctionValue.Create(ArrayFromAsync, PropertyNames.FromAsync, 1); leave the other registrations unchanged.units/Goccia.Parser.pas (1)
35-36: Run the native Pascal test suite for parser/AST changes.
Please run:./build.pas clean tests && for t in build/Goccia.*.Test; do "$t"; doneAs per coding guidelines "When modifying AST logic, scope chain, evaluator, or value types, build and run the native Pascal test suite with
./build.pas clean tests && for t in build/Goccia.*.Test; do "$t"; doneand updateunits/*.Test.pasto reflect any changes in behavior."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Parser.pas` around lines 35 - 36, Run the native Pascal parser/AST test suite using "./build.pas clean tests && for t in build/Goccia.*.Test; do "$t"; done"; if any tests fail due to your changes around the AST/scope chain in units/Goccia.Parser.pas (notably the FInAsyncFunction field), update the corresponding units/*.Test.pas expectations to match the new behavior or adjust the parser logic to preserve previous semantics; re-run the command until all tests pass and commit the updated test files alongside the code change.units/Goccia.Builtins.Benchmark.pas (2)
195-216: UseTGocciaArgumentValidatorforbenchinputs.
It standardizes argument count/type checks and error reporting across built-ins.As per coding guidelines "Use the
TGocciaArgumentValidatorfromGoccia.Arguments.Validator.pasfor standardized argument count and type validation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.Benchmark.pas` around lines 195 - 216, Replace the manual argument checks in the bench registration block with TGocciaArgumentValidator: validate that there are exactly 2 args and that arg0 is a TGocciaStringLiteralValue and arg1 is a TGocciaObjectValue using TGocciaArgumentValidator APIs (from Goccia.Arguments.Validator.pas), then extract BenchName, OptionsObj, and the Run/Setup/Teardown properties as before (RunFn, SetupFn, TeardownFn) and call FRegisteredBenchmarks.Add(TBenchmarkCase.Create(...)) — this centralizes error reporting and removes the handcrafted if/Exit checks around BenchName, OptionsObj, RunProp/RunFn, SetupProp/SetupFn, and TeardownProp/TeardownFn.
202-214: Replace hardcoded benchmark property names with constants.
This includesrun,setup,teardown,setupMs, andteardownMs.As per coding guidelines "Use split constant units instead of hardcoded string literals for property names, type names, error names, and constructor names:
Goccia.Constants.PropertyNames,Goccia.Constants.TypeNames,Goccia.Constants.ErrorNames,Goccia.Constants.ConstructorNames,Goccia.Constants."Also applies to: 452-453
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.Benchmark.pas` around lines 202 - 214, The code uses hardcoded property name strings ("run", "setup", "teardown", and also "setupMs"/"teardownMs" elsewhere) when calling OptionsObj.GetProperty and when checking function types; replace those literal strings with the corresponding constants from Goccia.Constants.PropertyNames (e.g. PropertyNames.Run, PropertyNames.Setup, PropertyNames.Teardown, PropertyNames.SetupMs, PropertyNames.TeardownMs) and keep the existing checks and casts (RunProp, RunFn, SetupProp, SetupFn, TeardownProp, TeardownFn, OptionsObj.GetProperty) unchanged otherwise so callers read from the constant values instead of raw string literals.BenchmarkRunner.dpr (1)
101-113: Use property-name constants forsetupMs/teardownMsaccess.
Keeps built-in property names centralized and consistent.As per coding guidelines "Use split constant units instead of hardcoded string literals for property names, type names, error names, and constructor names:
Goccia.Constants.PropertyNames,Goccia.Constants.TypeNames,Goccia.Constants.ErrorNames,Goccia.Constants.ConstructorNames,Goccia.Constants."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BenchmarkRunner.dpr` around lines 101 - 113, Replace hardcoded property name strings 'setupMs' and 'teardownMs' used in the SingleResult.GetProperty calls with the centralized property-name constants from Goccia.Constants.PropertyNames; update the expressions that assign Entry.SetupMs and Entry.TeardownMs (the lines using SingleResult.GetProperty('setupMs').ToNumberLiteral.Value and SingleResult.GetProperty('teardownMs').ToNumberLiteral.Value) to use Goccia.Constants.PropertyNames.<appropriateName> instead, keeping the rest of the call chain (ToNumberLiteral.Value) unchanged and ensuring you import or qualify Goccia.Constants if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/collections.js`:
- Around line 59-115: The forEach callback in the "set 50 entries" bench is
using an implicit return (items.forEach((i) => m.set("key" + i, i))) which trips
the linter; change the callback to a block-bodied arrow function so it does not
return a value (update the run callback inside the bench "set 50 entries" where
items.forEach is called, replacing the expression-bodied arrow with a
brace-bodied arrow function that calls m.set("key"+i, i) inside).
- Around line 5-57: The lint error comes from using an implicit return in the
forEach arrow callback in the "add 50 elements" bench; change the callback at
items.forEach((i) => s.add(i)); to use a block-bodied arrow function so it
doesn't return a value (e.g., items.forEach((i) => { s.add(i); });) — locate
this inside the bench run for "add 50 elements" where items.forEach is used and
update the callback accordingly.
In `@docs/benchmarks.md`:
- Around line 89-93: The markdown code fence containing the data-flow diagram
should specify a language to satisfy MD040; change the opening triple-backticks
from ``` to ```text and leave the diagram content unchanged (the block starting
with "setup() → [return value] → warmup(run × N) → calibrate(run × N) →
measure(run × N × rounds) → teardown()" and the following arrow/metrics line) so
the fence becomes ```text ... ```.
In `@units/Goccia.Builtins.GlobalArray.pas`:
- Around line 543-567: Cache the length property value from
Source.GetProperty(PROP_LENGTH) once before the loop instead of calling
Source.GetProperty(PROP_LENGTH) repeatedly: retrieve the PROP_LENGTH property
once (checking Assigned and not TGocciaUndefinedLiteralValue), call AwaitValue
on it if necessary, convert it to a numeric value (as currently done via
ToNumberLiteral.Value) into a local variable (e.g., Len), then use Len in the
Trunc(...) check and the while loop condition and for setting PROP_LENGTH on
ResultObj; update references in the K loop, Map handling, and the final
ResultObj.SetProperty call (symbols: Source.GetProperty(PROP_LENGTH),
AwaitValue, ToNumberLiteral.Value, K loop, UseConstructor,
ResultObj.SetProperty) to use the cached Len.
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 1058-1136: Add argument validation at the start of
TGocciaExpectationValue.GetResolves and TGocciaExpectationValue.GetRejects to
require exactly zero arguments using TGocciaArgumentValidator (use
RequireExactly on the incoming AArgs); place the validation before any promise
checks or microtask drains so the matcher fails fast and consistently with other
builtins, and keep the rest of the method logic unchanged (refer to GetResolves
and GetRejects and parameter AArgs for where to add the check).
- Around line 236-240: Replace the hardcoded property-name literals 'resolves'
and 'rejects' with constants defined in Goccia.Constants.PropertyNames: add two
new constants (e.g. PropertyNameResolves and PropertyNameRejects) in that unit,
then update the DefineProperty calls (the two
TGocciaPropertyDescriptorAccessor/Create lines where GetResolves/GetRejects are
used) to reference those constants instead of the string literals so property
names follow the project's split constant convention.
In `@units/Goccia.Evaluator.pas`:
- Around line 1408-1422: The per-iteration scope IterScope (created via
AContext.Scope.CreateChild(skBlock) and assigned to IterContext.Scope) is never
freed and will leak; wrap the per-iteration logic that calls
AssignPattern/IterScope.DefineLexicalBinding and Evaluate(AForOfStatement.Body,
IterContext) in a try..finally and call IterScope.Free (or appropriate
destructor) in the finally block so IterScope is released on normal completion,
on TGocciaBreakSignal, and on exceptions; apply the same pattern to the
analogous for-await-of branches (the other blocks around lines 1511-1525 and
1571-1585) ensuring IterContext still references the scope while inside the try
and that freeing happens after Evaluate returns or an exception is handled.
- Around line 1322-1350: The Promise created by TGocciaPromiseValue.Create is
only temp-rooted after TGocciaFunctionBase(ThenMethod).Call, which risks GC
dropping Promise during the thenable invocation; move the
TGocciaGarbageCollector.Instance.AddTempRoot(Promise) call to immediately after
Promise := TGocciaPromiseValue.Create (and before constructing ThenArgs/ calling
TGocciaFunctionBase.Call), and ensure the existing finally block still calls
RemoveTempRoot/cleans up Promise so the lifetime is preserved during the Call.
- Around line 1481-1507: The for-await-of implementation currently only treats
TGocciaPromiseValue as awaitable; extract the thenable-resolution code from
EvaluateAwait into a shared helper (e.g., AwaitValue(value: TGocciaValue):
TGocciaValue) that implements ECMA Await semantics (PromiseResolve/thenable
wrapping, draining microtask queue, handling gpsRejected and throwing
TGocciaThrowValue, and GC temp-root management like the existing EvaluateAwait
block), then call AwaitValue on NextResult and on CurrentValue in
EvaluateForAwaitOf (replacing the direct "if NextResult is TGocciaPromiseValue"
checks) so both Promises and thenables are correctly awaited before reading
PROP_DONE/PROP_VALUE.
---
Duplicate comments:
In `@units/Goccia.Evaluator.pas`:
- Around line 1304-1306: The ES comment above EvaluateAwait (and the two other
ECMAScript-annotated functions noted) is missing the Pascal method signature;
update each ES spec annotation immediately above the function implementation to
include the full Pascal method signature (function name with parameter list and
return type) — e.g. replace or augment "// ES2026 §27.7.5.3 ..." with a line
that includes the method signature for EvaluateAwait (EvaluateAwait(const
AAwaitExpression: TGocciaAwaitExpression; const AContext:
TGocciaEvaluationContext): TGocciaValue) and do the same for the other two
functions referenced in the review so each spec comment follows the required
"ESYYYY §X.Y.Z MethodName(params)" format.
In `@units/Goccia.Parser.pas`:
- Around line 1556-1557: The ES annotation above the ForStatement function is
missing the method signature; update the comment to follow the project's
spec-format by adding the signature after the spec reference (e.g. "// ES2026
§14.7.5 ForIn/OfStatement TGocciaParser.ForStatement: TGocciaStatement")
immediately above the ForStatement implementation in the implementation section
so it matches the required `// ESYYYY §X.Y.Z MethodName(params)` style and
references TGocciaParser.ForStatement.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 419-420: The docs for BenchmarkRunner/ggBenchmark describe
bench(name, { setup?, run, teardown? }) but omit that the callbacks (setup, run,
teardown) may be async and can use await; update the BenchmarkRunner/ggBenchmark
section to explicitly state that bench() supports async functions for setup,
run, and teardown, that the value returned/resolved by async setup is passed to
run/teardown, and that each phase (setupMs, runMs, teardownMs) times the awaited
completion; reference the bench() API, the setup/run/teardown callbacks, and
BenchmarkRunner/ggBenchmark in the text so readers know where async behavior is
supported.
In `@BenchmarkRunner.dpr`:
- Around line 101-113: Replace hardcoded property name strings 'setupMs' and
'teardownMs' used in the SingleResult.GetProperty calls with the centralized
property-name constants from Goccia.Constants.PropertyNames; update the
expressions that assign Entry.SetupMs and Entry.TeardownMs (the lines using
SingleResult.GetProperty('setupMs').ToNumberLiteral.Value and
SingleResult.GetProperty('teardownMs').ToNumberLiteral.Value) to use
Goccia.Constants.PropertyNames.<appropriateName> instead, keeping the rest of
the call chain (ToNumberLiteral.Value) unchanged and ensuring you import or
qualify Goccia.Constants if necessary.
In `@units/Goccia.Builtins.Benchmark.pas`:
- Around line 195-216: Replace the manual argument checks in the bench
registration block with TGocciaArgumentValidator: validate that there are
exactly 2 args and that arg0 is a TGocciaStringLiteralValue and arg1 is a
TGocciaObjectValue using TGocciaArgumentValidator APIs (from
Goccia.Arguments.Validator.pas), then extract BenchName, OptionsObj, and the
Run/Setup/Teardown properties as before (RunFn, SetupFn, TeardownFn) and call
FRegisteredBenchmarks.Add(TBenchmarkCase.Create(...)) — this centralizes error
reporting and removes the handcrafted if/Exit checks around BenchName,
OptionsObj, RunProp/RunFn, SetupProp/SetupFn, and TeardownProp/TeardownFn.
- Around line 202-214: The code uses hardcoded property name strings ("run",
"setup", "teardown", and also "setupMs"/"teardownMs" elsewhere) when calling
OptionsObj.GetProperty and when checking function types; replace those literal
strings with the corresponding constants from Goccia.Constants.PropertyNames
(e.g. PropertyNames.Run, PropertyNames.Setup, PropertyNames.Teardown,
PropertyNames.SetupMs, PropertyNames.TeardownMs) and keep the existing checks
and casts (RunProp, RunFn, SetupProp, SetupFn, TeardownProp, TeardownFn,
OptionsObj.GetProperty) unchanged otherwise so callers read from the constant
values instead of raw string literals.
In `@units/Goccia.Builtins.GlobalArray.pas`:
- Around line 20-21: The ArrayFromAsync and ArrayOf built-ins currently perform
ad-hoc argument/arity checks; replace those checks with the centralized
TGocciaArgumentValidator usage from Goccia.Arguments.Validator.pas to
standardize errors and behavior. Update the implementations of ArrayFromAsync
and ArrayOf (and similarly the other built-ins in the 315–398 region) to
construct a TGocciaArgumentValidator, call its ValidateCount/ValidateType (or
equivalent) for the expected arity and types, and use its validated/accessor
methods to obtain arguments instead of manual checks; ensure you add the unit
reference for Goccia.Arguments.Validator and preserve existing semantics while
delegating validation to the validator so error messages and arity handling are
consistent.
- Around line 55-58: Replace the hardcoded 'fromAsync' property string when
registering the native method in FBuiltinObject.RegisterNativeMethod: instead of
TGocciaNativeFunctionValue.Create(ArrayFromAsync, 'fromAsync', 1) use the
property-name constant from Goccia.Constants.PropertyNames (e.g.,
PropertyNames.FromAsync) and update the unit uses clause to include
Goccia.Constants so the call becomes
TGocciaNativeFunctionValue.Create(ArrayFromAsync, PropertyNames.FromAsync, 1);
leave the other registrations unchanged.
In `@units/Goccia.Parser.pas`:
- Around line 35-36: Run the native Pascal parser/AST test suite using
"./build.pas clean tests && for t in build/Goccia.*.Test; do "$t"; done"; if any
tests fail due to your changes around the AST/scope chain in
units/Goccia.Parser.pas (notably the FInAsyncFunction field), update the
corresponding units/*.Test.pas expectations to match the new behavior or adjust
the parser logic to preserve previous semantics; re-run the command until all
tests pass and commit the updated test files alongside the code change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
AGENTS.mdBenchmarkRunner.dprbenchmarks/arrays.jsbenchmarks/async-await.jsbenchmarks/classes.jsbenchmarks/closures.jsbenchmarks/collections.jsbenchmarks/destructuring.jsbenchmarks/fibonacci.jsbenchmarks/for-of.jsbenchmarks/iterators.jsbenchmarks/json.jsbenchmarks/numbers.jsbenchmarks/objects.jsbenchmarks/promises.jsbenchmarks/strings.jsdocs/benchmarks.mdunits/Goccia.Benchmark.Reporter.pasunits/Goccia.Builtins.Benchmark.pasunits/Goccia.Builtins.GlobalArray.pasunits/Goccia.Builtins.TestAssertions.pasunits/Goccia.Evaluator.pasunits/Goccia.GarbageCollector.pasunits/Goccia.Parser.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- benchmarks/async-await.js
Comment on lines
+1058
to
+1136
| function TGocciaExpectationValue.GetResolves(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; | ||
| var | ||
| Promise: TGocciaPromiseValue; | ||
| begin | ||
| if Assigned(TGocciaGarbageCollector.Instance) then | ||
| TGocciaGarbageCollector.Instance.AddTempRoot(FActualValue); | ||
| try | ||
| if Assigned(TGocciaMicrotaskQueue.Instance) then | ||
| TGocciaMicrotaskQueue.Instance.DrainQueue; | ||
|
|
||
| if not (FActualValue is TGocciaPromiseValue) then | ||
| begin | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('resolves', | ||
| 'Expected a Promise but received ' + FActualValue.ToStringLiteral.Value); | ||
| Result := TGocciaExpectationValue.Create(TGocciaUndefinedLiteralValue.UndefinedValue, FTestAssertions, FIsNegated); | ||
| Exit; | ||
| end; | ||
|
|
||
| Promise := TGocciaPromiseValue(FActualValue); | ||
|
|
||
| if Promise.State = gpsFulfilled then | ||
| Result := TGocciaExpectationValue.Create(Promise.PromiseResult, FTestAssertions, FIsNegated) | ||
| else if Promise.State = gpsRejected then | ||
| begin | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('resolves', | ||
| 'Expected Promise to resolve but it rejected with: ' + Promise.PromiseResult.ToStringLiteral.Value); | ||
| Result := TGocciaExpectationValue.Create(TGocciaUndefinedLiteralValue.UndefinedValue, FTestAssertions, FIsNegated); | ||
| end | ||
| else | ||
| begin | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('resolves', | ||
| 'Promise still pending after microtask drain'); | ||
| Result := TGocciaExpectationValue.Create(TGocciaUndefinedLiteralValue.UndefinedValue, FTestAssertions, FIsNegated); | ||
| end; | ||
| finally | ||
| if Assigned(TGocciaGarbageCollector.Instance) then | ||
| TGocciaGarbageCollector.Instance.RemoveTempRoot(FActualValue); | ||
| end; | ||
| end; | ||
|
|
||
| function TGocciaExpectationValue.GetRejects(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; | ||
| var | ||
| Promise: TGocciaPromiseValue; | ||
| begin | ||
| if Assigned(TGocciaGarbageCollector.Instance) then | ||
| TGocciaGarbageCollector.Instance.AddTempRoot(FActualValue); | ||
| try | ||
| if Assigned(TGocciaMicrotaskQueue.Instance) then | ||
| TGocciaMicrotaskQueue.Instance.DrainQueue; | ||
|
|
||
| if not (FActualValue is TGocciaPromiseValue) then | ||
| begin | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('rejects', | ||
| 'Expected a Promise but received ' + FActualValue.ToStringLiteral.Value); | ||
| Result := TGocciaExpectationValue.Create(TGocciaUndefinedLiteralValue.UndefinedValue, FTestAssertions, FIsNegated); | ||
| Exit; | ||
| end; | ||
|
|
||
| Promise := TGocciaPromiseValue(FActualValue); | ||
|
|
||
| if Promise.State = gpsRejected then | ||
| Result := TGocciaExpectationValue.Create(Promise.PromiseResult, FTestAssertions, FIsNegated) | ||
| else if Promise.State = gpsFulfilled then | ||
| begin | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('rejects', | ||
| 'Expected Promise to reject but it resolved with: ' + Promise.PromiseResult.ToStringLiteral.Value); | ||
| Result := TGocciaExpectationValue.Create(TGocciaUndefinedLiteralValue.UndefinedValue, FTestAssertions, FIsNegated); | ||
| end | ||
| else | ||
| begin | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('rejects', | ||
| 'Promise still pending after microtask drain'); | ||
| Result := TGocciaExpectationValue.Create(TGocciaUndefinedLiteralValue.UndefinedValue, FTestAssertions, FIsNegated); | ||
| end; | ||
| finally | ||
| if Assigned(TGocciaGarbageCollector.Instance) then | ||
| TGocciaGarbageCollector.Instance.RemoveTempRoot(FActualValue); | ||
| end; | ||
| end; |
Contributor
There was a problem hiding this comment.
Validate that resolves/rejects are called with zero args.
This keeps matcher accessors consistent with the rest of the builtins.
🛠️ Suggested update
function TGocciaExpectationValue.GetResolves(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue;
var
Promise: TGocciaPromiseValue;
begin
+ TGocciaArgumentValidator.RequireExactly(AArgs, 0, 'resolves', FTestAssertions.ThrowError);
if Assigned(TGocciaGarbageCollector.Instance) then
TGocciaGarbageCollector.Instance.AddTempRoot(FActualValue);
try
@@
function TGocciaExpectationValue.GetRejects(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue;
var
Promise: TGocciaPromiseValue;
begin
+ TGocciaArgumentValidator.RequireExactly(AArgs, 0, 'rejects', FTestAssertions.ThrowError);
if Assigned(TGocciaGarbageCollector.Instance) then
TGocciaGarbageCollector.Instance.AddTempRoot(FActualValue);
tryAs per coding guidelines "Built-in argument validation uses TGocciaArgumentValidator (Goccia.Arguments.Validator.pas) with RequireExactly and RequireAtLeast methods for standardized validation".
🤖 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 1058 - 1136, Add
argument validation at the start of TGocciaExpectationValue.GetResolves and
TGocciaExpectationValue.GetRejects to require exactly zero arguments using
TGocciaArgumentValidator (use RequireExactly on the incoming AArgs); place the
validation before any promise checks or microtask drains so the matcher fails
fast and consistently with other builtins, and keep the rest of the method logic
unchanged (refer to GetResolves and GetRejects and parameter AArgs for where to
add the check).
Comment on lines
+1408
to
+1422
| IterScope := AContext.Scope.CreateChild(skBlock); | ||
| IterContext := AContext; | ||
| IterContext.Scope := IterScope; | ||
|
|
||
| if AForOfStatement.BindingPattern <> nil then | ||
| AssignPattern(AForOfStatement.BindingPattern, CurrentValue, IterContext, True, DeclarationType) | ||
| else | ||
| IterScope.DefineLexicalBinding(AForOfStatement.BindingName, CurrentValue, DeclarationType); | ||
|
|
||
| try | ||
| Evaluate(AForOfStatement.Body, IterContext); | ||
| except | ||
| on E: TGocciaBreakSignal do | ||
| Exit; | ||
| end; |
Contributor
There was a problem hiding this comment.
Free per‑iteration scopes to avoid leaks.
IterScope is created each iteration but never freed. This can leak on large loops. Wrap per‑iteration work in try/finally and free the scope in both for‑of and for‑await‑of branches.
♻️ Suggested pattern (apply to all three loops)
- IterScope := AContext.Scope.CreateChild(skBlock);
- IterContext := AContext;
- IterContext.Scope := IterScope;
+ IterScope := AContext.Scope.CreateChild(skBlock);
+ try
+ IterContext := AContext;
+ IterContext.Scope := IterScope;
@@
- try
- Evaluate(AForOfStatement.Body, IterContext);
- except
- on E: TGocciaBreakSignal do
- Exit;
- end;
+ try
+ Evaluate(AForOfStatement.Body, IterContext);
+ except
+ on E: TGocciaBreakSignal do
+ Exit;
+ end;
+ finally
+ IterScope.Free;
+ end;Also applies to: 1511-1525, 1571-1585
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Evaluator.pas` around lines 1408 - 1422, The per-iteration scope
IterScope (created via AContext.Scope.CreateChild(skBlock) and assigned to
IterContext.Scope) is never freed and will leak; wrap the per-iteration logic
that calls AssignPattern/IterScope.DefineLexicalBinding and
Evaluate(AForOfStatement.Body, IterContext) in a try..finally and call
IterScope.Free (or appropriate destructor) in the finally block so IterScope is
released on normal completion, on TGocciaBreakSignal, and on exceptions; apply
the same pattern to the analogous for-await-of branches (the other blocks around
lines 1511-1525 and 1571-1585) ensuring IterContext still references the scope
while inside the try and that freeing happens after Evaluate returns or an
exception is handled.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
units/Goccia.Builtins.TestAssertions.pas (1)
1058-1136:⚠️ Potential issue | 🟡 MinorAdd zero‑arg validation for
.resolves/.rejects.
Line 1058 and Line 1098: these accessors should reject unexpected arguments to match other built‑ins.As per coding guidelines "Built-in argument validation uses `TGocciaArgumentValidator` (`Goccia.Arguments.Validator.pas`) with `RequireExactly` and `RequireAtLeast` methods for standardized validation".🛠️ Suggested fix
function TGocciaExpectationValue.GetResolves(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; var Promise: TGocciaPromiseValue; begin + TGocciaArgumentValidator.RequireExactly(AArgs, 0, PROP_RESOLVES, FTestAssertions.ThrowError); if Assigned(TGocciaGarbageCollector.Instance) then TGocciaGarbageCollector.Instance.AddTempRoot(FActualValue); @@ function TGocciaExpectationValue.GetRejects(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; var Promise: TGocciaPromiseValue; begin + TGocciaArgumentValidator.RequireExactly(AArgs, 0, PROP_REJECTS, FTestAssertions.ThrowError); if Assigned(TGocciaGarbageCollector.Instance) then TGocciaGarbageCollector.Instance.AddTempRoot(FActualValue);🤖 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 1058 - 1136, GetResolves and GetRejects are missing built-in argument validation and should reject unexpected args; add a zero-arg check using TGocciaArgumentValidator (call RequireExactly(0, AArgs) or the equivalent RequireAtLeast/RequireExactly API) at the start of both TGocciaExpectationValue.GetResolves and TGocciaExpectationValue.GetRejects before any use of AArgs or FActualValue so they follow the standardized validation pattern.units/Goccia.Evaluator.pas (3)
1305-1442:⚠️ Potential issue | 🟡 MinorSpec annotations must include full method signatures.
The comments above
AwaitValue,EvaluateAwait,EvaluateForOf, andEvaluateForAwaitOfonly show the spec title (e.g.,Await(value)), but the guideline requiresMethodName(params)in the annotation.✍️ Example fix (apply to all four annotations)
-// ES2026 §27.7.5.3 Await(value) +// ES2026 §27.7.5.3 AwaitValue(const AValue: TGocciaValue): TGocciaValueAs per coding guidelines, "When implementing ECMAScript-specified behavior, annotate each function or method with a comment referencing the relevant spec section using the format
// ESYYYY §X.Y.Z MethodName(params)immediately above the function body in theimplementationsection."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Evaluator.pas` around lines 1305 - 1442, The spec annotation comments above AwaitValue, EvaluateAwait, EvaluateForOf, and EvaluateForAwaitOf are missing full method signatures; update each comment to the required format (for example replace "// ES2026 §27.7.5.3 Await(value)" with "// ES2026 §27.7.5.3 Await(value: TGocciaValue): TGocciaValue" and likewise include parameter and return types for EvaluateAwait (e.g. "EvaluateAwait(AAwaitExpression: TGocciaAwaitExpression; AContext: TGocciaEvaluationContext): TGocciaValue"), EvaluateForOf and EvaluateForAwaitOf using their actual parameter names and return types as declared, keeping the ES section numbers intact and placing the comment immediately above each function implementation.
1496-1510:⚠️ Potential issue | 🟠 MajorFree per‑iteration scope in
for‑await‑ofbranches.
IterScopeis created each iteration and never released in both async‑iterator and sync‑fallback branches.♻️ Suggested fix (apply to both branches)
- IterScope := AContext.Scope.CreateChild(skBlock); - IterContext := AContext; - IterContext.Scope := IterScope; + IterScope := AContext.Scope.CreateChild(skBlock); + try + IterContext := AContext; + IterContext.Scope := IterScope; @@ - try - Evaluate(AForAwaitOfStatement.Body, IterContext); - except - on E: TGocciaBreakSignal do - Exit; - end; + try + Evaluate(AForAwaitOfStatement.Body, IterContext); + except + on E: TGocciaBreakSignal do + Exit; + end; + finally + IterScope.Free; + end;Also applies to: 1536-1550
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Evaluator.pas` around lines 1496 - 1510, The per-iteration child scope created via AContext.Scope.CreateChild (assigned to IterScope) is not being released; wrap the per-iteration logic (AssignPattern/DefineLexicalBinding and Evaluate calls that use IterContext/IterScope) in a try..finally and call IterScope.Free in the finally so the scope is freed each iteration, and ensure the TGocciaBreakSignal handling still exits correctly (keep the existing except for TGocciaBreakSignal but move it inside the try so the finally always runs); apply the same change to the sync-fallback branch (the similar block at the other occurrence).
1413-1427:⚠️ Potential issue | 🟠 MajorFree per‑iteration scope in
for‑of.
IterScopeis created every iteration but never freed; this leaks on large loops and onbreak.♻️ Suggested fix
- IterScope := AContext.Scope.CreateChild(skBlock); - IterContext := AContext; - IterContext.Scope := IterScope; + IterScope := AContext.Scope.CreateChild(skBlock); + try + IterContext := AContext; + IterContext.Scope := IterScope; @@ - try - Evaluate(AForOfStatement.Body, IterContext); - except - on E: TGocciaBreakSignal do - Exit; - end; + try + Evaluate(AForOfStatement.Body, IterContext); + except + on E: TGocciaBreakSignal do + Exit; + end; + finally + IterScope.Free; + end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Evaluator.pas` around lines 1413 - 1427, IterScope is allocated each iteration via AContext.Scope.CreateChild(skBlock) but never freed, causing a leak (especially on break/exception); wrap the per-iteration work in a try..finally that ensures IterScope.Free (or FreeAndNil) after using it — i.e., after setting IterContext.Scope and performing AssignPattern/DefineLexicalBinding and Evaluate(AForOfStatement.Body, IterContext), ensure IterScope is freed in the finally block while preserving the existing except handler for TGocciaBreakSignal so break still exits the loop cleanly.
🤖 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.Builtins.GlobalArray.pas`:
- Around line 423-574: The iterator-to-array conversion code is missing ES2026
spec annotations; add "// ES2026 §<section>" comments above the containing
function and at key algorithm steps: before the async iterator branch (where
IteratorMethod is resolved and IteratorObj created, and before the async loop
that calls NextMethod/AwaitValue and handles Done/Value), before the sync
iterator branch (where WellKnownIterator is resolved, IteratorObj/Iterator
created and the AdvanceNext loop using IterResult), and before the array‑like
branch (where LengthValue is read and the index loop runs); place annotations
near ThrowTypeError, AwaitValue calls, and where ResultObj length is set so the
implementation in methods like TGocciaObjectValue.GetSymbolProperty,
TGocciaFunctionBase.Call, AwaitValue, Iterator.AdvanceNext, and AddElement can
be trivially mapped to the cited ES2026 algorithm steps.
In `@units/Goccia.Evaluator.pas`:
- Around line 1322-1371: The call to the thenable's then
(TGocciaFunctionBase(ThenMethod).Call) must not let exceptions escape — catch
exceptions around that Call and on exception invoke the created Promise's reject
callback (call Promise.DoReject with the caught error) so the promise is
rejected per ECMA-262 semantics; ensure the temp-root cleanup
(TGocciaGarbageCollector.Instance.RemoveTempRoot(Promise)) still runs in the
existing finally, and keep the existing ThenArgs.Free behavior.
- Around line 1485-1490: After AwaitValue(NextResult) returns, you must validate
that NextResult is an Object per ECMA-262 §14.6.1.2 before calling
GetProperty(PROP_DONE); if it is not an object throw a TypeError. Update the
iterator loop where NextResult is obtained (the call sequence: NextResult :=
TGocciaFunctionBase(NextMethod).Call(...); NextResult :=
AwaitValue(NextResult);) to check the runtime type/shape of NextResult (e.g.,
object/record indicator or an IsObject helper) and raise the engine's TypeError
exception when the check fails, only then call
NextResult.GetProperty(PROP_DONE).
---
Duplicate comments:
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 1058-1136: GetResolves and GetRejects are missing built-in
argument validation and should reject unexpected args; add a zero-arg check
using TGocciaArgumentValidator (call RequireExactly(0, AArgs) or the equivalent
RequireAtLeast/RequireExactly API) at the start of both
TGocciaExpectationValue.GetResolves and TGocciaExpectationValue.GetRejects
before any use of AArgs or FActualValue so they follow the standardized
validation pattern.
In `@units/Goccia.Evaluator.pas`:
- Around line 1305-1442: The spec annotation comments above AwaitValue,
EvaluateAwait, EvaluateForOf, and EvaluateForAwaitOf are missing full method
signatures; update each comment to the required format (for example replace "//
ES2026 §27.7.5.3 Await(value)" with "// ES2026 §27.7.5.3 Await(value:
TGocciaValue): TGocciaValue" and likewise include parameter and return types for
EvaluateAwait (e.g. "EvaluateAwait(AAwaitExpression: TGocciaAwaitExpression;
AContext: TGocciaEvaluationContext): TGocciaValue"), EvaluateForOf and
EvaluateForAwaitOf using their actual parameter names and return types as
declared, keeping the ES section numbers intact and placing the comment
immediately above each function implementation.
- Around line 1496-1510: The per-iteration child scope created via
AContext.Scope.CreateChild (assigned to IterScope) is not being released; wrap
the per-iteration logic (AssignPattern/DefineLexicalBinding and Evaluate calls
that use IterContext/IterScope) in a try..finally and call IterScope.Free in the
finally so the scope is freed each iteration, and ensure the TGocciaBreakSignal
handling still exits correctly (keep the existing except for TGocciaBreakSignal
but move it inside the try so the finally always runs); apply the same change to
the sync-fallback branch (the similar block at the other occurrence).
- Around line 1413-1427: IterScope is allocated each iteration via
AContext.Scope.CreateChild(skBlock) but never freed, causing a leak (especially
on break/exception); wrap the per-iteration work in a try..finally that ensures
IterScope.Free (or FreeAndNil) after using it — i.e., after setting
IterContext.Scope and performing AssignPattern/DefineLexicalBinding and
Evaluate(AForOfStatement.Body, IterContext), ensure IterScope is freed in the
finally block while preserving the existing except handler for
TGocciaBreakSignal so break still exits the loop cleanly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
AGENTS.mdBenchmarkRunner.dprbenchmarks/collections.jsdocs/benchmarks.mdunits/Goccia.Builtins.GlobalArray.pasunits/Goccia.Builtins.TestAssertions.pasunits/Goccia.Constants.PropertyNames.pasunits/Goccia.Evaluator.pas
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
units/Goccia.Evaluator.pas (1)
1419-1433:⚠️ Potential issue | 🟠 MajorFree per‑iteration scopes to avoid leaks in for‑of / for‑await‑of.
IterScopeis created each iteration but never freed, so long-running loops will leak. Wrap per‑iteration work intry..finallyand free the scope in thefinally.♻️ Suggested pattern (apply to all three loops)
- IterScope := AContext.Scope.CreateChild(skBlock); - IterContext := AContext; - IterContext.Scope := IterScope; - - if AForOfStatement.BindingPattern <> nil then - AssignPattern(AForOfStatement.BindingPattern, CurrentValue, IterContext, True, DeclarationType) - else - IterScope.DefineLexicalBinding(AForOfStatement.BindingName, CurrentValue, DeclarationType); - - try - Evaluate(AForOfStatement.Body, IterContext); - except - on E: TGocciaBreakSignal do - Exit; - end; + IterScope := AContext.Scope.CreateChild(skBlock); + try + IterContext := AContext; + IterContext.Scope := IterScope; + + if AForOfStatement.BindingPattern <> nil then + AssignPattern(AForOfStatement.BindingPattern, CurrentValue, IterContext, True, DeclarationType) + else + IterScope.DefineLexicalBinding(AForOfStatement.BindingName, CurrentValue, DeclarationType); + + try + Evaluate(AForOfStatement.Body, IterContext); + except + on E: TGocciaBreakSignal do + Exit; + end; + finally + IterScope.Free; + end;Also applies to: 1506-1520, 1546-1560
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Evaluator.pas` around lines 1419 - 1433, IterScope is allocated each iteration and never freed, causing memory leaks; wrap the per-iteration work in a try..finally and free IterScope in the finally block. Specifically, after creating IterScope and setting IterContext, enclose the AssignPattern/DefineLexicalBinding and the Evaluate(...) call (and its TGocciaBreakSignal except handler) in a try..finally where the finally calls IterScope.Free; apply the same pattern to the other two loop sites (the blocks around lines with IterScope/IterContext creation at the other noted locations) so IterScope is always released even when exceptions or Exit occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 419-420: The docs claim "All three phases are independently timed
and reported (`setupMs`, `teardownMs`)" but only list two fields; update the
AGENTS.md text around BenchmarkRunner / ggBenchmark / bench to include the run
phase timing name (e.g., `runMs` or `durationMs`) or reword to say "setup and
teardown are additionally timed and reported" — locate references to
BenchmarkRunner, ggBenchmark, bench, setupMs and teardownMs in that paragraph
and either add the run timing identifier alongside them or adjust the sentence
to explicitly state that setup and teardown are additional timings.
- Line 481: The phrasing in the JS test framework doc is contradictory: change
the sentence that currently reads "Returning a Promise from a non-async test
callback is still supported as the primary pattern for testing Promise-based
code" to state that returning a Promise from a non-async test callback is
supported for backward compatibility rather than being the primary pattern;
update the JS test framework paragraph (the block describing ggTestAssertions
and describe/test/expect) to make async callbacks the default usage and mark
Promise-returning callbacks as backward-compatible support.
In `@docs/code-style.md`:
- Line 75: The documentation example for the abstract operation IteratorNext is
missing the optional second parameter and should be updated to match the spec;
locate the example of IteratorNext in docs (the line with
"IteratorNext(iteratorRecord)") and change it to include the optional value
parameter using the spec-style bracket notation (IteratorNext(iteratorRecord [,
value])) so it exactly matches ECMA-262 and the project's "must match exactly"
rule.
In `@README.md`:
- Line 47: Update the README example snippet `async () => { await promise }` to
include the required semicolon for GocciaScript ASI rules by changing the block
used in the example (the `async () => { await promise }` snippet) so it reads
`async () => { await promise; }`.
- Line 195: The README changes that add the "JSX Transformer" pipeline step are
out of scope for this async/await PR; either remove/revert the edits that
reference the JSX Transformer from the current diff (the line showing
Source["Source Code"] --> JSXTransformer["JSX Transformer"] and the other
mention at the other changed line) or extract them into a separate docs-only
commit/PR; specifically revert or move any documentation referencing "JSX
Transformer" and the file Goccia.JSX.Transformer.pas so this PR only contains
async/await-related changes and the JSX documentation is applied in its own
follow-up PR.
In `@units/Goccia.Builtins.GlobalArray.pas`:
- Around line 461-476: The code is awaiting the iterator's yielded value too
early (KValue := AwaitValue(KValue)) which breaks the async-iterable path and
the mapping semantics; remove the AwaitValue call immediately after retrieving
KValue from NextResult so that KValue remains the raw yielded value, and only
call AwaitValue on the result of InvokeCallable(MapCallback, MapArgs, ThisArg)
inside the Mapping branch (i.e., keep KValue := AwaitValue(KValue) only after
MapCallback is invoked), referencing DoneValue, NextResult, KValue, AwaitValue,
Mapping, MapCallback, InvokeCallable, MapArgs and ThisArg to locate and update
the logic.
- Around line 339-346: The AwaitValue implementation currently drains
TGocciaMicrotaskQueue.Instance but then unconditionally reads
AwaitedPromise.PromiseResult, which can still be gpsPending and lead to access
violations; update AwaitValue to re-check AwaitedPromise.State after
TGocciaMicrotaskQueue.Instance.DrainQueue and handle the gpsPending case (e.g.,
raise an appropriate exception instead of reading PromiseResult or return a safe
default) before the existing gpsRejected check and the final Result :=
AwaitedPromise.PromiseResult assignment; reference AwaitValue,
AwaitedPromise.State/PromiseResult, TGocciaMicrotaskQueue.Instance.DrainQueue,
gpsPending and gpsRejected, and TGocciaThrowValue.Create to implement the guard.
- Line 315: Update the spec section annotation for Array.fromAsync from
"§23.1.2.1" to "§23.1.2.2": locate the Array.fromAsync comment/annotation (the
line annotating "ES2026 §23.1.2.1 Array.fromAsync(...)") and change the section
number to §23.1.2.2 so it no longer duplicates Array.from; ensure the textual
annotation exactly reads "ES2026 §23.1.2.2 Array.fromAsync(...)" to match
ECMAScript 2026 ordering.
In `@units/Goccia.Evaluator.pas`:
- Around line 1387-1572: The change touches evaluator logic in EvaluateForOf and
EvaluateForAwaitOf but no native Pascal tests were run or updated; run the full
native test suite with "./build.pas clean tests && for t in build/Goccia.*.Test;
do "$t"; done", fix any failing tests caused by your changes, and update the
corresponding units/*.Test.pas files to reflect the new behavior (adjust
expectations/assertions for iterator semantics, for-await-of async handling, and
scope/binding behavior in EvaluateForOf/EvaluateForAwaitOf) before committing.
---
Duplicate comments:
In `@units/Goccia.Evaluator.pas`:
- Around line 1419-1433: IterScope is allocated each iteration and never freed,
causing memory leaks; wrap the per-iteration work in a try..finally and free
IterScope in the finally block. Specifically, after creating IterScope and
setting IterContext, enclose the AssignPattern/DefineLexicalBinding and the
Evaluate(...) call (and its TGocciaBreakSignal except handler) in a try..finally
where the finally calls IterScope.Free; apply the same pattern to the other two
loop sites (the blocks around lines with IterScope/IterContext creation at the
other noted locations) so IterScope is always released even when exceptions or
Exit occur.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
AGENTS.mdREADME.mddocs/code-style.mdunits/Goccia.Builtins.GlobalArray.pasunits/Goccia.Evaluator.pas
| ```mermaid | ||
| flowchart LR | ||
| Source["Source Code"] --> Lexer --> Parser --> Interpreter --> Evaluator --> Result | ||
| Source["Source Code"] --> JSXTransformer["JSX Transformer"] --> Lexer --> Parser --> Interpreter --> Evaluator --> Result |
Contributor
There was a problem hiding this comment.
JSX Transformer architecture docs appear out of scope for this PR.
Lines 195 and 201 document the JSX Transformer pipeline step, which is unrelated to async/await. If Goccia.JSX.Transformer.pas already existed before this PR, these documentation updates belong in the commit/PR that introduced JSX, not here. Consider moving them to a separate docs-only PR or at least calling this out in the PR description to keep history attributable.
Also applies to: 201-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 195, The README changes that add the "JSX Transformer"
pipeline step are out of scope for this async/await PR; either remove/revert the
edits that reference the JSX Transformer from the current diff (the line showing
Source["Source Code"] --> JSXTransformer["JSX Transformer"] and the other
mention at the other changed line) or extract them into a separate docs-only
commit/PR; specifically revert or move any documentation referencing "JSX
Transformer" and the file Goccia.JSX.Transformer.pas so this PR only contains
async/await-related changes and the JSX documentation is applied in its own
follow-up PR.
| end; | ||
| end; | ||
|
|
||
| // ES2026 §23.1.2.1 Array.fromAsync(asyncItems [, mapfn [, thisArg]]) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ECMAScript 2025 2026 Array.fromAsync specification section number §23.1.2
💡 Result:
- ECMAScript 2026 (ECMA-262):
Array.fromAsyncis specified in §23.1.2.2 (“Properties of the Array Constructor”). [1] - ECMAScript 2025 (ECMA-262):
Array.fromAsyncis not present; in §23.1.2 the numbering goesArray.from(§23.1.2.1) thenArray.isArray(§23.1.2.2), etc. [2]
Sources:
[1] (tc39.es)
[2] (tc39.es)
Citations:
- 1: https://tc39.es/ecma262/multipage/fundamental-objects.html?utm_source=openai
- 2: https://tc39.es/ecma262/2025/?utm_source=openai
Fix spec section annotation from §23.1.2.1 to §23.1.2.2 for Array.fromAsync.
Line 315 duplicates the section number from Array.from (line 74, §23.1.2.1). According to ECMAScript 2026, Array.fromAsync is specified in §23.1.2.2, consistent with the ordering shown in the file (Array.of is §23.1.2.3 at line 615).
📝 Proposed fix
-// ES2026 §23.1.2.1 Array.fromAsync(asyncItems [, mapfn [, thisArg]])
+// ES2026 §23.1.2.2 Array.fromAsync(asyncItems [, mapfn [, thisArg]])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Builtins.GlobalArray.pas` at line 315, Update the spec section
annotation for Array.fromAsync from "§23.1.2.1" to "§23.1.2.2": locate the
Array.fromAsync comment/annotation (the line annotating "ES2026 §23.1.2.1
Array.fromAsync(...)") and change the section number to §23.1.2.2 so it no
longer duplicates Array.from; ensure the textual annotation exactly reads
"ES2026 §23.1.2.2 Array.fromAsync(...)" to match ECMAScript 2026 ordering.
Comment on lines
+461
to
+476
| if Assigned(DoneValue) and DoneValue.ToBooleanLiteral.Value then | ||
| Break; | ||
|
|
||
| // ES2026 §23.1.2.1 step 4.b.v: Await(IteratorValue(next)) | ||
| KValue := NextResult.GetProperty(PROP_VALUE); | ||
| if not Assigned(KValue) then | ||
| KValue := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| KValue := AwaitValue(KValue); | ||
|
|
||
| if Mapping then | ||
| begin | ||
| // ES2026 §23.1.2.1 step 4.b.vii: Await(Call(mapfn, thisArg, « kValue, k »)) | ||
| MapArgs.SetElement(0, KValue); | ||
| MapArgs.SetElement(1, TGocciaNumberLiteralValue.SmallInt(K)); | ||
| KValue := InvokeCallable(MapCallback, MapArgs, ThisArg); | ||
| KValue := AwaitValue(KValue); |
Contributor
There was a problem hiding this comment.
Spec deviation: async-iterable path must not await the yielded value before passing it to mapFn.
The comment on line 464 claims "step 4.b.v: Await(IteratorValue(next))", but per the ECMAScript spec for Array.fromAsync the nextValue = IteratorValue(next) step carries no Await. Only the output of mapFn is awaited (and only when mapping is enabled). The MDN documentation confirms: "When items is an async iterable, the input to mapFn is not awaited, but the output is."
KValue := AwaitValue(KValue) at line 468 therefore:
- Pre-resolves raw
Promiseobjects yielded by the async iterator before the non-mapping path stores them (spec says they must be preserved as-is). - Passes an already-resolved value to
mapFninstead of the raw yielded value, contradicting the spec's intent that the caller ofmapFnnever awaits its input.
The fix is to remove the premature AwaitValue call from the async-iterator path, keeping only the await on the mapFn output at line 476.
🐛 Proposed fix
- // ES2026 §23.1.2.1 step 4.b.v: Await(IteratorValue(next))
+ // ES2026 §23.1.2.2 step 4.b.iv: nextValue = IteratorValue(next) (no Await)
KValue := NextResult.GetProperty(PROP_VALUE);
if not Assigned(KValue) then
KValue := TGocciaUndefinedLiteralValue.UndefinedValue;
- KValue := AwaitValue(KValue);
if Mapping then
begin
+ // ES2026 §23.1.2.2 step 4.b.v.3: Await(Call(mapfn, thisArg, « nextValue, k »))
MapArgs.SetElement(0, KValue);
MapArgs.SetElement(1, TGocciaNumberLiteralValue.SmallInt(K));
KValue := InvokeCallable(MapCallback, MapArgs, ThisArg);
KValue := AwaitValue(KValue);
end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Builtins.GlobalArray.pas` around lines 461 - 476, The code is
awaiting the iterator's yielded value too early (KValue := AwaitValue(KValue))
which breaks the async-iterable path and the mapping semantics; remove the
AwaitValue call immediately after retrieving KValue from NextResult so that
KValue remains the raw yielded value, and only call AwaitValue on the result of
InvokeCallable(MapCallback, MapArgs, ThisArg) inside the Mapping branch (i.e.,
keep KValue := AwaitValue(KValue) only after MapCallback is invoked),
referencing DoneValue, NextResult, KValue, AwaitValue, Mapping, MapCallback,
InvokeCallable, MapArgs and ThisArg to locate and update the logic.
Comment on lines
+1387
to
+1572
| // ES2026 §14.7.5.6 ForIn/OfBodyEvaluation(lhs, stmt, iteratorRecord, iterationKind, lhsKind) | ||
| function EvaluateForOf(const AForOfStatement: TGocciaForOfStatement; const AContext: TGocciaEvaluationContext): TGocciaValue; | ||
| var | ||
| IterableValue: TGocciaValue; | ||
| Iterator: TGocciaIteratorValue; | ||
| IterResult: TGocciaObjectValue; | ||
| CurrentValue: TGocciaValue; | ||
| IterScope: TGocciaScope; | ||
| IterContext: TGocciaEvaluationContext; | ||
| DeclarationType: TGocciaDeclarationType; | ||
| begin | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
|
|
||
| IterableValue := EvaluateExpression(AForOfStatement.Iterable, AContext); | ||
| Iterator := GetIteratorFromValue(IterableValue); | ||
| if Iterator = nil then | ||
| raise TGocciaTypeError.Create('Value is not iterable', AForOfStatement.Line, AForOfStatement.Column, '', nil); | ||
|
|
||
| if AForOfStatement.IsConst then | ||
| DeclarationType := dtConst | ||
| else | ||
| DeclarationType := dtLet; | ||
|
|
||
| if Assigned(TGocciaGarbageCollector.Instance) then | ||
| TGocciaGarbageCollector.Instance.AddTempRoot(Iterator); | ||
| try | ||
| try | ||
| IterResult := Iterator.AdvanceNext; | ||
| while not IterResult.GetProperty(PROP_DONE).ToBooleanLiteral.Value do | ||
| begin | ||
| CurrentValue := IterResult.GetProperty(PROP_VALUE); | ||
|
|
||
| IterScope := AContext.Scope.CreateChild(skBlock); | ||
| IterContext := AContext; | ||
| IterContext.Scope := IterScope; | ||
|
|
||
| if AForOfStatement.BindingPattern <> nil then | ||
| AssignPattern(AForOfStatement.BindingPattern, CurrentValue, IterContext, True, DeclarationType) | ||
| else | ||
| IterScope.DefineLexicalBinding(AForOfStatement.BindingName, CurrentValue, DeclarationType); | ||
|
|
||
| try | ||
| Evaluate(AForOfStatement.Body, IterContext); | ||
| except | ||
| on E: TGocciaBreakSignal do | ||
| Exit; | ||
| end; | ||
|
|
||
| IterResult := Iterator.AdvanceNext; | ||
| end; | ||
| except | ||
| on E: TGocciaBreakSignal do | ||
| ; | ||
| end; | ||
| finally | ||
| if Assigned(TGocciaGarbageCollector.Instance) then | ||
| TGocciaGarbageCollector.Instance.RemoveTempRoot(Iterator); | ||
| end; | ||
| end; | ||
|
|
||
| // ES2026 §14.7.5.6 ForIn/OfBodyEvaluation — for-await-of variant | ||
| function EvaluateForAwaitOf(const AForAwaitOfStatement: TGocciaForAwaitOfStatement; const AContext: TGocciaEvaluationContext): TGocciaValue; | ||
| var | ||
| IterableValue, IteratorMethod, IteratorObj, NextMethod, NextResult, DoneValue, CurrentValue: TGocciaValue; | ||
| Iterator: TGocciaIteratorValue; | ||
| GenericNextResult: TGocciaObjectValue; | ||
| IterScope: TGocciaScope; | ||
| IterContext: TGocciaEvaluationContext; | ||
| DeclarationType: TGocciaDeclarationType; | ||
| EmptyArgs: TGocciaArgumentsCollection; | ||
| begin | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
|
|
||
| IterableValue := EvaluateExpression(AForAwaitOfStatement.Iterable, AContext); | ||
|
|
||
| if AForAwaitOfStatement.IsConst then | ||
| DeclarationType := dtConst | ||
| else | ||
| DeclarationType := dtLet; | ||
|
|
||
| IteratorMethod := nil; | ||
| if IterableValue is TGocciaObjectValue then | ||
| IteratorMethod := TGocciaObjectValue(IterableValue).GetSymbolProperty(TGocciaSymbolValue.WellKnownAsyncIterator); | ||
|
|
||
| if Assigned(IteratorMethod) and IteratorMethod.IsCallable then | ||
| begin | ||
| EmptyArgs := TGocciaArgumentsCollection.Create; | ||
| try | ||
| IteratorObj := TGocciaFunctionBase(IteratorMethod).Call(EmptyArgs, IterableValue); | ||
| finally | ||
| EmptyArgs.Free; | ||
| end; | ||
|
|
||
| if Assigned(TGocciaGarbageCollector.Instance) then | ||
| TGocciaGarbageCollector.Instance.AddTempRoot(IteratorObj); | ||
| try | ||
| EmptyArgs := TGocciaArgumentsCollection.Create; | ||
| try | ||
| NextMethod := IteratorObj.GetProperty(PROP_NEXT); | ||
| if not Assigned(NextMethod) or not NextMethod.IsCallable then | ||
| ThrowTypeError('Async iterator .next is not callable'); | ||
|
|
||
| while True do | ||
| begin | ||
| NextResult := TGocciaFunctionBase(NextMethod).Call(EmptyArgs, IteratorObj); | ||
| NextResult := AwaitValue(NextResult); | ||
|
|
||
| // ES2026 §7.4.2 step 5: If nextResult is not an Object, throw a TypeError | ||
| if NextResult.IsPrimitive then | ||
| ThrowTypeError('Iterator result ' + NextResult.ToStringLiteral.Value + ' is not an object'); | ||
|
|
||
| DoneValue := NextResult.GetProperty(PROP_DONE); | ||
| if Assigned(DoneValue) and DoneValue.ToBooleanLiteral.Value then | ||
| Break; | ||
|
|
||
| CurrentValue := NextResult.GetProperty(PROP_VALUE); | ||
| if not Assigned(CurrentValue) then | ||
| CurrentValue := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
|
|
||
| IterScope := AContext.Scope.CreateChild(skBlock); | ||
| IterContext := AContext; | ||
| IterContext.Scope := IterScope; | ||
|
|
||
| if AForAwaitOfStatement.BindingPattern <> nil then | ||
| AssignPattern(AForAwaitOfStatement.BindingPattern, CurrentValue, IterContext, True, DeclarationType) | ||
| else | ||
| IterScope.DefineLexicalBinding(AForAwaitOfStatement.BindingName, CurrentValue, DeclarationType); | ||
|
|
||
| try | ||
| Evaluate(AForAwaitOfStatement.Body, IterContext); | ||
| except | ||
| on E: TGocciaBreakSignal do | ||
| Exit; | ||
| end; | ||
| end; | ||
| finally | ||
| EmptyArgs.Free; | ||
| end; | ||
| finally | ||
| if Assigned(TGocciaGarbageCollector.Instance) then | ||
| TGocciaGarbageCollector.Instance.RemoveTempRoot(IteratorObj); | ||
| end; | ||
| end | ||
| else | ||
| begin | ||
| Iterator := GetIteratorFromValue(IterableValue); | ||
| if Iterator = nil then | ||
| ThrowTypeError('Value is not iterable'); | ||
|
|
||
| if Assigned(TGocciaGarbageCollector.Instance) then | ||
| TGocciaGarbageCollector.Instance.AddTempRoot(Iterator); | ||
| try | ||
| try | ||
| GenericNextResult := Iterator.AdvanceNext; | ||
| while not GenericNextResult.GetProperty(PROP_DONE).ToBooleanLiteral.Value do | ||
| begin | ||
| CurrentValue := GenericNextResult.GetProperty(PROP_VALUE); | ||
| CurrentValue := AwaitValue(CurrentValue); | ||
|
|
||
| IterScope := AContext.Scope.CreateChild(skBlock); | ||
| IterContext := AContext; | ||
| IterContext.Scope := IterScope; | ||
|
|
||
| if AForAwaitOfStatement.BindingPattern <> nil then | ||
| AssignPattern(AForAwaitOfStatement.BindingPattern, CurrentValue, IterContext, True, DeclarationType) | ||
| else | ||
| IterScope.DefineLexicalBinding(AForAwaitOfStatement.BindingName, CurrentValue, DeclarationType); | ||
|
|
||
| try | ||
| Evaluate(AForAwaitOfStatement.Body, IterContext); | ||
| except | ||
| on E: TGocciaBreakSignal do | ||
| Exit; | ||
| end; | ||
|
|
||
| GenericNextResult := Iterator.AdvanceNext; | ||
| end; | ||
| except | ||
| on E: TGocciaBreakSignal do | ||
| ; | ||
| end; | ||
| finally | ||
| if Assigned(TGocciaGarbageCollector.Instance) then | ||
| TGocciaGarbageCollector.Instance.RemoveTempRoot(Iterator); | ||
| end; | ||
| end; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find test files related to Evaluator
find . -name "*Test*" -type f | grep -i evaluator | head -20Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
# List all test files in the units directory
ls -la units/*.Test.pas 2>/dev/null | head -20Repository: frostney/GocciaScript
Length of output: 434
🏁 Script executed:
# Search for for-of test cases
rg -l "for.*of|ForOf" --type pascal units/ | grep -i testRepository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
# Check if any test files were recently modified or if they cover iterator/for-of behavior
rg "ForOf|ForAwaitOf|for.*await.*of" --type pascal units/ -A 2 | head -40Repository: frostney/GocciaScript
Length of output: 2786
🏁 Script executed:
# Check if there's a Goccia.Evaluator.Test.pas file
find . -name "*.Test.pas" -type fRepository: frostney/GocciaScript
Length of output: 242
🏁 Script executed:
# Search in all test files for any for-of or iterator testing
rg "for\s+.*\s+of|ForOf|await" --type pascal units/*.Test.pas 2>/dev/null | head -30Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
# Look for test descriptions/assertions that might cover iteration
rg "describe.*for|test.*for|test.*iter" --type pascal units/*.Test.pas -i | head -20Repository: frostney/GocciaScript
Length of output: 2302
🏁 Script executed:
# Check the git history or recent changes to understand if tests were modified
git diff HEAD~1 HEAD -- "units/*.Test.pas" 2>/dev/null | head -100Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
# Look for test infrastructure and build files
ls -la build.pas 2>/dev/null || echo "build.pas not found"
ls -la *.sh 2>/dev/null | head -10
find . -name "*test*" -o -name "*Test*" | grep -v ".git" | grep -E "\.(pas|dpr|sh)$" | head -20Repository: frostney/GocciaScript
Length of output: 424
🏁 Script executed:
# Check if there's a TestRunner that might orchestrate evaluator tests
cat units/Goccia.Builtins.TestAssertions.Test.pas | head -50Repository: frostney/GocciaScript
Length of output: 1337
🏁 Script executed:
# Search for any script-based for-of tests (GocciaScript test code embedded in Pascal)
rg "for\s+\(\s*.*\s+of\s+" units/ --type pascal -B 2 -A 2 | head -50Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
# Look at the actual PR/commit changes to see what was modified
git log --oneline -20 2>/dev/null || echo "No git history available"
git show --stat 2>/dev/null | head -50Repository: frostney/GocciaScript
Length of output: 2973
🏁 Script executed:
# Check if there are benchmark or integration tests for for-of
cat benchmarks/for-of.jsRepository: frostney/GocciaScript
Length of output: 1619
🏁 Script executed:
# Look for any existing for-of test cases in the codebase
rg "for\s*\(" benchmarks/ examples/ 2>/dev/null -A 3 | head -40Repository: frostney/GocciaScript
Length of output: 1254
🏁 Script executed:
# Check what test suites exist and what patterns they follow
wc -l units/*.Test.pasRepository: frostney/GocciaScript
Length of output: 270
🏁 Script executed:
# Look for how tests are structured - check if there's a pattern for testing evaluator functions
head -100 units/Goccia.Values.FunctionValue.Test.pasRepository: frostney/GocciaScript
Length of output: 2962
🏁 Script executed:
# Check git diff to see what files were actually modified in this commit
git diff HEAD~1 HEAD --name-only 2>/dev/null | grep -i testRepository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
# Check if Goccia.Evaluator.Test.pas exists anywhere
find . -name "*Evaluator*Test*" -o -name "*Test*Evaluator*" 2>/dev/nullRepository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
# Look at the specific evaluator changes more carefully - check for iterator-related code
rg "GetIteratorFromValue|AdvanceNext|Iterator\s*=" units/Goccia.Evaluator.pas -A 2 -B 2 | head -50Repository: frostney/GocciaScript
Length of output: 1382
🏁 Script executed:
# Check what the actual test command would discover
ls build/Goccia.*.Test 2>/dev/null || echo "No built test executables found"Repository: frostney/GocciaScript
Length of output: 96
🏁 Script executed:
# Look at the coding guidelines and learnings to understand what test coverage is needed
cat AGENTS.md | grep -A 5 -B 5 "test suite\|Test.pas\|For.*Of" | head -80Repository: frostney/GocciaScript
Length of output: 5651
Run the native Pascal test suite and update test files for evaluator changes.
Per the coding guidelines, when modifying evaluator, AST logic, scope chain, or value types, you must build and run the native Pascal test suite with:
./build.pas clean tests && for t in build/Goccia.*.Test; do "$t"; done
Then update units/*.Test.pas to reflect any changes in behavior. No test files were updated in this commit, and no evidence shows the test suite was run.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Evaluator.pas` around lines 1387 - 1572, The change touches
evaluator logic in EvaluateForOf and EvaluateForAwaitOf but no native Pascal
tests were run or updated; run the full native test suite with "./build.pas
clean tests && for t in build/Goccia.*.Test; do "$t"; done", fix any failing
tests caused by your changes, and update the corresponding units/*.Test.pas
files to reflect the new behavior (adjust expectations/assertions for iterator
semantics, for-await-of async handling, and scope/binding behavior in
EvaluateForOf/EvaluateForAwaitOf) before committing.
This was referenced Apr 20, 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
New Features
Testing
Benchmarks
Documentation