From e07a39c8166bc3321b65e9730d71d980caad749c Mon Sep 17 00:00:00 2001 From: Linus Groh Date: Mon, 2 Nov 2020 21:03:19 +0000 Subject: [PATCH] LibJS: Replace 'size_t line, size_t column' with 'Optional' This is a bit nicer for two reasons: - The absence of line number/column information isn't based on 'values are zero' anymore but on Optional's value - When reporting syntax errors with position information other than the current token's position we had to store line and column ourselves, like this: auto foo_start_line = m_parser_state.m_current_token.line_number(); auto foo_start_column = m_parser_state.m_current_token.line_column(); ... syntax_error("...", foo_start_line, foo_start_column); Which now becomes: auto foo_start= position(); ... syntax_error("...", foo_start); This makes it easier to report correct positions for syntax errors that only emerge a few tokens later :^) --- Libraries/LibJS/Parser.cpp | 42 +++++++++++++++++++------------------- Libraries/LibJS/Parser.h | 21 +++++++++++-------- 2 files changed, 34 insertions(+), 29 deletions(-) diff --git a/Libraries/LibJS/Parser.cpp b/Libraries/LibJS/Parser.cpp index 254380cf9c..79d47cf889 100644 --- a/Libraries/LibJS/Parser.cpp +++ b/Libraries/LibJS/Parser.cpp @@ -662,24 +662,22 @@ NonnullRefPtr Parser::parse_unary_prefixed_expression() switch (m_parser_state.m_current_token.type()) { case TokenType::PlusPlus: { consume(); - auto rhs_start_line = m_parser_state.m_current_token.line_number(); - auto rhs_start_column = m_parser_state.m_current_token.line_column(); + auto rhs_start = position(); auto rhs = parse_expression(precedence, associativity); // FIXME: Apparently for functions this should also not be enforced on a parser level, // other engines throw ReferenceError for ++foo() if (!rhs->is_identifier() && !rhs->is_member_expression()) - syntax_error(String::formatted("Right-hand side of prefix increment operator must be identifier or member expression, got {}", rhs->class_name()), rhs_start_line, rhs_start_column); + syntax_error(String::formatted("Right-hand side of prefix increment operator must be identifier or member expression, got {}", rhs->class_name()), rhs_start); return create_ast_node(UpdateOp::Increment, move(rhs), true); } case TokenType::MinusMinus: { consume(); - auto rhs_start_line = m_parser_state.m_current_token.line_number(); - auto rhs_start_column = m_parser_state.m_current_token.line_column(); + auto rhs_start = position(); auto rhs = parse_expression(precedence, associativity); // FIXME: Apparently for functions this should also not be enforced on a parser level, // other engines throw ReferenceError for --foo() if (!rhs->is_identifier() && !rhs->is_member_expression()) - syntax_error(String::formatted("Right-hand side of prefix decrement operator must be identifier or member expression, got {}", rhs->class_name()), rhs_start_line, rhs_start_column); + syntax_error(String::formatted("Right-hand side of prefix decrement operator must be identifier or member expression, got {}", rhs->class_name()), rhs_start); return create_ast_node(UpdateOp::Decrement, move(rhs), true); } case TokenType::ExclamationMark: @@ -777,10 +775,7 @@ NonnullRefPtr Parser::parse_object_expression() if (property_type == ObjectProperty::Type::Getter || property_type == ObjectProperty::Type::Setter) { if (!match(TokenType::ParenOpen)) { - syntax_error( - "Expected '(' for object getter or setter property", - m_parser_state.m_current_token.line_number(), - m_parser_state.m_current_token.line_column()); + syntax_error("Expected '(' for object getter or setter property"); skip_to_next_property(); continue; } @@ -867,7 +862,7 @@ NonnullRefPtr Parser::parse_string_literal(Token token, bool in_t } if (!message.is_empty()) - syntax_error(message, token.line_number(), token.line_column()); + syntax_error(message, Position { token.line_number(), token.line_column() }); } auto is_use_strict_directive = !in_template_literal && (token.value() == "'use strict'" || token.value() == "\"use strict\""); @@ -1321,7 +1316,7 @@ Vector Parser::parse_function_parameters(int& function_ else if (has_rest_parameter) message = String::formatted("Duplicate parameter '{}' not allowed in function with rest parameter", parameter_name); if (!message.is_empty()) - syntax_error(message, token.line_number(), token.line_column()); + syntax_error(message, Position { token.line_number(), token.line_column() }); break; } return token; @@ -1915,11 +1910,10 @@ Token Parser::consume_and_validate_numeric_literal() auto is_unprefixed_octal_number = [](const StringView& value) { return value.length() > 1 && value[0] == '0' && isdigit(value[1]); }; - auto literal_start_line = m_parser_state.m_current_token.line_number(); - auto literal_start_column = m_parser_state.m_current_token.line_column(); + auto literal_start = position(); auto token = consume(TokenType::NumericLiteral); if (m_parser_state.m_strict_mode && is_unprefixed_octal_number(token.value())) - syntax_error("Unprefixed octal number not allowed in strict mode", literal_start_line, literal_start_column); + syntax_error("Unprefixed octal number not allowed in strict mode", literal_start); if (match_identifier_name() && m_parser_state.m_current_token.trivia().is_empty()) syntax_error("Numeric literal must not be immediately followed by identifier"); return token; @@ -1933,13 +1927,19 @@ void Parser::expected(const char* what) syntax_error(message); } -void Parser::syntax_error(const String& message, size_t line, size_t column) +Parser::Position Parser::position() const { - if (line == 0 || column == 0) { - line = m_parser_state.m_current_token.line_number(); - column = m_parser_state.m_current_token.line_column(); - } - m_parser_state.m_errors.append({ message, line, column }); + return { + m_parser_state.m_current_token.line_number(), + m_parser_state.m_current_token.line_column() + }; +} + +void Parser::syntax_error(const String& message, Optional position) +{ + if (!position.has_value()) + position = this->position(); + m_parser_state.m_errors.append({ message, position }); } void Parser::save_state() diff --git a/Libraries/LibJS/Parser.h b/Libraries/LibJS/Parser.h index 1ed1402aa7..1d9d190ccf 100644 --- a/Libraries/LibJS/Parser.h +++ b/Libraries/LibJS/Parser.h @@ -100,21 +100,25 @@ public: NonnullRefPtr parse_property_key(); NonnullRefPtr parse_assignment_expression(AssignmentOp, NonnullRefPtr lhs, int min_precedence, Associativity); - struct Error { - String message; + struct Position { size_t line; size_t column; + }; + + struct Error { + String message; + Optional position; String to_string() const { - if (line == 0 || column == 0) + if (!position.has_value()) return message; - return String::formatted("{} (line: {}, column: {})", message, line, column); + return String::formatted("{} (line: {}, column: {})", message, position.value().line, position.value().column); } String source_location_hint(const StringView& source, const char spacer = ' ', const char indicator = '^') const { - if (line == 0 || column == 0) + if (!position.has_value()) return {}; // We need to modify the source to match what the lexer considers one line - normalizing // line terminators to \n is easier than splitting using all different LT characters. @@ -124,9 +128,9 @@ public: source_string.replace(LINE_SEPARATOR, "\n"); source_string.replace(PARAGRAPH_SEPARATOR, "\n"); StringBuilder builder; - builder.append(source_string.split_view('\n', true)[line - 1]); + builder.append(source_string.split_view('\n', true)[position.value().line - 1]); builder.append('\n'); - for (size_t i = 0; i < column - 1; ++i) + for (size_t i = 0; i < position.value().column - 1; ++i) builder.append(spacer); builder.append(indicator); return builder.build(); @@ -156,13 +160,14 @@ private: bool match(TokenType type) const; bool done() const; void expected(const char* what); - void syntax_error(const String& message, size_t line = 0, size_t column = 0); + void syntax_error(const String& message, Optional = {}); Token consume(); Token consume(TokenType type); Token consume_and_validate_numeric_literal(); void consume_or_insert_semicolon(); void save_state(); void load_state(); + Position position() const; struct ParserState { Lexer m_lexer;