diff --git a/parser/internal/pratt_parser.cc b/parser/internal/pratt_parser.cc index c321fa7b8..e83c21fce 100644 --- a/parser/internal/pratt_parser.cc +++ b/parser/internal/pratt_parser.cc @@ -50,9 +50,11 @@ namespace { std::string DisplayParserError(const cel::Source& source, SourceLocation location, std::string_view message) { + int32_t display_column = + location.column >= 0 ? location.column + 1 : location.column; return absl::StrCat( - absl::StrFormat("ERROR: %s:%zu:%zu: %s", source.description(), - location.line, location.column + 1, message), + absl::StrFormat("ERROR: %s:%d:%d: %s", source.description(), + location.line, display_column, message), source.DisplayErrorLocation(location)); } diff --git a/parser/internal/pratt_parser_test.cc b/parser/internal/pratt_parser_test.cc index 708936a4e..5dd3bb182 100644 --- a/parser/internal/pratt_parser_test.cc +++ b/parser/internal/pratt_parser_test.cc @@ -1055,7 +1055,7 @@ std::string FormatIssues(const cel::Source& source, issues, "\n", [&source](std::string* out, const cel::ParseIssue& issue) { absl::StrAppend( out, - absl::StrFormat("ERROR: %s:%zu:%zu: %s", source.description(), + absl::StrFormat("ERROR: %s:%d:%d: %s", source.description(), issue.location().line, issue.location().column + 1, issue.message()), source.DisplayErrorLocation(issue.location())); @@ -1576,53 +1576,6 @@ INSTANTIATE_TEST_SUITE_P(PrattParserMacroTest, PrattParserMacroTest, testing::ValuesIn(GetMacroTestCases()), TestName); -TEST(PrattParserMacroErrorTest, ReportError) { - auto builder = NewPrattParserBuilder(); - ASSERT_OK_AND_ASSIGN( - auto error_macro, - Macro::Global("bad_macro", 1, - [](MacroExprFactory& macro_factory, - absl::Span args) -> std::optional { - return macro_factory.ReportError("custom macro error"); - })); - - ASSERT_THAT(builder->AddMacro(error_macro), IsOk()); - ASSERT_OK_AND_ASSIGN(auto parser, builder->Build()); - - ASSERT_OK_AND_ASSIGN(auto source, cel::NewSource("42 + bad_macro(x)")); - std::vector issues; - auto ast = parser->Parse(*source, &issues); - EXPECT_THAT(ast, StatusIs(absl::StatusCode::kInvalidArgument)); - EXPECT_EQ(FormatIssues(*source, issues), - "ERROR: :1:6: custom macro error\n" - " | 42 + bad_macro(x)\n" - " | .....^"); -} - -TEST(PrattParserMacroErrorTest, ReportErrorAt) { - auto builder = NewPrattParserBuilder(); - ASSERT_OK_AND_ASSIGN( - auto error_at_macro, - Macro::Global("bad_macro_at", 1, - [](MacroExprFactory& macro_factory, - absl::Span args) -> std::optional { - return macro_factory.ReportErrorAt(args[0], - "custom error at arg"); - })); - - ASSERT_THAT(builder->AddMacro(error_at_macro), IsOk()); - ASSERT_OK_AND_ASSIGN(auto parser, builder->Build()); - - ASSERT_OK_AND_ASSIGN(auto source, cel::NewSource("bad_macro_at(x)")); - std::vector issues; - auto ast = parser->Parse(*source, &issues); - EXPECT_THAT(ast, StatusIs(absl::StatusCode::kInvalidArgument)); - EXPECT_EQ(FormatIssues(*source, issues), - "ERROR: :1:14: custom error at arg\n" - " | bad_macro_at(x)\n" - " | .............^"); -} - TEST(PrattParserMacroCallsTest, MacroCallsDisabledByDefault) { cel::ParserOptions options; options.add_macro_calls = false; @@ -1737,5 +1690,77 @@ TEST(PrattParserMacroCallsTest, NestedMacroCallsUseCopyAndReplaceReplacer) { )")); } +TEST(PrattParserMacroErrorTest, ReportError) { + auto builder = NewPrattParserBuilder(); + ASSERT_OK_AND_ASSIGN( + auto error_macro, + Macro::Global("bad_macro", 1, + [](MacroExprFactory& macro_factory, + absl::Span args) -> std::optional { + return macro_factory.ReportError("custom macro error"); + })); + + ASSERT_THAT(builder->AddMacro(error_macro), IsOk()); + ASSERT_OK_AND_ASSIGN(auto parser, builder->Build()); + + ASSERT_OK_AND_ASSIGN(auto source, cel::NewSource("42 + bad_macro(x)")); + std::vector issues; + auto ast = parser->Parse(*source, &issues); + EXPECT_THAT(ast, StatusIs(absl::StatusCode::kInvalidArgument)); + EXPECT_EQ(FormatIssues(*source, issues), + "ERROR: :1:6: custom macro error\n" + " | 42 + bad_macro(x)\n" + " | .....^"); +} + +TEST(PrattParserMacroErrorTest, ReportErrorAt) { + auto builder = NewPrattParserBuilder(); + ASSERT_OK_AND_ASSIGN( + auto error_at_macro, + Macro::Global("bad_macro_at", 1, + [](MacroExprFactory& macro_factory, + absl::Span args) -> std::optional { + return macro_factory.ReportErrorAt(args[0], + "custom error at arg"); + })); + + ASSERT_THAT(builder->AddMacro(error_at_macro), IsOk()); + ASSERT_OK_AND_ASSIGN(auto parser, builder->Build()); + + ASSERT_OK_AND_ASSIGN(auto source, cel::NewSource("bad_macro_at(x)")); + std::vector issues; + auto ast = parser->Parse(*source, &issues); + EXPECT_THAT(ast, StatusIs(absl::StatusCode::kInvalidArgument)); + EXPECT_EQ(FormatIssues(*source, issues), + "ERROR: :1:14: custom error at arg\n" + " | bad_macro_at(x)\n" + " | .............^"); +} + +TEST(PrattParserErrorRecoveryTest, ErrorRecoveryLimitZero) { + cel::ParserOptions options; + options.error_recovery_limit = 0; + std::vector issues; + auto result = Parse("......", options, &issues); + EXPECT_THAT(result, StatusIs(absl::StatusCode::kInvalidArgument)); + ASSERT_OK_AND_ASSIGN(auto source, cel::NewSource("......")); + EXPECT_EQ(FormatIssues(*source, issues), + "ERROR: :-1:0: Error recovery limit (0) exceeded"); +} + +TEST(PrattParserErrorRecoveryTest, ErrorRecoveryLimitOne) { + cel::ParserOptions options; + options.error_recovery_limit = 1; + std::vector issues; + auto result = Parse("......", options, &issues); + EXPECT_THAT(result, StatusIs(absl::StatusCode::kInvalidArgument)); + ASSERT_OK_AND_ASSIGN(auto source, cel::NewSource("......")); + EXPECT_EQ(FormatIssues(*source, issues), + "ERROR: :1:2: expected identifier\n" + " | ......\n" + " | .^\n" + "ERROR: :-1:0: Error recovery limit (1) exceeded"); +} + } // namespace } // namespace cel::parser_internal diff --git a/parser/internal/pratt_parser_worker.cc b/parser/internal/pratt_parser_worker.cc index 4e40adc1e..8dece29a8 100644 --- a/parser/internal/pratt_parser_worker.cc +++ b/parser/internal/pratt_parser_worker.cc @@ -20,6 +20,7 @@ #include "absl/base/nullability.h" #include "absl/strings/str_cat.h" +#include "absl/strings/str_format.h" #include "absl/strings/string_view.h" #include "common/source.h" #include "parser/internal/lexer.h" @@ -50,6 +51,9 @@ std::string ParserWorker::GetTokenText(const Token& tok) const { } Token ParserWorker::NextSignificantToken() { + if (is_recovery_limit_exceeded()) { + return Token{.type = TokenType::kEnd, .start = 0, .end = 0}; + } while (true) { Token tok = lexer_.Lex(); if (tok.type == TokenType::kWhitespace || tok.type == TokenType::kComment) { @@ -57,6 +61,9 @@ Token ParserWorker::NextSignificantToken() { } if (tok.type == TokenType::kError) { ReportError(tok, lexer_.GetError().message); + if (is_recovery_limit_exceeded()) { + return Token{.type = TokenType::kEnd, .start = 0, .end = 0}; + } } return tok; } @@ -64,6 +71,10 @@ Token ParserWorker::NextSignificantToken() { Token ParserWorker::NextToken() { current_token_ = peek_token_; + if (is_recovery_limit_exceeded()) { + peek_token_ = Token{.type = TokenType::kEnd, .start = 0, .end = 0}; + return current_token_; + } if (peek_token_.type != TokenType::kEnd) { peek_token_ = NextSignificantToken(); } @@ -75,6 +86,9 @@ bool ParserWorker::Expect(TokenType type, absl::string_view msg) { NextToken(); return true; } + if (is_recovery_limit_exceeded()) { + return false; + } if (peek_token_.type != TokenType::kError) { std::string err_msg; if (msg.empty()) { @@ -98,9 +112,7 @@ bool ParserWorker::Expect(TokenType type, absl::string_view msg) { void ParserWorker::SynchronizeOnDelimiter() { if (is_recovery_limit_exceeded()) { - while (peek_token_.type != TokenType::kEnd) { - NextToken(); - } + peek_token_ = Token{.type = TokenType::kEnd, .start = 0, .end = 0}; return; } while (peek_token_.type != TokenType::kEnd) { @@ -149,8 +161,20 @@ void ParserWorker::ReportError(int32_t position, absl::string_view msg) { void ParserWorker::ReportError(const SourceLocation& loc, absl::string_view msg) { + if (error_count_ > options_.error_recovery_limit) { + return; + } error_count_++; - if (parse_issues_ != nullptr) { + if (error_count_ == options_.error_recovery_limit + 1) { + if (parse_issues_ != nullptr) { + parse_issues_->push_back( + cel::ParseIssue(absl::StrFormat("Error recovery limit (%d) exceeded", + options_.error_recovery_limit))); + } + peek_token_ = Token{.type = TokenType::kEnd, .start = 0, .end = 0}; + } + if (parse_issues_ != nullptr && + error_count_ <= options_.error_recovery_limit) { parse_issues_->push_back(cel::ParseIssue(loc, std::string(msg))); } } diff --git a/parser/internal/pratt_parser_worker.h b/parser/internal/pratt_parser_worker.h index 45221714f..a7dee687c 100644 --- a/parser/internal/pratt_parser_worker.h +++ b/parser/internal/pratt_parser_worker.h @@ -81,7 +81,7 @@ class ParserWorker { // Error reporting and recovery bool is_recovery_limit_exceeded() const { - return error_count_ >= options_.error_recovery_limit; + return error_count_ > options_.error_recovery_limit; } void ReportError(int32_t position, absl::string_view msg); void ReportError(const SourceLocation& loc, absl::string_view msg); @@ -214,7 +214,7 @@ class PrattParserWorker : public ParserWorker { template ExprNode PrattParserWorker::Parse() { ExprNode expr = ParseExpr(); - if (is_recursion_limit_exceeded()) { + if (is_recursion_limit_exceeded() || is_recovery_limit_exceeded()) { return expr; } if (peek_token_.type != TokenType::kEnd && @@ -226,7 +226,7 @@ ExprNode PrattParserWorker::Parse() { template ExprNode PrattParserWorker::ParseExpr() { - if (recursion_limit_exceeded_) { + if (recursion_limit_exceeded_ || is_recovery_limit_exceeded()) { return ExprNode(); } if (recursion_depth_ > options_.max_recursion_depth) { diff --git a/parser/parser_test.cc b/parser/parser_test.cc index 35f11b413..357a83a79 100644 --- a/parser/parser_test.cc +++ b/parser/parser_test.cc @@ -1584,7 +1584,7 @@ TEST(ExpressionTest, TsanOom) { .IgnoreError(); } -TEST(ExpressionTest, ErrorRecoveryLimits) { +TEST_P(ExpressionTest, ErrorRecoveryLimits) { ParserOptions options; options.error_recovery_limit = 1; auto result = Parse("......", "", options);