Handle iterable and array-like arguments in TypedArray constructor, from(), and set()#558
Merged
Merged
Conversation
…rom(), and set() (#551) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughExtends TypedArray constructor, TypedArray.from, and TypedArray.prototype.set to accept iterables and array-like objects per ES2026: prefer @@iterator iteration, otherwise use ToObject + LengthOfArrayLike + indexed reads; adds tests for constructors, from, and set behaviors. ChangesTypedArray Iterable and Array-Like Construction Support
Sequence Diagram(s)sequenceDiagram
participant Source
participant Iterator
participant Collector
participant TypedArray
Source->>Iterator: Get @@iterator / GetIteratorFromValue
alt iterable
Iterator->>Collector: next() -> value
Collector->>TypedArray: allocate(length) / apply mapfn / write values
else array-like
Source->>Source: ToObject / LengthOfArrayLike
Source->>TypedArray: Get indexed properties -> apply mapfn -> write
end
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. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
coderabbitai
Bot
added
bug
Contributor
Suite TimingTest Runner (interpreted: 8,844 passed; bytecode: 8,844 passed)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Test runner worker shutdown frees thread-local heaps in bulk; that shutdown reclamation is not counted as GC collections or collected objects.
Benchmarks (interpreted: 407; bytecode: 407)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Benchmark runner performs explicit between-file collections, so collection and collected-object counts can be much higher than the test runner.
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results407 benchmarks Interpreted: 🟢 12 improved · 🔴 233 regressed · 162 unchanged · avg -2.7% arraybuffer.js — Interp: 🔴 13, 1 unch. · avg -3.5% · Bytecode: 🔴 6, 8 unch. · avg -2.8%
arrays.js — Interp: 🔴 11, 8 unch. · avg -3.4% · Bytecode: 🔴 5, 14 unch. · avg -1.4%
async-await.js — Interp: 🔴 4, 2 unch. · avg -4.4% · Bytecode: 🟢 1, 5 unch. · avg +0.9%
async-generators.js — Interp: 2 unch. · avg -9.7% · Bytecode: 2 unch. · avg +1.5%
base64.js — Interp: 🔴 7, 3 unch. · avg -5.8% · Bytecode: 🟢 1, 🔴 5, 4 unch. · avg -5.0%
classes.js — Interp: 🔴 14, 17 unch. · avg -3.2% · Bytecode: 🟢 6, 🔴 5, 20 unch. · avg -0.8%
closures.js — Interp: 🔴 8, 3 unch. · avg -4.0% · Bytecode: 🟢 1, 🔴 2, 8 unch. · avg -1.2%
collections.js — Interp: 🔴 7, 5 unch. · avg -3.1% · Bytecode: 🟢 4, 🔴 1, 7 unch. · avg +1.4%
csv.js — Interp: 🔴 5, 8 unch. · avg -2.4% · Bytecode: 🔴 1, 12 unch. · avg -0.3%
destructuring.js — Interp: 🔴 15, 7 unch. · avg -2.9% · Bytecode: 🟢 3, 🔴 5, 14 unch. · avg -0.9%
fibonacci.js — Interp: 🔴 6, 2 unch. · avg -3.3% · Bytecode: 🔴 3, 5 unch. · avg -1.5%
float16array.js — Interp: 🟢 1, 🔴 19, 12 unch. · avg -5.9% · Bytecode: 🟢 12, 🔴 7, 13 unch. · avg +9.3%
for-of.js — Interp: 🔴 7 · avg -3.9% · Bytecode: 🔴 3, 4 unch. · avg -0.9%
generators.js — Interp: 🔴 3, 1 unch. · avg -4.4% · Bytecode: 🟢 1, 🔴 1, 2 unch. · avg -0.2%
iterators.js — Interp: 🔴 21, 21 unch. · avg -3.4% · Bytecode: 🟢 1, 🔴 17, 24 unch. · avg -1.8%
json.js — Interp: 🔴 4, 16 unch. · avg -2.3% · Bytecode: 🔴 6, 14 unch. · avg -1.7%
jsx.jsx — Interp: 🟢 2, 🔴 13, 6 unch. · avg -2.8% · Bytecode: 🔴 13, 8 unch. · avg -2.9%
modules.js — Interp: 🔴 6, 3 unch. · avg -5.9% · Bytecode: 🔴 2, 7 unch. · avg -0.7%
numbers.js — Interp: 🔴 7, 4 unch. · avg -2.5% · Bytecode: 🔴 1, 10 unch. · avg +0.7%
objects.js — Interp: 🔴 7 · avg -5.2% · Bytecode: 🔴 4, 3 unch. · avg -2.9%
promises.js — Interp: 🔴 7, 5 unch. · avg -3.5% · Bytecode: 🔴 4, 8 unch. · avg -1.6%
regexp.js — Interp: 🔴 9, 2 unch. · avg -4.1% · Bytecode: 🟢 1, 🔴 3, 7 unch. · avg -0.3%
strings.js — Interp: 🔴 7, 12 unch. · avg -2.4% · Bytecode: 🟢 2, 🔴 4, 13 unch. · avg -0.1%
tsv.js — Interp: 🔴 6, 3 unch. · avg -4.7% · Bytecode: 🔴 4, 5 unch. · avg -1.2%
typed-arrays.js — Interp: 🟢 1, 🔴 12, 9 unch. · avg -3.7% · Bytecode: 🟢 2, 🔴 12, 8 unch. · avg -12.4%
uint8array-encoding.js — Interp: 🔴 11, 7 unch. · avg -12.3% · Bytecode: 🟢 2, 🔴 5, 11 unch. · avg +5.2%
weak-collections.js — Interp: 🟢 8, 🔴 4, 3 unch. · avg +33.1% · Bytecode: 🟢 5, 🔴 1, 9 unch. · avg +5.8%
Deterministic profile diffDeterministic profile diff: no significant changes. 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. |
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/units/Goccia.Values.TypedArrayValue.pas`:
- Around line 2116-2138: The iterator consumption block that builds Values from
Iterator (using GetIteratorFromValue, Iterator.AdvanceNext and
IterResult.GetProperty) must be wrapped with try/except so that on any exception
you call CloseIteratorPreservingError (and for the raw-iterator path call
CloseRawIteratorPreservingError) before re-raising; specifically, change the
flow around TGocciaValueList creation /
TGarbageCollector.Instance.AddTempRoot(Iterator) / Iterator.AdvanceNext loop to
catch exceptions, call the appropriate Close*PreservingError helper with the
Iterator, ensure TGarbageCollector.Instance.RemoveTempRoot(Iterator) still runs,
then re-raise the original exception, and apply the same pattern to the other
iterable path that eventually creates TGocciaTypedArrayValue and uses
WriteValueToElement.
- Around line 2113-2148: The iterable detection (GetIteratorFromValue/Iterator
handling) must run before the TGocciaArrayValue/TGocciaTypedArrayValue
fast-paths so an object with an overridden Symbol.iterator wins; move the block
that calls GetIteratorFromValue and iterates (using Iterator.AdvanceNext /
PROP_DONE / PROP_VALUE into Values) to precede any type-check branches for
TGocciaArrayValue and TGocciaTypedArrayValue, then keep the
array-like/LengthOfArrayLike fallback after those iterator and fast-path checks;
apply the same reordering for the other occurrence around the code that spans
the later block (the similar section at the referenced second location).
🪄 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: f9d916d2-6f04-4b0d-8599-1f1cf7304852
📒 Files selected for processing (4)
source/units/Goccia.Values.TypedArrayValue.pastests/built-ins/TypedArray/constructors.jstests/built-ins/TypedArray/from.jstests/built-ins/TypedArray/prototype/set.js
Contributor
test262 Conformance
Areas closest to 100%
Per-test deltas (+174 / -0)Newly passing (174):
Steady-state failures are non-blocking; regressions vs the cached main baseline (lower total pass count, or any PASS → non-PASS transition) fail the conformance gate. Measured on ubuntu-latest x64, bytecode mode. Areas grouped by the first two test262 path components; minimum 25 attempted tests, areas already at 100% excluded. Δ vs main compares against the most recent cached |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
source/units/Goccia.Values.TypedArrayValue.pas (2)
2104-2116:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
TGocciaArrayValuefast path still sits before the@@iteratorcheck — spec ordering not restored.Per ES2026 §23.2.5.1, step 5a (
GetMethod(firstArgument, @@iterator)) applies to all objects, including plain arrays. The current layout exits at line 2110 for anyTGocciaArrayValueinput, so an array with an overriddenSymbol.iteratorwill always useElementsdirectly and never reach the iterable path at line 2116.The
TGocciaTypedArrayValue,TGocciaArrayBufferValue, andTGocciaSharedArrayBufferValuefast paths are spec-defined steps that correctly precede the iterator check; theTGocciaArrayValuefast path is not.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/units/Goccia.Values.TypedArrayValue.pas` around lines 2104 - 2116, The TGocciaArrayValue fast path is placed before the @@iterator check so arrays with a custom Symbol.iterator never invoke the iterable branch; move the TGocciaArrayValue handling (the block that creates TGocciaTypedArrayValue from SrcArr.Elements) to after the GetMethod/GetIteratorFromValue check (i.e. after the code that calls GetIteratorFromValue and handles iterables) so that GetMethod(firstArgument, @@iterator) runs for all objects (matching ES2026 §23.2.5.1 step 5a); keep the existing fast paths for TGocciaTypedArrayValue, TGocciaArrayBufferValue, and TGocciaSharedArrayBufferValue before the iterator check as they are spec-allowed.
2218-2265:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame spec ordering gap in
TypedArrayFrom: both fast paths sit before the@@iteratorcheck.Per ES2026 §23.2.2.1, step 5 (
GetMethod(source, @@iterator)) fires before any source-type dispatch — even forTGocciaTypedArrayValuesources (typed arrays exposeSymbol.iteratorvia the shared prototype registered at line 624). TheTGocciaTypedArrayValuefast path at line 2218 and theTGocciaArrayValuefast path at line 2241 both exit before the iterator check at line 2265.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/units/Goccia.Values.TypedArrayValue.pas` around lines 2218 - 2265, The fast-paths in TypedArrayFrom run before the spec-required iterator check; move the iterator detection (call to GetIteratorFromValue(Source) and the handling of a non-nil Iterator) so it executes before the TGocciaTypedArrayValue and TGocciaArrayValue branches in TypedArrayFrom; in practice: call GetIteratorFromValue(Source) first, if it returns a non-nil Iterator take the iterable path, otherwise fall through to the existing typed-array fast path (TGocciaTypedArrayValue) and then the regular array fast path (TGocciaArrayValue) as currently implemented.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@source/units/Goccia.Values.TypedArrayValue.pas`:
- Around line 2104-2116: The TGocciaArrayValue fast path is placed before the
@@iterator check so arrays with a custom Symbol.iterator never invoke the
iterable branch; move the TGocciaArrayValue handling (the block that creates
TGocciaTypedArrayValue from SrcArr.Elements) to after the
GetMethod/GetIteratorFromValue check (i.e. after the code that calls
GetIteratorFromValue and handles iterables) so that GetMethod(firstArgument,
@@iterator) runs for all objects (matching ES2026 §23.2.5.1 step 5a); keep the
existing fast paths for TGocciaTypedArrayValue, TGocciaArrayBufferValue, and
TGocciaSharedArrayBufferValue before the iterator check as they are
spec-allowed.
- Around line 2218-2265: The fast-paths in TypedArrayFrom run before the
spec-required iterator check; move the iterator detection (call to
GetIteratorFromValue(Source) and the handling of a non-nil Iterator) so it
executes before the TGocciaTypedArrayValue and TGocciaArrayValue branches in
TypedArrayFrom; in practice: call GetIteratorFromValue(Source) first, if it
returns a non-nil Iterator take the iterable path, otherwise fall through to the
existing typed-array fast path (TGocciaTypedArrayValue) and then the regular
array fast path (TGocciaArrayValue) as currently implemented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37d6414b-c0e3-4b1e-b10d-465af39741a6
📒 Files selected for processing (1)
source/units/Goccia.Values.TypedArrayValue.pas
In the constructor, move the TGocciaArrayValue fast path after the @@iterator check so arrays with overridden Symbol.iterator use the iterable branch (§23.2.5.1 step 5c). In from(), move the iterator detection before the TypedArray and Array fast paths (§23.2.2.1 step 5). The fast paths remain as fallbacks for sources without @@iterator. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
new TypedArray(object): check@@iterator(iterable path) then fall back to array-like (LengthOfArrayLike+ indexedGetProperty)TypedArray.from(source): iterable path withmapfnsupport, then array-like path viaToObject+LengthOfArrayLikeSetTypedArrayFromArrayLiketoTypedArray.prototype.set(): replace TypeError with array-like path viaToObject+LengthOfArrayLikeTest plan
1instead of0)Closes #551
🤖 Generated with Claude Code