diff --git a/Libraries/LibJS/AST.cpp b/Libraries/LibJS/AST.cpp index 86e13095f5..ca3a723774 100644 --- a/Libraries/LibJS/AST.cpp +++ b/Libraries/LibJS/AST.cpp @@ -25,7 +25,6 @@ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -#include #include #include #include @@ -37,6 +36,7 @@ #include #include #include +#include #include #include #include @@ -162,23 +162,14 @@ Value CallExpression::execute(Interpreter& interpreter, GlobalObject& global_obj if (interpreter.exception()) return {}; if (m_arguments[i].is_spread) { - // FIXME: Support generic iterables - if (value.is_string()) { - for (auto ch : value.as_string().string()) - arguments.append(Value(js_string(interpreter, String::format("%c", ch)))); - } else if (value.is_object() && value.as_object().is_array()) { - auto& array = static_cast(value.as_object()); - for (auto& entry : array.indexed_properties()) { - arguments.append(entry.value_and_attributes(&array).value); - if (interpreter.exception()) - return {}; - } - } else if (value.is_object() && value.as_object().is_string_object()) { - for (auto ch : static_cast(value.as_object()).primitive_string().string()) - arguments.append(Value(js_string(interpreter, String::format("%c", ch)))); - } else { - interpreter.throw_exception(ErrorType::NotIterable, value.to_string_without_side_effects().characters()); - } + get_iterator_values(global_object, value, [&](Value& iterator_value) { + if (interpreter.exception()) + return IterationDecision::Break; + arguments.append(iterator_value); + return IterationDecision::Continue; + }); + if (interpreter.exception()) + return {}; } else { arguments.append(value); } @@ -425,53 +416,30 @@ Value ForOfStatement::execute(Interpreter& interpreter, GlobalObject& global_obj auto rhs_result = m_rhs->execute(interpreter, global_object); if (interpreter.exception()) return {}; - // FIXME: We need to properly implement the iterator protocol - auto is_iterable = rhs_result.is_array() || rhs_result.is_string() || (rhs_result.is_object() && rhs_result.as_object().is_string_object()); - if (!is_iterable) - return interpreter.throw_exception(ErrorType::ForOfNotIterable); - size_t index = 0; - auto next = [&]() -> Optional { - if (rhs_result.is_array()) { - auto& array_elements = rhs_result.as_object().indexed_properties(); - if (index < array_elements.array_like_size()) { - auto result = array_elements.get(&rhs_result.as_object(), index); - if (interpreter.exception()) - return {}; - return result.value().value; - } - } else if (rhs_result.is_string()) { - auto string = rhs_result.as_string().string(); - if (index < string.length()) - return js_string(interpreter, string.substring(index, 1)); - } else if (rhs_result.is_object() && rhs_result.as_object().is_string_object()) { - auto string = static_cast(&rhs_result.as_object())->primitive_string().string(); - if (index < string.length()) - return js_string(interpreter, string.substring(index, 1)); - } - return {}; - }; - - for (;;) { - auto next_item = next(); - if (!next_item.has_value()) - break; - interpreter.set_variable(variable_name, next_item.value(), global_object); + get_iterator_values(global_object, rhs_result, [&](Value& value) { + interpreter.set_variable(variable_name, value, global_object); last_value = interpreter.run(global_object, *m_body); if (interpreter.exception()) - return {}; + return IterationDecision::Break; if (interpreter.should_unwind()) { if (interpreter.should_unwind_until(ScopeType::Continuable, m_label)) { interpreter.stop_unwind(); } else if (interpreter.should_unwind_until(ScopeType::Breakable, m_label)) { interpreter.stop_unwind(); - break; + return IterationDecision::Break; } else { - return js_undefined(); + return IterationDecision::Break; } } - ++index; - } + return IterationDecision::Continue; + }); + + if (interpreter.exception()) + return {}; + + if (interpreter.should_unwind()) + return js_undefined(); return last_value; } @@ -1672,34 +1640,17 @@ Value ArrayExpression::execute(Interpreter& interpreter, GlobalObject& global_ob auto value = Value(); if (element) { value = element->execute(interpreter, global_object); - if (interpreter.exception()) return {}; if (element->is_spread_expression()) { - // FIXME: Support arbitrary iterables - if (value.is_array()) { - auto& array_to_spread = static_cast(value.as_object()); - for (auto& entry : array_to_spread.indexed_properties()) { - array->indexed_properties().append(entry.value_and_attributes(&array_to_spread).value); - if (interpreter.exception()) - return {}; - } - continue; - } - if (value.is_string() || (value.is_object() && value.as_object().is_string_object())) { - String string_to_spread; - if (value.is_string()) { - string_to_spread = value.as_string().string(); - } else { - string_to_spread = static_cast(value.as_object()).primitive_string().string(); - } - for (size_t i = 0; i < string_to_spread.length(); ++i) - array->indexed_properties().append(js_string(interpreter, string_to_spread.substring(i, 1))); - continue; - } - interpreter.throw_exception(ErrorType::NotIterable, value.to_string_without_side_effects().characters()); - return {}; + get_iterator_values(global_object, value, [&](Value& iterator_value) { + array->indexed_properties().append(iterator_value); + return IterationDecision::Continue; + }); + if (interpreter.exception()) + return {}; + continue; } } array->indexed_properties().append(value); diff --git a/Libraries/LibJS/Runtime/ErrorTypes.h b/Libraries/LibJS/Runtime/ErrorTypes.h index 0c356f53f9..48a63e2b91 100644 --- a/Libraries/LibJS/Runtime/ErrorTypes.h +++ b/Libraries/LibJS/Runtime/ErrorTypes.h @@ -40,7 +40,6 @@ M(Convert, "Cannot convert %s to %s") \ M(ConvertUndefinedToObject, "Cannot convert undefined to object") \ M(DescChangeNonConfigurable, "Cannot change attributes of non-configurable property '%s'") \ - M(ForOfNotIterable, "for..of right-hand side must be iterable") \ M(FunctionArgsNotObject, "Argument array must be an object") \ M(InOperatorWithObject, "'in' operator must be used on an object") \ M(InstanceOfOperatorBadPrototype, "Prototype property of %s is not an object") \ @@ -48,6 +47,9 @@ M(InvalidLeftHandAssignment, "Invalid left-hand side in assignment") \ M(IsNotA, "%s is not a %s") \ M(IsNotAEvaluatedFrom, "%s is not a %s (evaluated from '%s')") \ + M(IterableNextBadReturn, "iterator.next() returned a non-object value") \ + M(IterableNextNotAFunction, "'next' property on returned object from Symbol.iterator method is " \ + "not a function") \ M(JsonBigInt, "Cannot serialize BigInt value to JSON") \ M(JsonCircular, "Cannot stringify circular object") \ M(JsonMalformed, "Malformed JSON string") \ diff --git a/Libraries/LibJS/Runtime/IteratorOperations.cpp b/Libraries/LibJS/Runtime/IteratorOperations.cpp index b5f98239af..ab8659f108 100644 --- a/Libraries/LibJS/Runtime/IteratorOperations.cpp +++ b/Libraries/LibJS/Runtime/IteratorOperations.cpp @@ -25,39 +25,51 @@ */ #include +#include +#include #include namespace JS { -Object* get_iterator(Object& obj, String hint, Value method) +Object* get_iterator(GlobalObject& global_object, Value value, String hint, Value method) { - auto& interpreter = obj.interpreter(); + auto& interpreter = global_object.interpreter(); ASSERT(hint == "sync" || hint == "async"); if (method.is_empty()) { if (hint == "async") TODO(); - method = obj.get(obj.interpreter().well_known_symbol_iterator()); + auto object = value.to_object(interpreter, global_object); + if (!object) + return {}; + method = object->get(interpreter.well_known_symbol_iterator()); if (interpreter.exception()) return {}; } - if (!method.is_function()) - TODO(); - auto iterator = interpreter.call(method.as_function(), &obj); + if (!method.is_function()) { + interpreter.throw_exception(ErrorType::NotIterable, value.to_string_without_side_effects().characters()); + return nullptr; + } + auto iterator = interpreter.call(method.as_function(), value); if (interpreter.exception()) return {}; - if (!iterator.is_object()) - TODO(); + if (!iterator.is_object()) { + interpreter.throw_exception(ErrorType::NotIterable, value.to_string_without_side_effects().characters()); + return nullptr; + } return &iterator.as_object(); } -Value iterator_next(Object& iterator, Value value) +Object* iterator_next(Object& iterator, Value value) { auto& interpreter = iterator.interpreter(); auto next_method = iterator.get("next"); if (interpreter.exception()) return {}; - ASSERT(next_method.is_function()); + if (!next_method.is_function()) { + interpreter.throw_exception(ErrorType::IterableNextNotAFunction); + return nullptr; + } Value result; if (value.is_empty()) { @@ -70,37 +82,12 @@ Value iterator_next(Object& iterator, Value value) if (interpreter.exception()) return {}; - if (!result.is_object()) - TODO(); + if (!result.is_object()) { + interpreter.throw_exception(ErrorType::IterableNextBadReturn); + return nullptr; + } - return result; -} - -bool is_iterator_complete(Object& iterator_result) -{ - auto done = iterator_result.get("done"); - if (iterator_result.interpreter().exception()) - return false; - return done.to_boolean(); -} - -Value iterator_value(Object& iterator_result) -{ - return iterator_result.get("value"); -} - -Value iterator_step(Object& iterator) -{ - auto& interpreter = iterator.interpreter(); - auto result = iterator_next(iterator); - if (interpreter.exception()) - return {}; - auto done = is_iterator_complete(result.as_object()); - if (interpreter.exception()) - return {}; - if (done) - return Value(false); - return result; + return &result.as_object(); } void iterator_close(Object& iterator) @@ -117,4 +104,35 @@ Value create_iterator_result_object(Interpreter& interpreter, GlobalObject& glob return object; } +void get_iterator_values(GlobalObject& global_object, Value value, AK::Function callback) +{ + auto& interpreter = global_object.interpreter(); + + auto iterator = get_iterator(global_object, value); + if (!iterator) + return; + + while (true) { + auto next_object = iterator_next(*iterator); + if (!next_object) + return; + + auto done_property = next_object->get("done"); + if (interpreter.exception()) + return; + + if (!done_property.is_empty() && done_property.to_boolean()) + return; + + auto next_value = next_object->get("value"); + if (interpreter.exception()) + return; + + auto result = callback(next_value); + if (result == IterationDecision::Break) + return; + ASSERT(result == IterationDecision::Continue); + } +} + } diff --git a/Libraries/LibJS/Runtime/IteratorOperations.h b/Libraries/LibJS/Runtime/IteratorOperations.h index 9b9caae4e9..35280bceb9 100644 --- a/Libraries/LibJS/Runtime/IteratorOperations.h +++ b/Libraries/LibJS/Runtime/IteratorOperations.h @@ -26,6 +26,7 @@ #pragma once +#include #include namespace JS { @@ -33,13 +34,13 @@ namespace JS { // Common iterator operations defined in ECMA262 7.4 // https://tc39.es/ecma262/#sec-operations-on-iterator-objects -Object* get_iterator(Object& obj, String hint = "sync", Value method = {}); +Object* get_iterator(GlobalObject&, Value value, String hint = "sync", Value method = {}); bool is_iterator_complete(Object& iterator_result); Value create_iterator_result_object(Interpreter&, GlobalObject&, Value value, bool done); -Value iterator_next(Object& iterator, Value value = {}); -Value iterator_value(Object& iterator_result); -Value iterator_step(Object& iterator); +Object* iterator_next(Object& iterator, Value value = {}); void iterator_close(Object& iterator); +void get_iterator_values(GlobalObject&, Value value, AK::Function callback); + } diff --git a/Libraries/LibJS/Tests/functions/function-spread.js b/Libraries/LibJS/Tests/functions/function-spread.js index 3cbd848b9c..fcfcd9cd48 100644 --- a/Libraries/LibJS/Tests/functions/function-spread.js +++ b/Libraries/LibJS/Tests/functions/function-spread.js @@ -13,6 +13,24 @@ test("basic functionality", () => { expect(foo(..."abc")).toBe("c"); }); +test("spreading custom iterable", () => { + let o = { + [Symbol.iterator]() { + return { + i: 0, + next() { + if (this.i++ === 3) { + return { done: true }; + } + return { value: this.i }; + }, + }; + }, + }; + + expect(Math.max(...o)).toBe(3); +}); + test("spreading non iterable", () => { expect(() => { [...1]; diff --git a/Libraries/LibJS/Tests/loops/for-of-basic.js b/Libraries/LibJS/Tests/loops/for-of-basic.js index 0a1d38a778..e08d096c68 100644 --- a/Libraries/LibJS/Tests/loops/for-of-basic.js +++ b/Libraries/LibJS/Tests/loops/for-of-basic.js @@ -28,6 +28,59 @@ describe("correct behavior", () => { for (char of "abc"); expect(char).toBe("c"); }); + + test("respects custom Symbol.iterator method", () => { + const o = { + [Symbol.iterator]() { + return { + i: 0, + next() { + if (this.i++ == 3) { + return { done: true }; + } + return { value: this.i, done: false }; + }, + }; + }, + }; + + const a = []; + for (const k of o) { + a.push(k); + } + + expect(a).toEqual([1, 2, 3]); + }); + + test("loops through custom iterator if there is an exception thrown part way through", () => { + // This tests against the way custom iterators used to be implemented, where the values + // were all collected at once before the for-of body was executed, instead of getting + // the values one at a time + const o = { + [Symbol.iterator]() { + return { + i: 0, + next() { + if (this.i++ === 3) { + throw new Error(); + } + return { value: this.i }; + }, + }; + }, + }; + + const a = []; + + try { + for (let k of o) { + a.push(k); + } + expect().fail(); + } catch (e) { + expect(a).toEqual([1, 2, 3]); + } + }); }); describe("errors", () => { @@ -35,13 +88,13 @@ describe("errors", () => { expect(() => { for (const _ of 123) { } - }).toThrowWithMessage(TypeError, "for..of right-hand side must be iterable"); + }).toThrowWithMessage(TypeError, "123 is not iterable"); }); test("right hand side is an object", () => { expect(() => { for (const _ of { foo: 1, bar: 2 }) { } - }).toThrowWithMessage(TypeError, "for..of right-hand side must be iterable"); + }).toThrowWithMessage(TypeError, "[object Object] is not iterable"); }); }); diff --git a/Libraries/LibJS/Tests/object-spread.js b/Libraries/LibJS/Tests/object-spread.js index 1da002cc05..11b33dfad5 100644 --- a/Libraries/LibJS/Tests/object-spread.js +++ b/Libraries/LibJS/Tests/object-spread.js @@ -91,3 +91,22 @@ test("spreading non-spreadable values", () => { }; expect(Object.getOwnPropertyNames(empty)).toHaveLength(0); }); + +test("respects custom Symbol.iterator method", () => { + let o = { + [Symbol.iterator]() { + return { + i: 0, + next() { + if (this.i++ == 3) { + return { done: true }; + } + return { value: this.i, done: false }; + }, + }; + }, + }; + + let a = [...o]; + expect(a).toEqual([1, 2, 3]); +}); diff --git a/Libraries/LibJS/Tests/test-common.js b/Libraries/LibJS/Tests/test-common.js index 3873b2fb99..4f827b6179 100644 --- a/Libraries/LibJS/Tests/test-common.js +++ b/Libraries/LibJS/Tests/test-common.js @@ -200,7 +200,6 @@ class ExpectationError extends Error { toContain(item) { this.__doMatcher(() => { - // FIXME: Iterator check for (let element of this.target) { if (item === element) return; } @@ -211,7 +210,6 @@ class ExpectationError extends Error { toContainEqual(item) { this.__doMatcher(() => { - // FIXME: Iterator check for (let element of this.target) { if (deepEquals(item, element)) return; }