From 4e9cc59a8de411c1577edbac022f4e942798dd62 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Wed, 12 Aug 2026 14:16:08 -0700 Subject: [PATCH] Call LegacyMessage reflection accesors directly. Removes the reflected AccessApis lookup now that we enforce only a few expected implementations and can determine which implementation was intended without vtable dispatch. PiperOrigin-RevId: 963655068 --- common/BUILD | 1 + common/legacy_value.cc | 61 +++++++++--------- eval/public/structs/BUILD | 15 ++++- eval/public/structs/legacy_type_info_apis.h | 7 ++- .../structs/proto_message_type_adapter.cc | 10 +++ .../structs/proto_message_type_adapter.h | 6 ++ .../public/structs/trivial_legacy_type_info.h | 41 +----------- .../trivial_legacy_type_info_internal.h | 63 +++++++++++++++++++ 8 files changed, 128 insertions(+), 76 deletions(-) create mode 100644 eval/public/structs/trivial_legacy_type_info_internal.h diff --git a/common/BUILD b/common/BUILD index 8f0304a65..5b91f419e 100644 --- a/common/BUILD +++ b/common/BUILD @@ -799,6 +799,7 @@ cc_library( "//eval/public/structs:legacy_type_adapter", "//eval/public/structs:legacy_type_info_apis", "//eval/public/structs:proto_message_type_adapter", + "//eval/public/structs:trivial_legacy_type_info_internal", "//extensions/protobuf/internal:map_reflection", "//extensions/protobuf/internal:qualify", "//internal:casts", diff --git a/common/legacy_value.cc b/common/legacy_value.cc index 50de9a0f8..3f3de6d85 100644 --- a/common/legacy_value.cc +++ b/common/legacy_value.cc @@ -55,6 +55,7 @@ #include "eval/public/structs/legacy_type_adapter.h" #include "eval/public/structs/legacy_type_info_apis.h" #include "eval/public/structs/proto_message_type_adapter.h" +#include "eval/public/structs/trivial_legacy_type_info_internal.h" #include "internal/json.h" #include "internal/status_macros.h" #include "internal/well_known_types.h" @@ -71,6 +72,7 @@ namespace cel { namespace { +using ::cel::interop_internal::TrivialTypeInfo; using ::google::api::expr::runtime::CelList; using ::google::api::expr::runtime::CelMap; using ::google::api::expr::runtime::CelValue; @@ -80,6 +82,7 @@ using ::google::api::expr::runtime::FieldBackedMapImpl; using ::google::api::expr::runtime::GetGenericProtoTypeInfoInstance; using ::google::api::expr::runtime::LegacyTypeInfoApis; using ::google::api::expr::runtime::MessageWrapper; +using ::google::api::expr::runtime::internal::GetGenericProtoAccessApisInstance; using ::google::api::expr::runtime::internal::MaybeWrapValueToMessage; absl::Status InvalidMapKeyTypeError(ValueKind kind) { @@ -885,17 +888,16 @@ absl::Status LegacyStructValue::Equal( if (auto legacy_struct_value = common_internal::AsLegacyStructValue(other); legacy_struct_value.has_value()) { auto message_wrapper = AsMessageWrapper(message_ptr_, legacy_type_info_); - const auto* access_apis = - message_wrapper.legacy_type_info()->GetAccessApis(message_wrapper); - if (ABSL_PREDICT_FALSE(access_apis == nullptr)) { + if (ABSL_PREDICT_FALSE(legacy_type_info_ == + TrivialTypeInfo::GetInstance())) { return absl::UnimplementedError( absl::StrCat("legacy access APIs missing for ", GetTypeName())); } auto other_message_wrapper = AsMessageWrapper(legacy_struct_value->message_ptr(), legacy_struct_value->legacy_type_info()); - *result = BoolValue{ - access_apis->IsEqualTo(message_wrapper, other_message_wrapper)}; + *result = BoolValue{GetGenericProtoAccessApisInstance().IsEqualTo( + message_wrapper, other_message_wrapper)}; return absl::OkStatus(); } if (auto struct_value = other.AsStruct(); struct_value.has_value()) { @@ -909,12 +911,12 @@ absl::Status LegacyStructValue::Equal( bool LegacyStructValue::IsZeroValue() const { auto message_wrapper = AsMessageWrapper(message_ptr_, legacy_type_info_); - const auto* access_apis = - message_wrapper.legacy_type_info()->GetAccessApis(message_wrapper); - if (ABSL_PREDICT_FALSE(access_apis == nullptr)) { + if (ABSL_PREDICT_FALSE(legacy_type_info_ == TrivialTypeInfo::GetInstance())) { return false; } - return access_apis->ListFields(message_wrapper).empty(); + return GetGenericProtoAccessApisInstance() + .ListFields(message_wrapper) + .empty(); } absl::Status LegacyStructValue::GetFieldByName( @@ -923,16 +925,14 @@ absl::Status LegacyStructValue::GetFieldByName( google::protobuf::MessageFactory* absl_nonnull message_factory, google::protobuf::Arena* absl_nonnull arena, Value* absl_nonnull result) const { auto message_wrapper = AsMessageWrapper(message_ptr_, legacy_type_info_); - const auto* access_apis = - message_wrapper.legacy_type_info()->GetAccessApis(message_wrapper); - if (ABSL_PREDICT_FALSE(access_apis == nullptr)) { + if (ABSL_PREDICT_FALSE(legacy_type_info_ == TrivialTypeInfo::GetInstance())) { *result = NoSuchFieldError(name); return absl::OkStatus(); } - CEL_ASSIGN_OR_RETURN( - auto cel_value, - access_apis->GetField(name, message_wrapper, unboxing_options, - MemoryManagerRef::Pooling(arena))); + CEL_ASSIGN_OR_RETURN(auto cel_value, + GetGenericProtoAccessApisInstance().GetField( + name, message_wrapper, unboxing_options, + MemoryManagerRef::Pooling(arena))); CEL_RETURN_IF_ERROR(ModernValue(arena, cel_value, *result)); return absl::OkStatus(); } @@ -949,12 +949,10 @@ absl::Status LegacyStructValue::GetFieldByNumber( absl::StatusOr LegacyStructValue::HasFieldByName( absl::string_view name) const { auto message_wrapper = AsMessageWrapper(message_ptr_, legacy_type_info_); - const auto* access_apis = - message_wrapper.legacy_type_info()->GetAccessApis(message_wrapper); - if (ABSL_PREDICT_FALSE(access_apis == nullptr)) { + if (ABSL_PREDICT_FALSE(legacy_type_info_ == TrivialTypeInfo::GetInstance())) { return NoSuchFieldError(name).NativeValue(); } - return access_apis->HasField(name, message_wrapper); + return GetGenericProtoAccessApisInstance().HasField(name, message_wrapper); } absl::StatusOr LegacyStructValue::HasFieldByNumber(int64_t number) const { @@ -968,20 +966,19 @@ absl::Status LegacyStructValue::ForEachField( google::protobuf::MessageFactory* absl_nonnull message_factory, google::protobuf::Arena* absl_nonnull arena) const { auto message_wrapper = AsMessageWrapper(message_ptr_, legacy_type_info_); - const auto* access_apis = - message_wrapper.legacy_type_info()->GetAccessApis(message_wrapper); - if (ABSL_PREDICT_FALSE(access_apis == nullptr)) { + if (ABSL_PREDICT_FALSE(legacy_type_info_ == TrivialTypeInfo::GetInstance())) { return absl::UnimplementedError( absl::StrCat("legacy access APIs missing for ", GetTypeName())); } - auto field_names = access_apis->ListFields(message_wrapper); + const auto& access_apis = GetGenericProtoAccessApisInstance(); + auto field_names = access_apis.ListFields(message_wrapper); Value value; for (const auto& field_name : field_names) { CEL_ASSIGN_OR_RETURN( auto cel_value, - access_apis->GetField(field_name, message_wrapper, - ProtoWrapperTypeOptions::kUnsetNull, - MemoryManagerRef::Pooling(arena))); + access_apis.GetField(field_name, message_wrapper, + ProtoWrapperTypeOptions::kUnsetNull, + MemoryManagerRef::Pooling(arena))); CEL_RETURN_IF_ERROR(ModernValue(arena, cel_value, value)); CEL_ASSIGN_OR_RETURN(auto ok, callback(field_name, value)); if (!ok) { @@ -1001,9 +998,7 @@ absl::Status LegacyStructValue::Qualify( return absl::InvalidArgumentError("invalid select qualifier path."); } auto message_wrapper = AsMessageWrapper(message_ptr_, legacy_type_info_); - const auto* access_apis = - message_wrapper.legacy_type_info()->GetAccessApis(message_wrapper); - if (ABSL_PREDICT_FALSE(access_apis == nullptr)) { + if (ABSL_PREDICT_FALSE(legacy_type_info_ == TrivialTypeInfo::GetInstance())) { absl::string_view field_name = absl::visit( absl::Overload( [](const FieldSpecifier& field) -> absl::string_view { @@ -1017,9 +1012,9 @@ absl::Status LegacyStructValue::Qualify( *count = -1; return absl::OkStatus(); } - CEL_ASSIGN_OR_RETURN( - auto legacy_result, - access_apis->Qualify(qualifiers, message_wrapper, presence_test, + CEL_ASSIGN_OR_RETURN(auto legacy_result, + GetGenericProtoAccessApisInstance().Qualify( + qualifiers, message_wrapper, presence_test, MemoryManager::Pooling(arena))); CEL_RETURN_IF_ERROR(ModernValue(arena, legacy_result.value, *result)); *count = legacy_result.qualifier_count; diff --git a/eval/public/structs/BUILD b/eval/public/structs/BUILD index 82e6fd5de..992651b8b 100644 --- a/eval/public/structs/BUILD +++ b/eval/public/structs/BUILD @@ -367,9 +367,8 @@ cc_library( ) cc_library( - name = "trivial_legacy_type_info", - testonly = True, - hdrs = ["trivial_legacy_type_info.h"], + name = "trivial_legacy_type_info_internal", + hdrs = ["trivial_legacy_type_info_internal.h"], deps = [ ":legacy_type_info_apis", "//eval/public:message_wrapper", @@ -379,6 +378,16 @@ cc_library( ], ) +cc_library( + name = "trivial_legacy_type_info", + testonly = True, + hdrs = ["trivial_legacy_type_info.h"], + deps = [ + ":legacy_type_info_apis", + ":trivial_legacy_type_info_internal", + ], +) + cc_test( name = "trivial_legacy_type_info_test", srcs = ["trivial_legacy_type_info_test.cc"], diff --git a/eval/public/structs/legacy_type_info_apis.h b/eval/public/structs/legacy_type_info_apis.h index d7f5db3fa..999f12945 100644 --- a/eval/public/structs/legacy_type_info_apis.h +++ b/eval/public/structs/legacy_type_info_apis.h @@ -23,6 +23,10 @@ #include "eval/public/message_wrapper.h" #include "google/protobuf/descriptor.h" +namespace cel::interop_internal { +struct TrivialTypeInfo; +} // namespace cel::interop_internal + namespace google::api::expr::runtime { // Forward declared to resolve cyclic dependency. @@ -31,7 +35,6 @@ class LegacyTypeAccessApis; // Forward declare permitted subclasses. class DucktypedMessageAdapter; class ProtoMessageTypeAdapter; -class TrivialTypeInfo; // Interface for providing type info from a user defined type (represented as a // message). @@ -100,7 +103,7 @@ class LegacyTypeInfoApis { // supported using the cel::Value APIs. friend class DucktypedMessageAdapter; friend class ProtoMessageTypeAdapter; - friend class TrivialTypeInfo; + friend class cel::interop_internal::TrivialTypeInfo; LegacyTypeInfoApis() = default; }; diff --git a/eval/public/structs/proto_message_type_adapter.cc b/eval/public/structs/proto_message_type_adapter.cc index 2f72ec68a..228710c59 100644 --- a/eval/public/structs/proto_message_type_adapter.cc +++ b/eval/public/structs/proto_message_type_adapter.cc @@ -362,6 +362,10 @@ class DucktypedMessageAdapter : public LegacyTypeAccessApis, static absl::NoDestructor instance; return *instance; } + + private: + friend class absl::NoDestructor; + DucktypedMessageAdapter() = default; }; namespace { @@ -488,4 +492,10 @@ const LegacyTypeInfoApis& GetGenericProtoTypeInfoInstance() { return DucktypedMessageAdapter::GetSingleton(); } +namespace internal { +const LegacyTypeAccessApis& GetGenericProtoAccessApisInstance() { + return DucktypedMessageAdapter::GetSingleton(); +} +} // namespace internal + } // namespace google::api::expr::runtime diff --git a/eval/public/structs/proto_message_type_adapter.h b/eval/public/structs/proto_message_type_adapter.h index 867ba39cf..f1598abd9 100644 --- a/eval/public/structs/proto_message_type_adapter.h +++ b/eval/public/structs/proto_message_type_adapter.h @@ -21,6 +21,8 @@ #include "absl/base/nullability.h" #include "absl/status/status.h" #include "absl/strings/string_view.h" +#include "absl/types/span.h" +#include "base/attribute.h" #include "common/memory.h" #include "eval/public/cel_options.h" #include "eval/public/cel_value.h" @@ -105,6 +107,10 @@ absl::StatusOr CreateCelValueFromField( // instead of expecting a particular message type given a TypeInfo. const LegacyTypeInfoApis& GetGenericProtoTypeInfoInstance(); +namespace internal { +const LegacyTypeAccessApis& GetGenericProtoAccessApisInstance(); +} // namespace internal + } // namespace google::api::expr::runtime #endif // THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_STRUCTS_PROTO_MESSAGE_TYPE_ADAPTER_H_ diff --git a/eval/public/structs/trivial_legacy_type_info.h b/eval/public/structs/trivial_legacy_type_info.h index dd413e723..2a1bf346b 100644 --- a/eval/public/structs/trivial_legacy_type_info.h +++ b/eval/public/structs/trivial_legacy_type_info.h @@ -15,49 +15,14 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_STRUCTS_TRIVIAL_LEGACY_TYPE_INFO_H_ #define THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_STRUCTS_TRIVIAL_LEGACY_TYPE_INFO_H_ -#include - -#include "absl/base/no_destructor.h" -#include "absl/base/nullability.h" -#include "absl/strings/string_view.h" -#include "eval/public/message_wrapper.h" -#include "eval/public/structs/legacy_type_info_apis.h" +#include "eval/public/structs/legacy_type_info_apis.h" // IWYU pragma: keep +#include "eval/public/structs/trivial_legacy_type_info_internal.h" namespace google::api::expr::runtime { // Implementation of type info APIs suitable for testing where no message // operations need to be supported. -class TrivialTypeInfo : public LegacyTypeInfoApis { - private: - struct Key {}; - - public: - explicit TrivialTypeInfo(Key&) {} - - absl::string_view GetTypename(const MessageWrapper& wrapper) const override { - return "opaque"; - } - - std::string DebugString(const MessageWrapper& wrapper) const override { - return "opaque"; - } - - const LegacyTypeAccessApis* GetAccessApis( - const MessageWrapper& wrapper) const override { - // Accessors unsupported -- caller should treat this as an opaque type (no - // fields defined, field access always results in a CEL error). - return nullptr; - } - - static const TrivialTypeInfo* absl_nonnull GetInstance() { - static absl::NoDestructor kKey; - static absl::NoDestructor kInstance(*kKey); - return &*kInstance; - } - - private: - TrivialTypeInfo() = default; -}; +using TrivialTypeInfo = cel::interop_internal::TrivialTypeInfo; } // namespace google::api::expr::runtime diff --git a/eval/public/structs/trivial_legacy_type_info_internal.h b/eval/public/structs/trivial_legacy_type_info_internal.h new file mode 100644 index 000000000..68408070a --- /dev/null +++ b/eval/public/structs/trivial_legacy_type_info_internal.h @@ -0,0 +1,63 @@ +// Copyright 2022 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_STRUCTS_TRIVIAL_LEGACY_TYPE_INFO_INTERNAL_H_ +#define THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_STRUCTS_TRIVIAL_LEGACY_TYPE_INFO_INTERNAL_H_ + +#include + +#include "absl/base/no_destructor.h" +#include "absl/base/nullability.h" +#include "absl/strings/string_view.h" +#include "eval/public/message_wrapper.h" +#include "eval/public/structs/legacy_type_info_apis.h" + +namespace cel::interop_internal { + +// Implementation details. Exported here for the sake of Legacy->Modern +// Value adaptation. +class TrivialTypeInfo : public google::api::expr::runtime::LegacyTypeInfoApis { + public: + absl::string_view GetTypename( + const google::api::expr::runtime::MessageWrapper& wrapper) + const override { + return "opaque"; + } + + std::string DebugString(const google::api::expr::runtime::MessageWrapper& + wrapper) const override { + return "opaque"; + } + + const google::api::expr::runtime::LegacyTypeAccessApis* GetAccessApis( + const google::api::expr::runtime::MessageWrapper& wrapper) + const override { + // Accessors unsupported -- caller should treat this as an opaque type (no + // fields defined, field access always results in a CEL error). + return nullptr; + } + + static const TrivialTypeInfo* absl_nonnull GetInstance() { + static absl::NoDestructor kInstance; + return &*kInstance; + } + + private: + friend class absl::NoDestructor; + TrivialTypeInfo() = default; +}; + +} // namespace cel::interop_internal + +#endif // THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_STRUCTS_TRIVIAL_LEGACY_TYPE_INFO_INTERNAL_H_