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.

Tests/LibJS/test-js.cpp

@@ -63,7 +63,7 @@ TESTJS_GLOBAL_FUNCTION(mark_as_garbage, markAsGarbage)
     if (!outer_environment.has_value())
         return vm.throw_completion<JS::ReferenceError>(global_object, JS::ErrorType::UnknownIdentifier, variable_name.string());
 
-    auto reference = vm.resolve_binding(variable_name.string(), outer_environment.value()->lexical_environment);
+    auto reference = TRY(vm.resolve_binding(variable_name.string(), outer_environment.value()->lexical_environment));
 
     auto value = TRY(reference.get_value(global_object));
 

Userland/Libraries/LibJS/AST.cpp

@@ -705,7 +705,7 @@ struct ForInOfHeadState {
 
             if (!destructuring) {
                 VERIFY(for_declaration.declarations().first().target().has<NonnullRefPtr<Identifier>>());
-                lhs_reference = interpreter.vm().resolve_binding(for_declaration.declarations().first().target().get<NonnullRefPtr<Identifier>>()->string());
+                lhs_reference = MUST(interpreter.vm().resolve_binding(for_declaration.declarations().first().target().get<NonnullRefPtr<Identifier>>()->string()));
             }
         }
 
@@ -764,7 +764,7 @@ static ThrowCompletionOr<ForInOfHeadState> for_in_of_head_execute(Interpreter& i
             if (variable.init()) {
                 VERIFY(variable.target().has<NonnullRefPtr<Identifier>>());
                 auto& binding_id = variable.target().get<NonnullRefPtr<Identifier>>()->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;

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<void> return
     (void)reference.put_value(interpreter.global_object(), interpreter.accumulator());
 }

Userland/Libraries/LibJS/Runtime/AbstractOperations.cpp

@@ -215,7 +215,8 @@ ThrowCompletionOr<void> 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));

Userland/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp

@@ -441,9 +441,7 @@ ThrowCompletionOr<void> ECMAScriptFunctionObject::function_declaration_instantia
                 Environment* used_environment = has_duplicates ? nullptr : environment;
 
                 if constexpr (IsSame<FlyString const&, decltype(param)>) {
-                    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);

Userland/Libraries/LibJS/Runtime/VM.cpp

@@ -237,7 +237,7 @@ ThrowCompletionOr<void> VM::property_binding_initialization(BindingPattern const
         if (property.is_rest) {
             Reference assignment_target;
             if (auto identifier_ptr = property.name.get_pointer<NonnullRefPtr<Identifier>>()) {
-                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<NonnullRefPtr<MemberExpression>>()) {
                 assignment_target = (*member_ptr)->to_reference(interpreter(), global_object);
             } else {
@@ -282,9 +282,7 @@ ThrowCompletionOr<void> VM::property_binding_initialization(BindingPattern const
         if (property.name.has<NonnullRefPtr<Identifier>>() && property.alias.has<Empty>()) {
             // 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<NonnullRefPtr<Identifier>>();
-            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<void> VM::property_binding_initialization(BindingPattern const
             continue;
         }
 
-        Optional<Reference> reference_to_assign_to;
-
-        property.alias.visit(
-            [&](Empty) {},
-            [&](NonnullRefPtr<Identifier> const& identifier) {
-                reference_to_assign_to = resolve_binding(identifier->string(), environment);
+        auto reference_to_assign_to = TRY(property.alias.visit(
+            [&](Empty) -> ThrowCompletionOr<Optional<Reference>> { return Optional<Reference> {}; },
+            [&](NonnullRefPtr<Identifier> const& identifier) -> ThrowCompletionOr<Optional<Reference>> {
+                return TRY(resolve_binding(identifier->string(), environment));
             },
-            [&](NonnullRefPtr<BindingPattern> const&) {},
-            [&](NonnullRefPtr<MemberExpression> const& member_expression) {
-                reference_to_assign_to = member_expression->to_reference(interpreter(), global_object);
-            });
+            [&](NonnullRefPtr<BindingPattern> const&) -> ThrowCompletionOr<Optional<Reference>> { return Optional<Reference> {}; },
+            [&](NonnullRefPtr<MemberExpression> const& member_expression) -> ThrowCompletionOr<Optional<Reference>> {
+                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<void> VM::iterator_binding_initialization(BindingPattern const
         auto& entry = binding.entries[i];
         Value value;
 
-        Optional<Reference> assignment_target;
-        entry.alias.visit(
-            [&](Empty) {},
-            [&](NonnullRefPtr<Identifier> const& identifier) {
-                assignment_target = resolve_binding(identifier->string(), environment);
+        auto assignment_target = TRY(entry.alias.visit(
+            [&](Empty) -> ThrowCompletionOr<Optional<Reference>> { return Optional<Reference> {}; },
+            [&](NonnullRefPtr<Identifier> const& identifier) -> ThrowCompletionOr<Optional<Reference>> {
+                return TRY(resolve_binding(identifier->string(), environment));
             },
-            [&](NonnullRefPtr<BindingPattern> const&) {},
-            [&](NonnullRefPtr<MemberExpression> 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<BindingPattern> const&) -> ThrowCompletionOr<Optional<Reference>> { return Optional<Reference> {}; },
+            [&](NonnullRefPtr<MemberExpression> const& member_expression) -> ThrowCompletionOr<Optional<Reference>> {
+                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<Reference> 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

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<Reference> resolve_binding(FlyString const&, Environment* = nullptr);
     Reference get_identifier_reference(Environment*, FlyString, bool strict, size_t hops = 0);
 
     template<typename T, typename... Args>

Userland/Utilities/js.cpp

@@ -1427,9 +1427,7 @@ ErrorOr<int> serenity_main(Main::Arguments arguments)
             switch (mode) {
             case CompleteProperty: {
                 Optional<JS::Value> 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());