Stable {Array,TypedArray}.prototype.sort

Summary: Make {Array,TypedArray}.prototype.sort stable, by keeping a parallel array of the original indices to break ties. This is a repeat of D23088088, but with proper bounds checking of the index array and added unit tests from D30662659.

Reviewed By: tmikov

Differential Revision: D31058489

fbshipit-source-id: 990c5ec232bd96d793ed490f5f1e0f25dd07dbc8

lib/VM/JSLib/Sorting.h → include/hermes/VM/JSLib/Sorting.h

@@ -32,7 +32,8 @@ class SortModel {
   virtual ExecutionStatus swap(uint32_t a, uint32_t b) = 0;
 
   // Compare elements at index a and at index b.
-  virtual CallResult<bool> less(uint32_t a, uint32_t b) = 0;
+  // Return negative if [a] < [b], positive if [a] > [b], 0 if [a] = [b]
+  virtual CallResult<int> compare(uint32_t a, uint32_t b) = 0;
 
   virtual ~SortModel() = 0;
 };

lib/VM/JSLib/Array.cpp

@@ -10,9 +10,9 @@
 /// ES5.1 15.4 Initialize the Array constructor.
 //===----------------------------------------------------------------------===//
 #include "JSLibInternal.h"
-#include "Sorting.h"
 
 #include "hermes/VM/HandleRootOwner-inline.h"
+#include "hermes/VM/JSLib/Sorting.h"
 #include "hermes/VM/Operations.h"
 #include "hermes/VM/StringBuilder.h"
 #include "hermes/VM/StringRefUtils.h"
@@ -1112,9 +1112,10 @@ class StandardSortModel : public SortModel {
     return ExecutionStatus::RETURNED;
   }
 
-  /// If compareFn isn't null, return compareFn(obj[a], obj[b]) < 0.
-  /// If compareFn is null, return obj[a] < obj[b].
-  CallResult<bool> less(uint32_t a, uint32_t b) override {
+  /// If compareFn isn't null, return compareFn(obj[a], obj[b])
+  /// If compareFn is null, return -1 if obj[a] < obj[b], 1 if obj[a] > obj[b],
+  /// 0 otherwise
+  CallResult<int> compare(uint32_t a, uint32_t b) override {
     // Ensure that we don't leave here with any new handles.
     GCScopeMarkerRAII gcMarker{gcScope_, gcMarker_};
 
@@ -1131,7 +1132,7 @@ class StandardSortModel : public SortModel {
     }
     if ((*propRes)->isEmpty()) {
       // Spec defines empty as greater than everything.
-      return false;
+      return 1;
     }
     aValue_ = std::move(*propRes);
     assert(!aValue_->isEmpty());
@@ -1150,18 +1151,18 @@ class StandardSortModel : public SortModel {
     }
     if ((*propRes)->isEmpty()) {
       // Spec defines empty as greater than everything.
-      return true;
+      return -1;
     }
     bValue_ = std::move(*propRes);
     assert(!bValue_->isEmpty());
 
     if (aValue_->isUndefined()) {
       // Spec defines undefined as greater than everything.
-      return false;
+      return 1;
     }
     if (bValue_->isUndefined()) {
       // Spec defines undefined as greater than everything.
-      return true;
+      return -1;
     }
 
     if (compareFn_) {
@@ -1180,9 +1181,11 @@ class StandardSortModel : public SortModel {
       if (LLVM_UNLIKELY(intRes == ExecutionStatus::EXCEPTION)) {
         return ExecutionStatus::EXCEPTION;
       }
-      return intRes->getNumber() < 0;
+      // Cannot return intRes's value directly because it can be NaN
+      auto res = intRes->getNumber();
+      return (res < 0) ? -1 : (res > 0 ? 1 : 0);
     } else {
-      // Convert both arguments to strings and use the lessOp on them.
+      // Convert both arguments to strings and compare
       auto aValueRes = toString_RJS(runtime_, aValue_);
       if (LLVM_UNLIKELY(aValueRes == ExecutionStatus::EXCEPTION)) {
         return ExecutionStatus::EXCEPTION;
@@ -1195,7 +1198,7 @@ class StandardSortModel : public SortModel {
       }
       bValue_ = bValueRes->getHermesValue();
 
-      return lessOp_RJS(runtime_, aValue_, bValue_).getValue();
+      return aValue_->getString()->compare(bValue_->getString());
     }
   }
 };