Misc consistency/style fixes for Struct types:

common/descriptor_pool_type_introspector.cc

@@ -44,8 +44,7 @@ FindStructTypeFieldByNameDirectly(
   if (descriptor == nullptr) {
     return std::nullopt;
   }
-  const google::protobuf::FieldDescriptor* absl_nullable field =
-      descriptor->FindFieldByName(name);
+  const google::protobuf::FieldDescriptor* field = descriptor->FindFieldByName(name);
   if (field != nullptr) {
     return StructTypeField(MessageTypeField(field));
   }

common/legacy_value.cc

@@ -71,14 +71,15 @@ namespace cel {
 
 namespace {
 
-using google::api::expr::runtime::CelList;
-using google::api::expr::runtime::CelMap;
-using google::api::expr::runtime::CelValue;
-using google::api::expr::runtime::FieldBackedListImpl;
-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::CelList;
+using ::google::api::expr::runtime::CelMap;
+using ::google::api::expr::runtime::CelValue;
+using ::google::api::expr::runtime::CreateCelValueFromField;
+using ::google::api::expr::runtime::FieldBackedListImpl;
+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::MaybeWrapValueToMessage;
 
 absl::Status InvalidMapKeyTypeError(ValueKind kind) {
@@ -1288,6 +1289,36 @@ TypeValue CreateTypeValueFromView(google::protobuf::Arena* arena,
   return TypeValue(common_internal::LegacyRuntimeType(input));
 }
 
+const google::protobuf::Message* absl_nullable GetLegacyMessage(const Value& value) {
+  if (!common_internal::IsLegacyStructValue(value)) {
+    return nullptr;
+  }
+
+  auto legacy = common_internal::GetLegacyStructValue(value);
+  const auto* legacy_type_info = legacy.legacy_type_info();
+  if (legacy_type_info == nullptr) {
+    return nullptr;
+  }
+  if (legacy_type_info != &GetGenericProtoTypeInfoInstance()) {
+    return nullptr;
+  }
+  if (IsWellKnownMessageType(legacy.message_ptr()->GetDescriptor())) {
+    return nullptr;
+  }
+  return legacy.message_ptr();
+}
+
+absl::Status WrapLegacyMessageField(
+    const google::protobuf::Message* absl_nonnull message,
+    const google::protobuf::FieldDescriptor* absl_nonnull field_descriptor,
+    ProtoWrapperTypeOptions unboxing_option, google::protobuf::Arena* arena,
+    Value* absl_nonnull out) {
+  CEL_ASSIGN_OR_RETURN(CelValue result,
+                       CreateCelValueFromField(message, field_descriptor,
+                                               unboxing_option, arena));
+  return ModernValue(arena, result, *out);
+}
+
 }  // namespace interop_internal
 
 }  // namespace cel

common/legacy_value.h

@@ -27,7 +27,9 @@
 #include "common/value.h"
 #include "eval/public/cel_value.h"
 #include "internal/status_macros.h"
+#include "runtime/runtime_options.h"
 #include "google/protobuf/arena.h"
+#include "google/protobuf/descriptor.h"
 
 namespace cel {
 
@@ -59,6 +61,19 @@ google::api::expr::runtime::CelValue UnsafeLegacyValue(
 
 namespace cel::interop_internal {
 
+// Returns the underlying `google::protobuf::Message` of a `cel::Value` if it is a legacy
+// message with the default type info, or `nullptr` otherwise.
+const google::protobuf::Message* absl_nullable GetLegacyMessage(const Value& value);
+
+// Access a field on a legacy message value, writing the result to `out`.
+// Prefers wrapping legacy values instead of using the modern value
+// representation.
+absl::Status WrapLegacyMessageField(
+    const google::protobuf::Message* absl_nonnull message,
+    const google::protobuf::FieldDescriptor* absl_nonnull field_descriptor,
+    ProtoWrapperTypeOptions unboxing_option, google::protobuf::Arena* arena,
+    Value* absl_nonnull out);
+
 absl::StatusOr<Value> FromLegacyValue(
     google::protobuf::Arena* arena,
     const google::api::expr::runtime::CelValue& legacy_value,

common/type.cc

@@ -606,6 +606,11 @@ absl::optional<MessageTypeField> StructTypeField::AsMessage() const {
   return std::nullopt;
 }
 
+MessageTypeField StructTypeField::GetMessage() const {
+  ABSL_DCHECK(IsMessage());
+  return absl::get<MessageTypeField>(variant_);
+}
+
 StructTypeField::operator MessageTypeField() const {
   ABSL_DCHECK(IsMessage());
   return absl::get<MessageTypeField>(variant_);

common/type.h

@@ -1140,6 +1140,7 @@ class StructTypeField final {
   }
 
   absl::optional<MessageTypeField> AsMessage() const;
+  MessageTypeField GetMessage() const;
 
   explicit operator MessageTypeField() const;
 

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_;
   }

common/value.cc

@@ -17,6 +17,7 @@
 #include <cstddef>
 #include <cstdint>
 #include <memory>
+#include <optional>
 #include <ostream>
 #include <string>
 #include <type_traits>

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<ParsedMessageValue>;
   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.

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));
   }
 

eval/public/structs/BUILD

@@ -325,6 +325,7 @@ cc_test(
         ":proto_message_type_adapter",
         "//base:attributes",
         "//common:value",
+        "//common:value_testing",
         "//eval/public:cel_value",
         "//eval/public:message_wrapper",
         "//eval/public/containers:container_backed_list_impl",
@@ -365,10 +366,13 @@ cc_test(
     deps = [
         ":legacy_type_info_apis",
         ":protobuf_descriptor_type_provider",
+        "//common:type",
         "//eval/public:cel_value",
         "//eval/public/testing:matchers",
         "//extensions/protobuf:memory_manager",
         "//internal:testing",
+        "@com_google_absl//absl/status:status_matchers",
+        "@com_google_cel_spec//proto/cel/expr/conformance/proto3:test_all_types_cc_proto",
         "@com_google_protobuf//:protobuf",
         "@com_google_protobuf//:wrappers_cc_proto",
     ],
@@ -414,7 +418,9 @@ cc_test(
     deps = [
         ":legacy_type_info_apis",
         ":legacy_type_provider",
+        "//common:type",
         "//internal:testing",
+        "@com_google_absl//absl/status:status_matchers",
         "@com_google_absl//absl/strings:string_view",
     ],
 )

eval/public/structs/legacy_type_info_apis.h

@@ -61,6 +61,11 @@ class LegacyTypeInfoApis {
   virtual absl::string_view GetTypename(
       const MessageWrapper& wrapped_message) const = 0;
 
+  // Return a pointer to the descriptor for the wrapped message's type.
+  //
+  // Should only be defined for messages with standard behavior (i.e. normal
+  // duck-typed behavior of resolving fields by associated descriptor is
+  // correct).
   virtual const google::protobuf::Descriptor* absl_nullable GetDescriptor(
       const MessageWrapper& wrapped_message [[maybe_unused]]) const {
     return nullptr;

eval/public/structs/legacy_type_provider.cc

@@ -16,6 +16,7 @@
 
 #include <cstdint>
 #include <memory>
+#include <optional>
 #include <utility>
 
 #include "absl/base/nullability.h"
@@ -35,6 +36,7 @@
 #include "extensions/protobuf/memory_manager.h"
 #include "internal/status_macros.h"
 #include "google/protobuf/arena.h"
+#include "google/protobuf/descriptor.h"
 #include "google/protobuf/message.h"
 
 namespace google::api::expr::runtime {
@@ -197,22 +199,34 @@ LegacyTypeProvider::FindStructTypeFieldByNameImpl(
       result.has_value()) {
     return result;
   }
-  if (auto type_info = ProvideLegacyTypeInfo(type); type_info.has_value()) {
-    if (auto field_desc = (*type_info)->FindFieldByName(name);
-        field_desc.has_value()) {
-      return cel::common_internal::BasicStructTypeField(
-          field_desc->name, field_desc->number, cel::DynType{});
-    } else {
-      const auto* mutation_apis =
-          (*type_info)->GetMutationApis(MessageWrapper());
-      if (mutation_apis == nullptr || !mutation_apis->DefinesField(name)) {
-        return std::nullopt;
-      }
-      return cel::common_internal::BasicStructTypeField(name, 0,
-                                                        cel::DynType{});
+  absl::optional<const LegacyTypeInfoApis*> type_info =
+      ProvideLegacyTypeInfo(type);
+  if (!type_info.has_value()) {
+    return std::nullopt;
+  }
+  if (const auto* descriptor = (*type_info)->GetDescriptor(MessageWrapper());
+      descriptor != nullptr) {
+    // If it's a normal proto, just use the descriptor to find the field.
+    // Allows us to get the same optimizations as the modern value in most
+    // cases.
+    const google::protobuf::FieldDescriptor* field = descriptor->FindFieldByName(name);
+    if (field != nullptr) {
+      return cel::StructTypeField(cel::MessageTypeField(field));
     }
   }
-  return std::nullopt;
+
+  if (auto field_desc = (*type_info)->FindFieldByName(name);
+      field_desc.has_value()) {
+    return cel::common_internal::BasicStructTypeField(
+        field_desc->name, field_desc->number, cel::DynType{});
+  }
+
+  const auto* mutation_apis = (*type_info)->GetMutationApis(MessageWrapper());
+  if (mutation_apis == nullptr || !mutation_apis->DefinesField(name)) {
+    return std::nullopt;
+  }
+
+  return cel::common_internal::BasicStructTypeField(name, 0, cel::DynType{});
 }
 
 }  // namespace google::api::expr::runtime

eval/public/structs/legacy_type_provider_test.cc

@@ -17,13 +17,17 @@
 #include <optional>
 #include <string>
 
+#include "absl/status/status_matchers.h"
 #include "absl/strings/string_view.h"
+#include "common/type.h"
 #include "eval/public/structs/legacy_type_info_apis.h"
 #include "internal/testing.h"
 
 namespace google::api::expr::runtime {
 namespace {
 
+using ::absl_testing::IsOk;
+
 class LegacyTypeProviderTestEmpty : public LegacyTypeProvider {
  public:
   absl::optional<LegacyTypeAdapter> ProvideLegacyType(
@@ -46,6 +50,13 @@ class LegacyTypeInfoApisEmpty : public LegacyTypeInfoApis {
       const MessageWrapper& wrapped_message) const override {
     return nullptr;
   }
+  absl::optional<FieldDescription> FindFieldByName(
+      absl::string_view name) const override {
+    if (name == "field1") {
+      return FieldDescription{1, "field1"};
+    }
+    return absl::nullopt;
+  }
 
  private:
   const std::string test_string_ = "test";
@@ -89,5 +100,22 @@ TEST(LegacyTypeProviderTest, NonEmptyTypeProviderProvidesSomeTypes) {
   EXPECT_EQ(provider.ProvideLegacyTypeInfo("other"), std::nullopt);
 }
 
+TEST(LegacyTypeProviderTest, FindStructTypeFieldByName) {
+  LegacyTypeInfoApisEmpty test_type_info;
+  LegacyTypeProviderTestImpl provider(&test_type_info);
+
+  ASSERT_OK_AND_ASSIGN(absl::optional<cel::StructTypeField> field,
+                       provider.FindStructTypeFieldByName("test", "field1"));
+  ASSERT_TRUE(field.has_value());
+  EXPECT_EQ(field->name(), "field1");
+  EXPECT_EQ(field->number(), 1);
+  EXPECT_EQ(field->GetType(), cel::DynType());
+
+  ASSERT_OK_AND_ASSIGN(
+      absl::optional<cel::StructTypeField> not_found_field,
+      provider.FindStructTypeFieldByName("test", "unknown_field"));
+  EXPECT_FALSE(not_found_field.has_value());
+}
+
 }  // namespace
 }  // namespace google::api::expr::runtime