Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Update sorting methods to use abstract closures #78

Merged
merged 7 commits into from
Mar 17, 2022
Merged

Update sorting methods to use abstract closures #78

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
32d1e92
update sorting methods to use abstract closures
acutmore Mar 13, 2022
a0c6748
Apply suggestions from code review
acutmore Mar 14, 2022
7e2a678
[squash] rename AOs to Compare[Typed]ArrayElements
acutmore Mar 14, 2022
333f55d
[squash] SortIndexProperties returns internal list instead of mutatin…
acutmore Mar 14, 2022
17779e6
[squash] type _comparefn_ param as function or undefined
acutmore Mar 14, 2022
7c57a62
[squash] checkHasProperty is now ignoreHoles
acutmore Mar 14, 2022
0152db2
[squash] add note about holes and rename param to skipHoles
acutmore Mar 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
233 changes: 189 additions & 44 deletions spec.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,110 @@ <h1>Array Objects</h1>
<emu-clause id="sec-properties-of-the-array-prototype-object">
<h1>Properties of the Array Prototype Object</h1>

<emu-clause id="sec-array.prototype.sort" oldids="sec-sortcompare">
Copy link
Collaborator Author

@acutmore acutmore Mar 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Array.prototype.sort is updated to move most of the body of the _SortCompare_ inline abstract closure into a new AO that can be shared with toSorted.

<h1>Array.prototype.sort ( _comparefn_ )</h1>

<p>When the `sort` method is called, the following steps are taken:</p>
<emu-alg>
1. [id="step-array-sort-comparefn"] If _comparefn_ is not *undefined* and IsCallable(_comparefn_) is *false*, throw a *TypeError* exception.
1. Let _obj_ be ? ToObject(*this* value).
1. [id="step-array-sort-len"] Let _len_ be ? LengthOfArrayLike(_obj_).
1. Let _SortCompare_ be a new Abstract Closure with parameters (_x_, _y_) that captures _comparefn_ and performs the following steps when called:
1. <del>If _x_ and _y_ are both *undefined*, return *+0*<sub>𝔽</sub>.</del>
1. <del>If _x_ is *undefined*, return *1*<sub>𝔽</sub>.</del>
1. <del>If _y_ is *undefined*, return *-1*<sub>𝔽</sub>.</del>
1. <del>If _comparefn_ is not *undefined*, then</del>
1. <del>Let _v_ be ? ToNumber(? Call(_comparefn_, *undefined*, &laquo; _x_, _y_ &raquo;)).</del>
1. <del>If _v_ is *NaN*, return *+0*<sub>𝔽</sub>.</del>
1. <del>Return _v_.</del>
1. <del>[id="step-sortcompare-tostring-x"] Let _xString_ be ? ToString(_x_).</del>
1. <del>[id="step-sortcompare-tostring-y"] Let _yString_ be ? ToString(_y_).</del>
1. <del>Let _xSmaller_ be ! IsLessThan(_xString_, _yString_, *true*).</del>
1. <del>If _xSmaller_ is *true*, return *-1*<sub>𝔽</sub>.</del>
1. <del>Let _ySmaller_ be ! IsLessThan(_yString_, _xString_, *true*).</del>
1. <del>If _ySmaller_ is *true*, return *1*<sub>𝔽</sub>.</del>
1. <del>Return *+0*<sub>𝔽</sub>.</del>
1. <ins>Return ? CompareArrayElements(_x_, _y_, _comparefn_).</ins>
1. <del>Return ? SortIndexedProperties(_obj_, _len_, _SortCompare_).</del>
1. <ins>Let _sortedList_ be ? SortIndexedProperties(_obj_, _len_, _SortCompare_, *true*).</ins>
1. <ins>Let _itemCount_ be the number of elements in _sortedList_.</ins>
1. <ins>Let _j_ be 0.</ins>
1. <ins>Repeat, while _j_ &lt; _itemCount_,</ins>
1. <ins>Perform ? CreateDataPropertyOrThrow(_obj_, ! ToString(𝔽(_j_)), _sortedList_[_j_]).</ins>
1. <ins>Set _j_ to _j_ + 1.</ins>
1. <ins>Repeat, while _j_ &lt; _len_,</ins>
1. <ins>Perform ? DeletePropertyOrThrow(_obj_, ! ToString(𝔽(_j_))).</ins>
1. <ins>Set _j_ to _j_ + 1.</ins>
1. <ins>Return _obj_.</ins>
</emu-alg>
</emu-clause>

<emu-clause id="sec-comparearrayelements" type="abstract operation">
<h1>
CompareArrayElements(
_x_: unknown,
_y_: unknown,
_comparefn_: unknown
ljharb marked this conversation as resolved.
Show resolved Hide resolved
): either a normal completion containing a Number or an abrupt completion
</h1>
<dl class="header">
</dl>
<emu-alg>
1. If _x_ and _y_ are both *undefined*, return *+0*<sub>𝔽</sub>.
1. If _x_ is *undefined*, return *1*<sub>𝔽</sub>.
1. If _y_ is *undefined*, return *-1*<sub>𝔽</sub>.
1. If _comparefn_ is not *undefined*, then
1. Let _v_ be ? ToNumber(? Call(_comparefn_, *undefined*, &laquo; _x_, _y_ &raquo;)).
1. If _v_ is *NaN*, return *+0*<sub>𝔽</sub>.
1. Return _v_.
1. [id="step-sortcompare-tostring-x"] Let _xString_ be ? ToString(_x_).
1. [id="step-sortcompare-tostring-y"] Let _yString_ be ? ToString(_y_).
1. Let _xSmaller_ be ! IsLessThan(_xString_, _yString_, *true*).
1. If _xSmaller_ is *true*, return *-1*<sub>𝔽</sub>.
1. Let _ySmaller_ be ! IsLessThan(_yString_, _xString_, *true*).
1. If _ySmaller_ is *true*, return *1*<sub>𝔽</sub>.
1. Return *+0*<sub>𝔽</sub>.
ljharb marked this conversation as resolved.
Show resolved Hide resolved
</emu-alg>
</emu-clause>

<emu-clause id="sec-sortindexedproperties" type="abstract operation">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sortIndexedProperties is updated to make it usable by toSorted.

a) toSorted needs to write out to a new array, rather than mutate the original instance
b) toSorted ignores 'holes' in arrays, and will read them regardless.

<h1>
SortIndexedProperties (
_obj_: an Object,
_len_: a non-negative integer,
_SortCompare_: an Abstract Closure with two parameters,
<ins>_checkHasProperty_: a Boolean</ins>
): either a normal completion containing <del>an Object</del><ins>a List</ins> or an abrupt completion
</h1>
<dl class="header">
</dl>
<emu-alg>
1. Let _items_ be a new empty List.
1. Let _k_ be 0.
1. Repeat, while _k_ &lt; _len_,
1. Let _Pk_ be ! ToString(𝔽(_k_)).
1. <del>Let _kPresent_ be ? HasProperty(_obj_, _Pk_).</del>
1. <ins>If _checkHasProperty_ is *true*, then</ins>
ljharb marked this conversation as resolved.
Show resolved Hide resolved
1. <ins>Let _kRead_ be ? HasProperty(_obj_, _Pk_).</ins>
1. <ins>Else,</ins>
1. <ins>Let _kRead_ be *true*.</ins>
1. If <del>_kPresent_</del><ins>_kRead_</ins> is *true*, then
1. Let _kValue_ be ? Get(_obj_, _Pk_).
1. Append _kValue_ to _items_.
1. Set _k_ to _k_ + 1.
1. <del>Let _itemCount_ be the number of elements in _items_.</del>
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.
1. <del>Let _j_ be 0.</del>
1. <del>Repeat, while _j_ &lt; _itemCount_,</del>
1. <del>Perform ? Set(_obj_, ! ToString(𝔽(_j_)), _items_[_j_], *true*).</del>
1. <del>Set _j_ to _j_ + 1.</del>
1. <del>Repeat, while _j_ &lt; _len_,</del>
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Add a note that this only happens when we respect holes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good shout. Note added in Array.prototype.sort where the delete prop loop was moved to.

1. <del>Perform ? DeletePropertyOrThrow(_obj_, ! ToString(𝔽(_j_))).</del>
1. <del>Set _j_ to _j_ + 1.</del>
1. <del>Return _obj_.</del>
1. <ins>Return _items_.</ins>
</emu-alg>

<emu-clause id="sec-array.prototype.toReversed">
<h1>Array.prototype.toReversed ( )</h1>

Expand All @@ -39,7 +143,7 @@ <h1>Array.prototype.toReversed ( )</h1>
</emu-clause>

<emu-clause id="sec-array.prototype.toSorted">
<h1>Array.prototype.toSorted ( _compareFn_ )</h1>
<h1>Array.prototype.toSorted ( _comparefn_ )</h1>

<p>When the *toSorted* method is called, the following steps are taken:</p>

Expand All @@ -48,19 +152,13 @@ <h1>Array.prototype.toSorted ( _compareFn_ )</h1>
1. Let _O_ be ? ToObject(*this* value).
1. Let _len_ be ? LengthOfArrayLike(_O_).
1. Let _A_ be ? ArrayCreate(𝔽(_len_)).
1. Let _items_ be a new empty List.
1. Let _k_ be 0.
1. Repeat, while _k_ &lt; _len_,
1. Let _Pk_ be ! ToString(𝔽(_k_)).
1. Let _kValue_ be ? Get(_O_, _Pk_).
1. Append _kValue_ to _items_.
1. Set _k_ to _k_ + 1.
1. 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. Let _SortCompare_ be a new Abstract Closure with parameters (_x_, _y_) that captures _comparefn_ and performs the following steps when called:
1. Return ? CompareArrayElements(_x_, _y_, _comparefn_).
1. Let _sortedList_ be ? SortIndexedProperties(_obj_, _len_, _SortCompare_, *false*).
1. Let _j_ be 0.
1. For each element _E_ of _items_, do
1. Let _Pj_ be ! ToString(𝔽(_j_)).
1. Perform ! CreateDataPropertyOrThrow(_A_, _Pj_, _E_).
1. Set _j_ to _j_ + 1.
1. Repeat, while _j_ &lt; _len_,
1. Perform ! CreateDataPropertyOrThrow(_A_, ! ToString(𝔽(_j_)), _sortedList_[_j_]).
1. Set _j_ to _j_ + 1.
1. Return _A_.
</emu-alg>
</emu-clause>
Expand Down Expand Up @@ -197,6 +295,75 @@ <h1>The %TypedArray% Intrinsic Object</h1>
<emu-clause id="sec-properties-of-the-%typedarrayprototype%-object">
<h1>Properties of the %TypedArray% Prototype Object</h1>

<emu-clause id="sec-%typedarray%.prototype.sort" oldids="sec-typedarraysortcompare">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

%typedarray%.prototype.sort updated for same reason as Array above.

<h1>%TypedArray%.prototype.sort ( _comparefn_ )</h1>

<p>The following steps are performed:</p>
<emu-alg>
1. If _comparefn_ is not *undefined* and IsCallable(_comparefn_) is *false*, throw a *TypeError* exception.
1. Let _obj_ be the *this* value.
1. Perform ? ValidateTypedArray(_obj_).
1. Let _buffer_ be _obj_.[[ViewedArrayBuffer]].
1. Let _len_ be _obj_.[[ArrayLength]].
1. NOTE: The following closure performs a numeric comparison rather than the string comparison used in <emu-xref href="#sec-array.prototype.sort"></emu-xref>.
1. Let _SortCompare_ be a new Abstract Closure with parameters (_x_, _y_) that captures _comparefn_ and _buffer_ and performs the following steps when called:
1. <del>Assert: Both Type(_x_) and Type(_y_) are Number or both are BigInt.</del>
1. <del>If _comparefn_ is not *undefined*, then</del>
1. <del>Let _v_ be ? ToNumber(? Call(_comparefn_, *undefined*, &laquo; _x_, _y_ &raquo;)).</del>
1. <del>If IsDetachedBuffer(_buffer_) is *true*, throw a *TypeError* exception.</del>
1. <del>If _v_ is *NaN*, return *+0*<sub>𝔽</sub>.</del>
1. <del>Return _v_.</del>
1. <del>If _x_ and _y_ are both *NaN*, return *+0*<sub>𝔽</sub>.</del>
1. <del>If _x_ is *NaN*, return *1*<sub>𝔽</sub>.</del>
1. <del>If _y_ is *NaN*, return *-1*<sub>𝔽</sub>.</del>
1. <del>If _x_ &lt; _y_, return *-1*<sub>𝔽</sub>.</del>
1. <del>If _x_ &gt; _y_, return *1*<sub>𝔽</sub>.</del>
1. <del>If _x_ is *-0*<sub>𝔽</sub> and _y_ is *+0*<sub>𝔽</sub>, return *-1*<sub>𝔽</sub>.</del>
1. <del>If _x_ is *+0*<sub>𝔽</sub> and _y_ is *-0*<sub>𝔽</sub>, return *1*<sub>𝔽</sub>.</del>
1. <del>Return *+0*<sub>𝔽</sub>.</del>
1. <ins>Return ? CompareTypedArrayElements(_x_, _y_, _comparefn_, _buffer_).</ins>
1. <del>Return ? SortIndexedProperties(_obj_, _len_, _SortCompare_).</del>
1. <ins>Let _sortedList_ be ? SortIndexedProperties(_obj_, _len_, _SortCompare_, *false*).</ins>
1. <ins>Let _j_ be 0.</ins>
1. <ins>Repeat, while _j_ &lt; _len_,</ins>
1. <ins>Perform ! Set(_obj_, ! ToString(𝔽(_j_)), _sortedList_[_j_], *true*).</ins>
1. <ins>Set _j_ to _j_ + 1.</ins>
1. <ins>Return _obj_.</ins>
</emu-alg>
</emu-clause>

<emu-clause id="sec-comparetypedarrayelements" type="abstract operation">
<h1>
CompareTypedArrayElements(
_x_: unknown,
_y_: unknown,
_comparefn_: unknown,
_buffer_: An ArrayBuffer
): either a normal completion containing a Number or an abrupt completion
</h1>
<dl class="header">
</dl>
<emu-alg>
1. Assert: Both Type(_x_) and Type(_y_) are Number or both are BigInt.
1. If _comparefn_ is not *undefined*, then
1. Let _v_ be ? ToNumber(? Call(_comparefn_, *undefined*, &laquo; _x_, _y_ &raquo;)).
1. If IsDetachedBuffer(_buffer_) is *true*, throw a *TypeError* exception.
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be checked? Isn't it done implicitly with the Get and Set calls?

Copy link

@bakkot bakkot Mar 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Nope, Get and Set don't fail on detached buffers.

Also, this check happens after every call to comparefn, whereas the Get and Set calls happen only before and after sorting the list.

[struck a false comment here, whoops]

Copy link
Collaborator Author

@acutmore acutmore Mar 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Good point. This check was there right from the ES6 version of TypedArray.prototype.sort.

However until ES2021 sorting was defined as performing:

an implementation-dependent sequence of calls to the Get, Set ...

So the buffer could become detached before reading had finished. This was updated to match the current semantics in tc39/ecma262#1585

There were a lot of comments on that PR and maybe this was missed in amongst the noise?

Copy link

Choose a reason for hiding this comment

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

Yeah, probably.

Let's move discussion of what the behavior should be to tc39/ecma262#2646 (comment) (which is about a similar change to a different TA method). Changing the existing behavior is out of scope for this proposal, I imagine.

1. If _v_ is *NaN*, return *+0*<sub>𝔽</sub>.
1. Return _v_.
1. If _x_ and _y_ are both *NaN*, return *+0*<sub>𝔽</sub>.
1. If _x_ is *NaN*, return *1*<sub>𝔽</sub>.
1. If _y_ is *NaN*, return *-1*<sub>𝔽</sub>.
1. If _x_ &lt; _y_, return *-1*<sub>𝔽</sub>.
1. If _x_ &gt; _y_, return *1*<sub>𝔽</sub>.
1. If _x_ is *-0*<sub>𝔽</sub> and _y_ is *+0*<sub>𝔽</sub>, return *-1*<sub>𝔽</sub>.
1. If _x_ is *+0*<sub>𝔽</sub> and _y_ is *-0*<sub>𝔽</sub>, return *1*<sub>𝔽</sub>.
1. Return *+0*<sub>𝔽</sub>.
</emu-alg>
<emu-note>
This performs a numeric comparison rather than the string comparison used in <emu-xref href="#sec-comparearrayelements"></emu-xref>.
</emu-note>
</emu-clause>

<emu-clause id="sec-%typedarray%.prototype.toReversed">
<h1>%TypedArray%.prototype.toReversed ( )</h1>

Expand All @@ -219,49 +386,27 @@ <h1>%TypedArray%.prototype.toReversed ( )</h1>
</emu-clause>

<emu-clause id="sec-%typedarray%.prototype.toSorted">
<h1>%TypedArray%.prototype.toSorted ( _compareFn_ )</h1>
<h1>%TypedArray%.prototype.toSorted ( _comparefn_ )</h1>

<p>When the *toSorted* method is called, the following steps are taken:</p>

<emu-alg>
1. If _comparefn_ is not *undefined* and IsCallable(_comparefn_) is *false*, throw a *TypeError* exception.
1. Let _O_ be the *this* value.
1. Perform ? ValidateTypedArray(_O_).
1. Let _buffer_ be _obj_.[[ViewedArrayBuffer]].
1. Let _len_ be _O_.[[ArrayLength]].
1. Let _A_ be ? TypedArrayCreateSameType(_O_, &laquo; 𝔽(_len_) &raquo;).
1. Let _items_ be a new empty List.
1. Let _k_ be 0.
1. Repeat, while _k_ &lt; _len_,
1. Let _Pk_ be ! ToString(𝔽(_k_)).
1. Let _kValue_ be ! Get(_O_, _Pk_).
1. Append _kValue_ to _items_.
1. Set _k_ to _k_ + 1.
1. 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. NOTE: The following closure performs a numeric comparison rather than the string comparison used in <emu-xref href="#sec-array.prototype.toSorted"></emu-xref>.
1. Let _SortCompare_ be a new Abstract Closure with parameters (_x_, _y_) that captures _comparefn_ and _buffer_ and performs the following steps when called:
1. Return ? CompareTypedArrayElements(_x_, _y_, _comparefn_, _buffer_).
1. Let _sortedList_ be ? SortIndexedProperties(_obj_, _len_, _SortCompare_, *false*).
1. Let _j_ be 0.
1. For each element _E_ of _items_, do
1. Let _Pj_ be ! ToString(𝔽(_j_)).
1. Perform ! Set(_A_, _Pj_, _E_, *true*).
1. Set _j_ to _j_ + 1.
1. Repeat, while _j_ &lt; _len_,
1. Perform ! Set(_A_, ! ToString(𝔽(_j_)), _sortedList_[_j_], *true*).
1. Set _j_ to _j_ + 1.
1. Return _A_.
</emu-alg>
<p>The following version of SortCompare is used by %TypedArray%`.prototype.toSorted`.</p>
<p>The abstract operation TypedArraySortCompare performs the following steps when called:</p>
<emu-alg>
1. Assert: Both Type(_x_) and Type(_y_) are Number or both are BigInt.
1. If _comparefn_ is not *undefined*, then
1. Let _v_ be ? ToNumber(? Call(_comparefn_, *undefined*, &laquo; _x_, _y_ &raquo;)).
1. If IsDetachedBuffer(_buffer_) is *true*, throw a *TypeError* exception.
1. If _v_ is *NaN*, return *+0*<sub>𝔽</sub>.
1. Return _v_.
1. If _x_ and _y_ are both *NaN*, return *+0*<sub>𝔽</sub>.
1. If _x_ is *NaN*, return *1*<sub>𝔽</sub>.
1. If _y_ is *NaN*, return *-1*<sub>𝔽</sub>.
1. If _x_ &lt; _y_, return *-1*<sub>𝔽</sub>.
1. If _x_ &gt; _y_, return *1*<sub>𝔽</sub>.
1. If _x_ is *-0*<sub>𝔽</sub> and _y_ is *+0*<sub>𝔽</sub>, return *-1*<sub>𝔽</sub>.
1. If _x_ is *+0*<sub>𝔽</sub> and _y_ is *-0*<sub>𝔽</sub>, return *1*<sub>𝔽</sub>.
1. Return *+0*<sub>𝔽</sub>.
</emu-alg>
</emu-clause>

<emu-clause id="sec-%typedarray%.prototype.toSpliced">
Expand Down