Conversation
📝 WalkthroughWalkthroughAdds native Promise bridging and static/prototype delegates, wires Promise constructor and methods into the runtime and microtask flows, and refactors iterators to expose DirectNext(out ADone: Boolean) across concrete, generic, and lazy iterator types for direct value retrieval. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.5)
participant JS as JavaScript caller
participant C as TGocciaRuntimeOperations (native bridge)
participant P as Promise constructor/value
participant MQ as MicrotaskQueue
participant H as Reaction handlers (then/catch/finally)
end
JS->>C: new Promise(...) (bridged constructor)
C->>P: create TGocciaPromiseValue, store FPromiseConstructor
JS->>P: attach .then/.catch/.finally (resolved via delegates)
P->>MQ: enqueue microtask on resolve/reject
MQ->>H: run reaction callbacks
H-->>JS: invoke user callbacks (possibly bridged callables)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Benchmark Results254 benchmarks Interpreted: 🟢 30 improved · 🔴 176 regressed · 48 unchanged · avg -10.5% arraybuffer.js — Interp: 🟢 4, 🔴 1, 9 unch. · avg +4.1% · Bytecode: 🔴 1, 13 unch. · avg -3.3%
arrays.js — Interp: 🟢 5, 🔴 5, 9 unch. · avg -0.3% · Bytecode: 19 unch. · avg -2.1%
async-await.js — Interp: 🟢 4, 2 unch. · avg +9.9% · Bytecode: 🟢 5, 1 unch. · avg +11.9%
classes.js — Interp: 🟢 2, 🔴 19, 10 unch. · avg -9.3% · Bytecode: 🔴 1, 30 unch. · avg -2.5%
closures.js — Interp: 🔴 11 · avg -23.5% · Bytecode: 🟢 1, 10 unch. · avg +1.8%
collections.js — Interp: 🔴 7, 5 unch. · avg -14.6% · Bytecode: 🟢 4, 🔴 1, 7 unch. · avg +231.1%
destructuring.js — Interp: 🔴 22 · avg -25.7% · Bytecode: 🟢 1, 21 unch. · avg -1.3%
fibonacci.js — Interp: 🟢 2, 🔴 4, 2 unch. · avg -7.1% · Bytecode: 🟢 4, 4 unch. · avg +16.4%
for-of.js — Interp: 🔴 7 · avg -24.6% · Bytecode: 0
iterators.js — Interp: 🟢 12, 🔴 1, 7 unch. · avg +60.1% · Bytecode: 🆕 20
json.js — Interp: 🔴 19, 1 unch. · avg -15.6% · Bytecode: 0
jsx.jsx — Interp: 🔴 21 · avg -27.5% · Bytecode: 🆕 7
numbers.js — Interp: 🔴 9, 2 unch. · avg -14.6% · Bytecode: 0
objects.js — Interp: 🔴 7 · avg -30.0% · Bytecode: 0
promises.js — Interp: 🔴 12 · avg -26.6% · Bytecode: 🆕 4
strings.js — Interp: 🔴 11 · avg -32.5% · Bytecode: 0
typed-arrays.js — Interp: 🟢 1, 🔴 20, 1 unch. · avg -24.2% · Bytecode: 0
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
units/Goccia.Values.PromiseValue.pas (1)
409-473: Add ES spec anchors above the new Promise helper paths.
InvokeThen,InvokeFinally, and the rewritten prototype entry points are new Promise-semantics code paths, so they should carry the required// ESYYYY §...annotations immediately above the implementation bodies.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 SpecMethodName(specParams)whereYYYYis the current edition year."Also applies to: 477-527
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.PromiseValue.pas` around lines 409 - 473, Add ECMAScript spec anchor comments above the new Promise helper implementations: place lines like "// ES2021 §25.4.5.3 InvokeThen(spec?)" (use the correct year & section) immediately above the TGocciaPromiseValue.InvokeThen implementation, similarly add the appropriate "// ES2021 §25.4.5.4 InvokeFinally(...)" comment above TGocciaPromiseValue.InvokeFinally, and add matching ES anchors above the rewritten prototype entry-point methods referenced in this diff; ensure the format follows the project convention "// ESYYYY §X.Y.Z SpecMethodName(specParams)".units/Goccia.Runtime.Operations.pas (1)
7259-7274: Please move the new Promise key strings into the constants units.
then,catch,finally,allSettled,withResolvers, etc. are runtime property names, and this unit already imports the constants units that centralize them.As per coding guidelines, "Use constant units for runtime values instead of hardcoded string literals:
Goccia.Constants.PropertyNamesfor property names,Goccia.Constants.TypeNamesfor type names,Goccia.Constants.ErrorNamesfor error names,Goccia.Constants.ConstructorNamesfor constructor names,Goccia.Constants.SymbolNamesfor symbol names, andGoccia.Constantsfor literal values."Also applies to: 7491-7496
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Runtime.Operations.pas` around lines 7259 - 7274, Replace hardcoded promise property name strings in the PROMISE_PROTOTYPE_METHODS and PROMISE_STATIC_METHODS arrays with the centralized constants from the constants units (e.g., use Goccia.Constants.PropertyNames.<Then/Catch/Finally/AllSettled/WithResolvers/Try/All/Race/Any/Resolve/Reject> as appropriate) instead of literal values; update the Callback references remain unchanged (NativePromiseThen, NativePromiseCatch, NativePromiseFinally, NativePromiseStaticResolve, NativePromiseStaticReject, NativePromiseStaticAll, NativePromiseStaticAllSettled, NativePromiseStaticRace, NativePromiseStaticAny, NativePromiseStaticWithResolvers, NativePromiseStaticTry) and ensure the unit uses/imports the Goccia.Constants.PropertyNames unit (or the specific constants unit already imported) so all runtime property names are sourced from the constants module; apply the same replacement for the similar entries noted around the other occurrence (the second array mentioned).
🤖 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.Runtime.Operations.pas`:
- Around line 7295-7298: Mark the newly created TSouffleRecord delegates
FPromiseDelegate and FPromiseStaticDelegate inside the MarkExternalRoots routine
so they cannot be collected; update MarkExternalRoots to check the runtime
fields (FPromiseDelegate, FPromiseStaticDelegate) and explicitly mark/root them
(or call the existing record-marking helper) if they are assigned, similar to
how other runtime delegates are rooted, ensuring the delegates produced by
BuildDelegate(PROMISE_PROTOTYPE_METHODS) and
BuildDelegate(PROMISE_STATIC_METHODS) are treated as external roots before GC
runs.
- Around line 3733-3747: The fast-path that clones TGocciaArrayValue from the
iterator is too broad; narrow it so you only clone plain array values (not
arrays that are themselves iterator-derived) by changing the condition in the
DirectNext handling: keep the check for (Value is TGocciaArrayValue) and
FArrayBridgeReverse but add a guard to skip cloning when Value is a
TGocciaIteratorValue (e.g. add "and not (Value is TGocciaIteratorValue)"); for
any array value that fails that guard, call ToSouffleValue(Value) instead so
iterator-produced arrays keep their original identity/mutation/Array prototype
behavior (and do not omit VM.ArrayDelegate).
- Around line 2791-2794: The current code returns early when AObject.AsReference
= FPromiseConstructor and FPromiseStaticDelegate.Get(AKey, Result) succeeds,
which bypasses the real object's property lookup and prevents wrapped Promises
from exposing changed/instance properties (e.g., p.then). Change the logic in
the block that checks AObject.AsReference, FPromiseConstructor and
FPromiseStaticDelegate.Get(AKey, Result) so it does not Exit immediately;
instead perform the normal property lookup on the underlying object first and
only apply the static delegate fallback if the object's lookup did not find the
property (i.e., call the usual lookup before or check that the object's lookup
returned false), and make the identical change for the analogous code at the
2821-2824 site.
In `@units/Goccia.Values.PromiseValue.pas`:
- Around line 457-472: The TGocciaPromiseValue.InvokeFinally creates wrapper
objects (WrapFulfilled, WrapRejected) that are only referenced via bound method
pointers and can be collected after this function returns; pin each wrapper with
the garbage collector immediately after creation (e.g. call
TGocciaGarbageCollector.Instance.PinValue(WrapFulfilled) and
PinValue(WrapRejected)) so they remain alive while reactions are queued, and
apply the same pinning for the TGocciaFinallyPassthrough instance created in the
wrapper's Invoke; alternatively, implement
TGocciaNativeFunctionValue.MarkReferences to mark its method-host TGocciaValue
so these wrappers are traced by GC.
---
Nitpick comments:
In `@units/Goccia.Runtime.Operations.pas`:
- Around line 7259-7274: Replace hardcoded promise property name strings in the
PROMISE_PROTOTYPE_METHODS and PROMISE_STATIC_METHODS arrays with the centralized
constants from the constants units (e.g., use
Goccia.Constants.PropertyNames.<Then/Catch/Finally/AllSettled/WithResolvers/Try/All/Race/Any/Resolve/Reject>
as appropriate) instead of literal values; update the Callback references remain
unchanged (NativePromiseThen, NativePromiseCatch, NativePromiseFinally,
NativePromiseStaticResolve, NativePromiseStaticReject, NativePromiseStaticAll,
NativePromiseStaticAllSettled, NativePromiseStaticRace, NativePromiseStaticAny,
NativePromiseStaticWithResolvers, NativePromiseStaticTry) and ensure the unit
uses/imports the Goccia.Constants.PropertyNames unit (or the specific constants
unit already imported) so all runtime property names are sourced from the
constants module; apply the same replacement for the similar entries noted
around the other occurrence (the second array mentioned).
In `@units/Goccia.Values.PromiseValue.pas`:
- Around line 409-473: Add ECMAScript spec anchor comments above the new Promise
helper implementations: place lines like "// ES2021 §25.4.5.3 InvokeThen(spec?)"
(use the correct year & section) immediately above the
TGocciaPromiseValue.InvokeThen implementation, similarly add the appropriate "//
ES2021 §25.4.5.4 InvokeFinally(...)" comment above
TGocciaPromiseValue.InvokeFinally, and add matching ES anchors above the
rewritten prototype entry-point methods referenced in this diff; ensure the
format follows the project convention "// ESYYYY §X.Y.Z
SpecMethodName(specParams)".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa4ce0f3-b148-4f3a-8896-4cfd0e42aa84
📒 Files selected for processing (6)
units/Goccia.Runtime.Operations.pasunits/Goccia.Values.Iterator.Concrete.pasunits/Goccia.Values.Iterator.Generic.pasunits/Goccia.Values.Iterator.Lazy.pasunits/Goccia.Values.IteratorValue.pasunits/Goccia.Values.PromiseValue.pas
…tion Add FPromiseDelegate and FPromiseStaticDelegate to MarkExternalRoots, matching the pattern used by String, Number, Describe, and Test delegates. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
units/Goccia.Runtime.Operations.pas (1)
5994-6132: Add ES section annotations to the new Promise builtins.These callbacks implement ECMAScript Promise behavior, but unlike the other spec-driven builtins in this unit they do not have
// ES2026 §...markers above the function bodies and key steps.As per coding guidelines:
When implementing ECMAScript-specified behavior, add // ESYYYY §X.Y.Z spec 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.Runtime.Operations.pas` around lines 5994 - 6132, The new Promise builtin callbacks lack the required ECMAScript spec annotations; add "// ES2026 §X.Y.Z" style comments above each function (NativePromiseThen, NativePromiseCatch, NativePromiseFinally, NativePromiseStaticResolve, NativePromiseStaticReject, NativePromiseStaticAll, NativePromiseStaticAllSettled, NativePromiseStaticRace, NativePromiseStaticAny, NativePromiseStaticWithResolvers, NativePromiseStaticTry) and insert matching "// ES2026 §X.Y.Z — step ..." markers at key algorithm steps inside each function body to mirror the spec-driven builtins per the coding guidelines; ensure the section numbers match the actual ECMAScript spec locations you relied upon and place the annotations immediately above the function declaration and before any important branches/steps.
🤖 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.Runtime.Operations.pas`:
- Around line 5959-6132: BridgePromiseStatic currently invokes the cached Goccia
function with an undefined receiver which loses subclass receivers (breaking
SubPromise.resolve/all/etc.); change BridgePromiseStatic to accept and forward
the AReceiver to TGocciaFunctionBase.Call instead of
TGocciaUndefinedLiteralValue.UndefinedValue, and update
NativePromiseStaticResolve and NativePromiseStaticReject to not construct
TGocciaPromiseValue directly but to invoke the constructor tied to the provided
receiver (i.e., call the promise constructor on AReceiver or route through the
modified BridgePromiseStatic/GPromise*Fn paths) so subclass-aware semantics are
preserved (references: BridgePromiseStatic, NativePromiseStaticResolve,
NativePromiseStaticReject,
GPromiseAllFn/GPromiseAnyFn/GPromiseRaceFn/GPromiseAllSettledFn/GPromiseWithResolversFn/GPromiseTryFn).
---
Nitpick comments:
In `@units/Goccia.Runtime.Operations.pas`:
- Around line 5994-6132: The new Promise builtin callbacks lack the required
ECMAScript spec annotations; add "// ES2026 §X.Y.Z" style comments above each
function (NativePromiseThen, NativePromiseCatch, NativePromiseFinally,
NativePromiseStaticResolve, NativePromiseStaticReject, NativePromiseStaticAll,
NativePromiseStaticAllSettled, NativePromiseStaticRace, NativePromiseStaticAny,
NativePromiseStaticWithResolvers, NativePromiseStaticTry) and insert matching
"// ES2026 §X.Y.Z — step ..." markers at key algorithm steps inside each
function body to mirror the spec-driven builtins per the coding guidelines;
ensure the section numbers match the actual ECMAScript spec locations you relied
upon and place the annotations immediately above the function declaration and
before any important branches/steps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90d2f770-eb28-4c3f-96a6-89ee0836b191
📒 Files selected for processing (1)
units/Goccia.Runtime.Operations.pas
Acknowledged. Benchmark report shows strong iterator and Promise bytecode gains. The widespread interpreted-mode regressions appear to be measurement noise from the CI runner (consistent ~20-30% across unrelated categories like JSON, strings, objects, JSX — unlikely to be caused by iterator/Promise changes). |
Summary by CodeRabbit
New Features
Performance