Implement Array.of #1127

camc · 2021-02-13T18:21:19Z

This Pull Request checks off part of #36.

It changes the following:

Adds Array.of static method
Adds Array.of tests

codecov · 2021-02-13T18:30:55Z

Codecov Report

Merging #1127 (8c5e372) into master (9e0cab5) will increase coverage by 0.06%.
The diff coverage is 75.00%.

❗ Current head 8c5e372 differs from pull request most recent head 2029298. Consider uploading reports for the commit 2029298 to get more accurate results

@@            Coverage Diff             @@
##           master    #1127      +/-   ##
==========================================
+ Coverage   58.53%   58.59%   +0.06%     
==========================================
  Files         176      176              
  Lines       12547    12559      +12     
==========================================
+ Hits         7344     7359      +15     
+ Misses       5203     5200       -3

Impacted Files	Coverage Δ
boa/src/builtins/array/mod.rs	`76.48% <75.00%> (-0.04%)`	⬇️
boa/src/syntax/ast/position.rs	`72.41% <0.00%> (-6.90%)`	⬇️
boa/src/object/mod.rs	`50.00% <0.00%> (+0.52%)`	⬆️
boa/src/object/internal_methods.rs	`57.89% <0.00%> (+0.80%)`	⬆️
boa/src/object/gcobject.rs	`68.42% <0.00%> (+1.31%)`	⬆️

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 ad7306d...2029298. Read the comment docs.

HalidOdat

Looks good! :)

Check my comments to see how we might improve the implementation.

boa/src/builtins/array/mod.rs

HalidOdat · 2021-02-16T01:14:33Z

boa/src/builtins/array/mod.rs

+        };
+
+        // add properties and set length
+        array.set_field("length", 0, context)?;


Suggested change

array.set_field("length", 0, context)?;

array.set_field("length", args.len(), context)?;

The length shouldn't be zero, should be the count of arguments, (Array.of spec step 8)

The length is set by Array::add_to_array_object, setting it to 0 was unnecessary and has been removed

HalidOdat · 2021-02-16T01:16:25Z

boa/src/builtins/array/mod.rs

+        array.set_field("length", 0, context)?;
+        Array::add_to_array_object(&array, args, context)?;


We should also swap the order since one can throw before the other. This could change the behaviour so it's best to keep it the same as the spec.

RageKnify

Code looks good, just needs a small change to deal with a recent API change.

boa/src/builtins/array/mod.rs

Razican

Hello! This looks perfect, but it requires a re-base in order to fix the build errors and to make it mergeable.

Razican · 2021-05-22T17:48:17Z

It seems the Array.of tests need an update

Razican · 2021-05-22T17:48:44Z

Test262 conformance changes:

Test result	master count	PR count	difference
Total	78,897	78,873	-24
Passed	26,719	26,685	-34
Ignored	15,628	15,604	-24
Failed	36,550	36,584	+34
Panics	14	14	0
Conformance	33.87%	33.83%	-0.03%

Fixed tests:

test/built-ins/Array/of/return-abrupt-from-setting-length.js [strict mode] (previously Failed)
test/built-ins/Array/of/return-abrupt-from-setting-length.js (previously Failed)
test/built-ins/Array/of/return-a-custom-instance.js [strict mode] (previously Failed)
test/built-ins/Array/of/return-a-custom-instance.js (previously Failed)
test/built-ins/Array/of/name.js [strict mode] (previously Failed)
test/built-ins/Array/of/name.js (previously Failed)
test/built-ins/Array/of/length.js [strict mode] (previously Failed)
test/built-ins/Array/of/length.js (previously Failed)
test/built-ins/Array/of/sets-length.js [strict mode] (previously Failed)
test/built-ins/Array/of/sets-length.js (previously Failed)
test/built-ins/Array/of/does-not-use-set-for-indices.js [strict mode] (previously Failed)
test/built-ins/Array/of/does-not-use-set-for-indices.js (previously Failed)
test/built-ins/Array/of/does-not-use-prototype-properties.js [strict mode] (previously Failed)
test/built-ins/Array/of/does-not-use-prototype-properties.js (previously Failed)
test/built-ins/Array/of/construct-this-with-the-number-of-arguments.js [strict mode] (previously Failed)
test/built-ins/Array/of/construct-this-with-the-number-of-arguments.js (previously Failed)
test/built-ins/Array/of/of.js [strict mode] (previously Failed)
test/built-ins/Array/of/of.js (previously Failed)
test/built-ins/Array/of/creates-a-new-array-from-arguments.js [strict mode] (previously Failed)
test/built-ins/Array/of/creates-a-new-array-from-arguments.js (previously Failed)

Broken tests:

test/language/line-terminators/7.3-5.js [strict mode] (previously Passed)
test/language/line-terminators/7.3-5.js (previously Passed)
test/language/line-terminators/7.3-15.js [strict mode] (previously Passed)
test/language/line-terminators/7.3-15.js (previously Passed)
test/language/line-terminators/7.3-6.js [strict mode] (previously Passed)
test/language/line-terminators/7.3-6.js (previously Passed)
test/harness/decimalToHexString.js [strict mode] (previously Passed)
test/harness/decimalToHexString.js (previously Passed)
test/built-ins/Object/getOwnPropertyDescriptor/primitive-string.js [strict mode] (previously Passed)
test/built-ins/Object/getOwnPropertyDescriptor/primitive-string.js (previously Passed)
test/built-ins/Object/getOwnPropertyDescriptor/15.2.3.3-3-14.js [strict mode] (previously Passed)
test/built-ins/Object/getOwnPropertyDescriptor/15.2.3.3-3-14.js (previously Passed)
test/built-ins/Date/prototype/toISOString/15.9.5.43-0-5.js [strict mode] (previously Passed)
test/built-ins/Date/prototype/toISOString/15.9.5.43-0-5.js (previously Passed)
test/built-ins/Array/prototype/filter/15.4.4.20-2-18.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/filter/15.4.4.20-2-18.js (previously Passed)
test/built-ins/Array/prototype/filter/15.4.4.20-1-8.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/filter/15.4.4.20-1-8.js (previously Passed)
test/built-ins/Array/prototype/filter/15.4.4.20-1-7.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/filter/15.4.4.20-1-7.js (previously Passed)
test/built-ins/Array/prototype/indexOf/15.4.4.14-1-8.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/indexOf/15.4.4.14-1-8.js (previously Passed)
test/built-ins/Array/prototype/indexOf/15.4.4.14-2-18.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/indexOf/15.4.4.14-2-18.js (previously Passed)
test/built-ins/Array/prototype/reduce/15.4.4.21-9-c-i-28.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/reduce/15.4.4.21-9-c-i-28.js (previously Passed)
test/built-ins/Array/prototype/reduce/15.4.4.21-2-18.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/reduce/15.4.4.21-2-18.js (previously Passed)
test/built-ins/Array/prototype/reduce/15.4.4.21-1-7.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/reduce/15.4.4.21-1-7.js (previously Passed)
test/built-ins/Array/prototype/reduce/15.4.4.21-1-8.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/reduce/15.4.4.21-1-8.js (previously Passed)
test/built-ins/Array/prototype/reduce/15.4.4.21-8-b-iii-1-28.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/reduce/15.4.4.21-8-b-iii-1-28.js (previously Passed)
test/built-ins/Array/prototype/lastIndexOf/15.4.4.15-1-8.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/lastIndexOf/15.4.4.15-1-8.js (previously Passed)
test/built-ins/Array/prototype/lastIndexOf/15.4.4.15-2-18.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/lastIndexOf/15.4.4.15-2-18.js (previously Passed)
test/built-ins/Array/prototype/every/15.4.4.16-2-18.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/every/15.4.4.16-2-18.js (previously Passed)
test/built-ins/Array/prototype/some/15.4.4.17-2-18.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/some/15.4.4.17-2-18.js (previously Passed)
test/built-ins/Array/prototype/reduceRight/15.4.4.22-2-18.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/reduceRight/15.4.4.22-2-18.js (previously Passed)
test/built-ins/Array/prototype/reduceRight/15.4.4.22-1-7.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/reduceRight/15.4.4.22-1-7.js (previously Passed)
test/built-ins/Array/prototype/reduceRight/15.4.4.22-9-c-i-28.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/reduceRight/15.4.4.22-9-c-i-28.js (previously Passed)
test/built-ins/Array/prototype/reduceRight/15.4.4.22-8-b-iii-1-28.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/reduceRight/15.4.4.22-8-b-iii-1-28.js (previously Passed)
test/built-ins/Array/prototype/reduceRight/15.4.4.22-1-8.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/reduceRight/15.4.4.22-1-8.js (previously Passed)
test/built-ins/Array/of/return-abrupt-from-data-property.js [strict mode] (previously Passed)
test/built-ins/Array/of/return-abrupt-from-data-property.js (previously Passed)

Razican · 2021-05-30T18:56:46Z

It seems there are some new broken tests, but it could be due to a missing re-base. Could we check that?

RageKnify · 2021-05-31T16:48:12Z

Latest numbers, fixed 20 but broke 2:

Test262 conformance changes:

Test result	master count	PR count	difference
Total	78,897	78,897	0
Passed	26,719	26,737	+18
Ignored	15,628	15,628	0
Failed	36,550	36,532	-18
Panics	14	14	0
Conformance	33.87%	33.89%	+0.02%

Fixed tests:

test/built-ins/Array/of/return-abrupt-from-setting-length.js [strict mode] (previously Failed)
test/built-ins/Array/of/return-abrupt-from-setting-length.js (previously Failed)
test/built-ins/Array/of/return-a-custom-instance.js [strict mode] (previously Failed)
test/built-ins/Array/of/return-a-custom-instance.js (previously Failed)
test/built-ins/Array/of/name.js [strict mode] (previously Failed)
test/built-ins/Array/of/name.js (previously Failed)
test/built-ins/Array/of/length.js [strict mode] (previously Failed)
test/built-ins/Array/of/length.js (previously Failed)
test/built-ins/Array/of/sets-length.js [strict mode] (previously Failed)
test/built-ins/Array/of/sets-length.js (previously Failed)
test/built-ins/Array/of/does-not-use-set-for-indices.js [strict mode] (previously Failed)
test/built-ins/Array/of/does-not-use-set-for-indices.js (previously Failed)
test/built-ins/Array/of/does-not-use-prototype-properties.js [strict mode] (previously Failed)
test/built-ins/Array/of/does-not-use-prototype-properties.js (previously Failed)
test/built-ins/Array/of/construct-this-with-the-number-of-arguments.js [strict mode] (previously Failed)
test/built-ins/Array/of/construct-this-with-the-number-of-arguments.js (previously Failed)
test/built-ins/Array/of/of.js [strict mode] (previously Failed)
test/built-ins/Array/of/of.js (previously Failed)
test/built-ins/Array/of/creates-a-new-array-from-arguments.js [strict mode] (previously Failed)
test/built-ins/Array/of/creates-a-new-array-from-arguments.js (previously Failed)

Broken tests:

test/built-ins/Array/of/return-abrupt-from-data-property.js [strict mode] (previously Passed)
test/built-ins/Array/of/return-abrupt-from-data-property.js (previously Passed)

camc · 2021-05-31T19:18:46Z

I think return-abrupt-from-data-property.js is broken due to Object.preventExtensions not being implemented yet

RageKnify · 2021-05-31T21:32:13Z

Halid has been tight on time, as long as another maintainer gives their go ahead I think we can merge.

HalidOdat reviewed Feb 16, 2021

View reviewed changes

HalidOdat mentioned this pull request Feb 16, 2021

Implement Array prototype methods #36

Closed

33 tasks

RageKnify requested changes Mar 10, 2021

View reviewed changes

boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved

RageKnify approved these changes Mar 29, 2021

View reviewed changes

RageKnify added this to the v0.12.0 milestone Mar 29, 2021

RageKnify added the builtins PRs and Issues related to builtins/intrinsics label Mar 29, 2021

Razican approved these changes May 10, 2021

View reviewed changes

Razican requested a review from HalidOdat May 30, 2021 18:31

implement array.of

2029298

RageKnify requested review from 0x7D2B and tofpie May 31, 2021 21:26

Razican requested review from jasonwilliams and Lan2u June 1, 2021 07:03

jasonwilliams approved these changes Jun 7, 2021

View reviewed changes

Razican merged commit 19a6cde into boa-dev:master Jun 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Array.of #1127

Implement Array.of #1127

camc commented Feb 13, 2021

codecov bot commented Feb 13, 2021 •

edited

Loading

HalidOdat left a comment

HalidOdat Feb 16, 2021

camc Feb 17, 2021

HalidOdat Feb 16, 2021

RageKnify left a comment

Razican left a comment

Razican commented May 22, 2021

Razican commented May 22, 2021 •

edited

Loading

Razican commented May 30, 2021

RageKnify commented May 31, 2021

camc commented May 31, 2021

RageKnify commented May 31, 2021

	array.set_field("length", 0, context)?;
	array.set_field("length", args.len(), context)?;

		array.set_field("length", 0, context)?;
		Array::add_to_array_object(&array, args, context)?;

Implement Array.of #1127

Implement Array.of #1127

Conversation

camc commented Feb 13, 2021

codecov bot commented Feb 13, 2021 • edited Loading

Codecov Report

HalidOdat left a comment

Choose a reason for hiding this comment

HalidOdat Feb 16, 2021

Choose a reason for hiding this comment

camc Feb 17, 2021

Choose a reason for hiding this comment

HalidOdat Feb 16, 2021

Choose a reason for hiding this comment

RageKnify left a comment

Choose a reason for hiding this comment

Razican left a comment

Choose a reason for hiding this comment

Razican commented May 22, 2021

Razican commented May 22, 2021 • edited Loading

Test262 conformance changes:

Razican commented May 30, 2021

RageKnify commented May 31, 2021

Test262 conformance changes:

camc commented May 31, 2021

RageKnify commented May 31, 2021

codecov bot commented Feb 13, 2021 •

edited

Loading

Razican commented May 22, 2021 •

edited

Loading