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..d6c1b385e 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; @@ -317,6 +318,8 @@ class PrattParserWorker : public ParserWorker { std::optional target, std::vector arguments); + int CountGroupingParentheses(); + AstFactoryInterface& ast_factory_; absl::flat_hash_map macro_calls_; }; @@ -652,9 +655,14 @@ PrattParserWorker::ParsePrimary() { ExprNode expr; TokenType tok_type = peek_token_.type; if (tok_type == TokenType::kLeftParen) { - NextToken(); + int grouping_paren_count = CountGroupingParentheses(); + for (int i = 0; i < grouping_paren_count; ++i) { + NextToken(); + } expr = ParseExpr(); - Expect(TokenType::kRightParen); + for (int i = 0; i < grouping_paren_count; ++i) { + Expect(TokenType::kRightParen); + } } else if (tok_type == TokenType::kNull) { Token tok = NextToken(); expr = ast_factory_.NewNullConst(NextId(tok)); @@ -1172,6 +1180,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()); }