From 676554d3f86d7c429f1969942190427666b5423e Mon Sep 17 00:00:00 2001 From: davidot Date: Thu, 30 Dec 2021 14:13:20 +0100 Subject: LibJS: Convert resolve_binding() to ThrowCompletionOr The spec has a note stating that resolve binding will always return a reference whose [[ReferencedName]] field is name. However this is not correct as the underlying method GetIdentifierReference may throw on env.HasBinding(name) thus it can throw. However, there are some scenarios where it cannot throw because the reference is known to exist in that case we use MUST with a comment. --- Userland/Libraries/LibJS/AST.cpp | 6 +-- Userland/Libraries/LibJS/Bytecode/Op.cpp | 15 +++++-- .../Libraries/LibJS/Runtime/AbstractOperations.cpp | 3 +- .../LibJS/Runtime/ECMAScriptFunctionObject.cpp | 4 +- Userland/Libraries/LibJS/Runtime/VM.cpp | 50 ++++++++++------------ Userland/Libraries/LibJS/Runtime/VM.h | 2 +- Userland/Utilities/js.cpp | 4 +- 7 files changed, 43 insertions(+), 41 deletions(-) (limited to 'Userland') diff --git a/Userland/Libraries/LibJS/AST.cpp b/Userland/Libraries/LibJS/AST.cpp index 997579d9fd..e0ee8d5346 100644 --- a/Userland/Libraries/LibJS/AST.cpp +++ b/Userland/Libraries/LibJS/AST.cpp @@ -705,7 +705,7 @@ struct ForInOfHeadState { if (!destructuring) { VERIFY(for_declaration.declarations().first().target().has>()); - lhs_reference = interpreter.vm().resolve_binding(for_declaration.declarations().first().target().get>()->string()); + lhs_reference = MUST(interpreter.vm().resolve_binding(for_declaration.declarations().first().target().get>()->string())); } } @@ -764,7 +764,7 @@ static ThrowCompletionOr for_in_of_head_execute(Interpreter& i if (variable.init()) { VERIFY(variable.target().has>()); auto& binding_id = variable.target().get>()->string(); - auto reference = interpreter.vm().resolve_binding(binding_id); + auto reference = TRY(interpreter.vm().resolve_binding(binding_id)); if (auto* exception = interpreter.exception()) return throw_completion(exception->value()); @@ -1142,7 +1142,7 @@ Reference Identifier::to_reference(Interpreter& interpreter, GlobalObject&) cons m_cached_environment_coordinate = {}; } - auto reference = interpreter.vm().resolve_binding(string()); + auto reference = TRY_OR_DISCARD(interpreter.vm().resolve_binding(string())); if (reference.environment_coordinate().has_value()) m_cached_environment_coordinate = reference.environment_coordinate(); return reference; diff --git a/Userland/Libraries/LibJS/Bytecode/Op.cpp b/Userland/Libraries/LibJS/Bytecode/Op.cpp index b2ea9b97f7..d22e20e49f 100644 --- a/Userland/Libraries/LibJS/Bytecode/Op.cpp +++ b/Userland/Libraries/LibJS/Bytecode/Op.cpp @@ -252,7 +252,13 @@ void GetVariable::execute_impl(Bytecode::Interpreter& interpreter) const m_cached_environment_coordinate = {}; } - auto reference = interpreter.vm().resolve_binding(string); + auto reference_or_error = interpreter.vm().resolve_binding(string); + if (reference_or_error.is_throw_completion()) { + interpreter.vm().throw_exception(interpreter.global_object(), reference_or_error.release_error().value()); + return Reference {}; + } + + auto reference = reference_or_error.release_value(); if (reference.environment_coordinate().has_value()) m_cached_environment_coordinate = reference.environment_coordinate(); return reference; @@ -270,10 +276,13 @@ void GetVariable::execute_impl(Bytecode::Interpreter& interpreter) const void SetVariable::execute_impl(Bytecode::Interpreter& interpreter) const { auto& vm = interpreter.vm(); - auto reference = vm.resolve_binding(interpreter.current_executable().get_identifier(m_identifier)); - if (vm.exception()) + auto reference_or_error = vm.resolve_binding(interpreter.current_executable().get_identifier(m_identifier)); + if (reference_or_error.is_throw_completion()) { + interpreter.vm().throw_exception(interpreter.global_object(), reference_or_error.release_error().value()); return; + } + auto reference = reference_or_error.release_value(); // TODO: ThrowCompletionOr return (void)reference.put_value(interpreter.global_object(), interpreter.accumulator()); } diff --git a/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp b/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp index 52b272327b..1ba178ae94 100644 --- a/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp +++ b/Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp @@ -215,7 +215,8 @@ ThrowCompletionOr initialize_bound_name(GlobalObject& global_object, FlySt // 2. Else, else { // a. Let lhs be ResolveBinding(name). - auto lhs = vm.resolve_binding(name); + // NOTE: Although the spec pretends resolve_binding cannot fail it can just not in this case. + auto lhs = MUST(vm.resolve_binding(name)); // b. Return ? PutValue(lhs, value). return TRY(lhs.put_value(global_object, value)); diff --git a/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp b/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp index 560f955d40..5f44bf8be3 100644 --- a/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp +++ b/Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp @@ -441,9 +441,7 @@ ThrowCompletionOr ECMAScriptFunctionObject::function_declaration_instantia Environment* used_environment = has_duplicates ? nullptr : environment; if constexpr (IsSame) { - Reference reference = vm.resolve_binding(param, used_environment); - if (auto* exception = vm.exception()) - return throw_completion(exception->value()); + Reference reference = TRY(vm.resolve_binding(param, used_environment)); // Here the difference from hasDuplicates is important if (has_duplicates) return reference.put_value(global_object(), argument_value); diff --git a/Userland/Libraries/LibJS/Runtime/VM.cpp b/Userland/Libraries/LibJS/Runtime/VM.cpp index b4ac310959..0a94ee27e6 100644 --- a/Userland/Libraries/LibJS/Runtime/VM.cpp +++ b/Userland/Libraries/LibJS/Runtime/VM.cpp @@ -237,7 +237,7 @@ ThrowCompletionOr VM::property_binding_initialization(BindingPattern const if (property.is_rest) { Reference assignment_target; if (auto identifier_ptr = property.name.get_pointer>()) { - assignment_target = resolve_binding((*identifier_ptr)->string(), environment); + assignment_target = TRY(resolve_binding((*identifier_ptr)->string(), environment)); } else if (auto member_ptr = property.alias.get_pointer>()) { assignment_target = (*member_ptr)->to_reference(interpreter(), global_object); } else { @@ -282,9 +282,7 @@ ThrowCompletionOr VM::property_binding_initialization(BindingPattern const if (property.name.has>() && property.alias.has()) { // FIXME: this branch and not taking this have a lot in common we might want to unify it more (like it was before). auto& identifier = *property.name.get>(); - auto reference = resolve_binding(identifier.string(), environment); - if (auto* thrown_exception = exception()) - return JS::throw_completion(thrown_exception->value()); + auto reference = TRY(resolve_binding(identifier.string(), environment)); auto value_to_assign = TRY(object->get(name)); if (property.initializer && value_to_assign.is_undefined()) { @@ -298,17 +296,15 @@ ThrowCompletionOr VM::property_binding_initialization(BindingPattern const continue; } - Optional reference_to_assign_to; - - property.alias.visit( - [&](Empty) {}, - [&](NonnullRefPtr const& identifier) { - reference_to_assign_to = resolve_binding(identifier->string(), environment); + auto reference_to_assign_to = TRY(property.alias.visit( + [&](Empty) -> ThrowCompletionOr> { return Optional {}; }, + [&](NonnullRefPtr const& identifier) -> ThrowCompletionOr> { + return TRY(resolve_binding(identifier->string(), environment)); }, - [&](NonnullRefPtr const&) {}, - [&](NonnullRefPtr const& member_expression) { - reference_to_assign_to = member_expression->to_reference(interpreter(), global_object); - }); + [&](NonnullRefPtr const&) -> ThrowCompletionOr> { return Optional {}; }, + [&](NonnullRefPtr const& member_expression) -> ThrowCompletionOr> { + return member_expression->to_reference(interpreter(), global_object); + })); if (auto* thrown_exception = exception()) return JS::throw_completion(thrown_exception->value()); @@ -347,19 +343,15 @@ ThrowCompletionOr VM::iterator_binding_initialization(BindingPattern const auto& entry = binding.entries[i]; Value value; - Optional assignment_target; - entry.alias.visit( - [&](Empty) {}, - [&](NonnullRefPtr const& identifier) { - assignment_target = resolve_binding(identifier->string(), environment); + auto assignment_target = TRY(entry.alias.visit( + [&](Empty) -> ThrowCompletionOr> { return Optional {}; }, + [&](NonnullRefPtr const& identifier) -> ThrowCompletionOr> { + return TRY(resolve_binding(identifier->string(), environment)); }, - [&](NonnullRefPtr const&) {}, - [&](NonnullRefPtr const& member_expression) { - assignment_target = member_expression->to_reference(interpreter(), global_object); - }); - - if (auto* thrown_exception = exception()) - return JS::throw_completion(thrown_exception->value()); + [&](NonnullRefPtr const&) -> ThrowCompletionOr> { return Optional {}; }, + [&](NonnullRefPtr const& member_expression) -> ThrowCompletionOr> { + return member_expression->to_reference(interpreter(), global_object); + })); if (entry.is_rest) { VERIFY(i == binding.entries.size() - 1); @@ -456,7 +448,7 @@ Reference VM::get_identifier_reference(Environment* environment, FlyString name, } // 9.4.2 ResolveBinding ( name [ , env ] ), https://tc39.es/ecma262/#sec-resolvebinding -Reference VM::resolve_binding(FlyString const& name, Environment* environment) +ThrowCompletionOr VM::resolve_binding(FlyString const& name, Environment* environment) { // 1. If env is not present or if env is undefined, then if (!environment) { @@ -472,6 +464,10 @@ Reference VM::resolve_binding(FlyString const& name, Environment* environment) // 4. Return ? GetIdentifierReference(env, name, strict). return get_identifier_reference(environment, name, strict); + + // NOTE: The spec says: + // Note: The result of ResolveBinding is always a Reference Record whose [[ReferencedName]] field is name. + // But this is not actually correct as GetIdentifierReference (or really the methods it calls) can throw. } // 7.3.33 InitializeInstanceElements ( O, constructor ), https://tc39.es/ecma262/#sec-initializeinstanceelements diff --git a/Userland/Libraries/LibJS/Runtime/VM.h b/Userland/Libraries/LibJS/Runtime/VM.h index 1b7f9e2fde..d10c1ac5a0 100644 --- a/Userland/Libraries/LibJS/Runtime/VM.h +++ b/Userland/Libraries/LibJS/Runtime/VM.h @@ -192,7 +192,7 @@ public: ScopeType unwind_until() const { return m_unwind_until; } FlyString unwind_until_label() const { return m_unwind_until_label; } - Reference resolve_binding(FlyString const&, Environment* = nullptr); + ThrowCompletionOr resolve_binding(FlyString const&, Environment* = nullptr); Reference get_identifier_reference(Environment*, FlyString, bool strict, size_t hops = 0); template diff --git a/Userland/Utilities/js.cpp b/Userland/Utilities/js.cpp index 480eaba48e..421b45e76b 100644 --- a/Userland/Utilities/js.cpp +++ b/Userland/Utilities/js.cpp @@ -1427,9 +1427,7 @@ ErrorOr serenity_main(Main::Arguments arguments) switch (mode) { case CompleteProperty: { Optional maybe_value; - auto maybe_variable = vm->resolve_binding(variable_name, &global_environment); - if (vm->exception()) - break; + auto maybe_variable = TRY_OR_DISCARD(vm->resolve_binding(variable_name, &global_environment)); maybe_value = TRY_OR_DISCARD(maybe_variable.get_value(interpreter->global_object())); VERIFY(!maybe_value->is_empty()); -- cgit v1.2.3