Editorial: Factor out common sort functionality

[jmdyck]

Specifically, create abstract operation SortArrayLike that is invoked by both Array.p.sort and %TypedArray%.p.sort.

This addresses the discussion here.

This is an alternative to PR #2302: if you're interested in this PR, then that one is mostly just churn.

It turned out that I needed a lot of preliminary refactorings: if you try to factor out SortArrayLike without any preliminaries, its clause ends up with lots of references to _comparefn_. This doesn't make sense, because _comparefn_ is now hidden within the closure that is the _SortCompare_ parameter, so SortArrayLike has no access to it.

I've included the preliminaries as separate commits to aid review. (You might want to also keep them at merge-time, I haven't looked at how much squashing will make sense.)

[bakkot]

This drops the

If comparefn is undefined and SortCompare does not act as a consistent comparison function.

condition, with the commit message saying

If comparefn is undefined, the only way that SortCompare can be inconsistent is for the ToString results to be inconsistent, which is covered by the last bullet.

said bullet being

If comparefn is undefined and all applications of ToString, to any specific value passed as an argument to SortCompare, do not produce the same result.

---

I'm not convinced this is correct. In particular, we have

A function comparefn is a consistent comparison function for a set of values S if all of the requirements below are met for all values a, b, and c (possibly the same value) in the set S: [...] Calling comparefn(a, b) does not modify obj or any object on obj's prototype chain.

Since SortCompare calls user code (via ToString) even when _comparefn_ is undefined, it can lead to the modification of obj. And any modification of obj means that SortCompare is not a consistent comparison function, even if such a modification does not lead to ToString returning different values.

[jmdyck]

I believe you're right that my commit message there is incorrect. However, I think the next commit mostly covers up the problem. But the Note about SortCompare being inconsistent could be better. I'll probably squash those two commits and improve the Note.

[jmdyck]

force-pushed to resolve @bakkot's comment. (Squash commits 2 + 3, and improve the Note re when _SortCompare_ is not a consistent comparator.)

In that Note, I use the phrase "if ToString performs method calls", based on the status quo's "Method calls performed by [ToString]". But I wonder if that's the clearest. Would talking about "calling user code" be better?

[michaelficarra]

This is great!

[bakkot]

Couple wordsmithing things.

[bakkot]

Nit: this reads like it's a definition. I'd prefer something which makes it clearer that it is just an observation, such as

It follows from the definition below that SortCompare is consistent comparator unless one or more of the following conditions are met:

[jmdyck]

re wording it as an observation: Yup, good point.

re changing it from "not consistent if" to "consistent unless": That presumes that the list is exhaustive, that there's no other way in which _SortCompare_ could be not consistent. I think that's true, but I'm not 100% sure.

[bakkot]

On further reflection, I'm not convinced this refactoring (of the conditions necessary to trigger implementation-defined sort order, specifically) is an improvement. The original was clear that if comparefn was undefined and any two calls to ToString on some value in S failed to return the same value then the sort order was implementation-defined. I can't tell whether that's entailed by the wording in this PR; if it is, it is much harder to see it.

(The only condition which appears related is reflexivity, but the way I read the definition of "consistent comparator", if it so happened that each time an implementation compared a value to itself it got 0 then the implementation would find reflexivity to hold, even if the observed ToString of that value varied on other occasions.)

[jmdyck]

I was puzzled by this comment, but I think I figured out your point.

For example, consider sorting 3 objects which I'll refer to as A, B, and C. _comparefn_ is *undefined*, so SortCompare will use calls to ToString:

• ToString(A) always returns "a",
• ToString(B) always returns "b", and
• ToString(C) returns either "c1" or "c2". (But C is clever enough to always have ToString return "c1" if SortCompare is ever comparing C to itself.)

So in both the status quo and this PR, the resulting SortCompare satisfies the requirements of a consistent comparison function (comparator). E.g., SortCompare(B,C) is always -1 regardless of what ToString(C) returns, because "b" < "c1" and "b" < "c2" are both true.

However, in the status quo, the inconsistent returns from ToString mean that the sort order is implementation-defined, whereas in this PR, the fact that SortCompare is a consistent comparator is enough to make the sort order not implementation-defined.

So: in some cases where the status quo says the sort order is imp-def, this PR says that the sort order is not imp-def. Is that your point?

If so, ... I don't know, it seems to me that being a consistent comparison function should be the only requirement. As long as C's @@toPrimitive method can make SortCompare behave as a consistent comparison function, then it shouldn't matter if it's doing weird things at a level that SortArrayLike can't see. But yeah, I guess that's a normative change.

[bakkot]

So: in some cases where the status quo says the sort order is imp-def, this PR says that the sort order is not imp-def. Is that your point?

Right. Sorry that wasn't clearer.

I don't know, it seems to me that being a consistent comparison function should be the only requirement.

I can see that argument, but I wouldn't want to push for that normative change - it seems reasonable-ish for an implementation to bail as soon as it sees inconsistent ToString results, even if they're inconsistent in a way which doesn't matter; having to figure out if it matters could be an additional burden. And there's no strong reason to want to add strict requirements for sort order when sorting sets which have elements with non-constant ToString behavior.

[jmdyck]

... it seems reasonable-ish for an implementation to bail as soon as it sees inconsistent ToString results, even if they're inconsistent in a way which doesn't matter; having to figure out if it matters could be an additional burden. ...

But an implementation doesn't have to figure out if it matters, because it doesn't have to detect when the sort order is imp-def. It just has to have code that will work in the non-imp-def cases; what that code does in imp-def cases doesn't matter.

This PR is saying: you (the implementation) have to prove to yourself that your sort code works as long as SortCompare is consistent.
The status quo says: you have to prove it works in cases where SortCompare is consistent minus those cases where SortCompare is consistent but ToString is inconsistent. But I don't think that "minus" helps: you'll just prove it works for all consistent-SortCompare cases and ignore that that covers some cases you didn't need to cover.

[bakkot]

But I don't think that "minus" helps: you'll just prove it works for all consistent-SortCompare cases and ignore that that covers some cases you didn't need to cover.

Eh. I can imagine an implementation which keeps track of whether ToString is consistent and bails if not, rather than writing code that blindly ignores it. Some implementations are more aggressive about checking internal invariants than others.

In any case, it sounds like we're agreed this would be a normative change, so it would need to go in its own PR either way. (Separately, I doubt there's much appetite for it.)

[bakkot]

I'd still like to get this in, but I still believe per discussion above that it is a normative change, and I would like it to be editorial.

To recap, the normative change is that where previously the sort order was implementation-defined if

comparefn is undefined and all applications of ToString, to any specific value passed as an argument to SortCompare, do not produce the same result.

that is no longer the case, and instead it only requires that SortCompare acts as a consistent comparison function, which it may be in certain cases when ToString returns different results on different calls with the same value.

The simplest way to resolve this would be to restore the original three-part list of conditions for implementation-definedness, instead of trying to fold it all into "If SortCompare is not a consistent comparator for the elements of items". We could pursue a normative change which would allow us to perform that simplification as a separate needs-consensus PR.

---

Separately, there is a note which claims

SortCompare is not a consistent comparator if any of the following conditions is true [...]

• comparefn is undefined and for any specific value passed as an argument to SortCompare, all applications of ToString do not produce the same result

This is not in fact sufficient to cause SortCompare not to be a consistent comparator, per the above observation.

[bakkot]

Suggested change

	_comparefn_ is *undefined* and any invocation of ToString (in step <emu-xref href="#step-sortcompare-tostring-x"></emu-xref> or <emu-xref href="#step-sortcompare-tostring-y"></emu-xref>) modifies _obj_ or any object on _obj_'s prototype chain.
	_comparefn_ is *undefined* and some invocation of ToString (in step <emu-xref href="#step-sortcompare-tostring-x"></emu-xref> or <emu-xref href="#step-sortcompare-tostring-y"></emu-xref>) modifies _obj_ or any object on _obj_'s prototype chain.

[jmdyck]

Okay.

[bakkot]

Suggested change

	_comparefn_ is *undefined* and for any specific value passed as an argument to _SortCompare_, all applications of ToString do not produce the same result.
	_comparefn_ is *undefined* and for some specific value passed as an argument to _SortCompare_, not all applications of ToString produce the same result.

[jmdyck]

Fine with me.

[acutmore]

As part of completing our stage 2 spec text for the Change Array By Copy proposal having the SortCompare implicit captures issue addressed in the main spec is seen as a prerequisite. As Array.prototype.toSorted and %TypedArray%.prototype.toSorted also makes use of the current SortCompare pattern.

We were hoping to ask for stage 3 at the next plenary. Do we think this PR has a chance to merge before then. Or is it still worth us following this suggestion to split off the subset of changes we require into a separate PR that could land sooner? Thanks!

[michaelficarra]

@acutmore You should probably split the non-normative changes you need into a separate PR.

[nicolo-ribaudo]

@acutmore I'm starting to work again on #2606, which will unblock the proposal.

[jmdyck]

Force-pushed to:

• Address @bakkot's objection (I think).
• Rename SortArrayLike to SortIndexedProperties (because a TypedArray isn't an array-like object!).
• Add id="consistent-comparator" to the <dfn> to provide a more precise landing spot for links.
• Add <dfn> tags for "sort order", because now there's a cross-clause reference.
• Rebase to main.

[bakkot]

Nit: this splits up the description of the sort order in a weird way. Previously it was ordered as "the sort order is implementation-defined in these cases; otherwise the sort order must satisfy the following properties:". Now it's "the sort order is implementation-defined in these cases", then a new subclause with a whole algorithm in it, then "if it's not implementation-defined the sort order must satisfy the following properties:".

I imagine this is because the two sections of constraints on sort order refer to specific steps of different algorithms, and so the constraints are grouped with the corresponding algorithms. Even so, it seems like they should be contiguous, as it was previously. I think the way I'd do it would be to place this section ("it must satisfy all of the following conditions") immediately following the "implementation-defined if any of the following conditions is true" section, and replace "after completion of step X of the algorithm above" with "after completion of step X of SortIndexedProperties". (And also move the definition of "sort order" to just prior to the first use of it.)

[jmdyck]

Nit: this splits up the description of the sort order in a weird way.

Yes, that seemed necessary to address your objection.

I think the way I'd do it would be to place this section ("it must satisfy all of the following conditions") immediately following the "implementation-defined if any of the following conditions is true" section, [...] (And also move the definition of "sort order" to just prior to the first use of it.)

The 'sort order' definition and the "it must satisfy all of the following conditions" chunk specify the required behavior of SortIndexedProperties, in terms of aliases that only occur in SortIndexedProperties. You're suggesting that that specification be moved out of the SortIndexedProperties clause?

[bakkot]

You're suggesting that that specification be moved out of the SortIndexedProperties clause?

Yup. I would also be OK with moving the other half into the SortIndexedProperties clause, I suppose, though that seems marginally worse.

Note that the "The sort order is implementation-defined if any of the following conditions is true" part is also specifying the behavior of SortIndexedProperties. As I said, I recognize that the "implementation-defined if" part is in terms of aliases in a different algorithm than the "it must satisfy all of the following conditions" part, but I don't think "which algorithm happens to have the aliases referred to in this portion of the constraints" should be controlling here. It is more important that the constraints be contiguous.

[jmdyck]

I would also be OK with moving the other half into the SortIndexedProperties clause, I suppose, though that seems marginally worse.

Of the two suggestions, I dislike that one far less, so I've added a fixup to that effect. (Compromise!)

[...] I don't think "which algorithm happens to have the aliases referred to in this portion of the constraints" should be controlling here. It is more important that the constraints be contiguous.

Understood, I just disagree. I'm not saying that locality-of-aliases should always win, only that it I think it gives the best outcome in this case.

[bakkot]

Thanks for updating; this does indeed now appear to be strictly editorial.

[arhadthedev]

Suggested change

	<p>The `localeCompare` method, if considered as a function of two arguments *this* and _that_, is a consistent comparator (as defined in <emu-xref href="#sec-array.prototype.sort"></emu-xref>) on the set of all Strings.</p>
	<p>The `localeCompare` method, if considered as a function of two arguments *this* and _that_, is a consistent comparator on the set of all Strings.</p>

Consistent comparator already autolinks inside 23.1.3.28.1.

[jmdyck]

Thanks, done.

[bakkot]

Why the reference to Array.prototype.sort?

It's true that the only other caller of SortIndexedProperties is TA.prototype.sort, and in that case the elements are all primitives and so ToString will necessarily be consistent, so this requirement is redundant in that case. But it still seems odd to explicitly carve out Array.prototype.sort here; it suggests to the reader that this requirement is not required in the TypedArray case, rather than being redundant there.

[jmdyck]

It's true that the only other caller of SortIndexedProperties is TA.prototype.sort, and in that case the elements are all primitives and so ToString will necessarily be consistent,

TA.p.sort's SortCompare doesn't call ToString. I think it might confuse the reader to suggest that it does. I suppose we could have a Note explaining why there isn't a corresponding imp-def criterion for TA.

[bakkot]

TA.p.sort's SortCompare doesn't call ToString

Ah, yeah. Perhaps

Suggested change

<p>The <dfn id="sort-order">sort order</dfn> is the ordering of _items_ after completion of step <emu-xref href="#step-array-sort"></emu-xref> of the algorithm above. The sort order is implementation-defined if _SortCompare_ is not a consistent comparator for the elements of _items_ (see below). The sort order is also implementation-defined if SortIndexedProperties is invoked by <emu-xref href="#sec-array.prototype.sort">Array.prototype.sort</emu-xref>, _comparefn_ is *undefined*, and all applications of ToString, to any specific value passed as an argument to _SortCompare_, do not produce the same result.</p>

<p>The <dfn id="sort-order">sort order</dfn> is the ordering of _items_ after completion of step <emu-xref href="#step-array-sort"></emu-xref> of the algorithm above. The sort order is implementation-defined if _SortCompare_ is not a consistent comparator for the elements of _items_ (see below). The sort order is also implementation-defined if _comparefn_ is *undefined* and all applications of ToString (if any), to any specific value passed as an argument to _SortCompare_, do not produce the same result.</p>

?

[jmdyck]

The thing is, specifically referring (and linking) to A.p.sort gives a context for the rest of the sentence, so there's a fair chance that the reader will understand that _comparefn_ refers to the A.p.sort parameter.

If we delete that reference, then it would look more like _comparefn_ refers to an alias in SortIndexedProperties, which might puzzle readers. Myself, I'd be inclined to guess it was a copy/paste error.

Now, you could address that problem by saying something like "The sort order is also implementation-defined if the function that calls SortIndexedProperties has a _comparefn_ parameter, and the value of that parameter is *undefined*, and [etc]." That at least would set up the context and look intentional.

But to me, it seems like unnecessary generalization: we know that the sentence only applies A.p.sort -- why not just say so? Are you thinking that there might be other callers in the future, and they will automatically be handled correctly by the general text? I'm not so sure.

But I'm willing to change it to something like that if you still think it's an improvement.

[bakkot]

The thing is, specifically referring (and linking) to A.p.sort gives a context for the rest of the sentence, so there's a fair chance that the reader will understand that _comparefn_ refers to the A.p.sort parameter.

Hm, that's fair, but I think that can be conveyed more clearly with a different phrasing. Perhaps just reordering this sentence a bit? As in

Suggested change

	<p>The <dfn id="sort-order">sort order</dfn> is the ordering of _items_ after completion of step <emu-xref href="#step-array-sort"></emu-xref> of the algorithm above. The sort order is implementation-defined if _SortCompare_ is not a consistent comparator for the elements of _items_ (see below). The sort order is also implementation-defined if SortIndexedProperties is invoked by <emu-xref href="#sec-array.prototype.sort">Array.prototype.sort</emu-xref>, _comparefn_ is *undefined*, and all applications of ToString, to any specific value passed as an argument to _SortCompare_, do not produce the same result.</p>
	<p>The <dfn id="sort-order">sort order</dfn> is the ordering of _items_ after completion of step <emu-xref href="#step-array-sort"></emu-xref> of the algorithm above. The sort order is implementation-defined if _SortCompare_ is not a consistent comparator for the elements of _items_ (see below). When SortIndexedProperties is invoked by <emu-xref href="#sec-array.prototype.sort">Array.prototype.sort</emu-xref>, the sort order is also implementation-defined if _comparefn_ is *undefined* and all applications of ToString, to any specific value passed as an argument to _SortCompare_, do not produce the same result.</p>

---

Are you thinking that there might be other callers in the future, and they will automatically be handled correctly by the general text?

Well, there is a currently proposed new caller, but that wasn't actually the motivation. It really was just what I put above: the current phrasing to me suggests that it is specifically exempting TA.prototype.sort, rather than (as is actually the case) simply not being relevant to that algorithm.

[ljharb]

If it will make the change array by copy proposal easier to review, it'd be nice to get it in before the March meeting.

[bakkot]

Doesn't need to be done in this PR, in my opinion.

[bakkot]

(Generally I'm of the opinion that changes should be well-motivated in their own right; if we would only be making a change for the sake of a later PR, we should just make that change in that PR.)

[acutmore]

I'm happy either way, the change being in this PR or as part of the proposal. Suggestion was only to reduce the frequency of spec chance in this area 🙂

[michaelficarra]

I think that change is more appropriate as part of the proposal.

[bakkot]

LGTM other than comments.

For anyone else reviewing, I recommend reviewing once with whitespace changes visible and once with whitespace changes suppressed.

[jmdyck]

(force-pushed to squash in the fixup commits)

[jmdyck]

(force-pushed to resolve merge conflicts from 2547)

[michaelficarra]

We don't use the term "completion" on its own any more. We've removed its <dfn>.

Suggested change

	1. [id="step-array-sort"] Sort _items_ using an implementation-defined sequence of calls to _SortCompare_. If any such call returns an abrupt completion, stop before performing any further calls to _SortCompare_ or steps in this algorithm and return that completion.
	1. [id="step-array-sort"] Sort _items_ using an implementation-defined sequence of calls to _SortCompare_. If any such call returns an abrupt completion, stop before performing any further calls to _SortCompare_ or steps in this algorithm and return that Completion Record.

[michaelficarra]

Also, "or steps in this algorithm" is of dubious value. That's how returning works everywhere else in this document.

[jmdyck]

We don't use the term "completion" on its own any more.

Actually, the status quo has 2 or 3 such occurrences, including this one. Also, there's 7 or 8 occurrences of "completion value": what's the editorial stance on those?

[jmdyck]

Also, "or steps in this algorithm" is of dubious value. That's how returning works everywhere else in this document.

That phrase was added by PR #1585, apparently as part of a suggestion from @bakkot.

[michaelficarra]

We meant to eliminate "completion" or "completion value" as part of #2547. The editorial convention is to use "Completion Record" when talking about one generically or the terms "normal completion", "abrupt completion", etc when referring to specific kinds.

[bakkot]

Re "or steps in this algorithm", I don't think it needs to be (or should be) removed as part of this PR - I'd prefer to keep this a pure refactoring, and making other changes at the same time obscures those changes. @michaelficarra I'm fine with dropping the phrase in a separate PR if you feel inclined to make that change.

[syg]

+1 to separate PR.

[michaelficarra]

Suggested change

<p>The <dfn id="sort-order">sort order</dfn> is the ordering of _items_ after completion of step <emu-xref href="#step-array-sort"></emu-xref> of the algorithm above. The sort order is implementation-defined if _SortCompare_ is not a consistent comparator for the elements of _items_ (see below). When SortIndexedProperties is invoked by <emu-xref href="#sec-array.prototype.sort">Array.prototype.sort</emu-xref>, the sort order is also implementation-defined if _comparefn_ is *undefined*, and all applications of ToString, to any specific value passed as an argument to _SortCompare_, do not produce the same result.</p>

<p>The <dfn id="sort-order">sort order</dfn> is the ordering of _items_ after completion of step <emu-xref href="#step-array-sort"></emu-xref> of the algorithm above. The sort order is implementation-defined if _SortCompare_ is not a consistent comparator for the elements of _items_. When SortIndexedProperties is invoked by <emu-xref href="#sec-array.prototype.sort">Array.prototype.sort</emu-xref>, the sort order is also implementation-defined if _comparefn_ is *undefined*, and all applications of ToString, to any specific value passed as an argument to _SortCompare_, do not produce the same result.</p>

The link should be sufficient.

[jmdyck]

Agreed.

[michaelficarra]

LGTM otherwise.

[jmdyck]

(force-pushed to resolve a merge conflict and squash in a fixup commit)

[syg]

lgtm!

[syg]

+1 to separate PR.