diff --git a/checker/internal/BUILD b/checker/internal/BUILD index 450e931ce..bf7c7b79f 100644 --- a/checker/internal/BUILD +++ b/checker/internal/BUILD @@ -139,9 +139,9 @@ cc_library( "//common/ast:expr", "//internal:status_macros", "//parser:macro", - "@com_google_absl//absl/algorithm:container", "@com_google_absl//absl/base:no_destructor", "@com_google_absl//absl/base:nullability", + "@com_google_absl//absl/cleanup", "@com_google_absl//absl/container:flat_hash_map", "@com_google_absl//absl/container:flat_hash_set", "@com_google_absl//absl/status", diff --git a/checker/internal/type_check_env.h b/checker/internal/type_check_env.h index 7d740b91c..a4f75b37a 100644 --- a/checker/internal/type_check_env.h +++ b/checker/internal/type_check_env.h @@ -94,7 +94,15 @@ class TypeCheckEnv { descriptor_pool) : descriptor_pool_(std::move(descriptor_pool)), container_(""), - parent_(nullptr) {}; + parent_(nullptr) {} + + TypeCheckEnv(absl::Nonnull> + descriptor_pool, + std::shared_ptr arena) + : descriptor_pool_(std::move(descriptor_pool)), + arena_(std::move(arena)), + container_(""), + parent_(nullptr) {} // Move-only. TypeCheckEnv(TypeCheckEnv&&) = default; @@ -110,7 +118,8 @@ class TypeCheckEnv { const absl::optional& expected_type() const { return expected_type_; } - absl::Span> type_providers() const { + absl::Span> type_providers() + const { return type_providers_; } @@ -118,6 +127,10 @@ class TypeCheckEnv { type_providers_.push_back(std::move(provider)); } + void AddTypeProvider(std::shared_ptr provider) { + type_providers_.push_back(std::move(provider)); + } + const absl::flat_hash_map& variables() const { return variables_; } @@ -179,17 +192,6 @@ class TypeCheckEnv { return descriptor_pool_.get(); } - // Return an arena that can be used to allocate memory for types that will be - // used by the TypeChecker being built. - // - // This is only intended to be used for configuration. - google::protobuf::Arena* ABSL_NONNULL arena() { - if (arena_ == nullptr) { - arena_ = std::make_unique(); - } - return arena_.get(); - } - private: explicit TypeCheckEnv(const TypeCheckEnv* ABSL_NONNULL parent) : descriptor_pool_(parent->descriptor_pool_), @@ -200,7 +202,8 @@ class TypeCheckEnv { absl::string_view type, absl::string_view value) const; ABSL_NONNULL std::shared_ptr descriptor_pool_; - ABSL_NULLABLE std::unique_ptr arena_; + // If set, an arena was needed to allocate types in the environment. + ABSL_NULLABLE std::shared_ptr arena_; std::string container_; const TypeCheckEnv* ABSL_NULLABLE parent_; @@ -209,7 +212,7 @@ class TypeCheckEnv { absl::flat_hash_map functions_; // Type providers for custom types. - std::vector> type_providers_; + std::vector> type_providers_; absl::optional expected_type_; }; diff --git a/checker/internal/type_checker_builder_impl.cc b/checker/internal/type_checker_builder_impl.cc index eeb0d3b22..34e03e199 100644 --- a/checker/internal/type_checker_builder_impl.cc +++ b/checker/internal/type_checker_builder_impl.cc @@ -19,9 +19,9 @@ #include #include -#include "absl/algorithm/container.h" #include "absl/base/no_destructor.h" #include "absl/base/nullability.h" +#include "absl/cleanup/cleanup.h" #include "absl/container/flat_hash_map.h" #include "absl/status/status.h" #include "absl/status/statusor.h" @@ -80,23 +80,20 @@ absl::Status CheckStdMacroOverlap(const FunctionDecl& decl) { return absl::OkStatus(); } -} // namespace - -absl::Status TypeCheckerBuilderImpl::AddContextDeclarationVariables( - const google::protobuf::Descriptor* ABSL_NONNULL descriptor) { +absl::Status AddContextDeclarationVariables( + const google::protobuf::Descriptor* ABSL_NONNULL descriptor, TypeCheckEnv& env) { for (int i = 0; i < descriptor->field_count(); i++) { const google::protobuf::FieldDescriptor* proto_field = descriptor->field(i); MessageTypeField cel_field(proto_field); - cel_field.name(); Type field_type = cel_field.GetType(); if (field_type.IsEnum()) { field_type = IntType(); } - if (!env_.InsertVariableIfAbsent( - MakeVariableDecl(std::string(cel_field.name()), field_type))) { + if (!env.InsertVariableIfAbsent( + MakeVariableDecl(cel_field.name(), field_type))) { return absl::AlreadyExistsError( absl::StrCat("variable '", cel_field.name(), - "' already exists (from context declaration: '", + "' declared multiple times (from context declaration: '", descriptor->full_name(), "')")); } } @@ -104,14 +101,105 @@ absl::Status TypeCheckerBuilderImpl::AddContextDeclarationVariables( return absl::OkStatus(); } -absl::StatusOr> -TypeCheckerBuilderImpl::Build() && { - for (const auto* type : context_types_) { - CEL_RETURN_IF_ERROR(AddContextDeclarationVariables(type)); +absl::StatusOr MergeFunctionDecls( + const FunctionDecl& existing_decl, const FunctionDecl& new_decl) { + if (existing_decl.name() != new_decl.name()) { + return absl::InternalError( + "Attempted to merge function decls with different names"); + } + + FunctionDecl merged_decl = existing_decl; + for (const auto& ovl : new_decl.overloads()) { + // We do not tolerate signature collisions, even if they are exact matches. + CEL_RETURN_IF_ERROR(merged_decl.AddOverload(ovl)); + } + + return merged_decl; +} + +} // namespace + +absl::Status TypeCheckerBuilderImpl::BuildLibraryConfig( + const CheckerLibrary& library, + TypeCheckerBuilderImpl::ConfigRecord* config) { + target_config_ = config; + absl::Cleanup reset([this] { target_config_ = &default_config_; }); + + return library.configure(*this); +} + +absl::Status TypeCheckerBuilderImpl::ApplyConfig( + TypeCheckerBuilderImpl::ConfigRecord config, TypeCheckEnv& env) { + using FunctionDeclRecord = TypeCheckerBuilderImpl::FunctionDeclRecord; + + for (auto& type_provider : config.type_providers) { + env.AddTypeProvider(std::move(type_provider)); + } + + // TODO: check for subsetter + for (FunctionDeclRecord& fn : config.functions) { + switch (fn.add_semantic) { + case AddSemantic::kInsertIfAbsent: { + std::string name = fn.decl.name(); + if (!env.InsertFunctionIfAbsent(std::move(fn.decl))) { + return absl::AlreadyExistsError( + absl::StrCat("function '", name, "' declared multiple times")); + } + break; + } + case AddSemantic::kTryMerge: + const FunctionDecl* existing_decl = env.LookupFunction(fn.decl.name()); + FunctionDecl to_add = std::move(fn.decl); + if (existing_decl != nullptr) { + CEL_ASSIGN_OR_RETURN(to_add, + MergeFunctionDecls(*existing_decl, to_add)); + } + env.InsertOrReplaceFunction(std::move(to_add)); + break; + } + } + + for (const google::protobuf::Descriptor* context_type : config.context_types) { + CEL_RETURN_IF_ERROR(AddContextDeclarationVariables(context_type, env)); + } + + for (VariableDecl& var : config.variables) { + if (!env.InsertVariableIfAbsent(var)) { + return absl::AlreadyExistsError( + absl::StrCat("variable '", var.name(), "' declared multiple times")); + } + } + + return absl::OkStatus(); +} + +absl::StatusOr> TypeCheckerBuilderImpl::Build() { + TypeCheckEnv env(descriptor_pool_, arena_); + env.set_container(container_); + if (expected_type_.has_value()) { + env.set_expected_type(*expected_type_); } + ConfigRecord anonymous_config; + std::vector configs; + for (const auto& library : libraries_) { + ConfigRecord* config = &anonymous_config; + if (!library.id.empty()) { + configs.emplace_back(); + config = &configs.back(); + } + CEL_RETURN_IF_ERROR(BuildLibraryConfig(library, config)); + } + + for (const ConfigRecord& config : configs) { + CEL_RETURN_IF_ERROR(ApplyConfig(std::move(config), env)); + } + CEL_RETURN_IF_ERROR(ApplyConfig(std::move(anonymous_config), env)); + + CEL_RETURN_IF_ERROR(ApplyConfig(default_config_, env)); + auto checker = std::make_unique( - std::move(env_), options_); + std::move(env), options_); return checker; } @@ -123,99 +211,66 @@ absl::Status TypeCheckerBuilderImpl::AddLibrary(CheckerLibrary library) { if (!library.configure) { return absl::OkStatus(); } - absl::Status status = library.configure(*this); libraries_.push_back(std::move(library)); - return status; + return absl::OkStatus(); } absl::Status TypeCheckerBuilderImpl::AddVariable(const VariableDecl& decl) { - bool inserted = env_.InsertVariableIfAbsent(decl); - if (!inserted) { - return absl::AlreadyExistsError( - absl::StrCat("variable '", decl.name(), "' already exists")); - } + target_config_->variables.push_back(std::move(decl)); return absl::OkStatus(); } absl::Status TypeCheckerBuilderImpl::AddContextDeclaration( absl::string_view type) { - CEL_ASSIGN_OR_RETURN(absl::optional resolved_type, - env_.LookupTypeName(type)); - - if (!resolved_type.has_value()) { + const google::protobuf::Descriptor* desc = + descriptor_pool_->FindMessageTypeByName(type); + if (desc == nullptr) { return absl::NotFoundError( absl::StrCat("context declaration '", type, "' not found")); } - if (!resolved_type->IsStruct()) { + if (IsWellKnownMessageType(desc)) { return absl::InvalidArgumentError( absl::StrCat("context declaration '", type, "' is not a struct")); } - if (!resolved_type->AsStruct()->IsMessage()) { - return absl::InvalidArgumentError( - absl::StrCat("context declaration '", type, - "' is not protobuf message backed struct")); - } - - const google::protobuf::Descriptor* descriptor = - &(**(resolved_type->AsStruct()->AsMessage())); - - if (absl::c_linear_search(context_types_, descriptor)) { - return absl::AlreadyExistsError( - absl::StrCat("context declaration '", type, "' already exists")); + for (const auto* context_type : target_config_->context_types) { + if (context_type->full_name() == desc->full_name()) { + return absl::AlreadyExistsError( + absl::StrCat("context declaration '", type, "' already exists")); + } } - context_types_.push_back(descriptor); + target_config_->context_types.push_back(desc); return absl::OkStatus(); } absl::Status TypeCheckerBuilderImpl::AddFunction(const FunctionDecl& decl) { CEL_RETURN_IF_ERROR(CheckStdMacroOverlap(decl)); - bool inserted = env_.InsertFunctionIfAbsent(decl); - if (!inserted) { - return absl::AlreadyExistsError( - absl::StrCat("function '", decl.name(), "' already exists")); - } + target_config_->functions.push_back( + {std::move(decl), AddSemantic::kInsertIfAbsent}); return absl::OkStatus(); } absl::Status TypeCheckerBuilderImpl::MergeFunction(const FunctionDecl& decl) { - const FunctionDecl* existing = env_.LookupFunction(decl.name()); - if (existing == nullptr) { - return AddFunction(decl); - } - CEL_RETURN_IF_ERROR(CheckStdMacroOverlap(decl)); - - FunctionDecl merged = *existing; - - for (const auto& overload : decl.overloads()) { - if (!merged.AddOverload(overload).ok()) { - return absl::AlreadyExistsError( - absl::StrCat("function '", decl.name(), - "' already has overload that conflicts with overload ''", - overload.id(), "'")); - } - } - - env_.InsertOrReplaceFunction(std::move(merged)); - + target_config_->functions.push_back( + {std::move(decl), AddSemantic::kTryMerge}); return absl::OkStatus(); } void TypeCheckerBuilderImpl::AddTypeProvider( std::unique_ptr provider) { - env_.AddTypeProvider(std::move(provider)); + target_config_->type_providers.push_back(std::move(provider)); } void TypeCheckerBuilderImpl::set_container(absl::string_view container) { - env_.set_container(std::string(container)); + container_ = container; } void TypeCheckerBuilderImpl::SetExpectedType(const Type& type) { - env_.set_expected_type(type); + expected_type_ = type; } } // namespace cel::checker_internal diff --git a/checker/internal/type_checker_builder_impl.h b/checker/internal/type_checker_builder_impl.h index fdf907e3f..57a886eef 100644 --- a/checker/internal/type_checker_builder_impl.h +++ b/checker/internal/type_checker_builder_impl.h @@ -25,6 +25,7 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/string_view.h" +#include "absl/types/optional.h" #include "checker/checker_options.h" #include "checker/internal/type_check_env.h" #include "checker/type_checker.h" @@ -46,7 +47,9 @@ class TypeCheckerBuilderImpl : public TypeCheckerBuilder { ABSL_NONNULL std::shared_ptr descriptor_pool, const CheckerOptions& options) - : options_(options), env_(std::move(descriptor_pool)) {} + : options_(options), + target_config_(&default_config_), + descriptor_pool_(std::move(descriptor_pool)) {} // Move only. TypeCheckerBuilderImpl(const TypeCheckerBuilderImpl&) = delete; @@ -54,7 +57,7 @@ class TypeCheckerBuilderImpl : public TypeCheckerBuilder { TypeCheckerBuilderImpl& operator=(const TypeCheckerBuilderImpl&) = delete; TypeCheckerBuilderImpl& operator=(TypeCheckerBuilderImpl&&) = default; - absl::StatusOr> Build() && override; + absl::StatusOr> Build() override; absl::Status AddLibrary(CheckerLibrary library) override; @@ -72,22 +75,58 @@ class TypeCheckerBuilderImpl : public TypeCheckerBuilder { const CheckerOptions& options() const override { return options_; } - google::protobuf::Arena* ABSL_NONNULL arena() override { return env_.arena(); } + google::protobuf::Arena* ABSL_NONNULL arena() override { + if (arena_ == nullptr) { + arena_ = std::make_shared(); + } + return arena_.get(); + } const google::protobuf::DescriptorPool* ABSL_NONNULL descriptor_pool() const override { - return env_.descriptor_pool(); + return descriptor_pool_.get(); } private: - absl::Status AddContextDeclarationVariables( - const google::protobuf::Descriptor* ABSL_NONNULL descriptor); + // Sematic for adding a possibly duplicated declaration. + enum class AddSemantic { + kInsertIfAbsent, + // Attempts to merge with any existing overloads for the same function. + // Will fail if any of the IDs or signatures collide. + kTryMerge, + }; + + struct FunctionDeclRecord { + FunctionDecl decl; + AddSemantic add_semantic; + }; + + // A record of configuration calls. + // Used to replay the configuration in calls to Build(). + struct ConfigRecord { + std::vector variables; + std::vector functions; + std::vector> type_providers; + std::vector context_types; + }; + + absl::Status BuildLibraryConfig(const CheckerLibrary& library, + ConfigRecord* ABSL_NONNULL config); + + absl::Status ApplyConfig(ConfigRecord config, TypeCheckEnv& env); CheckerOptions options_; + // Default target for configuration changes. Used for direct calls to + // AddVariable, AddFunction, etc. + ConfigRecord default_config_; + // Active target for configuration changes. + // This is used to track which library the change is made on behalf of. + ConfigRecord* ABSL_NONNULL target_config_; + std::shared_ptr descriptor_pool_; + std::shared_ptr arena_; std::vector libraries_; absl::flat_hash_set library_ids_; - std::vector context_types_; - - checker_internal::TypeCheckEnv env_; + std::string container_; + absl::optional expected_type_; }; } // namespace cel::checker_internal diff --git a/checker/internal/type_checker_builder_impl_test.cc b/checker/internal/type_checker_builder_impl_test.cc index 7d63f2592..ae351a37d 100644 --- a/checker/internal/type_checker_builder_impl_test.cc +++ b/checker/internal/type_checker_builder_impl_test.cc @@ -58,7 +58,7 @@ TEST_P(ContextDeclsFieldsDefinedTest, ContextDeclsFieldsDefined) { builder.AddContextDeclaration("cel.expr.conformance.proto3.TestAllTypes"), IsOk()); ASSERT_OK_AND_ASSIGN(std::unique_ptr type_checker, - std::move(builder).Build()); + builder.Build()); ASSERT_OK_AND_ASSIGN(auto ast, MakeTestParsedAst(GetParam().expr)); ASSERT_OK_AND_ASSIGN(ValidationResult result, type_checker->Check(std::move(ast))); @@ -168,9 +168,8 @@ TEST(ContextDeclsTest, CustomStructNotSupported) { builder.AddTypeProvider(std::make_unique()); EXPECT_THAT(builder.AddContextDeclaration("com.example.MyStruct"), - StatusIs(absl::StatusCode::kInvalidArgument, - "context declaration 'com.example.MyStruct' is not " - "protobuf message backed struct")); + StatusIs(absl::StatusCode::kNotFound, + "context declaration 'com.example.MyStruct' not found")); } TEST(ContextDeclsTest, ErrorOnOverlappingContextDeclaration) { @@ -186,9 +185,9 @@ TEST(ContextDeclsTest, ErrorOnOverlappingContextDeclaration) { IsOk()); EXPECT_THAT( - std::move(builder).Build(), + builder.Build(), StatusIs(absl::StatusCode::kAlreadyExists, - "variable 'single_int32' already exists (from context " + "variable 'single_int32' declared multiple times (from context " "declaration: 'cel.expr.conformance.proto2.TestAllTypes')")); } @@ -201,11 +200,9 @@ TEST(ContextDeclsTest, ErrorOnOverlappingVariableDeclaration) { ASSERT_THAT(builder.AddVariable(MakeVariableDecl("single_int64", IntType())), IsOk()); - EXPECT_THAT( - std::move(builder).Build(), - StatusIs(absl::StatusCode::kAlreadyExists, - "variable 'single_int64' already exists (from context " - "declaration: 'cel.expr.conformance.proto3.TestAllTypes')")); + EXPECT_THAT(builder.Build(), + StatusIs(absl::StatusCode::kAlreadyExists, + "variable 'single_int64' declared multiple times")); } } // namespace diff --git a/checker/type_checker_builder.h b/checker/type_checker_builder.h index a0f83ceb0..059b30060 100644 --- a/checker/type_checker_builder.h +++ b/checker/type_checker_builder.h @@ -54,6 +54,9 @@ class TypeCheckerBuilder { virtual ~TypeCheckerBuilder() = default; // Adds a library to the TypeChecker being built. + // + // Libraries are applied in the order they are added. They effectively + // apply before any direct calls to AddVariable, AddFunction, etc. virtual absl::Status AddLibrary(CheckerLibrary library) = 0; // Adds a variable declaration that may be referenced in expressions checked @@ -80,6 +83,8 @@ class TypeCheckerBuilder { // // Validation will fail with an ERROR level issue if the deduced type of the // expression is not assignable to this type. + // + // Note: if set multiple times, the last value is used. virtual void SetExpectedType(const Type& type) = 0; // Adds function declaration overloads to the TypeChecker being built. @@ -99,16 +104,16 @@ class TypeCheckerBuilder { // Set the container for the TypeChecker being built. // // This is used for resolving references in the expressions being built. + // + // Note: if set multiple times, the last value is used. This can lead to + // surprising behavior if used in a custom library. virtual void set_container(absl::string_view container) = 0; // The current options for the TypeChecker being built. virtual const CheckerOptions& options() const = 0; - // Builds the TypeChecker. - // - // This operation is destructive: the builder instance should not be used - // after this method is called. - virtual absl::StatusOr> Build() && = 0; + // Builds a new TypeChecker instance. + virtual absl::StatusOr> Build() = 0; // Returns a pointer to an arena that can be used to allocate memory for types // that will be used by the TypeChecker being built. diff --git a/checker/type_checker_builder_factory_test.cc b/checker/type_checker_builder_factory_test.cc index 2e36e0b4d..1549ddc22 100644 --- a/checker/type_checker_builder_factory_test.cc +++ b/checker/type_checker_builder_factory_test.cc @@ -43,7 +43,7 @@ TEST(TypeCheckerBuilderTest, AddVariable) { ASSERT_THAT(builder->AddVariable(MakeVariableDecl("x", IntType())), IsOk()); - ASSERT_OK_AND_ASSIGN(auto checker, std::move(*builder).Build()); + ASSERT_OK_AND_ASSIGN(auto checker, builder->Build()); ASSERT_OK_AND_ASSIGN(auto ast, MakeTestParsedAst("x")); ASSERT_OK_AND_ASSIGN(ValidationResult result, checker->Check(std::move(ast))); EXPECT_TRUE(result.IsValid()); @@ -58,7 +58,7 @@ TEST(TypeCheckerBuilderTest, AddComplexType) { ASSERT_THAT(builder->AddVariable(MakeVariableDecl("m", map_type)), IsOk()); - ASSERT_OK_AND_ASSIGN(auto checker, std::move(*builder).Build()); + ASSERT_OK_AND_ASSIGN(auto checker, builder->Build()); builder.reset(); ASSERT_OK_AND_ASSIGN(auto ast, MakeTestParsedAst("m.foo")); ASSERT_OK_AND_ASSIGN(ValidationResult result, checker->Check(std::move(ast))); @@ -71,8 +71,13 @@ TEST(TypeCheckerBuilderTest, AddVariableRedeclaredError) { CreateTypeCheckerBuilder(GetSharedTestingDescriptorPool())); ASSERT_THAT(builder->AddVariable(MakeVariableDecl("x", IntType())), IsOk()); - EXPECT_THAT(builder->AddVariable(MakeVariableDecl("x", IntType())), - StatusIs(absl::StatusCode::kAlreadyExists)); + // We resolve the variable declarations at the Build() call, so the error + // surfaces then. + ASSERT_THAT(builder->AddVariable(MakeVariableDecl("x", IntType())), IsOk()); + + EXPECT_THAT(builder->Build(), + StatusIs(absl::StatusCode::kAlreadyExists, + "variable 'x' declared multiple times")); } TEST(TypeCheckerBuilderTest, AddFunction) { @@ -86,7 +91,7 @@ TEST(TypeCheckerBuilderTest, AddFunction) { "add", MakeOverloadDecl("add_int", IntType(), IntType(), IntType()))); ASSERT_THAT(builder->AddFunction(fn_decl), IsOk()); - ASSERT_OK_AND_ASSIGN(auto checker, std::move(*builder).Build()); + ASSERT_OK_AND_ASSIGN(auto checker, builder->Build()); ASSERT_OK_AND_ASSIGN(auto ast, MakeTestParsedAst("add(1, 2)")); ASSERT_OK_AND_ASSIGN(ValidationResult result, checker->Check(std::move(ast))); EXPECT_TRUE(result.IsValid()); @@ -103,8 +108,11 @@ TEST(TypeCheckerBuilderTest, AddFunctionRedeclaredError) { "add", MakeOverloadDecl("add_int", IntType(), IntType(), IntType()))); ASSERT_THAT(builder->AddFunction(fn_decl), IsOk()); - EXPECT_THAT(builder->AddFunction(fn_decl), - StatusIs(absl::StatusCode::kAlreadyExists)); + ASSERT_THAT(builder->AddFunction(fn_decl), IsOk()); + + EXPECT_THAT(builder->Build(), + StatusIs(absl::StatusCode::kAlreadyExists, + "function 'add' declared multiple times")); } TEST(TypeCheckerBuilderTest, AddLibrary) { @@ -123,7 +131,7 @@ TEST(TypeCheckerBuilderTest, AddLibrary) { }}), IsOk()); - ASSERT_OK_AND_ASSIGN(auto checker, std::move(*builder).Build()); + ASSERT_OK_AND_ASSIGN(auto checker, builder->Build()); ASSERT_OK_AND_ASSIGN(auto ast, MakeTestParsedAst("add(1, 2)")); ASSERT_OK_AND_ASSIGN(ValidationResult result, checker->Check(std::move(ast))); EXPECT_TRUE(result.IsValid()); @@ -144,7 +152,7 @@ TEST(TypeCheckerBuilderTest, AddContextDeclaration) { IsOk()); ASSERT_THAT(builder->AddFunction(fn_decl), IsOk()); - ASSERT_OK_AND_ASSIGN(auto checker, std::move(*builder).Build()); + ASSERT_OK_AND_ASSIGN(auto checker, builder->Build()); ASSERT_OK_AND_ASSIGN(auto ast, MakeTestParsedAst("increment(single_int64)")); ASSERT_OK_AND_ASSIGN(ValidationResult result, checker->Check(std::move(ast))); EXPECT_TRUE(result.IsValid()); @@ -172,7 +180,7 @@ TEST(TypeCheckerBuilderTest, AddLibraryRedeclaredError) { StatusIs(absl::StatusCode::kAlreadyExists, HasSubstr("testlib"))); } -TEST(TypeCheckerBuilderTest, AddLibraryForwardsErrors) { +TEST(TypeCheckerBuilderTest, BuildForwardsLibraryErrors) { ASSERT_OK_AND_ASSIGN( std::unique_ptr builder, CreateTypeCheckerBuilder(GetSharedTestingDescriptorPool())); @@ -187,11 +195,14 @@ TEST(TypeCheckerBuilderTest, AddLibraryForwardsErrors) { return builder->AddFunction(fn_decl); }}), IsOk()); - EXPECT_THAT(builder->AddLibrary({"", + ASSERT_THAT(builder->AddLibrary({"", [](TypeCheckerBuilder& b) { return absl::InternalError("test error"); }}), - StatusIs(absl::StatusCode::kInternal, HasSubstr("test error"))); + IsOk()); + + EXPECT_THAT(builder->Build(), + StatusIs(absl::StatusCode::kInternal, "test error")); } TEST(TypeCheckerBuilderTest, AddFunctionOverlapsWithStdMacroError) {