From 6331d45a6f0a111b483a5c4b84cc24926bc9205d Mon Sep 17 00:00:00 2001 From: Linus Groh Date: Tue, 20 Oct 2020 18:32:51 +0100 Subject: [PATCH] LibJS: Move checks for invalid getter/setter params to parse_function_node This allows us to provide better error messages as we can point the syntax error location to the exact first invalid parameter instead of always the end of the function within a object literal or class definition. Before this change: const Foo = { set bar() {} } ^ Uncaught exception: [SyntaxError]: Object setter property must have one argument (line: 1, column: 28) class Foo { set bar() {} } ^ Uncaught exception: [SyntaxError]: Class setter method must have one argument (line: 1, column: 26) After this change: const Foo = { set bar() {} } ^ Uncaught exception: [SyntaxError]: Setter function must have one argument (line: 1, column: 23) class Foo { set bar() {} } ^ Uncaught exception: [SyntaxError]: Setter function must have one argument (line: 1, column: 21) The only possible downside of this change is that class getters/setters and functions in objects are not distinguished in the message anymore - I don't think that's important though, and classes are (mostly) just syntactic sugar anyway. --- Libraries/LibJS/Parser.cpp | 50 +++++++++++++++----------------------- Libraries/LibJS/Parser.h | 4 ++- 2 files changed, 23 insertions(+), 31 deletions(-) diff --git a/Libraries/LibJS/Parser.cpp b/Libraries/LibJS/Parser.cpp index 21bda41742b..9a68bdfe422 100644 --- a/Libraries/LibJS/Parser.cpp +++ b/Libraries/LibJS/Parser.cpp @@ -517,15 +517,11 @@ NonnullRefPtr Parser::parse_class_expression(bool expect_class_ u8 parse_options = FunctionNodeParseOptions::AllowSuperPropertyLookup; if (!super_class.is_null()) parse_options |= FunctionNodeParseOptions::AllowSuperConstructorCall; + if (method_kind == ClassMethod::Kind::Getter) + parse_options |= FunctionNodeParseOptions::IsGetterFunction; + if (method_kind == ClassMethod::Kind::Setter) + parse_options |= FunctionNodeParseOptions::IsSetterFunction; auto function = parse_function_node(parse_options); - auto arg_count = function->parameters().size(); - - if (method_kind == ClassMethod::Kind::Getter && arg_count != 0) { - syntax_error("Class getter method must have no arguments"); - } else if (method_kind == ClassMethod::Kind::Setter && arg_count != 1) { - syntax_error("Class setter method must have one argument"); - } - if (is_constructor) { constructor = move(function); } else if (!property_key.is_null()) { @@ -764,26 +760,12 @@ NonnullRefPtr Parser::parse_object_expression() if (match(TokenType::ParenOpen)) { ASSERT(property_name); - auto function = parse_function_node(FunctionNodeParseOptions::AllowSuperPropertyLookup); - auto arg_count = function->parameters().size(); - - if (property_type == ObjectProperty::Type::Getter && arg_count != 0) { - syntax_error( - "Object getter property must have no arguments", - m_parser_state.m_current_token.line_number(), - m_parser_state.m_current_token.line_column()); - skip_to_next_property(); - continue; - } - if (property_type == ObjectProperty::Type::Setter && arg_count != 1) { - syntax_error( - "Object setter property must have one argument", - m_parser_state.m_current_token.line_number(), - m_parser_state.m_current_token.line_column()); - skip_to_next_property(); - continue; - } - + u8 parse_options = FunctionNodeParseOptions::AllowSuperPropertyLookup; + if (property_type == ObjectProperty::Type::Getter) + parse_options |= FunctionNodeParseOptions::IsGetterFunction; + if (property_type == ObjectProperty::Type::Setter) + parse_options |= FunctionNodeParseOptions::IsSetterFunction; + auto function = parse_function_node(parse_options); properties.append(create_ast_node(*property_name, function, property_type, true)); } else if (match(TokenType::Colon)) { if (!property_name) { @@ -1256,6 +1238,8 @@ NonnullRefPtr Parser::parse_block_statement(bool& is_strict) template NonnullRefPtr Parser::parse_function_node(u8 parse_options) { + ASSERT(!(parse_options & FunctionNodeParseOptions::IsGetterFunction && parse_options & FunctionNodeParseOptions::IsSetterFunction)); + TemporaryChange super_property_access_rollback(m_parser_state.m_allow_super_property_lookup, !!(parse_options & FunctionNodeParseOptions::AllowSuperPropertyLookup)); TemporaryChange super_constructor_call_rollback(m_parser_state.m_allow_super_constructor_call, !!(parse_options & FunctionNodeParseOptions::AllowSuperConstructorCall)); @@ -1269,7 +1253,7 @@ NonnullRefPtr Parser::parse_function_node(u8 parse_options) } consume(TokenType::ParenOpen); i32 function_length = -1; - auto parameters = parse_function_parameters(function_length); + auto parameters = parse_function_parameters(function_length, parse_options); consume(TokenType::ParenClose); if (function_length == -1) @@ -1288,10 +1272,14 @@ NonnullRefPtr Parser::parse_function_node(u8 parse_options) return create_ast_node(name, move(body), move(parameters), function_length, NonnullRefPtrVector(), is_strict); } -Vector Parser::parse_function_parameters(int& function_length) +Vector Parser::parse_function_parameters(int& function_length, u8 parse_options) { Vector parameters; while (match(TokenType::Identifier) || match(TokenType::TripleDot)) { + if (parse_options & FunctionNodeParseOptions::IsGetterFunction) + syntax_error("Getter function must have no arguments"); + if (parse_options & FunctionNodeParseOptions::IsSetterFunction && parameters.size() >= 1) + syntax_error("Setter function must have one argument"); if (match(TokenType::TripleDot)) { consume(); auto parameter_name = consume(TokenType::Identifier).value(); @@ -1311,6 +1299,8 @@ Vector Parser::parse_function_parameters(int& function_ break; consume(TokenType::Comma); } + if (parse_options & FunctionNodeParseOptions::IsSetterFunction && parameters.is_empty()) + syntax_error("Setter function must have one argument"); return parameters; } diff --git a/Libraries/LibJS/Parser.h b/Libraries/LibJS/Parser.h index caeecd34da3..208b9a4a4fe 100644 --- a/Libraries/LibJS/Parser.h +++ b/Libraries/LibJS/Parser.h @@ -45,6 +45,8 @@ struct FunctionNodeParseOptions { CheckForFunctionAndName = 1 << 0, AllowSuperPropertyLookup = 1 << 1, AllowSuperConstructorCall = 1 << 2, + IsGetterFunction = 1 << 3, + IsSetterFunction = 1 << 4, }; }; @@ -56,7 +58,7 @@ public: template NonnullRefPtr parse_function_node(u8 parse_options = FunctionNodeParseOptions::CheckForFunctionAndName); - Vector parse_function_parameters(int& function_length); + Vector parse_function_parameters(int& function_length, u8 parse_options = 0); NonnullRefPtr parse_statement(); NonnullRefPtr parse_block_statement();