Add static asserts to enforce more assumptions made in repeated field containers.

PiperOrigin-RevId: 501897045

src/google/protobuf/arena_unittest.cc

@@ -1369,25 +1369,6 @@ TEST(ArenaTest, MessageLiteOnArena) {
 }
 #endif  // PROTOBUF_RTTI
 
-// RepeatedField should support non-POD types, and invoke constructors and
-// destructors appropriately, because it's used this way by lots of other code
-// (even if this was not its original intent).
-TEST(ArenaTest, RepeatedFieldWithNonPODType) {
-  {
-    RepeatedField<std::string> field_on_heap;
-    for (int i = 0; i < 100; i++) {
-      *field_on_heap.Add() = "test string long enough to exceed inline buffer";
-    }
-  }
-  {
-    Arena arena;
-    RepeatedField<std::string> field_on_arena(&arena);
-    for (int i = 0; i < 100; i++) {
-      *field_on_arena.Add() = "test string long enough to exceed inline buffer";
-    }
-  }
-}
-
 // Align n to next multiple of 8
 uint64_t Align8(uint64_t n) { return (n + 7) & -8; }
 

src/google/protobuf/repeated_field.h

@@ -55,6 +55,7 @@
 #include "google/protobuf/port.h"
 #include "google/protobuf/stubs/logging.h"
 #include "absl/strings/cord.h"
+#include "google/protobuf/generated_enum_util.h"
 #include "google/protobuf/message_lite.h"
 #include "google/protobuf/port.h"
 #include "google/protobuf/repeated_ptr_field.h"
@@ -158,6 +159,16 @@ class RepeatedField final {
   static_assert(!std::is_reference<Element>::value,
                 "We do not support reference value types.");
 #endif  // !PROTOBUF_FUTURE_CONTAINER_STATIC_ASSERTS
+  static constexpr PROTOBUF_ALWAYS_INLINE void StaticValidityCheck() {
+#ifdef PROTOBUF_FUTURE_CONTAINER_STATIC_ASSERTS
+    static_assert(
+        absl::disjunction<internal::is_supported_integral_type<Element>,
+                          internal::is_supported_floating_point_type<Element>,
+                          std::is_same<absl::Cord, Element>,
+                          is_proto_enum<Element>>::value,
+        "We only support non-string scalars in RepeatedField.");
+#endif  // PROTOBUF_FUTURE_CONTAINER_STATIC_ASSERTS
+  }
 
  public:
   constexpr RepeatedField();
@@ -534,15 +545,20 @@ struct ElementCopier {
 
 template <typename Element>
 constexpr RepeatedField<Element>::RepeatedField()
-    : current_size_(0), total_size_(0), arena_or_elements_(nullptr) {}
+    : current_size_(0), total_size_(0), arena_or_elements_(nullptr) {
+  StaticValidityCheck();
+}
 
 template <typename Element>
 inline RepeatedField<Element>::RepeatedField(Arena* arena)
-    : current_size_(0), total_size_(0), arena_or_elements_(arena) {}
+    : current_size_(0), total_size_(0), arena_or_elements_(arena) {
+  StaticValidityCheck();
+}
 
 template <typename Element>
 inline RepeatedField<Element>::RepeatedField(const RepeatedField& other)
     : current_size_(0), total_size_(0), arena_or_elements_(nullptr) {
+  StaticValidityCheck();
   if (other.current_size_ != 0) {
     Reserve(other.size());
     AddNAlreadyReserved(other.size());
@@ -554,11 +570,15 @@ template <typename Element>
 template <typename Iter, typename>
 RepeatedField<Element>::RepeatedField(Iter begin, Iter end)
     : current_size_(0), total_size_(0), arena_or_elements_(nullptr) {
+  StaticValidityCheck();
   Add(begin, end);
 }
 
 template <typename Element>
 RepeatedField<Element>::~RepeatedField() {
+  // Fail-safe in case we miss calling this in a constructor.  Note: this one
+  // won't trigger for leaked maps that never get destructed.
+  StaticValidityCheck();
 #ifndef NDEBUG
   // Try to trigger segfault / asan failure in non-opt builds if arena_
   // lifetime has ended before the destructor.

src/google/protobuf/repeated_field_unittest.cc

@@ -197,11 +197,8 @@ TEST(RepeatedField, ArenaAllocationSizesMatchExpectedValues) {
   // memory.
   // If the allocation size is wrong, ReturnArrayMemory will GOOGLE_ABSL_DCHECK.
   CheckAllocationSizes<RepeatedField<bool>>(false);
-  CheckAllocationSizes<RepeatedField<uint8_t>>(false);
-  CheckAllocationSizes<RepeatedField<uint16_t>>(false);
   CheckAllocationSizes<RepeatedField<uint32_t>>(false);
   CheckAllocationSizes<RepeatedField<uint64_t>>(false);
-  CheckAllocationSizes<RepeatedField<std::pair<uint64_t, uint64_t>>>(false);
 }
 
 template <typename Rep>
@@ -1062,15 +1059,32 @@ TEST(RepeatedField, ExtractSubrange) {
   }
 }
 
-TEST(RepeatedField, ClearThenReserveMore) {
+TEST(RepeatedField, TestSAddFromSelf) {
+  RepeatedField<int> field;
+  field.Add(0);
+  for (int i = 0; i < 1000; i++) {
+    field.Add(field[0]);
+  }
+}
+
+// ===================================================================
+// RepeatedPtrField tests.  These pretty much just mirror the RepeatedField
+// tests above.
+
+TEST(RepeatedPtrField, ConstInit) {
+  PROTOBUF_CONSTINIT static RepeatedPtrField<std::string> field{};  // NOLINT
+  EXPECT_TRUE(field.empty());
+}
+
+TEST(RepeatedPtrField, ClearThenReserveMore) {
   // Test that Reserve properly destroys the old internal array when it's forced
   // to allocate a new one, even when cleared-but-not-deleted objects are
   // present. Use a 'string' and > 16 bytes length so that the elements are
   // non-POD and allocate -- the leak checker will catch any skipped destructor
   // calls here.
-  RepeatedField<std::string> field;
+  RepeatedPtrField<std::string> field;
   for (int i = 0; i < 32; i++) {
-    field.Add(std::string("abcdefghijklmnopqrstuvwxyz0123456789"));
+    *field.Add() = std::string("abcdefghijklmnopqrstuvwxyz0123456789");
   }
   EXPECT_EQ(32, field.size());
   field.Clear();
@@ -1084,23 +1098,6 @@ TEST(RepeatedField, ClearThenReserveMore) {
   // strings.
 }
 
-TEST(RepeatedField, TestSAddFromSelf) {
-  RepeatedField<int> field;
-  field.Add(0);
-  for (int i = 0; i < 1000; i++) {
-    field.Add(field[0]);
-  }
-}
-
-// ===================================================================
-// RepeatedPtrField tests.  These pretty much just mirror the RepeatedField
-// tests above.
-
-TEST(RepeatedPtrField, ConstInit) {
-  PROTOBUF_CONSTINIT static RepeatedPtrField<std::string> field{};  // NOLINT
-  EXPECT_TRUE(field.empty());
-}
-
 // This helper overload set tests whether X::f can be called with a braced pair,
 // X::f({a, b}) of std::string iterators (specifically, pointers: That call is
 // ambiguous if and only if the call to ValidResolutionPointerRange is not.

src/google/protobuf/repeated_ptr_field.h

@@ -912,6 +912,15 @@ class RepeatedPtrField final : private internal::RepeatedPtrFieldBase {
   static_assert(!std::is_reference<Element>::value,
                 "We do not support reference value types.");
 #endif  // !PROTOBUF_FUTURE_CONTAINER_STATIC_ASSERTS
+  static constexpr PROTOBUF_ALWAYS_INLINE void StaticValidityCheck() {
+#ifdef PROTOBUF_FUTURE_CONTAINER_STATIC_ASSERTS
+    static_assert(
+        absl::disjunction<
+            internal::is_supported_string_type<Element>,
+            internal::is_supported_message_type<Element>>::value,
+        "We only support string and Message types in RepeatedPtrField.");
+#endif  // PROTOBUF_FUTURE_CONTAINER_STATIC_ASSERTS
+  }
 
  public:
   constexpr RepeatedPtrField();
@@ -1224,27 +1233,34 @@ class RepeatedPtrField<std::string>::TypeHandler
 
 template <typename Element>
 constexpr RepeatedPtrField<Element>::RepeatedPtrField()
-    : RepeatedPtrFieldBase() {}
+    : RepeatedPtrFieldBase() {
+  StaticValidityCheck();
+}
 
 template <typename Element>
 inline RepeatedPtrField<Element>::RepeatedPtrField(Arena* arena)
-    : RepeatedPtrFieldBase(arena) {}
+    : RepeatedPtrFieldBase(arena) {
+  StaticValidityCheck();
+}
 
 template <typename Element>
 inline RepeatedPtrField<Element>::RepeatedPtrField(
     const RepeatedPtrField& other)
     : RepeatedPtrFieldBase() {
+  StaticValidityCheck();
   MergeFrom(other);
 }
 
 template <typename Element>
 template <typename Iter, typename>
 inline RepeatedPtrField<Element>::RepeatedPtrField(Iter begin, Iter end) {
+  StaticValidityCheck();
   Add(begin, end);
 }
 
 template <typename Element>
 RepeatedPtrField<Element>::~RepeatedPtrField() {
+  StaticValidityCheck();
 #ifdef __cpp_if_constexpr
   if constexpr (std::is_base_of<MessageLite, Element>::value) {
 #else