X Tutup
The Wayback Machine - https://web.archive.org/web/20250730151906/https://github.com/nodejs/node/commit/6fbf0ba5c3
Skip to content

Commit 6fbf0ba

Browse files
nicolo-ribaudorichardlau
nicolo-ribaudo
authored and
richardlau
committed
deps: V8: cherry-pick a0fd3209dda8
Original commit message: Reland "[import-attributes] Implement import attributes, with `assert` fallback" This is a reland of commit 159c82c5e6392e78b9bba7161b1bed6e23758984, 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} Refs: v8/v8@a0fd320 PR-URL: #51136 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
1 parent 68fd751 commit 6fbf0ba

26 files changed

+398
-33
lines changed

‎deps/v8/include/v8-script.h

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

‎deps/v8/src/execution/isolate.cc

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4726,7 +4726,7 @@ MaybeHandle<FixedArray> Isolate::GetImportAssertionsFromArgument(
47264726

47274727
// The parser shouldn't have allowed the second argument to import() if
47284728
// the flag wasn't enabled.
4729-
DCHECK(FLAG_harmony_import_assertions);
4729+
DCHECK(FLAG_harmony_import_assertions || FLAG_harmony_import_attributes);
47304730

47314731
if (!import_assertions_argument->IsJSReceiver()) {
47324732
this->Throw(
@@ -4736,18 +4736,35 @@ MaybeHandle<FixedArray> Isolate::GetImportAssertionsFromArgument(
47364736

47374737
Handle<JSReceiver> import_assertions_argument_receiver =
47384738
Handle<JSReceiver>::cast(import_assertions_argument);
4739-
Handle<Name> key = factory()->assert_string();
47404739

47414740
Handle<Object> import_assertions_object;
4742-
if (!JSReceiver::GetProperty(this, import_assertions_argument_receiver, key)
4743-
.ToHandle(&import_assertions_object)) {
4744-
// This can happen if the property has a getter function that throws
4745-
// an error.
4746-
return MaybeHandle<FixedArray>();
4741+
4742+
if (FLAG_harmony_import_attributes) {
4743+
Handle<Name> with_key = factory()->with_string();
4744+
if (!JSReceiver::GetProperty(this, import_assertions_argument_receiver,
4745+
with_key)
4746+
.ToHandle(&import_assertions_object)) {
4747+
// This can happen if the property has a getter function that throws
4748+
// an error.
4749+
return MaybeHandle<FixedArray>();
4750+
}
47474751
}
47484752

4749-
// If there is no 'assert' option in the options bag, it's not an error. Just
4750-
// do the import() as if no assertions were provided.
4753+
if (FLAG_harmony_import_assertions &&
4754+
(!FLAG_harmony_import_attributes ||
4755+
import_assertions_object->IsUndefined())) {
4756+
Handle<Name> assert_key = factory()->assert_string();
4757+
if (!JSReceiver::GetProperty(this, import_assertions_argument_receiver,
4758+
assert_key)
4759+
.ToHandle(&import_assertions_object)) {
4760+
// This can happen if the property has a getter function that throws
4761+
// an error.
4762+
return MaybeHandle<FixedArray>();
4763+
}
4764+
}
4765+
4766+
// If there is no 'with' or 'assert' option in the options bag, it's not an
4767+
// error. Just do the import() as if no assertions were provided.
47514768
if (import_assertions_object->IsUndefined()) return import_assertions_array;
47524769

47534770
if (!import_assertions_object->IsJSReceiver()) {
@@ -4769,6 +4786,8 @@ MaybeHandle<FixedArray> Isolate::GetImportAssertionsFromArgument(
47694786
return MaybeHandle<FixedArray>();
47704787
}
47714788

4789+
bool has_non_string_attribute = false;
4790+
47724791
// The assertions will be passed to the host in the form: [key1,
47734792
// value1, key2, value2, ...].
47744793
constexpr size_t kAssertionEntrySizeForDynamicImport = 2;
@@ -4786,9 +4805,7 @@ MaybeHandle<FixedArray> Isolate::GetImportAssertionsFromArgument(
47864805
}
47874806

47884807
if (!assertion_value->IsString()) {
4789-
this->Throw(*factory()->NewTypeError(
4790-
MessageTemplate::kNonStringImportAssertionValue));
4791-
return MaybeHandle<FixedArray>();
4808+
has_non_string_attribute = true;
47924809
}
47934810

47944811
import_assertions_array->set((i * kAssertionEntrySizeForDynamicImport),
@@ -4797,6 +4814,12 @@ MaybeHandle<FixedArray> Isolate::GetImportAssertionsFromArgument(
47974814
*assertion_value);
47984815
}
47994816

4817+
if (has_non_string_attribute) {
4818+
this->Throw(*factory()->NewTypeError(
4819+
MessageTemplate::kNonStringImportAssertionValue));
4820+
return MaybeHandle<FixedArray>();
4821+
}
4822+
48004823
return import_assertions_array;
48014824
}
48024825

‎deps/v8/src/flags/flag-definitions.h

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

301301
// Features that are still work in progress (behind individual flags).
302302
#define HARMONY_INPROGRESS_BASE(V) \
303+
V(harmony_import_attributes, "harmony import attributes") \
303304
V(harmony_weak_refs_with_cleanup_some, \
304305
"harmony weak references with FinalizationRegistry.prototype.cleanupSome") \
305306
V(harmony_import_assertions, "harmony import assertions") \

‎deps/v8/src/init/bootstrapper.cc

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

44844484
EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_import_assertions)
4485+
EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_import_attributes)
44854486
EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_private_brand_checks)
44864487
EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_class_static_blocks)
44874488
EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_error_cause)

‎deps/v8/src/init/heap-symbols.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,6 +432,7 @@
432432
V(_, week_string, "week") \
433433
V(_, weeks_string, "weeks") \
434434
V(_, weekOfYear_string, "weekOfYear") \
435+
V(_, with_string, "with") \
435436
V(_, word_string, "word") \
436437
V(_, writable_string, "writable") \
437438
V(_, yearMonthFromFields_string, "yearMonthFromFields") \

‎deps/v8/src/parsing/parser-base.h

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

3728-
if (FLAG_harmony_import_assertions && Check(Token::COMMA)) {
3728+
if ((FLAG_harmony_import_assertions ||
3729+
FLAG_harmony_import_attributes) &&
3730+
Check(Token::COMMA)) {
37293731
if (Check(Token::RPAREN)) {
37303732
// A trailing comma allowed after the specifier.
37313733
return factory()->NewImportCallExpression(specifier, pos);

‎deps/v8/src/parsing/parser.cc

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1344,36 +1344,47 @@ ZonePtrList<const Parser::NamedImport>* Parser::ParseNamedImports(int pos) {
13441344
}
13451345

13461346
ImportAssertions* Parser::ParseImportAssertClause() {
1347-
// AssertClause :
1348-
// assert '{' '}'
1349-
// assert '{' AssertEntries '}'
1347+
// WithClause :
1348+
// with '{' '}'
1349+
// with '{' WithEntries ','? '}'
13501350

1351-
// AssertEntries :
1352-
// IdentifierName: AssertionKey
1353-
// IdentifierName: AssertionKey , AssertEntries
1351+
// WithEntries :
1352+
// LiteralPropertyName
1353+
// LiteralPropertyName ':' StringLiteral , WithEntries
13541354

1355-
// AssertionKey :
1356-
// IdentifierName
1357-
// StringLiteral
1355+
// (DEPRECATED)
1356+
// AssertClause :
1357+
// assert '{' '}'
1358+
// assert '{' WithEntries ','? '}'
13581359

13591360
auto import_assertions = zone()->New<ImportAssertions>(zone());
13601361

1361-
if (!FLAG_harmony_import_assertions) {
1362-
return import_assertions;
1363-
}
1362+
if (FLAG_harmony_import_attributes && Check(Token::WITH)) {
1363+
// 'with' keyword consumed
1364+
} else if (FLAG_harmony_import_assertions &&
1365+
!scanner()->HasLineTerminatorBeforeNext() &&
1366+
CheckContextualKeyword(ast_value_factory()->assert_string())) {
1367+
// The 'assert' contextual keyword is deprecated in favor of 'with', and we
1368+
// need to investigate feasibility of unshipping.
1369+
//
1370+
// TODO(v8:13856): Remove once decision is made to unship 'assert' or keep.
13641371

1365-
// Assert clause is optional, and cannot be preceded by a LineTerminator.
1366-
if (scanner()->HasLineTerminatorBeforeNext() ||
1367-
!CheckContextualKeyword(ast_value_factory()->assert_string())) {
1372+
// NOTE(Node.js): Commented out to avoid backporting this use counter to Node.js 18
1373+
// ++use_counts_[v8::Isolate::kImportAssertionDeprecatedSyntax];
1374+
} else {
13681375
return import_assertions;
13691376
}
13701377

13711378
Expect(Token::LBRACE);
13721379

13731380
while (peek() != Token::RBRACE) {
13741381
const AstRawString* attribute_key = nullptr;
1375-
if (Check(Token::STRING)) {
1382+
if (Check(Token::STRING) || Check(Token::SMI)) {
13761383
attribute_key = GetSymbol();
1384+
} else if (Check(Token::NUMBER)) {
1385+
attribute_key = GetNumberAsSymbol();
1386+
} else if (Check(Token::BIGINT)) {
1387+
attribute_key = GetBigIntAsSymbol();
13771388
} else {
13781389
attribute_key = ParsePropertyName();
13791390
}

‎deps/v8/test/mjsunit/harmony/modules-import-attributes-1.mjs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
// Copyright 2021 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
// Flags: --harmony-import-attributes
6+
7+
import { life } from 'modules-skip-1.mjs' with { };
8+
9+
assertEquals(42, life());

‎deps/v8/test/mjsunit/harmony/modules-import-attributes-2.mjs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
// Copyright 2021 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
// Flags: --harmony-import-attributes
6+
7+
import json from 'modules-skip-1.json' with { type: 'json' };
8+
9+
assertEquals(42, json.life);

‎deps/v8/test/mjsunit/harmony/modules-import-attributes-3.mjs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
// Copyright 2021 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
// Flags: --harmony-import-attributes
6+
7+
import {life} from 'modules-skip-imports-json-1.mjs';
8+
9+
assertEquals(42, life());

0 commit comments

Comments
 (0)
X Tutup