Support array-like arguments for apply and Reflect#244
Merged
Conversation
- Add shared CreateListFromArrayLike helper - Accept array-like arguments in Function.prototype.apply, Reflect.apply, and Reflect.construct - Expand end-to-end tests for array-like and invalid inputs
Contributor
📝 WalkthroughWalkthroughImplements CreateListFromArrayLike and updates Function.prototype.apply, Reflect.apply, and Reflect.construct to accept array-like argument lists; adds/expands tests covering array-like inputs, edge cases, and invalid argumentsList values. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as JS Caller
participant Builtin as Function/Reflect builtin
participant Converter as CreateListFromArrayLike
participant Target as Target function/constructor
Caller->>Builtin: invoke apply/construct(target, thisArg/newTarget, argumentsList)
Builtin->>Builtin: validate target (isFunction / isConstructor)
alt argumentsList is null/undefined or omitted
Builtin->>Target: Call/Construct with no args
Target-->>Builtin: result
else argumentsList is TGocciaArrayValue (fast-path)
Builtin->>Target: fast-path Call/Construct with 0..3 args
Target-->>Builtin: result
else array-like object
Builtin->>Converter: CreateListFromArrayLike(argumentsList, methodName)
Converter-->>Builtin: TGocciaArgumentsCollection
Builtin->>Target: Call/Construct with constructed args
Target-->>Builtin: result
end
Builtin-->>Caller: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/built-ins/Function/prototype/apply.js`:
- Around line 45-68: Add two tests to cover absent and non-numeric length
handling in Function.prototype.apply: one where an array-like value is a plain
{} (no length) and one where length is a non-numeric string like "foo"; both
should behave as zero-length argument lists when calling a function (e.g., use a
function fn = () => "no-args" or fn = (...args) => args.length and assert
apply(undefined, {}) yields zero-args / "no-args" and apply(undefined, { length:
"foo" }) also yields zero-args). Place these alongside the existing tests (refer
to the existing test cases and test names such as "works with array-like
objects" and "works with array-like object with string length") so the suite
prevents regressions in ToLength handling shared with
Reflect.apply/Reflect.construct.
In `@units/Goccia.Arguments.ArrayLike.pas`:
- Around line 52-58: LengthProp returned by ArrayObj.GetProperty can be nil and
must be treated like undefined before dereferencing; update the logic in the
block that sets Len so it first checks "if LengthProp = nil or (LengthProp is
TGocciaUndefinedLiteralValue) or (LengthProp is TGocciaNullLiteralValue) then
Len := 0 else Len := Trunc(LengthProp.ToNumberLiteral.Value)". Use the existing
LengthProp symbol and TGocciaUndefinedLiteralValue/TGocciaNullLiteralValue type
checks to locate the code and ensure ToNumberLiteral.Value is only accessed when
LengthProp is non-nil.
- Around line 58-61: The current use of Trunc on
LengthProp.ToNumberLiteral.Value (assigning to Len) throws on NaN/±Infinity and
out-of-range values; update the ToLength logic so you first read the numeric
value (e.g., value := LengthProp.ToNumberLiteral.Value) and then: if
IsNan(value) or (value = 0) or IsNegativeInfinite(value) set Len := 0; else if
IsPositiveInfinite(value) set Len := 9007199254740991 (2^53-1); otherwise
truncate toward zero and clamp the result to the 0..9007199254740991 range
before assigning to Len. Use IsNan/IsInfinite checks instead of calling Trunc
directly and ensure the constant 9007199254740991 (MaxSafeInteger) is used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98a4fd08-5dab-4283-be33-083ecae50e43
📒 Files selected for processing (6)
tests/built-ins/Function/prototype/apply.jstests/built-ins/Reflect/apply.jstests/built-ins/Reflect/construct.jsunits/Goccia.Arguments.ArrayLike.pasunits/Goccia.Builtins.GlobalReflect.pasunits/Goccia.Values.FunctionBase.pas
Contributor
Benchmark Results274 benchmarks Interpreted: 🟢 207 improved · 🔴 18 regressed · 49 unchanged · avg +4.8% arraybuffer.js — Interp: 🟢 12, 🔴 2 · avg +8.1% · Bytecode: 🟢 4, 🔴 3, 7 unch. · avg +0.7%
arrays.js — Interp: 🟢 18, 1 unch. · avg +6.2% · Bytecode: 🟢 12, 🔴 1, 6 unch. · avg +2.0%
async-await.js — Interp: 🟢 6 · avg +11.2% · Bytecode: 🟢 1, 5 unch. · avg +0.9%
classes.js — Interp: 🟢 23, 🔴 1, 7 unch. · avg +4.9% · Bytecode: 🟢 8, 🔴 2, 21 unch. · avg +2.5%
closures.js — Interp: 🟢 11 · avg +5.8% · Bytecode: 🟢 7, 4 unch. · avg +5.1%
collections.js — Interp: 🟢 9, 🔴 1, 2 unch. · avg +11.1% · Bytecode: 🟢 6, 🔴 2, 4 unch. · avg +1.9%
destructuring.js — Interp: 🟢 12, 🔴 1, 9 unch. · avg +2.5% · Bytecode: 🟢 22 · avg +8.4%
fibonacci.js — Interp: 🟢 5, 3 unch. · avg +6.7% · Bytecode: 🟢 4, 🔴 2, 2 unch. · avg +3.7%
for-of.js — Interp: 🔴 1, 6 unch. · avg -0.3% · Bytecode: 🟢 7 · avg +7.1%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 17, 3 unch. · avg +3.9% · Bytecode: 🟢 18, 2 unch. · avg +5.9%
json.js — Interp: 🟢 19, 1 unch. · avg +6.8% · Bytecode: 🟢 15, 5 unch. · avg +3.7%
jsx.jsx — Interp: 🟢 14, 7 unch. · avg +1.2% · Bytecode: 🟢 11, 🔴 2, 8 unch. · avg +1.7%
modules.js — Interp: 🟢 9 · avg +5.6% · Bytecode: 🟢 2, 🔴 7 · avg -0.5%
numbers.js — Interp: 🟢 11 · avg +8.2% · Bytecode: 🟢 3, 🔴 3, 5 unch. · avg -0.0%
objects.js — Interp: 🟢 2, 🔴 4, 1 unch. · avg -0.5% · Bytecode: 🟢 5, 2 unch. · avg +2.5%
promises.js — Interp: 🟢 11, 1 unch. · avg +3.0% · Bytecode: 🟢 11, 1 unch. · avg +4.1%
regexp.js — Interp: 🟢 8, 🔴 1, 2 unch. · avg +4.0% · Bytecode: 🟢 6, 🔴 1, 4 unch. · avg +2.4%
strings.js — Interp: 🟢 9, 2 unch. · avg +6.7% · Bytecode: 🟢 8, 3 unch. · avg +2.9%
typed-arrays.js — Interp: 🟢 11, 🔴 7, 4 unch. · avg +1.0% · Bytecode: 🟢 15, 🔴 1, 6 unch. · avg +2.7%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
…istFromArrayLike
Address CodeRabbit review feedback:
- Guard against nil from GetProperty before dereferencing LengthProp
- Use IsNan/range checks before Trunc to avoid EInvalidOp on NaN/Infinity
- Add tests for absent length ({}) and non-numeric string length ({length:"foo"})
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
units/Goccia.Arguments.ArrayLike.pas (1)
51-53: Minor doc inconsistency: comment says "cap at 2^53−1" but code caps atMaxInt.The comment references ES spec's 2^53−1 limit, but line 66-67 uses
MaxInt. This is a practical and correct limitation sinceLen: Integercannot exceedMaxIntanyway, and allocating billions of arguments is infeasible. Consider updating the comment to reflect the actual behavior.📝 Suggested documentation fix
// ES2026 §7.3.18 step 2: Let len be ? LengthOfArrayLike(obj) // ES2026 §7.3.3 LengthOfArrayLike: ToLength(? Get(obj, "length")) - // ES2026 §7.1.22 ToLength: NaN/negative → 0, cap at 2^53−1 + // ES2026 §7.1.22 ToLength: NaN/negative → 0, cap at MaxInt (practical limit)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Arguments.ArrayLike.pas` around lines 51 - 53, The comment in units/Goccia.Arguments.ArrayLike.pas incorrectly states the ES spec cap as "2^53−1" while the implementation for LengthOfArrayLike (ToLength → Len: Integer) actually uses MaxInt; update the comment to reflect the real behavior by noting that ToLength is conceptually capped at 2^53−1 per the ES spec but this implementation further restricts length to the platform Integer limit (MaxInt) and that extremely large allocations are infeasible — reference the LengthOfArrayLike/ToLength explanation and the Len: Integer/MaxInt behavior in the comment so readers understand the practical cap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@units/Goccia.Arguments.ArrayLike.pas`:
- Around line 51-53: The comment in units/Goccia.Arguments.ArrayLike.pas
incorrectly states the ES spec cap as "2^53−1" while the implementation for
LengthOfArrayLike (ToLength → Len: Integer) actually uses MaxInt; update the
comment to reflect the real behavior by noting that ToLength is conceptually
capped at 2^53−1 per the ES spec but this implementation further restricts
length to the platform Integer limit (MaxInt) and that extremely large
allocations are infeasible — reference the LengthOfArrayLike/ToLength
explanation and the Len: Integer/MaxInt behavior in the comment so readers
understand the practical cap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6857eef-26af-49b4-b999-f5d2f1444773
📒 Files selected for processing (2)
tests/built-ins/Function/prototype/apply.jsunits/Goccia.Arguments.ArrayLike.pas
✅ Files skipped from review due to trivial changes (1)
- tests/built-ins/Function/prototype/apply.js
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.Arguments.ArrayLike.pas`:
- Around line 74-75: Before calling
TGocciaArgumentsCollection.CreateWithCapacity, enforce a safe upper bound on Len
(which is script-controlled) and raise a RangeError if it exceeds that ceiling;
compute a boundedCount = Min(Len, MAX_SAFE_ARGS) (choose a constant like
MAX_SAFE_ARGS) and use boundedCount in the call to CreateWithCapacity, and if
Len > MAX_SAFE_ARGS raise a RangeError (or equivalent) before any allocation to
avoid pathological OOM/DoS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 665fb80a-ec82-4532-b98e-2bffb1812de4
📒 Files selected for processing (1)
units/Goccia.Arguments.ArrayLike.pas
…tFromArrayLike
Script-controlled length values (e.g. { length: 2e9 }) would allocate
multi-GB collections before iterating. Cap at MAX_ARGUMENTS_LIST_LENGTH
(2^20) and throw RangeError if exceeded.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
frostney
added
the
spec compliance
This was referenced Apr 21, 2026
5 tasks
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
CreateListFromArrayLikehelper to convert arrays and array-like objects into argument lists.Function.prototype.apply,Reflect.apply, andReflect.constructto accept array-likeargumentsListvalues.lengthvalues, and invalid non-object arguments.Fixes #231
Testing
tests/built-ins/Function/prototype/apply.js.tests/built-ins/Reflect/apply.js.tests/built-ins/Reflect/construct.js.