diff --git a/checker/type_checker_builder_factory_test.cc b/checker/type_checker_builder_factory_test.cc index 1549ddc22..6f2e58b7e 100644 --- a/checker/type_checker_builder_factory_test.cc +++ b/checker/type_checker_builder_factory_test.cc @@ -65,6 +65,56 @@ TEST(TypeCheckerBuilderTest, AddComplexType) { EXPECT_TRUE(result.IsValid()); } +TEST(TypeCheckerBuilderTest, TypeCheckersIndependent) { + ASSERT_OK_AND_ASSIGN( + std::unique_ptr builder, + CreateTypeCheckerBuilder(GetSharedTestingDescriptorPool())); + + MapType map_type(builder->arena(), StringType(), IntType()); + + ASSERT_THAT(builder->AddVariable(MakeVariableDecl("m", map_type)), IsOk()); + ASSERT_OK_AND_ASSIGN( + FunctionDecl fn, + MakeFunctionDecl( + "foo", MakeOverloadDecl("foo", IntType(), IntType(), IntType()))); + ASSERT_THAT(builder->AddFunction(std::move(fn)), IsOk()); + + ASSERT_OK_AND_ASSIGN(auto checker1, builder->Build()); + + ASSERT_THAT(builder->AddVariable(MakeVariableDecl("ns.m2", map_type)), + IsOk()); + builder->set_container("ns"); + ASSERT_OK_AND_ASSIGN(auto checker2, builder->Build()); + // Test for lifetime issues between separate type checker instances from the + // same builder. + builder.reset(); + + { + ASSERT_OK_AND_ASSIGN(auto ast1, MakeTestParsedAst("foo(m.bar, m.bar)")); + ASSERT_OK_AND_ASSIGN(auto ast2, MakeTestParsedAst("foo(m.bar, m2.bar)")); + + ASSERT_OK_AND_ASSIGN(ValidationResult result, + checker1->Check(std::move(ast1))); + EXPECT_TRUE(result.IsValid()); + ASSERT_OK_AND_ASSIGN(ValidationResult result2, + checker1->Check(std::move(ast2))); + EXPECT_FALSE(result2.IsValid()); + } + checker1.reset(); + + { + ASSERT_OK_AND_ASSIGN(auto ast1, MakeTestParsedAst("foo(m.bar, m.bar)")); + ASSERT_OK_AND_ASSIGN(auto ast2, MakeTestParsedAst("foo(m.bar, m2.bar)")); + + ASSERT_OK_AND_ASSIGN(ValidationResult result, + checker2->Check(std::move(ast1))); + EXPECT_TRUE(result.IsValid()); + ASSERT_OK_AND_ASSIGN(ValidationResult result2, + checker2->Check(std::move(ast2))); + EXPECT_TRUE(result2.IsValid()); + } +} + TEST(TypeCheckerBuilderTest, AddVariableRedeclaredError) { ASSERT_OK_AND_ASSIGN( std::unique_ptr builder, diff --git a/compiler/compiler.h b/compiler/compiler.h index 53587a8f7..7a7a3c6a5 100644 --- a/compiler/compiler.h +++ b/compiler/compiler.h @@ -92,7 +92,7 @@ class CompilerBuilder { virtual TypeCheckerBuilder& GetCheckerBuilder() = 0; virtual ParserBuilder& GetParserBuilder() = 0; - virtual absl::StatusOr> Build() && = 0; + virtual absl::StatusOr> Build() = 0; }; // Interface for CEL Compiler objects. diff --git a/compiler/compiler_factory.cc b/compiler/compiler_factory.cc index dac35c6ed..6cc5a41d1 100644 --- a/compiler/compiler_factory.cc +++ b/compiler/compiler_factory.cc @@ -100,13 +100,12 @@ class CompilerBuilderImpl : public CompilerBuilder { return *type_checker_builder_; } - absl::StatusOr> Build() && override { + absl::StatusOr> Build() override { for (const auto& library : parser_libraries_) { CEL_RETURN_IF_ERROR(library(*parser_builder_)); } - CEL_ASSIGN_OR_RETURN(auto parser, std::move(*parser_builder_).Build()); - CEL_ASSIGN_OR_RETURN(auto type_checker, - std::move(*type_checker_builder_).Build()); + CEL_ASSIGN_OR_RETURN(auto parser, parser_builder_->Build()); + CEL_ASSIGN_OR_RETURN(auto type_checker, type_checker_builder_->Build()); return std::make_unique(std::move(type_checker), std::move(parser)); } diff --git a/compiler/compiler_factory_test.cc b/compiler/compiler_factory_test.cc index 1992e5b60..f3b69b0bc 100644 --- a/compiler/compiler_factory_test.cc +++ b/compiler/compiler_factory_test.cc @@ -53,7 +53,7 @@ TEST(CompilerFactoryTest, Works) { NewCompilerBuilder(cel::internal::GetSharedTestingDescriptorPool())); ASSERT_THAT(builder->AddLibrary(StandardCheckerLibrary()), IsOk()); - ASSERT_OK_AND_ASSIGN(auto compiler, std::move(*builder).Build()); + ASSERT_OK_AND_ASSIGN(auto compiler, builder->Build()); ASSERT_OK_AND_ASSIGN( ValidationResult result, @@ -134,7 +134,7 @@ TEST(CompilerFactoryTest, ParserLibrary) { MakeVariableDecl("a", MapType())), IsOk()); - ASSERT_OK_AND_ASSIGN(auto compiler, std::move(*builder).Build()); + ASSERT_OK_AND_ASSIGN(auto compiler, builder->Build()); ASSERT_THAT(compiler->Compile("has(a.b)"), IsOk()); @@ -160,7 +160,7 @@ TEST(CompilerFactoryTest, ParserOptions) { MakeVariableDecl("a", MapType())), IsOk()); - ASSERT_OK_AND_ASSIGN(auto compiler, std::move(*builder).Build()); + ASSERT_OK_AND_ASSIGN(auto compiler, builder->Build()); ASSERT_THAT(compiler->Compile("a.?b.orValue('foo')"), IsOk()); } @@ -170,7 +170,7 @@ TEST(CompilerFactoryTest, GetParser) { auto builder, NewCompilerBuilder(cel::internal::GetSharedTestingDescriptorPool())); - ASSERT_OK_AND_ASSIGN(auto compiler, std::move(*builder).Build()); + ASSERT_OK_AND_ASSIGN(auto compiler, builder->Build()); const cel::Parser& parser = compiler->GetParser(); @@ -197,7 +197,7 @@ TEST(CompilerFactoryTest, GetTypeChecker) { s.Update(builder->GetCheckerBuilder().AddFunction(std::move(or_decl))); ASSERT_THAT(s, IsOk()); - ASSERT_OK_AND_ASSIGN(auto compiler, std::move(*builder).Build()); + ASSERT_OK_AND_ASSIGN(auto compiler, builder->Build()); const cel::Parser& parser = compiler->GetParser(); @@ -227,7 +227,7 @@ TEST(CompilerFactoryTest, DisableStandardMacros) { MakeVariableDecl("a", MapType())), IsOk()); - ASSERT_OK_AND_ASSIGN(auto compiler, std::move(*builder).Build()); + ASSERT_OK_AND_ASSIGN(auto compiler, builder->Build()); ASSERT_OK_AND_ASSIGN(ValidationResult result, compiler->Compile("a.b")); diff --git a/parser/parser.cc b/parser/parser.cc index cfb8df8db..cd2fe9b11 100644 --- a/parser/parser.cc +++ b/parser/parser.cc @@ -1738,7 +1738,7 @@ class ParserBuilderImpl : public cel::ParserBuilder { return absl::OkStatus(); } - absl::StatusOr> Build() && override { + absl::StatusOr> Build() override { cel::MacroRegistry macro_registry; if (!options_.disable_standard_macros) { diff --git a/parser/parser_interface.h b/parser/parser_interface.h index edbcb1fa3..61a6d9fcc 100644 --- a/parser/parser_interface.h +++ b/parser/parser_interface.h @@ -40,7 +40,7 @@ class ParserBuilder { virtual absl::Status AddMacro(const cel::Macro& macro) = 0; // Builds a new parser instance, may error if incompatible macros are added. - virtual absl::StatusOr> Build() && = 0; + virtual absl::StatusOr> Build() = 0; }; // Interface for stateful CEL parser objects for use with a `Compiler`