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

[Normative] Make Array.prototype.sort stable #1340

Merged
merged 1 commit into from Jan 29, 2019

Conversation

@mathiasbynens
Copy link
Member

@mathiasbynens mathiasbynens commented Nov 2, 2018

All major JavaScript engines now have a stable Array.prototype.sort implementation. Let’s update the spec accordingly.

References:

@mathiasbynens
Copy link
Member Author

@mathiasbynens mathiasbynens commented Nov 2, 2018

Added to November agenda: tc39/agendas@4fdf671

@mathiasbynens
Copy link
Member Author

@mathiasbynens mathiasbynens commented Nov 27, 2018

This got consensus at today’s TC39 meeting. 🎉

mathiasbynens added a commit to mathiasbynens/test262 that referenced this pull request Nov 29, 2018
mathiasbynens added a commit to mathiasbynens/test262 that referenced this pull request Nov 29, 2018
@mathiasbynens
Copy link
Member Author

@mathiasbynens mathiasbynens commented Nov 29, 2018

Test262 tests: tc39/test262#1977

mathiasbynens added a commit to mathiasbynens/test262 that referenced this pull request Nov 30, 2018
@littledan littledan added has tests and removed needs tests labels Dec 6, 2018
@littledan
Copy link
Member

@littledan littledan commented Dec 6, 2018

Since this has tests and consensus, is it ready to merge?

@ljharb
Copy link
Member

@ljharb ljharb commented Dec 6, 2018

The tests aren’t merged yet.

leobalter added a commit to tc39/test262 that referenced this pull request Dec 6, 2018
@mathiasbynens
Copy link
Member Author

@mathiasbynens mathiasbynens commented Dec 6, 2018

@ljharb They are now!

@ljharb ljharb requested review from ljharb, bterlson, zenparsing and tc39/ecma262-editors Dec 6, 2018
@ljharb
ljharb approved these changes Dec 6, 2018
@mathiasbynens mathiasbynens force-pushed the mathiasbynens:sort-stability branch from cca5a66 to a85c9d9 Dec 6, 2018
@8eecf0d2
Copy link

@8eecf0d2 8eecf0d2 commented Dec 7, 2018

I know it's not my place, but lgtm!

@Yaffle
Copy link
Contributor

@Yaffle Yaffle commented Dec 8, 2018

Is the spec for TypedArray#sort still points to the Array#sort? I am not fan of TypedArrays at all, but ... this changes also makes TypedArray#sort stable, which is not true in engines.

@mathiasbynens
Copy link
Member Author

@mathiasbynens mathiasbynens commented Dec 10, 2018

@Yaffle Yeah, %TypedArray%.prototype.sort indeed refers to Array.prototype.sort.

this changes also makes TypedArray#sort stable, which is not true in engines.

Can you elaborate? How is stability even observable for typed arrays (since they can only contain primitives)?

@anba
Copy link
Collaborator

@anba anba commented Dec 10, 2018

It's observable with a custom comparator function: https://bugzilla.mozilla.org/show_bug.cgi?id=1290554 (only the first test case, the second one is SpiderMonkey-specific).

@Yaffle
Copy link
Contributor

@Yaffle Yaffle commented Dec 10, 2018

Custom comparator, which ignores some bits...

const size = 32;
const array = new Uint32Array(size);
for (let i = 0; i < size; i++) {
  array[i] = (i % 3) * 1000 + i;
}
let comparefn = (a, b) => Math.floor(a / 1000) - Math.floor(b / 1000);

console.log('Array#sort', Array.prototype.sort.call(array.slice(0), comparefn).join(' '));
console.log('TypedArray#sort', array.slice(0).sort(comparefn).join(' '));
@mathiasbynens
Copy link
Member Author

@mathiasbynens mathiasbynens commented Dec 10, 2018

Looks like only V8 (issue) and SpiderMonkey (issue) have unstable %TypedArray%.prototype.sort implementations.

To unblock this PR, I'll amend the patch to move the previous non-stable warning to the TypedArray section. I'm hopeful that we'll be able to add the stability guarantee to %TypedArray%.prototype.sort in the spec as well.

@mathiasbynens mathiasbynens force-pushed the mathiasbynens:sort-stability branch from a85c9d9 to 2624806 Dec 10, 2018
@ljharb
ljharb approved these changes Dec 10, 2018
@tschneidereit
Copy link
Member

@tschneidereit tschneidereit commented Jan 29, 2019

FWIW, Mozilla would be ok with making sort stability normative for TypedArray#sort as well, even before our implementation is compliant.

@mathiasbynens
Copy link
Member Author

@mathiasbynens mathiasbynens commented Jan 29, 2019

V8 would be too. As soon as this PR is merged, I’ll work on a follow-up PR that makes %TypedArray%#sort stable as well.

@ljharb ljharb assigned ljharb and unassigned bterlson Jan 29, 2019
All major JavaScript engines now have a stable Array.prototype.sort
implementation. Let’s update the spec accordingly.

References:
- https://v8.dev/blog/array-sort
- chakra-core/ChakraCore@c565b12
- https://mathiasbynens.be/demo/sort-stability
- https://docs.google.com/presentation/d/1mHvxDciqsAchhjepMZlU5fn1DBvglCXSjDWUEtsPGvI/edit
@ljharb ljharb force-pushed the mathiasbynens:sort-stability branch from 2624806 to 257cae9 Jan 29, 2019
@ljharb ljharb merged commit 257cae9 into tc39:master Jan 29, 2019
1 check was pending
1 check was pending
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@mathiasbynens mathiasbynens deleted the mathiasbynens:sort-stability branch Jan 29, 2019
mathiasbynens added a commit to mathiasbynens/ecma262 that referenced this pull request Jan 29, 2019
Currently, only V8 [1] and SpiderMonkey [2] have unstable
`%TypedArray%.prototype.sort` implementations. Both teams have
publicly committed to making their implementations stable, to
match `Array.prototype.sort` [3].

[1] https://bugs.chromium.org/p/v8/issues/detail?id=8567
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1290554
[3] tc39#1340 (comment)
@mathiasbynens
Copy link
Member Author

@mathiasbynens mathiasbynens commented Jan 29, 2019

As soon as this PR is merged, I’ll work on a follow-up PR that makes %TypedArray%#sort stable as well.

OP delivers: #1433

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants