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] - Fix built-ins/Array/prototype/toString/non-callable-join-string-tag.js test case #2458

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - Fix built-ins/Array/prototype/toString/non-callable-join-string-tag.js test case #2458

wants to merge 1 commit into from

Conversation

akavi
Copy link
Contributor

@akavi akavi commented Nov 23, 2022 •
edited
Loading

This Pull Request fixes built-ins/Array/prototype/toString/non-callable-join-string-tag.js

Previously:

Array.prototype.toString.call(new Proxy(() => {}, {})) => "[object Object]"

With this change:

Array.prototype.toString.call(new Proxy(() => {}, {})) => "[object Function]"

Reasoning:

  1. 23.1.3.33 Array.prototype.toString: Assuming this does not have a callable join property, delegate to Object.prototype.toString.
  2. 20.1.3.6 Object.prototype.toString: if O has a [[Call]] internal method set, builtinTag should be "Function".
    a) This was what was not correctly done in the existing implementation
  3. 10.5.14 ProxyCreate: If the target passed isCallable, set the [[Call]] internal method

@akavi
Fix built-ins/Array/prototype/toString/non-callable-join-string-tag.j…
50bccd9 
…s test case

Fixes [built-ins/Array/prototype/toString/non-callable-join-string-tag.js](https://github.com/tc39/test262/blob/main/test/built-ins/Array/prototype/toString/non-callable-join-string-tag.js)

Previously:
```
Array.prototype.toString.call(new Proxy(() => {}, {})) => "[object Object]"
```

With this change:
```
Array.prototype.toString.call(new Proxy(() => {}, {})) => "[object Function]"
```

Reasoning:
1) [23.1.3.33 Array.prototype.toString](https://tc39.es/ecma262/#sec-array.prototype.tostring): Assuming `this` does not have a callable `join` property, delegate to `Object.prototype.toString`.
2) [20.1.3.6 Object.prototype.toString](https://tc39.es/ecma262/#sec-object.prototype.tostring): if `O` has a `[[Call]]` internal method set, `builtinTag` should be "Function"
3) [10.5.14 ProxyCreate](https://tc39.es/ecma262/#sec-proxycreate): If the `target` passed isCallable, set the `[[Call]]` internal method
@jedel1043 jedel1043 added bug Something isn't working builtins PRs and Issues related to builtins/intrinsics labels Nov 23, 2022
@jedel1043 jedel1043 added this to the v0.17.0 milestone Nov 23, 2022
@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Merging #2458 (50bccd9) into main (fdac8ec) will increase coverage by 12.66%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main    #2458       +/-   ##
===========================================
+ Coverage   38.86%   51.53%   +12.66%     
===========================================
  Files         316      335       +19     
  Lines       24123    35497    +11374     
===========================================
+ Hits         9375    18292     +8917     
- Misses      14748    17205     +2457
Impacted Files Coverage Δ
boa_engine/src/builtins/object/mod.rs 60.13% <100.00%> (+4.09%) ⬆️
boa_engine/src/vm/opcode/value/mod.rs 50.00% <0.00%> (-50.00%) ⬇️
boa_engine/src/context/icu.rs 54.54% <0.00%> (-25.46%) ⬇️
boa_engine/src/vm/opcode/push/mod.rs 75.00% <0.00%> (-25.00%) ⬇️
boa_engine/src/vm/opcode/push/numbers.rs 80.00% <0.00%> (-20.00%) ⬇️
boa_engine/src/vm/opcode/return_stm/mod.rs 7.40% <0.00%> (-17.60%) ⬇️
...a_engine/src/vm/opcode/binary_ops/macro_defined.rs 85.71% <0.00%> (-14.29%) ⬇️
boa_engine/src/vm/opcode/new/mod.rs 75.67% <0.00%> (-13.22%) ⬇️
boa_engine/src/vm/opcode/try_catch/mod.rs 82.05% <0.00%> (-10.81%) ⬇️
boa_engine/src/builtins/map/mod.rs 76.21% <0.00%> (-10.31%) ⬇️
... and 319 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

raskad
raskad approved these changes Nov 23, 2022
View reviewed changes
Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the fix!

@Razican
Copy link
Member

Razican commented Nov 23, 2022

Test262 conformance changes

Test result main count PR count difference
Total 93,831 93,831 0
Passed 69,626 69,636 +10
Ignored 18,472 18,472 0
Failed 5,733 5,723 -10
Panics 0 0 0
Conformance 74.20% 74.21% +0.01%
Fixed tests (10): 
test/built-ins/Function/prototype/bind/15.3.4.5-10-1.js [strict mode] (previously Failed)
test/built-ins/Function/prototype/bind/15.3.4.5-10-1.js (previously Failed)
test/built-ins/Function/prototype/bind/15.3.4.5-8-2.js [strict mode] (previously Failed)
test/built-ins/Function/prototype/bind/15.3.4.5-8-2.js (previously Failed)
test/built-ins/Object/prototype/toString/proxy-function.js [strict mode] (previously Failed)
test/built-ins/Object/prototype/toString/proxy-function.js (previously Failed)
test/built-ins/Object/prototype/toString/symbol-tag-non-str-proxy-function.js [strict mode] (previously Failed)
test/built-ins/Object/prototype/toString/symbol-tag-non-str-proxy-function.js (previously Failed)
test/built-ins/Array/prototype/toString/non-callable-join-string-tag.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/toString/non-callable-join-string-tag.js (previously Failed)

@Razican
Copy link
Member

Razican commented Nov 23, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 23, 2022
@akavi
Fix built-ins/Array/prototype/toString/non-callable-join-string-tag.j…
9c2bc0a 
…s test case (#2458)

This Pull Request fixes [built-ins/Array/prototype/toString/non-callable-join-string-tag.js](https://github.com/tc39/test262/blob/main/test/built-ins/Array/prototype/toString/non-callable-join-string-tag.js)

Previously:
```
Array.prototype.toString.call(new Proxy(() => {}, {})) => "[object Object]"
```

With this change:
```
Array.prototype.toString.call(new Proxy(() => {}, {})) => "[object Function]"
```

Reasoning:
1) [23.1.3.33 Array.prototype.toString](https://tc39.es/ecma262/#sec-array.prototype.tostring): Assuming `this` does not have a callable `join` property, delegate to `Object.prototype.toString`.
2) [20.1.3.6 Object.prototype.toString](https://tc39.es/ecma262/#sec-object.prototype.tostring): if `O` has a `[[Call]]` internal method set, `builtinTag` should be "Function". 
   a) This was what was not correctly done in the existing implementation
3) [10.5.14 ProxyCreate](https://tc39.es/ecma262/#sec-proxycreate): If the `target` passed isCallable, set the `[[Call]]` internal method
@bors
Copy link

bors bot commented Nov 23, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Fix built-ins/Array/prototype/toString/non-callable-join-string-tag.js test case [Merged by Bors] - Fix built-ins/Array/prototype/toString/non-callable-join-string-tag.js test case Nov 23, 2022
@bors bors bot closed this Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raskad raskad raskad approved these changes

@RageKnify RageKnify RageKnify approved these changes

@Razican Razican Razican approved these changes

@jedel1043 jedel1043 Awaiting requested review from jedel1043

@HalidOdat HalidOdat Awaiting requested review from HalidOdat

@jasonwilliams jasonwilliams Awaiting requested review from jasonwilliams

Assignees
No one assigned
Labels
bug Something isn't working builtins PRs and Issues related to builtins/intrinsics
Projects
None yet
Milestone
v0.17.0
Development

Successfully merging this pull request may close these issues.
5 participants
@akavi @Razican @RageKnify @raskad @jedel1043