Sort import assertions by code point order of the keys

Per https://tc39.es/proposal-import-assertions/#sec-assert-clause-to-assertions,
import assertions should be sorted by the import assertion [[Key]]s,
in order to prevent hosts from relying on a changing order of the
assertions to determine behavior.

Prior to this change, the assertions were being sorted by pointer. With
this CL, the keys are sorted using a code point ordering so that the
order of the assertions received by the host will be stable and
non-surprising.

This CL also switches the SourceTextModuleDescriptor's ModuleRequestMap,
RegularExportMap, and RegularImportMap to use the code point order
comparison rather than their former shortlex sort.  This change will not
be externally visible, but it seems best to make these consistent.

In order to avoid #including the fairly large ast-value-factory.h
into ast/modules.h, I changed ImportAssertions into a separate class
definition rather than keeping it as a typedef.  The alternative would
be to define a common AstRawStringComparer in ast-value-factory.h and
then #include ast-value-factory.h in both ast/modules.h and
parsing/parser.h so that the ImportAssertions typedef would have a
full, shared definition of the AstRawStringComparer type.

Bug: v8:10958
Change-Id: I29c9544aa0a4340c56e1ee631be6cabb2a2eb921
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2533038
Commit-Queue: Dan Clark <daniec@microsoft.com>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71165}

BUILD.gn

@@ -3113,6 +3113,8 @@ v8_source_set("v8_base_without_compiler") {
     "src/parsing/expression-scope.h",
     "src/parsing/func-name-inferrer.cc",
     "src/parsing/func-name-inferrer.h",
+    "src/parsing/import-assertions.cc",
+    "src/parsing/import-assertions.h",
     "src/parsing/literal-buffer.cc",
     "src/parsing/literal-buffer.h",
     "src/parsing/parse-info.cc",

src/ast/ast-value-factory.cc

@@ -114,7 +114,7 @@ uint16_t AstRawString::FirstCharacter() const {
   return *c;
 }
 
-bool AstRawString::Compare(const AstRawString* lhs, const AstRawString* rhs) {
+bool AstRawString::Equal(const AstRawString* lhs, const AstRawString* rhs) {
   DCHECK_EQ(lhs->Hash(), rhs->Hash());
 
   if (lhs->length() != rhs->length()) return false;
@@ -145,6 +145,44 @@ bool AstRawString::Compare(const AstRawString* lhs, const AstRawString* rhs) {
   }
 }
 
+int AstRawString::Compare(const AstRawString* lhs, const AstRawString* rhs) {
+  // Fast path for equal pointers.
+  if (lhs == rhs) return 0;
+
+  const unsigned char* lhs_data = lhs->raw_data();
+  const unsigned char* rhs_data = rhs->raw_data();
+  size_t length = std::min(lhs->byte_length(), rhs->byte_length());
+
+  // Code point order by contents.
+  if (lhs->is_one_byte()) {
+    if (rhs->is_one_byte()) {
+      if (int result = CompareCharsUnsigned(
+              reinterpret_cast<const uint8_t*>(lhs_data),
+              reinterpret_cast<const uint8_t*>(rhs_data), length))
+        return result;
+    } else {
+      if (int result = CompareCharsUnsigned(
+              reinterpret_cast<const uint8_t*>(lhs_data),
+              reinterpret_cast<const uint16_t*>(rhs_data), length))
+        return result;
+    }
+  } else {
+    if (rhs->is_one_byte()) {
+      if (int result = CompareCharsUnsigned(
+              reinterpret_cast<const uint16_t*>(lhs_data),
+              reinterpret_cast<const uint8_t*>(rhs_data), length))
+        return result;
+    } else {
+      if (int result = CompareCharsUnsigned(
+              reinterpret_cast<const uint16_t*>(lhs_data),
+              reinterpret_cast<const uint16_t*>(rhs_data), length))
+        return result;
+    }
+  }
+
+  return lhs->byte_length() - rhs->byte_length();
+}
+
 template <typename LocalIsolate>
 Handle<String> AstConsString::Allocate(LocalIsolate* isolate) const {
   DCHECK(string_.is_null());

src/ast/ast-value-factory.h

@@ -48,7 +48,12 @@ class Isolate;
 
 class AstRawString final : public ZoneObject {
  public:
-  static bool Compare(const AstRawString* a, const AstRawString* b);
+  static bool Equal(const AstRawString* lhs, const AstRawString* rhs);
+
+  // Returns 0 if lhs is equal to rhs.
+  // Returns <0 if lhs is less than rhs in code point order.
+  // Returns >0 if lhs is greater than than rhs in code point order.
+  static int Compare(const AstRawString* lhs, const AstRawString* rhs);
 
   bool IsEmpty() const { return literal_bytes_.length() == 0; }
   int length() const {
@@ -210,7 +215,7 @@ struct AstRawStringMapMatcher {
   bool operator()(uint32_t hash1, uint32_t hash2,
                   const AstRawString* lookup_key,
                   const AstRawString* entry_key) const {
-    return hash1 == hash2 && AstRawString::Compare(lookup_key, entry_key);
+    return hash1 == hash2 && AstRawString::Equal(lookup_key, entry_key);
   }
 };
 

src/ast/modules.cc

@@ -16,44 +16,35 @@ namespace internal {
 
 bool SourceTextModuleDescriptor::AstRawStringComparer::operator()(
     const AstRawString* lhs, const AstRawString* rhs) const {
-  return ThreeWayCompare(lhs, rhs) < 0;
-}
-
-int SourceTextModuleDescriptor::AstRawStringComparer::ThreeWayCompare(
-    const AstRawString* lhs, const AstRawString* rhs) {
-  // Fast path for equal pointers: a pointer is not strictly less than itself.
-  if (lhs == rhs) return false;
-
-  // Order by contents (ordering by hash is unstable across runs).
-  if (lhs->is_one_byte() != rhs->is_one_byte()) {
-    return lhs->is_one_byte() ? -1 : 1;
-  }
-  if (lhs->byte_length() != rhs->byte_length()) {
-    return lhs->byte_length() - rhs->byte_length();
-  }
-  return memcmp(lhs->raw_data(), rhs->raw_data(), lhs->byte_length());
+  return AstRawString::Compare(lhs, rhs) < 0;
 }
 
 bool SourceTextModuleDescriptor::ModuleRequestComparer::operator()(
     const AstModuleRequest* lhs, const AstModuleRequest* rhs) const {
-  if (int specifier_comparison = AstRawStringComparer::ThreeWayCompare(
-          lhs->specifier(), rhs->specifier()))
+  if (int specifier_comparison =
+          AstRawString::Compare(lhs->specifier(), rhs->specifier())) {
     return specifier_comparison < 0;
-
-  if (lhs->import_assertions()->size() != rhs->import_assertions()->size())
-    return (lhs->import_assertions()->size() <
-            rhs->import_assertions()->size());
+  }
 
   auto lhsIt = lhs->import_assertions()->cbegin();
   auto rhsIt = rhs->import_assertions()->cbegin();
-  for (; lhsIt != lhs->import_assertions()->cend(); ++lhsIt, ++rhsIt) {
+  for (; lhsIt != lhs->import_assertions()->cend() &&
+         rhsIt != rhs->import_assertions()->cend();
+       ++lhsIt, ++rhsIt) {
     if (int assertion_key_comparison =
-            AstRawStringComparer::ThreeWayCompare(lhsIt->first, rhsIt->first))
+            AstRawString::Compare(lhsIt->first, rhsIt->first)) {
       return assertion_key_comparison < 0;
+    }
 
-    if (int assertion_value_comparison = AstRawStringComparer::ThreeWayCompare(
-            lhsIt->second.first, rhsIt->second.first))
+    if (int assertion_value_comparison =
+            AstRawString::Compare(lhsIt->second.first, rhsIt->second.first)) {
       return assertion_value_comparison < 0;
+    }
+  }
+
+  if (lhs->import_assertions()->size() != rhs->import_assertions()->size()) {
+    return (lhs->import_assertions()->size() <
+            rhs->import_assertions()->size());
   }
 
   return false;

src/ast/modules.h

@@ -5,6 +5,7 @@
 #ifndef V8_AST_MODULES_H_
 #define V8_AST_MODULES_H_
 
+#include "src/parsing/import-assertions.h"
 #include "src/parsing/scanner.h"  // Only for Scanner::Location.
 #include "src/zone/zone-containers.h"
 
@@ -13,6 +14,7 @@ namespace internal {
 
 
 class AstRawString;
+class AstRawStringComparer;
 class ModuleRequest;
 class SourceTextModuleInfo;
 class SourceTextModuleInfoEntry;
@@ -27,10 +29,6 @@ class SourceTextModuleDescriptor : public ZoneObject {
         regular_exports_(zone),
         regular_imports_(zone) {}
 
-  using ImportAssertions =
-      ZoneMap<const AstRawString*,
-              std::pair<const AstRawString*, Scanner::Location>>;
-
   // The following Add* methods are high-level convenience functions for use by
   // the parser.
 
@@ -161,8 +159,6 @@ class SourceTextModuleDescriptor : public ZoneObject {
   // across parses.
   struct V8_EXPORT_PRIVATE AstRawStringComparer {
     bool operator()(const AstRawString* lhs, const AstRawString* rhs) const;
-    static int ThreeWayCompare(const AstRawString* lhs,
-                               const AstRawString* rhs);
   };
 
   struct V8_EXPORT_PRIVATE ModuleRequestComparer {

src/parsing/import-assertions.cc

@@ -0,0 +1,18 @@
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/parsing/import-assertions.h"
+
+#include "src/ast/ast-value-factory.h"
+
+namespace v8 {
+namespace internal {
+
+bool ImportAssertionsKeyComparer::operator()(const AstRawString* lhs,
+                                             const AstRawString* rhs) const {
+  return AstRawString::Compare(lhs, rhs) < 0;
+}
+
+}  // namespace internal
+}  // namespace v8

src/parsing/import-assertions.h

@@ -0,0 +1,32 @@
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef V8_PARSING_IMPORT_ASSERTIONS_H_
+#define V8_PARSING_IMPORT_ASSERTIONS_H_
+
+#include "src/parsing/scanner.h"  // Only for Scanner::Location.
+#include "src/zone/zone-containers.h"
+
+namespace v8 {
+namespace internal {
+
+struct V8_EXPORT_PRIVATE ImportAssertionsKeyComparer {
+  bool operator()(const AstRawString* lhs, const AstRawString* rhs) const;
+};
+
+class ImportAssertions
+    : public ZoneMap<const AstRawString*,
+                     std::pair<const AstRawString*, Scanner::Location>,
+                     ImportAssertionsKeyComparer> {
+ public:
+  explicit ImportAssertions(Zone* zone)
+      : ZoneMap<const AstRawString*,
+                std::pair<const AstRawString*, Scanner::Location>,
+                ImportAssertionsKeyComparer>(zone) {}
+};
+
+}  // namespace internal
+}  // namespace v8
+
+#endif  // V8_PARSING_IMPORT_ASSERTIONS_H_

src/parsing/parser.cc

@@ -1210,7 +1210,7 @@ ZonePtrList<const Parser::NamedImport>* Parser::ParseNamedImports(int pos) {
   return result;
 }
 
-Parser::ImportAssertions* Parser::ParseImportAssertClause() {
+ImportAssertions* Parser::ParseImportAssertClause() {
   // AssertClause :
   //    assert '{' '}'
   //    assert '{' AssertEntries '}'

src/parsing/parser.h

@@ -14,6 +14,7 @@
 #include "src/base/compiler-specific.h"
 #include "src/base/threaded-list.h"
 #include "src/common/globals.h"
+#include "src/parsing/import-assertions.h"
 #include "src/parsing/parse-info.h"
 #include "src/parsing/parser-base.h"
 #include "src/parsing/parsing.h"
@@ -283,9 +284,7 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   };
   const AstRawString* ParseExportSpecifierName();
   ZonePtrList<const NamedImport>* ParseNamedImports(int pos);
-  using ImportAssertions =
-      ZoneMap<const AstRawString*,
-              std::pair<const AstRawString*, Scanner::Location>>;
+
   ImportAssertions* ParseImportAssertClause();
   Statement* BuildInitializationBlock(DeclarationParsingResult* parsing_result);
   Expression* RewriteReturn(Expression* return_value, int pos);

test/cctest/test-parsing.cc

@@ -8267,7 +8267,7 @@ TEST(ModuleParsingInternalsWithImportAssertions) {
   }
 }
 
-TEST(ModuleParsingImportAssertionOrdering) {
+TEST(ModuleParsingModuleRequestOrdering) {
   i::FLAG_harmony_import_assertions = true;
   i::Isolate* isolate = CcTest::i_isolate();
   i::Factory* factory = isolate->factory();
@@ -8339,9 +8339,15 @@ TEST(ModuleParsingImportAssertionOrdering) {
   CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("a"));
   ++request_iterator;
 
+  CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("aa"));
+  ++request_iterator;
+
   CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("b"));
   ++request_iterator;
 
+  CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("baaaaaar"));
+  ++request_iterator;
+
   CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("c"));
   ++request_iterator;
 
@@ -8356,6 +8362,9 @@ TEST(ModuleParsingImportAssertionOrdering) {
   CHECK_EQ(1, (*request_iterator)->import_assertions()->size());
   ++request_iterator;
 
+  CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("foo"));
+  ++request_iterator;
+
   CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("g"));
   CHECK_EQ(0, (*request_iterator)->import_assertions()->size());
   ++request_iterator;
@@ -8508,14 +8517,6 @@ TEST(ModuleParsingImportAssertionOrdering) {
             .first->IsOneByteEqualTo(""));
   ++request_iterator;
 
-  CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("p"));
-  CHECK_EQ(1, (*request_iterator)->import_assertions()->size());
-  CHECK((*request_iterator)
-            ->import_assertions()
-            ->at(z_string)
-            .first->IsOneByteEqualTo("c"));
-  ++request_iterator;
-
   CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("p"));
   CHECK_EQ(2, (*request_iterator)->import_assertions()->size());
   CHECK((*request_iterator)
@@ -8528,13 +8529,107 @@ TEST(ModuleParsingImportAssertionOrdering) {
             .first->IsOneByteEqualTo("c"));
   ++request_iterator;
 
-  CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("aa"));
+  CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("p"));
+  CHECK_EQ(1, (*request_iterator)->import_assertions()->size());
+  CHECK((*request_iterator)
+            ->import_assertions()
+            ->at(z_string)
+            .first->IsOneByteEqualTo("c"));
+}
+
+TEST(ModuleParsingImportAssertionKeySorting) {
+  i::FLAG_harmony_import_assertions = true;
+  i::Isolate* isolate = CcTest::i_isolate();
+  i::Factory* factory = isolate->factory();
+  v8::HandleScope handles(CcTest::isolate());
+  v8::Local<v8::Context> context = v8::Context::New(CcTest::isolate());
+  v8::Context::Scope context_scope(context);
+  isolate->stack_guard()->SetStackLimit(base::Stack::GetCurrentStackPosition() -
+                                        128 * 1024);
+
+  static const char kSource[] =
+      "import 'a' assert { 'b':'z', 'a': 'c' };"
+      "import 'b' assert { 'aaaaaa': 'c', 'b': 'z' };"
+      "import 'c' assert { '': 'c', 'b': 'z' };"
+      "import 'd' assert { 'aabbbb': 'c', 'aaabbb': 'z' };"
+      // zzzz\u0005 is a one-byte string, yyyy\u0100 is a two-byte string.
+      "import 'e' assert { 'zzzz\\u0005': 'second', 'yyyy\\u0100': 'first' };"
+      // Both keys are two-byte strings.
+      "import 'f' assert { 'xxxx\\u0005\\u0101': 'first', "
+      "'xxxx\\u0100\\u0101': 'second' };";
+  i::Handle<i::String> source = factory->NewStringFromAsciiChecked(kSource);
+  i::Handle<i::Script> script = factory->NewScript(source);
+  i::UnoptimizedCompileState compile_state(isolate);
+  i::UnoptimizedCompileFlags flags =
+      i::UnoptimizedCompileFlags::ForScriptCompile(isolate, *script);
+  flags.set_is_module(true);
+  i::ParseInfo info(isolate, flags, &compile_state);
+  CHECK_PARSE_PROGRAM(&info, script, isolate);
+
+  i::FunctionLiteral* func = info.literal();
+  i::ModuleScope* module_scope = func->scope()->AsModuleScope();
+  CHECK(module_scope->is_module_scope());
+
+  i::SourceTextModuleDescriptor* descriptor = module_scope->module();
+  CHECK_NOT_NULL(descriptor);
+
+  CHECK_EQ(6u, descriptor->module_requests().size());
+  auto request_iterator = descriptor->module_requests().cbegin();
+
+  CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("a"));
+  CHECK_EQ(2, (*request_iterator)->import_assertions()->size());
+  auto assertion_iterator = (*request_iterator)->import_assertions()->cbegin();
+  CHECK(assertion_iterator->first->IsOneByteEqualTo("a"));
+  CHECK(assertion_iterator->second.first->IsOneByteEqualTo("c"));
+  ++assertion_iterator;
+  CHECK(assertion_iterator->first->IsOneByteEqualTo("b"));
+  CHECK(assertion_iterator->second.first->IsOneByteEqualTo("z"));
   ++request_iterator;
 
-  CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("foo"));
+  CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("b"));
+  CHECK_EQ(2, (*request_iterator)->import_assertions()->size());
+  assertion_iterator = (*request_iterator)->import_assertions()->cbegin();
+  CHECK(assertion_iterator->first->IsOneByteEqualTo("aaaaaa"));
+  CHECK(assertion_iterator->second.first->IsOneByteEqualTo("c"));
+  ++assertion_iterator;
+  CHECK(assertion_iterator->first->IsOneByteEqualTo("b"));
+  CHECK(assertion_iterator->second.first->IsOneByteEqualTo("z"));
   ++request_iterator;
 
-  CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("baaaaaar"));
+  CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("c"));
+  CHECK_EQ(2, (*request_iterator)->import_assertions()->size());
+  assertion_iterator = (*request_iterator)->import_assertions()->cbegin();
+  CHECK(assertion_iterator->first->IsOneByteEqualTo(""));
+  CHECK(assertion_iterator->second.first->IsOneByteEqualTo("c"));
+  ++assertion_iterator;
+  CHECK(assertion_iterator->first->IsOneByteEqualTo("b"));
+  CHECK(assertion_iterator->second.first->IsOneByteEqualTo("z"));
+  ++request_iterator;
+
+  CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("d"));
+  CHECK_EQ(2, (*request_iterator)->import_assertions()->size());
+  assertion_iterator = (*request_iterator)->import_assertions()->cbegin();
+  CHECK(assertion_iterator->first->IsOneByteEqualTo("aaabbb"));
+  CHECK(assertion_iterator->second.first->IsOneByteEqualTo("z"));
+  ++assertion_iterator;
+  CHECK(assertion_iterator->first->IsOneByteEqualTo("aabbbb"));
+  CHECK(assertion_iterator->second.first->IsOneByteEqualTo("c"));
+  ++request_iterator;
+
+  CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("e"));
+  CHECK_EQ(2, (*request_iterator)->import_assertions()->size());
+  assertion_iterator = (*request_iterator)->import_assertions()->cbegin();
+  CHECK(assertion_iterator->second.first->IsOneByteEqualTo("first"));
+  ++assertion_iterator;
+  CHECK(assertion_iterator->second.first->IsOneByteEqualTo("second"));
+  ++request_iterator;
+
+  CHECK((*request_iterator)->specifier()->IsOneByteEqualTo("f"));
+  CHECK_EQ(2, (*request_iterator)->import_assertions()->size());
+  assertion_iterator = (*request_iterator)->import_assertions()->cbegin();
+  CHECK(assertion_iterator->second.first->IsOneByteEqualTo("first"));
+  ++assertion_iterator;
+  CHECK(assertion_iterator->second.first->IsOneByteEqualTo("second"));
 }
 
 TEST(DuplicateProtoError) {