Optionally track structured error messages in parser.

compiler/BUILD

@@ -42,10 +42,12 @@ cc_library(
     hdrs = ["compiler_factory.h"],
     deps = [
         ":compiler",
+        "//checker:type_check_issue",
         "//checker:type_checker",
         "//checker:type_checker_builder",
         "//checker:type_checker_builder_factory",
         "//checker:validation_result",
+        "//common:ast",
         "//common:source",
         "//internal:noop_delete",
         "//internal:status_macros",

compiler/compiler_factory.cc

@@ -17,16 +17,19 @@
 #include <memory>
 #include <string>
 #include <utility>
+#include <vector>
 
 #include "absl/container/flat_hash_set.h"
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
 #include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
+#include "checker/type_check_issue.h"
 #include "checker/type_checker.h"
 #include "checker/type_checker_builder.h"
 #include "checker/type_checker_builder_factory.h"
 #include "checker/validation_result.h"
+#include "common/ast.h"
 #include "common/source.h"
 #include "compiler/compiler.h"
 #include "internal/status_macros.h"
@@ -55,9 +58,26 @@ class CompilerImpl : public Compiler {
       google::protobuf::Arena* arena) const override {
     CEL_ASSIGN_OR_RETURN(auto source,
                          cel::NewSource(expression, std::string(description)));
-    CEL_ASSIGN_OR_RETURN(auto ast, parser_->Parse(*source));
+    std::vector<cel::ParseIssue> parse_issues;
+    absl::StatusOr<std::unique_ptr<cel::Ast>> ast =
+        parser_->Parse(*source, &parse_issues);
+    if (!ast.ok()) {
+      if (ast.status().code() != absl::StatusCode::kInvalidArgument ||
+          parse_issues.empty()) {
+        return ast.status();
+      }
+      std::vector<TypeCheckIssue> check_issues;
+      check_issues.reserve(parse_issues.size());
+      for (const auto& issue : parse_issues) {
+        check_issues.push_back(TypeCheckIssue::CreateError(
+            issue.location(), std::string(issue.message())));
+      }
+      ValidationResult result(std::move(check_issues));
+      result.SetSource(std::move(source));
+      return result;
+    }
     CEL_ASSIGN_OR_RETURN(ValidationResult result,
-                         type_checker_->Check(std::move(ast), arena));
+                         type_checker_->Check(*std::move(ast), arena));
 
     result.SetSource(std::move(source));
     if (!validator_.validations().empty()) {

compiler/compiler_factory_test.cc

@@ -413,5 +413,17 @@ TEST(CompilerFactoryTest, SpecifyArenaKeepsResolvedTypes) {
       it->second.GetOptional().GetParameter().GetList().GetElement().IsInt());
 }
 
+TEST(CompilerFactoryTest, ReturnsIssuesFromParser) {
+  ASSERT_OK_AND_ASSIGN(
+      auto builder,
+      NewCompilerBuilder(cel::internal::GetSharedTestingDescriptorPool()));
+
+  ASSERT_OK_AND_ASSIGN(auto compiler, builder->Build());
+
+  ASSERT_OK_AND_ASSIGN(ValidationResult result, compiler->Compile("a +"));
+  EXPECT_FALSE(result.IsValid());
+  EXPECT_THAT(result.GetIssues(), testing::Not(testing::IsEmpty()));
+}
+
 }  // namespace
 }  // namespace cel

extensions/math_ext_test.cc

@@ -23,7 +23,6 @@
 #include "absl/algorithm/container.h"
 #include "absl/status/status.h"
 #include "absl/status/status_matchers.h"
-#include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
 #include "absl/types/optional.h"
 #include "absl/types/span.h"
@@ -110,19 +109,6 @@ struct MacroTestCase {
   absl::string_view err = "";
 };
 
-std::string FormatIssues(const cel::ValidationResult& result) {
-  std::string issues;
-  for (const auto& issue : result.GetIssues()) {
-    if (!issues.empty()) {
-      absl::StrAppend(&issues, "\n",
-                      issue.ToDisplayString(*result.GetSource()));
-    } else {
-      issues = issue.ToDisplayString(*result.GetSource());
-    }
-  }
-  return issues;
-}
-
 class TestFunction : public CelFunction {
  public:
   explicit TestFunction(absl::string_view name)
@@ -381,16 +367,16 @@ TEST_P(MathExtMacroParamsTest, ParserAndCheckerTests) {
 
   ASSERT_OK_AND_ASSIGN(auto compiler, std::move(*compiler_builder).Build());
 
-  auto result = compiler->Compile(test_case.expr, "<input>");
+  ASSERT_OK_AND_ASSIGN(auto result,
+                       compiler->Compile(test_case.expr, "<input>"));
 
   if (!test_case.err.empty()) {
-    EXPECT_THAT(result.status(), StatusIs(absl::StatusCode::kInvalidArgument,
-                                          HasSubstr(test_case.err)));
+    EXPECT_FALSE(result.IsValid());
+    EXPECT_THAT(result.FormatError(), HasSubstr(test_case.err));
     return;
   }
 
-  ASSERT_THAT(result, IsOk());
-  ASSERT_TRUE(result->IsValid()) << FormatIssues(*result);
+  ASSERT_TRUE(result.IsValid()) << result.FormatError();
 
   RuntimeOptions opts;
   ASSERT_OK_AND_ASSIGN(
@@ -411,9 +397,8 @@ TEST_P(MathExtMacroParamsTest, ParserAndCheckerTests) {
       IsOk());
 
   ASSERT_OK_AND_ASSIGN(auto runtime, std::move(runtime_builder).Build());
-
-  ASSERT_OK_AND_ASSIGN(auto program,
-                       runtime->CreateProgram(*result->ReleaseAst()));
+  ASSERT_OK_AND_ASSIGN(auto ast, result.ReleaseAst());
+  ASSERT_OK_AND_ASSIGN(auto program, runtime->CreateProgram(std::move(ast)));
 
   google::protobuf::Arena arena;
   cel::Activation activation;

parser/BUILD

@@ -244,9 +244,11 @@ cc_library(
         ":options",
         "//common:ast",
         "//common:source",
+        "@com_google_absl//absl/base:nullability",
         "@com_google_absl//absl/functional:any_invocable",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",
+        "@com_google_absl//absl/strings:string_view",
     ],
 )
 

parser/parser.cc

@@ -112,13 +112,12 @@ struct ParserError {
 };
 
 std::string DisplayParserError(const cel::Source& source,
-                               const ParserError& error) {
-  auto location =
-      source.GetLocation(error.range.begin).value_or(SourceLocation{});
+                               SourceLocation location,
+                               absl::string_view message) {
   return absl::StrCat(absl::StrFormat("ERROR: %s:%zu:%zu: %s",
                                       source.description(), location.line,
                                       // add one to the 0-based column
-                                      location.column + 1, error.message),
+                                      location.column + 1, message),
                       source.DisplayErrorLocation(location));
 }
 
@@ -209,7 +208,7 @@ class ParserMacroExprFactory final : public MacroExprFactory {
 
   bool HasErrors() const { return error_count_ != 0; }
 
-  std::string ErrorMessage() {
+  std::vector<cel::ParseIssue> CollectIssues() {
     // Errors are collected as they are encountered, not by their location
     // within the source. To have a more stable error message as implementation
     // details change, we sort the collected errors by their source location
@@ -226,20 +225,23 @@ class ParserMacroExprFactory final : public MacroExprFactory {
         });
     // Build the summary error message using the sorted errors.
     bool errors_truncated = error_count_ > 100;
-    std::vector<std::string> messages;
-    messages.reserve(
+    std::vector<cel::ParseIssue> issues;
+    issues.reserve(
         errors_.size() +
         errors_truncated);  // Reserve space for the transform and an
                             // additional element when truncation occurs.
-    std::transform(errors_.begin(), errors_.end(), std::back_inserter(messages),
-                   [this](const ParserError& error) {
-                     return cel::DisplayParserError(source_, error);
-                   });
+    std::transform(
+        errors_.begin(), errors_.end(), std::back_inserter(issues),
+        [this](const ParserError& error) {
+          auto location =
+              source_.GetLocation(error.range.begin).value_or(SourceLocation{});
+          return cel::ParseIssue(location, error.message);
+        });
     if (errors_truncated) {
-      messages.emplace_back(
-          absl::StrCat(error_count_ - 100, " more errors were truncated."));
+      issues.push_back(cel::ParseIssue(
+          absl::StrCat(error_count_ - 100, " more errors were truncated.")));
     }
-    return absl::StrJoin(messages, "\n");
+    return issues;
   }
 
   void AddMacroCall(int64_t macro_id, absl::string_view function,
@@ -602,6 +604,15 @@ Expr ExpressionBalancer::BalancedTree(int lo, int hi) {
   return factory_.NewCall(ops_[mid], function_, std::move(arguments));
 }
 
+std::string FormatIssues(const cel::Source& source,
+                         absl::Span<const cel::ParseIssue> issues) {
+  return absl::StrJoin(
+      issues, "\n", [&source](std::string* out, const cel::ParseIssue& issue) {
+        absl::StrAppend(out, cel::DisplayParserError(source, issue.location(),
+                                                     issue.message()));
+      });
+}
+
 class ParserVisitor final : public CelBaseVisitor,
                             public antlr4::BaseErrorListener {
  public:
@@ -673,7 +684,7 @@ class ParserVisitor final : public CelBaseVisitor,
                    const std::string& msg, std::exception_ptr e) override;
   bool HasErrored() const;
 
-  std::string ErrorMessage();
+  std::vector<cel::ParseIssue> CollectIssues();
 
  private:
   template <typename... Args>
@@ -1434,7 +1445,9 @@ void ParserVisitor::syntaxError(antlr4::Recognizer* recognizer,
 
 bool ParserVisitor::HasErrored() const { return factory_.HasErrors(); }
 
-std::string ParserVisitor::ErrorMessage() { return factory_.ErrorMessage(); }
+std::vector<cel::ParseIssue> ParserVisitor::CollectIssues() {
+  return factory_.CollectIssues();
+}
 
 Expr ParserVisitor::GlobalCallOrMacroImpl(int64_t expr_id,
                                           absl::string_view function,
@@ -1638,9 +1651,10 @@ struct ParseResult {
   EnrichedSourceInfo enriched_source_info;
 };
 
-absl::StatusOr<ParseResult> ParseImpl(const cel::Source& source,
-                                      const cel::MacroRegistry& registry,
-                                      const ParserOptions& options) {
+absl::StatusOr<ParseResult> ParseImpl(
+    const cel::Source& source, const cel::MacroRegistry& registry,
+    const ParserOptions& options,
+    std::vector<cel::ParseIssue>* parse_issues = nullptr) {
   try {
     CodePointStream input(source.content(), source.description());
     if (input.size() > options.expression_size_codepoint_limit) {
@@ -1673,13 +1687,23 @@ absl::StatusOr<ParseResult> ParseImpl(const cel::Source& source,
       expr = ExprFromAny(visitor.visit(parser.start()));
     } catch (const ParseCancellationException& e) {
       if (visitor.HasErrored()) {
-        return absl::InvalidArgumentError(visitor.ErrorMessage());
+        auto issues = visitor.CollectIssues();
+        std::string error_message = FormatIssues(source, issues);
+        if (parse_issues != nullptr) {
+          *parse_issues = std::move(issues);
+        }
+        return absl::InvalidArgumentError(error_message);
       }
       return absl::CancelledError(e.what());
     }
 
     if (visitor.HasErrored()) {
-      return absl::InvalidArgumentError(visitor.ErrorMessage());
+      auto issues = visitor.CollectIssues();
+      std::string error_message = FormatIssues(source, issues);
+      if (parse_issues != nullptr) {
+        *parse_issues = std::move(issues);
+      }
+      return absl::InvalidArgumentError(error_message);
     }
 
     return {
@@ -1706,10 +1730,12 @@ class ParserImpl : public cel::Parser {
         macro_registry_(std::move(macro_registry)),
         library_ids_(std::move(library_ids)) {}
 
-  absl::StatusOr<std::unique_ptr<cel::Ast>> Parse(
-      const cel::Source& source) const override {
+  absl::StatusOr<std::unique_ptr<cel::Ast>> ParseImpl(
+      const cel::Source& source,
+      std::vector<cel::ParseIssue>* parse_issues) const override {
     CEL_ASSIGN_OR_RETURN(auto parse_result,
-                         ParseImpl(source, macro_registry_, options_));
+                         ::google::api::expr::parser::ParseImpl(
+                             source, macro_registry_, options_, parse_issues));
     return std::make_unique<cel::Ast>(std::move(parse_result.expr),
                                       std::move(parse_result.source_info));
   }

parser/parser_interface.h

@@ -16,10 +16,14 @@
 
 #include <memory>
 #include <string>
+#include <utility>
+#include <vector>
 
+#include "absl/base/nullability.h"
 #include "absl/functional/any_invocable.h"
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
+#include "absl/strings/string_view.h"
 #include "common/ast.h"
 #include "common/source.h"
 #include "parser/macro.h"
@@ -73,6 +77,26 @@ class ParserBuilder {
   virtual absl::StatusOr<std::unique_ptr<Parser>> Build() = 0;
 };
 
+// Information about a parse failure.
+class ParseIssue {
+ public:
+  explicit ParseIssue(std::string message) : message_(std::move(message)) {}
+  ParseIssue(SourceLocation location, std::string message)
+      : location_(location), message_(std::move(message)) {}
+
+  ParseIssue(const ParseIssue& other) = default;
+  ParseIssue& operator=(const ParseIssue& other) = default;
+  ParseIssue(ParseIssue&& other) = default;
+  ParseIssue& operator=(ParseIssue&& other) = default;
+
+  SourceLocation location() const { return location_; }
+  absl::string_view message() const { return message_; }
+
+ private:
+  SourceLocation location_;
+  std::string message_;
+};
+
 // Interface for stateful CEL parser objects for use with a `Compiler`
 // (bundled parse and type check). This is not needed for most users:
 // prefer using the free functions in `parser.h` for more flexibility.
@@ -81,13 +105,35 @@ class Parser {
   virtual ~Parser() = default;
 
   // Parses the given source into a CEL AST.
-  virtual absl::StatusOr<std::unique_ptr<cel::Ast>> Parse(
-      const cel::Source& source) const = 0;
+  absl::StatusOr<std::unique_ptr<cel::Ast>> Parse(
+      const cel::Source& source) const;
+
+  // Parses the given source into a CEL AST, collecting parse errors in
+  // `issues`. If `issues` is non-null, it will be cleared and all parse
+  // issues will be appended to it.
+  absl::StatusOr<std::unique_ptr<cel::Ast>> Parse(
+      const cel::Source& source, std::vector<ParseIssue>* issues) const;
 
   // Returns a builder initialized with the configuration of this parser.
   virtual std::unique_ptr<ParserBuilder> ToBuilder() const = 0;
+
+ protected:
+  virtual absl::StatusOr<std::unique_ptr<cel::Ast>> ParseImpl(
+      const cel::Source& source,
+      std::vector<ParseIssue>* absl_nullable parse_issues) const = 0;
 };
 
+inline absl::StatusOr<std::unique_ptr<cel::Ast>> Parser::Parse(
+    const cel::Source& source) const {
+  return ParseImpl(source, nullptr);
+}
+
+inline absl::StatusOr<std::unique_ptr<cel::Ast>> Parser::Parse(
+    const cel::Source& source, std::vector<ParseIssue>* issues) const {
+  if (issues != nullptr) issues->clear();
+  return ParseImpl(source, issues);
+}
+
 }  // namespace cel
 
 #endif  // THIRD_PARTY_CEL_CPP_PARSER_PARSER_INTERFACE_H_