LibJS: Integrate iterator protocol into language features

Finally use Symbol.iterator protocol in language features :) currently
only used in for-of loops and spread expressions, but will have more
uses later (Maps, Sets, Array.from, etc).

Libraries/LibJS/AST.cpp

@@ -25,7 +25,6 @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#include <AK/Function.h>
 #include <AK/HashMap.h>
 #include <AK/ScopeGuard.h>
 #include <AK/StringBuilder.h>
@@ -37,6 +36,7 @@
 #include <LibJS/Runtime/BigInt.h>
 #include <LibJS/Runtime/Error.h>
 #include <LibJS/Runtime/GlobalObject.h>
+#include <LibJS/Runtime/IteratorOperations.h>
 #include <LibJS/Runtime/MarkedValueList.h>
 #include <LibJS/Runtime/NativeFunction.h>
 #include <LibJS/Runtime/PrimitiveString.h>
@@ -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<Array&>(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<const StringObject&>(value.as_object()).primitive_string().string())
-                    arguments.append(Value(js_string(interpreter, String::format("%c", ch))));
-            } else {
-                interpreter.throw_exception<TypeError>(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<TypeError>(ErrorType::ForOfNotIterable);
-
-    size_t index = 0;
-    auto next = [&]() -> Optional<Value> {
-        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<StringObject*>(&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<Array&>(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<const StringObject&>(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<TypeError>(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);

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")                                                          \

Libraries/LibJS/Runtime/IteratorOperations.cpp

@@ -25,39 +25,51 @@
  */
 
 #include <LibJS/Interpreter.h>
+#include <LibJS/Runtime/Error.h>
+#include <LibJS/Runtime/GlobalObject.h>
 #include <LibJS/Runtime/IteratorOperations.h>
 
 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<TypeError>(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<TypeError>(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<TypeError>(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();
-
-    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();
-}
+    if (!result.is_object()) {
+        interpreter.throw_exception<TypeError>(ErrorType::IterableNextBadReturn);
+        return nullptr;
+    }
 
-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<IterationDecision(Value&)> 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);
+    }
+}
+
 }

Libraries/LibJS/Runtime/IteratorOperations.h

@@ -26,6 +26,7 @@
 
 #pragma once
 
+#include <AK/Function.h>
 #include <LibJS/Runtime/Object.h>
 
 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<IterationDecision(Value&)> callback);
+
 }

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];

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");
     });
 });

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]);
+});

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;
                 }