diff --git a/parser/internal/BUILD b/parser/internal/BUILD index 8d6a85a55..cdc73288a 100644 --- a/parser/internal/BUILD +++ b/parser/internal/BUILD @@ -93,6 +93,7 @@ cc_library( "//parser:parser_interface", "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/base:nullability", + "@com_google_absl//absl/cleanup", "@com_google_absl//absl/container:flat_hash_map", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", diff --git a/parser/internal/lexer.h b/parser/internal/lexer.h index ef166d4bb..c35dc08e2 100644 --- a/parser/internal/lexer.h +++ b/parser/internal/lexer.h @@ -140,6 +140,13 @@ class Lexer final { std::numeric_limits::max())); } + struct Position final { + int32_t position = 0; + bool at_end = false; + bool done = false; + LexerError error; + }; + Lexer(const Lexer&) = delete; Lexer(Lexer&&) = delete; Lexer& operator=(const Lexer&) = delete; @@ -158,6 +165,17 @@ class Lexer final { [[nodiscard]] int32_t GetPosition() const { return position_; } + [[nodiscard]] Position SavePosition() const { + return Position{position_, at_end_, done_, error_}; + } + + void RestorePosition(const Position& position) { + position_ = position.position; + at_end_ = position.at_end; + done_ = position.done; + error_ = position.error; + } + private: [[nodiscard]] bool Match(char32_t c) const { return position_ < content_.size() && content_.at(position_) == c; diff --git a/parser/internal/lexer_test.cc b/parser/internal/lexer_test.cc index ca311baf8..ebe0011d3 100644 --- a/parser/internal/lexer_test.cc +++ b/parser/internal/lexer_test.cc @@ -495,5 +495,42 @@ TEST(LexerErrorRecoveryTest, ResumesAfterError) { EXPECT_EQ(token.end, 6); } +TEST(LexerPositionTest, SaveAndRestorePosition) { + ASSERT_OK_AND_ASSIGN(auto source, cel::NewSource("foo + bar * 42")); + Lexer lexer(*source); + + Token tok1 = lexer.Lex(); + EXPECT_EQ(tok1.type, TokenType::kIdent); + + Token tok2 = lexer.Lex(); + EXPECT_EQ(tok2.type, TokenType::kWhitespace); + + // Save position before '+' + Lexer::Position saved = lexer.SavePosition(); + + Token tok3 = lexer.Lex(); + EXPECT_EQ(tok3.type, TokenType::kPlus); + + Token tok4 = lexer.Lex(); + EXPECT_EQ(tok4.type, TokenType::kWhitespace); + + Token tok5 = lexer.Lex(); + EXPECT_EQ(tok5.type, TokenType::kIdent); + + // Restore position to before '+' + lexer.RestorePosition(saved); + + Token tok3_restored = lexer.Lex(); + EXPECT_EQ(tok3_restored.type, TokenType::kPlus); + EXPECT_EQ(tok3_restored.start, tok3.start); + EXPECT_EQ(tok3_restored.end, tok3.end); + + Token tok4_restored = lexer.Lex(); + EXPECT_EQ(tok4_restored.type, TokenType::kWhitespace); + + Token tok5_restored = lexer.Lex(); + EXPECT_EQ(tok5_restored.type, TokenType::kIdent); +} + } // namespace } // namespace cel::parser_internal diff --git a/parser/internal/pratt_parser_test.cc b/parser/internal/pratt_parser_test.cc index 539372e51..8ebe5123e 100644 --- a/parser/internal/pratt_parser_test.cc +++ b/parser/internal/pratt_parser_test.cc @@ -180,6 +180,32 @@ MATCHER_P(AstIs, expected_ast, "") { return false; } +MATCHER_P(AstEq, expected_expr, "") { + KindAndIdAdorner kind_and_id_adorner; + cel::ExprPrinter printer(kind_and_id_adorner); + ParserOptions options; + auto actual_ast = Parse(arg, options); + if (!actual_ast.ok()) { + *result_listener << "\n Actual expression failed to parse: " + << actual_ast.status(); + return false; + } + std::string actual = Unindent(printer.Print((*actual_ast)->root_expr())); + auto expected_ast = Parse(expected_expr, options); + if (!expected_ast.ok()) { + *result_listener << "\n Expected expression failed to parse: " + << expected_ast.status(); + return false; + } + std::string expected = Unindent(printer.Print((*expected_ast)->root_expr())); + if (actual == expected) { + return true; + } + *result_listener << "\n Actual: " << actual + << "\n Expected: " << expected; + return false; +} + TEST_P(PrattParserTest, Parse) { const TestCase& test_case = GetParam(); cel::ParserOptions options; @@ -1472,6 +1498,44 @@ TEST(PrattParserRecursionDepthTest, ParseRecursionDepth) { StatusIs(absl::StatusCode::kCancelled)); } +TEST(PrattParserRecursionDepthTest, ParseRecursionDepthIgnoreExtraParens) { + cel::ParserOptions options; + options.max_recursion_depth = 1; + EXPECT_THAT(Parse("((((1))))", options), IsOkAndHolds(NotNull())); +} + +TEST(PrattParserRecursionDepthTest, DeeplyNestedParens) { + cel::ParserOptions options; + options.max_recursion_depth = 1; + std::string literal_expr = + std::string(1000, '(') + "42" + std::string(1000, ')'); + EXPECT_THAT(Parse(literal_expr, options), IsOkAndHolds(NotNull())); + + std::string binary_expr = + std::string(1000, '(') + "1 + 2" + std::string(1000, ')'); + EXPECT_THAT(Parse(binary_expr, options), IsOkAndHolds(NotNull())); +} + +TEST(PrattParserRecursionDepthTest, NestedAndGroupingParensCombinations) { + EXPECT_THAT("(( (1) + 2 ))", AstEq("1 + 2")); + EXPECT_THAT("((1 + 2) * (3 + 4))", AstEq("(1 + 2) * (3 + 4)")); + EXPECT_THAT("((((1)) + ((2))))", AstEq("1 + 2")); + EXPECT_THAT("(((1 + 2) * 3) + 4)", AstEq("(1 + 2) * 3 + 4")); + EXPECT_THAT("f((((1))), (((2))))", AstEq("f(1, 2)")); + EXPECT_THAT("[{((1)): ((2))}]", AstEq("[{1: 2}]")); + EXPECT_THAT("(((a))).b[0]", AstEq("a.b[0]")); +} + +TEST(PrattParserRecursionDepthTest, MismatchedParensStillReportErrors) { + cel::ParserOptions options; + EXPECT_THAT(Parse("((((1))", options), + StatusIs(absl::StatusCode::kInvalidArgument)); + EXPECT_THAT(Parse("(((1 + 2]", options), + StatusIs(absl::StatusCode::kInvalidArgument)); + EXPECT_THAT(Parse("(( [ 1 ) ] ))", options), + StatusIs(absl::StatusCode::kInvalidArgument)); +} + TEST(PrattParserRecursionDepthTest, SequentialScopesDoNotAccumulateDepth) { cel::ParserOptions options; options.max_recursion_depth = 2; diff --git a/parser/internal/pratt_parser_worker.cc b/parser/internal/pratt_parser_worker.cc index 86319a853..b7356528d 100644 --- a/parser/internal/pratt_parser_worker.cc +++ b/parser/internal/pratt_parser_worker.cc @@ -123,7 +123,7 @@ std::string ParserWorker::GetTokenText(const Token& tok) const { return ""; } -Token ParserWorker::NextSignificantToken() { +Token ParserWorker::NextSignificantToken(bool report_error) { if (is_recovery_limit_exceeded()) { return Token{.type = TokenType::kEnd, .start = 0, .end = 0}; } @@ -132,7 +132,7 @@ Token ParserWorker::NextSignificantToken() { if (tok.type == TokenType::kWhitespace || tok.type == TokenType::kComment) { continue; } - if (tok.type == TokenType::kError) { + if (tok.type == TokenType::kError && report_error) { ReportError(tok, lexer_.GetError().message); if (is_recovery_limit_exceeded()) { return Token{.type = TokenType::kEnd, .start = 0, .end = 0}; diff --git a/parser/internal/pratt_parser_worker.h b/parser/internal/pratt_parser_worker.h index 35e9ff52b..bbd719e34 100644 --- a/parser/internal/pratt_parser_worker.h +++ b/parser/internal/pratt_parser_worker.h @@ -27,6 +27,7 @@ #include "absl/base/attributes.h" #include "absl/base/nullability.h" #include "absl/base/optimization.h" +#include "absl/cleanup/cleanup.h" #include "absl/container/flat_hash_map.h" #include "absl/status/statusor.h" #include "absl/strings/ascii.h" @@ -72,7 +73,7 @@ class ParserWorker { const cel::ParserOptions& options() const { return options_; } // Token stream management void InitTokenStream(); - Token NextSignificantToken(); + Token NextSignificantToken(bool report_error = true); Token NextToken(); bool Expect(TokenType type, absl::string_view msg = ""); std::string GetTokenText(const Token& tok) const; @@ -222,7 +223,7 @@ class PrattParserWorker : public ParserWorker { // Parses ternary conditional expressions (`condition ? true_expr : // false_expr`). - ExprNode ParseTernary(ExprNode lhs); + void ParseTernary(ExprNode& lhs); // Helper method for parsing a contiguous chain of same-precedence logical // operators (`&&` or `||`) iteratively into a list of terms and operator IDs. @@ -233,7 +234,7 @@ class PrattParserWorker : public ParserWorker { // Example (`a && b && c && d`): Iteratively collects terms `[a, b, c, d]` and // builds `((a && b) && c) && d` without ascending/descending C++ stack // frames for each term. - ExprNode ParseBalancedLogicalChain(ExprNode lhs, const BinaryOpInfo& op_info); + void ParseBalancedLogicalChain(ExprNode& lhs, const BinaryOpInfo& op_info); // Parses prefix unary operators (`!`, `-`) and trailing postfix // member/indexing operations (`.field`, `[index]`, `.method(args)`). First @@ -251,7 +252,7 @@ class PrattParserWorker : public ParserWorker { // `Type{field: val}`). // // Processes continuous postfix operation chains iteratively. - ExprNode ParseSelectorChainTail(ExprNode lhs); + void ParseSelectorChainTail(ExprNode& lhs); // Parses prefix unary operators (logical NOT `!` and negation `-`). If a // numeric literal immediately follows `-`, folds it directly into a negative @@ -265,6 +266,9 @@ class PrattParserWorker : public ParserWorker { // wrapping `has(x.y)`. ExprNode ParseUnary(); + // Parses unary operators (`!`, `-`). + ExprNode ParseUnaryOps(); + // Parses unary operator chains (`!`, `-`). ExprNode ParseUnaryOpsChain(Token first_op); @@ -283,7 +287,7 @@ class PrattParserWorker : public ParserWorker { // Example (`(a + b)`): Consumes `(`, recurses to `ParseExpr()`, and expects // `)`. Example (`has(x.y)`): Consumes `has`, parses arguments `(x.y)`, and // expands the `has` macro. - ABSL_ATTRIBUTE_ALWAYS_INLINE inline ExprNode ParsePrimary(); + ExprNode ParsePrimary(); ExprNode ParseList(); ExprNode ParseMap(); @@ -296,8 +300,8 @@ class PrattParserWorker : public ParserWorker { ExprNode ParseNegativeDoubleLiteral(int64_t node_id); ExprNode ParseStringLiteral(); ExprNode ParseBytesLiteral(); - ExprNode BuildBinaryCall(int64_t op_id, absl::string_view op_name, - ExprNode lhs, ExprNode rhs); + void BuildBinaryCall(int64_t op_id, absl::string_view op_name, ExprNode& lhs, + ExprNode rhs); ExprNode ParseIdentOrCall(); std::string NormalizeIdent(const Token& tok, bool allow_quoted); std::optional ExtractStructName(const ExprNode& expr); @@ -317,6 +321,8 @@ class PrattParserWorker : public ParserWorker { std::optional target, std::vector arguments); + int CountGroupingParentheses(); + AstFactoryInterface& ast_factory_; absl::flat_hash_map macro_calls_; }; @@ -350,12 +356,12 @@ ExprNode PrattParserWorker::ParseExpr() { } template -ExprNode PrattParserWorker::ParseTernary(ExprNode lhs) { +void PrattParserWorker::ParseTernary(ExprNode& lhs) { NextToken(); int64_t op_id = NextId(); ExprNode true_expr = ParseBinaryAndTernary(1); if (!Expect(TokenType::kColon, "expected ':' in conditional expression")) { - return lhs; + return; } ExprNode false_expr = ParseBinaryAndTernary(0); std::vector args; @@ -363,21 +369,20 @@ ExprNode PrattParserWorker::ParseTernary(ExprNode lhs) { args.push_back(std::move(lhs)); args.push_back(std::move(true_expr)); args.push_back(std::move(false_expr)); - return ast_factory_.NewCall(op_id, CelOperator::CONDITIONAL, std::move(args)); + lhs = ast_factory_.NewCall(op_id, CelOperator::CONDITIONAL, std::move(args)); } const BinaryOpInfo& GetBinaryOpInfo(TokenType type); template -ExprNode PrattParserWorker::BuildBinaryCall(int64_t op_id, - absl::string_view op_name, - ExprNode lhs, - ExprNode rhs) { +void PrattParserWorker::BuildBinaryCall(int64_t op_id, + absl::string_view op_name, + ExprNode& lhs, ExprNode rhs) { std::vector args; args.reserve(2); args.push_back(std::move(lhs)); args.push_back(std::move(rhs)); - return ast_factory_.NewCall(op_id, std::string(op_name), std::move(args)); + lhs = ast_factory_.NewCall(op_id, std::string(op_name), std::move(args)); } // Parses binary operator expressions and ternary conditional expressions @@ -388,7 +393,7 @@ ExprNode PrattParserWorker::ParseBinaryAndTernary(int min_prec) { while (true) { TokenType tok = peek_token_.type; if (tok == TokenType::kQuestion && min_prec <= 0) { - lhs = ParseTernary(std::move(lhs)); + ParseTernary(lhs); continue; } @@ -396,14 +401,14 @@ ExprNode PrattParserWorker::ParseBinaryAndTernary(int min_prec) { if (op_info.precedence < min_prec || op_info.precedence == 0) break; if (op_info.is_logical) { - lhs = ParseBalancedLogicalChain(std::move(lhs), op_info); + ParseBalancedLogicalChain(lhs, op_info); continue; } Token op_tok = NextToken(); int64_t op_id = NextId(op_tok); - ExprNode rhs = ParseBinaryAndTernary(op_info.precedence + 1); - lhs = BuildBinaryCall(op_id, op_info.name, std::move(lhs), std::move(rhs)); + BuildBinaryCall(op_id, op_info.name, lhs, + ParseBinaryAndTernary(op_info.precedence + 1)); } return lhs; } @@ -411,8 +416,8 @@ ExprNode PrattParserWorker::ParseBinaryAndTernary(int min_prec) { // Parses continuous chains of logical operators (`&&`, `||`) iteratively // (e.g., `a && b && c`) and constructs a balanced or variadic AST. template -ExprNode PrattParserWorker::ParseBalancedLogicalChain( - ExprNode lhs, const BinaryOpInfo& op_info) { +void PrattParserWorker::ParseBalancedLogicalChain( + ExprNode& lhs, const BinaryOpInfo& op_info) { std::vector terms; std::vector ops; terms.push_back(std::move(lhs)); @@ -422,8 +427,8 @@ ExprNode PrattParserWorker::ParseBalancedLogicalChain( ops.push_back(NextId(op_tok)); terms.push_back(std::move(rhs)); } - return BalanceLogical(op_info.name, std::move(terms), std::move(ops), - options_.enable_variadic_logical_operators); + lhs = BalanceLogical(op_info.name, std::move(terms), std::move(ops), + options_.enable_variadic_logical_operators); } template @@ -432,7 +437,7 @@ ExprNode PrattParserWorker::ParseSelectorChain() { TokenType tok = peek_token_.type; if (tok == TokenType::kDot || tok == TokenType::kLeftBracket || tok == TokenType::kLeftBrace) { - return ParseSelectorChainTail(std::move(lhs)); + ParseSelectorChainTail(lhs); } return lhs; } @@ -440,7 +445,7 @@ ExprNode PrattParserWorker::ParseSelectorChain() { // Parses prefix and postfix member/indexing operations iteratively // (e.g., `!a.b[0].c(x)`). template -ExprNode PrattParserWorker::ParseSelectorChainTail(ExprNode lhs) { +void PrattParserWorker::ParseSelectorChainTail(ExprNode& lhs) { while (true) { TokenType tok = peek_token_.type; if (tok == TokenType::kDot) { @@ -460,7 +465,7 @@ ExprNode PrattParserWorker::ParseSelectorChainTail(ExprNode lhs) { ReportError(id_tok, "expected identifier after '.'"); } SynchronizeOnDelimiter(); - return lhs; + return; } bool is_member_call = peek_token_.type == TokenType::kLeftParen; std::string id_text = @@ -472,7 +477,8 @@ ExprNode PrattParserWorker::ParseSelectorChainTail(ExprNode lhs) { args.push_back(std::move(lhs)); args.push_back( ast_factory_.NewStringConst(NextId(id_tok), std::move(id_text))); - lhs = ast_factory_.NewCall(op_id, "_?._", std::move(args)); + lhs = ast_factory_.NewCall(op_id, CelOperator::OPT_SELECT, + std::move(args)); } else if (peek_token_.type == TokenType::kLeftParen) { Token lparen = NextToken(); int64_t call_id = NextId(lparen); @@ -505,8 +511,9 @@ ExprNode PrattParserWorker::ParseSelectorChainTail(ExprNode lhs) { args.reserve(2); args.push_back(std::move(lhs)); args.push_back(std::move(index)); - lhs = ast_factory_.NewCall(op_id, optional ? "_[?_]" : CelOperator::INDEX, - std::move(args)); + lhs = ast_factory_.NewCall( + op_id, optional ? CelOperator::OPT_INDEX : CelOperator::INDEX, + std::move(args)); } else if (tok == TokenType::kLeftBrace) { int32_t struct_pos = GetLeftmostPosition(lhs); if (auto struct_name = ExtractStructName(lhs); struct_name.has_value()) { @@ -518,7 +525,6 @@ ExprNode PrattParserWorker::ParseSelectorChainTail(ExprNode lhs) { break; } } - return lhs; } template @@ -571,10 +577,14 @@ ExprNode PrattParserWorker::ParseUnaryOpsChain(Token first_op) { template ExprNode PrattParserWorker::ParseUnary() { TokenType tok = peek_token_.type; - if (tok != TokenType::kExclamation && tok != TokenType::kMinus) { - return ParsePrimary(); + if (tok == TokenType::kExclamation || tok == TokenType::kMinus) { + return ParseUnaryOps(); } + return ParsePrimary(); +} +template +ExprNode PrattParserWorker::ParseUnaryOps() { Token op = NextToken(); TokenType op_type = op.type; if (peek_token_.type == TokenType::kExclamation || @@ -647,52 +657,59 @@ ExprNode PrattParserWorker::ParseIdentOrCall() { // (`[...]`, `{...}`), and identifiers/global function calls (`foo`, // `has(x.y)`). template -ABSL_ATTRIBUTE_ALWAYS_INLINE inline ExprNode -PrattParserWorker::ParsePrimary() { - ExprNode expr; - TokenType tok_type = peek_token_.type; - if (tok_type == TokenType::kLeftParen) { - NextToken(); - expr = ParseExpr(); - Expect(TokenType::kRightParen); - } else if (tok_type == TokenType::kNull) { - Token tok = NextToken(); - expr = ast_factory_.NewNullConst(NextId(tok)); - } else if (tok_type == TokenType::kTrue || tok_type == TokenType::kFalse) { - Token tok = NextToken(); - expr = ast_factory_.NewBoolConst(NextId(tok), tok_type == TokenType::kTrue); - } else if (tok_type == TokenType::kInt) { - expr = ParseIntLiteral(); - } else if (tok_type == TokenType::kUint) { - expr = ParseUintLiteral(); - } else if (tok_type == TokenType::kFloat) { - expr = ParseDoubleLiteral(); - } else if (tok_type == TokenType::kString) { - expr = ParseStringLiteral(); - } else if (tok_type == TokenType::kBytes) { - expr = ParseBytesLiteral(); - } else if (tok_type == TokenType::kLeftBracket) { - expr = ParseList(); - } else if (tok_type == TokenType::kLeftBrace) { - expr = ParseMap(); - } else if (tok_type == TokenType::kDot || tok_type == TokenType::kIdent || - tok_type == TokenType::kReservedWord) { - expr = ParseIdentOrCall(); - } else { - Token bad_tok = NextToken(); - if (bad_tok.type != TokenType::kError) { - if (bad_tok.type == TokenType::kEnd) { - ReportError( - bad_tok, - "Syntax error: mismatched input '' expecting expression"); - } else { - ReportError(bad_tok, "unexpected token"); +ExprNode PrattParserWorker::ParsePrimary() { + switch (peek_token_.type) { + case TokenType::kLeftParen: { + int grouping_paren_count = CountGroupingParentheses(); + for (int i = 0; i < grouping_paren_count; ++i) { + NextToken(); + } + ExprNode expr = ParseExpr(); + for (int i = 0; i < grouping_paren_count; ++i) { + Expect(TokenType::kRightParen); + } + return expr; + } + case TokenType::kNull: + return ast_factory_.NewNullConst(NextId(NextToken())); + case TokenType::kTrue: + case TokenType::kFalse: { + Token tok = NextToken(); + return ast_factory_.NewBoolConst(NextId(tok), + tok.type == TokenType::kTrue); + } + case TokenType::kInt: + return ParseIntLiteral(); + case TokenType::kUint: + return ParseUintLiteral(); + case TokenType::kFloat: + return ParseDoubleLiteral(); + case TokenType::kString: + return ParseStringLiteral(); + case TokenType::kBytes: + return ParseBytesLiteral(); + case TokenType::kLeftBracket: + return ParseList(); + case TokenType::kLeftBrace: + return ParseMap(); + case TokenType::kDot: + case TokenType::kIdent: + case TokenType::kReservedWord: + return ParseIdentOrCall(); + default: { + Token bad_tok = NextToken(); + if (bad_tok.type != TokenType::kError) { + if (bad_tok.type == TokenType::kEnd) { + ReportError( + bad_tok, + "Syntax error: mismatched input '' expecting expression"); + } else { + ReportError(bad_tok, "unexpected token"); + } } + return ast_factory_.NewUnspecified(NextId(bad_tok)); } - expr = ast_factory_.NewUnspecified(NextId(bad_tok)); } - - return expr; } // Parses list creation literals (e.g., `[1, 2, ?3]`). @@ -711,8 +728,7 @@ ExprNode PrattParserWorker::ParseList() { ReportError(q, "unsupported syntax '?'"); } } - ExprNode elem = ParseExpr(); - builder.Add(std::move(elem), optional); + builder.Add(ParseExpr(), optional); if (peek_token_.type == TokenType::kComma) { NextToken(); } else { @@ -749,8 +765,7 @@ ExprNode PrattParserWorker::ParseMap() { break; } int64_t entry_id = NextId(colon); - ExprNode val = ParseExpr(); - builder.Add(entry_id, std::move(key), std::move(val), optional); + builder.Add(entry_id, std::move(key), ParseExpr(), optional); if (peek_token_.type == TokenType::kComma) { NextToken(); } else { @@ -793,8 +808,7 @@ ExprNode PrattParserWorker::ParseStruct( break; } int64_t field_id = NextId(colon); - ExprNode val = ParseExpr(); - builder.Add(field_id, std::move(field_name), std::move(val), optional); + builder.Add(field_id, std::move(field_name), ParseExpr(), optional); if (peek_token_.type == TokenType::kComma) { NextToken(); } else { @@ -1172,6 +1186,75 @@ void PrattParserWorker::RecordMacroCall( macro_calls_.insert({macro_id, std::move(call_expr)}); } +// Scans ahead in the token stream to detect contiguous grouping +// parentheses (e.g., `((((expr))))`). By determining the number of outermost +// parentheses that enclose the exact same expression and close contiguously, +// the parser unnests them in a single C++ stack frame, avoiding deep recursive +// descent. +template +int PrattParserWorker::CountGroupingParentheses() { + if (peek_token_.type != TokenType::kLeftParen) { + return 0; + } + + // Save lexer position to restore after scanning ahead. + const Lexer::Position saved_pos = lexer_.SavePosition(); + auto restore_lexer = absl::MakeCleanup( + [this, saved_pos] { lexer_.RestorePosition(saved_pos); }); + + int leading_open_parens = 1; + Token tok = this->NextSignificantToken(/*report_error=*/false); + while (tok.type == TokenType::kLeftParen) { + leading_open_parens++; + tok = this->NextSignificantToken(/*report_error=*/false); + } + if (leading_open_parens == 1) { + return 1; + } + + int open_parens = leading_open_parens; + int consecutive_leading_closed = 0; + + while (open_parens > 0) { + if (tok.type == TokenType::kEnd || tok.type == TokenType::kError) { + // Return 1 to ensure the parser consumes '(' and standard error handling + // catches incomplete expressions like `(ident`. + return 1; + } + + if (tok.type == TokenType::kLeftParen) { + // An inner parenthesis opens within the expression + // (e.g. `(x` in `((1 + (x) ))`). + open_parens++; + consecutive_leading_closed = 0; + } else if (tok.type == TokenType::kRightParen) { + if (leading_open_parens == open_parens) { + // All inner parentheses are balanced, so this ')' closes one of the + // initial leading '(' parentheses (e.g. trailing ')' in `(((expr)))`). + leading_open_parens--; + consecutive_leading_closed++; + } else { + // This ')' closes an inner nested parenthesis (e.g. `(1 + 2)` in + // `((1 + 2) * 3)`), not one of the outermost leading parentheses. + consecutive_leading_closed = 0; + } + open_parens--; + } else { + // Non-parenthesis token (identifier, operator, literal, etc.). Any + // preceding ')' did not close the entire expression, so reset the + // contiguous outer closing count. + consecutive_leading_closed = 0; + } + + if (open_parens > 0) { + tok = this->NextSignificantToken(/*report_error=*/false); + } + } + + // Return at least 1 to make sure we catch unclosed expressions like `(ident`. + return std::max(1, consecutive_leading_closed); +} + } // namespace cel::parser_internal #endif // THIRD_PARTY_CEL_CPP_PARSER_INTERNAL_PRATT_PARSER_WORKER_H_ diff --git a/parser/parser_test.cc b/parser/parser_test.cc index a7a2eb442..f7870b943 100644 --- a/parser/parser_test.cc +++ b/parser/parser_test.cc @@ -1836,8 +1836,9 @@ TEST_P(ExpressionImplTest, RecursionDepthLongArgList) { EXPECT_THAT(Parse("[1, 2, 3, 4, 5, 6, 7, 8, 9, 10]", "", options_), IsOk()); } -TEST(ExpressionTest, RecursionDepthExceeded) { +TEST(ExpressionTest, RecursionDepthExceeded_AntlrOnly) { ParserOptions options; + options.enable_pratt_parser = false; // AST visitor will recurse a variable amount depending on the terms used in // the expression. This check occurs in the business logic converting the raw // Antlr parse tree into an Expr. There is a separate check (via a custom @@ -1877,10 +1878,9 @@ TEST_P(ExpressionImplTest, DisableStandardMacros) { << adorned_string; } -TEST(ExpressionTest, RecursionDepthIgnoresParentheses) { - ParserOptions options; - options.max_recursion_depth = 6; - auto result = Parse("(((1 + 2 + 3 + 4 + (5 + 6))))", "", options); +TEST_P(ExpressionImplTest, RecursionDepthIgnoresParentheses) { + options_.max_recursion_depth = options_.enable_pratt_parser ? 2 : 6; + auto result = Parse("(((1 + 2 + 3 + 4 + (5 + 6))))", "", options_); EXPECT_THAT(result, IsOk()); }