Prevent new subclasses of legacy TypeInfoApis.

eval/eval/BUILD

@@ -789,9 +789,7 @@ cc_test(
     deps = [
         ":attribute_trail",
         ":cel_expression_flat_impl",
-        ":compiler_constant_step",
         ":const_value_step",
-        ":create_map_step",
         ":evaluator_core",
         ":ident_step",
         ":select_step",
@@ -810,9 +808,7 @@ cc_test(
         "//eval/public:unknown_set",
         "//eval/public/containers:container_backed_map_impl",
         "//eval/public/structs:cel_proto_wrapper",
-        "//eval/public/structs:legacy_type_adapter",
         "//eval/public/structs:trivial_legacy_type_info",
-        "//eval/public/testing:matchers",
         "//eval/testutil:test_extensions_cc_proto",
         "//eval/testutil:test_message_cc_proto",
         "//extensions/protobuf:value",

eval/eval/select_step_test.cc

@@ -24,19 +24,15 @@
 #include "common/value_testing.h"
 #include "eval/eval/attribute_trail.h"
 #include "eval/eval/cel_expression_flat_impl.h"
-#include "eval/eval/compiler_constant_step.h"
 #include "eval/eval/const_value_step.h"
-#include "eval/eval/create_map_step.h"
 #include "eval/eval/evaluator_core.h"
 #include "eval/eval/ident_step.h"
 #include "eval/public/activation.h"
 #include "eval/public/cel_attribute.h"
 #include "eval/public/cel_value.h"
 #include "eval/public/containers/container_backed_map_impl.h"
 #include "eval/public/structs/cel_proto_wrapper.h"
-#include "eval/public/structs/legacy_type_adapter.h"
 #include "eval/public/structs/trivial_legacy_type_info.h"
-#include "eval/public/testing/matchers.h"
 #include "eval/public/unknown_attribute_set.h"
 #include "eval/public/unknown_set.h"
 #include "eval/testutil/test_extensions.pb.h"
@@ -81,43 +77,15 @@ using ::cel::internal::test::EqualsProto;
 using ::cel::runtime_internal::NewTestingRuntimeEnv;
 using ::cel::runtime_internal::RuntimeEnv;
 using ::cel::test::IntValueIs;
-using ::testing::_;
 using ::testing::Eq;
 using ::testing::HasSubstr;
-using ::testing::Return;
 using ::testing::UnorderedElementsAre;
 
 struct RunExpressionOptions {
   bool enable_unknowns = false;
   bool enable_wrapper_type_null_unboxing = false;
 };
 
-// Simple implementation LegacyTypeAccessApis / LegacyTypeInfoApis that allows
-// mocking for getters/setters.
-class MockAccessor : public LegacyTypeAccessApis, public LegacyTypeInfoApis {
- public:
-  MOCK_METHOD(absl::StatusOr<bool>, HasField,
-              (absl::string_view field_name,
-               const CelValue::MessageWrapper& value),
-              (const, override));
-  MOCK_METHOD(absl::StatusOr<CelValue>, GetField,
-              (absl::string_view field_name,
-               const CelValue::MessageWrapper& instance,
-               ProtoWrapperTypeOptions unboxing_option,
-               cel::MemoryManagerRef memory_manager),
-              (const, override));
-  MOCK_METHOD(absl::string_view, GetTypename,
-              (const CelValue::MessageWrapper& instance), (const, override));
-  MOCK_METHOD(std::string, DebugString,
-              (const CelValue::MessageWrapper& instance), (const, override));
-  MOCK_METHOD(std::vector<absl::string_view>, ListFields,
-              (const CelValue::MessageWrapper& value), (const, override));
-  const LegacyTypeAccessApis* GetAccessApis(
-      const CelValue::MessageWrapper& instance) const override {
-    return this;
-  }
-};
-
 class SelectStepTest : public testing::Test {
  public:
   SelectStepTest() : env_(NewTestingRuntimeEnv()) {}
@@ -702,68 +670,6 @@ TEST_P(SelectStepConformanceTest, NullMessageAccessor) {
   EXPECT_THAT(*result.ErrorOrDie(), StatusIs(absl::StatusCode::kNotFound));
 }
 
-TEST_P(SelectStepConformanceTest, CustomAccessor) {
-  TestMessage message;
-  TestMessage* message2 = message.mutable_message_value();
-  message2->set_int32_value(1);
-  message2->set_string_value("test");
-  RunExpressionOptions options;
-  options.enable_unknowns = GetParam();
-  testing::NiceMock<MockAccessor> accessor;
-  CelValue value = CelValue::CreateMessageWrapper(
-      CelValue::MessageWrapper(&message, &accessor));
-
-  ON_CALL(accessor, GetField(_, _, _, _))
-      .WillByDefault(Return(CelValue::CreateInt64(2)));
-  ON_CALL(accessor, HasField(_, _)).WillByDefault(Return(false));
-
-  ASSERT_OK_AND_ASSIGN(CelValue result,
-                       RunExpression(value, "message_value",
-                                     /*test=*/false,
-                                     /*unknown_path=*/"", options));
-
-  EXPECT_THAT(result, test::IsCelInt64(2));
-
-  // testonly select (has)
-  ASSERT_OK_AND_ASSIGN(result, RunExpression(value, "message_value",
-                                             /*test=*/true,
-                                             /*unknown_path=*/"", options));
-
-  EXPECT_THAT(result, test::IsCelBool(false));
-}
-
-TEST_P(SelectStepConformanceTest, CustomAccessorErrorHandling) {
-  TestMessage message;
-  TestMessage* message2 = message.mutable_message_value();
-  message2->set_int32_value(1);
-  message2->set_string_value("test");
-  RunExpressionOptions options;
-  options.enable_unknowns = GetParam();
-  testing::NiceMock<MockAccessor> accessor;
-  CelValue value = CelValue::CreateMessageWrapper(
-      CelValue::MessageWrapper(&message, &accessor));
-
-  ON_CALL(accessor, GetField(_, _, _, _))
-      .WillByDefault(Return(absl::InternalError("bad data")));
-  ON_CALL(accessor, HasField(_, _))
-      .WillByDefault(Return(absl::NotFoundError("not found")));
-
-  // For get field, implementation may return an error-type cel value or a
-  // status (e.g. broken assumption using a core type).
-  ASSERT_OK_AND_ASSIGN(CelValue result,
-                       RunExpression(value, "message_value",
-                                     /*test=*/false,
-                                     /*unknown_path=*/"", options));
-  EXPECT_THAT(result, test::IsCelError(StatusIs(absl::StatusCode::kInternal)));
-
-  // testonly select (has) errors are coerced to CelError.
-  ASSERT_OK_AND_ASSIGN(result, RunExpression(value, "message_value",
-                                             /*test=*/true,
-                                             /*unknown_path=*/"", options));
-
-  EXPECT_THAT(result, test::IsCelError(StatusIs(absl::StatusCode::kNotFound)));
-}
-
 TEST_P(SelectStepConformanceTest, SimpleEnumTest) {
   TestMessage message;
   message.set_enum_value(TestMessage::TEST_ENUM_1);

eval/public/structs/BUILD

@@ -272,14 +272,9 @@ cc_test(
     srcs = ["legacy_type_adapter_test.cc"],
     deps = [
         ":legacy_type_adapter",
-        ":trivial_legacy_type_info",
-        "//eval/public:cel_value",
-        "//eval/public/testing:matchers",
+        ":proto_message_type_adapter",
         "//eval/testutil:test_message_cc_proto",
-        "//extensions/protobuf:memory_manager",
-        "//internal:status_macros",
         "//internal:testing",
-        "@com_google_protobuf//:protobuf",
     ],
 )
 
@@ -301,15 +296,13 @@ cc_library(
         "//eval/public/containers:internal_field_backed_map_impl",
         "//extensions/protobuf:memory_manager",
         "//extensions/protobuf/internal:qualify",
-        "//internal:casts",
         "//internal:status_macros",
         "@com_google_absl//absl/base:no_destructor",
         "@com_google_absl//absl/base:nullability",
         "@com_google_absl//absl/log:absl_check",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/strings",
-        "@com_google_absl//absl/types:optional",
         "@com_google_absl//absl/types:span",
         "@com_google_protobuf//:differencer",
         "@com_google_protobuf//:protobuf",
@@ -386,7 +379,6 @@ cc_library(
     deps = [
         "//eval/public:message_wrapper",
         "@com_google_absl//absl/base:nullability",
-        "@com_google_absl//absl/status",
         "@com_google_absl//absl/strings:string_view",
         "@com_google_protobuf//:protobuf",
     ],
@@ -418,11 +410,14 @@ cc_test(
     name = "legacy_type_provider_test",
     srcs = ["legacy_type_provider_test.cc"],
     deps = [
+        ":legacy_type_adapter",
         ":legacy_type_info_apis",
         ":legacy_type_provider",
+        ":proto_message_type_adapter",
+        ":trivial_legacy_type_info",
         "//common:type",
+        "//eval/testutil:test_message_cc_proto",
         "//internal:testing",
-        "@com_google_absl//absl/status:status_matchers",
         "@com_google_absl//absl/strings:string_view",
     ],
 )

eval/public/structs/legacy_type_adapter.h

@@ -32,6 +32,10 @@
 
 namespace google::api::expr::runtime {
 
+// Forward declare permitted subclasses.
+class DucktypedMessageAdapter;
+class ProtoMessageTypeAdapter;
+
 // Interface for mutation apis.
 // Note: in the new type system, a type provider represents this by returning
 // a cel::Type and cel::ValueManager for the type.
@@ -71,6 +75,13 @@ class LegacyTypeMutationApis {
       CelValue::MessageWrapper::Builder& instance [[maybe_unused]]) const {
     return absl::UnimplementedError("SetFieldByNumber is not yet implemented");
   }
+
+ private:
+  // This class should only be implemented by CEL. Custom structs are only
+  // supported using the cel::Value APIs.
+  friend class ProtoMessageTypeAdapter;
+
+  LegacyTypeMutationApis() = default;
 };
 
 // Interface for access apis.
@@ -138,6 +149,14 @@ class LegacyTypeAccessApis {
 
   virtual std::vector<absl::string_view> ListFields(
       const CelValue::MessageWrapper& instance) const = 0;
+
+ private:
+  // This class should only be implemented by CEL. Custom structs are only
+  // supported using the cel::Value APIs.
+  friend class DucktypedMessageAdapter;
+  friend class ProtoMessageTypeAdapter;
+
+  LegacyTypeAccessApis() = default;
 };
 
 // Type information about a legacy Struct type.

eval/public/structs/legacy_type_adapter_test.cc

@@ -14,49 +14,19 @@
 
 #include "eval/public/structs/legacy_type_adapter.h"
 
-#include <vector>
-
-#include "eval/public/cel_value.h"
-#include "eval/public/structs/trivial_legacy_type_info.h"
-#include "eval/public/testing/matchers.h"
+#include "eval/public/structs/proto_message_type_adapter.h"
 #include "eval/testutil/test_message.pb.h"
-#include "extensions/protobuf/memory_manager.h"
-#include "internal/status_macros.h"
 #include "internal/testing.h"
 
 namespace google::api::expr::runtime {
 namespace {
 
-class TestAccessApiImpl : public LegacyTypeAccessApis {
- public:
-  TestAccessApiImpl() {}
-  absl::StatusOr<bool> HasField(
-      absl::string_view field_name,
-      const CelValue::MessageWrapper& value) const override {
-    return absl::UnimplementedError("Not implemented");
-  }
-
-  absl::StatusOr<CelValue> GetField(
-      absl::string_view field_name, const CelValue::MessageWrapper& instance,
-      ProtoWrapperTypeOptions unboxing_option,
-      cel::MemoryManagerRef memory_manager) const override {
-    return absl::UnimplementedError("Not implemented");
-  }
-
-  std::vector<absl::string_view> ListFields(
-      const CelValue::MessageWrapper& instance) const override {
-    return std::vector<absl::string_view>();
-  }
-};
-
-TEST(LegacyTypeAdapterAccessApis, DefaultAlwaysInequal) {
-  TestMessage message;
-  MessageWrapper wrapper(&message, nullptr);
-  MessageWrapper wrapper2(&message, nullptr);
-
-  TestAccessApiImpl impl;
+TEST(LegacyTypeAdapter, Basic) {
+  ProtoMessageTypeAdapter adapter(TestMessage::descriptor(), nullptr);
+  LegacyTypeAdapter type_adapter(&adapter, &adapter);
 
-  EXPECT_FALSE(impl.IsEqualTo(wrapper, wrapper2));
+  EXPECT_EQ(type_adapter.access_apis(), &adapter);
+  EXPECT_EQ(type_adapter.mutation_apis(), &adapter);
 }
 
 }  // namespace

eval/public/structs/legacy_type_info_apis.h

@@ -15,10 +15,10 @@
 #ifndef THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_STRUCTS_LEGACY_TYPE_INFO_APIS_H_
 #define THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_STRUCTS_LEGACY_TYPE_INFO_APIS_H_
 
+#include <optional>
 #include <string>
 
 #include "absl/base/nullability.h"
-#include "absl/status/status.h"
 #include "absl/strings/string_view.h"
 #include "eval/public/message_wrapper.h"
 #include "google/protobuf/descriptor.h"
@@ -29,6 +29,11 @@ namespace google::api::expr::runtime {
 class LegacyTypeAccessApis;
 class LegacyTypeMutationApis;
 
+// Forward declare permitted subclasses.
+class DucktypedMessageAdapter;
+class ProtoMessageTypeAdapter;
+class TrivialTypeInfo;
+
 // Interface for providing type info from a user defined type (represented as a
 // message).
 //
@@ -97,10 +102,19 @@ class LegacyTypeInfoApis {
   //
   // The underlying string is expected to remain valid as long as the
   // LegacyTypeInfoApis instance.
-  virtual absl::optional<FieldDescription> FindFieldByName(
+  virtual std::optional<FieldDescription> FindFieldByName(
       absl::string_view name [[maybe_unused]]) const {
-    return absl::nullopt;
+    return std::nullopt;
   }
+
+ private:
+  // This class should only be implemented by CEL. Custom structs are only
+  // supported using the cel::Value APIs.
+  friend class DucktypedMessageAdapter;
+  friend class ProtoMessageTypeAdapter;
+  friend class TrivialTypeInfo;
+
+  LegacyTypeInfoApis() = default;
 };
 
 }  // namespace google::api::expr::runtime

eval/public/structs/legacy_type_provider.h

@@ -15,11 +15,12 @@
 #ifndef THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_STRUCTS_TYPE_PROVIDER_H_
 #define THIRD_PARTY_CEL_CPP_EVAL_PUBLIC_STRUCTS_TYPE_PROVIDER_H_
 
+#include <optional>
+
 #include "absl/base/attributes.h"
 #include "absl/base/nullability.h"
 #include "absl/status/statusor.h"
 #include "absl/strings/string_view.h"
-#include "absl/types/optional.h"
 #include "common/type.h"
 #include "common/type_reflector.h"
 #include "common/value.h"
@@ -45,7 +46,7 @@ class LegacyTypeProvider : public cel::TypeReflector {
   // Returned non-null pointers from the adapter implemententation must remain
   // valid as long as the type provider.
   // TODO(uncreated-issue/3): add alternative for new type system.
-  virtual absl::optional<LegacyTypeAdapter> ProvideLegacyType(
+  virtual std::optional<LegacyTypeAdapter> ProvideLegacyType(
       absl::string_view name) const = 0;
 
   // Return LegacyTypeInfoApis for the fully qualified type name if available.
@@ -55,9 +56,9 @@ class LegacyTypeProvider : public cel::TypeReflector {
   // Since custom type providers should create values compatible with evaluator
   // created ones, the TypeInfoApis returned from this method should be the same
   // as the ones used in value creation.
-  virtual absl::optional<const LegacyTypeInfoApis*> ProvideLegacyTypeInfo(
+  virtual std::optional<const LegacyTypeInfoApis*> ProvideLegacyTypeInfo(
       ABSL_ATTRIBUTE_UNUSED absl::string_view name) const {
-    return absl::nullopt;
+    return std::nullopt;
   }
 
   absl::StatusOr<absl_nullable cel::ValueBuilderPtr> NewValueBuilder(
@@ -66,10 +67,10 @@ class LegacyTypeProvider : public cel::TypeReflector {
       google::protobuf::Arena* absl_nonnull arena) const final;
 
  protected:
-  absl::StatusOr<absl::optional<cel::Type>> FindTypeImpl(
+  absl::StatusOr<std::optional<cel::Type>> FindTypeImpl(
       absl::string_view name) const final;
 
-  absl::StatusOr<absl::optional<cel::StructTypeField>>
+  absl::StatusOr<std::optional<cel::StructTypeField>>
   FindStructTypeFieldByNameImpl(absl::string_view type,
                                 absl::string_view name) const final;
 };