[Pratt Parser] Avoid incrementing recursion depth for non-functional parens

PiperOrigin-RevId: 953065581

Author: dmitriplotnikov

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",

parser/internal/lexer.h

@@ -140,6 +140,13 @@ class Lexer final {
                                         std::numeric_limits<int32_t>::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;

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

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;

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};

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<ExprNode> target,
                        std::vector<ExprNode> arguments);
 
+  int CountGroupingParentheses();
+
   AstFactoryInterface<ExprNode>& ast_factory_;
   absl::flat_hash_map<int64_t, ExprNode> macro_calls_;
 };
@@ -652,9 +655,14 @@ PrattParserWorker<ExprNode>::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<ExprNode>::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 <typename ExprNode>
+int PrattParserWorker<ExprNode>::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_

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());
 }