diff --git a/common/BUILD b/common/BUILD index 105ada4cf..03b687b45 100644 --- a/common/BUILD +++ b/common/BUILD @@ -42,6 +42,7 @@ cc_library( "@com_google_absl//absl/algorithm:container", "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/base:no_destructor", + "@com_google_absl//absl/functional:overload", "@com_google_absl//absl/strings:string_view", "@com_google_absl//absl/types:optional", "@com_google_absl//absl/types:span", @@ -1026,6 +1027,7 @@ cc_library( srcs = ["ast_proto.cc"], hdrs = ["ast_proto.h"], deps = [ + ":ast", ":constant", ":expr", "//base:ast", @@ -1056,18 +1058,26 @@ cc_test( deps = [ ":ast", ":ast_proto", + ":decl", ":expr", + ":source", + ":type", "//common/ast:ast_impl", "//common/ast:expr", + "//compiler", + "//compiler:compiler_factory", + "//compiler:optional", + "//compiler:standard_library", + "//extensions:comprehensions_v2", "//internal:proto_matchers", "//internal:status_macros", "//internal:testing", - "//parser", - "//parser:options", + "//internal:testing_descriptor_pool", "@com_google_absl//absl/container:flat_hash_map", "@com_google_absl//absl/status", "@com_google_absl//absl/status:status_matchers", "@com_google_absl//absl/status:statusor", + "@com_google_absl//absl/strings", "@com_google_absl//absl/strings:string_view", "@com_google_absl//absl/types:variant", "@com_google_cel_spec//proto/cel/expr:checked_cc_proto", diff --git a/common/ast.h b/common/ast.h index 81a477ec4..83d37f149 100644 --- a/common/ast.h +++ b/common/ast.h @@ -65,9 +65,8 @@ class Ast { expr_version_(std::move(expr_version)), is_checked_(true) {} - // Move-only - Ast(const Ast& other) = delete; - Ast& operator=(const Ast& other) = delete; + Ast(const Ast& other) = default; + Ast& operator=(const Ast& other) = default; Ast(Ast&& other) = default; Ast& operator=(Ast&& other) = default; diff --git a/common/ast/metadata.h b/common/ast/metadata.h index 00d70432c..a2932c3e0 100644 --- a/common/ast/metadata.h +++ b/common/ast/metadata.h @@ -168,6 +168,11 @@ class SourceInfo { macro_calls_(std::move(macro_calls)), extensions_(std::move(extensions)) {} + SourceInfo(const SourceInfo& other) = default; + SourceInfo(SourceInfo&& other) = default; + SourceInfo& operator=(const SourceInfo& other) = default; + SourceInfo& operator=(SourceInfo&& other) = default; + void set_syntax_version(std::string syntax_version) { syntax_version_ = std::move(syntax_version); } @@ -787,6 +792,11 @@ class Reference { overload_id_(std::move(overload_id)), value_(std::move(value)) {} + Reference(const Reference& other) = default; + Reference& operator=(const Reference& other) = default; + Reference(Reference&&) = default; + Reference& operator=(Reference&&) = default; + void set_name(std::string name) { name_ = std::move(name); } void set_overload_id(std::vector overload_id) { diff --git a/common/ast_proto.cc b/common/ast_proto.cc index 047a041ae..24ba55a73 100644 --- a/common/ast_proto.cc +++ b/common/ast_proto.cc @@ -30,7 +30,7 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/types/variant.h" -#include "base/ast.h" +#include "common/ast.h" #include "common/ast/ast_impl.h" #include "common/ast/constant_proto.h" #include "common/ast/expr.h" @@ -499,12 +499,10 @@ absl::StatusOr> CreateAstFromParsedExpr( absl::Status AstToParsedExpr(const Ast& ast, cel::expr::ParsedExpr* absl_nonnull out) { - const auto& ast_impl = ast_internal::AstImpl::CastFromPublicAst(ast); ParsedExprPb& parsed_expr = *out; - CEL_RETURN_IF_ERROR( - ExprToProto(ast_impl.root_expr(), parsed_expr.mutable_expr())); + CEL_RETURN_IF_ERROR(ExprToProto(ast.root_expr(), parsed_expr.mutable_expr())); CEL_RETURN_IF_ERROR(ast_internal::SourceInfoToProto( - ast_impl.source_info(), parsed_expr.mutable_source_info())); + ast.source_info(), parsed_expr.mutable_source_info())); return absl::OkStatus(); } @@ -539,25 +537,23 @@ absl::StatusOr> CreateAstFromCheckedExpr( absl::Status AstToCheckedExpr( const Ast& ast, cel::expr::CheckedExpr* absl_nonnull out) { - if (!ast.IsChecked()) { + if (!ast.is_checked()) { return absl::InvalidArgumentError("AST is not type-checked"); } - const auto& ast_impl = ast_internal::AstImpl::CastFromPublicAst(ast); CheckedExprPb& checked_expr = *out; - checked_expr.set_expr_version(ast_impl.expr_version()); + checked_expr.set_expr_version(ast.expr_version()); CEL_RETURN_IF_ERROR( - ExprToProto(ast_impl.root_expr(), checked_expr.mutable_expr())); + ExprToProto(ast.root_expr(), checked_expr.mutable_expr())); CEL_RETURN_IF_ERROR(ast_internal::SourceInfoToProto( - ast_impl.source_info(), checked_expr.mutable_source_info())); - for (auto it = ast_impl.reference_map().begin(); - it != ast_impl.reference_map().end(); ++it) { + ast.source_info(), checked_expr.mutable_source_info())); + for (auto it = ast.reference_map().begin(); it != ast.reference_map().end(); + ++it) { ReferencePb& dest_reference = (*checked_expr.mutable_reference_map())[it->first]; CEL_ASSIGN_OR_RETURN(dest_reference, ReferenceToProto(it->second)); } - for (auto it = ast_impl.type_map().begin(); it != ast_impl.type_map().end(); - ++it) { + for (auto it = ast.type_map().begin(); it != ast.type_map().end(); ++it) { TypePb& dest_type = (*checked_expr.mutable_type_map())[it->first]; CEL_RETURN_IF_ERROR(TypeToProto(it->second, &dest_type)); } diff --git a/common/ast_proto_test.cc b/common/ast_proto_test.cc index cc39c1156..e9b36a4bd 100644 --- a/common/ast_proto_test.cc +++ b/common/ast_proto_test.cc @@ -28,17 +28,25 @@ #include "absl/status/status.h" #include "absl/status/status_matchers.h" #include "absl/status/statusor.h" +#include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "absl/types/variant.h" #include "common/ast.h" #include "common/ast/ast_impl.h" #include "common/ast/expr.h" +#include "common/decl.h" #include "common/expr.h" +#include "common/source.h" +#include "common/type.h" +#include "compiler/compiler.h" +#include "compiler/compiler_factory.h" +#include "compiler/optional.h" +#include "compiler/standard_library.h" +#include "extensions/comprehensions_v2.h" #include "internal/proto_matchers.h" #include "internal/status_macros.h" #include "internal/testing.h" -#include "parser/options.h" -#include "parser/parser.h" +#include "internal/testing_descriptor_pool.h" #include "google/protobuf/text_format.h" namespace cel { @@ -51,7 +59,6 @@ using ::cel::ast_internal::WellKnownType; using ::cel::internal::test::EqualsProto; using ::cel::expr::CheckedExpr; using ::cel::expr::ParsedExpr; -using ::google::api::expr::parser::Parse; using ::testing::HasSubstr; using TypePb = cel::expr::Type; @@ -804,17 +811,50 @@ class ConversionRoundTripTest : public testing::TestWithParam { public: ConversionRoundTripTest() { - options_.add_macro_calls = true; - options_.enable_optional_syntax = true; + auto builder = + cel::NewCompilerBuilder(internal::GetTestingDescriptorPool()).value(); + builder->AddLibrary(cel::StandardCompilerLibrary()).IgnoreError(); + builder->AddLibrary(OptionalCompilerLibrary()).IgnoreError(); + builder->AddLibrary(extensions::ComprehensionsV2CompilerLibrary()) + .IgnoreError(); + builder->GetCheckerBuilder().set_container("cel.expr.conformance.proto3"); + builder->GetCheckerBuilder() + .AddVariable(MakeVariableDecl("ident", IntType())) + .IgnoreError(); + builder->GetCheckerBuilder() + .AddVariable(MakeVariableDecl("map_ident", JsonMapType())) + .IgnoreError(); + compiler_ = builder->Build().value(); + } + + absl::StatusOr ParseToProto(absl::string_view expr) { + CEL_ASSIGN_OR_RETURN(auto source, cel::NewSource(expr)); + + CEL_ASSIGN_OR_RETURN(auto result, compiler_->GetParser().Parse(*source)); + ParsedExpr parsed_expr; + + CEL_RETURN_IF_ERROR(AstToParsedExpr(*result, &parsed_expr)); + return parsed_expr; + } + + absl::StatusOr CompileToProto(absl::string_view expr) { + CEL_ASSIGN_OR_RETURN(auto result, compiler_->Compile(expr)); + if (!result.IsValid()) { + return absl::InvalidArgumentError(absl::StrCat( + "Compilation failed: '", expr, "': ", result.FormatError())); + } + CEL_ASSIGN_OR_RETURN(auto ast, result.ReleaseAst()); + CheckedExpr checked_expr; + CEL_RETURN_IF_ERROR(AstToCheckedExpr(*ast, &checked_expr)); + return checked_expr; } protected: - ParserOptions options_; + std::unique_ptr compiler_; }; TEST_P(ConversionRoundTripTest, ParsedExprCopyable) { - ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, - Parse(GetParam().expr, "", options_)); + ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, ParseToProto(GetParam().expr)); ASSERT_OK_AND_ASSIGN(std::unique_ptr ast, CreateAstFromParsedExpr(parsed_expr)); @@ -825,31 +865,52 @@ TEST_P(ConversionRoundTripTest, ParsedExprCopyable) { EXPECT_THAT(AstToCheckedExpr(impl, &expr_pb), StatusIs(absl::StatusCode::kInvalidArgument, HasSubstr("AST is not type-checked"))); - ParsedExpr copy; - ASSERT_THAT(AstToParsedExpr(impl, ©), IsOk()); - EXPECT_THAT(copy, EqualsProto(parsed_expr)); + ParsedExpr proto_out; + ASSERT_THAT(AstToParsedExpr(impl, &proto_out), IsOk()); + EXPECT_THAT(proto_out, EqualsProto(parsed_expr)); } -TEST_P(ConversionRoundTripTest, CheckedExprCopyable) { - ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, - Parse(GetParam().expr, "", options_)); +TEST_P(ConversionRoundTripTest, ExprCopyable) { + ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, ParseToProto(GetParam().expr)); - CheckedExpr checked_expr; - *checked_expr.mutable_expr() = parsed_expr.expr(); - *checked_expr.mutable_source_info() = parsed_expr.source_info(); + ASSERT_OK_AND_ASSIGN(std::unique_ptr ast, + CreateAstFromParsedExpr(parsed_expr)); + + Expr copy = ast->root_expr(); + ast->mutable_root_expr() = std::move(copy); + + ParsedExpr parsed_pb_out; + CheckedExpr checked_pb_out; + EXPECT_THAT(AstToCheckedExpr(*ast, &checked_pb_out), + StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("AST is not type-checked"))); + ASSERT_THAT(AstToParsedExpr(*ast, &parsed_pb_out), IsOk()); + EXPECT_THAT(parsed_pb_out, EqualsProto(parsed_expr)); +} - int64_t root_id = checked_expr.expr().id(); - (*checked_expr.mutable_reference_map())[root_id].add_overload_id("_==_"); - (*checked_expr.mutable_type_map())[root_id].set_primitive(TypePb::BOOL); +TEST_P(ConversionRoundTripTest, CheckedExprRoundTrip) { + ASSERT_OK_AND_ASSIGN(CheckedExpr checked_expr, + CompileToProto(GetParam().expr)); ASSERT_OK_AND_ASSIGN(std::unique_ptr ast, CreateAstFromCheckedExpr(checked_expr)); - const auto& impl = ast_internal::AstImpl::CastFromPublicAst(*ast); + CheckedExpr checked_pb_out; + ASSERT_THAT(AstToCheckedExpr(*ast, &checked_pb_out), IsOk()); + EXPECT_THAT(checked_pb_out, EqualsProto(checked_expr)); +} - CheckedExpr expr_pb; - ASSERT_THAT(AstToCheckedExpr(impl, &expr_pb), IsOk()); - EXPECT_THAT(expr_pb, EqualsProto(checked_expr)); +TEST_P(ConversionRoundTripTest, CheckedExprCopyRoundTrip) { + ASSERT_OK_AND_ASSIGN(CheckedExpr checked_expr, + CompileToProto(GetParam().expr)); + + ASSERT_OK_AND_ASSIGN(std::unique_ptr ast, + CreateAstFromCheckedExpr(checked_expr)); + + Ast copy = *ast; + CheckedExpr checked_pb_out; + ASSERT_THAT(AstToCheckedExpr(copy, &checked_pb_out), IsOk()); + EXPECT_THAT(checked_pb_out, EqualsProto(checked_expr)); } INSTANTIATE_TEST_SUITE_P( @@ -863,11 +924,12 @@ INSTANTIATE_TEST_SUITE_P( {R"cel("42" == "42")cel"}, {R"cel("s".startsWith("s") == true)cel"}, {R"cel([1, 2, 3] == [1, 2, 3])cel"}, + {R"cel([1, 2, 3].all(i, e, i == e - 1) == true)cel"}, {R"cel(TestAllTypes{single_int64: 42}.single_int64 == 42)cel"}, {R"cel([1, 2, 3].map(x, x + 2).size() == 3)cel"}, {R"cel({"a": 1, "b": 2}["a"] == 1)cel"}, {R"cel(ident == 42)cel"}, - {R"cel(ident.field == 42)cel"}, + {R"cel(map_ident.field == 42)cel"}, {R"cel({?"abc": {}[?1]}.?abc.orValue(42) == 42)cel"}, {R"cel([1, 2, ?optional.none()].size() == 2)cel"}})); @@ -895,10 +957,8 @@ TEST(ExtensionConversionRoundTripTest, RoundTrip) { ASSERT_OK_AND_ASSIGN(std::unique_ptr ast, CreateAstFromParsedExpr(parsed_expr)); - const auto& impl = ast_internal::AstImpl::CastFromPublicAst(*ast); - CheckedExpr expr_pb; - EXPECT_THAT(AstToCheckedExpr(impl, &expr_pb), + EXPECT_THAT(AstToCheckedExpr(*ast, &expr_pb), StatusIs(absl::StatusCode::kInvalidArgument, HasSubstr("AST is not type-checked"))); ParsedExpr copy; diff --git a/common/expr.cc b/common/expr.cc index 60fb97050..f32c9ebb2 100644 --- a/common/expr.cc +++ b/common/expr.cc @@ -14,10 +14,136 @@ #include "common/expr.h" +#include +#include + #include "absl/base/no_destructor.h" +#include "absl/functional/overload.h" +#include "absl/types/variant.h" +#include "common/constant.h" namespace cel { +namespace { + +struct CopyStackRecord { + const Expr* src; + Expr* dst; +}; + +void CopyNode(CopyStackRecord element, std::vector& stack) { + const Expr* src = element.src; + Expr* dst = element.dst; + dst->set_id(src->id()); + absl::visit( + absl::Overload( + [=](const UnspecifiedExpr&) { + dst->mutable_kind().emplace(); + }, + [=](const IdentExpr& i) { + dst->mutable_ident_expr().set_name(i.name()); + }, + [=](const Constant& c) { dst->mutable_const_expr() = c; }, + [&](const SelectExpr& s) { + dst->mutable_select_expr().set_field(s.field()); + dst->mutable_select_expr().set_test_only(s.test_only()); + + if (s.has_operand()) { + stack.push_back({&s.operand(), + &dst->mutable_select_expr().mutable_operand()}); + } + }, + [&](const CallExpr& c) { + dst->mutable_call_expr().set_function(c.function()); + if (c.has_target()) { + stack.push_back( + {&c.target(), &dst->mutable_call_expr().mutable_target()}); + } + dst->mutable_call_expr().mutable_args().resize(c.args().size()); + for (int i = 0; i < dst->mutable_call_expr().mutable_args().size(); + ++i) { + stack.push_back( + {&c.args()[i], &dst->mutable_call_expr().mutable_args()[i]}); + } + }, + [&](const ListExpr& c) { + auto& dst_list = dst->mutable_list_expr(); + dst_list.mutable_elements().resize(c.elements().size()); + for (int i = 0; i < src->list_expr().elements().size(); ++i) { + dst_list.mutable_elements()[i].set_optional( + c.elements()[i].optional()); + stack.push_back({&c.elements()[i].expr(), + &dst_list.mutable_elements()[i].mutable_expr()}); + } + }, + [&](const StructExpr& s) { + auto& dst_struct = dst->mutable_struct_expr(); + dst_struct.mutable_fields().resize(s.fields().size()); + dst_struct.set_name(s.name()); + for (int i = 0; i < s.fields().size(); ++i) { + dst_struct.mutable_fields()[i].set_optional( + s.fields()[i].optional()); + dst_struct.mutable_fields()[i].set_name(s.fields()[i].name()); + dst_struct.mutable_fields()[i].set_id(s.fields()[i].id()); + stack.push_back( + {&s.fields()[i].value(), + &dst_struct.mutable_fields()[i].mutable_value()}); + } + }, + [&](const MapExpr& c) { + auto& dst_map = dst->mutable_map_expr(); + dst_map.mutable_entries().resize(c.entries().size()); + for (int i = 0; i < c.entries().size(); ++i) { + dst_map.mutable_entries()[i].set_optional( + c.entries()[i].optional()); + dst_map.mutable_entries()[i].set_id(c.entries()[i].id()); + stack.push_back({&c.entries()[i].key(), + &dst_map.mutable_entries()[i].mutable_key()}); + stack.push_back({&c.entries()[i].value(), + &dst_map.mutable_entries()[i].mutable_value()}); + } + }, + [&](const ComprehensionExpr& c) { + auto& dst_comprehension = dst->mutable_comprehension_expr(); + dst_comprehension.set_iter_var(c.iter_var()); + dst_comprehension.set_iter_var2(c.iter_var2()); + dst_comprehension.set_accu_var(c.accu_var()); + if (c.has_accu_init()) { + stack.push_back( + {&c.accu_init(), &dst_comprehension.mutable_accu_init()}); + } + if (c.has_iter_range()) { + stack.push_back( + {&c.iter_range(), &dst_comprehension.mutable_iter_range()}); + } + if (c.has_loop_condition()) { + stack.push_back({&c.loop_condition(), + &dst_comprehension.mutable_loop_condition()}); + } + if (c.has_loop_step()) { + stack.push_back( + {&c.loop_step(), &dst_comprehension.mutable_loop_step()}); + } + if (c.has_result()) { + stack.push_back( + {&c.result(), &dst_comprehension.mutable_result()}); + } + }), + src->kind()); +} + +void CloneImpl(const Expr& expr, Expr& dst) { + std::vector stack; + stack.push_back({&expr, &dst}); + while (!stack.empty()) { + CopyStackRecord element = stack.back(); + stack.pop_back(); + CopyNode(element, stack); + } +} + +} // namespace + const UnspecifiedExpr& UnspecifiedExpr::default_instance() { static const absl::NoDestructor instance; return *instance; @@ -63,4 +189,16 @@ const Expr& Expr::default_instance() { return *instance; } +Expr& Expr::operator=(const Expr& other) { + if (this == &other) { + return *this; + } + Expr tmp; + CloneImpl(other, tmp); + *this = std::move(tmp); + return *this; +} + +Expr::Expr(const Expr& other) { CloneImpl(other, *this); } + } // namespace cel diff --git a/common/expr.h b/common/expr.h index fb7edbf1d..1a40fff23 100644 --- a/common/expr.h +++ b/common/expr.h @@ -24,7 +24,6 @@ #include "absl/algorithm/container.h" #include "absl/base/attributes.h" #include "absl/strings/string_view.h" -#include "absl/types/optional.h" #include "absl/types/span.h" #include "absl/types/variant.h" #include "common/constant.h" @@ -931,8 +930,8 @@ class Expr final { Expr(Expr&&) = default; Expr& operator=(Expr&&); - Expr(const Expr&) = delete; - Expr& operator=(const Expr&) = delete; + Expr(const Expr&); + Expr& operator=(const Expr&); void Clear(); diff --git a/common/expr_test.cc b/common/expr_test.cc index 569f4117d..9a550527c 100644 --- a/common/expr_test.cc +++ b/common/expr_test.cc @@ -14,7 +14,6 @@ #include "common/expr.h" -#include #include #include "internal/testing.h" @@ -80,7 +79,7 @@ TEST(IdentExpr, Name) { TEST(IdentExpr, Equality) { EXPECT_EQ(IdentExpr{}, IdentExpr{}); IdentExpr ident_expr; - ident_expr.set_name(std::string("foo")); + ident_expr.set_name("foo"); EXPECT_NE(IdentExpr{}, ident_expr); } @@ -556,6 +555,31 @@ TEST(Expr, Comprehension) { EXPECT_EQ(expr, Expr{}); } +TEST(Expr, CopyCtor) { + Expr expr; + expr.mutable_select_expr().set_field("foo"); + Expr& operand = expr.mutable_select_expr().mutable_operand(); + operand.mutable_ident_expr().set_name("bar"); + Expr expr_copy = expr; + EXPECT_EQ(expr_copy.select_expr().field(), "foo"); + EXPECT_EQ(expr_copy.select_expr().operand().ident_expr().name(), "bar"); +} + +TEST(Expr, CopyAssignChildReference) { + Expr expr; + expr.mutable_select_expr().set_field("foo"); + Expr& operand = expr.mutable_select_expr().mutable_operand(); + operand.mutable_call_expr().set_function("bar"); + auto& args = operand.mutable_call_expr().mutable_args(); + args.emplace_back().mutable_ident_expr().set_name("baz"); + args.emplace_back().mutable_ident_expr().set_name("qux"); + expr = expr.mutable_select_expr().mutable_operand(); + EXPECT_EQ(expr.call_expr().function(), "bar"); + EXPECT_EQ(expr.call_expr().args().size(), 2); + EXPECT_EQ(expr.call_expr().args()[0].ident_expr().name(), "baz"); + EXPECT_EQ(expr.call_expr().args()[1].ident_expr().name(), "qux"); +} + TEST(Expr, Id) { Expr expr; EXPECT_THAT(expr.id(), Eq(0));