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

Support ES2019 Array.prototype.flat #1313

Merged
merged 7 commits into from
May 31, 2023
Merged

Conversation

midgleyc
Copy link
Contributor

Closes #948.

I'm not sure whether the implementation of the various ids are correct: I just copied the surrounding code.

I took the harmony test from https://github.com/v8/v8/blob/main/test/mjsunit/harmony/array-flat.js, added load("testsrc/assert.js"); to the start and "success"; to the end, and then made adjustments as Rhino doesn't supported redeclared consts in blocks or the Proxy object.

The language version in the test I have set to ES6, but as flat is part of ES2019, perhaps there should be a new version? Perhaps not, and I should just remove the comment?

By spec, we should throw a TypeError if the index j passed to defineElem is greater than 253 - 1. I have reused the message msg.arraylength.too.big here, but let me know if you'd like me to do something different.

Copy link
Contributor

@tuchida tuchida left a comment

Choose a reason for hiding this comment

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

Thanks for your pull request.

Rhino uses Test262 to test ECMAScript specifications. Could you test the specifications by following these steps?

  1. Install Test262
  2. Delete the following line
  3. Updating the test262.properties file

@@ -151,6 +151,7 @@ protected void fillConstructorProperties(IdFunctionObject ctor) {
addIdFunctionProperty(ctor, ARRAY_TAG, ConstructorId_findIndex, "findIndex", 1);
addIdFunctionProperty(ctor, ARRAY_TAG, ConstructorId_reduce, "reduce", 1);
addIdFunctionProperty(ctor, ARRAY_TAG, ConstructorId_reduceRight, "reduceRight", 1);
addIdFunctionProperty(ctor, ARRAY_TAG, ConstructorId_flat, "flat", 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This code adds the method to the constructor(Array.flat), not the prototype(Array.prototype.flat).
It is an old specification called Array generic methods.
https://lia.disi.unibo.it/materiale/JS/developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array.html#Array_generic_methods

I do not think this is necessary as it is not in latest ECMA262 specification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I just added these because I saw other functions doing this -- e.g. Array.reduceRight doesn't exist either, just Array.prototype.reduceRight.

I've removed ConstructorId_flat entirely.

@midgleyc
Copy link
Contributor Author

midgleyc commented Mar 18, 2023

Thanks, I've updated.

Some of the failing spec tests seem to be failing on something unrelated to flat itself -- I see the error

missing ) after formal parameters

so either I've failed to wire something up correctly, or there's something wrong with a different bit of Rhino.

@tuchida
Copy link
Contributor

tuchida commented Mar 18, 2023

missing ) after formal parameters

ref. #652, #817
That is the problem that test262/harness/compareArray.js cannot be loaded because Rest Parameters are not implemented.
I am sorry, but some of the Test262 code does not work because of a specification that is not implemented in Rhino.

@rbri
Copy link
Collaborator

rbri commented May 31, 2023

Hi @midgleyc,
sorry for the silence here. This looks good to me and i like to merge.

Can you please do

  • an rebase, and a regeneration of the test262.properties file (i have done a fix for the compareArray.js). This will show that even more tests are passing
  • move the files ArrayFlatTest.java and array-flat.js into the testsrc/org/mozilla/javascript/tests/es6 folder (or if you like create a new es2019 folder

Thanks a lot for providing this

@rbri rbri merged commit ab3bd1f into mozilla:master May 31, 2023
4 checks passed
@midgleyc
Copy link
Contributor Author

Sure, no problem.

As you have merged this already (thanks for this!) I will make those changes in another PR.

@rbri
Copy link
Collaborator

rbri commented May 31, 2023

sorry, doing it right no. No need for changes.

@midgleyc midgleyc deleted the array-flat branch May 31, 2023 11:09
@rbri
Copy link
Collaborator

rbri commented May 31, 2023

Thanks for the PR.

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 ES2019 Array.prototype.flat
3 participants