Skip to content
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

Implement ES2023 copy methods on Array and TypedArray #1569

Merged
merged 13 commits into from
Aug 23, 2024

Conversation

andreabergia
Copy link
Contributor

@andreabergia andreabergia commented Aug 20, 2024

This PR adds:

  • Array.prototype.toReversed
  • Array.prototype.toSorted
  • Array.prototype.toSpliced
  • Array.prototype.with
  • TypedArray.prototype.toReversed
  • TypedArray.prototype.toSorted
  • TypedArray.prototype.with

(There is no TypedArray.prototype.toSpliced in the standard).
I tried to follow the spec very closely, even matching (most) variable names.

For arrays, it passes all test262 cases except those marked with Reflect.construct. Note that, to do so, I couldn't rely on some optimization, for example the denseOnly flag.

For typed arrays, in addition to the cases marked with Reflect.construct, there are some other test262 cases that don't pass:

Should fix #1307

I ended up with a single PR that does both Array and TypedArray because I imagined they would share part of the implementation... but in the end, they are completely separate. Let me know if you would prefer two separate PRs.

Please squash this if accepted! 🙂

@andreabergia andreabergia marked this pull request as ready for review August 21, 2024 15:03
@andreabergia andreabergia changed the title WIP: Implement ES2023 methods on Array Implement ES2023 methods on Array Aug 21, 2024
@andreabergia andreabergia changed the title Implement ES2023 methods on Array Implement ES2023 copy methods on Array and TypedArray Aug 21, 2024
@p-bakker
Copy link
Collaborator

Is prototype/xxx/length-property-ignored.js - do not pass because rhino does not allow to configure TypedArray.prototype.length due to the length propertyDescriptor being setup with configurable: false or is something else going on? I assume we need a case for this one as well?

@andreabergia
Copy link
Contributor Author

Is prototype/xxx/length-property-ignored.js - do not pass because rhino does not allow to configure TypedArray.prototype.length due to the length propertyDescriptor being setup with configurable: false or is something else going on? I assume we need a case for this one as well?

Yeah, I meant to do that, but I had a long meeting while I was updating the description and I missed it 😅 Here it is: #1572

@gbrail
Copy link
Collaborator

gbrail commented Aug 22, 2024

This looks pretty complete to me -- anyone else have an opinion? Otherwise I will merge it in the next few days.

@gbrail
Copy link
Collaborator

gbrail commented Aug 22, 2024

This looks good, but I want to keep the code clean and fix one warning:

I added "ErrorProne" a while back and I'm trying to clear its warnings (and may consider making them errors).

Would you mind fixing this one minor one that shows up in the build with these new changes?

/home/runner/work/rhino/rhino/rhino/src/main/java/org/mozilla/javascript/NativeArray.java:2239: warning: [UnnecessaryParentheses] These grouping parentheses are unnecessary; it is unlikely the code will be misinterpreted without them
int from = (int) (len) - k - 1;
^
(see https://errorprone.info/bugpattern/UnnecessaryParentheses)
Did you mean 'int from = (int) len - k - 1;'?

@andreabergia
Copy link
Contributor Author

I added "ErrorProne" a while back and I'm trying to clear its warnings (and may consider making them errors).

Would you mind fixing this one minor one that shows up in the build with these new changes?

Sure, done.

@gbrail gbrail merged commit 86a2d60 into mozilla:master Aug 23, 2024
3 checks passed
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.

Support ES2023 Change Array by copy
3 participants