From f327daf0d1ffeadcb9d32b264f44fba822e6a421 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Wed, 15 Jul 2026 13:39:30 -0700 Subject: [PATCH] Add support for resolving types at plan time. Add an option to prefetch field descriptors when type is known. PiperOrigin-RevId: 948520646 --- checker/internal/BUILD | 35 +- checker/internal/type_check_env.h | 2 +- common/BUILD | 35 ++ .../descriptor_pool_type_introspector.cc | 8 +- .../descriptor_pool_type_introspector.h | 13 +- .../descriptor_pool_type_introspector_test.cc | 8 +- common/signature.cc | 2 +- common/signature_test.cc | 9 +- common/type.cc | 5 + common/type.h | 1 + common/type_spec_resolver.cc | 94 ++-- common/type_spec_resolver.h | 10 +- common/type_spec_resolver_test.cc | 66 ++- common/types/message_type.h | 13 +- common/value.cc | 1 + common/values/parsed_message_value.h | 24 +- eval/compiler/BUILD | 1 + eval/compiler/flat_expr_builder.cc | 88 +++- eval/eval/BUILD | 4 +- eval/eval/attribute_trail.h | 1 + eval/eval/select_step.cc | 430 ++++++++---------- eval/eval/select_step.h | 13 +- eval/eval/select_step_test.cc | 62 ++- eval/public/cel_options.cc | 49 +- eval/tests/BUILD | 10 +- eval/tests/modern_benchmark_test.cc | 104 +++-- runtime/internal/BUILD | 1 + runtime/internal/runtime_type_provider.cc | 45 +- runtime/internal/runtime_type_provider.h | 7 +- runtime/runtime_options.h | 13 + 30 files changed, 683 insertions(+), 471 deletions(-) rename {checker/internal => common}/descriptor_pool_type_introspector.cc (98%) rename {checker/internal => common}/descriptor_pool_type_introspector.h (91%) rename {checker/internal => common}/descriptor_pool_type_introspector_test.cc (97%) diff --git a/checker/internal/BUILD b/checker/internal/BUILD index 20c476db2..5e1bdbdcd 100644 --- a/checker/internal/BUILD +++ b/checker/internal/BUILD @@ -65,12 +65,12 @@ cc_library( srcs = ["type_check_env.cc"], hdrs = ["type_check_env.h"], deps = [ - ":descriptor_pool_type_introspector", ":proto_type_mask", ":proto_type_mask_registry", "//common:constant", "//common:container", "//common:decl", + "//common:descriptor_pool_type_introspector", "//common:type", "//internal:status_macros", "@com_google_absl//absl/base:core_headers", @@ -275,39 +275,6 @@ cc_test( ], ) -cc_library( - name = "descriptor_pool_type_introspector", - srcs = ["descriptor_pool_type_introspector.cc"], - hdrs = ["descriptor_pool_type_introspector.h"], - deps = [ - "//common:type", - "@com_google_absl//absl/base:core_headers", - "@com_google_absl//absl/base:nullability", - "@com_google_absl//absl/container:flat_hash_map", - "@com_google_absl//absl/log:absl_check", - "@com_google_absl//absl/status:statusor", - "@com_google_absl//absl/strings:string_view", - "@com_google_absl//absl/synchronization", - "@com_google_absl//absl/types:optional", - "@com_google_absl//absl/types:span", - "@com_google_protobuf//:protobuf", - ], -) - -cc_test( - name = "descriptor_pool_type_introspector_test", - srcs = ["descriptor_pool_type_introspector_test.cc"], - deps = [ - ":descriptor_pool_type_introspector", - "//common:type", - "//internal:testing", - "//internal:testing_descriptor_pool", - "@com_google_absl//absl/status:status_matchers", - "@com_google_absl//absl/status:statusor", - "@com_google_absl//absl/types:optional", - ], -) - cc_library( name = "field_path", srcs = ["field_path.cc"], diff --git a/checker/internal/type_check_env.h b/checker/internal/type_check_env.h index 00fea0ba3..0fda608ab 100644 --- a/checker/internal/type_check_env.h +++ b/checker/internal/type_check_env.h @@ -30,12 +30,12 @@ #include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "absl/types/span.h" -#include "checker/internal/descriptor_pool_type_introspector.h" #include "checker/internal/proto_type_mask.h" #include "checker/internal/proto_type_mask_registry.h" #include "common/constant.h" #include "common/container.h" #include "common/decl.h" +#include "common/descriptor_pool_type_introspector.h" #include "common/type.h" #include "common/type_introspector.h" #include "internal/status_macros.h" diff --git a/common/BUILD b/common/BUILD index 0426c0827..a6b57ab31 100644 --- a/common/BUILD +++ b/common/BUILD @@ -52,12 +52,14 @@ cc_library( hdrs = ["type_spec_resolver.h"], deps = [ ":ast", + ":descriptor_pool_type_introspector", ":type", ":type_kind", "//internal:status_macros", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", + "@com_google_absl//absl/types:optional", "@com_google_protobuf//:protobuf", ], ) @@ -79,6 +81,39 @@ cc_test( ], ) +cc_library( + name = "descriptor_pool_type_introspector", + srcs = ["descriptor_pool_type_introspector.cc"], + hdrs = ["descriptor_pool_type_introspector.h"], + deps = [ + ":type", + "@com_google_absl//absl/base:core_headers", + "@com_google_absl//absl/base:nullability", + "@com_google_absl//absl/container:flat_hash_map", + "@com_google_absl//absl/log:absl_check", + "@com_google_absl//absl/status:statusor", + "@com_google_absl//absl/strings:string_view", + "@com_google_absl//absl/synchronization", + "@com_google_absl//absl/types:optional", + "@com_google_absl//absl/types:span", + "@com_google_protobuf//:protobuf", + ], +) + +cc_test( + name = "descriptor_pool_type_introspector_test", + srcs = ["descriptor_pool_type_introspector_test.cc"], + deps = [ + ":descriptor_pool_type_introspector", + ":type", + "//internal:testing", + "//internal:testing_descriptor_pool", + "@com_google_absl//absl/status:status_matchers", + "@com_google_absl//absl/status:statusor", + "@com_google_absl//absl/types:optional", + ], +) + cc_library( name = "signature", srcs = ["signature.cc"], diff --git a/checker/internal/descriptor_pool_type_introspector.cc b/common/descriptor_pool_type_introspector.cc similarity index 98% rename from checker/internal/descriptor_pool_type_introspector.cc rename to common/descriptor_pool_type_introspector.cc index 733e4a3cb..fa9ed2a0e 100644 --- a/checker/internal/descriptor_pool_type_introspector.cc +++ b/common/descriptor_pool_type_introspector.cc @@ -12,9 +12,10 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "checker/internal/descriptor_pool_type_introspector.h" +#include "common/descriptor_pool_type_introspector.h" #include +#include #include #include @@ -24,13 +25,12 @@ #include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "absl/synchronization/mutex.h" -#include "absl/types/optional.h" #include "absl/types/span.h" #include "common/type.h" #include "common/type_introspector.h" #include "google/protobuf/descriptor.h" -namespace cel::checker_internal { +namespace cel { namespace { // Standard implementation for field lookups. @@ -242,4 +242,4 @@ DescriptorPoolTypeIntrospector::CreateFieldTable( return result; } -} // namespace cel::checker_internal +} // namespace cel diff --git a/checker/internal/descriptor_pool_type_introspector.h b/common/descriptor_pool_type_introspector.h similarity index 91% rename from checker/internal/descriptor_pool_type_introspector.h rename to common/descriptor_pool_type_introspector.h index 8a970ea00..e7d6091d2 100644 --- a/checker/internal/descriptor_pool_type_introspector.h +++ b/common/descriptor_pool_type_introspector.h @@ -12,8 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -#ifndef THIRD_PARTY_CEL_CPP_CHECKER_INTERNAL_DESCRIPTOR_POOL_TYPE_INTROSPECTOR_H_ -#define THIRD_PARTY_CEL_CPP_CHECKER_INTERNAL_DESCRIPTOR_POOL_TYPE_INTROSPECTOR_H_ +#ifndef THIRD_PARTY_CEL_CPP_COMMON_DESCRIPTOR_POOL_TYPE_INTROSPECTOR_H_ +#define THIRD_PARTY_CEL_CPP_COMMON_DESCRIPTOR_POOL_TYPE_INTROSPECTOR_H_ #include #include @@ -30,14 +30,15 @@ #include "common/type_introspector.h" #include "google/protobuf/descriptor.h" -namespace cel::checker_internal { +namespace cel { // Implementation of `TypeIntrospector` that uses a `google::protobuf::DescriptorPool`. // // This is used by the type checker to resolve protobuf types and their fields // and apply any options like using JSON names. // -// Neither copyable nor movable. Should be managed by a TypeCheckEnv. +// Neither copyable nor movable. Should be managed by a TypeCheckEnv or a +// runtime Environment. class DescriptorPoolTypeIntrospector : public TypeIntrospector { public: struct Field { @@ -100,6 +101,6 @@ class DescriptorPoolTypeIntrospector : public TypeIntrospector { const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool_; }; -} // namespace cel::checker_internal +} // namespace cel -#endif // THIRD_PARTY_CEL_CPP_CHECKER_INTERNAL_DESCRIPTOR_POOL_TYPE_INTROSPECTOR_H_ +#endif // THIRD_PARTY_CEL_CPP_COMMON_DESCRIPTOR_POOL_TYPE_INTROSPECTOR_H_ diff --git a/checker/internal/descriptor_pool_type_introspector_test.cc b/common/descriptor_pool_type_introspector_test.cc similarity index 97% rename from checker/internal/descriptor_pool_type_introspector_test.cc rename to common/descriptor_pool_type_introspector_test.cc index db766b347..94acd317f 100644 --- a/checker/internal/descriptor_pool_type_introspector_test.cc +++ b/common/descriptor_pool_type_introspector_test.cc @@ -12,19 +12,19 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "checker/internal/descriptor_pool_type_introspector.h" +#include "common/descriptor_pool_type_introspector.h" +#include #include #include "absl/status/status_matchers.h" #include "absl/status/statusor.h" -#include "absl/types/optional.h" #include "common/type.h" #include "common/type_introspector.h" #include "internal/testing.h" #include "internal/testing_descriptor_pool.h" -namespace cel::checker_internal { +namespace cel { namespace { using ::absl_testing::IsOkAndHolds; @@ -172,4 +172,4 @@ TEST(DescriptorPoolTypeIntrospectorTest, ListFieldsForStructTypeNotFound) { } } // namespace -} // namespace cel::checker_internal +} // namespace cel diff --git a/common/signature.cc b/common/signature.cc index 54d312777..3f4ea8d29 100644 --- a/common/signature.cc +++ b/common/signature.cc @@ -634,7 +634,7 @@ absl::StatusOr ParseTypeSpec(std::string_view signature) { absl::StatusOr ParseType(std::string_view signature, google::protobuf::Arena* arena, const google::protobuf::DescriptorPool& pool) { CEL_ASSIGN_OR_RETURN(auto type_spec, ParseTypeSpec(signature)); - return cel::ConvertTypeSpecToType(type_spec, arena, pool); + return cel::ConvertTypeSpecToType(type_spec, pool, arena); } } // namespace cel diff --git a/common/signature_test.cc b/common/signature_test.cc index ea51eb566..e157d5be0 100644 --- a/common/signature_test.cc +++ b/common/signature_test.cc @@ -85,7 +85,7 @@ TEST_P(TypeSignatureTest, TypeSignature) { EXPECT_THAT(signature, IsOkAndHolds(param.expected_signature)); absl::StatusOr type = ConvertTypeSpecToType( - param.type, GetTestArena(), *GetTestingDescriptorPool()); + param.type, *GetTestingDescriptorPool(), GetTestArena()); ASSERT_THAT(type, ::absl_testing::IsOk()); EXPECT_THAT(MakeTypeSignature(*type), IsOkAndHolds(param.expected_signature)); @@ -285,9 +285,10 @@ TEST_P(TypeSignatureTest, ParseTypeCheck) { auto parsed = ParseType(param.expected_signature, GetTestArena(), *GetTestingDescriptorPool()); ASSERT_THAT(parsed, ::absl_testing::IsOk()); - ASSERT_OK_AND_ASSIGN(auto expected_type, - ConvertTypeSpecToType(param.type, GetTestArena(), - *GetTestingDescriptorPool())); + ASSERT_OK_AND_ASSIGN( + auto expected_type, + ConvertTypeSpecToType(param.type, *GetTestingDescriptorPool(), + GetTestArena())); VerifyTypesEqual(*parsed, expected_type); } } diff --git a/common/type.cc b/common/type.cc index 9ea85954c..2d3469540 100644 --- a/common/type.cc +++ b/common/type.cc @@ -606,6 +606,11 @@ absl::optional StructTypeField::AsMessage() const { return std::nullopt; } +MessageTypeField StructTypeField::GetMessage() const { + ABSL_DCHECK(IsMessage()); + return absl::get(variant_); +} + StructTypeField::operator MessageTypeField() const { ABSL_DCHECK(IsMessage()); return absl::get(variant_); diff --git a/common/type.h b/common/type.h index c8851dd4e..e60452bfd 100644 --- a/common/type.h +++ b/common/type.h @@ -1140,6 +1140,7 @@ class StructTypeField final { } absl::optional AsMessage() const; + MessageTypeField GetMessage() const; explicit operator MessageTypeField() const; diff --git a/common/type_spec_resolver.cc b/common/type_spec_resolver.cc index 90c9930a8..c93645718 100644 --- a/common/type_spec_resolver.cc +++ b/common/type_spec_resolver.cc @@ -23,18 +23,36 @@ #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" +#include "absl/types/optional.h" #include "common/ast.h" +#include "common/descriptor_pool_type_introspector.h" #include "common/type.h" +#include "common/type_introspector.h" #include "common/type_kind.h" #include "internal/status_macros.h" #include "google/protobuf/arena.h" #include "google/protobuf/descriptor.h" namespace cel { +namespace { -absl::StatusOr ConvertTypeSpecToType(const TypeSpec& type_spec, - google::protobuf::Arena* arena, - const google::protobuf::DescriptorPool& pool) { +absl::StatusOr> ResolveNamedType( + absl::string_view name, const TypeIntrospector& type_introspector) { + absl::optional type = FindWellKnownType(name); + if (type.has_value()) { + return type; + } + return type_introspector.FindType(name); +} + +bool TypeAcceptsParameters(const Type& type) { + return type.IsOpaque() || type.IsType() || type.IsList() || type.IsMap(); +} +} // namespace + +absl::StatusOr ConvertTypeSpecToType( + const TypeSpec& type_spec, const TypeIntrospector& type_introspector, + google::protobuf::Arena* arena) { if (type_spec.has_null()) return Type(NullType{}); if (type_spec.has_dyn()) return Type(DynType{}); @@ -94,7 +112,7 @@ absl::StatusOr ConvertTypeSpecToType(const TypeSpec& type_spec, if (type_spec.list_type().elem_type().is_specified()) { CEL_ASSIGN_OR_RETURN( elem_type, ConvertTypeSpecToType(type_spec.list_type().elem_type(), - arena, pool)); + type_introspector, arena)); } return Type(ListType(arena, elem_type)); } @@ -103,15 +121,15 @@ absl::StatusOr ConvertTypeSpecToType(const TypeSpec& type_spec, Type key_type; if (type_spec.map_type().key_type().is_specified()) { CEL_ASSIGN_OR_RETURN( - key_type, - ConvertTypeSpecToType(type_spec.map_type().key_type(), arena, pool)); + key_type, ConvertTypeSpecToType(type_spec.map_type().key_type(), + type_introspector, arena)); } Type value_type; if (type_spec.map_type().value_type().is_specified()) { CEL_ASSIGN_OR_RETURN( value_type, ConvertTypeSpecToType(type_spec.map_type().value_type(), - arena, pool)); + type_introspector, arena)); } return Type(MapType(arena, key_type, value_type)); } @@ -120,15 +138,16 @@ absl::StatusOr ConvertTypeSpecToType(const TypeSpec& type_spec, const auto& func_spec = type_spec.function(); Type result_type; if (func_spec.result_type().is_specified()) { - CEL_ASSIGN_OR_RETURN( - result_type, - ConvertTypeSpecToType(func_spec.result_type(), arena, pool)); + CEL_ASSIGN_OR_RETURN(result_type, + ConvertTypeSpecToType(func_spec.result_type(), + type_introspector, arena)); } std::vector arg_types; arg_types.reserve(func_spec.arg_types().size()); for (const auto& arg_spec : func_spec.arg_types()) { - CEL_ASSIGN_OR_RETURN(auto arg_type, - ConvertTypeSpecToType(arg_spec, arena, pool)); + CEL_ASSIGN_OR_RETURN( + auto arg_type, + ConvertTypeSpecToType(arg_spec, type_introspector, arena)); arg_types.push_back(std::move(arg_type)); } return Type(FunctionType(arena, result_type, arg_types)); @@ -142,43 +161,35 @@ absl::StatusOr ConvertTypeSpecToType(const TypeSpec& type_spec, if (type_spec.has_message_type()) { const std::string& name = type_spec.message_type().type(); - const google::protobuf::Descriptor* descriptor = pool.FindMessageTypeByName(name); - if (descriptor == nullptr) { + CEL_ASSIGN_OR_RETURN(absl::optional type, + ResolveNamedType(name, type_introspector)); + if (!type.has_value()) { return absl::InvalidArgumentError(absl::StrCat( "Message type '", name, "' not found in descriptor pool")); } - return Type::Message(descriptor); + return *type; } if (type_spec.has_abstract_type()) { const std::string& name = type_spec.abstract_type().name(); - // Check if it's a message type in the pool - const google::protobuf::Descriptor* descriptor = pool.FindMessageTypeByName(name); - if (descriptor != nullptr) { - if (!type_spec.abstract_type().parameter_types().empty()) { - return absl::InvalidArgumentError(absl::StrCat( - "Message type '", name, "' cannot have type parameters")); - } - return Type::Message(descriptor); - } - - // Check if it's an enum type in the pool - const google::protobuf::EnumDescriptor* enum_descriptor = - pool.FindEnumTypeByName(name); - if (enum_descriptor != nullptr) { - if (!type_spec.abstract_type().parameter_types().empty()) { + CEL_ASSIGN_OR_RETURN(absl::optional type, + ResolveNamedType(name, type_introspector)); + if (type.has_value()) { + if (!TypeAcceptsParameters(*type) && + !type_spec.abstract_type().parameter_types().empty()) { return absl::InvalidArgumentError( - absl::StrCat("Enum type '", name, "' cannot have type parameters")); + absl::StrCat("Type '", name, "' cannot have type parameters")); } - return Type::Enum(enum_descriptor); + return *type; } // Otherwise fallback to OpaqueType std::vector params; for (const auto& param_spec : type_spec.abstract_type().parameter_types()) { - CEL_ASSIGN_OR_RETURN(auto param, - ConvertTypeSpecToType(param_spec, arena, pool)); + CEL_ASSIGN_OR_RETURN( + auto param, + ConvertTypeSpecToType(param_spec, type_introspector, arena)); params.push_back(std::move(param)); } auto* allocated_name = google::protobuf::Arena::Create(arena, name); @@ -186,8 +197,9 @@ absl::StatusOr ConvertTypeSpecToType(const TypeSpec& type_spec, } if (type_spec.has_type()) { - CEL_ASSIGN_OR_RETURN(auto contained_type, - ConvertTypeSpecToType(type_spec.type(), arena, pool)); + CEL_ASSIGN_OR_RETURN( + auto contained_type, + ConvertTypeSpecToType(type_spec.type(), type_introspector, arena)); return Type(TypeType(arena, contained_type)); } @@ -198,6 +210,16 @@ absl::StatusOr ConvertTypeSpecToType(const TypeSpec& type_spec, return absl::InvalidArgumentError("Unknown TypeSpec kind"); } +absl::StatusOr ConvertTypeSpecToType(const TypeSpec& type_spec, + const google::protobuf::DescriptorPool& pool, + google::protobuf::Arena* arena) { + // In this configuration we will only ever reference objects backed by the + // given DescriptorPool or global constants so it is safe for `introspector` + // to go out of scope, but this is not sound for arbitrary TypeIntrospectors. + DescriptorPoolTypeIntrospector introspector(&pool); + return ConvertTypeSpecToType(type_spec, introspector, arena); +} + absl::StatusOr ConvertTypeToTypeSpec(const Type& type) { switch (type.kind()) { case TypeKind::kNull: diff --git a/common/type_spec_resolver.h b/common/type_spec_resolver.h index edbfa3bde..2d460be29 100644 --- a/common/type_spec_resolver.h +++ b/common/type_spec_resolver.h @@ -18,6 +18,7 @@ #include "absl/status/statusor.h" #include "common/ast.h" #include "common/type.h" +#include "common/type_introspector.h" #include "google/protobuf/arena.h" #include "google/protobuf/descriptor.h" @@ -28,9 +29,14 @@ namespace cel { // TypeSpec only specifies a type while Type provides support for inspecting // properties of the type when used in CEL. Returns a status with code // `InvalidArgument` if the input cannot be resolved to a type. +absl::StatusOr ConvertTypeSpecToType( + const TypeSpec& type_spec, const cel::TypeIntrospector& type_introspector, + google::protobuf::Arena* arena); + +// Convenience overload when assuming no special type resolution is needed. absl::StatusOr ConvertTypeSpecToType(const TypeSpec& type_spec, - google::protobuf::Arena* arena, - const google::protobuf::DescriptorPool& pool); + const google::protobuf::DescriptorPool& pool, + google::protobuf::Arena* arena); // Resolves a `cel::Type` to a `cel::TypeSpec`. absl::StatusOr ConvertTypeToTypeSpec(const Type& type); diff --git a/common/type_spec_resolver_test.cc b/common/type_spec_resolver_test.cc index 1cda7280f..f678cde6a 100644 --- a/common/type_spec_resolver_test.cc +++ b/common/type_spec_resolver_test.cc @@ -49,7 +49,7 @@ google::protobuf::Arena* GetTestArena() { TEST(TypeSpecResolverTest, NullTypeSpec) { TypeSpec spec(NullTypeSpec{}); auto t = - ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool()); + ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena()); ASSERT_THAT(t, IsOk()); EXPECT_TRUE(t->IsNull()); } @@ -57,7 +57,7 @@ TEST(TypeSpecResolverTest, NullTypeSpec) { TEST(TypeSpecResolverTest, DynTypeSpec) { TypeSpec spec(DynTypeSpec{}); auto t = - ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool()); + ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena()); ASSERT_THAT(t, IsOk()); EXPECT_TRUE(t->IsDyn()); } @@ -66,8 +66,9 @@ using ConversionTest = testing::TestWithParam>; TEST_P(ConversionTest, TestTypeSpecConversion) { ASSERT_OK_AND_ASSIGN( - auto t, ConvertTypeSpecToType(std::get<0>(GetParam()), GetTestArena(), - *GetTestingDescriptorPool())); + auto t, + ConvertTypeSpecToType(std::get<0>(GetParam()), + *GetTestingDescriptorPool(), GetTestArena())); EXPECT_EQ(t.kind(), std::get<1>(GetParam())); EXPECT_THAT(ConvertTypeToTypeSpec(t), IsOkAndHolds(std::get<0>(GetParam()))); } @@ -103,7 +104,7 @@ TEST(TypeSpecResolverTest, ListTypeConversion) { auto elem = std::make_unique(PrimitiveType::kInt64); TypeSpec spec(ListTypeSpec(std::move(elem))); auto t = - ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool()); + ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena()); ASSERT_THAT(t, IsOk()); EXPECT_TRUE(t->IsList()); EXPECT_TRUE(t->GetList().element().IsInt()); @@ -116,7 +117,7 @@ TEST(TypeSpecResolverTest, MapTypeConversion) { auto val = std::make_unique(PrimitiveType::kBytes); TypeSpec spec(MapTypeSpec(std::move(key), std::move(val))); auto t = - ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool()); + ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena()); ASSERT_THAT(t, IsOk()); EXPECT_TRUE(t->IsMap()); EXPECT_TRUE(t->GetMap().key().IsString()); @@ -131,7 +132,7 @@ TEST(TypeSpecResolverTest, FunctionTypeConversion) { args.push_back(TypeSpec(PrimitiveType::kString)); TypeSpec spec(FunctionTypeSpec(std::move(result), std::move(args))); auto t = - ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool()); + ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena()); ASSERT_THAT(t, IsOk()); EXPECT_TRUE(t->IsFunction()); EXPECT_EQ(t->GetFunction().args().size(), 1); @@ -143,7 +144,7 @@ TEST(TypeSpecResolverTest, FunctionTypeConversion) { TEST(TypeSpecResolverTest, TypeParamConversion) { TypeSpec spec(ParamTypeSpec("T")); auto t = - ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool()); + ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena()); ASSERT_THAT(t, IsOk()); EXPECT_TRUE(t->IsTypeParam()); EXPECT_EQ(t->GetTypeParam().name(), "T"); @@ -155,7 +156,7 @@ TEST(TypeSpecResolverTest, MessageTypeConversion) { TypeSpec spec( AbstractType("cel.expr.conformance.proto3.TestAllTypes", /*params=*/{})); auto t = - ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool()); + ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena()); ASSERT_THAT(t, IsOk()); EXPECT_TRUE(t->IsMessage()); EXPECT_EQ(t->name(), "cel.expr.conformance.proto3.TestAllTypes"); @@ -171,7 +172,7 @@ TEST(TypeSpecResolverTest, MessageTypeWithParamsError) { TypeSpec spec(AbstractType("cel.expr.conformance.proto3.TestAllTypes", std::move(params))); auto t = - ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool()); + ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena()); EXPECT_THAT(t, StatusIs(absl::StatusCode::kInvalidArgument, HasSubstr("cannot have type parameters"))); } @@ -181,7 +182,7 @@ TEST(TypeSpecResolverTest, UnresolvedAbstractTypeFallbackToOpaque) { params.push_back(TypeSpec(PrimitiveType::kInt64)); TypeSpec spec(AbstractType("my.custom.OpaqueType", std::move(params))); auto t = - ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool()); + ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena()); ASSERT_THAT(t, IsOk()); EXPECT_TRUE(t->IsOpaque()); EXPECT_EQ(t->name(), "my.custom.OpaqueType"); @@ -196,7 +197,7 @@ TEST(TypeSpecResolverTest, OptionalType) { params.push_back(TypeSpec(PrimitiveType::kInt64)); TypeSpec spec(AbstractType("optional_type", std::move(params))); auto t = - ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool()); + ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena()); ASSERT_THAT(t, IsOk()); EXPECT_TRUE(t->IsOpaque()); EXPECT_EQ(t->name(), "optional_type"); @@ -211,7 +212,7 @@ TEST(TypeSpecResolverTest, TypeTypeConversion) { auto nested = std::make_unique(PrimitiveType::kInt64); TypeSpec spec(std::move(nested)); auto t = - ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool()); + ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena()); ASSERT_THAT(t, IsOk()); EXPECT_TRUE(t->IsType()); EXPECT_TRUE(t->GetType().GetType().IsInt()); @@ -222,7 +223,7 @@ TEST(TypeSpecResolverTest, TypeTypeConversion) { TEST(TypeSpecResolverTest, ErrorTypeConversion) { TypeSpec spec(ErrorTypeSpec::kValue); auto t = - ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool()); + ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena()); ASSERT_THAT(t, IsOk()); EXPECT_TRUE(t->IsError()); ASSERT_OK_AND_ASSIGN(auto spec2, ConvertTypeToTypeSpec(*t)); @@ -232,7 +233,7 @@ TEST(TypeSpecResolverTest, ErrorTypeConversion) { TEST(TypeSpecResolverTest, MessageTypeSpecConversion) { TypeSpec spec(MessageTypeSpec("cel.expr.conformance.proto3.TestAllTypes")); auto t = - ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool()); + ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena()); ASSERT_THAT(t, IsOk()); EXPECT_TRUE(t->IsMessage()); EXPECT_EQ(t->name(), "cel.expr.conformance.proto3.TestAllTypes"); @@ -243,7 +244,7 @@ TEST(TypeSpecResolverTest, MessageTypeSpecConversion) { TEST(TypeSpecResolverTest, MessageTypeSpecNotFoundError) { TypeSpec spec(MessageTypeSpec("cel.expr.conformance.proto3.NonExistentType")); auto t = - ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool()); + ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena()); EXPECT_THAT(t, StatusIs(absl::StatusCode::kInvalidArgument, HasSubstr("not found in descriptor pool"))); } @@ -252,7 +253,7 @@ TEST(TypeSpecResolverTest, EnumTypeConversion) { TypeSpec spec(AbstractType( "cel.expr.conformance.proto3.TestAllTypes.NestedEnum", /*params=*/{})); auto t = - ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool()); + ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena()); ASSERT_THAT(t, IsOk()); EXPECT_TRUE(t->IsEnum()); EXPECT_EQ(t->name(), "cel.expr.conformance.proto3.TestAllTypes.NestedEnum"); @@ -267,15 +268,42 @@ TEST(TypeSpecResolverTest, EnumTypeWithParamsError) { AbstractType("cel.expr.conformance.proto3.TestAllTypes.NestedEnum", std::move(params))); auto t = - ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool()); + ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena()); EXPECT_THAT(t, StatusIs(absl::StatusCode::kInvalidArgument, HasSubstr("cannot have type parameters"))); } +TEST(TypeSpecResolverTest, WellKnownMessageTypeFiltering) { + TypeSpec message_spec(MessageTypeSpec("google.protobuf.Timestamp")); + ASSERT_OK_AND_ASSIGN( + auto message_type, + ConvertTypeSpecToType(message_spec, *GetTestingDescriptorPool(), + GetTestArena())); + EXPECT_TRUE(message_type.IsTimestamp()); + + TypeSpec abstract_spec( + AbstractType("google.protobuf.Duration", /*params=*/{})); + ASSERT_OK_AND_ASSIGN( + auto abstract_type, + ConvertTypeSpecToType(abstract_spec, *GetTestingDescriptorPool(), + GetTestArena())); + EXPECT_TRUE(abstract_type.IsDuration()); + + std::vector params; + params.push_back(TypeSpec(PrimitiveType::kInt64)); + TypeSpec abstract_with_params( + AbstractType("google.protobuf.Timestamp", std::move(params))); + EXPECT_THAT( + ConvertTypeSpecToType(abstract_with_params, *GetTestingDescriptorPool(), + GetTestArena()), + StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("cannot have type parameters"))); +} + TEST(TypeSpecResolverTest, UnknownTypeSpecKindError) { TypeSpec spec; auto t = - ConvertTypeSpecToType(spec, GetTestArena(), *GetTestingDescriptorPool()); + ConvertTypeSpecToType(spec, *GetTestingDescriptorPool(), GetTestArena()); EXPECT_THAT(t, StatusIs(absl::StatusCode::kInvalidArgument, HasSubstr("Unknown TypeSpec kind"))); } diff --git a/common/types/message_type.h b/common/types/message_type.h index 782af87aa..eea811971 100644 --- a/common/types/message_type.h +++ b/common/types/message_type.h @@ -87,6 +87,11 @@ class MessageType final { return descriptor_; } + const google::protobuf::Descriptor* absl_nonnull descriptor() const { + ABSL_DCHECK(*this); + return descriptor_; + } + explicit operator bool() const { return descriptor_ != nullptr; } private: @@ -166,8 +171,12 @@ class MessageTypeField final { return *descriptor_; } - const google::protobuf::FieldDescriptor* absl_nonnull operator->() const - ABSL_ATTRIBUTE_LIFETIME_BOUND { + const google::protobuf::FieldDescriptor* absl_nonnull operator->() const { + ABSL_DCHECK(*this); + return descriptor_; + } + + const google::protobuf::FieldDescriptor* absl_nonnull descriptor() const { ABSL_DCHECK(*this); return descriptor_; } diff --git a/common/value.cc b/common/value.cc index f13ee958e..6284626da 100644 --- a/common/value.cc +++ b/common/value.cc @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include diff --git a/common/values/parsed_message_value.h b/common/values/parsed_message_value.h index f3d1f7b40..2e356d3e8 100644 --- a/common/values/parsed_message_value.h +++ b/common/values/parsed_message_value.h @@ -99,10 +99,8 @@ class ParsedMessageValue final return *value_; } - const google::protobuf::Message* absl_nonnull operator->() const - ABSL_ATTRIBUTE_LIFETIME_BOUND { - return value_; - } + const google::protobuf::Message* absl_nonnull operator->() const { return value_; } + const google::protobuf::Message* absl_nonnull message() const { return value_; } bool IsZeroValue() const; @@ -175,6 +173,15 @@ class ParsedMessageValue final swap(lhs.arena_, rhs.arena_); } + absl::Status GetField( + const google::protobuf::FieldDescriptor* absl_nonnull field, + ProtoWrapperTypeOptions unboxing_options, + const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, + google::protobuf::MessageFactory* absl_nonnull message_factory, + google::protobuf::Arena* absl_nonnull arena, Value* absl_nonnull result) const; + + bool HasField(const google::protobuf::FieldDescriptor* absl_nonnull field) const; + private: friend std::pointer_traits; friend class StructValue; @@ -203,15 +210,6 @@ class ParsedMessageValue final return absl::OkStatus(); } - absl::Status GetField( - const google::protobuf::FieldDescriptor* absl_nonnull field, - ProtoWrapperTypeOptions unboxing_options, - const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, - google::protobuf::MessageFactory* absl_nonnull message_factory, - google::protobuf::Arena* absl_nonnull arena, Value* absl_nonnull result) const; - - bool HasField(const google::protobuf::FieldDescriptor* absl_nonnull field) const; - const google::protobuf::Message* absl_nonnull value_; // Arena that is attributed as owning the value. May be null to indicate that // the value is managed externally. diff --git a/eval/compiler/BUILD b/eval/compiler/BUILD index ea4dd46b3..2012abeda 100644 --- a/eval/compiler/BUILD +++ b/eval/compiler/BUILD @@ -109,6 +109,7 @@ cc_library( "//common:expr", "//common:kind", "//common:type", + "//common:type_spec_resolver", "//common:value", "//eval/eval:comprehension_step", "//eval/eval:const_value_step", diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index fc7289bd2..4e57bd149 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -30,8 +31,6 @@ #include #include "absl/algorithm/container.h" -#include "absl/base/attributes.h" -#include "absl/base/optimization.h" #include "absl/container/flat_hash_map.h" #include "absl/container/flat_hash_set.h" #include "absl/container/node_hash_map.h" @@ -45,7 +44,6 @@ #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "absl/strings/strip.h" -#include "absl/types/optional.h" #include "absl/types/span.h" #include "absl/types/variant.h" #include "base/ast.h" @@ -59,6 +57,7 @@ #include "common/expr.h" #include "common/kind.h" #include "common/type.h" +#include "common/type_spec_resolver.h" #include "common/value.h" #include "eval/compiler/check_ast_extensions.h" #include "eval/compiler/flat_expr_builder_extensions.h" @@ -529,6 +528,7 @@ class FlatExprVisitor : public cel::AstVisitor { const Resolver& resolver, const cel::RuntimeOptions& options, std::vector> program_optimizers, const absl::flat_hash_map& reference_map, + const absl::flat_hash_map& type_map, const cel::TypeProvider& type_provider, IssueCollector& issue_collector, ProgramBuilder& program_builder, PlannerContext& extension_context, bool enable_optional_types) @@ -538,6 +538,8 @@ class FlatExprVisitor : public cel::AstVisitor { resolved_select_expr_(nullptr), options_(options), program_optimizers_(std::move(program_optimizers)), + reference_map_(reference_map), + type_map_(type_map), issue_collector_(issue_collector), program_builder_(program_builder), extension_context_(extension_context), @@ -606,6 +608,21 @@ class FlatExprVisitor : public cel::AstVisitor { bool PlanRecursiveProgram() const { return max_recursion_depth_ > 0; } + void SetResolvedType(const cel::Expr& expr, cel::Type type) { + resolved_types_[&expr] = std::move(type); + } + + std::optional GetResolvedType(const cel::Expr* expr) const { + if (expr == nullptr) { + return std::nullopt; + } + auto it = resolved_types_.find(expr); + if (it != resolved_types_.end()) { + return it->second; + } + return std::nullopt; + } + void PreVisitExpr(const cel::Expr& expr) override { ValidateOrError(!absl::holds_alternative(expr.kind()), "Invalid empty expression"); @@ -617,6 +634,10 @@ class FlatExprVisitor : public cel::AstVisitor { resume_from_suppressed_branch_ = &expr; } + if (options_.enable_typed_field_access) { + MaybeResolveType(expr); + } + if (block_.has_value()) { BlockInfo& block = *block_; if (block.in && block.bindings_set.contains(&expr)) { @@ -976,6 +997,25 @@ class FlatExprVisitor : public cel::AstVisitor { return; } + StringValue field = cel::StringValue(select_expr.field()); + std::optional struct_type; + std::optional field_type; + if (options_.enable_typed_field_access) { + std::optional operand_type = + GetResolvedType(&select_expr.operand()); + if (operand_type.has_value() && operand_type->IsStruct()) { + struct_type = operand_type->GetStruct(); + if (struct_type.has_value()) { + auto field_lookup = + extension_context_.type_reflector().FindStructTypeFieldByName( + *struct_type, select_expr.field()); + // Swallow error to fallback to duck typing behavior. + if (field_lookup.ok() && field_lookup->has_value()) { + field_type = *std::move(field_lookup); + } + } + } + } if (auto depth = RecursionEligible(); depth.has_value()) { auto deps = ExtractRecursiveDependencies(); if (deps.size() != 1) { @@ -983,7 +1023,6 @@ class FlatExprVisitor : public cel::AstVisitor { "unexpected number of dependencies for select operation.")); return; } - StringValue field = cel::StringValue(select_expr.field()); SetRecursiveStep( CreateDirectSelectStep(std::move(deps[0]), std::move(field), @@ -994,9 +1033,16 @@ class FlatExprVisitor : public cel::AstVisitor { return; } - AddStep(CreateSelectStep(select_expr, expr.id(), - options_.enable_empty_wrapper_null_unboxing, - enable_optional_types_)); + if (field_type.has_value()) { + AddStep(CreateTypedSelectStep( + std::move(field), *struct_type, *std::move(field_type), + select_expr.test_only(), expr.id(), + options_.enable_empty_wrapper_null_unboxing, enable_optional_types_)); + return; + } + AddStep(CreateSelectStep( + std::move(field), select_expr.test_only(), expr.id(), + options_.enable_empty_wrapper_null_unboxing, enable_optional_types_)); } // Call node handler group. @@ -1921,6 +1967,8 @@ class FlatExprVisitor : public cel::AstVisitor { CallHandlerResult HandleHeterogeneousEqualityIn(const cel::Expr& expr, const cel::CallExpr& call); + void MaybeResolveType(const cel::Expr& expr); + const Resolver& resolver_; const cel::TypeProvider& type_provider_; absl::Status progress_status_; @@ -1942,6 +1990,9 @@ class FlatExprVisitor : public cel::AstVisitor { absl::flat_hash_set suppressed_branches_; const cel::Expr* resume_from_suppressed_branch_ = nullptr; std::vector> program_optimizers_; + const absl::flat_hash_map& reference_map_; + const absl::flat_hash_map& type_map_; + absl::flat_hash_map resolved_types_; IssueCollector& issue_collector_; ProgramBuilder& program_builder_; @@ -2161,6 +2212,23 @@ FlatExprVisitor::HandleHeterogeneousEqualityIn(const cel::Expr& expr, return CallHandlerResult::kIntercepted; } +void FlatExprVisitor::MaybeResolveType(const cel::Expr& expr) { + // Try to resolve the type from the type map, but don't fail if it's not + // there. This permits cases where the activation type is compatible but not + // the same as the type checked type. + auto it = type_map_.find(expr.id()); + if (it == type_map_.end()) { + return; + } + absl::StatusOr type = cel::ConvertTypeSpecToType( + it->second, extension_context_.type_reflector(), + extension_context_.MutableArena()); + if (!type.ok()) { + return; + } + SetResolvedType(expr, *type); +} + void LogicalCondVisitor::PreVisit(const cel::Expr* expr) { visitor_->ValidateOrError( !expr->call_expr().has_target() && expr->call_expr().args().size() >= 2, @@ -2561,9 +2629,9 @@ absl::StatusOr FlatExprBuilder::CreateExpressionImpl( // These objects are expected to remain scoped to one build call -- references // to them shouldn't be persisted in any part of the result expression. FlatExprVisitor visitor(resolver, options_, std::move(optimizers), - ast->reference_map(), GetTypeProvider(), - issue_collector, program_builder, extension_context, - enable_optional_types_); + ast->reference_map(), ast->type_map(), + GetTypeProvider(), issue_collector, program_builder, + extension_context, enable_optional_types_); if (options_.max_recursion_depth == -1 || options_.max_recursion_depth > 0) { int depth_limit = options_.max_recursion_depth == -1 diff --git a/eval/eval/BUILD b/eval/eval/BUILD index c0f544405..58538a361 100644 --- a/eval/eval/BUILD +++ b/eval/eval/BUILD @@ -314,13 +314,11 @@ cc_library( ":direct_expression_step", ":evaluator_core", ":expression_step_base", - "//common:expr", + "//common:type", "//common:value", "//common:value_kind", - "//eval/internal:errors", "//internal:status_macros", "//runtime:runtime_options", - "@com_google_absl//absl/base:nullability", "@com_google_absl//absl/log:absl_check", "@com_google_absl//absl/log:absl_log", "@com_google_absl//absl/status", diff --git a/eval/eval/attribute_trail.h b/eval/eval/attribute_trail.h index 576d0be34..838cc8875 100644 --- a/eval/eval/attribute_trail.h +++ b/eval/eval/attribute_trail.h @@ -47,6 +47,7 @@ class AttributeTrail { // Creates AttributeTrail with attribute path incremented by "qualifier". AttributeTrail Step(const std::string* qualifier) const { + if (empty()) return AttributeTrail(); return Step(cel::AttributeQualifier::OfString(*qualifier)); } diff --git a/eval/eval/select_step.cc b/eval/eval/select_step.cc index b815f5d87..bd97fd02b 100644 --- a/eval/eval/select_step.cc +++ b/eval/eval/select_step.cc @@ -5,14 +5,13 @@ #include #include -#include "absl/base/nullability.h" #include "absl/log/absl_check.h" #include "absl/log/absl_log.h" #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" -#include "common/expr.h" +#include "common/type.h" #include "common/value.h" #include "common/value_kind.h" #include "eval/eval/attribute_trail.h" @@ -32,7 +31,6 @@ namespace { using ::cel::BoolValue; using ::cel::ErrorValue; using ::cel::MapValue; -using ::cel::NullValue; using ::cel::OptionalValue; using ::cel::ProtoWrapperTypeOptions; using ::cel::StringValue; @@ -75,35 +73,97 @@ absl::optional CheckForMarkedAttributes(const AttributeTrail& trail, return std::nullopt; } -void TestOnlySelect(const StructValue& msg, const std::string& field, - const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, - google::protobuf::MessageFactory* absl_nonnull message_factory, - google::protobuf::Arena* absl_nonnull arena, - Value* absl_nonnull result) { - absl::StatusOr has_field = msg.HasFieldByName(field); +absl::Status PerformHas(const Value& target, absl::string_view field, + const StringValue& field_value, + const google::protobuf::DescriptorPool* descriptor_pool, + google::protobuf::MessageFactory* message_factory, + google::protobuf::Arena* arena, Value& result) { + switch (target.kind()) { + case ValueKind::kMap: { + CEL_RETURN_IF_ERROR(target.GetMap().Has(field_value, descriptor_pool, + message_factory, arena, &result)); + return absl::OkStatus(); + } + case ValueKind::kStruct: { + auto has_field = target.GetStruct().HasFieldByName(field); + if (!has_field.ok()) { + result = ErrorValue(std::move(has_field).status()); + } else { + result = BoolValue{*has_field}; + } + return absl::OkStatus(); + } + default: + return InvalidSelectTargetError(); + } +} - if (!has_field.ok()) { - *result = ErrorValue(std::move(has_field).status()); - return; +absl::Status PerformGet(const Value& target, absl::string_view field, + const StringValue& field_value, + ProtoWrapperTypeOptions unboxing_option, + const google::protobuf::DescriptorPool* descriptor_pool, + google::protobuf::MessageFactory* message_factory, + google::protobuf::Arena* arena, Value& result) { + switch (target.kind()) { + case ValueKind::kMap: { + auto status = target.GetMap().Get(field_value, descriptor_pool, + message_factory, arena, &result); + if (!status.ok()) { + result = ErrorValue(std::move(status)); + } + return absl::OkStatus(); + } + case ValueKind::kStruct: { + auto status = target.GetStruct().GetFieldByName( + field, unboxing_option, descriptor_pool, message_factory, arena, + &result); + if (!status.ok()) { + result = ErrorValue(std::move(status)); + } + return absl::OkStatus(); + } + default: + return InvalidSelectTargetError(); } - *result = BoolValue{*has_field}; } -void TestOnlySelect(const MapValue& map, const StringValue& field_name, - const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, - google::protobuf::MessageFactory* absl_nonnull message_factory, - google::protobuf::Arena* absl_nonnull arena, - Value* absl_nonnull result) { - // Field presence only supports string keys containing valid identifier - // characters. - absl::Status presence = - map.Has(field_name, descriptor_pool, message_factory, arena, result); - - if (!presence.ok()) { - *result = ErrorValue(std::move(presence)); - return; +absl::Status PerformOptionalGet(const Value& target, absl::string_view field, + const StringValue& field_value, + ProtoWrapperTypeOptions unboxing_option, + const google::protobuf::DescriptorPool* descriptor_pool, + google::protobuf::MessageFactory* message_factory, + google::protobuf::Arena* arena, Value& result) { + switch (target.kind()) { + case ValueKind::kMap: { + CEL_ASSIGN_OR_RETURN( + bool found, target.GetMap().Find(field_value, descriptor_pool, + message_factory, arena, &result)); + if (!found) { + result = OptionalValue::None(); + return absl::OkStatus(); + } + ABSL_DCHECK(!result.IsUnknown()); + result = OptionalValue::Of(std::move(result), arena); + return absl::OkStatus(); + } + case ValueKind::kStruct: { + CEL_ASSIGN_OR_RETURN(bool found, + target.GetStruct().HasFieldByName(field)); + if (!found) { + result = OptionalValue::None(); + return absl::OkStatus(); + } + CEL_RETURN_IF_ERROR(target.GetStruct().GetFieldByName( + field, unboxing_option, descriptor_pool, message_factory, arena, + &result)); + + ABSL_DCHECK(!result.IsUnknown()); + result = OptionalValue::Of(std::move(result), arena); + return absl::OkStatus(); + } + default: + return InvalidSelectTargetError(); } - ABSL_DCHECK(!result->IsUnknown()); } // SelectStep performs message field access specified by Expr::Select @@ -123,12 +183,7 @@ class SelectStep : public ExpressionStepBase { absl::Status Evaluate(ExecutionFrame* frame) const override; - private: - absl::Status PerformTestOnlySelect(ExecutionFrame* frame, - const Value& arg) const; - absl::StatusOr PerformSelect(ExecutionFrame* frame, const Value& arg, - Value& result) const; - + protected: cel::StringValue field_value_; std::string field_; bool test_field_presence_; @@ -153,7 +208,7 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { AttributeTrail result_trail; // Handle unknown resolution. - if (frame->enable_unknowns() || frame->enable_missing_attribute_errors()) { + if (frame->attribute_tracking_enabled()) { result_trail = trail.Step(&field_); } @@ -177,128 +232,48 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { return absl::OkStatus(); } - // Handle test only Select. + Value result; if (test_field_presence_) { + const Value* target = &arg; if (optional_arg) { if (!optional_arg->HasValue()) { - frame->value_stack().PopAndPush(cel::BoolValue{false}); + frame->value_stack().PopAndPush(cel::BoolValue{false}, + std::move(result_trail)); return absl::OkStatus(); } - Value value; - optional_arg->Value(&value); - return PerformTestOnlySelect(frame, value); + optional_arg->Value(&result); + target = &result; } - return PerformTestOnlySelect(frame, arg); + CEL_RETURN_IF_ERROR( + PerformHas(*target, field_, field_value_, frame->descriptor_pool(), + frame->message_factory(), frame->arena(), result)); + frame->value_stack().PopAndPush(std::move(result), std::move(result_trail)); + return absl::OkStatus(); } - // Normal select path. - // Select steps can be applied to either maps or messages if (optional_arg) { if (!optional_arg->HasValue()) { - // Leave optional_arg at the top of the stack. Its empty. + frame->value_stack().PopAndPush(OptionalValue::None(), + std::move(result_trail)); return absl::OkStatus(); } Value value; - Value result; - bool ok; optional_arg->Value(&value); - CEL_ASSIGN_OR_RETURN(ok, PerformSelect(frame, value, result)); - if (!ok) { - frame->value_stack().PopAndPush(cel::OptionalValue::None(), - std::move(result_trail)); - return absl::OkStatus(); + auto status = PerformOptionalGet( + value, field_, field_value_, unboxing_option_, frame->descriptor_pool(), + frame->message_factory(), frame->arena(), result); + if (!status.ok()) { + result = ErrorValue(std::move(status)); } - frame->value_stack().PopAndPush( - cel::OptionalValue::Of(std::move(result), frame->arena()), - std::move(result_trail)); + frame->value_stack().PopAndPush(std::move(result), std::move(result_trail)); return absl::OkStatus(); } - // Normal select path. - // Select steps can be applied to either maps or messages - switch (arg.kind()) { - case ValueKind::kStruct: { - Value result; - auto status = arg.GetStruct().GetFieldByName( - field_, unboxing_option_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), &result); - if (!status.ok()) { - result = ErrorValue(std::move(status)); - } - frame->value_stack().PopAndPush(std::move(result), - std::move(result_trail)); - return absl::OkStatus(); - } - case ValueKind::kMap: { - Value result; - auto status = - arg.GetMap().Get(field_value_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), &result); - if (!status.ok()) { - result = ErrorValue(std::move(status)); - } - frame->value_stack().PopAndPush(std::move(result), - std::move(result_trail)); - return absl::OkStatus(); - } - default: - // Control flow should have returned earlier. - return InvalidSelectTargetError(); - } -} - -absl::Status SelectStep::PerformTestOnlySelect(ExecutionFrame* frame, - const Value& arg) const { - switch (arg.kind()) { - case ValueKind::kMap: { - Value result; - TestOnlySelect(arg.GetMap(), field_value_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), &result); - frame->value_stack().PopAndPush(std::move(result)); - return absl::OkStatus(); - } - case ValueKind::kMessage: { - Value result; - TestOnlySelect(arg.GetStruct(), field_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), &result); - frame->value_stack().PopAndPush(std::move(result)); - return absl::OkStatus(); - } - default: - // Control flow should have returned earlier. - return InvalidSelectTargetError(); - } -} - -absl::StatusOr SelectStep::PerformSelect(ExecutionFrame* frame, - const Value& arg, - Value& result) const { - switch (arg->kind()) { - case ValueKind::kStruct: { - const auto& struct_value = arg.GetStruct(); - CEL_ASSIGN_OR_RETURN(auto ok, struct_value.HasFieldByName(field_)); - if (!ok) { - result = NullValue{}; - return false; - } - CEL_RETURN_IF_ERROR(struct_value.GetFieldByName( - field_, unboxing_option_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), &result)); - ABSL_DCHECK(!result.IsUnknown()); - return true; - } - case ValueKind::kMap: { - CEL_ASSIGN_OR_RETURN( - auto found, - arg.GetMap().Find(field_value_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), &result)); - ABSL_DCHECK(!found || !result.IsUnknown()); - return found; - } - default: - // Control flow should have returned earlier. - return InvalidSelectTargetError(); - } + CEL_RETURN_IF_ERROR(PerformGet( + arg, field_, field_value_, unboxing_option_, frame->descriptor_pool(), + frame->message_factory(), frame->arena(), result)); + frame->value_stack().PopAndPush(std::move(result), std::move(result_trail)); + return absl::OkStatus(); } class DirectSelectStep : public DirectExpressionStep { @@ -362,11 +337,11 @@ class DirectSelectStep : public DirectExpressionStep { } Value value; optional_arg->Value(&value); - PerformTestOnlySelect(frame, value, result); - return absl::OkStatus(); + return PerformHas(value, field_, field_value_, frame.descriptor_pool(), + frame.message_factory(), frame.arena(), result); } - PerformTestOnlySelect(frame, result, result); - return absl::OkStatus(); + return PerformHas(result, field_, field_value_, frame.descriptor_pool(), + frame.message_factory(), frame.arena(), result); } if (optional_arg) { @@ -376,26 +351,24 @@ class DirectSelectStep : public DirectExpressionStep { } Value value; optional_arg->Value(&value); - return PerformOptionalSelect(frame, value, result); + auto status = + PerformOptionalGet(value, field_, field_value_, unboxing_option_, + frame.descriptor_pool(), frame.message_factory(), + frame.arena(), result); + if (!status.ok()) { + result = ErrorValue(std::move(status)); + } + return absl::OkStatus(); } - auto status = PerformSelect(frame, result, result); - if (!status.ok()) { - result = ErrorValue(std::move(status)); - } - return absl::OkStatus(); + return PerformGet(result, field_, field_value_, unboxing_option_, + frame.descriptor_pool(), frame.message_factory(), + frame.arena(), result); } private: std::unique_ptr operand_; - void PerformTestOnlySelect(ExecutionFrameBase& frame, const Value& value, - Value& result) const; - absl::Status PerformOptionalSelect(ExecutionFrameBase& frame, - const Value& value, Value& result) const; - absl::Status PerformSelect(ExecutionFrameBase& frame, const Value& value, - Value& result) const; - // Field name in formats supported by each of the map and struct field access // APIs. // @@ -410,83 +383,59 @@ class DirectSelectStep : public DirectExpressionStep { bool enable_optional_types_; }; -void DirectSelectStep::PerformTestOnlySelect(ExecutionFrameBase& frame, - const cel::Value& value, - Value& result) const { - switch (value.kind()) { - case ValueKind::kMap: - TestOnlySelect(value.GetMap(), field_value_, frame.descriptor_pool(), - frame.message_factory(), frame.arena(), &result); - return; - case ValueKind::kMessage: - TestOnlySelect(value.GetStruct(), field_, frame.descriptor_pool(), - frame.message_factory(), frame.arena(), &result); - return; - default: - // Control flow should have returned earlier. - result = cel::ErrorValue(InvalidSelectTargetError()); - return; +class ProtoSelectStep : public SelectStep { + public: + ProtoSelectStep(StringValue value, int64_t expr_id, + bool enable_wrapper_type_null_unboxing, + bool enable_optional_types, + const google::protobuf::Descriptor* descriptor, + const google::protobuf::FieldDescriptor* field_descriptor) + : SelectStep(std::move(value), /*test_field_presence=*/false, expr_id, + enable_wrapper_type_null_unboxing, enable_optional_types), + descriptor_(descriptor), + field_descriptor_(field_descriptor) { + ABSL_DCHECK(descriptor_ != nullptr); + ABSL_DCHECK(field_descriptor_ != nullptr); } -} -absl::Status DirectSelectStep::PerformOptionalSelect(ExecutionFrameBase& frame, - const Value& value, - Value& result) const { - switch (value.kind()) { - case ValueKind::kStruct: { - auto struct_value = value.GetStruct(); - CEL_ASSIGN_OR_RETURN(auto ok, struct_value.HasFieldByName(field_)); - if (!ok) { - result = OptionalValue::None(); - return absl::OkStatus(); - } - CEL_RETURN_IF_ERROR(struct_value.GetFieldByName( - field_, unboxing_option_, frame.descriptor_pool(), - frame.message_factory(), frame.arena(), &result)); - ABSL_DCHECK(!result.IsUnknown()); - result = OptionalValue::Of(std::move(result), frame.arena()); - return absl::OkStatus(); + absl::Status Evaluate(ExecutionFrame* frame) const override { + if (!frame->value_stack().HasEnough(1)) { + return absl::InternalError( + "No arguments supplied for Select-type expression"); } - case ValueKind::kMap: { - CEL_ASSIGN_OR_RETURN( - auto found, - value.GetMap().Find(field_value_, frame.descriptor_pool(), - frame.message_factory(), frame.arena(), &result)); - if (!found) { - result = OptionalValue::None(); + + const Value& arg = frame->value_stack().Peek(); + auto parsed_message = arg.AsParsedMessage(); + if (!parsed_message.has_value() || + parsed_message->GetDescriptor() != descriptor_) { + // fallback to generic implementation + return SelectStep::Evaluate(frame); + } + + AttributeTrail result_trail; + if (frame->attribute_tracking_enabled()) { + result_trail = frame->value_stack().PeekAttribute().Step(&field_); + absl::optional marked_attribute_check = + CheckForMarkedAttributes(result_trail, *frame); + if (marked_attribute_check.has_value()) { + frame->value_stack().PopAndPush( + std::move(marked_attribute_check).value(), std::move(result_trail)); return absl::OkStatus(); } - ABSL_DCHECK(!result.IsUnknown()); - result = OptionalValue::Of(std::move(result), frame.arena()); - return absl::OkStatus(); } - default: - // Control flow should have returned earlier. - return InvalidSelectTargetError(); - } -} -absl::Status DirectSelectStep::PerformSelect(ExecutionFrameBase& frame, - const cel::Value& value, - Value& result) const { - switch (value.kind()) { - case ValueKind::kStruct: - CEL_RETURN_IF_ERROR(value.GetStruct().GetFieldByName( - field_, unboxing_option_, frame.descriptor_pool(), - frame.message_factory(), frame.arena(), &result)); - ABSL_DCHECK(!result.IsUnknown()); - return absl::OkStatus(); - case ValueKind::kMap: - CEL_RETURN_IF_ERROR( - value.GetMap().Get(field_value_, frame.descriptor_pool(), - frame.message_factory(), frame.arena(), &result)); - ABSL_DCHECK(!result.IsUnknown()); - return absl::OkStatus(); - default: - // Control flow should have returned earlier. - return InvalidSelectTargetError(); + CEL_RETURN_IF_ERROR(parsed_message->GetField( + field_descriptor_, unboxing_option_, frame->descriptor_pool(), + frame->message_factory(), frame->arena(), + &frame->value_stack().Peek())); + frame->value_stack().PeekAttribute() = std::move(result_trail); + return absl::OkStatus(); } -} + + private: + const google::protobuf::Descriptor* descriptor_; + const google::protobuf::FieldDescriptor* field_descriptor_; +}; } // namespace @@ -501,11 +450,36 @@ std::unique_ptr CreateDirectSelectStep( // Factory method for Select - based Execution step absl::StatusOr> CreateSelectStep( - const cel::SelectExpr& select_expr, int64_t expr_id, + cel::StringValue field, bool test_only, int64_t expr_id, bool enable_wrapper_type_null_unboxing, bool enable_optional_types) { - return std::make_unique( - cel::StringValue(select_expr.field()), select_expr.test_only(), expr_id, - enable_wrapper_type_null_unboxing, enable_optional_types); + return std::make_unique(std::move(field), test_only, expr_id, + enable_wrapper_type_null_unboxing, + enable_optional_types); +} + +// Factory method for Select - based Execution step +absl::StatusOr> CreateTypedSelectStep( + cel::StringValue field, cel::StructType resolved_operand_type, + cel::StructTypeField resolved_field, bool test_only, int64_t expr_id, + bool enable_wrapper_type_null_unboxing, bool enable_optional_types) { + if (!resolved_operand_type.IsMessage() || test_only) { + // The specialization only supports messages. Fallback to the generic + // implementation for other types. + // TODO(uncreated-issue/89): support has() for messages. + return CreateSelectStep(std::move(field), test_only, expr_id, + enable_wrapper_type_null_unboxing, + enable_optional_types); + } + const google::protobuf::Descriptor* descriptor = + resolved_operand_type.GetMessage().descriptor(); + + ABSL_DCHECK(resolved_field.IsMessage()); + const google::protobuf::FieldDescriptor* field_descriptor = + resolved_field.GetMessage().descriptor(); + + return std::make_unique( + std::move(field), expr_id, enable_wrapper_type_null_unboxing, + enable_optional_types, descriptor, field_descriptor); } } // namespace google::api::expr::runtime diff --git a/eval/eval/select_step.h b/eval/eval/select_step.h index 6eaaf9487..c3f965a94 100644 --- a/eval/eval/select_step.h +++ b/eval/eval/select_step.h @@ -5,7 +5,7 @@ #include #include "absl/status/statusor.h" -#include "common/expr.h" +#include "common/type.h" #include "common/value.h" #include "eval/eval/direct_expression_step.h" #include "eval/eval/evaluator_core.h" @@ -18,10 +18,15 @@ std::unique_ptr CreateDirectSelectStep( bool test_only, int64_t expr_id, bool enable_wrapper_type_null_unboxing, bool enable_optional_types = false); -// Factory method for Select - based Execution step +// Factory method for Select stack machine based Execution step absl::StatusOr> CreateSelectStep( - const cel::SelectExpr& select_expr, int64_t expr_id, - bool enable_wrapper_type_null_unboxing, bool enable_optional_types = false); + cel::StringValue field, bool test_only, int64_t expr_id, + bool enable_wrapper_type_null_unboxing, bool enable_optional_ytpes = false); + +absl::StatusOr> CreateTypedSelectStep( + cel::StringValue field, cel::StructType resolved_operand_type, + cel::StructTypeField resolved_field, bool test_only, int64_t expr_id, + bool enable_wrapper_type_null_unboxing, bool enable_optional_types); } // namespace google::api::expr::runtime diff --git a/eval/eval/select_step_test.cc b/eval/eval/select_step_test.cc index ce532eabd..98c478ab5 100644 --- a/eval/eval/select_step_test.cc +++ b/eval/eval/select_step_test.cc @@ -133,8 +133,8 @@ class SelectStepTest : public testing::Test { CEL_ASSIGN_OR_RETURN(auto step0, CreateIdentStep(ident.name(), expr0.id())); CEL_ASSIGN_OR_RETURN( auto step1, - CreateSelectStep(select, expr.id(), - options.enable_wrapper_type_null_unboxing)); + CreateSelectStep(cel::StringValue(select.field()), select.test_only(), + expr.id(), options.enable_wrapper_type_null_unboxing)); path.push_back(std::move(step0)); path.push_back(std::move(step1)); @@ -330,11 +330,13 @@ TEST_F(SelectStepTest, MapPresenseIsErrorTest) { ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep(ident.name(), expr0.id())); ASSERT_OK_AND_ASSIGN( auto step1, - CreateSelectStep(select_map, expr1.id(), + CreateSelectStep(cel::StringValue(select_map.field()), + select_map.test_only(), expr1.id(), /*enable_wrapper_type_null_unboxing=*/false)); ASSERT_OK_AND_ASSIGN( auto step2, - CreateSelectStep(select, select_expr.id(), + CreateSelectStep(cel::StringValue(select.field()), select.test_only(), + select_expr.id(), /*enable_wrapper_type_null_unboxing=*/false)); ExecutionPath path; @@ -836,7 +838,8 @@ TEST_P(SelectStepConformanceTest, CelErrorAsArgument) { ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep(ident.name(), expr0.id())); ASSERT_OK_AND_ASSIGN( auto step1, - CreateSelectStep(select, dummy_expr.id(), + CreateSelectStep(cel::StringValue(select.field()), select.test_only(), + dummy_expr.id(), /*enable_wrapper_type_null_unboxing=*/false)); path.push_back(std::move(step0)); @@ -877,7 +880,8 @@ TEST_F(SelectStepTest, DisableMissingAttributeOK) { ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep(ident.name(), expr0.id())); ASSERT_OK_AND_ASSIGN( auto step1, - CreateSelectStep(select, dummy_expr.id(), + CreateSelectStep(cel::StringValue(select.field()), select.test_only(), + dummy_expr.id(), /*enable_wrapper_type_null_unboxing=*/false)); path.push_back(std::move(step0)); @@ -919,7 +923,8 @@ TEST_F(SelectStepTest, UnrecoverableUnknownValueProducesError) { ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep(ident.name(), expr0.id())); ASSERT_OK_AND_ASSIGN( auto step1, - CreateSelectStep(select, dummy_expr.id(), + CreateSelectStep(cel::StringValue(select.field()), select.test_only(), + dummy_expr.id(), /*enable_wrapper_type_null_unboxing=*/false)); path.push_back(std::move(step0)); @@ -965,9 +970,9 @@ TEST_F(SelectStepTest, UnknownPatternResolvesToUnknown) { auto& ident = expr0.mutable_ident_expr(); ident.set_name("message"); auto step0_status = CreateIdentStep(ident.name(), expr0.id()); - auto step1_status = - CreateSelectStep(select, dummy_expr.id(), - /*enable_wrapper_type_null_unboxing=*/false); + auto step1_status = CreateSelectStep( + cel::StringValue(select.field()), select.test_only(), dummy_expr.id(), + /*enable_wrapper_type_null_unboxing=*/false); ASSERT_THAT(step0_status, IsOk()); ASSERT_THAT(step1_status, IsOk()); @@ -1057,6 +1062,43 @@ TEST_F(SelectStepTest, UnknownPatternResolvesToUnknown) { } } +TEST_F(SelectStepTest, TypedSelectStepTest) { + ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep("message", -1)); + + cel::StructType resolved_operand_type( + (cel::MessageType(TestAllTypes::descriptor()))); + const google::protobuf::FieldDescriptor* field_desc = + TestAllTypes::descriptor()->FindFieldByName("single_int64"); + ASSERT_NE(field_desc, nullptr); + cel::StructTypeField resolved_field((cel::MessageTypeField(field_desc))); + + ASSERT_OK_AND_ASSIGN( + auto step1, + CreateTypedSelectStep(cel::StringValue("single_int64"), + resolved_operand_type, resolved_field, + /*test_only=*/false, -1, + /*enable_wrapper_type_null_unboxing=*/false, + /*enable_optional_types=*/false)); + + ExecutionPath path; + path.push_back(std::move(step0)); + path.push_back(std::move(step1)); + CelExpressionFlatImpl cel_expr( + env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, + env_->type_registry.GetComposedTypeProvider(), + cel::RuntimeOptions{})); + + TestAllTypes message; + message.set_single_int64(42); + Activation activation; + activation.InsertValue("message", + CelProtoWrapper::CreateMessage(&message, &arena_)); + + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr.Evaluate(activation, &arena_)); + ASSERT_TRUE(result.IsInt64()); + EXPECT_EQ(result.Int64OrDie(), 42); +} + INSTANTIATE_TEST_SUITE_P(UnknownsEnabled, SelectStepConformanceTest, testing::Bool()); diff --git a/eval/public/cel_options.cc b/eval/public/cel_options.cc index 938b5e96f..ed1fde5f4 100644 --- a/eval/public/cel_options.cc +++ b/eval/public/cel_options.cc @@ -19,29 +19,32 @@ namespace google::api::expr::runtime { cel::RuntimeOptions ConvertToRuntimeOptions(const InterpreterOptions& options) { - return cel::RuntimeOptions{/*.container=*/"", - options.unknown_processing, - options.enable_missing_attribute_errors, - options.enable_timestamp_duration_overflow_errors, - options.short_circuiting, - options.enable_comprehension, - options.comprehension_max_iterations, - options.enable_comprehension_list_append, - options.enable_comprehension_mutable_map, - options.enable_regex, - options.regex_max_program_size, - options.enable_string_conversion, - options.enable_string_concat, - options.enable_list_concat, - options.enable_list_contains, - options.fail_on_warnings, - options.enable_qualified_type_identifiers, - options.enable_heterogeneous_equality, - options.enable_empty_wrapper_null_unboxing, - options.enable_lazy_bind_initialization, - options.max_recursion_depth, - options.enable_recursive_tracing, - options.enable_fast_builtins}; + return cel::RuntimeOptions{ + /*.container=*/"", + options.unknown_processing, + options.enable_missing_attribute_errors, + options.enable_timestamp_duration_overflow_errors, + options.short_circuiting, + options.enable_comprehension, + options.comprehension_max_iterations, + options.enable_comprehension_list_append, + options.enable_comprehension_mutable_map, + options.enable_regex, + options.regex_max_program_size, + options.enable_string_conversion, + options.enable_string_concat, + options.enable_list_concat, + options.enable_list_contains, + options.fail_on_warnings, + options.enable_qualified_type_identifiers, + options.enable_heterogeneous_equality, + options.enable_empty_wrapper_null_unboxing, + options.enable_lazy_bind_initialization, + options.max_recursion_depth, + options.enable_recursive_tracing, + options.enable_fast_builtins, + /*enable_typed_field_access=*/false, + }; } } // namespace google::api::expr::runtime diff --git a/eval/tests/BUILD b/eval/tests/BUILD index 9163548d1..762d3bd3e 100644 --- a/eval/tests/BUILD +++ b/eval/tests/BUILD @@ -61,22 +61,25 @@ cc_test( ], deps = [ ":request_context_cc_proto", + "//checker:validation_result", "//common:allocator", "//common:casting", + "//common:decl", "//common:legacy_value", - "//common:memory", "//common:native_type", + "//common:type", "//common:value", + "//compiler", + "//compiler:compiler_factory", + "//compiler:standard_library", "//extensions:comprehensions_v2_functions", "//extensions:comprehensions_v2_macros", "//extensions/protobuf:runtime_adapter", - "//extensions/protobuf:value", "//internal:benchmark", "//internal:testing", "//internal:testing_descriptor_pool", "//internal:testing_message_factory", "//parser", - "//parser:macro", "//parser:macro_registry", "//runtime", "//runtime:activation", @@ -94,7 +97,6 @@ cc_test( "@com_google_absl//absl/status:status_matchers", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", - "@com_google_absl//absl/types:optional", "@com_google_cel_spec//proto/cel/expr:syntax_cc_proto", "@com_google_googleapis//google/rpc/context:attribute_context_cc_proto", "@com_google_protobuf//:protobuf", diff --git a/eval/tests/modern_benchmark_test.cc b/eval/tests/modern_benchmark_test.cc index 005f93aa5..28daed981 100644 --- a/eval/tests/modern_benchmark_test.cc +++ b/eval/tests/modern_benchmark_test.cc @@ -14,7 +14,6 @@ // // General benchmarks for CEL evaluator. -#include #include #include #include @@ -36,19 +35,24 @@ #include "absl/status/status_matchers.h" #include "absl/status/statusor.h" #include "absl/strings/match.h" +#include "absl/strings/string_view.h" +#include "checker/validation_result.h" #include "common/allocator.h" #include "common/casting.h" +#include "common/decl.h" #include "common/native_type.h" +#include "common/type.h" #include "common/value.h" +#include "compiler/compiler.h" +#include "compiler/compiler_factory.h" +#include "compiler/standard_library.h" #include "eval/tests/request_context.pb.h" #include "extensions/comprehensions_v2_functions.h" #include "extensions/comprehensions_v2_macros.h" #include "extensions/protobuf/runtime_adapter.h" -#include "extensions/protobuf/value.h" #include "internal/benchmark.h" #include "internal/testing.h" #include "internal/testing_descriptor_pool.h" -#include "internal/testing_message_factory.h" #include "parser/macro_registry.h" #include "parser/parser.h" #include "runtime/activation.h" @@ -62,6 +66,7 @@ #include "google/protobuf/text_format.h" ABSL_FLAG(bool, enable_recursive_planning, false, "enable recursive planning"); +ABSL_FLAG(bool, enable_typed_field_access, false, "enable typed field access"); namespace cel { @@ -85,16 +90,21 @@ RuntimeOptions GetOptions() { options.max_recursion_depth = -1; } + if (absl::GetFlag(FLAGS_enable_typed_field_access)) { + options.enable_typed_field_access = true; + } + return options; } enum class ConstFoldingEnabled { kNo, kYes }; std::unique_ptr StandardRuntimeOrDie( - const cel::RuntimeOptions& options, google::protobuf::Arena* arena = nullptr, + const cel::RuntimeOptions& options, + const google::protobuf::DescriptorPool* descriptor_pool, + google::protobuf::Arena* arena = nullptr, ConstFoldingEnabled const_folding = ConstFoldingEnabled::kNo) { - auto builder = CreateStandardRuntimeBuilder( - internal::GetTestingDescriptorPool(), options); + auto builder = CreateStandardRuntimeBuilder(descriptor_pool, options); ABSL_CHECK_OK(builder.status()); switch (const_folding) { @@ -111,13 +121,17 @@ std::unique_ptr StandardRuntimeOrDie( return std::move(runtime).value(); } +std::unique_ptr StandardRuntimeOrDie( + const cel::RuntimeOptions& options, google::protobuf::Arena* arena = nullptr, + ConstFoldingEnabled const_folding = ConstFoldingEnabled::kNo) { + return StandardRuntimeOrDie(options, internal::GetTestingDescriptorPool(), + arena, const_folding); +} + template Value WrapMessageOrDie(const T& message, google::protobuf::Arena* absl_nonnull arena) { - auto value = extensions::ProtoMessageToValue( - message, internal::GetTestingDescriptorPool(), - internal::GetTestingMessageFactory(), arena); - ABSL_CHECK_OK(value.status()); - return std::move(value).value(); + return Value::FromMessage(message, google::protobuf::DescriptorPool::generated_pool(), + google::protobuf::MessageFactory::generated_factory(), arena); } // Benchmark test @@ -335,7 +349,23 @@ BENCHMARK(BM_PolicyNative); void BM_PolicySymbolic(benchmark::State& state) { google::protobuf::Arena arena; - ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, Parse(R"cel( + ASSERT_OK_AND_ASSIGN( + auto builder, + NewCompilerBuilder(cel::internal::GetSharedTestingDescriptorPool())); + ASSERT_THAT(builder->AddLibrary(StandardCompilerLibrary()), IsOk()); + ASSERT_THAT(builder->GetCheckerBuilder().AddVariable( + MakeVariableDecl("ip", StringType())), + IsOk()); + ASSERT_THAT(builder->GetCheckerBuilder().AddVariable( + MakeVariableDecl("path", StringType())), + IsOk()); + ASSERT_THAT(builder->GetCheckerBuilder().AddVariable( + MakeVariableDecl("token", StringType())), + IsOk()); + ASSERT_OK_AND_ASSIGN(auto compiler, builder->Build()); + + ASSERT_OK_AND_ASSIGN(ValidationResult validation_result, + compiler->Compile(R"cel( !(ip in ["10.0.1.4", "10.0.1.5", "10.0.1.6"]) && ((path.startsWith("v1") && token in ["v1", "v2", "admin"]) || (path.startsWith("v2") && token in ["v2", "admin"]) || @@ -343,13 +373,14 @@ void BM_PolicySymbolic(benchmark::State& state) { "10.0.1.1", "10.0.1.2", "10.0.1.3" ]) ))cel")); + ASSERT_TRUE(validation_result.IsValid()); + ASSERT_OK_AND_ASSIGN(auto ast, validation_result.ReleaseAst()); RuntimeOptions options = GetOptions(); auto runtime = StandardRuntimeOrDie(options, &arena, ConstFoldingEnabled::kYes); - ASSERT_OK_AND_ASSIGN(auto cel_expr, ProtobufRuntimeAdapter::CreateProgram( - *runtime, parsed_expr)); + ASSERT_OK_AND_ASSIGN(auto cel_expr, runtime->CreateProgram(std::move(ast))); Activation activation; activation.InsertOrAssignValue("ip", StringValue(&arena, kIP)); @@ -437,21 +468,31 @@ class RequestMapImpl : public CustomMapValueInterface { // Uses a lazily constructed map container for "ip", "path", and "token". void BM_PolicySymbolicMap(benchmark::State& state) { google::protobuf::Arena arena; - ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, Parse(R"cel( + ASSERT_OK_AND_ASSIGN( + auto builder, + NewCompilerBuilder(cel::internal::GetSharedTestingDescriptorPool())); + ASSERT_THAT(builder->AddLibrary(StandardCompilerLibrary()), IsOk()); + ASSERT_THAT(builder->GetCheckerBuilder().AddVariable(MakeVariableDecl( + "request", + cel::MapType(&arena, cel::StringType(), cel::StringType()))), + IsOk()); + ASSERT_OK_AND_ASSIGN(auto compiler, builder->Build()); + + ASSERT_OK_AND_ASSIGN(ValidationResult validation_result, + compiler->Compile(R"cel( !(request.ip in ["10.0.1.4", "10.0.1.5", "10.0.1.6"]) && ((request.path.startsWith("v1") && request.token in ["v1", "v2", "admin"]) || (request.path.startsWith("v2") && request.token in ["v2", "admin"]) || (request.path.startsWith("/admin") && request.token == "admin" && request.ip in ["10.0.1.1", "10.0.1.2", "10.0.1.3"]) ))cel")); + ASSERT_TRUE(validation_result.IsValid()); + ASSERT_OK_AND_ASSIGN(auto ast, validation_result.ReleaseAst()); RuntimeOptions options = GetOptions(); - auto runtime = StandardRuntimeOrDie(options); - SourceInfo source_info; - ASSERT_OK_AND_ASSIGN(auto cel_expr, ProtobufRuntimeAdapter::CreateProgram( - *runtime, parsed_expr)); + ASSERT_OK_AND_ASSIGN(auto cel_expr, runtime->CreateProgram(std::move(ast))); Activation activation; CustomMapValue map_value(google::protobuf::Arena::Create(&arena), @@ -472,21 +513,31 @@ BENCHMARK(BM_PolicySymbolicMap); // Uses a protobuf container for "ip", "path", and "token". void BM_PolicySymbolicProto(benchmark::State& state) { google::protobuf::Arena arena; - ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, Parse(R"cel( + ASSERT_OK_AND_ASSIGN( + auto builder, + NewCompilerBuilder(google::protobuf::DescriptorPool::generated_pool())); + ASSERT_THAT(builder->AddLibrary(StandardCompilerLibrary()), IsOk()); + ASSERT_THAT(builder->GetCheckerBuilder().AddVariable(MakeVariableDecl( + "request", cel::MessageType(RequestContext::descriptor()))), + IsOk()); + ASSERT_OK_AND_ASSIGN(auto compiler, builder->Build()); + + ASSERT_OK_AND_ASSIGN(ValidationResult validation_result, + compiler->Compile(R"cel( !(request.ip in ["10.0.1.4", "10.0.1.5", "10.0.1.6"]) && ((request.path.startsWith("v1") && request.token in ["v1", "v2", "admin"]) || (request.path.startsWith("v2") && request.token in ["v2", "admin"]) || (request.path.startsWith("/admin") && request.token == "admin" && request.ip in ["10.0.1.1", "10.0.1.2", "10.0.1.3"]) ))cel")); + ASSERT_TRUE(validation_result.IsValid()); + ASSERT_OK_AND_ASSIGN(auto ast, validation_result.ReleaseAst()); RuntimeOptions options = GetOptions(); + auto runtime = + StandardRuntimeOrDie(options, google::protobuf::DescriptorPool::generated_pool()); - auto runtime = StandardRuntimeOrDie(options); - - SourceInfo source_info; - ASSERT_OK_AND_ASSIGN(auto cel_expr, ProtobufRuntimeAdapter::CreateProgram( - *runtime, parsed_expr)); + ASSERT_OK_AND_ASSIGN(auto cel_expr, runtime->CreateProgram(std::move(ast))); Activation activation; RequestContext request; @@ -497,8 +548,7 @@ void BM_PolicySymbolicProto(benchmark::State& state) { for (auto _ : state) { ASSERT_OK_AND_ASSIGN(cel::Value result, cel_expr->Evaluate(&arena, activation)); - ASSERT_TRUE(InstanceOf(result) && - Cast(result).NativeValue()); + ASSERT_TRUE(result.IsBool() && result.GetBool().NativeValue()); } } diff --git a/runtime/internal/BUILD b/runtime/internal/BUILD index 1223ff6d1..8cfb2b430 100644 --- a/runtime/internal/BUILD +++ b/runtime/internal/BUILD @@ -193,6 +193,7 @@ cc_library( srcs = ["runtime_type_provider.cc"], hdrs = ["runtime_type_provider.h"], deps = [ + "//common:descriptor_pool_type_introspector", "//common:type", "//common:value", "//internal:status_macros", diff --git a/runtime/internal/runtime_type_provider.cc b/runtime/internal/runtime_type_provider.cc index e6fd55d2c..5314918bf 100644 --- a/runtime/internal/runtime_type_provider.cc +++ b/runtime/internal/runtime_type_provider.cc @@ -14,6 +14,7 @@ #include "runtime/internal/runtime_type_provider.h" +#include #include #include "absl/base/nullability.h" @@ -28,7 +29,6 @@ #include "common/value.h" #include "common/values/value_builder.h" #include "google/protobuf/arena.h" -#include "google/protobuf/descriptor.h" #include "google/protobuf/message.h" namespace cel::runtime_internal { @@ -48,9 +48,14 @@ absl::StatusOr> RuntimeTypeProvider::FindTypeImpl( if (type.has_value()) { return type; } - const auto* desc = descriptor_pool_->FindMessageTypeByName(name); - if (desc != nullptr) { - return MessageType(desc); + + auto result = descriptor_pool_provider_.FindType(name); + if (!result.ok()) { + return result; + } + + if (result->has_value() && !result->value().IsEnum()) { + return result; } if (const auto it = types_.find(name); it != types_.end()) { @@ -66,24 +71,7 @@ RuntimeTypeProvider::FindEnumConstantImpl(absl::string_view type, if (enum_constant.has_value()) { return enum_constant; } - const google::protobuf::EnumDescriptor* enum_desc = - descriptor_pool_->FindEnumTypeByName(type); - if (enum_desc == nullptr) { - return std::nullopt; - } - - // Note: we don't support strong enum typing at this time so only the fully - // qualified enum values are meaningful, so we don't provide any signal if the - // enum type is found but can't match the value name. - const google::protobuf::EnumValueDescriptor* value_desc = - enum_desc->FindValueByName(value); - if (value_desc == nullptr) { - return std::nullopt; - } - - return TypeIntrospector::EnumConstant{ - EnumType(enum_desc), enum_desc->full_name(), value_desc->name(), - value_desc->number()}; + return descriptor_pool_provider_.FindEnumConstant(type, value); } absl::StatusOr> @@ -93,18 +81,7 @@ RuntimeTypeProvider::FindStructTypeFieldByNameImpl( if (field.has_value()) { return field; } - const auto* desc = descriptor_pool_->FindMessageTypeByName(type); - if (desc == nullptr) { - return std::nullopt; - } - const auto* field_desc = desc->FindFieldByName(name); - if (field_desc == nullptr) { - field_desc = descriptor_pool_->FindExtensionByPrintableName(desc, name); - if (field_desc == nullptr) { - return std::nullopt; - } - } - return MessageTypeField(field_desc); + return descriptor_pool_provider_.FindStructTypeFieldByName(type, name); } absl::StatusOr diff --git a/runtime/internal/runtime_type_provider.h b/runtime/internal/runtime_type_provider.h index 3f418af4d..d2a343a58 100644 --- a/runtime/internal/runtime_type_provider.h +++ b/runtime/internal/runtime_type_provider.h @@ -21,6 +21,7 @@ #include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" +#include "common/descriptor_pool_type_introspector.h" #include "common/type.h" #include "common/type_reflector.h" #include "common/value.h" @@ -30,11 +31,12 @@ namespace cel::runtime_internal { -class RuntimeTypeProvider final : public TypeReflector { +class RuntimeTypeProvider final : public virtual TypeReflector { public: explicit RuntimeTypeProvider( const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool) - : descriptor_pool_(descriptor_pool) {} + : descriptor_pool_(descriptor_pool), + descriptor_pool_provider_(descriptor_pool) {} absl::Status RegisterType(const OpaqueType& type); @@ -55,6 +57,7 @@ class RuntimeTypeProvider final : public TypeReflector { private: const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool_; + DescriptorPoolTypeIntrospector descriptor_pool_provider_; absl::flat_hash_map types_; }; diff --git a/runtime/runtime_options.h b/runtime/runtime_options.h index 7a61208a0..072daf26c 100644 --- a/runtime/runtime_options.h +++ b/runtime/runtime_options.h @@ -188,6 +188,19 @@ struct RuntimeOptions { // // If disabled, will use the legacy behavior of rounding to 6 decimal places. bool enable_precision_preserving_double_format = true; + + // When enabled, the planner will attempt to use a more performant execution + // path for field access when the type is known at plan time, instead of using + // the generic field access implementation. + // + // The runtime will try to verify that the field access is compatible with the + // actual type at evaluation time, and will fall back to the generic + // implementation if the value is not what was expected. + // + // This is not recommended if the values bound to the activation are typically + // not what the planner expected (e.g. a map that was declared as a proto or + // a different message with matching field names). + bool enable_typed_field_access = false; }; // LINT.ThenChange(//depot/google3/eval/public/cel_options.h)