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

Normative: `Array.prototype.sort`: comparefn must be fn or undefined #785

Merged
merged 2 commits into from Jun 12, 2017

Conversation

Projects
None yet
9 participants
@ljharb
Member

ljharb commented Jan 25, 2017

This requires implementations throw a TypeError when the compare function is neither undefined nor a function, which is currently vaguely implied but not explicitly required.

This reflects web reality in that every engine except v8 already throws in this case. (Safari does not, but Safari TP and webkit latest do)

eshost output of [1, 2].sort(true):

┌─────────────────────┬─────────────────────────────────────────────────────────────────────────────────────────────┐
│ chakra              │                                                                                             │
│ chakra-es6          │ TypeError: Array.prototype.sort: argument is not a JavaScript object                        │
│ chakra-experimental │                                                                                             │
├─────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────┤
│ d8                  │ 1,2                                                                                         │
│ d8 harmony          │                                                                                             │
├─────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────┤
│ sm                  │                                                                                             │
│                     │ TypeError: invalid Array.prototype.sort argument                                            │
├─────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────┤
│ jsc                 │                                                                                             │
│                     │ TypeError: Array.prototype.sort requires the comparsion function be a function or undefined │
└─────────────────────┴─────────────────────────────────────────────────────────────────────────────────────────────┘

This will likely need "web reality" and "needs consensus" labels.

Normative: `Array.prototype.sort`: comparefn must be fn or undefined
This requires implementations throw a `TypeError` when the compare
function is neither undefined nor a function, which is currently vaguely
implied but not explicitly required.

This reflects web reality in that every engine except v8 already throws
in this case.
spec.html Outdated
@@ -30780,6 +30780,7 @@
<emu-alg>
1. Let _obj_ be ? ToObject(*this* value).
1. Let _len_ be ? ToLength(? Get(_obj_, `"length"`)).
1. If _comparefn_ is not *undefined* and IsCallable(_comparefn_) is *false*, throw a *TypeError* exception.

This comment has been minimized.

@mathiasbynens

mathiasbynens Jan 25, 2017

Member

This could be step 1 instead of step 3, since the other steps aren’t needed in case we throw.

This comment has been minimized.

@mathiasbynens

mathiasbynens Jan 25, 2017

Member

Or have you tested the order in which existing implementations (except for V8) execute these steps?

This comment has been minimized.

@mathiasbynens

mathiasbynens Jan 25, 2017

Member

It seems SpiderMonkey and ChakraCore check comparefn first. Other engines check ToObject(this) first.

$ eshost -e 'Array.prototype.sort.call(null, null)'
#### JavaScriptCore
TypeError: Array.prototype.sort requires that |this| not be null

#### ChakraCore
TypeError: Array.prototype.sort: argument is not a JavaScript object

#### SpiderMonkey
TypeError: invalid Array.prototype.sort argument

#### V8
TypeError: Array.prototype.sort called on null or undefined

#### V8 Harmony
TypeError: Array.prototype.sort called on null or undefined

This comment has been minimized.

@ljharb

ljharb Jan 25, 2017

Member

Since in Firefox, Array.prototype.sort.call(null) throws "TypeError: can't convert null to object" but Array.prototype.sort.call(null, true) throws "TypeError: invalid Array.prototype.sort argument", this matches web reality. Changing now.

This comment has been minimized.

@ljharb

ljharb Jan 25, 2017

Member

aha, thanks for the full comparison :-) I think since more engines agree on your change, that it's still the correct way to go. Updated!

This comment has been minimized.

@claudepache

claudepache Jan 25, 2017

Contributor

For v8, it’s quite normal that it throws on ToObject(this), since it doesn’t throw on comparefn at all. (Still, I also prefer JSC behaviour over SpiderMonkey/Chakra one. ... nevermind)

This comment has been minimized.

@ljharb

ljharb Jan 25, 2017

Member

I'm ambivalent over whether the ToObject check or the IsCallable check comes first, but I think both should happen before the ToLength check.

This comment has been minimized.

@leobalter

leobalter Jun 12, 2017

Member

GitHub was hiding this part here. I'm also glad this is placed before the get length operation. +1 for this change.

@ariya

This comment has been minimized.

ariya commented Jan 27, 2017

escope output

Did you mean eshost ?

@ljharb

This comment has been minimized.

Member

ljharb commented Jan 27, 2017

@ariya thanks, fixed

@bterlson

This comment has been minimized.

Member

bterlson commented Feb 1, 2017

I'm on board with this but let's wait to hear from our v8 friends.

@ajklein

This comment has been minimized.

Contributor

ajklein commented Feb 7, 2017

This seems reasonable to me, though this seems like a fairly low-leverage tightening of Array.prototype.sort given what's still allowed by the spec.

@ljharb

This comment has been minimized.

Member

ljharb commented Feb 7, 2017

@ajklein baby steps!

@evilpie

This comment has been minimized.

Contributor

evilpie commented Feb 7, 2017

Btw, I just checked in SpiderMonkey and we allow [1].sort({}), so non-callable objects. Can you check how other engines behave there?

@evilpie

This comment has been minimized.

Contributor

evilpie commented Feb 7, 2017

Turns out we optimize for arrays with length <= 1 so we never try to call the sort function in that case. We will still fail for [1, 2].sort({}). So this change is probably still fine from our side.

@ljharb

This comment has been minimized.

Member

ljharb commented Feb 7, 2017

I think most engines probably choose not to call the sorting function for < 2 elements; I'll look into that in a separate PR :-)

@claudepache

This comment has been minimized.

Contributor

claudepache commented Feb 7, 2017

Firefox seems to accept a-priori non-callable objects (but not primitives) as sort function but throws when attempting to use them. Safari TP has also an interesting behaviour:

testcase Chrome
Safari 10
Safari TP Firefox Edge
[1].sort(_ => { throw new Error }) [1] [1] [1] [1]
[1].sort({}) [1] [1] [1] TypeError
[1].sort(2) [1] [1] TypeError TypeError
[1,2].sort(_ => { throw new Error }) Error Error Error Error
[1,2].sort({}) [1,2] TypeError TypeError TypeError
[1,2].sort(2) [1,2] TypeError TypeError TypeError

(I vote for Edge semantics.)

Normative: `TypedArray.prototype.sort`: comparefn must be fn or undef…
…ined

This requires implementations throw a `TypeError` when the compare
function is neither undefined nor a function, which is currently vaguely
implied but not explicitly required.
@ljharb

This comment has been minimized.

Member

ljharb commented Jun 10, 2017

I updated to add a commit making the same change in TypedArray.prototype.sort.

test262 tests are here

@bterlson

This comment has been minimized.

Member

bterlson commented Jun 12, 2017

Thank you @ljharb! I will pull this now that tests are pending on test262.

@bterlson bterlson merged commit 564b30b into tc39:master Jun 12, 2017

@ljharb ljharb deleted the ljharb:sort_throws branch Jun 12, 2017

leobalter added a commit to tc39/test262 that referenced this pull request Jun 12, 2017

@rwaldron rwaldron added has tests and removed needs tests labels Jun 22, 2017

kisg pushed a commit to paul99/v8mips that referenced this pull request Sep 15, 2017

[js] Check `comparefn` in (Typed)Array#sort
This patch ensures a `TypeError` is thrown when the argument passed to
`Array.prototype.sort` or `%TypedArray%.prototype.sort` is neither a
function nor `undefined`.

Every other major JavaScript engine already threw in this case. Making
V8’s behavior match increases interoperability.

tc39/ecma262#785

BUG=v8:6542

Cq-Include-Trybots: master.tryserver.v8:v8_linux_noi18n_rel_ng
Change-Id: I412a59810abdd118217c8d8361389ec6c2f640bd
Reviewed-on: https://chromium-review.googlesource.com/668356
Commit-Queue: Mathias Bynens <mathias@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48028}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment