From 4bf59ba35d1c3666a314230f2438f1eff4bfabed Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Mon, 20 Jul 2026 13:38:57 -0700 Subject: [PATCH] Update type_spec_resolver to support TypeIntrospector. Needed to support looking up types at runtime. PiperOrigin-RevId: 951027622 --- 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/type_spec_resolver.cc | 95 ++++++++++++------- common/type_spec_resolver.h | 6 ++ common/type_spec_resolver_test.cc | 57 +++++++++++ 9 files changed, 174 insertions(+), 85 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/type_spec_resolver.cc b/common/type_spec_resolver.cc index c3aa3d5a2..aab099255 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, - const google::protobuf::DescriptorPool& pool, - google::protobuf::Arena* arena) { +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(), - pool, arena)); + 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(), pool, arena)); + 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(), - pool, arena)); + 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(), pool, arena)); + 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, pool, arena)); + 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,36 @@ 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; + params.reserve(type_spec.abstract_type().parameter_types().size()); for (const auto& param_spec : type_spec.abstract_type().parameter_types()) { - CEL_ASSIGN_OR_RETURN(auto param, - ConvertTypeSpecToType(param_spec, pool, arena)); + 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 +198,9 @@ absl::StatusOr ConvertTypeSpecToType(const TypeSpec& type_spec, } if (type_spec.has_type()) { - CEL_ASSIGN_OR_RETURN(auto contained_type, - ConvertTypeSpecToType(type_spec.type(), pool, arena)); + CEL_ASSIGN_OR_RETURN( + auto contained_type, + ConvertTypeSpecToType(type_spec.type(), type_introspector, arena)); return Type(TypeType(arena, contained_type)); } @@ -198,6 +211,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 2cd860f02..17fbf27de 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,6 +29,11 @@ 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 for type resolution using only a descriptor pool. absl::StatusOr ConvertTypeSpecToType(const TypeSpec& type_spec, const google::protobuf::DescriptorPool& pool, google::protobuf::Arena* arena); diff --git a/common/type_spec_resolver_test.cc b/common/type_spec_resolver_test.cc index dbde63e6d..056ba8f67 100644 --- a/common/type_spec_resolver_test.cc +++ b/common/type_spec_resolver_test.cc @@ -25,6 +25,7 @@ #include "absl/status/status_matchers.h" #include "common/ast.h" #include "common/type.h" +#include "common/type_introspector.h" #include "common/type_kind.h" #include "internal/testing.h" #include "internal/testing_descriptor_pool.h" @@ -273,6 +274,62 @@ TEST(TypeSpecResolverTest, EnumTypeWithParamsError) { 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, WellKnownTypeIntrospector) { + WellKnownTypeIntrospector introspector; + + TypeSpec message_spec(MessageTypeSpec("google.protobuf.Timestamp")); + ASSERT_OK_AND_ASSIGN( + auto message_type, + ConvertTypeSpecToType(message_spec, introspector, GetTestArena())); + EXPECT_TRUE(message_type.IsTimestamp()); + + TypeSpec abstract_spec( + AbstractType("google.protobuf.Duration", /*params=*/{})); + ASSERT_OK_AND_ASSIGN( + auto abstract_type, + ConvertTypeSpecToType(abstract_spec, introspector, GetTestArena())); + EXPECT_TRUE(abstract_type.IsDuration()); + + TypeSpec primitive_spec(PrimitiveType::kInt64); + ASSERT_OK_AND_ASSIGN( + auto primitive_type, + ConvertTypeSpecToType(primitive_spec, introspector, GetTestArena())); + EXPECT_TRUE(primitive_type.IsInt()); + + TypeSpec custom_spec(MessageTypeSpec("custom.UnknownMessage")); + EXPECT_THAT( + ConvertTypeSpecToType(custom_spec, introspector, GetTestArena()), + StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("Message type 'custom.UnknownMessage' not found"))); +} + TEST(TypeSpecResolverTest, UnknownTypeSpecKindError) { TypeSpec spec; auto t =