diff --git a/parser/internal/pratt_parser_benchmark.cc b/parser/internal/pratt_parser_benchmark.cc index 952585306..53d451bf1 100644 --- a/parser/internal/pratt_parser_benchmark.cc +++ b/parser/internal/pratt_parser_benchmark.cc @@ -222,9 +222,46 @@ void BM_Antlr_ParseNestedParentheses(benchmark::State& state) { BM_ParseNestedParentheses(state, ParserImplType::kAntlr); } -BENCHMARK(BM_Pratt_ParseNestedParentheses)->Arg(10)->Arg(50); +BENCHMARK(BM_Pratt_ParseNestedParentheses)->Arg(10)->Arg(50)->Arg(200); BENCHMARK(BM_Antlr_ParseNestedParentheses)->Arg(10)->Arg(50); +// ----------------------------------------------------------------------------- +// Workload 5b: Deeply Nested Left Parentheses with Calc ("((((a + 1) + 1))") +// ----------------------------------------------------------------------------- +std::string BuildNestedLeftParenthesesCalc(int depth) { + std::string expr(depth, '('); + absl::StrAppend(&expr, "1 + 2"); + for (int i = 0; i < depth; ++i) { + absl::StrAppend(&expr, ") + 1"); + } + return expr; +} + +void BM_ParseNestedLeftParenthesesCalc(benchmark::State& state, + ParserImplType type) { + cel::ParserOptions options; + auto parser = CreateParser(type, options); + std::string expr = BuildNestedLeftParenthesesCalc(state.range(0)); + + for (auto _ : state) { + auto source = cel::NewSource(expr); + ABSL_DCHECK_OK(source.status()); + auto ast = parser->Parse(**source); + ABSL_DCHECK_OK(ast.status()); + benchmark::DoNotOptimize(ast); + } +} + +void BM_Pratt_ParseNestedLeftParenthesesCalc(benchmark::State& state) { + BM_ParseNestedLeftParenthesesCalc(state, ParserImplType::kPratt); +} +void BM_Antlr_ParseNestedLeftParenthesesCalc(benchmark::State& state) { + BM_ParseNestedLeftParenthesesCalc(state, ParserImplType::kAntlr); +} + +BENCHMARK(BM_Pratt_ParseNestedLeftParenthesesCalc)->Arg(10)->Arg(50)->Arg(200); +BENCHMARK(BM_Antlr_ParseNestedLeftParenthesesCalc)->Arg(10)->Arg(50); + // ----------------------------------------------------------------------------- // Workload 6: Common Representative Expressions with Syntax Errors // ----------------------------------------------------------------------------- diff --git a/parser/internal/pratt_parser_test.cc b/parser/internal/pratt_parser_test.cc index fc6477745..60659e179 100644 --- a/parser/internal/pratt_parser_test.cc +++ b/parser/internal/pratt_parser_test.cc @@ -1714,6 +1714,12 @@ TEST(PrattParserRecursionDepthTest, DeeplyNestedParens) { std::string binary_expr = std::string(1000, '(') + "1 + 2" + std::string(1000, ')'); EXPECT_THAT(Parse(binary_expr, options), IsOkAndHolds(NotNull())); + + std::string left_nested_calc_expr = std::string(1000, '(') + "1 + 2"; + for (int i = 0; i < 1000; ++i) { + left_nested_calc_expr += ") + 1"; + } + EXPECT_THAT(Parse(left_nested_calc_expr, options), IsOkAndHolds(NotNull())); } TEST(PrattParserRecursionDepthTest, NestedAndGroupingParensCombinations) { @@ -1724,6 +1730,7 @@ TEST(PrattParserRecursionDepthTest, NestedAndGroupingParensCombinations) { 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]")); + EXPECT_THAT("((((a).b[0]) + 1) ? 2 : 3)", AstEq("(a.b[0] + 1) ? 2 : 3")); } TEST(PrattParserRecursionDepthTest, MismatchedParensStillReportErrors) { @@ -1742,6 +1749,15 @@ TEST(PrattParserRecursionDepthTest, SequentialScopesDoNotAccumulateDepth) { EXPECT_THAT(Parse("[1] + [2] + [3]", options), IsOkAndHolds(NotNull())); } +TEST(PrattParserRecursionDepthTest, DeeplyNestedTernary) { + cel::ParserOptions options; + options.max_recursion_depth = 4; + EXPECT_THAT(Parse("a ? b : a ? b : a ? b : a ? b : c", options), + IsOkAndHolds(NotNull())); + EXPECT_THAT(Parse("a ? b : a ? b : a ? b : a ? b : a ? b : c", options), + StatusIs(absl::StatusCode::kCancelled)); +} + class TestParserWorker : public ParserWorker { // Expose the protected constructor and methods for testing. public: diff --git a/parser/internal/pratt_parser_worker.cc b/parser/internal/pratt_parser_worker.cc index b01408774..1a086c421 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(bool report_error) { +Token ParserWorker::NextSignificantToken() { if (is_recovery_limit_exceeded()) { return Token{.type = TokenType::kEnd, .start = 0, .end = 0}; } @@ -132,7 +132,7 @@ Token ParserWorker::NextSignificantToken(bool report_error) { if (tok.type == TokenType::kWhitespace || tok.type == TokenType::kComment) { continue; } - if (tok.type == TokenType::kError && report_error) { + if (tok.type == TokenType::kError) { ReportSyntaxError(tok, lexer_.GetError().message); if (is_recovery_limit_exceeded()) { return Token{.type = TokenType::kEnd, .start = 0, .end = 0}; @@ -159,7 +159,7 @@ bool ParserWorker::Expect(TokenType type, absl::string_view msg) { NextToken(); return true; } - if (is_recovery_limit_exceeded()) { + if (recursion_limit_exceeded_ || is_recovery_limit_exceeded()) { return false; } if (peek_token_.type != TokenType::kError) { @@ -184,7 +184,7 @@ bool ParserWorker::Expect(TokenType type, absl::string_view msg) { } void ParserWorker::SynchronizeOnDelimiter() { - if (is_recovery_limit_exceeded()) { + if (recursion_limit_exceeded_ || is_recovery_limit_exceeded()) { peek_token_ = Token{.type = TokenType::kEnd, .start = 0, .end = 0}; return; } diff --git a/parser/internal/pratt_parser_worker.h b/parser/internal/pratt_parser_worker.h index 2cfe84b79..63ee80c0d 100644 --- a/parser/internal/pratt_parser_worker.h +++ b/parser/internal/pratt_parser_worker.h @@ -15,7 +15,6 @@ #ifndef THIRD_PARTY_CEL_CPP_PARSER_INTERNAL_PRATT_PARSER_WORKER_H_ #define THIRD_PARTY_CEL_CPP_PARSER_INTERNAL_PRATT_PARSER_WORKER_H_ -#include #include #include #include @@ -74,7 +73,7 @@ class ParserWorker { const cel::ParserOptions& options() const { return options_; } // Token stream management void InitTokenStream(); - Token NextSignificantToken(bool report_error = true); + Token NextSignificantToken(); Token NextToken(); bool Expect(TokenType type, absl::string_view msg = ""); std::string GetTokenText(const Token& tok) const; @@ -221,6 +220,7 @@ class PrattParserWorker : public ParserWorker { // `?`, consumes `?`, and recurses with `ParseBinary(1)` for true branch `b` // and `ParseBinary(0)` for false branch `c`. ExprNode ParseBinaryAndTernary(int min_prec); + void ParseBinaryAndTernaryFromLhs(ExprNode& lhs, int min_prec); // Parses ternary conditional expressions (`condition ? true_expr : // false_expr`). @@ -322,8 +322,6 @@ class PrattParserWorker : public ParserWorker { std::optional target, std::vector arguments); - int CountGroupingParentheses(); - AstFactoryInterface& ast_factory_; absl::flat_hash_map macro_calls_; }; @@ -348,6 +346,7 @@ ExprNode PrattParserWorker::ParseExpr() { } if (recursion_depth_ > options_.max_recursion_depth) { recursion_limit_exceeded_ = true; + peek_token_ = Token{.type = TokenType::kEnd, .start = 0, .end = 0}; return ExprNode(); } recursion_depth_++; @@ -358,18 +357,22 @@ ExprNode PrattParserWorker::ParseExpr() { template void PrattParserWorker::ParseTernary(ExprNode& lhs) { - Token op_tok = NextToken(); - int64_t op_id = NextId(op_tok); - ExprNode true_expr = ParseBinaryAndTernary(1); - if (!Expect(TokenType::kColon, "expected ':' in conditional expression")) { + if (recursion_depth_ > options_.max_recursion_depth) { + recursion_limit_exceeded_ = true; + peek_token_ = Token{.type = TokenType::kEnd, .start = 0, .end = 0}; return; } - ExprNode false_expr = ParseBinaryAndTernary(0); + recursion_depth_++; + absl::Cleanup depth_cleanup = [this] { recursion_depth_--; }; + int64_t op_id = NextId(NextToken()); std::vector args; args.reserve(3); args.push_back(std::move(lhs)); - args.push_back(std::move(true_expr)); - args.push_back(std::move(false_expr)); + args.push_back(ParseBinaryAndTernary(1)); + if (!Expect(TokenType::kColon, "expected ':' in conditional expression")) { + return; + } + args.push_back(ParseBinaryAndTernary(0)); lhs = ast_factory_.NewCall(op_id, CelOperator::CONDITIONAL, std::move(args)); } @@ -391,7 +394,14 @@ void PrattParserWorker::BuildBinaryCall(int64_t op_id, template ExprNode PrattParserWorker::ParseBinaryAndTernary(int min_prec) { ExprNode lhs = ParseSelectorChain(); - while (true) { + ParseBinaryAndTernaryFromLhs(lhs, min_prec); + return lhs; +} + +template +void PrattParserWorker::ParseBinaryAndTernaryFromLhs(ExprNode& lhs, + int min_prec) { + while (!recursion_limit_exceeded_ && !is_recovery_limit_exceeded()) { TokenType tok = peek_token_.type; if (tok == TokenType::kQuestion && min_prec <= 0) { ParseTernary(lhs); @@ -411,7 +421,6 @@ ExprNode PrattParserWorker::ParseBinaryAndTernary(int min_prec) { BuildBinaryCall(op_id, op_info.name, lhs, ParseBinaryAndTernary(op_info.precedence + 1)); } - return lhs; } // Parses continuous chains of logical operators (`&&`, `||`) iteratively @@ -683,14 +692,42 @@ template ExprNode PrattParserWorker::ParsePrimary() { switch (peek_token_.type) { case TokenType::kLeftParen: { - int grouping_paren_count = CountGroupingParentheses(); - for (int i = 0; i < grouping_paren_count; ++i) { + if (recursion_limit_exceeded_ || is_recovery_limit_exceeded()) { + return ExprNode(); + } + if (recursion_depth_ > options_.max_recursion_depth) { + recursion_limit_exceeded_ = true; + peek_token_ = Token{.type = TokenType::kEnd, .start = 0, .end = 0}; + return ExprNode(); + } + // To avoid deep call-stack recursion on heavily nested parentheses (e.g. + // "((((a))))" or "((((a + 1) + 1) + 1))"), consume all consecutive + // leading '(' tokens upfront, parse the innermost expression once, and + // then iteratively unwind each enclosing '(' from innermost to outermost. + // After consuming each matching ')', if more enclosing '(' remain open + // and the next token is not another ')', continue parsing any trailing + // selectors or binary/ternary operators belonging to that enclosing + // parenthesized level using the already-parsed inner expression as the + // LHS. + int open_parens = 0; + while (peek_token_.type == TokenType::kLeftParen) { + open_parens++; NextToken(); } - ExprNode expr = ParseExpr(); - for (int i = 0; i < grouping_paren_count; ++i) { + recursion_depth_++; + ExprNode expr = ParseBinaryAndTernary(0); + for (int i = 0; i < open_parens; ++i) { Expect(TokenType::kRightParen); + if (i < open_parens - 1 && peek_token_.type != TokenType::kRightParen) { + TokenType tok = peek_token_.type; + if (tok == TokenType::kDot || tok == TokenType::kLeftBracket || + tok == TokenType::kLeftBrace) { + ParseSelectorChainTail(expr); + } + ParseBinaryAndTernaryFromLhs(expr, 0); + } } + recursion_depth_--; return expr; } case TokenType::kNull: @@ -1210,75 +1247,6 @@ 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 int32_t 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 922968d3e..399ea1403 100644 --- a/parser/parser_test.cc +++ b/parser/parser_test.cc @@ -1086,6 +1086,12 @@ std::vector test_cases = { "]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]" "]]]]]]", "", "Expression recursion limit exceeded. limit: 32", "", "", ""}, + {"a ? b : a ? b : a ? b : a ? b : a ? b : a ? b : a ? b : a ? b : " + "a ? b : a ? b : a ? b : a ? b : a ? b : a ? b : a ? b : a ? b : " + "a ? b : a ? b : a ? b : a ? b : a ? b : a ? b : a ? b : a ? b : " + "a ? b : a ? b : a ? b : a ? b : a ? b : a ? b : a ? b : a ? b : " + "a ? b : a ? b : a ? b : a ? b : a ? b : a ? b : a ? b : a ? b : c", + "", "Expression recursion limit exceeded. limit: 32", "", "", ""}, { // Note, the ANTLR parse stack may recurse much more deeply and permit // more detailed expressions than the visitor can recurse over in