Refactor factory functions for TypeCheckerBuilders.

- move the factories to their own cc_library
- update return value to be std::unique_ptr<Builder>

PiperOrigin-RevId: 698067791

Author: jnthntatum

checker/BUILD

@@ -91,28 +91,44 @@ cc_library(
         "//checker/internal:type_checker_impl",
         "//common:decl",
         "//common:type",
-        "//internal:noop_delete",
         "//internal:status_macros",
-        "//internal:well_known_types",
         "//parser:macro",
         "@com_google_absl//absl/base:no_destructor",
         "@com_google_absl//absl/base:nullability",
         "@com_google_absl//absl/container:flat_hash_map",
         "@com_google_absl//absl/container:flat_hash_set",
         "@com_google_absl//absl/functional:any_invocable",
-        "@com_google_absl//absl/log:absl_check",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/strings",
         "@com_google_protobuf//:protobuf",
     ],
 )
 
+cc_library(
+    name = "type_checker_builder_factory",
+    srcs = ["type_checker_builder_factory.cc"],
+    hdrs = ["type_checker_builder_factory.h"],
+    deps = [
+        ":checker_options",
+        ":type_checker_builder",
+        "//internal:noop_delete",
+        "//internal:status_macros",
+        "//internal:well_known_types",
+        "@com_google_absl//absl/base:nullability",
+        "@com_google_absl//absl/log:absl_check",
+        "@com_google_absl//absl/memory",
+        "@com_google_absl//absl/status:statusor",
+        "@com_google_protobuf//:protobuf",
+    ],
+)
+
 cc_test(
-    name = "type_checker_builder_test",
-    srcs = ["type_checker_builder_test.cc"],
+    name = "type_checker_builder_factory_test",
+    srcs = ["type_checker_builder_factory_test.cc"],
     deps = [
         ":type_checker_builder",
+        ":type_checker_builder_factory",
         ":validation_result",
         "//checker/internal:test_ast_helpers",
         "//common:decl",
@@ -145,9 +161,11 @@ cc_test(
     name = "standard_library_test",
     srcs = ["standard_library_test.cc"],
     deps = [
+        ":checker_options",
         ":standard_library",
         ":type_checker",
         ":type_checker_builder",
+        ":type_checker_builder_factory",
         ":validation_result",
         "//base/ast_internal:ast_impl",
         "//base/ast_internal:expr",
@@ -190,6 +208,7 @@ cc_test(
         ":type_check_issue",
         ":type_checker",
         ":type_checker_builder",
+        ":type_checker_builder_factory",
         "//base/ast_internal:ast_impl",
         "//base/ast_internal:expr",
         "//checker/internal:test_ast_helpers",

checker/optional_test.cc

@@ -29,6 +29,7 @@
 #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 "internal/testing.h"
 #include "internal/testing_descriptor_pool.h"
 
@@ -77,13 +78,13 @@ MATCHER_P(IsOptionalType, inner_type, "") {
 
 TEST(OptionalTest, OptSelectDoesNotAnnotateFieldType) {
   ASSERT_OK_AND_ASSIGN(
-      TypeCheckerBuilder builder,
+      std::unique_ptr<TypeCheckerBuilder> builder,
       CreateTypeCheckerBuilder(GetSharedTestingDescriptorPool()));
-  ASSERT_THAT(builder.AddLibrary(StandardCheckerLibrary()), IsOk());
-  ASSERT_THAT(builder.AddLibrary(OptionalCheckerLibrary()), IsOk());
-  builder.set_container("cel.expr.conformance.proto3");
+  ASSERT_THAT(builder->AddLibrary(StandardCheckerLibrary()), IsOk());
+  ASSERT_THAT(builder->AddLibrary(OptionalCheckerLibrary()), IsOk());
+  builder->set_container("cel.expr.conformance.proto3");
   ASSERT_OK_AND_ASSIGN(std::unique_ptr<TypeChecker> checker,
-                       std::move(builder).Build());
+                       std::move(*builder).Build());
 
   ASSERT_OK_AND_ASSIGN(auto ast,
                        MakeTestParsedAst("TestAllTypes{}.?single_int64"));
@@ -113,13 +114,13 @@ class OptionalTest : public testing::TestWithParam<TestCase> {};
 
 TEST_P(OptionalTest, Runner) {
   ASSERT_OK_AND_ASSIGN(
-      TypeCheckerBuilder builder,
+      std::unique_ptr<TypeCheckerBuilder> builder,
       CreateTypeCheckerBuilder(GetSharedTestingDescriptorPool()));
   const TestCase& test_case = GetParam();
-  ASSERT_THAT(builder.AddLibrary(StandardCheckerLibrary()), IsOk());
-  ASSERT_THAT(builder.AddLibrary(OptionalCheckerLibrary()), IsOk());
+  ASSERT_THAT(builder->AddLibrary(StandardCheckerLibrary()), IsOk());
+  ASSERT_THAT(builder->AddLibrary(OptionalCheckerLibrary()), IsOk());
   ASSERT_OK_AND_ASSIGN(std::unique_ptr<TypeChecker> checker,
-                       std::move(builder).Build());
+                       std::move(*builder).Build());
 
   ASSERT_OK_AND_ASSIGN(auto ast, MakeTestParsedAst(test_case.expr));
 
@@ -284,13 +285,13 @@ TEST_P(OptionalStrictNullAssignmentTest, Runner) {
   CheckerOptions options;
   options.enable_legacy_null_assignment = false;
   ASSERT_OK_AND_ASSIGN(
-      TypeCheckerBuilder builder,
+      std::unique_ptr<TypeCheckerBuilder> builder,
       CreateTypeCheckerBuilder(GetSharedTestingDescriptorPool(), options));
   const TestCase& test_case = GetParam();
-  ASSERT_THAT(builder.AddLibrary(StandardCheckerLibrary()), IsOk());
-  ASSERT_THAT(builder.AddLibrary(OptionalCheckerLibrary()), IsOk());
+  ASSERT_THAT(builder->AddLibrary(StandardCheckerLibrary()), IsOk());
+  ASSERT_THAT(builder->AddLibrary(OptionalCheckerLibrary()), IsOk());
   ASSERT_OK_AND_ASSIGN(std::unique_ptr<TypeChecker> checker,
-                       std::move(builder).Build());
+                       std::move(*builder).Build());
 
   ASSERT_OK_AND_ASSIGN(auto ast, MakeTestParsedAst(test_case.expr));
 

checker/standard_library_test.cc

@@ -22,9 +22,11 @@
 #include "absl/status/status_matchers.h"
 #include "base/ast_internal/ast_impl.h"
 #include "base/ast_internal/expr.h"
+#include "checker/checker_options.h"
 #include "checker/internal/test_ast_helpers.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/constant.h"
@@ -50,27 +52,27 @@ using AstType = cel::ast_internal::Type;
 
 TEST(StandardLibraryTest, StandardLibraryAddsDecls) {
   ASSERT_OK_AND_ASSIGN(
-      TypeCheckerBuilder builder,
+      std::unique_ptr<TypeCheckerBuilder> builder,
       CreateTypeCheckerBuilder(GetSharedTestingDescriptorPool()));
-  EXPECT_THAT(builder.AddLibrary(StandardCheckerLibrary()), IsOk());
-  EXPECT_THAT(std::move(builder).Build(), IsOk());
+  EXPECT_THAT(builder->AddLibrary(StandardCheckerLibrary()), IsOk());
+  EXPECT_THAT(std::move(*builder).Build(), IsOk());
 }
 
 TEST(StandardLibraryTest, StandardLibraryErrorsIfAddedTwice) {
   ASSERT_OK_AND_ASSIGN(
-      TypeCheckerBuilder builder,
+      std::unique_ptr<TypeCheckerBuilder> builder,
       CreateTypeCheckerBuilder(GetSharedTestingDescriptorPool()));
-  EXPECT_THAT(builder.AddLibrary(StandardCheckerLibrary()), IsOk());
-  EXPECT_THAT(builder.AddLibrary(StandardCheckerLibrary()),
+  EXPECT_THAT(builder->AddLibrary(StandardCheckerLibrary()), IsOk());
+  EXPECT_THAT(builder->AddLibrary(StandardCheckerLibrary()),
               StatusIs(absl::StatusCode::kAlreadyExists));
 }
 
 TEST(StandardLibraryTest, ComprehensionVarsIndirectCyclicParamAssignability) {
   google::protobuf::Arena arena;
   ASSERT_OK_AND_ASSIGN(
-      TypeCheckerBuilder builder,
+      std::unique_ptr<TypeCheckerBuilder> builder,
       CreateTypeCheckerBuilder(GetSharedTestingDescriptorPool()));
-  ASSERT_THAT(builder.AddLibrary(StandardCheckerLibrary()), IsOk());
+  ASSERT_THAT(builder->AddLibrary(StandardCheckerLibrary()), IsOk());
 
   // Note: this is atypical -- parameterized variables aren't well supported
   // outside of built-in syntax.
@@ -83,13 +85,13 @@ TEST(StandardLibraryTest, ComprehensionVarsIndirectCyclicParamAssignability) {
   Type list_type = ListType(&arena, TypeParamType("V"));
   Type map_type = MapType(&arena, TypeParamType("K"), TypeParamType("V"));
 
-  ASSERT_THAT(builder.AddVariable(MakeVariableDecl("list_var", list_type)),
+  ASSERT_THAT(builder->AddVariable(MakeVariableDecl("list_var", list_type)),
               IsOk());
-  ASSERT_THAT(builder.AddVariable(MakeVariableDecl("map_var", map_type)),
+  ASSERT_THAT(builder->AddVariable(MakeVariableDecl("map_var", map_type)),
               IsOk());
 
   ASSERT_OK_AND_ASSIGN(std::unique_ptr<TypeChecker> type_checker,
-                       std::move(builder).Build());
+                       std::move(*builder).Build());
 
   ASSERT_OK_AND_ASSIGN(
       auto ast, checker_internal::MakeTestParsedAst(
@@ -108,10 +110,10 @@ class StandardLibraryDefinitionsTest : public ::testing::Test {
  public:
   void SetUp() override {
     ASSERT_OK_AND_ASSIGN(
-        TypeCheckerBuilder builder,
+        std::unique_ptr<TypeCheckerBuilder> builder,
         CreateTypeCheckerBuilder(GetSharedTestingDescriptorPool()));
-    ASSERT_THAT(builder.AddLibrary(StandardCheckerLibrary()), IsOk());
-    ASSERT_OK_AND_ASSIGN(stdlib_type_checker_, std::move(builder).Build());
+    ASSERT_THAT(builder->AddLibrary(StandardCheckerLibrary()), IsOk());
+    ASSERT_OK_AND_ASSIGN(stdlib_type_checker_, std::move(*builder).Build());
   }
 
  protected:
@@ -212,12 +214,12 @@ class StdLibDefinitionsTest
 // Type-parameterized functions are not yet checkable.
 TEST_P(StdLibDefinitionsTest, Runner) {
   ASSERT_OK_AND_ASSIGN(
-      TypeCheckerBuilder builder,
+      std::unique_ptr<TypeCheckerBuilder> builder,
       CreateTypeCheckerBuilder(GetSharedTestingDescriptorPool(),
                                GetParam().options));
-  ASSERT_THAT(builder.AddLibrary(StandardCheckerLibrary()), IsOk());
+  ASSERT_THAT(builder->AddLibrary(StandardCheckerLibrary()), IsOk());
   ASSERT_OK_AND_ASSIGN(std::unique_ptr<TypeChecker> type_checker,
-                       std::move(builder).Build());
+                       std::move(*builder).Build());
 
   ASSERT_OK_AND_ASSIGN(std::unique_ptr<Ast> ast,
                        checker_internal::MakeTestParsedAst(GetParam().expr));

checker/type_checker_builder.cc

@@ -20,25 +20,19 @@
 #include <vector>
 
 #include "absl/base/no_destructor.h"
-#include "absl/base/nullability.h"
 #include "absl/container/flat_hash_map.h"
-#include "absl/log/absl_check.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/checker_options.h"
 #include "checker/internal/type_check_env.h"
 #include "checker/internal/type_checker_impl.h"
 #include "checker/type_checker.h"
 #include "common/decl.h"
 #include "common/type.h"
 #include "common/type_introspector.h"
-#include "internal/noop_delete.h"
 #include "internal/status_macros.h"
-#include "internal/well_known_types.h"
 #include "parser/macro.h"
-#include "google/protobuf/descriptor.h"
 
 namespace cel {
 namespace {
@@ -83,29 +77,6 @@ absl::Status CheckStdMacroOverlap(const FunctionDecl& decl) {
 
 }  // namespace
 
-absl::StatusOr<TypeCheckerBuilder> CreateTypeCheckerBuilder(
-    absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
-    const CheckerOptions& options) {
-  ABSL_DCHECK(descriptor_pool != nullptr);
-  return CreateTypeCheckerBuilder(
-      std::shared_ptr<const google::protobuf::DescriptorPool>(
-          descriptor_pool,
-          internal::NoopDeleteFor<const google::protobuf::DescriptorPool>()),
-      options);
-}
-
-absl::StatusOr<TypeCheckerBuilder> CreateTypeCheckerBuilder(
-    absl::Nonnull<std::shared_ptr<const google::protobuf::DescriptorPool>>
-        descriptor_pool,
-    const CheckerOptions& options) {
-  ABSL_DCHECK(descriptor_pool != nullptr);
-  // Verify the standard descriptors, we do not need to keep
-  // `well_known_types::Reflection` at the moment here.
-  CEL_RETURN_IF_ERROR(
-      well_known_types::Reflection().Initialize(descriptor_pool.get()));
-  return TypeCheckerBuilder(std::move(descriptor_pool), options);
-}
-
 absl::StatusOr<std::unique_ptr<TypeChecker>> TypeCheckerBuilder::Build() && {
   auto checker = std::make_unique<checker_internal::TypeCheckerImpl>(
       std::move(env_), options_);

checker/type_checker_builder.h

@@ -38,34 +38,6 @@ namespace cel {
 
 class TypeCheckerBuilder;
 
-// Creates a new `TypeCheckerBuilder`.
-//
-// When passing a raw pointer to a descriptor pool, the descriptor pool must
-// outlive the type checker builder and the type checker builder it creates.
-//
-// The descriptor pool must include the minimally necessary
-// descriptors required by CEL. Those are the following:
-// - google.protobuf.NullValue
-// - google.protobuf.BoolValue
-// - google.protobuf.Int32Value
-// - google.protobuf.Int64Value
-// - google.protobuf.UInt32Value
-// - google.protobuf.UInt64Value
-// - google.protobuf.FloatValue
-// - google.protobuf.DoubleValue
-// - google.protobuf.BytesValue
-// - google.protobuf.StringValue
-// - google.protobuf.Any
-// - google.protobuf.Duration
-// - google.protobuf.Timestamp
-absl::StatusOr<TypeCheckerBuilder> CreateTypeCheckerBuilder(
-    absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
-    const CheckerOptions& options = {});
-absl::StatusOr<TypeCheckerBuilder> CreateTypeCheckerBuilder(
-    absl::Nonnull<std::shared_ptr<const google::protobuf::DescriptorPool>>
-        descriptor_pool,
-    const CheckerOptions& options = {});
-
 // Functional implementation to apply the library features to a
 // TypeCheckerBuilder.
 using TypeCheckerBuilderConfigurer =
@@ -109,7 +81,8 @@ class TypeCheckerBuilder {
   const CheckerOptions& options() const { return options_; }
 
  private:
-  friend absl::StatusOr<TypeCheckerBuilder> CreateTypeCheckerBuilder(
+  friend absl::StatusOr<std::unique_ptr<TypeCheckerBuilder>>
+  CreateTypeCheckerBuilder(
       absl::Nonnull<std::shared_ptr<const google::protobuf::DescriptorPool>>
           descriptor_pool,
       const CheckerOptions& options);

checker/type_checker_builder_factory.cc

@@ -0,0 +1,58 @@
+
+// Copyright 2024 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "checker/type_checker_builder_factory.h"
+
+#include <memory>
+#include <utility>
+
+#include "absl/base/nullability.h"
+#include "absl/log/absl_check.h"
+#include "absl/memory/memory.h"
+#include "absl/status/statusor.h"
+#include "checker/checker_options.h"
+#include "checker/type_checker_builder.h"
+#include "internal/noop_delete.h"
+#include "internal/status_macros.h"
+#include "internal/well_known_types.h"
+#include "google/protobuf/descriptor.h"
+
+namespace cel {
+
+absl::StatusOr<std::unique_ptr<TypeCheckerBuilder>> CreateTypeCheckerBuilder(
+    absl::Nonnull<const google::protobuf::DescriptorPool*> descriptor_pool,
+    const CheckerOptions& options) {
+  ABSL_DCHECK(descriptor_pool != nullptr);
+  return CreateTypeCheckerBuilder(
+      std::shared_ptr<const google::protobuf::DescriptorPool>(
+          descriptor_pool,
+          internal::NoopDeleteFor<const google::protobuf::DescriptorPool>()),
+      options);
+}
+
+absl::StatusOr<std::unique_ptr<TypeCheckerBuilder>> CreateTypeCheckerBuilder(
+    absl::Nonnull<std::shared_ptr<const google::protobuf::DescriptorPool>>
+        descriptor_pool,
+    const CheckerOptions& options) {
+  ABSL_DCHECK(descriptor_pool != nullptr);
+  // Verify the standard descriptors, we do not need to keep
+  // `well_known_types::Reflection` at the moment here.
+  CEL_RETURN_IF_ERROR(
+      well_known_types::Reflection().Initialize(descriptor_pool.get()));
+  auto* builder = new TypeCheckerBuilder(std::move(descriptor_pool), options);
+  return absl::WrapUnique(builder);
+}
+
+}  // namespace cel