LibJS: Convert get_iterator_values helper to ThrowCompletionOr

This one is a bit unusual, so to clarify:

Previously, callers of get_iterator_values() would supply a callback
that would return an IterationDecision, and an enum to indicate whether
iterator_close() should be invoked upon IterationDecision::Break.

Now, use of both those enums is removed, and callers must return an
Optional<Completion>. If a Completion is provided, the iterator will be
closed, and that completion will be returned from get_iterator_values.
Otherwise, once the iterator is exhausted, a default-initialized
Completion will be returned.

Userland/Libraries/LibJS/AST.cpp

@@ -251,13 +251,11 @@ static void argument_list_evaluation(Interpreter& interpreter, GlobalObject& glo
         if (vm.exception())
             return;
         if (argument.is_spread) {
-            get_iterator_values(global_object, value, [&](Value iterator_value) {
-                if (vm.exception())
-                    return IterationDecision::Break;
+            auto result = get_iterator_values(global_object, value, [&](Value iterator_value) -> Optional<Completion> {
                 list.append(iterator_value);
-                return IterationDecision::Continue;
+                return {};
             });
-            if (vm.exception())
+            if (result.is_error())
                 return;
         } else {
             list.append(value);
@@ -871,29 +869,24 @@ Value ForOfStatement::execute(Interpreter& interpreter, GlobalObject& global_obj
         interpreter.vm().running_execution_context().lexical_environment = old_environment;
     });
 
-    get_iterator_values(global_object, rhs_result, [&](Value value) {
-        auto result = for_of_head_state.execute_head(interpreter, global_object, value);
-        if (result.is_error())
-            return IterationDecision::Break;
+    TRY_OR_DISCARD(get_iterator_values(global_object, rhs_result, [&](Value value) -> Optional<Completion> {
+        TRY(for_of_head_state.execute_head(interpreter, global_object, value));
         last_value = m_body->execute(interpreter, global_object).value_or(last_value);
         interpreter.vm().running_execution_context().lexical_environment = old_environment;
-        if (interpreter.exception())
-            return IterationDecision::Break;
+        if (auto* exception = interpreter.exception())
+            return throw_completion(exception->value());
         if (interpreter.vm().should_unwind()) {
             if (interpreter.vm().should_unwind_until(ScopeType::Continuable, m_labels)) {
                 interpreter.vm().stop_unwind();
             } else if (interpreter.vm().should_unwind_until(ScopeType::Breakable, m_labels)) {
                 interpreter.vm().stop_unwind();
-                return IterationDecision::Break;
+                return normal_completion(last_value);
             } else {
-                return IterationDecision::Break;
+                return normal_completion(last_value);
             }
         }
-        return IterationDecision::Continue;
-    });
-
-    if (interpreter.exception())
         return {};
+    }));
 
     return last_value;
 }
@@ -2885,12 +2878,10 @@ Value ArrayExpression::execute(Interpreter& interpreter, GlobalObject& global_ob
                 return {};
 
             if (is<SpreadExpression>(*element)) {
-                get_iterator_values(global_object, value, [&](Value iterator_value) {
+                TRY_OR_DISCARD(get_iterator_values(global_object, value, [&](Value iterator_value) -> Optional<Completion> {
                     array->indexed_properties().put(index++, iterator_value, default_attributes);
-                    return IterationDecision::Continue;
-                });
-                if (interpreter.exception())
                     return {};
+                }));
                 continue;
             }
         }

Userland/Libraries/LibJS/Runtime/IteratorOperations.cpp

@@ -143,50 +143,32 @@ MarkedValueList iterable_to_list(GlobalObject& global_object, Value iterable, Va
 {
     auto& vm = global_object.vm();
     MarkedValueList values(vm.heap());
-    get_iterator_values(
-        global_object, iterable, [&](auto value) {
-            if (vm.exception())
-                return IterationDecision::Break;
+
+    (void)get_iterator_values(
+        global_object, iterable, [&](auto value) -> Optional<Completion> {
             values.append(value);
-            return IterationDecision::Continue;
+            return {};
         },
-        method, CloseOnAbrupt::No);
+        method);
+
     return values;
 }
 
-void get_iterator_values(GlobalObject& global_object, Value value, Function<IterationDecision(Value)> callback, Value method, CloseOnAbrupt close_on_abrupt)
+Completion get_iterator_values(GlobalObject& global_object, Value iterable, IteratorValueCallback callback, Value method)
 {
-    auto& vm = global_object.vm();
-
-    auto iterator_or_error = get_iterator(global_object, value, IteratorHint::Sync, method);
-    if (iterator_or_error.is_error())
-        return;
-    auto* iterator = iterator_or_error.release_value();
+    auto* iterator = TRY(get_iterator(global_object, iterable, IteratorHint::Sync, method));
 
     while (true) {
-        auto next_object_or_error = iterator_next(*iterator);
-        if (next_object_or_error.is_error())
-            return;
-        auto* next_object = next_object_or_error.release_value();
-
-        auto done_property_or_error = next_object->get(vm.names.done);
-        if (done_property_or_error.is_error())
-            return;
-
-        if (done_property_or_error.release_value().to_boolean())
-            return;
-
-        auto next_value_or_error = next_object->get(vm.names.value);
-        if (next_value_or_error.is_error())
-            return;
-
-        auto result = callback(next_value_or_error.release_value());
-        if (result == IterationDecision::Break) {
-            if (close_on_abrupt == CloseOnAbrupt::Yes)
-                iterator_close(*iterator);
-            return;
+        auto* next_object = TRY(iterator_step(global_object, *iterator));
+        if (!next_object)
+            return {};
+
+        auto next_value = TRY(iterator_value(global_object, *next_object));
+
+        if (auto completion = callback(next_value); completion.has_value()) {
+            iterator_close(*iterator);
+            return completion.release_value();
         }
-        VERIFY(result == IterationDecision::Continue);
     }
 }
 

Userland/Libraries/LibJS/Runtime/IteratorOperations.h

@@ -7,6 +7,7 @@
 #pragma once
 
 #include <AK/Function.h>
+#include <AK/Optional.h>
 #include <LibJS/Runtime/Completion.h>
 #include <LibJS/Runtime/Object.h>
 
@@ -28,10 +29,7 @@ void iterator_close(Object& iterator);
 Object* create_iterator_result_object(GlobalObject&, Value value, bool done);
 MarkedValueList iterable_to_list(GlobalObject&, Value iterable, Value method = {});
 
-enum class CloseOnAbrupt {
-    No,
-    Yes
-};
-void get_iterator_values(GlobalObject&, Value value, Function<IterationDecision(Value)> callback, Value method = {}, CloseOnAbrupt close_on_abrupt = CloseOnAbrupt::Yes);
+using IteratorValueCallback = Function<Optional<Completion>(Value)>;
+Completion get_iterator_values(GlobalObject& global_object, Value iterable, IteratorValueCallback callback, Value method = {});
 
 }

Userland/Libraries/LibJS/Runtime/MapConstructor.cpp

@@ -59,26 +59,18 @@ Value MapConstructor::construct(FunctionObject& new_target)
         vm.throw_exception<TypeError>(global_object, ErrorType::NotAFunction, "'set' property of Map");
         return {};
     }
-    get_iterator_values(global_object, vm.argument(0), [&](Value iterator_value) {
-        if (vm.exception())
-            return IterationDecision::Break;
-        if (!iterator_value.is_object()) {
-            vm.throw_exception<TypeError>(global_object, ErrorType::NotAnObject, String::formatted("Iterator value {}", iterator_value.to_string_without_side_effects()));
-            return IterationDecision::Break;
-        }
-        auto key_or_error = iterator_value.as_object().get(0);
-        if (key_or_error.is_error())
-            return IterationDecision::Break;
-        auto key = key_or_error.release_value();
-        auto value_or_error = iterator_value.as_object().get(1);
-        if (value_or_error.is_error())
-            return IterationDecision::Break;
-        auto value = value_or_error.release_value();
-        (void)vm.call(adder.as_function(), Value(map), key, value);
-        return vm.exception() ? IterationDecision::Break : IterationDecision::Continue;
-    });
-    if (vm.exception())
+
+    TRY_OR_DISCARD(get_iterator_values(global_object, vm.argument(0), [&](Value iterator_value) -> Optional<Completion> {
+        if (!iterator_value.is_object())
+            return vm.throw_completion<TypeError>(global_object, ErrorType::NotAnObject, String::formatted("Iterator value {}", iterator_value.to_string_without_side_effects()));
+
+        auto key = TRY(iterator_value.as_object().get(0));
+        auto value = TRY(iterator_value.as_object().get(1));
+        TRY(vm.call(adder.as_function(), Value(map), key, value));
+
         return {};
+    }));
+
     return map;
 }
 

Userland/Libraries/LibJS/Runtime/ObjectConstructor.cpp

@@ -232,31 +232,19 @@ JS_DEFINE_OLD_NATIVE_FUNCTION(ObjectConstructor::from_entries)
 
     auto* object = Object::create(global_object, global_object.object_prototype());
 
-    get_iterator_values(global_object, iterable, [&](Value iterator_value) {
-        if (vm.exception())
-            return IterationDecision::Break;
-        if (!iterator_value.is_object()) {
-            vm.throw_exception<TypeError>(global_object, ErrorType::NotAnObject, String::formatted("Iterator value {}", iterator_value.to_string_without_side_effects()));
-            return IterationDecision::Break;
-        }
-        auto key_or_error = iterator_value.as_object().get(0);
-        if (key_or_error.is_error())
-            return IterationDecision::Break;
-        auto key = key_or_error.release_value();
-        auto value_or_error = iterator_value.as_object().get(1);
-        if (value_or_error.is_error())
-            return IterationDecision::Break;
-        auto value = value_or_error.release_value();
-        auto property_key_or_error = key.to_property_key(global_object);
-        if (property_key_or_error.is_error())
-            return IterationDecision::Break;
-        auto result_or_error = object->create_data_property_or_throw(property_key_or_error.release_value(), value);
-        if (result_or_error.is_error())
-            return IterationDecision::Break;
-        return IterationDecision::Continue;
-    });
-    if (vm.exception())
+    TRY_OR_DISCARD(get_iterator_values(global_object, iterable, [&](Value iterator_value) -> Optional<Completion> {
+        if (!iterator_value.is_object())
+            return vm.throw_completion<TypeError>(global_object, ErrorType::NotAnObject, String::formatted("Iterator value {}", iterator_value.to_string_without_side_effects()));
+
+        auto key = TRY(iterator_value.as_object().get(0));
+        auto value = TRY(iterator_value.as_object().get(1));
+
+        auto property_key = TRY(key.to_property_key(global_object));
+        MUST(object->create_data_property_or_throw(property_key, value));
+
         return {};
+    }));
+
     return object;
 }
 

Userland/Libraries/LibJS/Runtime/SetConstructor.cpp

@@ -59,14 +59,12 @@ Value SetConstructor::construct(FunctionObject& new_target)
         vm.throw_exception<TypeError>(global_object, ErrorType::NotAFunction, "'add' property of Set");
         return {};
     }
-    get_iterator_values(global_object, vm.argument(0), [&](Value iterator_value) {
-        if (vm.exception())
-            return IterationDecision::Break;
-        (void)vm.call(adder.as_function(), Value(set), iterator_value);
-        return vm.exception() ? IterationDecision::Break : IterationDecision::Continue;
-    });
-    if (vm.exception())
+
+    TRY_OR_DISCARD(get_iterator_values(global_object, vm.argument(0), [&](Value iterator_value) -> Optional<Completion> {
+        TRY(vm.call(adder.as_function(), Value(set), iterator_value));
         return {};
+    }));
+
     return set;
 }
 

Userland/Libraries/LibJS/Runtime/WeakMapConstructor.cpp

@@ -57,26 +57,18 @@ Value WeakMapConstructor::construct(FunctionObject& new_target)
         vm.throw_exception<TypeError>(global_object, ErrorType::NotAFunction, "'set' property of WeakMap");
         return {};
     }
-    get_iterator_values(global_object, vm.argument(0), [&](Value iterator_value) {
-        if (vm.exception())
-            return IterationDecision::Break;
-        if (!iterator_value.is_object()) {
-            vm.throw_exception<TypeError>(global_object, ErrorType::NotAnObject, String::formatted("Iterator value {}", iterator_value.to_string_without_side_effects()));
-            return IterationDecision::Break;
-        }
-        auto key_or_error = iterator_value.as_object().get(0);
-        if (key_or_error.is_error())
-            return IterationDecision::Break;
-        auto key = key_or_error.release_value();
-        auto value_or_error = iterator_value.as_object().get(1);
-        if (value_or_error.is_error())
-            return IterationDecision::Break;
-        auto value = value_or_error.release_value();
-        auto result = vm.call(adder.as_function(), Value(weak_map), key, value);
-        return result.is_error() ? IterationDecision::Break : IterationDecision::Continue;
-    });
-    if (vm.exception())
+
+    TRY_OR_DISCARD(get_iterator_values(global_object, vm.argument(0), [&](Value iterator_value) -> Optional<Completion> {
+        if (!iterator_value.is_object())
+            return vm.throw_completion<TypeError>(global_object, ErrorType::NotAnObject, String::formatted("Iterator value {}", iterator_value.to_string_without_side_effects()));
+
+        auto key = TRY(iterator_value.as_object().get(0));
+        auto value = TRY(iterator_value.as_object().get(1));
+        TRY(vm.call(adder.as_function(), Value(weak_map), key, value));
+
         return {};
+    }));
+
     return weak_map;
 }
 

Userland/Libraries/LibJS/Runtime/WeakSetConstructor.cpp

@@ -57,14 +57,12 @@ Value WeakSetConstructor::construct(FunctionObject& new_target)
         vm.throw_exception<TypeError>(global_object, ErrorType::NotAFunction, "'add' property of WeakSet");
         return {};
     }
-    get_iterator_values(global_object, vm.argument(0), [&](Value iterator_value) {
-        if (vm.exception())
-            return IterationDecision::Break;
-        auto result = vm.call(adder.as_function(), Value(weak_set), iterator_value);
-        return result.is_error() ? IterationDecision::Break : IterationDecision::Continue;
-    });
-    if (vm.exception())
+
+    TRY_OR_DISCARD(get_iterator_values(global_object, vm.argument(0), [&](Value iterator_value) -> Optional<Completion> {
+        TRY(vm.call(adder.as_function(), Value(weak_set), iterator_value));
         return {};
+    }));
+
     return weak_set;
 }