Editorial: Tweak format of *.prototype.sort #1612

jmdyck · 2019-07-06T21:49:09Z

The main point was to to make a single <emu-alg> out of some of the bits and pieces that currently define Array.prototype.sort.

Also, %TypedArray%.prototype.sort's version of SortCompare should have its own name and sub-clause, so I did that.

... and set _objIsSparse_ metavariable.

Formerly, there were 5 spots where we said: "the sort order is [also] implementation-defined" Instead, collect the various conditions into a single list. And rather than a <ul>, make it an ecmarkdown bullet-list, which can then be merged with the "entry steps" and the "following steps" into a single <emu-alg>. (Note that this is basically just a change in formatting -- there is no change to the normative requirements.)

... to a more logical spot in the list (in Array.p.sort).

(Give %TypedArray%.prototype.sort's version of SortCompare its own name and sub-clause.)

ljharb · 2019-07-07T05:00:20Z

spec.html

@@ -32711,6 +32674,18 @@ <h1>Array.prototype.sort ( _comparefn_ )</h1>
          <p>The `sort` function is intentionally generic; it does not require that its *this* value be an Array object. Therefore, it can be transferred to other kinds of objects for use as a method.</p>
        </emu-note>

+        <emu-clause id="sec-objectissparse" aoid="ObjectIsSparse">
+          <h1>Runtime Semantics: ObjectIsSparse ( _obj_, _len_ )</h1>


this seems more like it should be "ArraylikeIsSparse"?

Maybe.

The spec doesn't actually define "array-like", and I don't think I want to contribute to the suggestion that it does. It'd be different if we were to define it...

Based on the evidence (in CreateListFromArrayLike and the preamble of ToLength), it seems like the only requirement on an array-like object is that it have a "length" property whose value is (or can be converted by various means to) an integer in the range [0, 2^53 - 1]. Or more specifically, that ToLength(? Get(obj, "length"))
return an integer rather than an abrupt completion. (There's also an expectation that it will have some integer index properties, though that presumably isn't a requirement, i.e. you can have an 'empty' array-like.)

Assuming that's the definition, the IsSparse operation doesn't require that its _obj_ be array-like (since it doesn't care where _len_ came from), but on the other hand, the spec will only call the operation with an array-like object. Maybe the operation should only have the _obj_ parameter, and 'recalculate' _len_. That would make it more self-contained and more deserving of "ArrayLike" in the name.

Maybe there should be a GetLengthOfArrayLike operation, and maybe that should be used wherever an array-like is required. (E.g., that would help convey the generic-ness of Array.prototype.*.) That (and defining "array-like") sounds like a separate PR though.

I’d consider it already defined based on the abstract operation you mentioned, just not explicitly.

ljharb · 2019-07-07T05:01:05Z

spec.html

-        </ul>
-        <p>The following steps are taken:</p>
-        <emu-alg>
+          1. Let _objIsSparse_ be ObjectIsSparse(_obj_, _len_).


Suggested change

1. Let _objIsSparse_ be ObjectIsSparse(_obj_, _len_).

1. Let _objIsSparse_ be ? ObjectIsSparse(_obj_, _len_).

ljharb · 2019-07-07T05:01:17Z

spec.html

-        <p>The following steps are taken:</p>
-        <emu-alg>
+          1. Let _objIsSparse_ be ObjectIsSparse(_obj_, _len_).
+          1. Let _proto_ be _obj_.[[GetPrototypeOf]]().


? or !?

?, presumably. I don't think there's anything here to guarantee a normal return.

ljharb · 2019-07-07T05:01:54Z

spec.html

+          1. Let _sortOrderIsImplementationDefined_ be *true* if any of the following conditions are satisfied, and let it be *false* otherwise:
+            * _objIsSparse_ is *true* and _proto_ is not *null* and there exists a nonnegative integer _j_ less than _len_ such that HasProperty(_proto_, ToString(_j_)) is *true*.
+            * _objIsSparse_ is *true* and IsExtensible(_obj_) is *false*.
+            * _objIsSparse_ is *true* and any integer index property of _obj_ whose name is a nonnegative integer less than _len_ is a data property whose [[Configurable]] attribute is *false*.


this seems like it needs to be specified in terms of a MOP operation

It's the same wording as the status quo, so it seems like it shouldn't be this PR's problem to resolve.

But yeah, I think writing it properly would involve _obj_.[[GetOwnProperty]]

ljharb · 2019-07-07T05:01:59Z

spec.html

+          1. Let _proto_ be _obj_.[[GetPrototypeOf]]().
+          1. Let _sortOrderIsImplementationDefined_ be *true* if any of the following conditions are satisfied, and let it be *false* otherwise:
+            * _objIsSparse_ is *true* and _proto_ is not *null* and there exists a nonnegative integer _j_ less than _len_ such that HasProperty(_proto_, ToString(_j_)) is *true*.
+            * _objIsSparse_ is *true* and IsExtensible(_obj_) is *false*.


Suggested change

* _objIsSparse_ is *true* and IsExtensible(_obj_) is *false*.

* _objIsSparse_ is *true* and ! IsExtensible(_obj_) is *false*.

Hm, I would have thought ?.

if it can indeed return abruptly here then use ?, without the explicit notation its harder to be sure :-)

ljharb · 2019-07-07T05:02:12Z

spec.html

+          1. Let _objIsSparse_ be ObjectIsSparse(_obj_, _len_).
+          1. Let _proto_ be _obj_.[[GetPrototypeOf]]().
+          1. Let _sortOrderIsImplementationDefined_ be *true* if any of the following conditions are satisfied, and let it be *false* otherwise:
+            * _objIsSparse_ is *true* and _proto_ is not *null* and there exists a nonnegative integer _j_ less than _len_ such that HasProperty(_proto_, ToString(_j_)) is *true*.


Suggested change

* _objIsSparse_ is *true* and _proto_ is not *null* and there exists a nonnegative integer _j_ less than _len_ such that HasProperty(_proto_, ToString(_j_)) is *true*.

* _objIsSparse_ is *true* and _proto_ is not *null* and there exists a nonnegative integer _j_ less than _len_ such that ! HasProperty(_proto_, ! ToString(_j_)) is *true*.

Yes to the ! for ToString. As for HasProperty, I think that has to be ?.

But now I'm wondering: with the status quo, if HasProperty (or IsExtensible, or various others) returns an abrupt completion, does that cause sort to return an abrupt completion, or does that merely cause the condition in question to not be satisfied, which then only affects whether the sort order is implementation-defined? (Note that these conditions are outside the "Perform a sequence of calls" step, so its point about abrupt completions doesn't apply.)

ljharb · 2019-07-07T05:02:35Z

spec.html

+            * _objIsSparse_ is *true* and IsExtensible(_obj_) is *false*.
+            * _objIsSparse_ is *true* and any integer index property of _obj_ whose name is a nonnegative integer less than _len_ is a data property whose [[Configurable]] attribute is *false*.
+            * _obj_ is an exotic object (including Proxy exotic objects) whose behaviour for [[Get]], [[Set]], [[Delete]], and [[GetOwnProperty]] is not the ordinary object implementation of these internal methods.
+            * any index property of _obj_ whose name is a nonnegative integer less than _len_ is an accessor property or is a data property whose [[Writable]] attribute is *false*.


can this be written in terms of abstract operations?

If you mean current abstract operations, I don't think so. If you mean, could we create an abstract operation for this purpose, then of course.

ljharb · 2019-07-07T05:03:18Z

spec.html

+            1. Assert: _obj_ is an Object.
+            1. Assert: _len_ is a nonnegative integer.
+            1. For each integer _i_ in the range 0 &le; _i_ &lt; _len_, do
+              1. Let _elem_ be _obj_.[[GetOwnProperty]](! ToString(_i_)).


Suggested change

1. Let _elem_ be _obj_.[[GetOwnProperty]](! ToString(_i_)).

1. Let _elem_ be ? _obj_.[[GetOwnProperty]](! ToString(_i_)).

because i believe this can throw on, for example, an uninitialized module namespace object?

A module namespace object probably doesn't have a "length" property, so it wouldn't get this far. But in general, yes, [[GetOwnProperty]] could throw.

ljharb · 2019-07-07T05:04:22Z

spec.html

+          <h1>Runtime Semantics: TypedArraySortCompare ( _x_, _y_ )</h1>
+          <p>The TypedArraySortCompare abstract operation is called with two arguments _x_ and _y_. It also has access to the _comparefn_ and _buffer_ values of the current invocation of the %TypedArray%`.prototype.sort` method. It performs a numeric comparison rather than the string comparison used in <emu-xref href="#sec-array.prototype.sort"></emu-xref>. The following steps are taken:</p>
+          <emu-alg>
+            1. Assert: Both Type(_x_) and Type(_y_) is Number.


Suggested change

1. Assert: Both Type(_x_) and Type(_y_) is Number.

1. Assert: both Type(_x_) and Type(_y_) are Number.

(Note that the <emu-alg> text is the same as in the current spec -- github is only showing it as different because I had to change the indentation.)

re lowercase 'b': The spec is pretty consistent in capitalizing the first word after "Assert:".

re "is" to "are": That would be better, but I think I'd prefer Assert: Type(_x_) is Number and Type(_y_) is Number

Sounds good on both points.

ljharb · 2019-07-07T05:04:47Z

spec.html

+            1. Assert: Both Type(_x_) and Type(_y_) is Number.
+            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.


Suggested change

1. If IsDetachedBuffer(_buffer_) is *true*, throw a *TypeError* exception.

1. If ! IsDetachedBuffer(_buffer_) is *true*, throw a *TypeError* exception.

If you insist.

mathiasbynens · 2019-07-08T06:50:20Z

(Note that #1585 is up for discussion at this month's TC39 meeting. Depending on the outcome, we may or may not have a merge conflict.)

jmdyck · 2019-07-08T13:16:51Z

Ah, should have checked the agenda. So we might as well pause this PR until that's decided.

(Salvaged from PR tc39#1612.)

jmdyck · 2021-02-10T15:58:16Z

The merge of PR #1585 rendered most of this PR unnecessary. The one bit that still makes sense is in the last commit, which I've extracted into PR #2302.

(Salvaged from PR tc39#1612.)

jmdyck added 4 commits July 6, 2019 16:55

Editorial: Factor out ObjectIsSparse operation

19992b6

... and set _objIsSparse_ metavariable.

Editorial: Move two sort order conditions

d04a126

... to a more logical spot in the list (in Array.p.sort).

Editorial: introduce TypedArraySortCompare

1e2638b

(Give %TypedArray%.prototype.sort's version of SortCompare its own name and sub-clause.)

jmdyck added the editorial change label Jul 6, 2019

ljharb reviewed Jul 7, 2019

View reviewed changes

ljharb requested a review from mathiasbynens July 7, 2019 05:05

jmdyck mentioned this pull request Jul 9, 2019

Editorial: Define + use LengthOfArrayLike operation #1616

Merged

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Feb 10, 2021

Editorial: Give TypedArraySortCompare its own sub-clause

31d9ccd

(Salvaged from PR tc39#1612.)

jmdyck mentioned this pull request Feb 10, 2021

Editorial: Give TypedArraySortCompare its own sub-clause #2302

Merged

jmdyck closed this Feb 10, 2021

jmdyck deleted the sort_algo branch February 10, 2021 15:58

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Jul 18, 2021

Editorial: Give TypedArraySortCompare its own sub-clause

bb18dcc

(Salvaged from PR tc39#1612.)

ljharb pushed a commit to jmdyck/ecma262 that referenced this pull request Jan 5, 2022

Editorial: Give TypedArraySortCompare its own sub-clause (tc39#2302)

93183b8

(Salvaged from PR tc39#1612.)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editorial: Tweak format of *.prototype.sort #1612

Editorial: Tweak format of *.prototype.sort #1612

jmdyck commented Jul 6, 2019

ljharb Jul 7, 2019

jmdyck Jul 7, 2019

ljharb Jul 7, 2019

ljharb Jul 7, 2019

jmdyck Jul 7, 2019

ljharb Jul 7, 2019

jmdyck Jul 7, 2019

ljharb Jul 7, 2019

jmdyck Jul 7, 2019

ljharb Jul 7, 2019

jmdyck Jul 7, 2019

ljharb Jul 8, 2019

ljharb Jul 7, 2019

jmdyck Jul 7, 2019

ljharb Jul 7, 2019

jmdyck Jul 7, 2019

ljharb Jul 7, 2019

jmdyck Jul 7, 2019

ljharb Jul 7, 2019

jmdyck Jul 7, 2019

ljharb Jul 7, 2019

ljharb Jul 7, 2019

jmdyck Jul 7, 2019

mathiasbynens commented Jul 8, 2019

jmdyck commented Jul 8, 2019

jmdyck commented Feb 10, 2021

	1. Let _objIsSparse_ be ObjectIsSparse(_obj_, _len_).
	1. Let _objIsSparse_ be ? ObjectIsSparse(_obj_, _len_).

	* _objIsSparse_ is true and IsExtensible(_obj_) is false.
	* _objIsSparse_ is true and ! IsExtensible(_obj_) is false.

	* _objIsSparse_ is true and _proto_ is not null and there exists a nonnegative integer _j_ less than _len_ such that HasProperty(_proto_, ToString(_j_)) is true.
	* _objIsSparse_ is true and _proto_ is not null and there exists a nonnegative integer _j_ less than _len_ such that ! HasProperty(_proto_, ! ToString(_j_)) is true.

	1. Let _elem_ be _obj_.[[GetOwnProperty]](! ToString(_i_)).
	1. Let _elem_ be ? _obj_.[[GetOwnProperty]](! ToString(_i_)).

	1. Assert: Both Type(_x_) and Type(_y_) is Number.
	1. Assert: both Type(_x_) and Type(_y_) are Number.

	1. If IsDetachedBuffer(_buffer_) is true, throw a TypeError exception.
	1. If ! IsDetachedBuffer(_buffer_) is true, throw a TypeError exception.

Editorial: Tweak format of *.prototype.sort #1612

Editorial: Tweak format of *.prototype.sort #1612

Conversation

jmdyck commented Jul 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathiasbynens commented Jul 8, 2019

jmdyck commented Jul 8, 2019

jmdyck commented Feb 10, 2021