Skip to content

Commit

Permalink
Revert "[import-attributes] Remove support for import assertions"
Browse files Browse the repository at this point in the history
This reverts commit 8e18266.
  • Loading branch information
ry authored and github-actions[bot] committed Feb 8, 2025
1 parent 8d8094d commit 8b325d6
Show file tree
Hide file tree
Showing 50 changed files with 736 additions and 167 deletions.
4 changes: 2 additions & 2 deletions BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -2221,8 +2221,8 @@ filegroup(
"src/parsing/expression-scope.h",
"src/parsing/func-name-inferrer.cc",
"src/parsing/func-name-inferrer.h",
"src/parsing/import-attributes.cc",
"src/parsing/import-attributes.h",
"src/parsing/import-assertions.cc",
"src/parsing/import-assertions.h",
"src/parsing/keywords-gen.h",
"src/parsing/literal-buffer.cc",
"src/parsing/literal-buffer.h",
Expand Down
4 changes: 2 additions & 2 deletions BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -4065,7 +4065,7 @@ v8_header_set("v8_internal_headers") {
"src/objects/waiter-queue-node.h",
"src/parsing/expression-scope.h",
"src/parsing/func-name-inferrer.h",
"src/parsing/import-attributes.h",
"src/parsing/import-assertions.h",
"src/parsing/keywords-gen.h",
"src/parsing/literal-buffer.h",
"src/parsing/parse-info.h",
Expand Down Expand Up @@ -5585,7 +5585,7 @@ v8_source_set("v8_base_without_compiler") {
"src/objects/visitors.cc",
"src/objects/waiter-queue-node.cc",
"src/parsing/func-name-inferrer.cc",
"src/parsing/import-attributes.cc",
"src/parsing/import-assertions.cc",
"src/parsing/literal-buffer.cc",
"src/parsing/parse-info.cc",
"src/parsing/parser.cc",
Expand Down
2 changes: 1 addition & 1 deletion include/v8-isolate.h
Original file line number Diff line number Diff line change
Expand Up @@ -585,7 +585,7 @@ class V8_EXPORT Isolate {
kDurationFormat = 117,
kInvalidatedNumberStringNotRegexpLikeProtector = 118,
kOBSOLETE_RegExpUnicodeSetIncompatibilitiesWithUnicodeMode = 119,
kOBSOLETE_ImportAssertionDeprecatedSyntax = 120,
kImportAssertionDeprecatedSyntax = 120,
kLocaleInfoObsoletedGetters = 121,
kLocaleInfoFunctions = 122,
kCompileHintsMagicAll = 123,
Expand Down
2 changes: 1 addition & 1 deletion include/v8-script.h
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ class V8_EXPORT ModuleRequest : public Data {
*/
Local<FixedArray> GetImportAttributes() const;

V8_DEPRECATED("Use GetImportAttributes instead")
V8_DEPRECATE_SOON("Use GetImportAttributes instead")
Local<FixedArray> GetImportAssertions() const {
return GetImportAttributes();
}
Expand Down
2 changes: 1 addition & 1 deletion src/ast/modules.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#ifndef V8_AST_MODULES_H_
#define V8_AST_MODULES_H_

#include "src/parsing/import-attributes.h"
#include "src/parsing/import-assertions.h"
#include "src/parsing/scanner.h" // Only for Scanner::Location.
#include "src/zone/zone-containers.h"

Expand Down
9 changes: 6 additions & 3 deletions src/common/message-template.h
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,10 @@ namespace internal {
T(IllegalInvocation, "Illegal invocation") \
T(ImmutablePrototypeSet, \
"Immutable prototype object '%' cannot have their prototype set") \
T(ImportAttributesDuplicateKey, "Import attribute has duplicate key '%'") \
T(ImportAssertDeprecated, \
"'assert' is deprecated in import statements and support will be removed " \
"in %; use 'with' instead") \
T(ImportAssertionDuplicateKey, "Import assertion has duplicate key '%'") \
T(ImportCallNotNewExpression, "Cannot use new with import") \
T(ImportOutsideModule, "Cannot use import statement outside a module") \
T(ImportMetaOutsideModule, "Cannot use 'import.meta' outside a module") \
Expand Down Expand Up @@ -161,7 +164,7 @@ namespace internal {
T(NonCoercibleWithProperty, \
"Cannot destructure property '%' of '%' as it is %.") \
T(NonExtensibleProto, "% is not extensible") \
T(NonObjectAttributesOption, "The 'with' option must be an object") \
T(NonObjectAssertOption, "The 'assert' option must be an object") \
T(NonObjectInInstanceOfCheck, \
"Right-hand side of 'instanceof' is not an object") \
T(NonObjectPrivateNameAccess, "Cannot access private name % from %") \
Expand All @@ -173,7 +176,7 @@ namespace internal {
"Cannot set properties of % (setting '%')") \
T(NonObjectImportArgument, \
"The second argument to import() must be an object") \
T(NonStringImportAttributeValue, "Import attribute value must be a string") \
T(NonStringImportAssertionValue, "Import assertion value must be a string") \
T(NoSetterInCallback, "Cannot set property % of % which has only a getter") \
T(NotAnIterator, "% is not an iterator") \
T(PromiseNewTargetUndefined, \
Expand Down
68 changes: 47 additions & 21 deletions src/execution/isolate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6533,7 +6533,8 @@ MaybeDirectHandle<FixedArray> Isolate::GetImportAttributesFromArgument(

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

if (!IsJSReceiver(*import_options_argument)) {
this->Throw(
Expand All @@ -6557,62 +6558,87 @@ MaybeDirectHandle<FixedArray> Isolate::GetImportAttributesFromArgument(
}
}

// If there is no 'with' option in the options bag, it's not an error. Just do
// the import() as if no attributes were provided.
if (v8_flags.harmony_import_assertions &&
(!v8_flags.harmony_import_attributes ||
IsUndefined(*import_attributes_object))) {
Handle<Name> assert_key = factory()->assert_string();
if (!JSReceiver::GetProperty(this, import_options_argument_receiver,
assert_key)
.ToHandle(&import_attributes_object)) {
// This can happen if the property has a getter function that throws
// an error.
return MaybeHandle<FixedArray>();
}

if (V8_UNLIKELY(!IsUndefined(*import_attributes_object))) {
MessageLocation* location = nullptr;
MessageLocation computed_location;
if (ComputeLocation(&computed_location)) {
location = &computed_location;
}
DirectHandle<JSMessageObject> message = MessageHandler::MakeMessageObject(
this, MessageTemplate::kImportAssertDeprecated, location,
factory()->NewStringFromAsciiChecked("V8 v12.6 and Chrome 126"));
message->set_error_level(v8::Isolate::kMessageWarning);
MessageHandler::ReportMessage(this, location, message);
}
}

// 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 (IsUndefined(*import_attributes_object)) return import_attributes_array;

if (!IsJSReceiver(*import_attributes_object)) {
this->Throw(
*factory()->NewTypeError(MessageTemplate::kNonObjectAttributesOption));
return MaybeDirectHandle<FixedArray>();
*factory()->NewTypeError(MessageTemplate::kNonObjectAssertOption));
return MaybeHandle<FixedArray>();
}

DirectHandle<JSReceiver> import_attributes_object_receiver =
Cast<JSReceiver>(import_attributes_object);

DirectHandle<FixedArray> attribute_keys;
Handle<FixedArray> assertion_keys;
if (!KeyAccumulator::GetKeys(this, import_attributes_object_receiver,
KeyCollectionMode::kOwnOnly, ENUMERABLE_STRINGS,
GetKeysConversion::kConvertToString)
.ToHandle(&attribute_keys)) {
// This happens if the attributes object is a Proxy whose ownKeys() or
.ToHandle(&assertion_keys)) {
// This happens if the assertions object is a Proxy whose ownKeys() or
// getOwnPropertyDescriptor() trap throws.
return MaybeDirectHandle<FixedArray>();
}

bool has_non_string_attribute = false;

// The attributes will be passed to the host in the form: [key1,
// The assertions will be passed to the host in the form: [key1,
// value1, key2, value2, ...].
constexpr size_t kAttributeEntrySizeForDynamicImport = 2;
import_attributes_array = factory()->NewFixedArray(static_cast<int>(
attribute_keys->length() * kAttributeEntrySizeForDynamicImport));
for (int i = 0; i < attribute_keys->length(); i++) {
DirectHandle<String> attribute_key(Cast<String>(attribute_keys->get(i)),
this);
DirectHandle<Object> attribute_value;
assertion_keys->length() * kAttributeEntrySizeForDynamicImport));
for (int i = 0; i < assertion_keys->length(); i++) {
Handle<String> assertion_key(Cast<String>(assertion_keys->get(i)), this);
Handle<Object> assertion_value;
if (!Object::GetPropertyOrElement(this, import_attributes_object_receiver,
attribute_key)
.ToHandle(&attribute_value)) {
assertion_key)
.ToHandle(&assertion_value)) {
// This can happen if the property has a getter function that throws
// an error.
return MaybeDirectHandle<FixedArray>();
}

if (!IsString(*attribute_value)) {
if (!IsString(*assertion_value)) {
has_non_string_attribute = true;
}

import_attributes_array->set((i * kAttributeEntrySizeForDynamicImport),
*attribute_key);
*assertion_key);
import_attributes_array->set((i * kAttributeEntrySizeForDynamicImport) + 1,
*attribute_value);
*assertion_value);
}

if (has_non_string_attribute) {
this->Throw(*factory()->NewTypeError(
MessageTemplate::kNonStringImportAttributeValue));
return MaybeDirectHandle<FixedArray>();
MessageTemplate::kNonStringImportAssertionValue));
return MaybeHandle<FixedArray>();
}

return import_attributes_array;
Expand Down
3 changes: 2 additions & 1 deletion src/flags/flag-definitions.h
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,8 @@ DEFINE_BOOL(js_shipping, true, "enable all shipped JavaScript features")
#define HARMONY_INPROGRESS_BASE(V) \
V(harmony_temporal, "Temporal") \
V(harmony_shadow_realm, "harmony ShadowRealm") \
V(harmony_struct, "harmony structs, shared structs, and shared arrays")
V(harmony_struct, "harmony structs, shared structs, and shared arrays") \
V(harmony_import_assertions, "harmony import assertions (deprecated)")

#define JAVASCRIPT_INPROGRESS_FEATURES_BASE(V) \
V(js_decorators, "decorators") \
Expand Down
1 change: 1 addition & 0 deletions src/init/bootstrapper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5506,6 +5506,7 @@ void Genesis::InitializeConsole(DirectHandle<JSObject> extras_binding) {
#define EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(id) \
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(js_regexp_modifiers)
EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(js_regexp_duplicate_named_groups)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "src/parsing/import-attributes.h"
#include "src/parsing/import-assertions.h"

#include "src/ast/ast-value-factory.h"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef V8_PARSING_IMPORT_ATTRIBUTES_H_
#define V8_PARSING_IMPORT_ATTRIBUTES_H_
#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"
Expand All @@ -29,4 +29,4 @@ class ImportAttributes
} // namespace internal
} // namespace v8

#endif // V8_PARSING_IMPORT_ATTRIBUTES_H_
#endif // V8_PARSING_IMPORT_ASSERTIONS_H_
8 changes: 5 additions & 3 deletions src/parsing/parser-base.h
Original file line number Diff line number Diff line change
Expand Up @@ -4193,15 +4193,17 @@ ParserBase<Impl>::ParseImportExpressions() {
v8_flags.js_source_phase_imports);
// TODO(42204365): Enable import attributes with source phase import once
// specified.
if (v8_flags.harmony_import_attributes &&
phase == ModuleImportPhase::kEvaluation && Check(Token::kComma)) {
const bool check_import_attributes = (v8_flags.harmony_import_assertions ||
v8_flags.harmony_import_attributes) &&
phase == ModuleImportPhase::kEvaluation;
if (check_import_attributes && Check(Token::kComma)) {
if (Check(Token::kRightParen)) {
// A trailing comma allowed after the specifier.
return factory()->NewImportCallExpression(specifier, phase, pos);
} else {
ExpressionT import_options = ParseAssignmentExpressionCoverGrammar();
Check(Token::kComma); // A trailing comma is allowed after the import
// attributes.
// assertions.
Expect(Token::kRightParen);
return factory()->NewImportCallExpression(specifier, phase,
import_options, pos);
Expand Down
18 changes: 17 additions & 1 deletion src/parsing/parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1446,10 +1446,26 @@ ImportAttributes* Parser::ParseImportWithOrAssertClause() {
// LiteralPropertyName
// LiteralPropertyName ':' StringLiteral , WithEntries

// (DEPRECATED)
// AssertClause :
// assert '{' '}'
// assert '{' WithEntries ','? '}'

auto import_attributes = zone()->New<ImportAttributes>(zone());

if (v8_flags.harmony_import_attributes && Check(Token::kWith)) {
// '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];
info_->pending_error_handler()->ReportWarningAt(
position(), end_position(), MessageTemplate::kImportAssertDeprecated,
"V8 v12.6 and Chrome 126");
} else {
return import_attributes;
}
Expand All @@ -1475,7 +1491,7 @@ ImportAttributes* Parser::ParseImportWithOrAssertClause() {
attribute_key, std::make_pair(attribute_value, location)));
if (!result.second) {
// It is a syntax error if two WithEntries have the same key.
ReportMessageAt(location, MessageTemplate::kImportAttributesDuplicateKey,
ReportMessageAt(location, MessageTemplate::kImportAssertionDuplicateKey,
attribute_key);
break;
}
Expand Down
2 changes: 1 addition & 1 deletion src/parsing/parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
#include "src/base/small-vector.h"
#include "src/base/threaded-list.h"
#include "src/common/globals.h"
#include "src/parsing/import-attributes.h"
#include "src/parsing/import-assertions.h"
#include "src/parsing/parse-info.h"
#include "src/parsing/parser-base.h"
#include "src/parsing/parsing.h"
Expand Down
10 changes: 5 additions & 5 deletions test/cctest/test-api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26438,7 +26438,7 @@ TEST(DynamicImport) {
}

v8::MaybeLocal<v8::Promise>
HostImportModuleDynamicallyWithAttributesCallbackResolve(
HostImportModuleDynamicallyWithAssertionsCallbackResolve(
Local<v8::Context> context, Local<v8::Data> host_defined_options,
Local<v8::Value> resource_name, Local<v8::String> specifier,
Local<v8::FixedArray> import_attributes) {
Expand Down Expand Up @@ -26487,14 +26487,14 @@ HostImportModuleDynamicallyWithAttributesCallbackResolve(
return resolver->GetPromise();
}

TEST(DynamicImportWithAttributes) {
FLAG_SCOPE(harmony_import_attributes);
TEST(DynamicImportWithAssertions) {
FLAG_SCOPE(harmony_import_assertions);

LocalContext context;
v8::Isolate* isolate = context->GetIsolate();
v8::HandleScope scope(isolate);
isolate->SetHostImportModuleDynamicallyCallback(
HostImportModuleDynamicallyWithAttributesCallbackResolve);
HostImportModuleDynamicallyWithAssertionsCallbackResolve);

i::DirectHandle<i::String> url =
v8::Utils::OpenDirectHandle(*v8_str("www.google.com"));
Expand All @@ -26504,7 +26504,7 @@ TEST(DynamicImportWithAttributes) {
i::DirectHandle<i::String> source(v8::Utils::OpenHandle(*v8_str("foo")));
v8::Local<v8::Object> import_options =
CompileRun(
"var arg = { with: { 'b': 'w', aa: 'x', c: 'y', a: 'z'} };"
"var arg = { assert: { 'b': 'w', aa: 'x', c: 'y', a: 'z'} };"
"arg;")
->ToObject(context.local())
.ToLocalChecked();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@
//
// MODULE
//
// Flags: --harmony-import-assertions

import "modules-skip-1-import-attributes-fail.mjs" with { type: "notARealType"}
import "modules-skip-1-import-assertions-fail.mjs" assert { type: "notARealType"}
6 changes: 6 additions & 0 deletions test/message/fail/modules-import-assertions-fail-1.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
*%(basename)s:9: 'assert' is deprecated in import statements and support will be removed in V8 v12.6 and Chrome 126; use 'with' instead
undefined:0: Error: Invalid module type was asserted


Error: Invalid module type was asserted

Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@
//
// MODULE
//
// Flags: --harmony-import-assertions

import "modules-skip-1-import-attributes-fail.mjs" with { type: "json"}
import "modules-skip-1-import-assertions-fail.mjs" assert { type: "json"}
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
*%(basename)s:9: 'assert' is deprecated in import statements and support will be removed in V8 v12.6 and Chrome 126; use 'with' instead
undefined:1: SyntaxError: Unexpected token '/', "// Copyrig"... is not valid JSON
// Copyright 2021 the V8 project authors. All rights reserved.
^
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@
//
// MODULE
//
// Flags: --harmony-import-assertions

import "modules-skip-3-import-attributes-fail.json"
import "modules-skip-3-import-assertions-fail.json"
4 changes: 4 additions & 0 deletions test/message/fail/modules-import-assertions-fail-3.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
*modules-skip-3-import-assertions-fail.json:1: SyntaxError: Unexpected token ':'
{ "life": 42 }
^
SyntaxError: Unexpected token ':'
5 changes: 0 additions & 5 deletions test/message/fail/modules-import-attributes-fail-1.out

This file was deleted.

Loading

0 comments on commit 8b325d6

Please sign in to comment.