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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Implement unscopables for Array.prototype #1963

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - Implement unscopables for Array.prototype #1963

wants to merge 3 commits into from

Conversation

NorbertGarfield
Copy link
Contributor

This Pull Request fixes/closes #1916.

It implements Array.prototype [ @@unscopables ] as described in ECMAScript

@codecov
Copy link

codecov bot commented Mar 21, 2022 •
edited
Loading

Codecov Report

Merging #1963 (d4e844f) into main (5fa1668) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1963      +/-   ##
==========================================
+ Coverage   45.87%   45.94%   +0.06%     
==========================================
  Files         206      206              
  Lines       17102    17135      +33     
==========================================
+ Hits         7846     7872      +26     
- Misses       9256     9263       +7
Impacted Files Coverage Δ
boa_engine/src/builtins/array/mod.rs 75.78% <100.00%> (+1.02%) ⬆️
boa_engine/src/realm.rs 33.33% <0.00%> (-6.67%) ⬇️
boa_engine/src/builtins/array/array_iterator.rs 83.33% <0.00%> (-3.34%) ⬇️
boa_engine/src/vm/code_block.rs 45.81% <0.00%> (-1.77%) ⬇️
boa_engine/src/symbol.rs 31.81% <0.00%> (-1.52%) ⬇️
boa_engine/src/object/internal_methods/global.rs 31.25% <0.00%> (-0.90%) ⬇️
boa_engine/src/object/operations.rs 58.89% <0.00%> (-0.62%) ⬇️
boa_engine/src/builtins/regexp/mod.rs 67.28% <0.00%> (-0.27%) ⬇️
boa_engine/src/builtins/generator/mod.rs 7.69% <0.00%> (-0.09%) ⬇️
boa_engine/src/bytecompiler.rs 37.14% <0.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fa1668...d4e844f. Read the comment docs.

@jedel1043
Copy link
Member

jedel1043 commented Mar 21, 2022 •
edited
Loading

VM implementation

Test result main count PR count difference
Total 88,428 88,428 0
Passed 43,986 43,988 +2
Ignored 21,495 21,495 0
Failed 22,947 22,945 -2
Panics 0 0 0
Conformance 49.74% 49.74% +0.00%
Fixed tests (2): 
test/built-ins/Array/prototype/Symbol.unscopables/prop-desc.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/Symbol.unscopables/prop-desc.js (previously Failed)

jedel1043
jedel1043 requested changes Mar 21, 2022
View reviewed changes
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution! Just a little mistake on the result object. Everything else looks good!

boa_engine/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
HalidOdat
HalidOdat requested changes Mar 22, 2022
View reviewed changes
Copy link
Member

@HalidOdat HalidOdat left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good! Just some documentation is needed :)

We should document the unscopables_intrinsic spec steps, like we do in other Array methods

@HalidOdat HalidOdat added enhancement New feature or request builtins PRs and Issues related to builtins/intrinsics labels Mar 22, 2022
@HalidOdat HalidOdat added this to the v0.15.0 milestone Mar 22, 2022
@Razican
Copy link
Member

Razican commented Mar 22, 2022

@NorbertGarfield I think with the documentation of the steps they mentioned adding the spec documentation:

1. Let unscopableList be [OrdinaryObjectCreate](https://tc39.es/ecma262/#sec-ordinaryobjectcreate)(null).
2. Perform ! [CreateDataPropertyOrThrow](https://tc39.es/ecma262/#sec-createdatapropertyorthrow)(unscopableList, "at", true).
3. Perform ! [CreateDataPropertyOrThrow](https://tc39.es/ecma262/#sec-createdatapropertyorthrow)(unscopableList, "copyWithin", true).
4. Perform ! [CreateDataPropertyOrThrow](https://tc39.es/ecma262/#sec-createdatapropertyorthrow)(unscopableList, "entries", true).
5. Perform ! [CreateDataPropertyOrThrow](https://tc39.es/ecma262/#sec-createdatapropertyorthrow)(unscopableList, "fill", true).
6. Perform ! [CreateDataPropertyOrThrow](https://tc39.es/ecma262/#sec-createdatapropertyorthrow)(unscopableList, "find", true).
7. Perform ! [CreateDataPropertyOrThrow](https://tc39.es/ecma262/#sec-createdatapropertyorthrow)(unscopableList, "findIndex", true).
8. Perform ! [CreateDataPropertyOrThrow](https://tc39.es/ecma262/#sec-createdatapropertyorthrow)(unscopableList, "flat", true).
9. Perform ! [CreateDataPropertyOrThrow](https://tc39.es/ecma262/#sec-createdatapropertyorthrow)(unscopableList, "flatMap", true).
10. Perform ! [CreateDataPropertyOrThrow](https://tc39.es/ecma262/#sec-createdatapropertyorthrow)(unscopableList, "includes", true).
11. Perform ! [CreateDataPropertyOrThrow](https://tc39.es/ecma262/#sec-createdatapropertyorthrow)(unscopableList, "keys", true).
12. Perform ! [CreateDataPropertyOrThrow](https://tc39.es/ecma262/#sec-createdatapropertyorthrow)(unscopableList, "values", true).
13. Return unscopableList.

So, the idea would be to write the code like this:

// 1. Let unscopableList be [OrdinaryObjectCreate](https://tc39.es/ecma262/#sec-ordinaryobjectcreate)(null).
let result_obj = JsObject::empty();
// 2. Perform ! [CreateDataPropertyOrThrow](https://tc39.es/ecma262/#sec-createdatapropertyorthrow)(unscopableList, "at", true).
result_obj
    .create_data_property_or_throw("at", true, context)
    .expect("CreateDataPropertyOrThrow for 'at' must not fail");
// ... and so on

Also, I would call the variable unscopable_list, as the spec mentions, instead of result_obj.

@NorbertGarfield
Copy link
Contributor Author

@Razican done. I omitted direct links for the sake of consistency (it seems to me that those are only used in More information sections). I hope, that's fine.

Razican
Razican approved these changes Mar 22, 2022
View reviewed changes
Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

This looks perfect for me now :) thank you!!

jedel1043
jedel1043 approved these changes Mar 22, 2022
View reviewed changes
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Thank you!

@HalidOdat
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Mar 25, 2022
@NorbertGarfield
Implement unscopables for Array.prototype (#1963)
da47fb5 
This Pull Request fixes/closes #1916.

It implements `Array.prototype [ @@unscopables ]` as described in [ECMAScript](https://tc39.es/ecma262/#sec-array.prototype-@@unscopables)
@bors
Copy link

bors bot commented Mar 25, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Implement unscopables for Array.prototype [Merged by Bors] - Implement unscopables for Array.prototype Mar 25, 2022
@bors bors bot closed this Mar 25, 2022
Razican pushed a commit that referenced this pull request Jun 8, 2022
@NorbertGarfield @Razican
Implement unscopables for Array.prototype (#1963)
dc54837 
This Pull Request fixes/closes #1916.

It implements `Array.prototype [ @@unscopables ]` as described in [ECMAScript](https://tc39.es/ecma262/#sec-array.prototype-@@unscopables)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jedel1043 jedel1043 jedel1043 approved these changes

@HalidOdat HalidOdat HalidOdat approved these changes

@Razican Razican Razican approved these changes

Assignees
No one assigned
Labels
builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request
Projects
None yet
Milestone
v0.15.0
Development

Successfully merging this pull request may close these issues.

Implement Array.prototype [ @@unscopables ]
4 participants
@NorbertGarfield @jedel1043 @Razican @HalidOdat