fix(class): class extends Array sizes the instance and supports fill#5499
Merged
Conversation
📝 WalkthroughWalkthroughAdds support for ChangesArray subclass super() initialization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🤖 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/node_stream_constructors.rs`:
- Around line 466-473: The ToLength implementation in the block starting with
`let nv = JSValue::from_bits(n.to_bits())` is incomplete. Currently it treats
all non-finite values as 0.0, but the ToLength specification requires Infinity
to clamp to 2^53-1 (the maximum safe integer length) instead of 0.0.
Additionally, positive finite values that exceed the maximum safe length should
also be clamped to 2^53-1 rather than used directly. Replace the simple
conditional logic with proper handling that checks if n is Infinity and returns
2^53-1, checks if the floored result exceeds 2^53-1 and clamps it, and only
returns 0.0 for undefined, NaN, or negative values.
🪄 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: 68fb8a43-1a18-4f9e-a342-53a70b496f25
📒 Files selected for processing (6)
crates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/this_super_call.rscrates/perry-codegen/src/expr/write_barrier.rscrates/perry-codegen/src/runtime_decls/stdlib_ffi.rscrates/perry-runtime/src/node_stream_constructors.rscrates/perry/tests/issue_array_subclass_super_init.rs
proggeramlug
force-pushed
the
fix/class-extends-array
branch
2 times, most recently
from
ace1907 to
e3c487d
Compare
`class X extends Array { constructor(n){ super(n); this.fill(0) } }` threw
`TypeError: value is not a function` at `this.fill`. perry models a subclass
instance as a plain object, not a real exotic Array (ArrayHeader has no class_id
slot), so `super(n)` left it length-less with no Array methods.
The super() lowering for an Array parent now calls a runtime
js_array_subclass_init(this, n) that sets a visible `length = ToLength(n)` and
installs the Array surface the instance relies on (currently `fill`), delegating
to the generic array-like impl (js_array_fill_generic, which operates on the
receiver's own length + indexed properties). Indexed get/set already work as
ordinary object properties. Mirrors the EventEmitter subclass-init pattern;
additional Array methods can be added as bundles need them. Adds an e2e test.
Contributor
Author
|
Addressed: |
proggeramlug
force-pushed
the
fix/class-extends-array
branch
from
e3c487d to
1a5f1fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/node_stream_constructors.rs`:
- Line 482: The `fill` method registration on line 482 uses
`cast1(ns_array_fill)` which only accepts a single value parameter and hardcodes
the start and end parameters as zeros when calling `js_array_fill_generic`,
causing the optional range arguments to be ignored. Replace `cast1` with an
appropriate multi-parameter wrapper (such as `cast3`) that properly accepts and
forwards the value, start, and end parameters to `js_array_fill_generic` instead
of hardcoding them, so that `subclass.fill(value, start, end)` respects the
optional range arguments as per Array.prototype.fill semantics.
🪄 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: 96f4a9f5-add1-4d5f-b3ea-2d031da58bcf
📒 Files selected for processing (6)
crates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/this_super_call.rscrates/perry-codegen/src/expr/write_barrier.rscrates/perry-codegen/src/runtime_decls/stdlib_ffi.rscrates/perry-runtime/src/node_stream_constructors.rscrates/perry/tests/issue_array_subclass_super_init.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/perry-codegen/src/runtime_decls/stdlib_ffi.rs
- crates/perry/tests/issue_array_subclass_super_init.rs
- crates/perry-codegen/src/expr/this_super_call.rs
- crates/perry-codegen/src/expr/write_barrier.rs
- crates/perry-codegen/src/expr/mod.rs
| let length_key = crate::string::js_string_from_bytes(b"length".as_ptr(), 6); | ||
| js_object_set_field_by_name(obj, length_key, len); | ||
| crate::closure::js_register_closure_arity(ns_array_fill as *const u8, 1); | ||
| let methods: [(&str, StubFn); 1] = [("fill", super::cast1(ns_array_fill))]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expectation: the generic fill helper accepts value/start/end inputs, and the
# normal Array.fill wrapper, if present, threads optional start/end through it.
rg -nP -C4 '\b(js_array_fill_generic|ns_array_fill|array_fill)\s*\('
# Expectation: installed native method wrappers have a cast/helper that can pass
# optional call arguments while preserving JS function length metadata if needed.
rg -nP -C4 '\bcast[0-9]\(|js_register_closure_arity\(.*fill'Repository: PerryTS/perry
Length of output: 47309
Thread fill's optional range arguments through this wrapper.
ns_array_fill is registered with arity 1 and wrapped using cast1, accepting only a value parameter. It hardcodes has_start=0, start=0.0, has_end=0, end=0.0 when calling js_array_fill_generic, which means subclass.fill(value, start, end) fills the entire receiver rather than the specified slice. To match Array.prototype.fill semantics, accept and forward the optional start and end parameters.
🤖 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/node_stream_constructors.rs` at line 482, The `fill`
method registration on line 482 uses `cast1(ns_array_fill)` which only accepts a
single value parameter and hardcodes the start and end parameters as zeros when
calling `js_array_fill_generic`, causing the optional range arguments to be
ignored. Replace `cast1` with an appropriate multi-parameter wrapper (such as
`cast3`) that properly accepts and forwards the value, start, and end parameters
to `js_array_fill_generic` instead of hardcoding them, so that
`subclass.fill(value, start, end)` respects the optional range arguments as per
Array.prototype.fill semantics.
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.
Problem
class X extends Array { constructor(n){ super(n); this.fill(0) } }threwTypeError: value is not a functionatthis.fill. perry models a subclass instance as a plain object, not a real exotic Array (ArrayHeaderhas only{length, capacity}— noclass_idslot), sosuper(n)left the instance length-less with no Array methods:length=0,isArray=false,fill/push/mapallundefined. (lru-cache'sZeroArrayis the motivating shape — a fixed-size zero-initialised indexed buffer.)Fix
The
super()lowering for anArrayparent (expr/this_super_call.rs) now calls a runtimejs_array_subclass_init(this, n)that:length = ToLength(n)own property, andfill) as a method delegating to the existing generic array-like impljs_array_fill_generic— which operates on the receiver's ownlength+ indexed properties.Indexed get/set already worked as ordinary object properties. This mirrors the EventEmitter subclass-init pattern (
js_event_emitter_subclass_init). Additional Array methods can be added to the install list as needed.Test
crates/perry/tests/issue_array_subclass_super_init.rs—class Zero extends Arraywithsuper(n); this.fill(0): assertslength, zero-initialised slots, indexed write,fillwith a non-zero value, andtypeof fill === 'function'.Summary by CodeRabbit
Array, including correct handling ofsuper(n, ...)to set the instancelength.fillmethod is installed and works correctly aftersuper(n, ...), with subclass field initialization applied at the right time.Array-parentsuper(...)lowering so it no longer falls through to unrelated paths.Arraysubclasses usingsuper(n)followed bythis.fill(0), verifyinglength, indexed behavior, and independent instances.