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

Make Array.prototype.concat spec compliant #1353

Merged
merged 11 commits into from
Jul 30, 2021
Merged

Conversation

neeldug
Copy link
Contributor

@neeldug neeldug commented Jun 21, 2021

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
Copy link
Contributor Author

neeldug commented Jun 21, 2021

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
Copy link
Member

Razican commented Jun 21, 2021

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
Copy link
Member

Razican commented Jun 21, 2021

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
Copy link
Contributor Author

neeldug commented Jun 21, 2021

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 neeldug force-pushed the fix-concat branch 2 times, most recently from 285e4c4 to 11a346c Compare June 21, 2021 19:17
@neeldug neeldug requested a review from Razican June 22, 2021 10:48
@neeldug
Copy link
Contributor Author

neeldug commented Jun 26, 2021

@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.

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.

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

boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
@HalidOdat HalidOdat added the builtins PRs and Issues related to builtins/intrinsics label Jul 21, 2021
@HalidOdat HalidOdat added this to the v0.13.0 milestone Jul 21, 2021
boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/array/mod.rs Show resolved Hide resolved
@HalidOdat HalidOdat added the bug Something isn't working label Jul 22, 2021
- 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
Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

neeldug commented Jul 27, 2021

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)

@neeldug neeldug requested a review from HalidOdat July 27, 2021 21:47
@neeldug neeldug requested a review from Razican July 27, 2021 21:47
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
boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
@HalidOdat
Copy link
Member

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)

Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

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

Besides that it looks good to me :)

@neeldug neeldug requested a review from HalidOdat July 29, 2021 22:50
@HalidOdat HalidOdat changed the title fix(boa): fixes concat limit Make Array.prototype.concat spec compliant Jul 30, 2021
@HalidOdat HalidOdat merged commit 9e14cb8 into boa-dev:master Jul 30, 2021
@neeldug neeldug deleted the fix-concat branch July 30, 2021 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working builtins PRs and Issues related to builtins/intrinsics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some Test262 tests run forever
3 participants