Make `Array.prototype.concat` spec compliant #1353

neeldug · 2021-06-21T00:18:04Z

This Pull Request fixes/closes #1306.

It changes the following:

adds limit checks to array concat
still fails test/built-ins/Array/prototype/concat/arg-length-exceeding-integer-limit.js due to Proxy not being implemented
no longer runs forever

neeldug · 2021-06-21T00:31:31Z

Only thing would be I think the way I define spreadable isn’t super spec compliant, and perhaps we should create a utility function for isConcatSpreadable?

Razican · 2021-06-21T07:51:28Z

Only thing would be I think the way I define spreadable isn’t super spec compliant, and perhaps we should create a utility function for isConcatSpreadable?

I think it makes sense, if it would make it more spec compliant.

Razican · 2021-06-21T07:51:55Z

Test262 conformance changes:

Test result	master count	PR count	difference
Total	78,897	78,897	0
Passed	26,958	26,958	0
Ignored	15,618	15,616	-2
Failed	36,321	36,323	+2
Panics	0	0	0
Conformance	34.17%	34.17%	0.00%

Broken tests:

test/built-ins/Array/prototype/concat/arg-length-exceeding-integer-limit.js [strict mode] (previously Ignored)
test/built-ins/Array/prototype/concat/arg-length-exceeding-integer-limit.js (previously Ignored)

neeldug · 2021-06-21T18:33:02Z

I think it makes sense, if it would make it more spec compliant.

@Razican I've been working on creating the utility function is_concat_spreadable, but haven't quite worked out how to access the attribute here from Value using the WellKnownSymbol, would it be correct in matching over match this.get_property(spreadable), where spreadable is the WellKnownSymbol::is_concat_spreadable()

neeldug · 2021-06-26T19:39:00Z

@Razican when you get the chance, could you check this PR, the test is failing due to proxies not being implemented, but the function is working as expected for the first part of the test-case, we could wait for #1300 to be resolved before merging, but as it's a test, I'm not sure if that's necessary.

Razican

Thanks! Let's merge it and wait for Buffer objects in order to fix the test.

boa/src/builtins/array/mod.rs

- adds limit checks to array concat - still fails test/built-ins/Array/prototype/concat/arg-length-exceeding-integer-limit.js - no longer runs forever Closes boa-dev#1306 add `is_concat_spreadable` utility add array check make suggested changes perf add docs & propagate error add `is_concat_spreadable` utility add array check add `is_concat_spreadable` utility add array check make suggested changes fix(boa): fixes concat limit - adds limit checks to array concat - still fails test/built-ins/Array/prototype/concat/arg-length-exceeding-integer-limit.js - no longer runs forever Closes boa-dev#1306 add `is_concat_spreadable` utility add array check make suggested changes perf add docs & propagate error add docs & propagate error

HalidOdat

I think it's best to make Array.prototype.concat spec compliant like some that were done in #1422, we have all the necessary spec function (like length_of_array_like) so we don't use functions like .get_field, which we should deprecate/remove in the near future #577 .

neeldug · 2021-07-27T21:13:40Z

Test262 conformance changes:

Test result	master count	PR count	difference
Total	78,897	78,897	0
Passed	28,432	28,480	+48
Ignored	15,614	15,612	-2
Failed	34,851	34,805	-46
Panics	2	2	0
Conformance	36.04%	36.10%	+0.06%

Fixed tests:

test/built-ins/Array/prototype/concat/S15.4.4.4_A1_T2.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A1_T2.js (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-number-wrapper.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-number-wrapper.js (previously Failed)
test/built-ins/Array/prototype/concat/create-species-with-non-writable-property.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-species-with-non-writable-property.js (previously Failed)
test/built-ins/Array/prototype/concat/create-species-non-ctor.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-species-non-ctor.js (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A1_T3.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A1_T3.js (previously Failed)
test/built-ins/Array/prototype/concat/create-non-array.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-non-array.js (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-getter-throws.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-getter-throws.js (previously Failed)
test/built-ins/Array/prototype/concat/create-species-with-non-configurable-property-spreadable.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-species-with-non-configurable-property-spreadable.js (previously Failed)
test/built-ins/Array/prototype/concat/create-species-with-non-configurable-property.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-species-with-non-configurable-property.js (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_array-like.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_array-like.js (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A3_T3.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A3_T3.js (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A3_T1.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A3_T1.js (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A2_T2.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A2_T2.js (previously Failed)
test/built-ins/Array/prototype/concat/is-concat-spreadable-get-err.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/is-concat-spreadable-get-err.js (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-boolean-wrapper.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-boolean-wrapper.js (previously Failed)
test/built-ins/Array/prototype/concat/create-proto-from-ctor-realm-non-array.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-proto-from-ctor-realm-non-array.js (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A2_T1.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A2_T1.js (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-reg-exp.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-reg-exp.js (previously Failed)
test/built-ins/Array/prototype/concat/create-ctor-non-object.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-ctor-non-object.js (previously Failed)
test/built-ins/Array/prototype/concat/is-concat-spreadable-val-falsey.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/is-concat-spreadable-val-falsey.js (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-function.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-function.js (previously Failed)
test/built-ins/Array/prototype/concat/create-species-poisoned.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-species-poisoned.js (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A3_T2.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A3_T2.js (previously Failed)
test/built-ins/Array/prototype/concat/create-ctor-poisoned.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-ctor-poisoned.js (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_array-like-string-length.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_array-like-string-length.js (previously Failed)

Broken tests:

test/built-ins/Array/prototype/concat/create-proto-from-ctor-realm-array.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/concat/create-proto-from-ctor-realm-array.js (previously Passed)
test/built-ins/Array/prototype/concat/arg-length-exceeding-integer-limit.js [strict mode] (previously Ignored)
test/built-ins/Array/prototype/concat/arg-length-exceeding-integer-limit.js (previously Ignored)

boa/src/builtins/array/mod.rs

HalidOdat · 2021-07-28T00:31:47Z

Test result	master count	PR count	difference
Total	78,897	78,897	0
Passed	28,432	28,484	+52
Ignored	15,614	15,612	-2
Failed	34,851	34,801	-50
Panics	2	2	0
Conformance	36.04%	36.10%	+0.07%

Fixed tests:

test/built-ins/Array/prototype/concat/S15.4.4.4_A1_T2.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A1_T2.js (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-number-wrapper.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-number-wrapper.js (previously Failed)
test/built-ins/Array/prototype/concat/create-species-with-non-writable-property.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-species-with-non-writable-property.js (previously Failed)
test/built-ins/Array/prototype/concat/create-species-non-ctor.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-species-non-ctor.js (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A1_T3.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A1_T3.js (previously Failed)
test/built-ins/Array/prototype/concat/create-non-array.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-non-array.js (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-getter-throws.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-getter-throws.js (previously Failed)
test/built-ins/Array/prototype/concat/create-species-with-non-configurable-property-spreadable.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-species-with-non-configurable-property-spreadable.js (previously Failed)
test/built-ins/Array/prototype/concat/create-species-with-non-configurable-property.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-species-with-non-configurable-property.js (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_array-like.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_array-like.js (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A3_T3.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A3_T3.js (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A3_T1.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A3_T1.js (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A2_T2.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A2_T2.js (previously Failed)
test/built-ins/Array/prototype/concat/is-concat-spreadable-get-err.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/is-concat-spreadable-get-err.js (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-boolean-wrapper.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-boolean-wrapper.js (previously Failed)
test/built-ins/Array/prototype/concat/call-with-boolean.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/call-with-boolean.js (previously Failed)
test/built-ins/Array/prototype/concat/create-proto-from-ctor-realm-non-array.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-proto-from-ctor-realm-non-array.js (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A2_T1.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A2_T1.js (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-reg-exp.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-reg-exp.js (previously Failed)
test/built-ins/Array/prototype/concat/create-ctor-non-object.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-ctor-non-object.js (previously Failed)
test/built-ins/Array/prototype/concat/is-concat-spreadable-val-falsey.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/is-concat-spreadable-val-falsey.js (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-function.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_spreadable-function.js (previously Failed)
test/built-ins/Array/prototype/concat/create-species-poisoned.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-species-poisoned.js (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A3_T2.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/S15.4.4.4_A3_T2.js (previously Failed)
test/built-ins/Array/prototype/concat/create-ctor-poisoned.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/create-ctor-poisoned.js (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_array-like-string-length.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/Array.prototype.concat_array-like-string-length.js (previously Failed)
test/built-ins/Array/prototype/concat/15.4.4.4-5-c-i-1.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/concat/15.4.4.4-5-c-i-1.js (previously Failed)

Broken tests:

test/built-ins/Array/prototype/concat/create-proto-from-ctor-realm-array.js [strict mode] (previously Passed)
test/built-ins/Array/prototype/concat/create-proto-from-ctor-realm-array.js (previously Passed)
test/built-ins/Array/prototype/concat/arg-length-exceeding-integer-limit.js [strict mode] (previously Ignored)
test/built-ins/Array/prototype/concat/arg-length-exceeding-integer-limit.js (previously Ignored)

HalidOdat

I think we should remove the array_cancat benchmarks since, we already have benchmarks about array access.

Besides that it looks good to me :)

Razican approved these changes Jun 21, 2021

View reviewed changes

Razican requested review from RageKnify, jasonwilliams and HalidOdat June 21, 2021 09:41

neeldug force-pushed the fix-concat branch 2 times, most recently from 285e4c4 to 11a346c Compare June 21, 2021 19:17

neeldug requested a review from Razican June 22, 2021 10:48

Razican approved these changes Jun 27, 2021

View reviewed changes

HalidOdat reviewed Jul 21, 2021

View reviewed changes

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

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

HalidOdat added the builtins PRs and Issues related to builtins/intrinsics label Jul 21, 2021

HalidOdat added this to the v0.13.0 milestone Jul 21, 2021

neeldug force-pushed the fix-concat branch from c254485 to 59d567b Compare July 21, 2021 19:27

HalidOdat reviewed Jul 22, 2021

View reviewed changes

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

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

HalidOdat added the bug Something isn't working label Jul 22, 2021

neeldug force-pushed the fix-concat branch from 14cbfe5 to 9b7314f Compare July 22, 2021 21:02

fmt

07e7c40

neeldug force-pushed the fix-concat branch from 2c40305 to 07e7c40 Compare July 22, 2021 21:06

neeldug requested a review from HalidOdat July 22, 2021 21:24

Merge branch 'master' into fix-concat

e32f838

HalidOdat reviewed Jul 27, 2021

View reviewed changes

neeldug added 2 commits July 27, 2021 21:04

follow spec

8ad9499

fmt

c290767

neeldug requested a review from HalidOdat July 27, 2021 21:47

neeldug requested a review from Razican July 27, 2021 21:47

neeldug commented Jul 27, 2021

View reviewed changes

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

adds benchmark for concat

0d59dad

HalidOdat reviewed Jul 27, 2021

View reviewed changes

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

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

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

neeldug added 2 commits July 28, 2021 00:42

fixes

c9f00db

fmt

819b15a

HalidOdat reviewed Jul 27, 2021

View reviewed changes

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

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

switch to existing method

c1c171b

neeldug force-pushed the fix-concat branch from 8d527c4 to c1c171b Compare July 27, 2021 23:56

use chain

3836c86

HalidOdat reviewed Jul 28, 2021

View reviewed changes

remove benchmark

1971b2e

neeldug requested a review from HalidOdat July 29, 2021 22:50

HalidOdat approved these changes Jul 30, 2021

View reviewed changes

HalidOdat changed the title ~~fix(boa): fixes concat limit~~ Make Array.prototype.concat spec compliant Jul 30, 2021

HalidOdat merged commit 9e14cb8 into boa-dev:master Jul 30, 2021

neeldug deleted the fix-concat branch July 30, 2021 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `Array.prototype.concat` spec compliant #1353

Make `Array.prototype.concat` spec compliant #1353

neeldug commented Jun 21, 2021

neeldug commented Jun 21, 2021

Razican commented Jun 21, 2021

Razican commented Jun 21, 2021 •

edited

neeldug commented Jun 21, 2021 •

edited

neeldug commented Jun 26, 2021

Razican left a comment

HalidOdat left a comment

neeldug commented Jul 27, 2021 •

edited

HalidOdat commented Jul 28, 2021

HalidOdat left a comment •

edited

Make Array.prototype.concat spec compliant #1353

Make Array.prototype.concat spec compliant #1353

Conversation

neeldug commented Jun 21, 2021

neeldug commented Jun 21, 2021

Razican commented Jun 21, 2021

Razican commented Jun 21, 2021 • edited

Test262 conformance changes:

neeldug commented Jun 21, 2021 • edited

neeldug commented Jun 26, 2021

Razican left a comment

Choose a reason for hiding this comment

HalidOdat left a comment

Choose a reason for hiding this comment

neeldug commented Jul 27, 2021 • edited

Test262 conformance changes:

HalidOdat commented Jul 28, 2021

HalidOdat left a comment • edited

Choose a reason for hiding this comment

Make `Array.prototype.concat` spec compliant #1353

Make `Array.prototype.concat` spec compliant #1353

Razican commented Jun 21, 2021 •

edited

neeldug commented Jun 21, 2021 •

edited

neeldug commented Jul 27, 2021 •

edited

HalidOdat left a comment •

edited