feat(ecmascript): %TypedArray%.prototype.slice #736

yossydev · 2025-05-25T09:00:40Z

.../ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs

aapoalas

Some nitpicks and one or two issues, that may actually be non-issues?

But overall, looks really good! Thank you, and great work <3

.../ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs

aapoalas · 2025-06-06T09:09:28Z

.../ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs

+            .unbind()?
+            .bind(gc.nogc());
+        let o = ta_record.object;
+        // 3. Let srcArrayLength be TypedArrayLength(taRecord).


nitpick: This comment should be inside the slice_typed_array method.

aapoalas · 2025-06-06T09:10:43Z

.../ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs

        gc: GcScope<'gc, '_>,
    ) -> JsResult<'gc, Value<'gc>> {
-        Err(agent.todo("TypedArray.prototype.slice", gc.into_nogc()))
+        let start = arguments_list.get(0);


nitpick: Both the get call results and the o = this_value; should have a .bind(gc.nogc()) attached. Here it does not make a difference, but it's better to have them here just so that anyone coming in later and using this method as a template to copy-paste from will have the right "idioms" in place.

aapoalas · 2025-06-06T09:14:55Z

.../ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs

+    let end = end.scope(agent, gc.nogc());
+    let src_array_length = typed_array_length::<T>(agent, &ta_record, gc.nogc()) as i64;
+    // 4. Let relativeStart be ? ToIntegerOrInfinity(start).
+    let relative_start = to_integer_or_infinity(agent, start.get(agent), gc.reborrow()).unbind()?;


issue: No reason to scope start as it is accessed here directly after. You can just remove the scoping line and change this to use start.unbind():

Suggested change

let relative_start = to_integer_or_infinity(agent, start.get(agent), gc.reborrow()).unbind()?;

let relative_start = to_integer_or_infinity(agent, start.unbind(), gc.reborrow()).unbind()?;

aapoalas · 2025-06-06T09:15:04Z

.../ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs

+    let end = end.bind(gc.nogc());
+    let o = ta_record.object;
+    let o = o.scope(agent, gc.nogc());
+    let start = start.scope(agent, gc.nogc());


Suggested change

let start = start.scope(agent, gc.nogc());

aapoalas · 2025-06-06T09:16:46Z

.../ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs

+        relative_start.into_i64().min(src_array_length)
+    };
+    // 8. If end is undefined, let relativeEnd be srcArrayLength; else let relativeEnd be ? ToIntegerOrInfinity(end).
+    let end_index = if end.get(agent).is_undefined() {


nitpick: You could take end out of the scoping here:

Suggested change

let end_index = if end.get(agent).is_undefined() {

// SAFETY: end is not shared.

let end = unsafe { end.take(agent) }.bind(gc.nogc());

let end_index = if end.is_undefined() {

aapoalas · 2025-06-06T09:16:56Z

.../ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs

+        src_array_length
+    } else {
+        let integer_or_infinity =
+            to_integer_or_infinity(agent, end.get(agent), gc.reborrow()).unbind()?;


Suggested change

to_integer_or_infinity(agent, end.get(agent), gc.reborrow()).unbind()?;

to_integer_or_infinity(agent, end.unbind(), gc.reborrow()).unbind()?;

aapoalas · 2025-06-06T09:19:23Z

.../ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs

+    .unbind()?
+    .bind(gc.nogc());
+    // 14. If countBytes > 0, then
+    if count_bytes > 0 {


nitpick: Could maybe return early if count_bytes == 0 and de-indent the if-branch.

aapoalas · 2025-06-06T09:19:44Z

.../ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs

+        if is_typed_array_out_of_bounds::<T>(agent, &ta_record, gc.nogc()) {
+            return Err(agent.throw_exception_with_static_message(
+                ExceptionType::TypeError,
+                "Callback is not callable",


issue: Wrong error message.

aapoalas · 2025-06-06T09:20:46Z

.../ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs

+            ));
+        };
+        // c. Set endIndex to min(endIndex, TypedArrayLength(taRecord)).
+        // let end_index = end_index.min(typed_array_length::<T>(agent, &ta_record, gc.nogc()) as i64);


issue: Commented out code. Are these being done inside the with_typed_array_viewable!(a, ...) branch? If so, the comments should be moved there. Otherwise, the code should presumably be actually performed.

Thank you! It seems that this was left commented out. I fixed it because the end_index is necessary to determine the width of the slice.

yossydev · 2025-06-11T17:13:34Z

tests/expectations.json

-  "built-ins/TypedArray/prototype/slice/speciesctor-get-species-returns-throws.js": "FAIL",
-  "built-ins/TypedArray/prototype/slice/speciesctor-get-species-use-default-ctor.js": "FAIL",
-  "built-ins/TypedArray/prototype/slice/speciesctor-get-species.js": "FAIL",
-  "built-ins/TypedArray/prototype/slice/speciesctor-resize.js": "FAIL",
  "built-ins/TypedArray/prototype/slice/speciesctor-return-same-buffer-with-offset.js": "FAIL",


I don’t really understand why this test crashes on my machine, but is marked as FAIL in the metrics.

Desktop/oss/nova on  feat/typedarray-prototype-slice ❯ cargo build && cargo run --bin test262 eval-test built-ins/TypedArray/prototype/slice/speciesctor-return-same-buffer-with-offset.js Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.14s Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.06s Running `target/debug/test262 eval-test built-ins/TypedArray/prototype/slice/speciesctor-return-same-buffer-with-offset.js` Loose mode run: thread 'main' panicked at nova_vm/src/ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs:3215:9: Must not point to the same buffer note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace Test result: Crash

This is perhaps caused by the --dont-treat-crashes-as-failures flag usage with --bin test262. I've taken fully to using that, and I should actually just remove the flag entirely since we have so few crashes nowadays.

yossydev · 2025-06-11T17:14:18Z

tests/test262

I updated it because the expectations differed from the test content on my machine. 🙇‍♂️

yossydev force-pushed the feat/typedarray-prototype-slice branch from fa5ab1e to 710456b Compare May 25, 2025 09:10

yossydev commented May 25, 2025

View reviewed changes

.../ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs Outdated Show resolved Hide resolved

yossydev marked this pull request as ready for review May 25, 2025 09:44

yossydev force-pushed the feat/typedarray-prototype-slice branch from e16349a to 555ae9c Compare May 31, 2025 22:53

yossydev requested a review from aapoalas May 31, 2025 22:54

yossydev force-pushed the feat/typedarray-prototype-slice branch from 555ae9c to 5a70dcb Compare May 31, 2025 23:13

yossydev commented May 31, 2025

View reviewed changes

.../ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs Show resolved Hide resolved

aapoalas requested changes Jun 6, 2025

View reviewed changes

yossydev added 6 commits June 12, 2025 01:34

feat(ecmascript): %TypedArray%.prototype.slice

5146d1c

chore(ecmascript): update expectations

bd96efe

chore(ecmascript): update metrics

2b886fc

chore: srcType/targetType dirrerent fast

ccd96fe

chore(ecmascript): update

437e207

chore(ecmascript): update

40fd9a7

yossydev force-pushed the feat/typedarray-prototype-slice branch from efc08d0 to 40fd9a7 Compare June 11, 2025 17:05

yossydev commented Jun 11, 2025

View reviewed changes

yossydev requested a review from aapoalas June 11, 2025 17:15

	let relative_start = to_integer_or_infinity(agent, start.get(agent), gc.reborrow()).unbind()?;
	let relative_start = to_integer_or_infinity(agent, start.unbind(), gc.reborrow()).unbind()?;

-    let end_index = if end.get(agent).is_undefined() {
+    // SAFETY: end is not shared.
+    let end = unsafe { end.take(agent) }.bind(gc.nogc());
+    let end_index = if end.is_undefined() {

	to_integer_or_infinity(agent, end.get(agent), gc.reborrow()).unbind()?;
	to_integer_or_infinity(agent, end.unbind(), gc.reborrow()).unbind()?;

feat(ecmascript): %TypedArray%.prototype.slice #736

Are you sure you want to change the base?

feat(ecmascript): %TypedArray%.prototype.slice #736

Uh oh!

Conversation

yossydev commented May 25, 2025

Uh oh!

Uh oh!

Uh oh!

aapoalas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!