feat(class): class X extends Array — instance methods + iteration (#6232)#6260
Open
proggeramlug wants to merge 1 commit into
Open
feat(class): class X extends Array — instance methods + iteration (#6232)#6260proggeramlug wants to merge 1 commit into
class X extends Array — instance methods + iteration (#6232)#6260proggeramlug wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughArray subclass support adds transitive compiler detection, runtime subclass classification, dense snapshots for iteration and spread, inherited method dispatch, concat handling, relaxed mutator guards, and coverage for core Array behavior. ChangesArray subclass support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TypeScriptCode
participant try_lower_native_method_str_dispatch
participant js_native_call_method
participant dispatch_arraylike_read_method
participant array_subclass_dense_snapshot
TypeScriptCode->>try_lower_native_method_str_dispatch: invoke inherited Array method
try_lower_native_method_str_dispatch->>js_native_call_method: route Array-subclass method
js_native_call_method->>dispatch_arraylike_read_method: dispatch read operation
dispatch_arraylike_read_method-->>TypeScriptCode: return array-like result
TypeScriptCode->>array_subclass_dense_snapshot: materialize subclass for iteration, spread, or concat
array_subclass_dense_snapshot-->>TypeScriptCode: provide dense indexed elements
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@crates/perry-runtime/src/array/from_concat.rs`:
- Around line 263-275: Preserve sparse holes when creating subclass snapshots:
update array_subclass_dense_snapshot in generic.rs to detect absent indices and
store TAG_HOLE instead of writing al_get’s undefined result. Ensure
append_spread_array can replay HasProperty/Get behavior for those holes so
sparse Array subclass concat results retain their original holes.
In `@crates/perry-runtime/src/array/generic.rs`:
- Around line 1939-1952: Guard the length in array_subclass_dense_snapshot
before allocation: after obtaining len from al_length, invoke the existing
array_length_range_error() when len exceeds the supported u32 range, matching
the validation used by js_arraylike_map. Only cast to u32, allocate, and iterate
after this check so the loop cannot write beyond the allocated buffer.
In `@crates/perry-runtime/src/array/iterator.rs`:
- Around line 738-746: Update the array-subclass fast path in the iterator
resolution logic to check for an own or inherited @@iterator override before
calling array_subclass_dense_snapshot. Only use the dense snapshot when the
subclass retains the default array iterator; otherwise fall through to generic
iterator lookup so custom iterators are respected.
In `@crates/perry-runtime/src/object/native_call_method.rs`:
- Around line 889-920: Array-subclass read methods are dispatched before
user-defined overrides, causing subclass methods or own properties to be
skipped. Update the dispatch block in the native method lookup to first check
the relevant own slot/vtable override, or move dispatch_arraylike_read_method
behind generic dispatch, while preserving fast-path handling only when no
override exists.
In `@crates/perry-runtime/src/symbol/iterator.rs`:
- Around line 171-183: Ensure the Array subclass fast path in the iterator
dispatch checks for an own or inherited custom @@iterator before snapshotting.
Update the branch using is_array_subclass_instance and
array_subclass_dense_snapshot to detect the default Array.prototype iterator,
and fall through to the generic symbol lookup when the iterator is overridden or
patched, matching the Map/Set subclass handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a4d7289-bcc7-4812-9348-e0fc073c5bc5
📒 Files selected for processing (9)
crates/perry-codegen/src/lower_call/console_promise.rscrates/perry-runtime/src/array/from_concat.rscrates/perry-runtime/src/array/generic.rscrates/perry-runtime/src/array/is_array.rscrates/perry-runtime/src/array/iterator.rscrates/perry-runtime/src/array/mod.rscrates/perry-runtime/src/object/native_call_method.rscrates/perry-runtime/src/symbol/iterator.rstest-files/test_gap_6232_class_extends_array.ts
Comment on lines
+263
to
+275
| // `class X extends Array` — the instance is object-backed, so the dense | ||
| // `ArrayHeader` spread below would misread it. `js_array_is_array` reports | ||
| // true for it, so intercept before that branch and spread a dense snapshot | ||
| // of its elements (honoring an explicit `@@isConcatSpreadable === false`). | ||
| if crate::array::is_array_subclass_instance(value) { | ||
| if spreadable == Some(false) { | ||
| return js_array_push_f64(result, value); | ||
| } | ||
| let snap = crate::array::array_subclass_dense_snapshot(value); | ||
| let snap_ptr = (snap.to_bits() & 0x0000_FFFF_FFFF_FFFF) as *const ArrayHeader; | ||
| return append_spread_array(result, snap_ptr); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what al_get returns for absent indices on object-backed array subclass instances.
rg -n -A 20 'pub.*fn al_get\b' crates/perry-runtime/src/array/generic.rsRepository: PerryTS/perry
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant symbols and files.
rg -n "array_subclass_dense_snapshot|append_spread_array|al_get\\b|TAG_HOLE|is_array_subclass_instance|subclass_backing_for_default_iteration" crates/perry-runtime/srcRepository: PerryTS/perry
Length of output: 14478
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the array module files and sizes before reading targeted sections.
git ls-files 'crates/perry-runtime/src/array/**' | sortRepository: PerryTS/perry
Length of output: 1114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the source files that define the relevant helpers.
fd -a -t f 'generic.rs|from_concat.rs|*.rs' crates/perry-runtime/src/arrayRepository: PerryTS/perry
Length of output: 487
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '412,470p;1939,1968p' crates/perry-runtime/src/array/generic.rs
printf '\n----\n'
sed -n '724,770p' crates/perry-runtime/src/array/from_concat.rsRepository: PerryTS/perry
Length of output: 5844
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the array-subclass snapshot path and the concat spreader.
sed -n '412,470p;1939,1968p' crates/perry-runtime/src/array/generic.rs
printf '\n----\n'
sed -n '724,770p' crates/perry-runtime/src/array/from_concat.rsRepository: PerryTS/perry
Length of output: 5844
Preserve holes in array-subclass concat snapshots. crates/perry-runtime/src/array/generic.rs::array_subclass_dense_snapshot writes al_get results directly, so absent indices become undefined instead of TAG_HOLE. append_spread_array only replays HasProperty/Get for TAG_HOLE, so sparse class X extends Array instances will spread with explicit undefined elements.
🤖 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 `@crates/perry-runtime/src/array/from_concat.rs` around lines 263 - 275,
Preserve sparse holes when creating subclass snapshots: update
array_subclass_dense_snapshot in generic.rs to detect absent indices and store
TAG_HOLE instead of writing al_get’s undefined result. Ensure
append_spread_array can replay HasProperty/Get behavior for those holes so
sparse Array subclass concat results retain their original holes.
Comment on lines
+1939
to
+1952
| pub fn array_subclass_dense_snapshot(recv: f64) -> f64 { | ||
| let len = al_length(recv).max(0); | ||
| let result = js_array_alloc_with_length(len as u32); | ||
| let elems = unsafe { (result as *mut u8).add(std::mem::size_of::<ArrayHeader>()) as *mut f64 }; | ||
| for k in 0..len { | ||
| let v = al_get(recv, k); | ||
| unsafe { | ||
| // GC_STORE_AUDIT(BARRIERED): note_array_slot re-stores with the barrier. | ||
| ptr::write(elems.add(k as usize), v); | ||
| note_array_slot(result, k as usize, v.to_bits()); | ||
| } | ||
| } | ||
| nanbox_arr(result) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm js_arraylike_map's guard pattern and array_length_range_error signature.
rg -nP -C3 'array_length_range_error' crates/perry-runtime/src/array/Repository: PerryTS/perry
Length of output: 2682
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the snapshot implementation and the length helper it relies on.
sed -n '1928,1960p' crates/perry-runtime/src/array/generic.rs
printf '\n---- al_length / related helpers ----\n'
rg -n -C3 '\bal_length\b|ToLength|length property|array_length_range_error' crates/perry-runtime/src/array/generic.rs crates/perry-runtime/src/array/alloc.rsRepository: PerryTS/perry
Length of output: 24117
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect al_length and the array allocator to check for any internal range checks or truncation behavior.
sed -n '260,330p' crates/perry-runtime/src/array/generic.rs
printf '\n---- allocator ----\n'
sed -n '1,120p' crates/perry-runtime/src/array/alloc.rs
printf '\n---- snapshot neighbors for comparison ----\n'
sed -n '640,680p' crates/perry-runtime/src/array/generic.rs
sed -n '1038,1052p' crates/perry-runtime/src/array/generic.rsRepository: PerryTS/perry
Length of output: 10942
Guard len before allocating the snapshot array
al_length can exceed u32::MAX; casting it to u32 truncates the allocation while the loop still writes len slots, which can overrun the buffer. Mirror the array_length_range_error() check used in js_arraylike_map here.
🤖 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 `@crates/perry-runtime/src/array/generic.rs` around lines 1939 - 1952, Guard
the length in array_subclass_dense_snapshot before allocation: after obtaining
len from al_length, invoke the existing array_length_range_error() when len
exceeds the supported u32 range, matching the validation used by
js_arraylike_map. Only cast to u32, allocate, and iterate after this check so
the loop cannot write beyond the allocated buffer.
proggeramlug
force-pushed
the
feat/class-extends-array
branch
from
925ac01 to
268f4ea
Compare
…erryTS#6232) An `Array` subclass instance is a plain `ObjectHeader` (perry has no exotic array-object representation), so the inherited `Array.prototype` methods, index access, `Array.isArray`, and iteration were all unsupported: `x.push(1)` threw "value is not a function". This wires the object-backed instance into the runtime's existing spec-generic array-like engine so real code using an Array subclass as a custom collection works. Codegen (console_promise.rs): - Array-subclass carve-out from the `skip_native` gate, mirroring the existing Map/Set-subclass carve-out: an inherited Array method (`push`/`map`/`join`/…) on a `class X extends Array` receiver is NOT a class method, so route it to `js_native_call_method` instead of the closure-call fallthrough (which read the method as a non-callable property and threw). A user-defined override of the same name still resolves earlier via the static class tower. Runtime: - array/generic.rs: `is_array_subclass_class_id` / `is_array_subclass_instance` predicates; relaxed the "plain objects only" guard in the array-like mutator engine and `plain_object_value` to admit Array-subclass instances; `array_subclass_dense_snapshot` materializer. - object/native_call_method.rs: read-method arm (`map`/`filter`/`join`/`at`/…) for Array-subclass receivers (the mutator arm was already covered by the relaxed guard; the read engines had no arm). - array/is_array.rs: `Array.isArray` returns true for an Array-subclass instance (ECMA-262 IsArray). - symbol/iterator.rs + array/from_concat.rs + array/iterator.rs: iterate/spread a dense snapshot of the object-backed instance (for-of, `[...x]`, `fn(...x)`, `Array.from`, `[].concat(x)`), since the array iterator / concat spread read a real `ArrayHeader` and would otherwise misread the plain object. Verified byte-for-byte against Node (test-files/test_gap_6232_class_extends_array.ts): push/pop/length, index access, own methods, map/filter/forEach/reduce/join/at/ indexOf/includes/slice, Array.isArray, for-of, spread, Array.from, concat, instanceof. Additive: plain arrays, plain-object array-like borrows, Map/Set subclasses, and ordinary same-named-method classes are unchanged. Known follow-ups (distinct mechanisms, out of scope here): array-format `toString`/`String()`/`JSON.stringify` (still object-formatted, not garbage); inherited statics (`Sub.from`/`.of`); explicit `.values()`/`.keys()`/`.entries()`; and generic-class `instanceof` / fresh-empty `.length` (a pre-existing perry generics limitation — `class Box<T>{}; new Box() instanceof Box` is also false).
proggeramlug
force-pushed
the
feat/class-extends-array
branch
from
268f4ea to
770607a
Compare
Contributor
Author
|
Thanks @coderabbitai — addressed the review in the latest push (770607a):
Verified byte-for-byte against Node — the fixture now also covers a |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 6260 --repo PerryTS/perry --json commits -q '.commits[-3:]' || true
gh api repos/PerryTS/perry/commits/770607a --jq '.files[].filename' || trueLength of output: 3794 🏁 Script executed: #!/bin/bash
set -euo pipefail
git fetch origin pull/6260/head:pr6260 2>&1 | tail -5 || true
git checkout pr6260 2>&1 | tail -5 || true
echo "--- subclass.rs length guard ---"
rg -n "u32::MAX|array_length_range_error" crates/perry-runtime/src/array/subclass.rs
echo "--- native_call_method.rs own method gate ---"
rg -n "object_owns_user_method|is_array_subclass" crates/perry-runtime/src/object/native_call_method.rs
echo "--- iterator override in symbol/iterator.rs ---"
rg -n "array_subclass_has_iterator_override|is_array" crates/perry-runtime/src/symbol/iterator.rs
echo "--- iterator override in array/iterator.rs ---"
rg -n "array_subclass_has_iterator_override" crates/perry-runtime/src/array/iterator.rs
echo "--- subclass.rs iterator override fn ---"
rg -n "fn array_subclass_has_iterator_override" -A 30 crates/perry-runtime/src/array/subclass.rsLength of output: 2806 All five fixes check out in commit 770607a: the Thanks for the thorough turnaround and the byte-for-byte Node parity verification, including the new ヽ(・∀・)ノ |
Contributor
Author
|
CI status — 13/14 required checks pass (lint, cargo-test, compiler-output-regression, api-docs-drift, CodeRabbit, and conformance-smoke shards 2–8). Only shard 1 is red, on three tests entirely unrelated to
This diff only touches the Array subclass path ( Happy to rebase again if a later main pins the Node version, but this branch is otherwise green. |
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.
Closes #6232 (the instance-side of it).
Problem
An
Arraysubclass instance is a plainObjectHeader— perry has no exotic array-object representation — so every inheritedArray.prototypemethod, index-based iteration,Array.isArray, for-of, and spread was unsupported.x.push(1)threwTypeError: value is not a function. This madeclass X extends Array(a very common way to build a custom collection with extra methods) effectively unusable, and was the root of #6232'snew Blob(arraySubclassInstance)misbehavior.Approach
Wire the object-backed instance into the runtime's existing spec-generic array-like engine, rather than introducing a new exotic-array representation. The change is additive: it only newly-admits Array subclasses; plain arrays, plain-object array-like borrows, Map/Set subclasses, and ordinary same-named-method classes are untouched.
Codegen (
lower_call/console_promise.rs)skip_nativegate, mirroring the existing Map/Set-subclass carve-out. An inherited Array method on aclass X extends Arrayreceiver is not a class method, so it routes tojs_native_call_methodinstead of the closure-call fallthrough (which read the method as a non-callable property and threw). A user-defined override of the same name still resolves earlier via the static class tower — verified.Runtime
array/generic.rs—is_array_subclass_class_id/is_array_subclass_instancepredicates; relaxed the "plain objects only" guard in the array-like mutator engine +plain_object_value;array_subclass_dense_snapshotmaterializer.object/native_call_method.rs— read-method arm (map/filter/join/at/indexOf/…) for Array-subclass receivers (the mutator family was already covered by the relaxed guard; the read engines had no arm outside the Proxy block).array/is_array.rs—Array.isArrayreturns true for an Array-subclass instance (ECMA-262 IsArray).symbol/iterator.rs,array/from_concat.rs,array/iterator.rs— for-of /[...x]/fn(...x)/Array.from/[].concat(x)iterate a dense snapshot of the object-backed instance (the array iterator + concat spread read a realArrayHeaderand would otherwise misread the plain object —Array.isArraynow reporting true routes them there).Testing
test-files/test_gap_6232_class_extends_array.tspasses the parity harness byte-for-byte against Node (./run_parity_tests.sh --filter test_gap_6232→ Parity Pass: 1). Covers: push/pop/length, index access, own methods, map/filter/forEach/reduce/join/at/indexOf/includes/slice,Array.isArray, for-of, spread,Array.from,concat,instanceof. Manual regression pass confirmed plain arrays, plain-object borrows, Map/Set subclasses, and user same-named-method overrides are unchanged.Known follow-ups (distinct mechanisms, intentionally out of scope)
toString/String()/JSON.stringifyon a subclass still return object formatting ([object Object]/{"0":..}) — wrong but safe (no garbage/crash).Sub.from/Sub.of/Sub.isArray) — a separate static-dispatch path; this is what Bun'sblob-array-fast-pathDerivedArraytest additionally needs..values()/.keys()/.entries()method calls.instanceofand fresh-empty.lengthon a generic subclass — a pre-existing perry generics limitation, not specific to this feature (class Box<T>{}; new Box() instanceof Boxis also false; the instance methods + iteration above all work on generic subclasses regardless).Summary by CodeRabbit
class X extends Array, including more accurateArray.isArray/instanceofbehavior.Arraymethods (e.g.,map,filter,reduce,join,slice,concat,at,includes) so calls resolve correctly.Array.from, andconcat, including proper handling ofSymbol.iteratoroverrides andSymbol.isConcatSpreadable.