Optionally track structured error messages in parser.

Add a parse overload that reports issues with location to an output parameter where reasonable.
This is used make the error handling more consistent when using bundled parse + typecheck.

PiperOrigin-RevId: 910112013

Author: jnthntatum

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.h

@@ -97,6 +97,8 @@ struct CompilerLibrarySubset {
 struct CompilerOptions {
   ParserOptions parser_options;
   CheckerOptions checker_options;
+  // If true, parse errors will be adapted to issues where possible.
+  bool adapt_parser_errors = false;
 };
 
 // Interface for CEL CompilerBuilder objects.

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"
@@ -45,19 +48,38 @@ class CompilerImpl : public Compiler {
   CompilerImpl(std::unique_ptr<TypeChecker> type_checker,
                std::unique_ptr<Parser> parser,
                // Copy the validator in case builder is reused.
-               Validator validator)
+               Validator validator, CompilerOptions options)
       : type_checker_(std::move(type_checker)),
         parser_(std::move(parser)),
-        validator_(std::move(validator)) {}
+        validator_(std::move(validator)),
+        options_(options) {}
 
   absl::StatusOr<ValidationResult> Compile(
       absl::string_view expression, absl::string_view description,
       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 (!options_.adapt_parser_errors ||
+          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()) {
@@ -76,16 +98,18 @@ class CompilerImpl : public Compiler {
   std::unique_ptr<TypeChecker> type_checker_;
   std::unique_ptr<Parser> parser_;
   Validator validator_;
+  CompilerOptions options_;
 };
 
 class CompilerBuilderImpl : public CompilerBuilder {
  public:
   CompilerBuilderImpl(std::unique_ptr<TypeCheckerBuilder> type_checker_builder,
                       std::unique_ptr<ParserBuilder> parser_builder,
-                      Validator validator = Validator())
+                      Validator validator, CompilerOptions options)
       : type_checker_builder_(std::move(type_checker_builder)),
         parser_builder_(std::move(parser_builder)),
-        validator_(std::move(validator)) {}
+        validator_(std::move(validator)),
+        options_(options) {}
 
   absl::Status AddLibrary(CompilerLibrary library) override {
     if (!library.id.empty()) {
@@ -146,23 +170,23 @@ class CompilerBuilderImpl : public CompilerBuilder {
   absl::StatusOr<std::unique_ptr<Compiler>> Build() override {
     CEL_ASSIGN_OR_RETURN(auto parser, parser_builder_->Build());
     CEL_ASSIGN_OR_RETURN(auto type_checker, type_checker_builder_->Build());
-    return std::make_unique<CompilerImpl>(std::move(type_checker),
-                                          std::move(parser), validator_);
+    return std::make_unique<CompilerImpl>(
+        std::move(type_checker), std::move(parser), validator_, options_);
   }
 
  private:
   std::unique_ptr<TypeCheckerBuilder> type_checker_builder_;
   std::unique_ptr<ParserBuilder> parser_builder_;
   Validator validator_;
+  CompilerOptions options_;
 
   absl::flat_hash_set<std::string> library_ids_;
   absl::flat_hash_set<std::string> subsets_;
 };
 
 std::unique_ptr<CompilerBuilder> CompilerImpl::ToBuilder() const {
-  auto builder = std::make_unique<CompilerBuilderImpl>(
-      type_checker_->ToBuilder(), parser_->ToBuilder(), validator_);
-  return builder;
+  return std::make_unique<CompilerBuilderImpl>(
+      type_checker_->ToBuilder(), parser_->ToBuilder(), validator_, options_);
 }
 
 }  // namespace
@@ -179,7 +203,8 @@ absl::StatusOr<std::unique_ptr<CompilerBuilder>> NewCompilerBuilder(
   auto parser_builder = NewParserBuilder(options.parser_options);
 
   return std::make_unique<CompilerBuilderImpl>(std::move(type_checker_builder),
-                                               std::move(parser_builder));
+                                               std::move(parser_builder),
+                                               Validator(), options);
 }
 
 }  // namespace cel

compiler/compiler_factory_test.cc

@@ -413,5 +413,19 @@ TEST(CompilerFactoryTest, SpecifyArenaKeepsResolvedTypes) {
       it->second.GetOptional().GetParameter().GetList().GetElement().IsInt());
 }
 
+TEST(CompilerFactoryTest, ReturnsIssuesFromParser) {
+  CompilerOptions opts;
+  opts.adapt_parser_errors = true;
+  ASSERT_OK_AND_ASSIGN(
+      auto builder, NewCompilerBuilder(
+                        cel::internal::GetSharedTestingDescriptorPool(), opts));
+
+  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)
@@ -352,10 +338,11 @@ TEST_P(MathExtMacroParamsTest, ParserTests) {
 
 TEST_P(MathExtMacroParamsTest, ParserAndCheckerTests) {
   const MacroTestCase& test_case = GetParam();
-
-  ASSERT_OK_AND_ASSIGN(
-      auto compiler_builder,
-      cel::NewCompilerBuilder(internal::GetTestingDescriptorPool()));
+  CompilerOptions compile_opts;
+  compile_opts.adapt_parser_errors = true;
+  ASSERT_OK_AND_ASSIGN(auto compiler_builder,
+                       cel::NewCompilerBuilder(
+                           internal::GetTestingDescriptorPool(), compile_opts));
 
   ASSERT_THAT(compiler_builder->AddLibrary(StandardCheckerLibrary()), IsOk());
   ASSERT_THAT(compiler_builder->AddLibrary(MathCompilerLibrary()), IsOk());
@@ -381,16 +368,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 +398,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",
     ],
 )