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

Bug(or Miss): Array/TypedArray#toSorted seems not requiring stability while *#sort requires. #3422

Open
LumaKernel opened this issue Sep 9, 2024 · 1 comment · May be fixed by #3424
Open

Comments

@LumaKernel
Copy link

LumaKernel commented Sep 9, 2024

Description:

Array#sort is defined as stable in ES2019 (10th).
Stable sort for Array.prototype.sort

Array#toSorted is introduced in ES2023 (14th).

In ES2024 (15th), the statements for stability can be found as follows:

  • In introduction:

    ECMAScript 2019 introduced a few <OMITTED> Other updates included requiring that Array.prototype.sort be a stable sort, requiring that <OMITTED>

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

  • In 23.1.3.30 Array.prototype.sort ( comparefn ):

    This method sorts the elements of this array. The sort must be stable (that is, elements that compare equal must remain in their original order). <OMITTED>

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

  • In 23.2.3.29 %TypedArray%.prototype.sort ( comparefn ):

    This is a distinct method that, except as described below, implements the same requirements as those of Array.prototype.sort as defined in 23.1.3.30. <OMITTED>

    So it is indirectly mentioning the requirements for stability of the sort. It's introduced in [Normative] Make %TypedArray%.prototype.sort stable #1433

While them, it seems there're no references to theses stability from toSroted functions. First introduced in #2997
toSorted is just using SortIndexedProperties. But SortIndexedProperties is also not requiring the stability (to me), so currently it seems it's ok to be not stable.

In 23.1.3.30.1 SortIndexedProperties ( obj, len, SortCompare, holes ), there's the requirements for permutation used for the sort, but it's only requiring that any 2 elements which is strictly ordered, is also ordered in the result.

I'm not sure why stability is just mentioned in Array#sort but not in SortIndexedProperties, but I believe that's why this is just missed when toSorted is introduced.

SortIndexedProperties seems just referenced to by 5:

  • 23.1.3.30 Array.prototype.sort ( comparefn ) (2)
  • 23.1.3.34 Array.prototype.toSorted ( comparefn )
  • 23.2.3.29 %TypedArray%.prototype.sort ( comparefn )
  • 23.2.3.33 %TypedArray%.prototype.toSorted ( comparefn )

So one idea I can come up with is just moving the stability requirements to SortIndexedProperties.

eshost Output:

If I have chance to see implementations of engines I want to confirm, but now I could think they may share the implementations of sort and toSorted internally. (not sure)

@bakkot bakkot linked a pull request Sep 9, 2024 that will close this issue
@bakkot
Copy link
Contributor

bakkot commented Sep 9, 2024

Thanks for the catch and the detailed report. I'm almost certain this was just an oversight; I've filed #3424 to fix.

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

Successfully merging a pull request may close this issue.

2 participants