Update sorting methods to use abstract closures #78

acutmore · 2022-03-13T23:17:57Z

Updating toSorted now that tc39/ecma262#2305 has merged upstream.

Replaces #69 and fixes #51

acutmore · 2022-03-13T23:19:00Z

spec.html

@@ -18,6 +18,102 @@ <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">


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.

acutmore · 2022-03-13T23:20:41Z

spec.html

+                </emu-alg>
+            </emu-clause>
+
+            <emu-clause id="sec-sortindexedproperties" type="abstract operation">


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.

acutmore · 2022-03-13T23:21:42Z

spec.html

@@ -197,6 +282,68 @@ <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">


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

spec.html

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

…g input

spec.html

jridgewell · 2022-03-15T00:26:56Z

spec.html

+                  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>


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

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

jridgewell · 2022-03-15T00:28:54Z

spec.html

+                        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.


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

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]

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?

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.

acutmore · 2022-03-15T13:04:58Z

@ljharb @nicolo-ribaudo after you approved I renamed the ignoreHoles param to skipHoles and flipped the true/false logic, as this felt more intuitive when I was writing a note that referenced it.

skipHoles === true -> when there is a hole, skip that index so it won't be in the returned sorted List
skipHoles === false -> do not check for holes, read all indexes < length and append the value to the List that is sorted

ljharb · 2022-03-15T16:24:46Z

@acutmore i'm not sure that feels cleaner to me; it's not a huge deal because this isn't actual code, but all the existing callsites obviously omit this boolean, and will now be passing true instead of false. In a real API, you'd want omitting and false to always have the same behavior, ideally.

acutmore · 2022-03-15T16:40:45Z

@acutmore i'm not sure that feels cleaner to me; it's not a huge deal because this isn't actual code, but all the existing callsites obviously omit this boolean, and will now be passing true instead of false. In a real API, you'd want omitting and false to always have the same behavior, ideally.

Ah, as in this is like a breaking change for implementations that match the spec exactly. Yes I see that. My theory is that this AO was only added last week so there wouldn't be that many places that implement and call it yet.

ljharb · 2022-03-15T19:02:12Z

@acutmore sure, but it'll be in ES2022, and this proposal won't be until at least ES2023 :-) (obv this isn't going to be a motivation for most people, but es-abstract will thus be shipping the current version as 2022, and the breaking one as 202X)

acutmore · 2022-03-15T23:53:30Z

@ljharb good point that the change would cross versions! I have also changed the return type of the AO so I think call sites would likely need to be updated regardless?

Im happy to flip the true/false logic back, was more about using the word “exclude” instead of “ignore”, as I kept forgetting if it was the hole that was being ignored (e.g read and allow prototype lookup) or indexes with holes being ignored (not reading when the property is absent).

ljharb · 2022-03-16T03:56:16Z

@acutmore true enough :-) it’s not a strong argument.

I don’t particularly care about the naming, but i lightly prefer the default/false to mean “what sort does”.

acutmore · 2022-03-17T10:53:45Z

As it's only a light preference I'll stick with what we've got for now and merge this PR 😀

update sorting methods to use abstract closures

32d1e92

acutmore added the spec label Mar 13, 2022

acutmore added this to the Stage 3 milestone Mar 13, 2022

acutmore requested review from ljharb, jridgewell, rricard, chicoxyzzy and nicolo-ribaudo March 13, 2022 23:17

acutmore commented Mar 13, 2022

View reviewed changes

nicolo-ribaudo reviewed Mar 14, 2022

View reviewed changes

spec.html Outdated Show resolved Hide resolved

spec.html Outdated Show resolved Hide resolved

spec.html Outdated Show resolved Hide resolved

ljharb reviewed Mar 14, 2022

View reviewed changes

spec.html Outdated Show resolved Hide resolved

acutmore and others added 3 commits March 14, 2022 16:00

Apply suggestions from code review

a0c6748

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

[squash] rename AOs to Compare[Typed]ArrayElements

7e2a678

[squash] SortIndexProperties returns internal list instead of mutatin…

333f55d

…g input

acutmore requested review from ljharb, nicolo-ribaudo and bakkot March 14, 2022 17:59

ljharb reviewed Mar 14, 2022

View reviewed changes

spec.html Outdated Show resolved Hide resolved

spec.html Show resolved Hide resolved

[squash] type _comparefn_ param as function or undefined

17779e6

acutmore requested a review from ljharb March 14, 2022 21:45

acutmore mentioned this pull request Mar 14, 2022

Stage 3 reviews #74

Closed

7 tasks

ljharb reviewed Mar 14, 2022

View reviewed changes

spec.html Outdated Show resolved Hide resolved

[squash] checkHasProperty is now ignoreHoles

7c57a62

acutmore requested a review from ljharb March 14, 2022 22:42

ljharb approved these changes Mar 14, 2022

View reviewed changes

jridgewell reviewed Mar 15, 2022

View reviewed changes

acutmore mentioned this pull request Mar 15, 2022

Explicitly pass _comparefn_ as a parameter to SortCompare #69

Closed

nicolo-ribaudo approved these changes Mar 15, 2022

View reviewed changes

[squash] add note about holes and rename param to skipHoles

0152db2

jridgewell approved these changes Mar 15, 2022

View reviewed changes

acutmore merged commit 7588000 into tc39:main Mar 17, 2022

acutmore deleted the sorting-uses-abstract-closures branch March 17, 2022 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update sorting methods to use abstract closures #78

Update sorting methods to use abstract closures #78

acutmore commented Mar 13, 2022

acutmore Mar 13, 2022 •

edited

Loading

acutmore Mar 13, 2022

acutmore Mar 13, 2022

jridgewell Mar 15, 2022

acutmore Mar 15, 2022

jridgewell Mar 15, 2022

bakkot Mar 15, 2022 •

edited

Loading

acutmore Mar 15, 2022 •

edited

Loading

bakkot Mar 15, 2022

acutmore commented Mar 15, 2022

ljharb commented Mar 15, 2022

acutmore commented Mar 15, 2022

ljharb commented Mar 15, 2022

acutmore commented Mar 15, 2022

ljharb commented Mar 16, 2022

acutmore commented Mar 17, 2022

Update sorting methods to use abstract closures #78

Update sorting methods to use abstract closures #78

Conversation

acutmore commented Mar 13, 2022

acutmore Mar 13, 2022 • edited Loading

Choose a reason for hiding this comment

acutmore Mar 13, 2022

Choose a reason for hiding this comment

acutmore Mar 13, 2022

Choose a reason for hiding this comment

jridgewell Mar 15, 2022

Choose a reason for hiding this comment

acutmore Mar 15, 2022

Choose a reason for hiding this comment

jridgewell Mar 15, 2022

Choose a reason for hiding this comment

bakkot Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

acutmore Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

bakkot Mar 15, 2022

Choose a reason for hiding this comment

acutmore commented Mar 15, 2022

ljharb commented Mar 15, 2022

acutmore commented Mar 15, 2022

ljharb commented Mar 15, 2022

acutmore commented Mar 15, 2022

ljharb commented Mar 16, 2022

acutmore commented Mar 17, 2022

acutmore Mar 13, 2022 •

edited

Loading

bakkot Mar 15, 2022 •

edited

Loading

acutmore Mar 15, 2022 •

edited

Loading