From fb2c92931084ee31c1c3605314595e364baa8706 Mon Sep 17 00:00:00 2001 From: Dan Klishch Date: Wed, 24 Jan 2024 14:45:11 -0500 Subject: [PATCH] LibJS: Don't use null DFS in {Import,Export}Entry --- Userland/Libraries/LibJS/AST.cpp | 4 +-- Userland/Libraries/LibJS/AST.h | 32 +++++++++---------- Userland/Libraries/LibJS/Parser.cpp | 15 ++++----- Userland/Libraries/LibJS/SourceTextModule.cpp | 24 +++++++------- 4 files changed, 36 insertions(+), 39 deletions(-) diff --git a/Userland/Libraries/LibJS/AST.cpp b/Userland/Libraries/LibJS/AST.cpp index 640ada3fde..571d7b83c1 100644 --- a/Userland/Libraries/LibJS/AST.cpp +++ b/Userland/Libraries/LibJS/AST.cpp @@ -1530,8 +1530,8 @@ void ExportStatement::dump(int indent) const print_indent(indent + 1); outln("(ExportEntries)"); - auto string_or_null = [](ByteString const& string) -> ByteString { - if (string.is_empty()) { + auto string_or_null = [](Optional const& string) -> ByteString { + if (!string.has_value()) { return "null"; } return ByteString::formatted("\"{}\"", string); diff --git a/Userland/Libraries/LibJS/AST.h b/Userland/Libraries/LibJS/AST.h index 879a778c13..2a948413df 100644 --- a/Userland/Libraries/LibJS/AST.h +++ b/Userland/Libraries/LibJS/AST.h @@ -350,18 +350,17 @@ private: // ImportEntry Record, https://tc39.es/ecma262/#table-importentry-record-fields struct ImportEntry { - DeprecatedFlyString import_name; // [[ImportName]] if a String - DeprecatedFlyString local_name; // [[LocalName]] - bool is_namespace { false }; // [[ImportName]] if `namespace-object` + Optional import_name; // [[ImportName]]: stored string if Optional is not empty, NAMESPACE-OBJECT otherwise + DeprecatedFlyString local_name; // [[LocalName]] - ImportEntry(DeprecatedFlyString import_name_, DeprecatedFlyString local_name_, bool is_namespace_ = false) + ImportEntry(Optional import_name_, DeprecatedFlyString local_name_) : import_name(move(import_name_)) , local_name(move(local_name_)) - , is_namespace(is_namespace_) { - VERIFY(!is_namespace || import_name.is_null()); } + bool is_namespace() const { return !import_name.has_value(); } + ModuleRequest const& module_request() const { VERIFY(m_module_request); @@ -370,7 +369,7 @@ struct ImportEntry { private: friend class ImportStatement; - ModuleRequest* m_module_request; // [[ModuleRequest]] + ModuleRequest* m_module_request = nullptr; // [[ModuleRequest]] }; class ImportStatement final : public Statement { @@ -410,10 +409,10 @@ struct ExportEntry { EmptyNamedExport, } kind; - DeprecatedFlyString export_name; // [[ExportName]] - DeprecatedFlyString local_or_import_name; // Either [[ImportName]] or [[LocalName]] + Optional export_name; // [[ExportName]] + Optional local_or_import_name; // Either [[ImportName]] or [[LocalName]] - ExportEntry(Kind export_kind, DeprecatedFlyString export_name_, DeprecatedFlyString local_or_import_name_) + ExportEntry(Kind export_kind, Optional export_name_, Optional local_or_import_name_) : kind(export_kind) , export_name(move(export_name_)) , local_or_import_name(move(local_or_import_name_)) @@ -425,7 +424,7 @@ struct ExportEntry { return m_module_request != nullptr; } - static ExportEntry indirect_export_entry(ModuleRequest const& module_request, DeprecatedFlyString export_name, DeprecatedFlyString import_name) + static ExportEntry indirect_export_entry(ModuleRequest const& module_request, Optional export_name, Optional import_name) { ExportEntry entry { Kind::NamedExport, move(export_name), move(import_name) }; entry.m_module_request = &module_request; @@ -468,16 +467,16 @@ class ExportStatement final : public Statement { public: static DeprecatedFlyString local_name_for_default; - ExportStatement(SourceRange source_range, RefPtr statement, Vector entries, bool is_default_export, ModuleRequest module_request) + ExportStatement(SourceRange source_range, RefPtr statement, Vector entries, bool is_default_export, Optional module_request) : Statement(move(source_range)) , m_statement(move(statement)) , m_entries(move(entries)) , m_is_default_export(is_default_export) , m_module_request(move(module_request)) { - if (!m_module_request.module_specifier.is_null()) { + if (m_module_request.has_value()) { for (auto& entry : m_entries) - entry.m_module_request = &m_module_request; + entry.m_module_request = &m_module_request.value(); } } @@ -500,15 +499,14 @@ public: ModuleRequest const& module_request() const { - VERIFY(!m_module_request.module_specifier.is_null()); - return m_module_request; + return m_module_request.value(); } private: RefPtr m_statement; Vector m_entries; bool m_is_default_export { false }; - ModuleRequest m_module_request; + Optional m_module_request; }; class Program final : public ScopeNode { diff --git a/Userland/Libraries/LibJS/Parser.cpp b/Userland/Libraries/LibJS/Parser.cpp index 5282ce619f..afbf3a8f92 100644 --- a/Userland/Libraries/LibJS/Parser.cpp +++ b/Userland/Libraries/LibJS/Parser.cpp @@ -783,7 +783,7 @@ void Parser::parse_module(Program& program) found = true; })); for (auto& import : program.imports()) { - if (import->has_bound_name(exported_name)) { + if (import->has_bound_name(exported_name.value())) { found = true; break; } @@ -4624,7 +4624,7 @@ NonnullRefPtr Parser::parse_import_statement(Program& pro if (match_imported_binding()) { auto namespace_position = position(); auto namespace_name = consume().value(); - entries_with_location.append({ ImportEntry({}, namespace_name, true), namespace_position }); + entries_with_location.append({ ImportEntry({}, namespace_name), namespace_position }); } else { syntax_error(ByteString::formatted("Unexpected token: {}", m_state.current_token.name())); } @@ -4746,14 +4746,14 @@ NonnullRefPtr Parser::parse_export_statement(Program& pro RefPtr expression = {}; bool is_default = false; - ModuleRequest from_specifier; + Optional from_specifier; if (match_default()) { is_default = true; auto default_position = position(); consume(TokenType::Default); - DeprecatedFlyString local_name; + Optional local_name; auto lookahead_token = next_token(); @@ -4856,11 +4856,10 @@ NonnullRefPtr Parser::parse_export_statement(Program& pro local_name = "!!invalid!!"; } - if (local_name.is_null()) { + if (!local_name.has_value()) local_name = ExportStatement::local_name_for_default; - } - entries_with_location.append({ ExportEntry::named_export(default_string_value, move(local_name)), default_position }); + entries_with_location.append({ ExportEntry::named_export(default_string_value, local_name.release_value()), default_position }); } else { enum class FromSpecifier { NotAllowed, @@ -5001,7 +5000,7 @@ NonnullRefPtr Parser::parse_export_statement(Program& pro for (auto& entry : entries_with_location) { for (auto& export_statement : program.exports()) { - if (export_statement->has_export(entry.entry.export_name)) + if (export_statement->has_export(entry.entry.export_name.value_or(""))) syntax_error(ByteString::formatted("Duplicate export with name: '{}'", entry.entry.export_name), entry.position); } diff --git a/Userland/Libraries/LibJS/SourceTextModule.cpp b/Userland/Libraries/LibJS/SourceTextModule.cpp index 563fe975c7..9c5fd5a41b 100644 --- a/Userland/Libraries/LibJS/SourceTextModule.cpp +++ b/Userland/Libraries/LibJS/SourceTextModule.cpp @@ -206,7 +206,7 @@ Result, Vector> SourceTextModule::pa auto& import_entry = *in_imported_bound_names; // 2. If ie.[[ImportName]] is namespace-object, then - if (import_entry.is_namespace) { + if (import_entry.is_namespace()) { // a. NOTE: This is a re-export of an imported module namespace object. // b. Append ee to localExportEntries. local_export_entries.empend(export_entry); @@ -222,7 +222,7 @@ Result, Vector> SourceTextModule::pa // b. Else if ee.[[ImportName]] is all-but-default, then else if (export_entry.kind == ExportEntry::Kind::ModuleRequestAllButDefault) { // i. Assert: ee.[[ExportName]] is null. - VERIFY(export_entry.export_name.is_null()); + VERIFY(!export_entry.export_name.has_value()); // ii. Append ee to starExportEntries. star_export_entries.empend(export_entry); } @@ -289,10 +289,10 @@ ThrowCompletionOr> SourceTextModule::get_exported_na // FIXME: How do we check that? // b. Assert: e.[[ExportName]] is not null. - VERIFY(!entry.export_name.is_null()); + VERIFY(entry.export_name.has_value()); // c. Append e.[[ExportName]] to exportedNames. - exported_names.empend(entry.export_name); + exported_names.empend(entry.export_name.value()); } // 7. For each ExportEntry Record e of module.[[IndirectExportEntries]], do @@ -301,10 +301,10 @@ ThrowCompletionOr> SourceTextModule::get_exported_na // FIXME: How do we check that? // b. Assert: e.[[ExportName]] is not null. - VERIFY(!entry.export_name.is_null()); + VERIFY(entry.export_name.has_value()); // c. Append e.[[ExportName]] to exportedNames. - exported_names.empend(entry.export_name); + exported_names.empend(entry.export_name.value()); } // 8. For each ExportEntry Record e of module.[[StarExportEntries]], do @@ -339,7 +339,7 @@ ThrowCompletionOr SourceTextModule::initialize_environment(VM& vm) // 1. For each ExportEntry Record e of module.[[IndirectExportEntries]], do for (auto& entry : m_indirect_export_entries) { // a. Let resolution be ? module.ResolveExport(e.[[ExportName]]). - auto resolution = TRY(resolve_export(vm, entry.export_name)); + auto resolution = TRY(resolve_export(vm, entry.export_name.value())); // b. If resolution is null or ambiguous, throw a SyntaxError exception. if (!resolution.is_valid()) return vm.throw_completion(ErrorType::InvalidOrAmbiguousExportEntry, entry.export_name); @@ -369,7 +369,7 @@ ThrowCompletionOr SourceTextModule::initialize_environment(VM& vm) // b. NOTE: The above call cannot fail because imported module requests are a subset of module.[[RequestedModules]], and these have been resolved earlier in this algorithm. // c. If in.[[ImportName]] is namespace-object, then - if (import_entry.is_namespace) { + if (import_entry.is_namespace()) { // i. Let namespace be ? GetModuleNamespace(importedModule). auto* namespace_ = TRY(imported_module->get_module_namespace(vm)); @@ -382,7 +382,7 @@ ThrowCompletionOr SourceTextModule::initialize_environment(VM& vm) // d. Else, else { // i. Let resolution be ? importedModule.ResolveExport(in.[[ImportName]]). - auto resolution = TRY(imported_module->resolve_export(vm, import_entry.import_name)); + auto resolution = TRY(imported_module->resolve_export(vm, import_entry.import_name.value())); // ii. If resolution is null or ambiguous, throw a SyntaxError exception. if (!resolution.is_valid()) @@ -520,7 +520,7 @@ ThrowCompletionOr SourceTextModule::initialize_environment(VM& vm) dbgln_if(JS_MODULE_DEBUG, "[JS MODULE] Adding default export to lexical declarations: local name: {}, Expression: {}", name, statement.class_name()); // 1. Perform ! env.CreateMutableBinding(dn, false). - MUST(environment->create_mutable_binding(vm, name, false)); + MUST(environment->create_mutable_binding(vm, name.value(), false)); // Note: Since this is not a function declaration 24.a.iii never applies } @@ -569,7 +569,7 @@ ThrowCompletionOr SourceTextModule::resolve_export(VM& vm, Depr return ResolvedBinding { ResolvedBinding::Type::BindingName, this, - entry.local_or_import_name, + entry.local_or_import_name.value(), }; } @@ -601,7 +601,7 @@ ThrowCompletionOr SourceTextModule::resolve_export(VM& vm, Depr // FIXME: What does this mean? / How do we check this // 2. Return ? importedModule.ResolveExport(e.[[ImportName]], resolveSet). - return imported_module->resolve_export(vm, entry.local_or_import_name, resolve_set); + return imported_module->resolve_export(vm, entry.local_or_import_name.value(), resolve_set); } }