From 2e81cc4cf77ee7228092b3d00869d218cec92825 Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Thu, 6 Jul 2023 17:49:38 +0200 Subject: [PATCH] LibJS: Use Identifier to represent FunctionParameter name Using identifier instead of string allows to store supplemental information about whether it can be represented as local variable. --- Userland/Libraries/LibJS/AST.cpp | 25 +++++++++++++---- Userland/Libraries/LibJS/AST.h | 13 ++++----- Userland/Libraries/LibJS/Parser.cpp | 27 ++++++++++--------- .../LibJS/Runtime/AbstractOperations.cpp | 2 +- .../Runtime/ECMAScriptFunctionObject.cpp | 14 +++++----- 5 files changed, 50 insertions(+), 31 deletions(-) diff --git a/Userland/Libraries/LibJS/AST.cpp b/Userland/Libraries/LibJS/AST.cpp index e0f2194952..c964378fac 100644 --- a/Userland/Libraries/LibJS/AST.cpp +++ b/Userland/Libraries/LibJS/AST.cpp @@ -2537,11 +2537,14 @@ void FunctionNode::dump(int indent, DeprecatedString const& class_name) const for (auto& parameter : m_parameters) { parameter.binding.visit( - [&](DeprecatedFlyString const& name) { - print_indent(indent + 2); - if (parameter.is_rest) + [&](Identifier const& identifier) { + if (parameter.is_rest) { + print_indent(indent + 2); out("..."); - outln("{}", name); + identifier.dump(0); + } else { + identifier.dump(indent + 2); + } }, [&](BindingPattern const& pattern) { pattern.dump(indent + 2); @@ -4479,6 +4482,16 @@ ThrowCompletionOr ScopeNode::for_each_lexically_declared_name(ThrowComplet return {}; } +ThrowCompletionOr ScopeNode::for_each_lexically_declared_identifier(ThrowCompletionOrVoidCallback&& callback) const +{ + for (auto const& declaration : m_lexical_declarations) { + TRY(declaration->for_each_bound_identifier([&](auto const& identifier) { + return callback(identifier); + })); + } + return {}; +} + ThrowCompletionOr ScopeNode::for_each_var_declared_name(ThrowCompletionOrVoidCallback&& callback) const { for (auto& declaration : m_var_declarations) { @@ -4770,7 +4783,9 @@ ThrowCompletionOr Program::global_declaration_instantiation(VM& vm, Global // 1. Let lexNames be the LexicallyDeclaredNames of script. // 2. Let varNames be the VarDeclaredNames of script. // 3. For each element name of lexNames, do - TRY(for_each_lexically_declared_name([&](DeprecatedFlyString const& name) -> ThrowCompletionOr { + TRY(for_each_lexically_declared_identifier([&](Identifier const& identifier) -> ThrowCompletionOr { + auto const& name = identifier.string(); + // a. If env.HasVarDeclaration(name) is true, throw a SyntaxError exception. if (global_environment.has_var_declaration(name)) return vm.throw_completion(ErrorType::TopLevelVariableAlreadyDeclared, name); diff --git a/Userland/Libraries/LibJS/AST.h b/Userland/Libraries/LibJS/AST.h index 4f60e8eafe..b033e5b783 100644 --- a/Userland/Libraries/LibJS/AST.h +++ b/Userland/Libraries/LibJS/AST.h @@ -308,6 +308,7 @@ public: ThrowCompletionOr for_each_lexically_scoped_declaration(ThrowCompletionOrVoidCallback&& callback) const; ThrowCompletionOr for_each_lexically_declared_name(ThrowCompletionOrVoidCallback&& callback) const; + ThrowCompletionOr for_each_lexically_declared_identifier(ThrowCompletionOrVoidCallback&& callback) const; ThrowCompletionOr for_each_var_declared_name(ThrowCompletionOrVoidCallback&& callback) const; ThrowCompletionOr for_each_var_declared_identifier(ThrowCompletionOrVoidCallback&& callback) const; @@ -665,12 +666,6 @@ struct BindingPattern : RefCounted { Kind kind { Kind::Object }; }; -struct FunctionParameter { - Variant> binding; - RefPtr default_value; - bool is_rest { false }; -}; - class Identifier final : public Expression { public: explicit Identifier(SourceRange source_range, DeprecatedFlyString string) @@ -703,6 +698,12 @@ private: Optional m_local_variable_index; }; +struct FunctionParameter { + Variant, NonnullRefPtr> binding; + RefPtr default_value; + bool is_rest { false }; +}; + class FunctionNode { public: StringView name() const { return m_name ? m_name->string().view() : ""sv; } diff --git a/Userland/Libraries/LibJS/Parser.cpp b/Userland/Libraries/LibJS/Parser.cpp index 3f8a809610..4a07074748 100644 --- a/Userland/Libraries/LibJS/Parser.cpp +++ b/Userland/Libraries/LibJS/Parser.cpp @@ -79,8 +79,8 @@ public: scope_pusher.m_function_parameters = parameters; for (auto& parameter : parameters) { parameter.binding.visit( - [&](DeprecatedFlyString const& name) { - scope_pusher.m_forbidden_lexical_names.set(name); + [&](Identifier const& identifier) { + scope_pusher.m_forbidden_lexical_names.set(identifier.string()); }, [&](NonnullRefPtr const& binding_pattern) { // NOTE: Nothing in the callback throws an exception. @@ -864,7 +864,7 @@ static bool is_strict_reserved_word(StringView str) static bool is_simple_parameter_list(Vector const& parameters) { return all_of(parameters, [](FunctionParameter const& parameter) { - return !parameter.is_rest && parameter.default_value.is_null() && parameter.binding.has(); + return !parameter.is_rest && parameter.default_value.is_null() && parameter.binding.has>(); }); } @@ -939,7 +939,8 @@ RefPtr Parser::try_parse_arrow_function_expression(boo syntax_error("BindingIdentifier may not be 'arguments' or 'eval' in strict mode"); if (is_async && token.value() == "await"sv) syntax_error("'await' is a reserved identifier in async functions"); - parameters.append({ DeprecatedFlyString { token.value() }, {} }); + auto identifier = create_ast_node({ m_source_code, rule_start.position(), position() }, token.DeprecatedFlyString_value()); + parameters.append({ identifier, {} }); } // If there's a newline between the closing paren and arrow it's not a valid arrow function, // ASI should kick in instead (it'll then fail with "Unexpected token Arrow") @@ -1003,8 +1004,8 @@ RefPtr Parser::try_parse_arrow_function_expression(boo if (body->in_strict_mode()) { for (auto& parameter : parameters) { parameter.binding.visit( - [&](DeprecatedFlyString const& name) { - check_identifier_name_for_assignment_validity(name, true); + [&](Identifier const& identifier) { + check_identifier_name_for_assignment_validity(identifier.string(), true); }, [&](auto const&) {}); } @@ -1495,7 +1496,7 @@ NonnullRefPtr Parser::parse_class_expression(bool expect_ // this function does not. // So we use a custom version of SuperCall which doesn't use the @@iterator // method on %Array.prototype% visibly. - DeprecatedFlyString argument_name = "args"; + auto argument_name = create_ast_node({ m_source_code, rule_start.position(), position() }, "args"); auto super_call = create_ast_node( { m_source_code, rule_start.position(), position() }, SuperCall::IsPartOfSyntheticConstructor::Yes, @@ -2675,7 +2676,9 @@ NonnullRefPtr Parser::parse_function_body(Vector parameter_names; for (auto& parameter : parameters) { parameter.binding.visit( - [&](DeprecatedFlyString const& parameter_name) { + [&](Identifier const& identifier) { + auto const& parameter_name = identifier.string(); + check_identifier_name_for_assignment_validity(parameter_name, function_body->in_strict_mode()); if (function_kind == FunctionKind::Generator && parameter_name == "yield"sv) syntax_error("Parameter name 'yield' not allowed in this context"); @@ -2842,7 +2845,7 @@ Vector Parser::parse_formal_parameters(int& function_length, Vector parameters; - auto consume_identifier_or_binding_pattern = [&]() -> Variant> { + auto consume_identifier_or_binding_pattern = [&]() -> Variant, NonnullRefPtr> { if (auto pattern = parse_binding_pattern(AllowDuplicates::No, AllowMemberExpressions::No)) return pattern.release_nonnull(); @@ -2853,8 +2856,8 @@ Vector Parser::parse_formal_parameters(int& function_length, for (auto& parameter : parameters) { bool has_same_name = parameter.binding.visit( - [&](DeprecatedFlyString const& name) { - return name == parameter_name; + [&](Identifier const& identifier) { + return identifier.string() == parameter_name; }, [&](NonnullRefPtr const& bindings) { bool found_duplicate = false; @@ -2882,7 +2885,7 @@ Vector Parser::parse_formal_parameters(int& function_length, syntax_error(message, Position { token.line_number(), token.line_column() }); break; } - return DeprecatedFlyString { token.value() }; + return create_ast_node({ m_source_code, rule_start.position(), position() }, token.DeprecatedFlyString_value()); }; while (match(TokenType::CurlyOpen) || match(TokenType::BracketOpen) || match_identifier() || match(TokenType::TripleDot)) { diff --git a/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp b/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp index 3bb8980a7e..ff8d2787c0 100644 --- a/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp +++ b/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp @@ -1128,7 +1128,7 @@ Object* create_mapped_arguments_object(VM& vm, FunctionObject& function, Vector< VERIFY(formals.size() <= NumericLimits::max()); for (i32 index = static_cast(formals.size()) - 1; index >= 0; --index) { // a. Let name be parameterNames[index]. - auto const& name = formals[index].binding.get(); + auto const& name = formals[index].binding.get>()->string(); // b. If name is not an element of mappedNames, then if (mapped_names.contains(name)) diff --git a/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp b/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp index 8b2ee56294..ef31371a07 100644 --- a/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp +++ b/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp @@ -94,7 +94,7 @@ ECMAScriptFunctionObject::ECMAScriptFunctionObject(DeprecatedFlyString name, Dep return false; if (parameter.default_value) return false; - if (!parameter.binding.template has()) + if (!parameter.binding.template has>()) return false; return true; }); @@ -353,8 +353,8 @@ ThrowCompletionOr ECMAScriptFunctionObject::function_declaration_instantia has_parameter_expressions = true; parameter.binding.visit( - [&](DeprecatedFlyString const& name) { - if (parameter_names.set(name) != AK::HashSetResult::InsertedNewEntry) + [&](Identifier const& identifier) { + if (parameter_names.set(identifier.string()) != AK::HashSetResult::InsertedNewEntry) has_duplicates = true; }, [&](NonnullRefPtr const& pattern) { @@ -362,8 +362,8 @@ ThrowCompletionOr ECMAScriptFunctionObject::function_declaration_instantia has_parameter_expressions = true; // NOTE: Nothing in the callback throws an exception. - MUST(pattern->for_each_bound_name([&](auto& name) { - if (parameter_names.set(name) != AK::HashSetResult::InsertedNewEntry) + MUST(pattern->for_each_bound_identifier([&](auto& identifier) { + if (parameter_names.set(identifier.string()) != AK::HashSetResult::InsertedNewEntry) has_duplicates = true; })); }); @@ -478,8 +478,8 @@ ThrowCompletionOr ECMAScriptFunctionObject::function_declaration_instantia Environment* used_environment = has_duplicates ? nullptr : environment; - if constexpr (IsSame) { - Reference reference = TRY(vm.resolve_binding(param, used_environment)); + if constexpr (IsSame const&, decltype(param)>) { + Reference reference = TRY(vm.resolve_binding(param->string(), used_environment)); // Here the difference from hasDuplicates is important if (has_duplicates) return reference.put_value(vm, argument_value);