make %TypedArray%.prototype.sort stricter

zloirock · Jun 5, 2021 · 586d578 · 586d578
1 parent 07ca565
commit 586d578
Show file tree

Hide file tree

Showing 6 changed files with 141 additions and 102 deletions.
diff --git a/packages/core-js/internals/array-sort.js b/packages/core-js/internals/array-sort.js
@@ -1,65 +1,6 @@
 'use strict';
-var aFunction = require('../internals/a-function');
-var toObject = require('../internals/to-object');
-var toLength = require('../internals/to-length');
-var fails = require('../internals/fails');
-var arrayMethodIsStrict = require('../internals/array-method-is-strict');
-var FF = require('../internals/engine-ff-version');
-var IE_OR_EDGE = require('../internals/engine-is-ie-or-edge');
-var V8 = require('../internals/engine-v8-version');
-var WEBKIT = require('../internals/engine-webkit-version');
-
-var test = [];
-var nativeSort = test.sort;
 var floor = Math.floor;
 
-// IE8-
-var FAILS_ON_UNDEFINED = fails(function () {
-  test.sort(undefined);
-});
-// V8 bug
-var FAILS_ON_NULL = fails(function () {
-  test.sort(null);
-});
-// Old WebKit
-var STRICT_METHOD = arrayMethodIsStrict('sort');
-
-var STABLE_SORT = !fails(function () {
-  // feature detection can be too slow, so check engines versions
-  if (V8) return V8 < 70;
-  if (FF && FF > 3) return;
-  if (IE_OR_EDGE) return true;
-  if (WEBKIT) return WEBKIT < 603;
-
-  var result = '';
-  var code, chr, value, index;
-
-  // generate an array with more 512 elements (Chakra and old V8 fails only in this case)
-  for (code = 65; code < 76; code++) {
-    chr = String.fromCharCode(code);
-    switch (code) {
-      case 66: case 69: case 70: case 72: value = 3; break;
-      case 68: case 71: value = 4; break;
-      default: value = 2;
-    }
-
-    for (index = 0; index < 47; index++) {
-      test.push({ k: chr + index, v: value });
-    }
-  }
-
-  test.sort(function (a, b) { return b.v - a.v; });
-
-  for (index = 0; index < test.length; index++) {
-    chr = test[index].k.charAt(0);
-    if (result.charAt(result.length - 1) !== chr) result += chr;
-  }
-
-  return result !== 'DGBEFHACIJK';
-});
-
-var FORCED = FAILS_ON_UNDEFINED || !FAILS_ON_NULL || !STRICT_METHOD || !STABLE_SORT;
-
 var mergeSort = function (array, comparefn) {
   var length = array.length;
   var middle = floor(length / 2);
@@ -78,7 +19,7 @@ var insertionSort = function (array, comparefn) {
   while (i < length) {
     j = i;
     element = array[i];
-    while (j && sortCompare(array[j - 1], element, comparefn) > 0) {
+    while (j && comparefn(array[j - 1], element) > 0) {
       array[j] = array[--j];
     }
     if (j !== i++) array[j] = element;
@@ -94,45 +35,11 @@ var merge = function (left, right, comparefn) {
 
   while (lindex < llength || rindex < rlength) {
     if (lindex < llength && rindex < rlength) {
-      result.push(sortCompare(left[lindex], right[rindex], comparefn) <= 0 ? left[lindex++] : right[rindex++]);
+      result.push(comparefn(left[lindex], right[rindex]) <= 0 ? left[lindex++] : right[rindex++]);
     } else {
       result.push(lindex < llength ? left[lindex++] : right[rindex++]);
     }
   } return result;
 };
 
-var sortCompare = function (x, y, comparefn) {
-  if (y === undefined) return -1;
-  if (x === undefined) return 1;
-  if (comparefn !== undefined) {
-    return +comparefn(x, y) || 0;
-  } return String(x) > String(y) ? 1 : -1;
-};
-
-// `Array.prototype.sort` method
-// https://tc39.es/ecma262/#sec-array.prototype.sort
-module.exports = FORCED ? function sort(comparefn) {
-  if (comparefn !== undefined) aFunction(comparefn);
-
-  var array = toObject(this);
-
-  if (STABLE_SORT) return comparefn === undefined ? nativeSort.call(array) : nativeSort.call(array, comparefn);
-
-  var items = [];
-  var arrayLength = toLength(array.length);
-  var itemsLength, index;
-
-  for (index = 0; index < arrayLength; index++) {
-    if (index in array) items.push(array[index]);
-  }
-
-  // TODO: use something more complex like timsort?
-  items = mergeSort(items, comparefn);
-  itemsLength = items.length;
-  index = 0;
-
-  while (index < itemsLength) array[index] = items[index++];
-  while (index < arrayLength) delete array[index++];
-
-  return array;
-} : nativeSort;
+module.exports = mergeSort;
diff --git a/packages/core-js/modules/es.array.sort.js b/packages/core-js/modules/es.array.sort.js
@@ -1,9 +1,102 @@
 'use strict';
 var $ = require('../internals/export');
-var sort = require('../internals/array-sort');
+var aFunction = require('../internals/a-function');
+var toObject = require('../internals/to-object');
+var toLength = require('../internals/to-length');
+var fails = require('../internals/fails');
+var internalSort = require('../internals/array-sort');
+var arrayMethodIsStrict = require('../internals/array-method-is-strict');
+var FF = require('../internals/engine-ff-version');
+var IE_OR_EDGE = require('../internals/engine-is-ie-or-edge');
+var V8 = require('../internals/engine-v8-version');
+var WEBKIT = require('../internals/engine-webkit-version');
+
+var test = [];
+var nativeSort = test.sort;
+
+// IE8-
+var FAILS_ON_UNDEFINED = fails(function () {
+  test.sort(undefined);
+});
+// V8 bug
+var FAILS_ON_NULL = fails(function () {
+  test.sort(null);
+});
+// Old WebKit
+var STRICT_METHOD = arrayMethodIsStrict('sort');
+
+var STABLE_SORT = !fails(function () {
+  // feature detection can be too slow, so check engines versions
+  if (V8) return V8 < 70;
+  if (FF && FF > 3) return;
+  if (IE_OR_EDGE) return true;
+  if (WEBKIT) return WEBKIT < 603;
+
+  var result = '';
+  var code, chr, value, index;
+
+  // generate an array with more 512 elements (Chakra and old V8 fails only in this case)
+  for (code = 65; code < 76; code++) {
+    chr = String.fromCharCode(code);
+
+    switch (code) {
+      case 66: case 69: case 70: case 72: value = 3; break;
+      case 68: case 71: value = 4; break;
+      default: value = 2;
+    }
+
+    for (index = 0; index < 47; index++) {
+      test.push({ k: chr + index, v: value });
+    }
+  }
+
+  test.sort(function (a, b) { return b.v - a.v; });
+
+  for (index = 0; index < test.length; index++) {
+    chr = test[index].k.charAt(0);
+    if (result.charAt(result.length - 1) !== chr) result += chr;
+  }
+
+  return result !== 'DGBEFHACIJK';
+});
+
+var FORCED = FAILS_ON_UNDEFINED || !FAILS_ON_NULL || !STRICT_METHOD || !STABLE_SORT;
+
+var getSortCompare = function (comparefn) {
+  return function (x, y) {
+    if (y === undefined) return -1;
+    if (x === undefined) return 1;
+    if (comparefn !== undefined) return +comparefn(x, y) || 0;
+    return String(x) > String(y) ? 1 : -1;
+  };
+};
 
 // `Array.prototype.sort` method
 // https://tc39.es/ecma262/#sec-array.prototype.sort
-$({ target: 'Array', proto: true, forced: [].sort !== sort }, {
-  sort: sort
+$({ target: 'Array', proto: true, forced: FORCED }, {
+  sort: function sort(comparefn) {
+    if (comparefn !== undefined) aFunction(comparefn);
+
+    var array = toObject(this);
+
+    if (STABLE_SORT) return comparefn === undefined ? nativeSort.call(array) : nativeSort.call(array, comparefn);
+
+    var items = [];
+    var arrayLength = toLength(array.length);
+    var itemsLength, index;
+
+    for (index = 0; index < arrayLength; index++) {
+      if (index in array) items.push(array[index]);
+    }
+
+    // TODO: use something more complex like timsort?
+    items = internalSort(items, getSortCompare(comparefn));
+    itemsLength = items.length;
+    index = 0;
+
+    while (index < itemsLength) array[index] = items[index++];
+    while (index < arrayLength) delete array[index++];
+
+    return array;
+  }
 });
diff --git a/packages/core-js/modules/es.typed-array.sort.js b/packages/core-js/modules/es.typed-array.sort.js
@@ -3,7 +3,8 @@ var ArrayBufferViewCore = require('../internals/array-buffer-view-core');
 var global = require('../internals/global');
 var fails = require('../internals/fails');
 var aFunction = require('../internals/a-function');
-var arraySort = require('../internals/array-sort');
+var toLength = require('../internals/to-length');
+var internalSort = require('../internals/array-sort');
 var FF = require('../internals/engine-ff-version');
 var IE_OR_EDGE = require('../internals/engine-is-ie-or-edge');
 var V8 = require('../internals/engine-v8-version');
@@ -47,10 +48,39 @@ var STABLE_SORT = !!nativeSort && !fails(function () {
   }
 });
 
+var getSortCompare = function (comparefn) {
+  return function (x, y) {
+    if (comparefn !== undefined) return +comparefn(x, y) || 0;
+    // eslint-disable-next-line no-self-compare -- NaN check
+    if (y !== y) return -1;
+    // eslint-disable-next-line no-self-compare -- NaN check
+    if (x !== x) return 1;
+    if (x === 0 && y === 0) return 1 / x > 0 && 1 / y < 0 ? 1 : -1;
+    return x > y;
+  };
+};
+
 // `%TypedArray%.prototype.sort` method
 // https://tc39.es/ecma262/#sec-%typedarray%.prototype.sort
 exportTypedArrayMethod('sort', function sort(comparefn) {
-  if (!STABLE_SORT) return arraySort.call(aTypedArray(this), comparefn);
+  var array = this;
   if (comparefn !== undefined) aFunction(comparefn);
-  return nativeSort.call(this, comparefn);
+  if (STABLE_SORT) return nativeSort.call(array, comparefn);
+
+  aTypedArray(array);
+  var arrayLength = toLength(array.length);
+  var items = Array(arrayLength);
+  var index;
+
+  for (index = 0; index < arrayLength; index++) {
+    items[index] = array[index];
+  }
+
+  items = internalSort(array, getSortCompare(comparefn));
+
+  for (index = 0; index < arrayLength; index++) {
+    array[index] = items[index];
+  }
+
+  return array;
 }, !STABLE_SORT || ACCEPT_INCORRECT_ARGUMENTS);
diff --git a/tests/pure/es.array.sort.js b/tests/pure/es.array.sort.js
@@ -80,6 +80,8 @@ QUnit.test('Array#sort', assert => {
 
   sort(array, (a, b) => (a / 4 | 0) - (b / 4 | 0));
 
+  assert.ok(1 / sort([0, -0])[0] > 0, '-0');
+
   assert.same(String(array), String(expected), 'stable #1');
 
   let result = '';

diff --git a/tests/tests/es.array.sort.js b/tests/tests/es.array.sort.js
@@ -80,6 +80,8 @@ QUnit.test('Array#sort', assert => {
 
   array.sort((a, b) => (a / 4 | 0) - (b / 4 | 0));
 
+  assert.ok(1 / [0, -0].sort()[0] > 0, '-0');
+
   assert.same(String(array), String(expected), 'stable #1');
 
   let result = '';

diff --git a/tests/tests/es.typed-array.sort.js b/tests/tests/es.typed-array.sort.js
@@ -10,6 +10,11 @@ if (DESCRIPTORS) QUnit.test('%TypedArrayPrototype%.sort', assert => {
     assert.name(sort, 'sort', `${ name }::sort name is 'sort'`);
     assert.looksNative(sort, `${ name }::sort looks native`);
 
+    if (name.indexOf('Float') === 0) {
+      assert.ok(1 / new TypedArray([0, -0]).sort()[0] < 0, '-0');
+      assert.deepEqual(new TypedArray([NaN, 1, NaN]).sort(), new TypedArray([1, NaN, NaN]), 'NaN');
+    }
+
     if (name.indexOf('8') === -1) {
       const expected = Array(516);
       let array = new TypedArray(516);