Conversation
📝 WalkthroughWalkthroughThe pull request adds a global Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
Benchmark Results113 benchmarks · 🟢 3 improved · 110 unchanged · avg -0.1% arrays.js — 🟢 3 improved, 8 unchanged · avg +3.6%
classes.js — 10 unchanged · avg -0.2%
closures.js — 11 unchanged · avg -0.8%
collections.js — 12 unchanged · avg -0.9%
destructuring.js — 14 unchanged · avg -0.9%
fibonacci.js — 3 unchanged · avg +0.3%
json.js — 11 unchanged · avg -0.8%
numbers.js — 11 unchanged · avg -0.5%
objects.js — 7 unchanged · avg -0.2%
promises.js — 12 unchanged · avg -1.1%
strings.js — 11 unchanged · avg +0.4%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/built-ins/queueMicrotask/queueMicrotask.js (1)
55-93: Add a test for a throwing callback to verify drain continues.The docs say queueMicrotask errors are silently discarded; a small test asserting that subsequent microtasks still run would lock this in.
Based on learnings: "When implementing a new language feature, create test files under tests/ covering happy paths, edge cases, and error cases following existing directory and naming patterns."➕ Suggested test
+test("queueMicrotask errors don't stop the queue", () => { + const log = []; + queueMicrotask(() => { throw new Error("boom"); }); + queueMicrotask(() => log.push("after")); + return Promise.resolve().then(() => { + expect(log).toEqual(["after"]); + }); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/built-ins/queueMicrotask/queueMicrotask.js` around lines 55 - 93, Add a new unit test in the same tests/built-ins/queueMicrotask/queueMicrotask.js file that verifies a throwing callback passed to queueMicrotask is silently discarded and does not prevent later microtasks from running: schedule a microtask that throws, then schedule another microtask (or use Promise.resolve().then chain) that records a value, and assert after microtask drainage that the recorder contains the expected value while the throw did not propagate; keep the pattern consistent with existing tests (use queueMicrotask, Promise.resolve().then(...) and expect(...).toEqual(...)) and give the test a descriptive name like "queueMicrotask throwing callback does not stop subsequent microtasks".
🤖 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.Globals.pas`:
- Around line 152-170: QueueMicrotaskCallback currently assigns the callback to
Task.Handler and enqueues it without GC protection; call AddTempRoot(Callback)
before TGocciaMicrotaskQueue.Instance.Enqueue(Task) so the JS function is
GC-rooted while stored in the Pascal microtask queue, and then ensure matching
RemoveTempRoot(Task.Handler) calls are made when tasks are removed (e.g., in
TGocciaMicrotaskQueue.Enqueue/DrainQueue/Clear or the code paths that execute or
discard a TGocciaMicrotask) so the temp root is released once the task is
drained or cleared.
- Line 23: Rename the parameters of QueueMicrotaskCallback to use the A prefix
(e.g., change Args to AArgs and ThisValue to AThisValue) and update all
references inside the function body and its declaration to match; also ensure
the function signature and body use 2-space indentation consistent with
.editorconfig.
---
Nitpick comments:
In `@tests/built-ins/queueMicrotask/queueMicrotask.js`:
- Around line 55-93: Add a new unit test in the same
tests/built-ins/queueMicrotask/queueMicrotask.js file that verifies a throwing
callback passed to queueMicrotask is silently discarded and does not prevent
later microtasks from running: schedule a microtask that throws, then schedule
another microtask (or use Promise.resolve().then chain) that records a value,
and assert after microtask drainage that the recorder contains the expected
value while the throw did not propagate; keep the pattern consistent with
existing tests (use queueMicrotask, Promise.resolve().then(...) and
expect(...).toEqual(...)) and give the test a descriptive name like
"queueMicrotask throwing callback does not stop subsequent microtasks".
| function TypeErrorConstructor(Args: TGocciaArgumentsCollection; ThisValue: TGocciaValue): TGocciaValue; | ||
| function ReferenceErrorConstructor(Args: TGocciaArgumentsCollection; ThisValue: TGocciaValue): TGocciaValue; | ||
| function RangeErrorConstructor(Args: TGocciaArgumentsCollection; ThisValue: TGocciaValue): TGocciaValue; | ||
| function QueueMicrotaskCallback(Args: TGocciaArgumentsCollection; ThisValue: TGocciaValue): TGocciaValue; |
There was a problem hiding this comment.
Rename parameters to use the A prefix.
The new method introduces parameters Args and ThisValue, which deviates from the Pascal naming rule used elsewhere in the codebase.
🔧 Suggested rename
- function QueueMicrotaskCallback(Args: TGocciaArgumentsCollection; ThisValue: TGocciaValue): TGocciaValue;
+ function QueueMicrotaskCallback(AArgs: TGocciaArgumentsCollection; AThisValue: TGocciaValue): TGocciaValue;-function TGocciaGlobals.QueueMicrotaskCallback(Args: TGocciaArgumentsCollection; ThisValue: TGocciaValue): TGocciaValue;
+function TGocciaGlobals.QueueMicrotaskCallback(AArgs: TGocciaArgumentsCollection; AThisValue: TGocciaValue): TGocciaValue;- if Args.Length = 0 then
+ if AArgs.Length = 0 then
ThrowTypeError('Failed to execute ''queueMicrotask'': 1 argument required, but only 0 present.');
- Callback := Args.GetElement(0);
+ Callback := AArgs.GetElement(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Builtins.Globals.pas` at line 23, Rename the parameters of
QueueMicrotaskCallback to use the A prefix (e.g., change Args to AArgs and
ThisValue to AThisValue) and update all references inside the function body and
its declaration to match; also ensure the function signature and body use
2-space indentation consistent with .editorconfig.
| function TGocciaGlobals.QueueMicrotaskCallback(Args: TGocciaArgumentsCollection; ThisValue: TGocciaValue): TGocciaValue; | ||
| var | ||
| Callback: TGocciaValue; | ||
| Task: TGocciaMicrotask; | ||
| begin | ||
| if Args.Length = 0 then | ||
| ThrowTypeError('Failed to execute ''queueMicrotask'': 1 argument required, but only 0 present.'); | ||
|
|
||
| Callback := Args.GetElement(0); | ||
| if not Callback.IsCallable then | ||
| ThrowTypeError('Failed to execute ''queueMicrotask'': parameter 1 is not of type ''Function''.'); | ||
|
|
||
| Task.Handler := Callback; | ||
| Task.ResultPromise := nil; | ||
| Task.Value := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| Task.ReactionType := prtFulfill; | ||
|
|
||
| TGocciaMicrotaskQueue.Instance.Enqueue(Task); | ||
|
|
There was a problem hiding this comment.
Ensure queued callbacks are GC-rooted between enqueue and drain.
queueMicrotask can enqueue callbacks that are no longer referenced in any JS scope after the call returns. If GC runs before DrainQueue, those values can be collected because they are only held by the Pascal queue. Please add temp-rooting for microtasks at enqueue time and remove roots when tasks are drained or cleared.
🛡️ Example fix (in `units/Goccia.MicrotaskQueue.pas`)
procedure TGocciaMicrotaskQueue.Enqueue(const AMicrotask: TGocciaMicrotask);
begin
+ if Assigned(TGocciaGC.Instance) then
+ begin
+ if Assigned(AMicrotask.Handler) then
+ TGocciaGC.Instance.AddTempRoot(AMicrotask.Handler);
+ if Assigned(AMicrotask.Value) then
+ TGocciaGC.Instance.AddTempRoot(AMicrotask.Value);
+ if Assigned(AMicrotask.ResultPromise) then
+ TGocciaGC.Instance.AddTempRoot(AMicrotask.ResultPromise);
+ end;
FQueue.Add(AMicrotask);
end;- if Assigned(TGocciaGC.Instance) then
- begin
- if Assigned(Task.Handler) then
- TGocciaGC.Instance.AddTempRoot(Task.Handler);
- if Assigned(Task.Value) then
- TGocciaGC.Instance.AddTempRoot(Task.Value);
- if Assigned(Promise) then
- TGocciaGC.Instance.AddTempRoot(Promise);
- end;
+ // Temp roots already added at Enqueue; just remove in finally. procedure TGocciaMicrotaskQueue.ClearQueue;
+var
+ Task: TGocciaMicrotask;
begin
+ if Assigned(TGocciaGC.Instance) then
+ for Task in FQueue do
+ begin
+ if Assigned(Task.Handler) then
+ TGocciaGC.Instance.RemoveTempRoot(Task.Handler);
+ if Assigned(Task.Value) then
+ TGocciaGC.Instance.RemoveTempRoot(Task.Value);
+ if Assigned(Task.ResultPromise) then
+ TGocciaGC.Instance.RemoveTempRoot(Task.ResultPromise);
+ end;
FQueue.Clear;
end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Builtins.Globals.pas` around lines 152 - 170,
QueueMicrotaskCallback currently assigns the callback to Task.Handler and
enqueues it without GC protection; call AddTempRoot(Callback) before
TGocciaMicrotaskQueue.Instance.Enqueue(Task) so the JS function is GC-rooted
while stored in the Pascal microtask queue, and then ensure matching
RemoveTempRoot(Task.Handler) calls are made when tasks are removed (e.g., in
TGocciaMicrotaskQueue.Enqueue/DrainQueue/Clear or the code paths that execute or
discard a TGocciaMicrotask) so the temp root is released once the task is
drained or cleared.
Summary by CodeRabbit
New Features
queueMicrotask()global function for enqueueing callbacks in the microtask queue, sharing the same queue as Promise reactions.Documentation
queueMicrotask()integration, behavior, and error handling semantics.Tests