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: toSorted methods must be stable #3424

Merged
merged 4 commits into from
Oct 10, 2024
Merged

Normative: toSorted methods must be stable #3424

merged 4 commits into from
Oct 10, 2024

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Sep 9, 2024

Fixes #3422. Also fixes a closely related issue where toSorted was incorrectly not permitted to have implementation-defined order using the default comparator when ToString is inconsistent.

The stability requirements of Array.prototype.sort and TypedArray.prototype.sort are documented only in the introductory paragraphs of those sections. The actual sorting, and its detailed requirements except for stability, are in SortIndexedProperties.

When the toSorted methods were introduced to Array and TypedArray they referenced only SortIndexedProperties, thus implicitly (and accidentally) omitting the stability requirement. This PR spells out the stability requirement more formally in SortIndexedProperties, which means it is enforced on toSorted as well.

@ljharb ljharb added normative change Affects behavior required to correctly evaluate some ECMAScript source text web reality labels Sep 9, 2024
@bakkot
Copy link
Contributor Author

bakkot commented Sep 9, 2024

I've added another commit fixing a closely related issue, which is that SortIndexedProperties says the order is implementation-defined for sort, but not toSorted, when using the default comparison and ToString returns inconsistent results. The commit makes that apply to toSorted as well.

Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

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

lgtm. WDYT about removing the "sort must be stable" sentence in the description of Array.prototype.sort, now that it's pushed to this AO and shared by all callers?

@bakkot
Copy link
Contributor Author

bakkot commented Sep 9, 2024

SGTM, done.

@acutmore
Copy link
Contributor

Thanks! Sorry I missed this in the original.

@bakkot bakkot added has consensus This has committee consensus. ready to merge Editors believe this PR needs no further reviews, and is ready to land. labels Oct 9, 2024
ljharb pushed a commit to ljharb/ecma262 that referenced this pull request Oct 10, 2024
ljharb pushed a commit to ljharb/ecma262 that referenced this pull request Oct 10, 2024
ljharb pushed a commit to ljharb/ecma262 that referenced this pull request Oct 10, 2024
@ljharb
Copy link
Member

ljharb commented Oct 10, 2024

ac511f4 is the commit that will fix the IPR check that I included in this PR; I'm going to merge past that one failure to fix main.

@ljharb ljharb merged commit 6de33c0 into main Oct 10, 2024
8 of 9 checks passed
@ljharb ljharb deleted the stable-tosorted branch October 10, 2024 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. normative change Affects behavior required to correctly evaluate some ECMAScript source text ready to merge Editors believe this PR needs no further reviews, and is ready to land. web reality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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