[import-attributes] Implement import attributes, with assert fallback

In the past six months, the old import assertions proposal has been
renamed to "import attributes" with the follwing major changes:
1. the keyword is now `with` instead of `assert`
2. unknown assertions cause an error rather than being ignored

To preserve backward compatibility with existing applications that use
`assert`, implementations _can_ keep it around as a fallback for both
the static and dynamic forms.

Additionally, the proposal has some minor changes that came up during
the stage 3 reviews:
3. dynamic import first reads all the attributes, and then verifies
   that they are all strings
4. there is no need for a `[no LineTerminator here]` restriction before
   the `with` keyword
5. static import syntax allows any `LiteralPropertyName` as attribute
   keys, to align with every other syntax using key-value pairs

The new syntax is enabled by a new `--harmony-import-attributes` flag,
disabled by default. However, the new behavioral changes also apply to
the old syntax that is under the `--harmony-import-assertions` flag.

This patch does implements (1), (3), (4) and (5). Handling of unknown
import assertions was not implemented directly in V8, but delegated
to embedders. As such, it will be implemented separately in d8 and
Chromium.

To simplify the review, this patch doesn't migrate usage of the term
"assertions" to "attributes". There are many variables and internal
functions that could be easily renamed as soon as this patch landes.
There is one usage in the public API
(`ModuleRequest::GetImportAssertions`) that will probably need to be
aliased and then removed following the same process as for other API
breaking changes.

Bug: v8:13856
Change-Id: I78b167348d898887332c5ca7468bc5d58cd9b1ca
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4632799
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Reviewed-by: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#89110}

include/v8-script.h

@@ -143,10 +143,9 @@ class V8_EXPORT ModuleRequest : public Data {
    *
    * All assertions present in the module request will be supplied in this
    * list, regardless of whether they are supported by the host. Per
-   * https://tc39.es/proposal-import-assertions/#sec-hostgetsupportedimportassertions,
-   * hosts are expected to ignore assertions that they do not support (as
-   * opposed to, for example, triggering an error if an unsupported assertion is
-   * present).
+   * https://tc39.es/proposal-import-attributes/#sec-hostgetsupportedimportattributes,
+   * hosts are expected to throw for assertions that they do not support (as
+   * opposed to, for example, ignoring them).
    */
   Local<FixedArray> GetImportAssertions() const;
 

src/execution/isolate.cc

@@ -5334,7 +5334,8 @@ MaybeHandle<FixedArray> Isolate::GetImportAssertionsFromArgument(
 
   // The parser shouldn't have allowed the second argument to import() if
   // the flag wasn't enabled.
-  DCHECK(v8_flags.harmony_import_assertions);
+  DCHECK(v8_flags.harmony_import_assertions ||
+         v8_flags.harmony_import_attributes);
 
   if (!import_assertions_argument->IsJSReceiver()) {
     this->Throw(
@@ -5344,18 +5345,35 @@ MaybeHandle<FixedArray> Isolate::GetImportAssertionsFromArgument(
 
   Handle<JSReceiver> import_assertions_argument_receiver =
       Handle<JSReceiver>::cast(import_assertions_argument);
-  Handle<Name> key = factory()->assert_string();
 
   Handle<Object> import_assertions_object;
-  if (!JSReceiver::GetProperty(this, import_assertions_argument_receiver, key)
-           .ToHandle(&import_assertions_object)) {
-    // This can happen if the property has a getter function that throws
-    // an error.
-    return MaybeHandle<FixedArray>();
+
+  if (v8_flags.harmony_import_attributes) {
+    Handle<Name> with_key = factory()->with_string();
+    if (!JSReceiver::GetProperty(this, import_assertions_argument_receiver,
+                                 with_key)
+             .ToHandle(&import_assertions_object)) {
+      // This can happen if the property has a getter function that throws
+      // an error.
+      return MaybeHandle<FixedArray>();
+    }
   }
 
-  // If there is no 'assert' option in the options bag, it's not an error. Just
-  // do the import() as if no assertions were provided.
+  if (v8_flags.harmony_import_assertions &&
+      (!v8_flags.harmony_import_attributes ||
+       import_assertions_object->IsUndefined())) {
+    Handle<Name> assert_key = factory()->assert_string();
+    if (!JSReceiver::GetProperty(this, import_assertions_argument_receiver,
+                                 assert_key)
+             .ToHandle(&import_assertions_object)) {
+      // This can happen if the property has a getter function that throws
+      // an error.
+      return MaybeHandle<FixedArray>();
+    }
+  }
+
+  // If there is no 'with' or 'assert' option in the options bag, it's not an
+  // error. Just do the import() as if no assertions were provided.
   if (import_assertions_object->IsUndefined()) return import_assertions_array;
 
   if (!import_assertions_object->IsJSReceiver()) {
@@ -5377,6 +5395,8 @@ MaybeHandle<FixedArray> Isolate::GetImportAssertionsFromArgument(
     return MaybeHandle<FixedArray>();
   }
 
+  bool has_non_string_attribute = false;
+
   // The assertions will be passed to the host in the form: [key1,
   // value1, key2, value2, ...].
   constexpr size_t kAssertionEntrySizeForDynamicImport = 2;
@@ -5394,9 +5414,7 @@ MaybeHandle<FixedArray> Isolate::GetImportAssertionsFromArgument(
     }
 
     if (!assertion_value->IsString()) {
-      this->Throw(*factory()->NewTypeError(
-          MessageTemplate::kNonStringImportAssertionValue));
-      return MaybeHandle<FixedArray>();
+      has_non_string_attribute = true;
     }
 
     import_assertions_array->set((i * kAssertionEntrySizeForDynamicImport),
@@ -5405,6 +5423,12 @@ MaybeHandle<FixedArray> Isolate::GetImportAssertionsFromArgument(
                                  *assertion_value);
   }
 
+  if (has_non_string_attribute) {
+    this->Throw(*factory()->NewTypeError(
+        MessageTemplate::kNonStringImportAssertionValue));
+    return MaybeHandle<FixedArray>();
+  }
+
   return import_assertions_array;
 }
 

src/flags/flag-definitions.h

@@ -241,6 +241,7 @@ DEFINE_BOOL(harmony_shipping, true, "enable all shipped harmony features")
 
 // Features that are still work in progress (behind individual flags).
 #define HARMONY_INPROGRESS_BASE(V)                                             \
+  V(harmony_import_attributes, "harmony import attributes")                    \
   V(harmony_weak_refs_with_cleanup_some,                                       \
     "harmony weak references with FinalizationRegistry.prototype.cleanupSome") \
   V(harmony_temporal, "Temporal")                                              \

src/init/bootstrapper.cc

@@ -4577,6 +4577,7 @@ void Genesis::InitializeConsole(Handle<JSObject> extras_binding) {
   void Genesis::InitializeGlobal_##id() {}
 
 EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_import_assertions)
+EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_import_attributes)
 EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_rab_gsab_transfer)
 
 #ifdef V8_INTL_SUPPORT

src/init/heap-symbols.h

@@ -467,6 +467,7 @@
   V(_, week_string, "week")                                                   \
   V(_, weeks_string, "weeks")                                                 \
   V(_, weekOfYear_string, "weekOfYear")                                       \
+  V(_, with_string, "with")                                                   \
   V(_, word_string, "word")                                                   \
   V(_, yearMonthFromFields_string, "yearMonthFromFields")                     \
   V(_, year_string, "year")                                                   \

src/parsing/parser-base.h

@@ -3757,7 +3757,9 @@ ParserBase<Impl>::ParseImportExpressions() {
   AcceptINScope scope(this, true);
   ExpressionT specifier = ParseAssignmentExpressionCoverGrammar();
 
-  if (v8_flags.harmony_import_assertions && Check(Token::COMMA)) {
+  if ((v8_flags.harmony_import_assertions ||
+       v8_flags.harmony_import_attributes) &&
+      Check(Token::COMMA)) {
     if (Check(Token::RPAREN)) {
       // A trailing comma allowed after the specifier.
       return factory()->NewImportCallExpression(specifier, pos);

src/parsing/parser.cc

@@ -1376,36 +1376,45 @@ ZonePtrList<const Parser::NamedImport>* Parser::ParseNamedImports(int pos) {
 }
 
 ImportAssertions* Parser::ParseImportAssertClause() {
-  // AssertClause :
-  //    assert '{' '}'
-  //    assert '{' AssertEntries '}'
+  // WithClause :
+  //    with '{' '}'
+  //    with '{' WithEntries ','? '}'
 
-  // AssertEntries :
-  //    IdentifierName: AssertionKey
-  //    IdentifierName: AssertionKey , AssertEntries
+  // WithEntries :
+  //    LiteralPropertyName
+  //    LiteralPropertyName ':' StringLiteral , WithEntries
 
-  // AssertionKey :
-  //     IdentifierName
-  //     StringLiteral
+  // (DEPRECATED)
+  // AssertClause :
+  //    assert '{' '}'
+  //    assert '{' WithEntries ','? '}'
 
   auto import_assertions = zone()->New<ImportAssertions>(zone());
 
-  if (!v8_flags.harmony_import_assertions) {
-    return import_assertions;
-  }
-
-  // Assert clause is optional, and cannot be preceded by a LineTerminator.
-  if (scanner()->HasLineTerminatorBeforeNext() ||
-      !CheckContextualKeyword(ast_value_factory()->assert_string())) {
+  if (v8_flags.harmony_import_attributes && Check(Token::WITH)) {
+    // 'with' keyword consumed
+  } else if (v8_flags.harmony_import_assertions &&
+             !scanner()->HasLineTerminatorBeforeNext() &&
+             CheckContextualKeyword(ast_value_factory()->assert_string())) {
+    // The 'assert' contextual keyword is deprecated in favor of 'with', and we
+    // need to investigate feasibility of unshipping.
+    //
+    // TODO(v8:13856): Remove once decision is made to unship 'assert' or keep.
+    ++use_counts_[v8::Isolate::kImportAssertionDeprecatedSyntax];
+  } else {
     return import_assertions;
   }
 
   Expect(Token::LBRACE);
 
   while (peek() != Token::RBRACE) {
     const AstRawString* attribute_key = nullptr;
-    if (Check(Token::STRING)) {
+    if (Check(Token::STRING) || Check(Token::SMI)) {
       attribute_key = GetSymbol();
+    } else if (Check(Token::NUMBER)) {
+      attribute_key = GetNumberAsSymbol();
+    } else if (Check(Token::BIGINT)) {
+      attribute_key = GetBigIntAsSymbol();
     } else {
       attribute_key = ParsePropertyName();
     }
@@ -1439,12 +1448,6 @@ ImportAssertions* Parser::ParseImportAssertClause() {
 
   Expect(Token::RBRACE);
 
-  // The 'assert' contextual keyword is deprecated in favor of 'with', and we
-  // need to investigate feasibility of unshipping.
-  //
-  // TODO(v8:13856): Remove once decision is made to unship 'assert' or keep.
-  ++use_counts_[v8::Isolate::kImportAssertionDeprecatedSyntax];
-
   return import_assertions;
 }