LibJS: Implement IteratorClose with Completions and align to the spec

8 files changed, 98 insertions, 80 deletions

diff --git a/Userland/Libraries/LibJS/Runtime/ArrayConstructor.cpp b/Userland/Libraries/LibJS/Runtime/ArrayConstructor.cpp
index 32bc5740cb..670b4bea60 100644
--- a/Userland/Libraries/LibJS/Runtime/ArrayConstructor.cpp
+++ b/Userland/Libraries/LibJS/Runtime/ArrayConstructor.cpp
@@ -123,9 +123,8 @@ JS_DEFINE_OLD_NATIVE_FUNCTION(ArrayConstructor::from)
         size_t k = 0;
         while (true) {
             if (k >= MAX_ARRAY_LIKE_INDEX) {
-                vm.throw_exception<TypeError>(global_object, ErrorType::ArrayMaxSize);
-                iterator_close(*iterator);
-                return {};
+                auto error = vm.throw_completion<TypeError>(global_object, ErrorType::ArrayMaxSize);
+                return TRY_OR_DISCARD(iterator_close(*iterator, move(error)));
             }
 
             auto* next = TRY_OR_DISCARD(iterator_step(global_object, *iterator));
@@ -139,20 +138,16 @@ JS_DEFINE_OLD_NATIVE_FUNCTION(ArrayConstructor::from)
             Value mapped_value;
             if (map_fn) {
                 auto mapped_value_or_error = vm.call(*map_fn, this_arg, next_value, Value(k));
-                if (mapped_value_or_error.is_error()) {
-                    iterator_close(*iterator);
-                    return {};
-                }
+                if (mapped_value_or_error.is_error())
+                    return TRY_OR_DISCARD(iterator_close(*iterator, mapped_value_or_error.release_error()));
                 mapped_value = mapped_value_or_error.release_value();
             } else {
                 mapped_value = next_value;
             }
 
             auto result_or_error = array_object.create_data_property_or_throw(k, mapped_value);
-            if (result_or_error.is_error()) {
-                iterator_close(*iterator);
-                return {};
-            }
+            if (result_or_error.is_error())
+                return TRY_OR_DISCARD(iterator_close(*iterator, result_or_error.release_error()));
 
             ++k;
         }
diff --git a/Userland/Libraries/LibJS/Runtime/Intl/ListFormat.cpp b/Userland/Libraries/LibJS/Runtime/Intl/ListFormat.cpp
index c44267031f..03a56dfe84 100644
--- a/Userland/Libraries/LibJS/Runtime/Intl/ListFormat.cpp
+++ b/Userland/Libraries/LibJS/Runtime/Intl/ListFormat.cpp
@@ -297,11 +297,10 @@ ThrowCompletionOr<Vector<String>> string_list_from_iterable(GlobalObject& global
             // ii. If Type(nextValue) is not String, then
             if (!next_value.is_string()) {
                 // 1. Let error be ThrowCompletion(a newly created TypeError object).
-                auto completion = vm.throw_completion<TypeError>(global_object, ErrorType::NotAString, next_value);
+                auto error = vm.throw_completion<TypeError>(global_object, ErrorType::NotAString, next_value);
 
                 // 2. Return ? IteratorClose(iteratorRecord, error).
-                iterator_close(*iterator_record);
-                return completion;
+                return iterator_close(*iterator_record, move(error));
             }
 
             // iii. Append nextValue to the end of the List list.
diff --git a/Userland/Libraries/LibJS/Runtime/IteratorOperations.cpp b/Userland/Libraries/LibJS/Runtime/IteratorOperations.cpp
index 6271189b29..a0a1a894ed 100644
--- a/Userland/Libraries/LibJS/Runtime/IteratorOperations.cpp
+++ b/Userland/Libraries/LibJS/Runtime/IteratorOperations.cpp
@@ -9,6 +9,7 @@
 #include <LibJS/Runtime/FunctionObject.h>
 #include <LibJS/Runtime/GlobalObject.h>
 #include <LibJS/Runtime/IteratorOperations.h>
+#include <LibJS/Runtime/TemporaryClearException.h>
 
 namespace JS {
 
@@ -87,45 +88,52 @@ ThrowCompletionOr<Object*> iterator_step(GlobalObject& global_object, Object& it
 }
 
 // 7.4.6 IteratorClose ( iteratorRecord, completion ), https://tc39.es/ecma262/#sec-iteratorclose
-void iterator_close(Object& iterator)
+Completion iterator_close(Object& iterator, Completion completion)
 {
     auto& vm = iterator.vm();
     auto& global_object = iterator.global_object();
 
-    // Emulates `completion` behavior
-    auto* completion_exception = vm.exception();
-    vm.clear_exception();
-    auto unwind_until = vm.unwind_until();
-    auto unwind_until_label = vm.unwind_until_label();
-    vm.stop_unwind();
-    auto restore_completion = [&]() {
-        if (completion_exception)
-            vm.set_exception(*completion_exception);
-        if (unwind_until != ScopeType::None)
-            vm.unwind(unwind_until, unwind_until_label);
-    };
-
-    auto return_method_or_error = Value(&iterator).get_method(global_object, vm.names.return_);
-    Value result;
-    if (!return_method_or_error.is_error()) { // If innerResult.[[Type]] is normal, then
-        auto return_method = return_method_or_error.release_value();
+    // The callers of iterator_close() are often in an exceptional state.
+    // Temporarily clear that exception for invocation(s) to Call.
+    TemporaryClearException clear_exception(vm);
+
+    // 3. Let innerResult be GetMethod(iterator, "return").
+    auto inner_result_or_error = Value(&iterator).get_method(global_object, vm.names.return_);
+    Value inner_result;
+
+    // 4. If innerResult.[[Type]] is normal, then
+    if (!inner_result_or_error.is_error()) {
+        // a. Let return be innerResult.[[Value]].
+        auto* return_method = inner_result_or_error.release_value();
+
+        // b. If return is undefined, return Completion(completion).
         if (!return_method)
-            return restore_completion(); // If return is undefined, return Completion(completion).
+            return completion;
+
+        vm.stop_unwind();
+
+        // c. Set innerResult to Call(return, iterator).
         auto result_or_error = vm.call(*return_method, &iterator);
         if (result_or_error.is_error())
-            return_method_or_error = result_or_error.release_error();
+            inner_result_or_error = result_or_error.release_error();
         else
-            result = result_or_error.release_value();
+            inner_result = result_or_error.release_value();
     }
-    if (completion_exception)
-        return restore_completion(); // If completion.[[Type]] is throw, return Completion(completion).
-    if (return_method_or_error.is_error())
-        return; // If innerResult.[[Type]] is throw, return Completion(innerResult).
-    if (!result.is_object()) {
-        vm.throw_exception<TypeError>(global_object, ErrorType::IterableReturnBadReturn);
-        return; // If Type(innerResult.[[Value]]) is not Object, throw a TypeError exception.
-    }
-    restore_completion(); // Return Completion(completion).
+
+    // 5. If completion.[[Type]] is throw, return Completion(completion).
+    if (completion.is_error())
+        return completion;
+
+    // 6. If innerResult.[[Type]] is throw, return Completion(innerResult).
+    if (inner_result_or_error.is_error())
+        return inner_result_or_error.release_error();
+
+    // 7. If Type(innerResult.[[Value]]) is not Object, throw a TypeError exception.
+    if (!inner_result.is_object())
+        return vm.throw_completion<TypeError>(global_object, ErrorType::IterableReturnBadReturn);
+
+    // 8. Return Completion(completion).
+    return completion;
 }
 
 // 7.4.8 CreateIterResultObject ( value, done ), https://tc39.es/ecma262/#sec-createiterresultobject
@@ -165,10 +173,8 @@ Completion get_iterator_values(GlobalObject& global_object, Value iterable, Iter
 
         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();
-        }
+        if (auto completion = callback(next_value); completion.has_value())
+            return iterator_close(*iterator, completion.release_value());
     }
 }
 
diff --git a/Userland/Libraries/LibJS/Runtime/IteratorOperations.h b/Userland/Libraries/LibJS/Runtime/IteratorOperations.h
index 8338983bf7..385b275250 100644
--- a/Userland/Libraries/LibJS/Runtime/IteratorOperations.h
+++ b/Userland/Libraries/LibJS/Runtime/IteratorOperations.h
@@ -25,7 +25,7 @@ ThrowCompletionOr<Object*> iterator_next(Object& iterator, Value value = {});
 ThrowCompletionOr<Object*> iterator_step(GlobalObject&, Object& iterator);
 ThrowCompletionOr<bool> iterator_complete(GlobalObject&, Object& iterator_result);
 ThrowCompletionOr<Value> iterator_value(GlobalObject&, Object& iterator_result);
-void iterator_close(Object& iterator);
+Completion iterator_close(Object& iterator, Completion completion);
 Object* create_iterator_result_object(GlobalObject&, Value value, bool done);
 MarkedValueList iterable_to_list(GlobalObject&, Value iterable, Value method = {});
 
diff --git a/Userland/Libraries/LibJS/Runtime/PromiseConstructor.cpp b/Userland/Libraries/LibJS/Runtime/PromiseConstructor.cpp
index 3a9feb3b1b..5272d60231 100644
--- a/Userland/Libraries/LibJS/Runtime/PromiseConstructor.cpp
+++ b/Userland/Libraries/LibJS/Runtime/PromiseConstructor.cpp
@@ -302,11 +302,16 @@ JS_DEFINE_OLD_NATIVE_FUNCTION(PromiseConstructor::all)
     auto* iterator_record = iterator_record_or_error.release_value();
 
     auto result = perform_promise_all(global_object, *iterator_record, constructor, promise_capability, promise_resolve);
-    if (vm.exception()) {
-        if (!iterator_record_is_complete(global_object, *iterator_record))
-            iterator_close(*iterator_record);
+    if (auto* exception = vm.exception()) {
+        // FIXME: Once perform_promise_all returns a throw completion, pass that to iterator_close() instead.
+        auto result_error = throw_completion(exception->value());
+
+        if (!iterator_record_is_complete(global_object, *iterator_record)) {
+            TemporaryClearException clear_exception(vm); // iterator_close() may invoke vm.call(), which VERIFYs no exception.
+            result_error = iterator_close(*iterator_record, move(result_error));
+        }
 
-        auto abrupt = if_abrupt_reject_promise(global_object, result, promise_capability);
+        auto abrupt = if_abrupt_reject_promise(global_object, result_error, promise_capability);
         return abrupt.value();
     }
 
@@ -332,11 +337,16 @@ JS_DEFINE_OLD_NATIVE_FUNCTION(PromiseConstructor::all_settled)
     auto* iterator_record = iterator_record_or_error.release_value();
 
     auto result = perform_promise_all_settled(global_object, *iterator_record, constructor, promise_capability, promise_resolve);
-    if (vm.exception()) {
-        if (!iterator_record_is_complete(global_object, *iterator_record))
-            iterator_close(*iterator_record);
+    if (auto* exception = vm.exception()) {
+        // FIXME: Once perform_promise_all_settled returns a throw completion, pass that to iterator_close() instead.
+        auto result_error = throw_completion(exception->value());
+
+        if (!iterator_record_is_complete(global_object, *iterator_record)) {
+            TemporaryClearException clear_exception(vm); // iterator_close() may invoke vm.call(), which VERIFYs no exception.
+            result_error = iterator_close(*iterator_record, move(result_error));
+        }
 
-        auto abrupt = if_abrupt_reject_promise(global_object, result, promise_capability);
+        auto abrupt = if_abrupt_reject_promise(global_object, result_error, promise_capability);
         return abrupt.value();
     }
 
@@ -362,11 +372,16 @@ JS_DEFINE_OLD_NATIVE_FUNCTION(PromiseConstructor::any)
     auto* iterator_record = iterator_record_or_error.release_value();
 
     auto result = perform_promise_any(global_object, *iterator_record, constructor, promise_capability, promise_resolve);
-    if (vm.exception()) {
-        if (!iterator_record_is_complete(global_object, *iterator_record))
-            iterator_close(*iterator_record);
+    if (auto* exception = vm.exception()) {
+        // FIXME: Once perform_promise_any returns a throw completion, pass that to iterator_close() instead.
+        auto result_error = throw_completion(exception->value());
+
+        if (!iterator_record_is_complete(global_object, *iterator_record)) {
+            TemporaryClearException clear_exception(vm); // iterator_close() may invoke vm.call(), which VERIFYs no exception.
+            result_error = iterator_close(*iterator_record, move(result_error));
+        }
 
-        auto abrupt = if_abrupt_reject_promise(global_object, result, promise_capability);
+        auto abrupt = if_abrupt_reject_promise(global_object, result_error, promise_capability);
         return abrupt.value();
     }
 
@@ -392,11 +407,16 @@ JS_DEFINE_OLD_NATIVE_FUNCTION(PromiseConstructor::race)
     auto* iterator_record = iterator_record_or_error.release_value();
 
     auto result = perform_promise_race(global_object, *iterator_record, constructor, promise_capability, promise_resolve);
-    if (vm.exception()) {
-        if (!iterator_record_is_complete(global_object, *iterator_record))
-            iterator_close(*iterator_record);
+    if (auto* exception = vm.exception()) {
+        // FIXME: Once perform_promise_race returns a throw completion, pass that to iterator_close() instead.
+        auto result_error = throw_completion(exception->value());
+
+        if (!iterator_record_is_complete(global_object, *iterator_record)) {
+            TemporaryClearException clear_exception(vm);
+            result_error = iterator_close(*iterator_record, move(result_error)); // iterator_close() may invoke vm.call(), which VERIFYs no exception.
+        }
 
-        auto abrupt = if_abrupt_reject_promise(global_object, result, promise_capability);
+        auto abrupt = if_abrupt_reject_promise(global_object, result_error, promise_capability);
         return abrupt.value();
     }
 
diff --git a/Userland/Libraries/LibJS/Runtime/Temporal/AbstractOperations.cpp b/Userland/Libraries/LibJS/Runtime/Temporal/AbstractOperations.cpp
index ecc42e189e..f2a3c302ec 100644
--- a/Userland/Libraries/LibJS/Runtime/Temporal/AbstractOperations.cpp
+++ b/Userland/Libraries/LibJS/Runtime/Temporal/AbstractOperations.cpp
@@ -63,8 +63,7 @@ ThrowCompletionOr<MarkedValueList> iterable_to_list_of_type(GlobalObject& global
                 // 1. Let completion be ThrowCompletion(a newly created TypeError object).
                 auto completion = vm.throw_completion<TypeError>(global_object, ErrorType::IterableToListOfTypeInvalidValue, next_value.to_string_without_side_effects());
                 // 2. Return ? IteratorClose(iteratorRecord, completion).
-                iterator_close(*iterator_record);
-                return completion;
+                return iterator_close(*iterator_record, move(completion));
             }
             // iii. Append nextValue to the end of the List values.
             values.append(next_value);
diff --git a/Userland/Libraries/LibJS/Runtime/Temporal/CalendarPrototype.cpp b/Userland/Libraries/LibJS/Runtime/Temporal/CalendarPrototype.cpp
index 8b3ce39613..36bdab819c 100644
--- a/Userland/Libraries/LibJS/Runtime/Temporal/CalendarPrototype.cpp
+++ b/Userland/Libraries/LibJS/Runtime/Temporal/CalendarPrototype.cpp
@@ -513,31 +513,28 @@ JS_DEFINE_OLD_NATIVE_FUNCTION(CalendarPrototype::fields)
         // ii. If Type(nextValue) is not String, then
         if (!next_value.is_string()) {
             // 1. Let completion be ThrowCompletion(a newly created TypeError object).
-            vm.throw_exception<TypeError>(global_object, ErrorType::TemporalInvalidCalendarFieldValue, next_value.to_string_without_side_effects());
+            auto completion = vm.throw_completion<TypeError>(global_object, ErrorType::TemporalInvalidCalendarFieldValue, next_value.to_string_without_side_effects());
 
             // 2. Return ? IteratorClose(iteratorRecord, completion).
-            iterator_close(*iterator_record);
-            return {};
+            return TRY_OR_DISCARD(iterator_close(*iterator_record, move(completion)));
         }
 
         // iii. If fieldNames contains nextValue, then
         if (field_names.contains_slow(next_value)) {
             // 1. Let completion be ThrowCompletion(a newly created RangeError object).
-            vm.throw_exception<RangeError>(global_object, ErrorType::TemporalDuplicateCalendarField, next_value.as_string().string());
+            auto completion = vm.throw_completion<RangeError>(global_object, ErrorType::TemporalDuplicateCalendarField, next_value.as_string().string());
 
             // 2. Return ? IteratorClose(iteratorRecord, completion).
-            iterator_close(*iterator_record);
-            return {};
+            return TRY_OR_DISCARD(iterator_close(*iterator_record, move(completion)));
         }
 
         // iv. If nextValue is not one of "year", "month", "monthCode", "day", "hour", "minute", "second", "millisecond", "microsecond", "nanosecond", then
         if (!next_value.as_string().string().is_one_of("year"sv, "month"sv, "monthCode"sv, "day"sv, "hour"sv, "minute"sv, "second"sv, "millisecond"sv, "microsecond"sv, "nanosecond"sv)) {
             // 1. Let completion be ThrowCompletion(a newly created RangeError object).
-            vm.throw_exception<RangeError>(global_object, ErrorType::TemporalInvalidCalendarFieldName, next_value.as_string().string());
+            auto completion = vm.throw_completion<RangeError>(global_object, ErrorType::TemporalInvalidCalendarFieldName, next_value.as_string().string());
 
             // 2. Return ? IteratorClose(iteratorRecord, completion).
-            iterator_close(*iterator_record);
-            return {};
+            return TRY_OR_DISCARD(iterator_close(*iterator_record, move(completion)));
         }
 
         // v. Append nextValue to the end of the List fieldNames.
diff --git a/Userland/Libraries/LibJS/Runtime/VM.cpp b/Userland/Libraries/LibJS/Runtime/VM.cpp
index 5e8dc951a8..6e3e3d6a2e 100644
--- a/Userland/Libraries/LibJS/Runtime/VM.cpp
+++ b/Userland/Libraries/LibJS/Runtime/VM.cpp
@@ -202,11 +202,13 @@ ThrowCompletionOr<void> VM::binding_initialization(NonnullRefPtr<BindingPattern>
         auto result = iterator_binding_initialization(*target, iterator, iterator_done, environment, global_object);
 
         if (!iterator_done) {
-            // FIXME: Iterator close should take result and potentially return that. This logic should achieve the same until that is possible.
-            iterator_close(*iterator);
-            if (auto* thrown_exception = exception())
-                return JS::throw_completion(thrown_exception->value());
+            // iterator_close() always returns a Completion, which ThrowCompletionOr will interpret as a throw
+            // completion. So only return the result of iterator_close() if it is indeed a throw completion.
+            auto completion = result.is_throw_completion() ? result.release_error() : normal_completion({});
+            if (completion = iterator_close(*iterator, move(completion)); completion.is_error())
+                return completion.release_error();
         }
+
         return result;
     }
 }