Skip to content

Commit

Permalink
deps: V8: cherry-pick a0fd3209dda8
Browse files Browse the repository at this point in the history
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 <[email protected]>
    > Reviewed-by: Adam Klein <[email protected]>
    > 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 <[email protected]>
    Reviewed-by: Shu-yu Guo <[email protected]>
    Reviewed-by: Adam Klein <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#89115}

Refs: v8/v8@a0fd320
PR-URL: #51136
Reviewed-By: Yagiz Nizipli <[email protected]>
  • Loading branch information
nicolo-ribaudo authored and richardlau committed Mar 18, 2024
1 parent 68fd751 commit 6fbf0ba
Show file tree
Hide file tree
Showing 26 changed files with 398 additions and 33 deletions.
7 changes: 3 additions & 4 deletions deps/v8/include/v8-script.h
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,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;

Expand Down
47 changes: 35 additions & 12 deletions deps/v8/src/execution/isolate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4726,7 +4726,7 @@ MaybeHandle<FixedArray> Isolate::GetImportAssertionsFromArgument(

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

if (!import_assertions_argument->IsJSReceiver()) {
this->Throw(
Expand All @@ -4736,18 +4736,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 (FLAG_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 (FLAG_harmony_import_assertions &&
(!FLAG_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()) {
Expand All @@ -4769,6 +4786,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;
Expand All @@ -4786,9 +4805,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),
Expand All @@ -4797,6 +4814,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;
}

Expand Down
1 change: 1 addition & 0 deletions deps/v8/src/flags/flag-definitions.h
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,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_import_assertions, "harmony import assertions") \
Expand Down
1 change: 1 addition & 0 deletions deps/v8/src/init/bootstrapper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4482,6 +4482,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_private_brand_checks)
EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_class_static_blocks)
EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_error_cause)
Expand Down
1 change: 1 addition & 0 deletions deps/v8/src/init/heap-symbols.h
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,7 @@
V(_, week_string, "week") \
V(_, weeks_string, "weeks") \
V(_, weekOfYear_string, "weekOfYear") \
V(_, with_string, "with") \
V(_, word_string, "word") \
V(_, writable_string, "writable") \
V(_, yearMonthFromFields_string, "yearMonthFromFields") \
Expand Down
4 changes: 3 additions & 1 deletion deps/v8/src/parsing/parser-base.h
Original file line number Diff line number Diff line change
Expand Up @@ -3725,7 +3725,9 @@ ParserBase<Impl>::ParseImportExpressions() {
AcceptINScope scope(this, true);
ExpressionT specifier = ParseAssignmentExpressionCoverGrammar();

if (FLAG_harmony_import_assertions && Check(Token::COMMA)) {
if ((FLAG_harmony_import_assertions ||
FLAG_harmony_import_attributes) &&
Check(Token::COMMA)) {
if (Check(Token::RPAREN)) {
// A trailing comma allowed after the specifier.
return factory()->NewImportCallExpression(specifier, pos);
Expand Down
43 changes: 27 additions & 16 deletions deps/v8/src/parsing/parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1344,36 +1344,47 @@ 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 (!FLAG_harmony_import_assertions) {
return import_assertions;
}
if (FLAG_harmony_import_attributes && Check(Token::WITH)) {
// 'with' keyword consumed
} else if (FLAG_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.

// Assert clause is optional, and cannot be preceded by a LineTerminator.
if (scanner()->HasLineTerminatorBeforeNext() ||
!CheckContextualKeyword(ast_value_factory()->assert_string())) {
// NOTE(Node.js): Commented out to avoid backporting this use counter to Node.js 18
// ++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();
}
Expand Down
9 changes: 9 additions & 0 deletions deps/v8/test/mjsunit/harmony/modules-import-attributes-1.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Copyright 2021 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.

// Flags: --harmony-import-attributes

import { life } from 'modules-skip-1.mjs' with { };

assertEquals(42, life());
9 changes: 9 additions & 0 deletions deps/v8/test/mjsunit/harmony/modules-import-attributes-2.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Copyright 2021 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.

// Flags: --harmony-import-attributes

import json from 'modules-skip-1.json' with { type: 'json' };

assertEquals(42, json.life);
9 changes: 9 additions & 0 deletions deps/v8/test/mjsunit/harmony/modules-import-attributes-3.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Copyright 2021 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.

// Flags: --harmony-import-attributes

import {life} from 'modules-skip-imports-json-1.mjs';

assertEquals(42, life());
9 changes: 9 additions & 0 deletions deps/v8/test/mjsunit/harmony/modules-import-attributes-4.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Copyright 2021 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.

// Flags: --harmony-import-attributes

import json from 'modules-skip-1.json' with { type: 'json', notARealAssertion: 'value'};

assertEquals(42, json.life);
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Copyright 2021 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.

// Flags: --allow-natives-syntax --harmony-import-attributes

var life;
import('modules-skip-1.mjs', { }).then(namespace => life = namespace.life());

%PerformMicrotaskCheckpoint();

assertEquals(42, life);
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright 2021 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.

// Flags: --allow-natives-syntax --harmony-import-attributes

var result1;
var result2;
import('modules-skip-1.json', { get with() { throw 'bad \'with\' getter!'; } }).then(
() => assertUnreachable('Should have failed due to throwing getter'),
error => result1 = error);
import('modules-skip-1.json', { with: { get assertionKey() { throw 'bad \'assertionKey\' getter!'; } } }).then(
() => assertUnreachable('Should have failed due to throwing getter'),
error => result2 = error);

%PerformMicrotaskCheckpoint();

assertEquals('bad \'with\' getter!', result1);
assertEquals('bad \'assertionKey\' getter!', result2);
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright 2021 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.

// Flags: --allow-natives-syntax --harmony-import-attributes --harmony-top-level-await

var life1;
var life2;
import('modules-skip-1.json', { with: { type: 'json' } }).then(
namespace => life1 = namespace.default.life);

// Try loading the same module a second time.
import('modules-skip-1.json', { with: { type: 'json' } }).then(
namespace => life2 = namespace.default.life);

%PerformMicrotaskCheckpoint();

assertEquals(42, life1);
assertEquals(42, life2);
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright 2021 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.

// Flags: --allow-natives-syntax --harmony-import-attributes

let result1;
let result2;

let badAssertProxy1 = new Proxy({}, { ownKeys() { throw "bad ownKeys!"; } });
import('./modules-skip-1.mjs', { with: badAssertProxy1 }).then(
() => assertUnreachable('Should have failed due to throwing ownKeys'),
error => result1 = error);

let badAssertProxy2 = new Proxy(
{foo: "bar"},
{ getOwnPropertyDescriptor() { throw "bad getOwnPropertyDescriptor!"; } });
import('./modules-skip-1.mjs', { with: badAssertProxy2 }).then(
() => assertUnreachable(
'Should have failed due to throwing getOwnPropertyDescriptor'),
error => result2 = error);

%PerformMicrotaskCheckpoint();

assertEquals('bad ownKeys!', result1);
assertEquals('bad getOwnPropertyDescriptor!', result2);
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright 2021 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.

// Flags: --allow-natives-syntax --harmony-import-attributes

let getters = 0;
let result;

import('./modules-skip-1.mjs', { with: {
get attr1() {
getters++;
return {};
},
get attr2() {
getters++;
return {};
},
} }).then(
() => assertUnreachable('Should have failed due to invalid attributes values'),
error => result = error);

%PerformMicrotaskCheckpoint();

assertEquals(2, getters);
assertInstanceof(result, TypeError);
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright 2021 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.

// Flags: --allow-natives-syntax --harmony-import-attributes

var life;
import('modules-skip-1.mjs', { with: { } }).then(
namespace => life = namespace.life());

%PerformMicrotaskCheckpoint();

assertEquals(42, life);
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright 2021 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.

// Flags: --allow-natives-syntax --harmony-import-attributes

var life;
import('modules-skip-1.json', { with: { type: 'json' } }).then(
namespace => life = namespace.default.life);

%PerformMicrotaskCheckpoint();

assertEquals(42, life);
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright 2021 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.

// Flags: --allow-natives-syntax --harmony-import-attributes

var result;
import('modules-skip-1.json', { with: { type: 'notARealType' } }).then(
() => assertUnreachable('Should have failed due to bad module type'),
error => result = error.message);

%PerformMicrotaskCheckpoint();

assertEquals('Invalid module type was asserted', result);
Loading

0 comments on commit 6fbf0ba

Please sign in to comment.