From 04b4307b3d57a4c48abfa19a22d4745b23760d78 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Wed, 20 Oct 2021 12:10:23 -0400 Subject: [PATCH] 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. 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 | 33 +++++-------- .../LibJS/Runtime/IteratorOperations.cpp | 48 ++++++------------- .../LibJS/Runtime/IteratorOperations.h | 8 ++-- .../LibJS/Runtime/MapConstructor.cpp | 30 +++++------- .../LibJS/Runtime/ObjectConstructor.cpp | 36 +++++--------- .../LibJS/Runtime/SetConstructor.cpp | 12 ++--- .../LibJS/Runtime/WeakMapConstructor.cpp | 30 +++++------- .../LibJS/Runtime/WeakSetConstructor.cpp | 12 ++--- 8 files changed, 74 insertions(+), 135 deletions(-) diff --git a/Userland/Libraries/LibJS/AST.cpp b/Userland/Libraries/LibJS/AST.cpp index a715fa384b..1d055097ac 100644 --- a/Userland/Libraries/LibJS/AST.cpp +++ b/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 { 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 { + 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(*element)) { - get_iterator_values(global_object, value, [&](Value iterator_value) { + TRY_OR_DISCARD(get_iterator_values(global_object, value, [&](Value iterator_value) -> Optional { array->indexed_properties().put(index++, iterator_value, default_attributes); - return IterationDecision::Continue; - }); - if (interpreter.exception()) return {}; + })); continue; } } diff --git a/Userland/Libraries/LibJS/Runtime/IteratorOperations.cpp b/Userland/Libraries/LibJS/Runtime/IteratorOperations.cpp index 0d3f69bc1e..6271189b29 100644 --- a/Userland/Libraries/LibJS/Runtime/IteratorOperations.cpp +++ b/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 { values.append(value); - return IterationDecision::Continue; + return {}; }, - method, CloseOnAbrupt::No); + method); + return values; } -void get_iterator_values(GlobalObject& global_object, Value value, Function 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* next_object = TRY(iterator_step(global_object, *iterator)); + if (!next_object) + return {}; - auto done_property_or_error = next_object->get(vm.names.done); - if (done_property_or_error.is_error()) - return; + auto next_value = TRY(iterator_value(global_object, *next_object)); - 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; + if (auto completion = callback(next_value); completion.has_value()) { + iterator_close(*iterator); + return completion.release_value(); } - VERIFY(result == IterationDecision::Continue); } } diff --git a/Userland/Libraries/LibJS/Runtime/IteratorOperations.h b/Userland/Libraries/LibJS/Runtime/IteratorOperations.h index 8844a5a3a8..8338983bf7 100644 --- a/Userland/Libraries/LibJS/Runtime/IteratorOperations.h +++ b/Userland/Libraries/LibJS/Runtime/IteratorOperations.h @@ -7,6 +7,7 @@ #pragma once #include +#include #include #include @@ -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 callback, Value method = {}, CloseOnAbrupt close_on_abrupt = CloseOnAbrupt::Yes); +using IteratorValueCallback = Function(Value)>; +Completion get_iterator_values(GlobalObject& global_object, Value iterable, IteratorValueCallback callback, Value method = {}); } diff --git a/Userland/Libraries/LibJS/Runtime/MapConstructor.cpp b/Userland/Libraries/LibJS/Runtime/MapConstructor.cpp index 24a0829a28..3744e7ff53 100644 --- a/Userland/Libraries/LibJS/Runtime/MapConstructor.cpp +++ b/Userland/Libraries/LibJS/Runtime/MapConstructor.cpp @@ -59,26 +59,18 @@ Value MapConstructor::construct(FunctionObject& new_target) vm.throw_exception(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(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 { + if (!iterator_value.is_object()) + return vm.throw_completion(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; } diff --git a/Userland/Libraries/LibJS/Runtime/ObjectConstructor.cpp b/Userland/Libraries/LibJS/Runtime/ObjectConstructor.cpp index 0dbf0d3922..4c8fefdf42 100644 --- a/Userland/Libraries/LibJS/Runtime/ObjectConstructor.cpp +++ b/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(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 { + if (!iterator_value.is_object()) + return vm.throw_completion(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; } diff --git a/Userland/Libraries/LibJS/Runtime/SetConstructor.cpp b/Userland/Libraries/LibJS/Runtime/SetConstructor.cpp index fa10df35fc..0bd301d452 100644 --- a/Userland/Libraries/LibJS/Runtime/SetConstructor.cpp +++ b/Userland/Libraries/LibJS/Runtime/SetConstructor.cpp @@ -59,14 +59,12 @@ Value SetConstructor::construct(FunctionObject& new_target) vm.throw_exception(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 { + TRY(vm.call(adder.as_function(), Value(set), iterator_value)); return {}; + })); + return set; } diff --git a/Userland/Libraries/LibJS/Runtime/WeakMapConstructor.cpp b/Userland/Libraries/LibJS/Runtime/WeakMapConstructor.cpp index 5f844af019..2eed46b414 100644 --- a/Userland/Libraries/LibJS/Runtime/WeakMapConstructor.cpp +++ b/Userland/Libraries/LibJS/Runtime/WeakMapConstructor.cpp @@ -57,26 +57,18 @@ Value WeakMapConstructor::construct(FunctionObject& new_target) vm.throw_exception(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(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 { + if (!iterator_value.is_object()) + return vm.throw_completion(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; } diff --git a/Userland/Libraries/LibJS/Runtime/WeakSetConstructor.cpp b/Userland/Libraries/LibJS/Runtime/WeakSetConstructor.cpp index 85ac2a78e0..926ac3ec78 100644 --- a/Userland/Libraries/LibJS/Runtime/WeakSetConstructor.cpp +++ b/Userland/Libraries/LibJS/Runtime/WeakSetConstructor.cpp @@ -57,14 +57,12 @@ Value WeakSetConstructor::construct(FunctionObject& new_target) vm.throw_exception(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 { + TRY(vm.call(adder.as_function(), Value(weak_set), iterator_value)); return {}; + })); + return weak_set; }