LibJS: Restructure and fully implement BindingPatterns

Userland/Libraries/LibJS/AST.cpp

@@ -1133,29 +1133,41 @@ void BindingPattern::dump(int indent) const
 {
     print_indent(indent);
     outln("BindingPattern {}", kind == Kind::Array ? "Array" : "Object");
-    print_indent(++indent);
-    outln("(Properties)");
-    for (auto& property : properties) {
+
+    for (auto& entry : entries) {
         print_indent(indent + 1);
+        outln("(Property)");
+
+        if (kind == Kind::Object) {
+            print_indent(indent + 2);
             outln("(Identifier)");
-        if (property.name) {
-            property.name->dump(indent + 2);
+            if (entry.name.has<NonnullRefPtr<Identifier>>()) {
+                entry.name.get<NonnullRefPtr<Identifier>>()->dump(indent + 3);
             } else {
+                entry.name.get<NonnullRefPtr<Expression>>()->dump(indent + 3);
+            }
+        } else if (entry.is_elision()) {
             print_indent(indent + 2);
-            outln("(None)");
+            outln("(Elision)");
+            continue;
         }
 
-        print_indent(indent + 1);
-        outln("(Pattern)");
-        if (property.pattern) {
-            property.pattern->dump(indent + 2);
-        } else {
         print_indent(indent + 2);
-            outln("(None)");
+        outln("(Pattern{})", entry.is_rest ? " rest=true" : "");
+        if (entry.alias.has<NonnullRefPtr<Identifier>>()) {
+            entry.alias.get<NonnullRefPtr<Identifier>>()->dump(indent + 3);
+        } else if (entry.alias.has<NonnullRefPtr<BindingPattern>>()) {
+            entry.alias.get<NonnullRefPtr<BindingPattern>>()->dump(indent + 3);
+        } else {
+            print_indent(indent + 3);
+            outln("<empty>");
         }
 
-        print_indent(indent + 1);
-        outln("(Is Rest = {})", property.is_rest);
+        if (entry.initializer) {
+            print_indent(indent + 2);
+            outln("(Initializer)");
+            entry.initializer->dump(indent + 3);
+        }
     }
 }
 

Userland/Libraries/LibJS/AST.h

@@ -215,12 +215,15 @@ public:
 };
 
 struct BindingPattern : RefCounted<BindingPattern> {
-    struct BindingProperty {
-        RefPtr<Identifier> name;
-        RefPtr<Identifier> alias;
-        RefPtr<BindingPattern> pattern;
-        RefPtr<Expression> initializer;
+    // This covers both BindingProperty and BindingElement, hence the more generic name
+    struct BindingEntry {
+        // If this entry represents a BindingElement, then name will be Empty
+        Variant<NonnullRefPtr<Identifier>, NonnullRefPtr<Expression>, Empty> name { Empty {} };
+        Variant<NonnullRefPtr<Identifier>, NonnullRefPtr<BindingPattern>, Empty> alias { Empty {} };
+        RefPtr<Expression> initializer {};
         bool is_rest { false };
+
+        bool is_elision() const { return name.has<Empty>() && alias.has<Empty>(); }
     };
 
     enum class Kind {
@@ -229,10 +232,11 @@ struct BindingPattern : RefCounted<BindingPattern> {
     };
 
     void dump(int indent) const;
-    template<typename C>
-    void for_each_assigned_name(C&& callback) const;
 
-    Vector<BindingProperty> properties;
+    template<typename C>
+    void for_each_bound_name(C&& callback) const;
+
+    Vector<BindingEntry> entries;
     Kind kind { Kind::Object };
 };
 
@@ -1398,14 +1402,15 @@ public:
 };
 
 template<typename C>
-void BindingPattern::for_each_assigned_name(C&& callback) const
+void BindingPattern::for_each_bound_name(C&& callback) const
 {
-    for (auto& property : properties) {
-        if (property.name) {
-            callback(property.name->string());
-            continue;
+    for (auto& entry : entries) {
+        auto& alias = entry.alias;
+        if (alias.has<NonnullRefPtr<Identifier>>()) {
+            callback(alias.get<NonnullRefPtr<Identifier>>()->string());
+        } else if (alias.has<NonnullRefPtr<BindingPattern>>()) {
+            alias.get<NonnullRefPtr<BindingPattern>>()->for_each_bound_name(forward<C>(callback));
         }
-        property.pattern->template for_each_assigned_name(forward<C>(callback));
     }
 }
 

Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp

@@ -43,7 +43,7 @@ void ScopeNode::generate_bytecode(Bytecode::Generator& generator) const
                         generator.emit<Bytecode::Op::PutById>(Bytecode::Register::global_object(), generator.intern_string(id->string()));
                     },
                     [&](const NonnullRefPtr<BindingPattern>& binding) {
-                        binding->for_each_assigned_name([&](const auto& name) {
+                        binding->for_each_bound_name([&](const auto& name) {
                             generator.emit<Bytecode::Op::LoadImmediate>(js_undefined());
                             generator.emit<Bytecode::Op::PutById>(Bytecode::Register::global_object(), generator.intern_string(name));
                         });
@@ -54,7 +54,7 @@ void ScopeNode::generate_bytecode(Bytecode::Generator& generator) const
                         scope_variables_with_declaration_kind.set((size_t)generator.intern_string(id->string()).value(), { js_undefined(), declaration.declaration_kind() });
                     },
                     [&](const NonnullRefPtr<BindingPattern>& binding) {
-                        binding->for_each_assigned_name([&](const auto& name) {
+                        binding->for_each_bound_name([&](const auto& name) {
                             scope_variables_with_declaration_kind.set((size_t)generator.intern_string(name).value(), { js_undefined(), declaration.declaration_kind() });
                         });
                     });

Userland/Libraries/LibJS/Interpreter.cpp

@@ -108,7 +108,7 @@ void Interpreter::enter_scope(const ScopeNode& scope_node, ScopeType scope_type,
                         global_object.put(id->string(), js_undefined());
                     },
                     [&](const NonnullRefPtr<BindingPattern>& binding) {
-                        binding->for_each_assigned_name([&](const auto& name) {
+                        binding->for_each_bound_name([&](const auto& name) {
                             global_object.put(name, js_undefined());
                         });
                     });
@@ -120,7 +120,7 @@ void Interpreter::enter_scope(const ScopeNode& scope_node, ScopeType scope_type,
                         scope_variables_with_declaration_kind.set(id->string(), { js_undefined(), declaration.declaration_kind() });
                     },
                     [&](const NonnullRefPtr<BindingPattern>& binding) {
-                        binding->for_each_assigned_name([&](const auto& name) {
+                        binding->for_each_bound_name([&](const auto& name) {
                             scope_variables_with_declaration_kind.set(name, { js_undefined(), declaration.declaration_kind() });
                         });
                     });

Userland/Libraries/LibJS/Parser.cpp

@@ -1248,6 +1248,15 @@ NonnullRefPtr<AssignmentExpression> Parser::parse_assignment_expression(Assignme
     return create_ast_node<AssignmentExpression>({ m_parser_state.m_current_token.filename(), rule_start.position(), position() }, assignment_op, move(lhs), move(rhs));
 }
 
+NonnullRefPtr<Identifier> Parser::parse_identifier()
+{
+    auto identifier_start = position();
+    auto token = consume(TokenType::Identifier);
+    return create_ast_node<Identifier>(
+        { m_parser_state.m_current_token.filename(), identifier_start, position() },
+        token.value());
+}
+
 NonnullRefPtr<CallExpression> Parser::parse_call_expression(NonnullRefPtr<Expression> lhs)
 {
     auto rule_start = push_start();
@@ -1522,36 +1531,28 @@ RefPtr<BindingPattern> Parser::parse_binding_pattern()
 {
     auto rule_start = push_start();
 
-    auto pattern_ptr = adopt_ref(*new BindingPattern);
-    auto& pattern = *pattern_ptr;
     TokenType closing_token;
-    auto allow_named_property = false;
-    auto elide_extra_commas = false;
-    auto allow_nested_pattern = false;
+    bool is_object = true;
 
     if (match(TokenType::BracketOpen)) {
         consume();
-        pattern.kind = BindingPattern::Kind::Array;
         closing_token = TokenType::BracketClose;
-        elide_extra_commas = true;
-        allow_nested_pattern = true;
+        is_object = false;
     } else if (match(TokenType::CurlyOpen)) {
         consume();
-        pattern.kind = BindingPattern::Kind::Object;
         closing_token = TokenType::CurlyClose;
-        allow_named_property = true;
     } else {
         return {};
     }
 
-    while (!match(closing_token)) {
-        if (elide_extra_commas && match(TokenType::Comma))
-            consume();
+    Vector<BindingPattern::BindingEntry> entries;
 
-        ScopeGuard consume_commas { [&] {
-            if (match(TokenType::Comma))
+    while (!match(closing_token)) {
+        if (!is_object && match(TokenType::Comma)) {
             consume();
-        } };
+            entries.append(BindingPattern::BindingEntry {});
+            continue;
+        }
 
         auto is_rest = false;
 
@@ -1560,89 +1561,88 @@ RefPtr<BindingPattern> Parser::parse_binding_pattern()
             is_rest = true;
         }
 
+        decltype(BindingPattern::BindingEntry::name) name = Empty {};
+        decltype(BindingPattern::BindingEntry::alias) alias = Empty {};
+        RefPtr<Expression> initializer = {};
+
+        if (is_object) {
             if (match(TokenType::Identifier)) {
-            auto identifier_start = position();
-            auto token = consume(TokenType::Identifier);
-            auto name = create_ast_node<Identifier>(
-                { m_parser_state.m_current_token.filename(), identifier_start, position() },
-                token.value());
-
-            if (!is_rest && allow_named_property && match(TokenType::Colon)) {
+                name = parse_identifier();
+            } else if (match(TokenType::BracketOpen)) {
                 consume();
-                if (!match(TokenType::Identifier)) {
-                    syntax_error("Expected a binding pattern as the value of a named element in destructuring object");
-                    break;
+                name = parse_expression(0);
+                consume(TokenType::BracketOpen);
             } else {
-                    auto identifier_start = position();
-                    auto token = consume(TokenType::Identifier);
-                    auto alias_name = create_ast_node<Identifier>(
-                        { m_parser_state.m_current_token.filename(), identifier_start, position() },
-                        token.value());
-                    pattern.properties.append(BindingPattern::BindingProperty {
-                        .name = move(name),
-                        .alias = move(alias_name),
-                        .pattern = nullptr,
-                        .initializer = nullptr,
-                        .is_rest = false,
-                    });
-                }
-                continue;
+                syntax_error("Expected identifier or computed property name");
+                return {};
             }
 
-            RefPtr<Expression> initializer;
-            if (match(TokenType::Equals)) {
+            if (!is_rest && match(TokenType::Colon)) {
                 consume();
-                initializer = parse_expression(2);
-            }
-            pattern.properties.append(BindingPattern::BindingProperty {
-                .name = move(name),
-                .alias = nullptr,
-                .pattern = nullptr,
-                .initializer = move(initializer),
-                .is_rest = is_rest,
-            });
-            if (is_rest)
-                break;
-            continue;
-        }
-
-        if (allow_nested_pattern) {
+                if (match(TokenType::CurlyOpen) || match(TokenType::BracketOpen)) {
                     auto binding_pattern = parse_binding_pattern();
-            if (!binding_pattern) {
-                if (is_rest)
-                    syntax_error("Expected a binding pattern after ... in destructuring list");
-                else
-                    syntax_error("Expected a binding pattern or identifier in destructuring list");
-                break;
+                    if (!binding_pattern)
+                        return {};
+                    alias = binding_pattern.release_nonnull();
+                } else if (match_identifier_name()) {
+                    alias = parse_identifier();
                 } else {
-                RefPtr<Expression> initializer;
+                    syntax_error("Expected identifier or binding pattern");
+                    return {};
+                }
+            }
+        } else {
+            if (match(TokenType::Identifier)) {
+                // BindingElement must always have an Empty name field
+                alias = parse_identifier();
+            } else if (match(TokenType::BracketOpen) || match(TokenType::CurlyOpen)) {
+                auto pattern = parse_binding_pattern();
+                if (!pattern) {
+                    syntax_error("Expected binding pattern");
+                    return {};
+                }
+                alias = pattern.release_nonnull();
+            } else {
+                syntax_error("Expected identifier or binding pattern");
+                return {};
+            }
+        }
+
         if (match(TokenType::Equals)) {
+            if (is_rest) {
+                syntax_error("Unexpected initializer after rest element");
+                return {};
+            }
+
             consume();
+
             initializer = parse_expression(2);
+            if (!initializer) {
+                syntax_error("Expected initialization expression");
+                return {};
             }
-                pattern.properties.append(BindingPattern::BindingProperty {
-                    .name = nullptr,
-                    .alias = nullptr,
-                    .pattern = move(binding_pattern),
-                    .initializer = move(initializer),
-                    .is_rest = is_rest,
-                });
-                if (is_rest)
-                    break;
-                continue;
         }
 
-            continue;
+        entries.append(BindingPattern::BindingEntry { move(name), move(alias), move(initializer), is_rest });
+
+        if (match(TokenType::Comma)) {
+            if (is_rest) {
+                syntax_error("Rest element may not be followed by a comma");
+                return {};
+            }
+            consume();
+        }
     }
 
-        break;
-    }
-
-    while (elide_extra_commas && match(TokenType::Comma))
+    while (!is_object && match(TokenType::Comma))
         consume();
 
     consume(closing_token);
 
+    auto kind = is_object ? BindingPattern::Kind::Object : BindingPattern::Kind::Array;
+    auto pattern = adopt_ref(*new BindingPattern);
+    pattern->entries = move(entries);
+    pattern->kind = kind;
     return pattern;
 }
 

Userland/Libraries/LibJS/Parser.h

@@ -86,6 +86,7 @@ public:
     NonnullRefPtr<YieldExpression> parse_yield_expression();
     NonnullRefPtr<Expression> parse_property_key();
     NonnullRefPtr<AssignmentExpression> parse_assignment_expression(AssignmentOp, NonnullRefPtr<Expression> lhs, int min_precedence, Associativity);
+    NonnullRefPtr<Identifier> parse_identifier();
 
     RefPtr<FunctionExpression> try_parse_arrow_function_expression(bool expect_parens);
     RefPtr<Statement> try_parse_labelled_statement();

Userland/Libraries/LibJS/Runtime/PropertyName.h

@@ -87,6 +87,8 @@ public:
         }
     }
 
+    ALWAYS_INLINE Type type() const { return m_type; }
+
     bool is_valid() const { return m_type != Type::Invalid; }
     bool is_number() const
     {
@@ -176,6 +178,35 @@ private:
     u32 m_number { 0 };
 };
 
+struct PropertyNameTraits : public Traits<PropertyName> {
+    static unsigned hash(PropertyName const& name)
+    {
+        VERIFY(name.is_valid());
+        if (name.is_string())
+            return name.as_string().hash();
+        if (name.is_number())
+            return int_hash(name.as_number());
+        return ptr_hash(name.as_symbol());
+    }
+
+    static bool equals(PropertyName const& a, PropertyName const& b)
+    {
+        if (a.type() != b.type())
+            return false;
+
+        switch (a.type()) {
+        case PropertyName::Type::Number:
+            return a.as_number() == b.as_number();
+        case PropertyName::Type::String:
+            return a.as_string() == b.as_string();
+        case PropertyName::Type::Symbol:
+            return a.as_symbol() == b.as_symbol();
+        default:
+            VERIFY_NOT_REACHED();
+        }
+    }
+};
+
 }
 
 namespace AK {

Userland/Libraries/LibJS/Runtime/ScriptFunction.cpp

@@ -100,7 +100,7 @@ LexicalEnvironment* ScriptFunction::create_environment()
         parameter.binding.visit(
             [&](const FlyString& name) { variables.set(name, { js_undefined(), DeclarationKind::Var }); },
             [&](const NonnullRefPtr<BindingPattern>& binding) {
-                binding->for_each_assigned_name([&](const auto& name) {
+                binding->for_each_bound_name([&](const auto& name) {
                     variables.set(name, { js_undefined(), DeclarationKind::Var });
                 });
             });
@@ -114,7 +114,7 @@ LexicalEnvironment* ScriptFunction::create_environment()
                         variables.set(id->string(), { js_undefined(), declaration.declaration_kind() });
                     },
                     [&](const NonnullRefPtr<BindingPattern>& binding) {
-                        binding->for_each_assigned_name([&](const auto& name) {
+                        binding->for_each_bound_name([&](const auto& name) {
                             variables.set(name, { js_undefined(), declaration.declaration_kind() });
                         });
                     });

Userland/Libraries/LibJS/Runtime/VM.cpp

@@ -208,18 +208,15 @@ void VM::assign(const NonnullRefPtr<BindingPattern>& target, Value value, Global
         if (!iterator)
             return;
 
-        size_t index = 0;
-        while (true) {
+        for (size_t i = 0; i < binding.entries.size(); i++) {
             if (exception())
                 return;
 
-            if (index >= binding.properties.size())
-                break;
+            auto& entry = binding.entries[i];
 
-            auto pattern_property = binding.properties[index];
-            ++index;
+            if (entry.is_rest) {
+                VERIFY(i == binding.entries.size() - 1);
 
-            if (pattern_property.is_rest) {
                 auto* array = Array::create(global_object);
                 for (;;) {
                     auto next_object = iterator_next(*iterator);
@@ -240,7 +237,7 @@ void VM::assign(const NonnullRefPtr<BindingPattern>& target, Value value, Global
                     array->indexed_properties().append(next_value);
                 }
                 value = array;
-            } else {
+            } else if (iterator) {
                 auto next_object = iterator_next(*iterator);
                 if (!next_object)
                     return;
@@ -249,65 +246,83 @@ void VM::assign(const NonnullRefPtr<BindingPattern>& target, Value value, Global
                 if (exception())
                     return;
 
-                if (!done_property.is_empty() && done_property.to_boolean())
-                    break;
-
+                if (!done_property.is_empty() && done_property.to_boolean()) {
+                    iterator = nullptr;
+                    value = js_undefined();
+                } else {
                     value = next_object->get(names.value);
                     if (exception())
                         return;
                 }
+            } else {
+                value = js_undefined();
+            }
 
-            if (value.is_undefined() && pattern_property.initializer)
-                value = pattern_property.initializer->execute(interpreter(), global_object);
-
+            if (value.is_undefined() && entry.initializer) {
+                value = entry.initializer->execute(interpreter(), global_object);
                 if (exception())
                     return;
-
-            if (pattern_property.name) {
-                set_variable(pattern_property.name->string(), value, global_object, first_assignment, specific_scope);
-                if (pattern_property.is_rest)
-                    break;
-                continue;
             }
 
-            if (pattern_property.pattern) {
-                assign(NonnullRefPtr(*pattern_property.pattern), value, global_object, first_assignment, specific_scope);
-                if (pattern_property.is_rest)
+            entry.alias.visit(
+                [&](Empty) {},
+                [&](NonnullRefPtr<Identifier> const& identifier) {
+                    set_variable(identifier->string(), value, global_object, first_assignment, specific_scope);
+                },
+                [&](NonnullRefPtr<BindingPattern> const& pattern) {
+                    assign(pattern, value, global_object, first_assignment, specific_scope);
+                });
+
+            if (entry.is_rest)
                 break;
-                continue;
-            }
         }
+
         break;
     }
     case BindingPattern::Kind::Object: {
         auto object = value.to_object(global_object);
-        HashTable<FlyString> seen_names;
-        for (auto& property : binding.properties) {
-            VERIFY(!property.pattern);
+        HashTable<PropertyName, PropertyNameTraits> seen_names;
+        for (auto& property : binding.entries) {
+            VERIFY(!property.is_elision());
+
+            PropertyName assignment_name;
             JS::Value value_to_assign;
             if (property.is_rest) {
-                auto* rest_object = Object::create(global_object, nullptr);
-                for (auto& property : object->shape().property_table()) {
-                    if (!property.value.attributes.has_enumerable())
+                VERIFY(property.name.has<NonnullRefPtr<Identifier>>());
+                assignment_name = property.name.get<NonnullRefPtr<Identifier>>()->string();
+
+                auto* rest_object = Object::create(global_object, global_object.object_prototype());
+                for (auto& object_property : object->shape().property_table()) {
+                    if (!object_property.value.attributes.has_enumerable())
                         continue;
-                    if (seen_names.contains(property.key.to_display_string()))
+                    if (seen_names.contains(object_property.key.to_display_string()))
                         continue;
-                    rest_object->put(property.key, object->get(property.key));
+                    rest_object->put(object_property.key, object->get(object_property.key));
                     if (exception())
                         return;
                 }
+
                 value_to_assign = rest_object;
             } else {
-                value_to_assign = object->get(property.name->string());
-            }
+                property.name.visit(
+                    [&](Empty) { VERIFY_NOT_REACHED(); },
+                    [&](NonnullRefPtr<Identifier> const& identifier) {
+                        assignment_name = identifier->string();
+                    },
+                    [&](NonnullRefPtr<Expression> const& expression) {
+                        auto result = expression->execute(interpreter(), global_object);
+                        if (exception())
+                            return;
+                        assignment_name = result.to_property_key(global_object);
+                    });
 
-            seen_names.set(property.name->string());
                 if (exception())
                     break;
 
-            auto assignment_name = property.name->string();
-            if (property.alias)
-                assignment_name = property.alias->string();
+                value_to_assign = object->get(assignment_name);
+            }
+
+            seen_names.set(assignment_name);
 
             if (value_to_assign.is_empty())
                 value_to_assign = js_undefined();
@@ -318,7 +333,18 @@ void VM::assign(const NonnullRefPtr<BindingPattern>& target, Value value, Global
             if (exception())
                 break;
 
-            set_variable(assignment_name, value_to_assign, global_object, first_assignment, specific_scope);
+            property.alias.visit(
+                [&](Empty) {
+                    set_variable(assignment_name.to_string(), value_to_assign, global_object, first_assignment, specific_scope);
+                },
+                [&](NonnullRefPtr<Identifier> const& identifier) {
+                    VERIFY(!property.is_rest);
+                    set_variable(identifier->string(), value_to_assign, global_object, first_assignment, specific_scope);
+                },
+                [&](NonnullRefPtr<BindingPattern> const& pattern) {
+                    VERIFY(!property.is_rest);
+                    assign(pattern, value_to_assign, global_object, first_assignment, specific_scope);
+                });
 
             if (property.is_rest)
                 break;