LibJS: Share parameter parsing between regular and arrow functions

This simplifies try_parse_arrow_function_expression() and fixes a few
cases that should not produce an arrow function AST but did:

    (a,,) => {}
    (a b) => {}
    (a ...b) => {}
    (...b a) => {}

The new parsing logic checks whether parens are expected and uses
parse_function_parameters() if so, rolling back if a new syntax error
occurs during that. Otherwise it's just an identifier in which case we
parse the single parameter ourselves.

Libraries/LibJS/Parser.cpp

@@ -334,62 +334,28 @@ RefPtr<FunctionExpression> Parser::try_parse_arrow_function_expression(bool expe
     };
 
     Vector<FunctionNode::Parameter> parameters;
-    bool parse_failed = false;
-    bool has_rest_parameter = false;
     i32 function_length = -1;
-    while (true) {
+    if (expect_parens) {
-        if (match(TokenType::Comma)) {
+        // We have parens around the function parameters and can re-use the same parsing
-            if (has_rest_parameter || !expect_parens) {
+        // logic used for regular functions: multiple parameters, default values, rest
-                parse_failed = true;
+        // parameter, maybe a trailing comma. If we have a new syntax error afterwards we
-                break;
+        // know parsing failed and rollback the parser state.
-            }
+        auto previous_syntax_errors = m_parser_state.m_errors.size();
-            consume(TokenType::Comma);
+        parameters = parse_function_parameters(function_length);
-        } else if (match(TokenType::Identifier)) {
+        if (m_parser_state.m_errors.size() > previous_syntax_errors)
-            auto parameter_name = consume(TokenType::Identifier).value();
+            return nullptr;
-            RefPtr<Expression> default_value;
+        if (!match(TokenType::ParenClose))
-            if (expect_parens && match(TokenType::Equals)) {
+            return nullptr;
-                consume(TokenType::Equals);
+        consume();
-                function_length = parameters.size();
+    } else {
-                default_value = parse_expression(2);
+        // No parens - this must be an identifier followed by arrow. That's it.
-            }
+        if (!match(TokenType::Identifier))
-            parameters.append({ parameter_name, default_value });
+            return nullptr;
-        } else if (match(TokenType::TripleDot)) {
+        parameters.append({ consume().value(), {} });
-            consume();
-            if (has_rest_parameter) {
-                parse_failed = true;
-                break;
-            }
-            has_rest_parameter = true;
-            function_length = parameters.size();
-            auto parameter_name = consume(TokenType::Identifier).value();
-            parameters.append({ parameter_name, nullptr, true });
-        } else if (match(TokenType::ParenClose)) {
-            if (expect_parens) {
-                consume(TokenType::ParenClose);
-                if (match(TokenType::Arrow)) {
-                    consume(TokenType::Arrow);
-                } else {
-                    parse_failed = true;
-                }
-                break;
-            }
-            parse_failed = true;
-            break;
-        } else if (match(TokenType::Arrow)) {
-            if (!expect_parens) {
-                consume(TokenType::Arrow);
-                break;
-            }
-            parse_failed = true;
-            break;
-        } else {
-            parse_failed = true;
-            break;
-        }
     }
-
+    if (!match(TokenType::Arrow))
-    if (parse_failed)
         return nullptr;
+    consume();
 
     if (function_length == -1)
         function_length = parameters.size();
@@ -1299,8 +1265,29 @@ NonnullRefPtr<FunctionNodeType> Parser::parse_function_node(bool check_for_funct
         }
     }
     consume(TokenType::ParenOpen);
-    Vector<FunctionNode::Parameter> parameters;
     i32 function_length = -1;
+    auto parameters = parse_function_parameters(function_length);
+    consume(TokenType::ParenClose);
+
+    if (function_length == -1)
+        function_length = parameters.size();
+
+    TemporaryChange change(m_parser_state.m_in_function_context, true);
+    auto old_labels_in_scope = move(m_parser_state.m_labels_in_scope);
+    ScopeGuard guard([&]() {
+        m_parser_state.m_labels_in_scope = move(old_labels_in_scope);
+    });
+
+    bool is_strict = false;
+    auto body = parse_block_statement(is_strict);
+    body->add_variables(m_parser_state.m_var_scopes.last());
+    body->add_functions(m_parser_state.m_function_scopes.last());
+    return create_ast_node<FunctionNodeType>(name, move(body), move(parameters), function_length, NonnullRefPtrVector<VariableDeclaration>(), is_strict);
+}
+
+Vector<FunctionNode::Parameter> Parser::parse_function_parameters(int& function_length)
+{
+    Vector<FunctionNode::Parameter> parameters;
     while (match(TokenType::Identifier) || match(TokenType::TripleDot)) {
         if (match(TokenType::TripleDot)) {
             consume();
@@ -1321,22 +1308,7 @@ NonnullRefPtr<FunctionNodeType> Parser::parse_function_node(bool check_for_funct
             break;
         consume(TokenType::Comma);
     }
-    consume(TokenType::ParenClose);
+    return parameters;
-
-    if (function_length == -1)
-        function_length = parameters.size();
-
-    TemporaryChange change(m_parser_state.m_in_function_context, true);
-    auto old_labels_in_scope = move(m_parser_state.m_labels_in_scope);
-    ScopeGuard guard([&]() {
-        m_parser_state.m_labels_in_scope = move(old_labels_in_scope);
-    });
-
-    bool is_strict = false;
-    auto body = parse_block_statement(is_strict);
-    body->add_variables(m_parser_state.m_var_scopes.last());
-    body->add_functions(m_parser_state.m_function_scopes.last());
-    return create_ast_node<FunctionNodeType>(name, move(body), move(parameters), function_length, NonnullRefPtrVector<VariableDeclaration>(), is_strict);
 }
 
 NonnullRefPtr<VariableDeclaration> Parser::parse_variable_declaration(bool with_semicolon)

Libraries/LibJS/Parser.h

@@ -48,6 +48,7 @@ public:
 
     template<typename FunctionNodeType>
     NonnullRefPtr<FunctionNodeType> parse_function_node(bool check_for_function_and_name = true, bool allow_super_property_lookup = false, bool allow_super_constructor_call = false);
+    Vector<FunctionNode::Parameter> parse_function_parameters(int& function_length);
 
     NonnullRefPtr<Statement> parse_statement();
     NonnullRefPtr<BlockStatement> parse_block_statement();

Libraries/LibJS/Tests/functions/arrow-functions.js

@@ -148,3 +148,16 @@ test("cannot be constructed", () => {
         new foo();
     }).toThrowWithMessage(TypeError, "foo is not a constructor");
 });
+
+test("syntax errors", () => {
+    expect("a, => {}").not.toEval();
+    expect("(a, => {}").not.toEval();
+    expect("(,) => {}").not.toEval();
+    expect("(,,) => {}").not.toEval();
+    expect("(a,,) => {}").not.toEval();
+    expect("(a,,b) => {}").not.toEval();
+    expect("(a, ...b, ...c) => {}").not.toEval();
+    expect("(a b) => {}").not.toEval();
+    expect("(a ...b) => {}").not.toEval();
+    expect("(a = 1 = 2) => {}").not.toEval();
+});