From 602190f66f7d74bba019b42870d1fc93e5a26a51 Mon Sep 17 00:00:00 2001 From: Anonymous Date: Tue, 15 Feb 2022 22:34:59 -0800 Subject: [PATCH] LibJS: Fix mixing of logical and coalescing operators The same expression is not allowed to contain both the logical && and || operators, and the coalescing ?? operator. This patch changes how "forbidden" tokens are handled, using a finite set instead of an Vector. This supports much more efficient merging of the forbidden tokens when propagating forward, and allowing the return of forbidden tokens to parent contexts. --- Userland/Libraries/LibJS/Parser.cpp | 111 +++++++++++++++--- Userland/Libraries/LibJS/Parser.h | 40 ++++++- .../coalesce-logic-expression-mixing.js | 28 +++++ 3 files changed, 160 insertions(+), 19 deletions(-) create mode 100644 Userland/Libraries/LibJS/Tests/syntax/coalesce-logic-expression-mixing.js diff --git a/Userland/Libraries/LibJS/Parser.cpp b/Userland/Libraries/LibJS/Parser.cpp index 3d8002308f..51712bf3e9 100644 --- a/Userland/Libraries/LibJS/Parser.cpp +++ b/Userland/Libraries/LibJS/Parser.cpp @@ -1050,7 +1050,7 @@ NonnullRefPtr Parser::parse_class_expression(bool expect_class_ } if (match(TokenType::BracketOpen) || match(TokenType::Period) || match(TokenType::ParenOpen)) { auto precedence = g_operator_precedence.get(m_state.current_token.type()); - expression = parse_secondary_expression(move(expression), precedence); + expression = parse_secondary_expression(move(expression), precedence).expression; continue; } break; @@ -1880,7 +1880,7 @@ NonnullRefPtr Parser::parse_template_literal(bool is_tagged) return create_ast_node({ m_state.current_token.filename(), rule_start.position(), position() }, expressions); } -NonnullRefPtr Parser::parse_expression(int min_precedence, Associativity associativity, const Vector& forbidden) +NonnullRefPtr Parser::parse_expression(int min_precedence, Associativity associativity, ForbiddenTokens forbidden) { auto rule_start = push_start(); auto [expression, should_continue_parsing] = parse_primary_expression(); @@ -1922,7 +1922,9 @@ NonnullRefPtr Parser::parse_expression(int min_precedence, Associati check_for_invalid_object_property(expression); Associativity new_associativity = operator_associativity(m_state.current_token.type()); - expression = parse_secondary_expression(move(expression), new_precedence, new_associativity, forbidden); + auto result = parse_secondary_expression(move(expression), new_precedence, new_associativity, forbidden); + expression = result.expression; + forbidden = forbidden.merge(result.forbidden); while (match(TokenType::TemplateLiteralStart) && !is(*expression)) { auto template_literal = parse_template_literal(true); expression = create_ast_node({ m_state.current_token.filename(), rule_start.position(), position() }, move(expression), move(template_literal)); @@ -1965,7 +1967,7 @@ NonnullRefPtr Parser::parse_expression(int min_precedence, Associati return expression; } -NonnullRefPtr Parser::parse_secondary_expression(NonnullRefPtr lhs, int min_precedence, Associativity associativity, Vector const& forbidden) +Parser::ExpressionResult Parser::parse_secondary_expression(NonnullRefPtr lhs, int min_precedence, Associativity associativity, ForbiddenTokens forbidden) { auto rule_start = push_start(); switch (m_state.current_token.type()) { @@ -2110,19 +2112,25 @@ NonnullRefPtr Parser::parse_secondary_expression(NonnullRefPtr({ m_state.current_token.filename(), rule_start.position(), position() }, UpdateOp::Decrement, move(lhs)); - case TokenType::DoubleAmpersand: + case TokenType::DoubleAmpersand: { consume(); - return create_ast_node({ m_state.current_token.filename(), rule_start.position(), position() }, LogicalOp::And, move(lhs), parse_expression(min_precedence, associativity, forbidden)); + auto expression = create_ast_node({ m_state.current_token.filename(), rule_start.position(), position() }, LogicalOp::And, move(lhs), parse_expression(min_precedence, associativity, forbidden.forbid({ TokenType::DoubleQuestionMark }))); + return { expression, { TokenType::DoubleQuestionMark } }; + } case TokenType::DoubleAmpersandEquals: return parse_assignment_expression(AssignmentOp::AndAssignment, move(lhs), min_precedence, associativity, forbidden); - case TokenType::DoublePipe: + case TokenType::DoublePipe: { consume(); - return create_ast_node({ m_state.current_token.filename(), rule_start.position(), position() }, LogicalOp::Or, move(lhs), parse_expression(min_precedence, associativity, forbidden)); + auto expression = create_ast_node({ m_state.current_token.filename(), rule_start.position(), position() }, LogicalOp::Or, move(lhs), parse_expression(min_precedence, associativity, forbidden.forbid({ TokenType::DoubleQuestionMark }))); + return { expression, { TokenType::DoubleQuestionMark } }; + } case TokenType::DoublePipeEquals: return parse_assignment_expression(AssignmentOp::OrAssignment, move(lhs), min_precedence, associativity, forbidden); - case TokenType::DoubleQuestionMark: + case TokenType::DoubleQuestionMark: { consume(); - return create_ast_node({ m_state.current_token.filename(), rule_start.position(), position() }, LogicalOp::NullishCoalescing, move(lhs), parse_expression(min_precedence, associativity, forbidden)); + auto expression = create_ast_node({ m_state.current_token.filename(), rule_start.position(), position() }, LogicalOp::NullishCoalescing, move(lhs), parse_expression(min_precedence, associativity, forbidden.forbid({ TokenType::DoubleAmpersand, TokenType::DoublePipe }))); + return { expression, { TokenType::DoubleAmpersand, TokenType::DoublePipe } }; + } case TokenType::DoubleQuestionMarkEquals: return parse_assignment_expression(AssignmentOp::NullishAssignment, move(lhs), min_precedence, associativity, forbidden); case TokenType::QuestionMark: @@ -2192,7 +2200,7 @@ RefPtr Parser::synthesize_binding_pattern(Expression const& expr return result; } -NonnullRefPtr Parser::parse_assignment_expression(AssignmentOp assignment_op, NonnullRefPtr lhs, int min_precedence, Associativity associativity, Vector const& forbidden) +NonnullRefPtr Parser::parse_assignment_expression(AssignmentOp assignment_op, NonnullRefPtr lhs, int min_precedence, Associativity associativity, ForbiddenTokens forbidden) { auto rule_start = push_start(); VERIFY(match(TokenType::Equals) @@ -3026,7 +3034,7 @@ NonnullRefPtr Parser::parse_continue_statement() return create_ast_node({ m_state.current_token.filename(), rule_start.position(), position() }, target_label); } -NonnullRefPtr Parser::parse_conditional_expression(NonnullRefPtr test, Vector const& forbidden) +NonnullRefPtr Parser::parse_conditional_expression(NonnullRefPtr test, ForbiddenTokens forbidden) { auto rule_start = push_start(); consume(TokenType::QuestionMark); @@ -3583,10 +3591,10 @@ bool Parser::match_unary_prefixed_expression() const || type == TokenType::Delete; } -bool Parser::match_secondary_expression(const Vector& forbidden) const +bool Parser::match_secondary_expression(ForbiddenTokens forbidden) const { auto type = m_state.current_token.type(); - if (forbidden.contains_slow(type)) + if (!forbidden.allows(type)) return false; return type == TokenType::Plus || type == TokenType::PlusEquals @@ -4484,4 +4492,79 @@ NonnullRefPtr Parser::parse_export_statement(Program& program) return create_ast_node({ m_state.current_token.filename(), rule_start.position(), position() }, move(expression), move(entries), is_default, move(from_specifier)); } + +Parser::ForbiddenTokens::ForbiddenTokens(std::initializer_list const& forbidden) +{ + forbid_tokens(forbidden); +} + +void Parser::ForbiddenTokens::forbid_tokens(std::initializer_list const& forbidden) +{ + for (auto token : forbidden) { + switch (token) { + case TokenType::In: + m_forbid_in_token = true; + break; + case TokenType::DoubleAmpersand: + case TokenType::DoublePipe: + m_forbid_logical_tokens = true; + break; + case TokenType::DoubleQuestionMark: + m_forbid_coalesce_token = true; + break; + case TokenType::QuestionMarkPeriod: + m_forbid_question_mark_period = true; + break; + case TokenType::ParenOpen: + m_forbid_paren_open = true; + break; + case TokenType::Equals: + m_forbid_equals = true; + break; + default: + VERIFY_NOT_REACHED(); + } + } +} + +bool Parser::ForbiddenTokens::allows(TokenType token) const +{ + switch (token) { + case TokenType::In: + return !m_forbid_in_token; + case TokenType::DoubleAmpersand: + case TokenType::DoublePipe: + return !m_forbid_logical_tokens; + case TokenType::DoubleQuestionMark: + return !m_forbid_coalesce_token; + case TokenType::QuestionMarkPeriod: + return !m_forbid_question_mark_period; + case TokenType::ParenOpen: + return !m_forbid_paren_open; + case TokenType::Equals: + return !m_forbid_equals; + default: + return true; + } +} + +Parser::ForbiddenTokens Parser::ForbiddenTokens::merge(ForbiddenTokens other) const +{ + ForbiddenTokens result = *this; + result.m_forbid_in_token |= other.m_forbid_in_token; + result.m_forbid_logical_tokens |= other.m_forbid_logical_tokens; + result.m_forbid_coalesce_token |= other.m_forbid_coalesce_token; + result.m_forbid_paren_open |= other.m_forbid_paren_open; + result.m_forbid_question_mark_period |= other.m_forbid_question_mark_period; + result.m_forbid_equals |= other.m_forbid_equals; + return result; +} + +Parser::ForbiddenTokens Parser::ForbiddenTokens::forbid(std::initializer_list const& forbidden) const +{ + ForbiddenTokens result = *this; + result.forbid_tokens(forbidden); + return result; +} + } diff --git a/Userland/Libraries/LibJS/Parser.h b/Userland/Libraries/LibJS/Parser.h index f4641c79d1..4d1eeb378d 100644 --- a/Userland/Libraries/LibJS/Parser.h +++ b/Userland/Libraries/LibJS/Parser.h @@ -7,6 +7,7 @@ #pragma once +#include #include #include #include @@ -14,6 +15,8 @@ #include #include #include +#include +#include #include namespace JS { @@ -84,6 +87,33 @@ public: Yes }; + struct ForbiddenTokens { + ForbiddenTokens(std::initializer_list const& forbidden); + ForbiddenTokens merge(ForbiddenTokens other) const; + bool allows(TokenType token) const; + ForbiddenTokens forbid(std::initializer_list const& forbidden) const; + + private: + void forbid_tokens(std::initializer_list const& forbidden); + bool m_forbid_in_token : 1 { false }; + bool m_forbid_logical_tokens : 1 { false }; + bool m_forbid_coalesce_token : 1 { false }; + bool m_forbid_paren_open : 1 { false }; + bool m_forbid_question_mark_period : 1 { false }; + bool m_forbid_equals : 1 { false }; + }; + + struct ExpressionResult { + template + ExpressionResult(NonnullRefPtr expression, ForbiddenTokens forbidden = {}) + : expression(expression) + , forbidden(forbidden) + { + } + NonnullRefPtr expression; + ForbiddenTokens forbidden; + }; + NonnullRefPtr parse_for_in_of_statement(NonnullRefPtr lhs, IsForAwaitLoop is_await); NonnullRefPtr parse_if_statement(); NonnullRefPtr parse_throw_statement(); @@ -97,9 +127,9 @@ public: NonnullRefPtr parse_while_statement(); NonnullRefPtr parse_with_statement(); NonnullRefPtr parse_debugger_statement(); - NonnullRefPtr parse_conditional_expression(NonnullRefPtr test, Vector const&); + NonnullRefPtr parse_conditional_expression(NonnullRefPtr test, ForbiddenTokens); NonnullRefPtr parse_optional_chain(NonnullRefPtr base); - NonnullRefPtr parse_expression(int min_precedence, Associativity associate = Associativity::Right, const Vector& forbidden = {}); + NonnullRefPtr parse_expression(int min_precedence, Associativity associate = Associativity::Right, ForbiddenTokens forbidden = {}); PrimaryExpressionParseResult parse_primary_expression(); NonnullRefPtr parse_unary_prefixed_expression(); NonnullRefPtr parse_regexp_literal(); @@ -107,7 +137,7 @@ public: NonnullRefPtr parse_array_expression(); NonnullRefPtr parse_string_literal(const Token& token, bool in_template_literal = false); NonnullRefPtr parse_template_literal(bool is_tagged); - NonnullRefPtr parse_secondary_expression(NonnullRefPtr, int min_precedence, Associativity associate = Associativity::Right, Vector const& forbidden = {}); + ExpressionResult parse_secondary_expression(NonnullRefPtr, int min_precedence, Associativity associate = Associativity::Right, ForbiddenTokens forbidden = {}); NonnullRefPtr parse_call_expression(NonnullRefPtr); NonnullRefPtr parse_new_expression(); NonnullRefPtr parse_class_declaration(); @@ -115,7 +145,7 @@ public: NonnullRefPtr parse_yield_expression(); NonnullRefPtr parse_await_expression(); NonnullRefPtr parse_property_key(); - NonnullRefPtr parse_assignment_expression(AssignmentOp, NonnullRefPtr lhs, int min_precedence, Associativity, Vector const& forbidden = {}); + NonnullRefPtr parse_assignment_expression(AssignmentOp, NonnullRefPtr lhs, int min_precedence, Associativity, ForbiddenTokens forbidden = {}); NonnullRefPtr parse_identifier(); NonnullRefPtr parse_import_statement(Program& program); NonnullRefPtr parse_export_statement(Program& program); @@ -186,7 +216,7 @@ private: Associativity operator_associativity(TokenType) const; bool match_expression() const; bool match_unary_prefixed_expression() const; - bool match_secondary_expression(const Vector& forbidden = {}) const; + bool match_secondary_expression(ForbiddenTokens forbidden = {}) const; bool match_statement() const; bool match_export_or_import() const; bool match_assert_clause() const; diff --git a/Userland/Libraries/LibJS/Tests/syntax/coalesce-logic-expression-mixing.js b/Userland/Libraries/LibJS/Tests/syntax/coalesce-logic-expression-mixing.js new file mode 100644 index 0000000000..303e53b85b --- /dev/null +++ b/Userland/Libraries/LibJS/Tests/syntax/coalesce-logic-expression-mixing.js @@ -0,0 +1,28 @@ +test("mixing coalescing and logical operators isn't allowed", () => { + expect("if (0) a ?? b || c").not.toEval(); + expect("if (0) a ?? b && c").not.toEval(); + expect("if (0) a ?? b * c || d").not.toEval(); + expect("if (0) a ?? b * c && d").not.toEval(); + expect("if (0) a && b ?? c").not.toEval(); + expect("if (0) a || b ?? c").not.toEval(); + expect("if (0) a && b * c ?? d").not.toEval(); + expect("if (0) a || b * c ?? d").not.toEval(); +}); + +test("mixing coalescing and logical operators with parens", () => { + expect("if (0) a ?? (b || c)").toEval(); + expect("if (0) (a ?? b) && c").toEval(); + expect("if (0) a ?? (b * c || d)").toEval(); + expect("if (0) (a ?? b * c) && d").toEval(); + expect("if (0) a && (b ?? c)").toEval(); + expect("if (0) (a || b) ?? c").toEval(); + expect("if (0) a && (b * c) ?? d").not.toEval(); + expect("if (0) a || (b * c) ?? d").not.toEval(); +}); + +test("mixing coalescing and logical operators when 'in' isn't allowed", () => { + expect("for (a ?? b || c in a; false;);").not.toEval(); + expect("for (a ?? b && c in a; false;);").not.toEval(); + expect("for (a || b ?? c in a; false;);").not.toEval(); + expect("for (a && b ?? c in a; false;);").not.toEval(); +});