Skip to content

Commit a0fd320

Browse files
nicolo-ribaudoV8 LUCI CQ
nicolo-ribaudo
authored and
V8 LUCI CQ
committed
Reland "[import-attributes] Implement import attributes, with assert fallback"
This is a reland of commit 159c82c, to set the correct author. Original change's description: > [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} Bug: v8:13856 Change-Id: Ic59aa3bd9101618e47ddf6cf6d6416a3a438ebec Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4705558 Commit-Queue: Shu-yu Guo <syg@chromium.org> Reviewed-by: Shu-yu Guo <syg@chromium.org> Reviewed-by: Adam Klein <adamk@chromium.org> Cr-Commit-Position: refs/heads/main@{#89115}
1 parent 934e49d commit a0fd320

28 files changed

Lines changed: 728 additions & 139 deletions

‎include/v8-script.h‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -143,10 +143,9 @@ class V8_EXPORT ModuleRequest : public Data {
143143
*
144144
* All assertions present in the module request will be supplied in this
145145
* list, regardless of whether they are supported by the host. Per
146-
* https://tc39.es/proposal-import-assertions/#sec-hostgetsupportedimportassertions,
147-
* hosts are expected to ignore assertions that they do not support (as
148-
* opposed to, for example, triggering an error if an unsupported assertion is
149-
* present).
146+
* https://tc39.es/proposal-import-attributes/#sec-hostgetsupportedimportattributes,
147+
* hosts are expected to throw for assertions that they do not support (as
148+
* opposed to, for example, ignoring them).
150149
*/
151150
Local<FixedArray> GetImportAssertions() const;
152151

‎src/execution/isolate.cc‎

Lines changed: 36 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5334,7 +5334,8 @@ MaybeHandle<FixedArray> Isolate::GetImportAssertionsFromArgument(
53345334

53355335
// The parser shouldn't have allowed the second argument to import() if
53365336
// the flag wasn't enabled.
5337-
DCHECK(v8_flags.harmony_import_assertions);
5337+
DCHECK(v8_flags.harmony_import_assertions ||
5338+
v8_flags.harmony_import_attributes);
53385339

53395340
if (!import_assertions_argument->IsJSReceiver()) {
53405341
this->Throw(
@@ -5344,18 +5345,35 @@ MaybeHandle<FixedArray> Isolate::GetImportAssertionsFromArgument(
53445345

53455346
Handle<JSReceiver> import_assertions_argument_receiver =
53465347
Handle<JSReceiver>::cast(import_assertions_argument);
5347-
Handle<Name> key = factory()->assert_string();
53485348

53495349
Handle<Object> import_assertions_object;
5350-
if (!JSReceiver::GetProperty(this, import_assertions_argument_receiver, key)
5351-
.ToHandle(&import_assertions_object)) {
5352-
// This can happen if the property has a getter function that throws
5353-
// an error.
5354-
return MaybeHandle<FixedArray>();
5350+
5351+
if (v8_flags.harmony_import_attributes) {
5352+
Handle<Name> with_key = factory()->with_string();
5353+
if (!JSReceiver::GetProperty(this, import_assertions_argument_receiver,
5354+
with_key)
5355+
.ToHandle(&import_assertions_object)) {
5356+
// This can happen if the property has a getter function that throws
5357+
// an error.
5358+
return MaybeHandle<FixedArray>();
5359+
}
53555360
}
53565361

5357-
// If there is no 'assert' option in the options bag, it's not an error. Just
5358-
// do the import() as if no assertions were provided.
5362+
if (v8_flags.harmony_import_assertions &&
5363+
(!v8_flags.harmony_import_attributes ||
5364+
import_assertions_object->IsUndefined())) {
5365+
Handle<Name> assert_key = factory()->assert_string();
5366+
if (!JSReceiver::GetProperty(this, import_assertions_argument_receiver,
5367+
assert_key)
5368+
.ToHandle(&import_assertions_object)) {
5369+
// This can happen if the property has a getter function that throws
5370+
// an error.
5371+
return MaybeHandle<FixedArray>();
5372+
}
5373+
}
5374+
5375+
// If there is no 'with' or 'assert' option in the options bag, it's not an
5376+
// error. Just do the import() as if no assertions were provided.
53595377
if (import_assertions_object->IsUndefined()) return import_assertions_array;
53605378

53615379
if (!import_assertions_object->IsJSReceiver()) {
@@ -5377,6 +5395,8 @@ MaybeHandle<FixedArray> Isolate::GetImportAssertionsFromArgument(
53775395
return MaybeHandle<FixedArray>();
53785396
}
53795397

5398+
bool has_non_string_attribute = false;
5399+
53805400
// The assertions will be passed to the host in the form: [key1,
53815401
// value1, key2, value2, ...].
53825402
constexpr size_t kAssertionEntrySizeForDynamicImport = 2;
@@ -5394,9 +5414,7 @@ MaybeHandle<FixedArray> Isolate::GetImportAssertionsFromArgument(
53945414
}
53955415

53965416
if (!assertion_value->IsString()) {
5397-
this->Throw(*factory()->NewTypeError(
5398-
MessageTemplate::kNonStringImportAssertionValue));
5399-
return MaybeHandle<FixedArray>();
5417+
has_non_string_attribute = true;
54005418
}
54015419

54025420
import_assertions_array->set((i * kAssertionEntrySizeForDynamicImport),
@@ -5405,6 +5423,12 @@ MaybeHandle<FixedArray> Isolate::GetImportAssertionsFromArgument(
54055423
*assertion_value);
54065424
}
54075425

5426+
if (has_non_string_attribute) {
5427+
this->Throw(*factory()->NewTypeError(
5428+
MessageTemplate::kNonStringImportAssertionValue));
5429+
return MaybeHandle<FixedArray>();
5430+
}
5431+
54085432
return import_assertions_array;
54095433
}
54105434

‎src/flags/flag-definitions.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,7 @@ DEFINE_BOOL(harmony_shipping, true, "enable all shipped harmony features")
241241

242242
// Features that are still work in progress (behind individual flags).
243243
#define HARMONY_INPROGRESS_BASE(V) \
244+
V(harmony_import_attributes, "harmony import attributes") \
244245
V(harmony_weak_refs_with_cleanup_some, \
245246
"harmony weak references with FinalizationRegistry.prototype.cleanupSome") \
246247
V(harmony_temporal, "Temporal") \

‎src/init/bootstrapper.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4577,6 +4577,7 @@ void Genesis::InitializeConsole(Handle<JSObject> extras_binding) {
45774577
void Genesis::InitializeGlobal_##id() {}
45784578

45794579
EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_import_assertions)
4580+
EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_import_attributes)
45804581
EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_rab_gsab_transfer)
45814582

45824583
#ifdef V8_INTL_SUPPORT

‎src/init/heap-symbols.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -467,6 +467,7 @@
467467
V(_, week_string, "week") \
468468
V(_, weeks_string, "weeks") \
469469
V(_, weekOfYear_string, "weekOfYear") \
470+
V(_, with_string, "with") \
470471
V(_, word_string, "word") \
471472
V(_, yearMonthFromFields_string, "yearMonthFromFields") \
472473
V(_, year_string, "year") \

‎src/parsing/parser-base.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3757,7 +3757,9 @@ ParserBase<Impl>::ParseImportExpressions() {
37573757
AcceptINScope scope(this, true);
37583758
ExpressionT specifier = ParseAssignmentExpressionCoverGrammar();
37593759

3760-
if (v8_flags.harmony_import_assertions && Check(Token::COMMA)) {
3760+
if ((v8_flags.harmony_import_assertions ||
3761+
v8_flags.harmony_import_attributes) &&
3762+
Check(Token::COMMA)) {
37613763
if (Check(Token::RPAREN)) {
37623764
// A trailing comma allowed after the specifier.
37633765
return factory()->NewImportCallExpression(specifier, pos);

‎src/parsing/parser.cc‎

Lines changed: 26 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1376,36 +1376,45 @@ ZonePtrList<const Parser::NamedImport>* Parser::ParseNamedImports(int pos) {
13761376
}
13771377

13781378
ImportAssertions* Parser::ParseImportAssertClause() {
1379-
// AssertClause :
1380-
// assert '{' '}'
1381-
// assert '{' AssertEntries '}'
1379+
// WithClause :
1380+
// with '{' '}'
1381+
// with '{' WithEntries ','? '}'
13821382

1383-
// AssertEntries :
1384-
// IdentifierName: AssertionKey
1385-
// IdentifierName: AssertionKey , AssertEntries
1383+
// WithEntries :
1384+
// LiteralPropertyName
1385+
// LiteralPropertyName ':' StringLiteral , WithEntries
13861386

1387-
// AssertionKey :
1388-
// IdentifierName
1389-
// StringLiteral
1387+
// (DEPRECATED)
1388+
// AssertClause :
1389+
// assert '{' '}'
1390+
// assert '{' WithEntries ','? '}'
13901391

13911392
auto import_assertions = zone()->New<ImportAssertions>(zone());
13921393

1393-
if (!v8_flags.harmony_import_assertions) {
1394-
return import_assertions;
1395-
}
1396-
1397-
// Assert clause is optional, and cannot be preceded by a LineTerminator.
1398-
if (scanner()->HasLineTerminatorBeforeNext() ||
1399-
!CheckContextualKeyword(ast_value_factory()->assert_string())) {
1394+
if (v8_flags.harmony_import_attributes && Check(Token::WITH)) {
1395+
// 'with' keyword consumed
1396+
} else if (v8_flags.harmony_import_assertions &&
1397+
!scanner()->HasLineTerminatorBeforeNext() &&
1398+
CheckContextualKeyword(ast_value_factory()->assert_string())) {
1399+
// The 'assert' contextual keyword is deprecated in favor of 'with', and we
1400+
// need to investigate feasibility of unshipping.
1401+
//
1402+
// TODO(v8:13856): Remove once decision is made to unship 'assert' or keep.
1403+
++use_counts_[v8::Isolate::kImportAssertionDeprecatedSyntax];
1404+
} else {
14001405
return import_assertions;
14011406
}
14021407

14031408
Expect(Token::LBRACE);
14041409

14051410
while (peek() != Token::RBRACE) {
14061411
const AstRawString* attribute_key = nullptr;
1407-
if (Check(Token::STRING)) {
1412+
if (Check(Token::STRING) || Check(Token::SMI)) {
14081413
attribute_key = GetSymbol();
1414+
} else if (Check(Token::NUMBER)) {
1415+
attribute_key = GetNumberAsSymbol();
1416+
} else if (Check(Token::BIGINT)) {
1417+
attribute_key = GetBigIntAsSymbol();
14091418
} else {
14101419
attribute_key = ParsePropertyName();
14111420
}
@@ -1439,12 +1448,6 @@ ImportAssertions* Parser::ParseImportAssertClause() {
14391448

14401449
Expect(Token::RBRACE);
14411450

1442-
// The 'assert' contextual keyword is deprecated in favor of 'with', and we
1443-
// need to investigate feasibility of unshipping.
1444-
//
1445-
// TODO(v8:13856): Remove once decision is made to unship 'assert' or keep.
1446-
++use_counts_[v8::Isolate::kImportAssertionDeprecatedSyntax];
1447-
14481451
return import_assertions;
14491452
}
14501453

0 commit comments

Comments
 (0)