Skip to content

feat(ecmascript): %TypedArray%.prototype.set #744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

yossydev
Copy link
Contributor

@yossydev yossydev commented Jun 1, 2025

"built-ins/TypedArray/prototype/set/BigInt/boolean-tobigint.js": "FAIL",
"built-ins/TypedArray/prototype/set/BigInt/null-tobigint.js": "FAIL",
"built-ins/TypedArray/prototype/set/BigInt/number-tobigint.js": "FAIL",
"built-ins/TypedArray/prototype/set/BigInt/src-typedarray-big.js": "FAIL",
"built-ins/TypedArray/prototype/set/BigInt/src-typedarray-not-big-throws.js": "FAIL",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are the reasons for the test262 failures.

internal error: entered unreachable code

thread 'main' panicked at nova_vm/src/ecmascript/types/spec/data_block.rs:683:13:
internal error: entered unreachable code
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

 fn from_le_value(agent: &mut Agent, value: Numeric) -> Self {
        let Ok(value) = BigInt::try_from(value) else {
            unreachable!()
        };
        to_big_int64_big_int(agent, value).to_le()
    }

Uncaught exception: Error: SharedArrayBuffer not implemented (Testing with Int16Array.)

Uncaught exception: Error: SharedArrayBuffer not implemented (Testing with Int16Array.)

Test result: Fail

@yossydev yossydev changed the title feat: %TypedArray%.prototype.set feat(ecmascript): %TypedArray%.prototype.set Jun 1, 2025
@yossydev yossydev marked this pull request as ready for review June 1, 2025 09:12
Copy link
Member

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit more work to do around the GC handling here. You could also use the existing optimised TypedArray copy functions here instead of using the generic GetValueFromBuffer and SetValueFromBuffer methods.

/// targetOffset, reading the values from source.
pub(crate) fn set_typed_array_from_typed_array<
'a,
TargetType: Viewable + std::fmt::Debug,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Preferablytake out the std::fmt::Debug bounds once you don't need them for debugging anymore.

));
};
// 17. If target.[[ContentType]] is not source.[[ContentType]], throw a TypeError exception.
let is_type_match = has_matching_content_type::<TargetType>(target.get(agent));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: You should be checking source against TargetType; this is probably the reason for one of the failing tests.

Suggested change
let is_type_match = has_matching_content_type::<TargetType>(target.get(agent));
let is_type_match = has_matching_content_type::<TargetType>(source.get(agent));

// 18. If IsSharedArrayBuffer(srcBuffer) is true, IsSharedArrayBuffer(targetBuffer) is true, and srcBuffer.[[ArrayBufferData]] is targetBuffer.[[ArrayBufferData]], let sameSharedArrayBuffer be true; otherwise, let sameSharedArrayBuffer be false.
// TODO: SharedArrayBuffer that we can even take here.
// 19. If SameValue(srcBuffer, targetBuffer) is true or sameSharedArrayBuffer is true, then
let mut src_byte_index = if same_value(agent, src_buffer, target_buffer) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I think you can just compare src_buffer == target_buffer directly; SameValue(objA, objB) is equal to objA == objB.

src_byte_length,
gc.nogc(),
)
.unbind()?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: You need to rebind here since the resulting cloned src_buffer needs to be tracked by the GC:

Suggested change
.unbind()?;
.unbind()?.bind(gc.nogc());

if core::any::TypeId::of::<SrcType>() == core::any::TypeId::of::<TargetType>() {
// a. NOTE: The transfer must be performed in a manner that preserves the bit-level encoding of the source data.
// Repeat, while targetByteIndex < limit,
while target_byte_index < limit {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Here we want to use the methods you've already added to dealing with copying data from one TypedArray to another, that just perform copy_from_slice from &[SrcType: Viewable] to &mut [TargetType: Viewable].

scoped_o.get(agent),
target_offset,
scoped_source.get(agent),
gc.reborrow()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: You could just pass in gc directly to all the SetTypedArrayFrom... calls, which would mean you don't need to .unbind()? the result but can just use ? directly.

scoped_o.get(agent),
target_offset,
source,
gc.reborrow()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
gc.reborrow()
gc

),
V
)
.unbind()?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.unbind()?;
?;

scoped_o.get(agent),
set_typed_array_from_array_like::<T>(
agent,
scoped_o.get(agent),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Since you already have scoped_o as Scoped, it doesn't make sense to get its result and then immediately scope it again inside the SetTypedArrayFromX calls: Just pass the Scoped<TypedArray> directly instead, and that'll save you one unnecessary scoping.

agent,
scoped_o.get(agent),
target_offset,
scoped_source.get(agent),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Here too, you can just pass in the scoped_source: Scoped<Value> directly; inside the call you'll need to do let src = source.replace_self(agent, src); after you've done let src = to_object(agent, source.get(agent), gc.nogc()); but otherwise it should be all good.

@yossydev yossydev force-pushed the feat/typedarray-prototype-set branch from cae79d9 to 3967d8d Compare June 13, 2025 14:02
@yossydev
Copy link
Contributor Author

I’m still investigating just that one: #744 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants