Migrate AllowSkipThriftCow checks to Thrift always-on reflection, structured annotation

Summary:
Migrate templates for checking for *allow skip Thrift cow* annotation to use always-on reflection, and look for the equivalent structured annotation.

The previous implementation used Thrift legacy reflection to check for an unstructured annotation.

Reviewed By: iahs

Differential Revision: D90624675

fbshipit-source-id: ee72a666b2de291bdfab4d93da4b4cb21b1a521f

Author: hchokshi

fboss/thrift_cow/nodes/BUCK

@@ -52,6 +52,7 @@ cpp_library(
         ":node_utils",
         ":serializer",
         "//fatal:fatal",
+        "//fboss/agent/if:common-cpp2-types",
         "//fboss/agent/state:nodebase",
         "//fboss/fsdb/if:fsdb_oper-cpp2-types",
         "//fboss/thrift_cow/visitors:visitors",
@@ -60,6 +61,7 @@ cpp_library(
         "//folly:fbstring",
         "//folly/json:dynamic",
         "//thrift/lib/cpp2/folly_dynamic:folly_dynamic",
+        "//thrift/lib/cpp2/gen:module_types_h",
         "//thrift/lib/cpp2/protocol:protocol",
         "//thrift/lib/cpp2/protocol/detail:protocol_methods",
         "//thrift/lib/cpp2/reflection:reflection",

fboss/thrift_cow/nodes/ThriftStructNode-inl.h

@@ -169,8 +169,9 @@ struct ThriftStructFields : public FieldBaseType {
   using Members = typename Info::members;
 
   // Extracting useful common types out of each member via Traits.h
-  using MemberTypes = fatal::
-      transform<Members, ExtractStructFields<Derived, EnableHybridStorage>>;
+  using MemberTypes = fatal::transform<
+      Members,
+      ExtractStructFields<TType, Derived, EnableHybridStorage>>;
 
   // type list of members with SkipThriftCow enabled
   using MemberTypesWithSkipThriftCow =

fboss/thrift_cow/nodes/Traits.h

@@ -10,59 +10,28 @@
 
 #pragma once
 
+#include <fboss/agent/if/gen-cpp2/common_types.h>
 #include <fboss/thrift_cow/nodes/Types.h>
-#include <thrift/lib/cpp2/reflection/reflection.h>
+#include <thrift/lib/cpp2/gen/module_types_h.h>
 #include <type_traits>
 
 namespace facebook::fboss::thrift_cow {
 
-// helper struct to read Thrift annotation allow_skip_thrift_cow
-template <typename T, typename T2 = void>
-struct read_annotation_allow_skip_thrift_cow {
-  static constexpr bool value = false;
-};
-
-// fatal respsents true and false as
-// "constexpr" char sequence of "1" and "0", respectively
-using fatal_true = fatal::sequence<char, '1'>;
-
-// need a little template specialization magic since annotation values are void
-// when nothing is set. without this we can't try to pull out
-// annotation allow_skip_thrift_cow on structs that don't have annotatiosn
-template <>
-struct read_annotation_allow_skip_thrift_cow<void> {
-  static constexpr bool value = false;
-};
-
-FATAL_S(allow_skip_thrift_cow_annotation, "allow_skip_thrift_cow");
-
-template <typename Annotations>
-struct read_annotation_allow_skip_thrift_cow<
-    Annotations,
-    typename std::enable_if_t<std::is_same_v<
-        typename Annotations::keys::allow_skip_thrift_cow,
-        allow_skip_thrift_cow_annotation>>> {
-  static constexpr bool value = std::is_same<
-      typename Annotations::values::allow_skip_thrift_cow,
-      fatal_true>::value;
-};
-
-template <typename TC, typename TType, typename = void>
-struct read_type_annotation_allow_skip_thrift_cow {
-  static constexpr bool value = false;
-};
-
-template <typename TC, typename TType>
-struct read_type_annotation_allow_skip_thrift_cow<
-    TC,
-    TType,
-    typename std::enable_if_t<
-        std::is_same_v<TC, apache::thrift::type_class::structure>>> {
-  using annotations =
-      typename apache::thrift::reflect_struct<TType>::annotations;
-  static constexpr bool value =
-      read_annotation_allow_skip_thrift_cow<annotations>::value;
-};
+// Concept to check if a Thrift field is annotated with AllowSkipThriftCow at
+// compile time
+template <typename TStruct, typename FieldId>
+concept field_allow_skip_thrift_cow = apache::thrift::is_thrift_class_v<
+                                          TStruct> &&
+    apache::thrift::has_field_annotation<facebook::fboss::AllowSkipThriftCow,
+                                         TStruct,
+                                         FieldId>();
+
+// Concept to check if a Thrift type is annotated with AllowSkipThriftCow at
+// compile time
+template <typename TType>
+concept type_allow_skip_thrift_cow = apache::thrift::is_thrift_class_v<TType> &&
+    apache::thrift::
+        has_struct_annotation<facebook::fboss::AllowSkipThriftCow, TType>();
 
 template <typename TType, bool EnableHybridStorage = false>
 struct ThriftStructResolver {
@@ -125,10 +94,7 @@ struct ConvertToNodeTraits<
     apache::thrift::type_class::structure,
     TType> {
   static constexpr bool skipThriftCow =
-      EnableHybridStorage &&
-      read_type_annotation_allow_skip_thrift_cow<
-          apache::thrift::type_class::structure,
-          TType>::value;
+      EnableHybridStorage && type_allow_skip_thrift_cow<TType>;
   using default_type = std::conditional_t<
       skipThriftCow,
       ThriftHybridNode<apache::thrift::type_class::structure, TType>,
@@ -232,19 +198,25 @@ struct ConvertToImmutableNodeTraits {
 template <typename Derived, typename Name>
 struct ResolveMemberType : std::false_type {};
 
-template <typename Derived, typename Member, bool EnableHybridStorage>
+template <
+    typename Struct,
+    typename Derived,
+    typename Member,
+    bool EnableHybridStorage>
 struct StructMemberTraits {
   using member = Member;
-  using traits = StructMemberTraits<Derived, Member, EnableHybridStorage>;
+  using traits =
+      StructMemberTraits<Struct, Derived, Member, EnableHybridStorage>;
   using name = typename Member::name;
   using ttype = typename Member::type;
   using tc = typename Member::type_class;
 
   // read member annotations
-  using member_annotations = typename Member::annotations;
   static constexpr bool allowSkipThriftCow = EnableHybridStorage &&
-      (read_annotation_allow_skip_thrift_cow<member_annotations>::value ||
-       read_type_annotation_allow_skip_thrift_cow<tc, ttype>::value);
+      (field_allow_skip_thrift_cow<
+           Struct,
+           apache::thrift::field_id<Member::id::value>> ||
+       type_allow_skip_thrift_cow<ttype>);
 
   // need to resolve here
   using default_type = std::conditional_t<
@@ -263,10 +235,10 @@ struct StructMemberTraits {
       default_type>;
 };
 
-template <typename Derived, bool EnableHybridStorage>
+template <typename Struct, typename Derived, bool EnableHybridStorage>
 struct ExtractStructFields {
   template <typename T>
-  using apply = StructMemberTraits<Derived, T, EnableHybridStorage>;
+  using apply = StructMemberTraits<Struct, Derived, T, EnableHybridStorage>;
 };
 
 template <typename Member, bool EnableHybridStorage = false>

fboss/thrift_cow/nodes/tests/BUCK

@@ -32,8 +32,6 @@ cpp_unittest(
         "//fboss/thrift_cow/nodes:serializer",
         "//thrift/lib/cpp2/folly_dynamic:folly_dynamic",
         "//thrift/lib/cpp2/protocol:protocol",
-        "//thrift/lib/cpp2/reflection:reflection",
-        "//thrift/lib/cpp2/reflection:testing",
     ],
 )
 

fboss/thrift_cow/nodes/tests/ThriftHybridStructNodeTests.cpp

@@ -10,8 +10,6 @@
 
 #include <thrift/lib/cpp2/folly_dynamic/folly_dynamic.h>
 #include <thrift/lib/cpp2/protocol/Serializer.h>
-#include <thrift/lib/cpp2/reflection/reflection.h>
-#include <thrift/lib/cpp2/reflection/testing.h>
 #include "fboss/thrift_cow/nodes/Serializer.h"
 #include "fboss/thrift_cow/nodes/Types.h"
 #include "fboss/thrift_cow/nodes/tests/gen-cpp2/test_fatal_types.h"
@@ -27,10 +25,8 @@ using k = test_tags::strings;
 
 template <typename ThriftType, bool EnableHybridStorage>
 struct is_allow_skip_thrift_cow {
-  using annotations = apache::thrift::reflect_struct<ThriftType>::annotations;
   static constexpr bool value =
-      read_annotation_allow_skip_thrift_cow<annotations>::value &&
-      EnableHybridStorage;
+      type_allow_skip_thrift_cow<ThriftType> && EnableHybridStorage;
 };
 
 struct TestParams {

fboss/thrift_cow/nodes/tests/ThriftStructNodeTests.cpp

@@ -11,8 +11,6 @@
 #include <fboss/thrift_cow/visitors/tests/VisitorTestUtils.h>
 #include <thrift/lib/cpp2/folly_dynamic/folly_dynamic.h>
 #include <thrift/lib/cpp2/protocol/Serializer.h>
-#include <thrift/lib/cpp2/reflection/reflection.h>
-#include <thrift/lib/cpp2/reflection/testing.h>
 #include "fboss/fsdb/if/gen-cpp2/fsdb_oper_types.h"
 #include "fboss/thrift_cow/nodes/Serializer.h"
 #include "fboss/thrift_cow/nodes/Types.h"
@@ -38,18 +36,9 @@ cfg::L4PortRange buildPortRange(int min, int max) {
 } // namespace
 
 TEST(ThriftStructNodeTests, ReadThriftStructAnnotation) {
-  static_assert(
-      read_type_annotation_allow_skip_thrift_cow<
-          apache::thrift::type_class::structure,
-          TestStruct3>::value == true);
-  static_assert(
-      read_type_annotation_allow_skip_thrift_cow<
-          apache::thrift::type_class::structure,
-          ParentTestStruct>::value == false);
-  static_assert(
-      read_type_annotation_allow_skip_thrift_cow<
-          apache::thrift::type_class::integral,
-          int>::value == false);
+  static_assert(type_allow_skip_thrift_cow<TestStruct3> == true);
+  static_assert(type_allow_skip_thrift_cow<ParentTestStruct> == false);
+  static_assert(type_allow_skip_thrift_cow<int> == false);
 }
 
 TEST(ThriftStructNodeTests, ThriftStructFieldsSimple) {