Add static asserts to enforce assumptions made in Map.

PiperOrigin-RevId: 499334862

cmake/abseil-cpp.cmake

@@ -64,6 +64,7 @@ else()
     absl::strings
     absl::synchronization
     absl::time
+    absl::type_traits
     absl::utility
     absl::variant
   )

src/google/protobuf/BUILD.bazel

@@ -178,6 +178,9 @@ cc_library(
         "//:__subpackages__",
         "//src/google/protobuf:__subpackages__",
     ],
+    deps = [
+        "@com_google_absl//absl/meta:type_traits",
+    ],
 )
 
 cc_library(
@@ -334,6 +337,7 @@ cc_library(
         "//src/google/protobuf/stubs:lite",
         "@com_google_absl//absl/container:btree",
         "@com_google_absl//absl/container:flat_hash_set",
+        "@com_google_absl//absl/meta:type_traits",
         "@com_google_absl//absl/numeric:bits",
         "@com_google_absl//absl/strings:internal",
         "@com_google_absl//absl/synchronization",

src/google/protobuf/arena.h

@@ -47,7 +47,7 @@ using type_info = ::type_info;
 #include <typeinfo>
 #endif
 
-#include <type_traits>
+#include "absl/meta/type_traits.h"
 #include "google/protobuf/arena_align.h"
 #include "google/protobuf/arena_config.h"
 #include "google/protobuf/port.h"

src/google/protobuf/map.h

@@ -55,8 +55,9 @@
 #endif
 
 #include "google/protobuf/stubs/common.h"
-#include "google/protobuf/arena.h"
 #include "absl/container/btree_map.h"
+#include "absl/meta/type_traits.h"
+#include "google/protobuf/arena.h"
 #include "google/protobuf/generated_enum_util.h"
 #include "google/protobuf/map_type_handler.h"
 #include "google/protobuf/port.h"
@@ -98,6 +99,14 @@ class DynamicMapField;
 
 class GeneratedMessageReflection;
 
+// Internal type traits that can be used to define custom key/value types. These
+// are only be specialized by protobuf internals, and never by users.
+template <typename T, typename VoidT = void>
+struct is_internal_map_key_type : std::false_type {};
+
+template <typename T, typename VoidT = void>
+struct is_internal_map_value_type : std::false_type {};
+
 // re-implement std::allocator to use arena allocator for memory allocation.
 // Used for Map implementation. Users should not use this class
 // directly.
@@ -987,8 +996,8 @@ class Map {
   using size_type = size_t;
   using hasher = typename internal::TransparentSupport<Key>::hash;
 
-  constexpr Map() : elements_(nullptr) {}
-  explicit Map(Arena* arena) : elements_(arena) {}
+  constexpr Map() : elements_(nullptr) { StaticValidityCheck(); }
+  explicit Map(Arena* arena) : elements_(arena) { StaticValidityCheck(); }
 
   Map(const Map& other) : Map() { insert(other.begin(), other.end()); }
 
@@ -1016,9 +1025,47 @@ class Map {
     insert(first, last);
   }
 
-  ~Map() {}
+  ~Map() {
+    // 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();
+  }
 
  private:
+#ifdef PROTOBUF_FUTURE_CONTAINER_STATIC_ASSERTS
+  static_assert(!std::is_const<mapped_type>::value &&
+                    !std::is_const<key_type>::value,
+                "We do not support const types.");
+  static_assert(!std::is_volatile<mapped_type>::value &&
+                    !std::is_volatile<key_type>::value,
+                "We do not support volatile types.");
+  static_assert(!std::is_pointer<mapped_type>::value &&
+                    !std::is_pointer<key_type>::value,
+                "We do not support pointer types.");
+  static_assert(!std::is_reference<mapped_type>::value &&
+                    !std::is_reference<key_type>::value,
+                "We do not support reference types.");
+#endif  // PROTOBUF_FUTURE_CONTAINER_STATIC_ASSERTS
+  static constexpr PROTOBUF_ALWAYS_INLINE void StaticValidityCheck() {
+#ifdef PROTOBUF_FUTURE_CONTAINER_STATIC_ASSERTS
+    static_assert(alignof(internal::NodeBase) >= alignof(mapped_type),
+                  "Alignment of mapped type is too high.");
+    static_assert(
+        absl::disjunction<internal::is_supported_integral_type<key_type>,
+                          internal::is_supported_string_type<key_type>,
+                          internal::is_internal_map_key_type<key_type>>::value,
+        "We only support integer, string, or designated internal key "
+        "types.");
+    static_assert(absl::disjunction<
+                      internal::is_supported_scalar_type<mapped_type>,
+                      is_proto_enum<mapped_type>,
+                      internal::is_supported_message_type<mapped_type>,
+                      internal::is_internal_map_value_type<mapped_type>>::value,
+                  "We only support scalar, Message, and designated internal "
+                  "mapped types.");
+#endif  // PROTOBUF_FUTURE_CONTAINER_STATIC_ASSERTS
+  }
+
   template <typename P>
   struct SameAsElementReference
       : std::is_same<typename std::remove_cv<

src/google/protobuf/map_field.h

@@ -33,6 +33,7 @@
 
 #include <atomic>
 #include <functional>
+#include <type_traits>
 
 #include "google/protobuf/arena.h"
 #include "google/protobuf/port.h"
@@ -274,6 +275,11 @@ class PROTOBUF_EXPORT MapKey {
   FieldDescriptor::CppType type_;
 };
 
+namespace internal {
+template <>
+struct is_internal_map_key_type<MapKey> : std::true_type {};
+}  // namespace internal
+
 }  // namespace protobuf
 }  // namespace google
 namespace std {
@@ -937,6 +943,13 @@ class PROTOBUF_EXPORT MapIterator {
   MapValueRef value_;
 };
 
+namespace internal {
+template <>
+struct is_internal_map_value_type<class MapValueConstRef> : std::true_type {};
+template <>
+struct is_internal_map_value_type<class MapValueRef> : std::true_type {};
+}  // namespace internal
+
 }  // namespace protobuf
 }  // namespace google
 

src/google/protobuf/map_test.cc

@@ -28,6 +28,8 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+#include <type_traits>
+
 #include "absl/container/flat_hash_set.h"
 #include "google/protobuf/map_proto2_unittest.pb.h"
 #include "google/protobuf/map_unittest.pb.h"
@@ -54,8 +56,6 @@
 namespace google {
 namespace protobuf {
 namespace internal {
-namespace {
-
 
 struct AlignedAsDefault {
   int x;
@@ -64,6 +64,14 @@ struct alignas(8) AlignedAs8 {
   int x;
 };
 
+template <>
+struct is_internal_map_value_type<AlignedAsDefault> : std::true_type {};
+template <>
+struct is_internal_map_value_type<AlignedAs8> : std::true_type {};
+
+namespace {
+
+
 template <typename Aligned, bool on_arena = false>
 void MapTest_Aligned() {
   Arena arena;

src/google/protobuf/map_test.inc

@@ -90,6 +90,91 @@ void MapTestForceDeterministic() {
   io::CodedOutputStream::SetDefaultSerializationDeterministic();
 }
 
+struct MoveTestKey {
+  MoveTestKey(int data, int* copies) : data(data), copies(copies) {}
+
+  MoveTestKey(const MoveTestKey& other)
+      : data(other.data), copies(other.copies) {
+    ++*copies;
+  }
+
+  MoveTestKey(MoveTestKey&& other) noexcept
+      : data(other.data), copies(other.copies) {}
+
+  friend bool operator==(const MoveTestKey& lhs, const MoveTestKey& rhs) {
+    return lhs.data == rhs.data;
+  }
+  friend bool operator<(const MoveTestKey& lhs, const MoveTestKey& rhs) {
+    return lhs.data < rhs.data;
+  }
+
+  int data;
+  int* copies;
+};
+
+enum class ConstructorType {
+  kDefault,
+  kCopy,
+  kMove,
+};
+
+struct ConstructorTag {
+  ConstructorTag() : invoked_constructor(ConstructorType::kDefault) {}
+  ConstructorTag(const ConstructorTag&)
+      : invoked_constructor(ConstructorType::kCopy) {}
+  ConstructorTag(ConstructorTag&&)
+      : invoked_constructor(ConstructorType::kMove) {}
+
+  ConstructorType invoked_constructor;
+};
+
+struct CountedInstance {
+  CountedInstance() { ++num_created; }
+  CountedInstance(const CountedInstance&) : CountedInstance() {}
+  CountedInstance(CountedInstance&&) : CountedInstance() {}
+
+  CountedInstance& operator=(const CountedInstance&) {
+    ++num_assigned;
+    return *this;
+  }
+
+  explicit CountedInstance(int x) : CountedInstance() {}
+
+  static int num_created;
+  static int num_assigned;
+};
+
+int CountedInstance::num_created = 0;
+int CountedInstance::num_assigned = 0;
+
+struct ArenaConstructible {
+  using InternalArenaConstructable_ = void;
+  using DestructorSkippable_ = void;
+
+  ArenaConstructible() = default;
+  ArenaConstructible(const ArenaConstructible&) = default;
+  ArenaConstructible(Arena*) : ArenaConstructible() {}
+
+  ArenaConstructible& operator=(const ArenaConstructible&) = default;
+
+  explicit ArenaConstructible(int) : ArenaConstructible() {}
+
+  Arena* arena() const { return nullptr; }
+
+  CountedInstance unused;
+};
+
+template <>
+struct is_internal_map_key_type<MoveTestKey> : std::true_type {};
+template <>
+struct is_internal_map_key_type<ConstructorTag> : std::true_type {};
+template <>
+struct is_internal_map_value_type<ConstructorTag> : std::true_type {};
+template <>
+struct is_internal_map_value_type<ArenaConstructible> : std::true_type {};
+template <>
+struct is_internal_map_value_type<CountedInstance> : std::true_type {};
+
 namespace {
 using internal::DownCast;
 
@@ -188,28 +273,6 @@ TEST_F(MapImplTest, OperatorBracket) {
   ExpectSingleElement(key, value2);
 }
 
-struct MoveTestKey {
-  MoveTestKey(int data, int* copies) : data(data), copies(copies) {}
-
-  MoveTestKey(const MoveTestKey& other)
-      : data(other.data), copies(other.copies) {
-    ++*copies;
-  }
-
-  MoveTestKey(MoveTestKey&& other) noexcept
-      : data(other.data), copies(other.copies) {}
-
-  friend bool operator==(const MoveTestKey& lhs, const MoveTestKey& rhs) {
-    return lhs.data == rhs.data;
-  }
-  friend bool operator<(const MoveTestKey& lhs, const MoveTestKey& rhs) {
-    return lhs.data < rhs.data;
-  }
-
-  int data;
-  int* copies;
-};
-
 }  // namespace
 }  // namespace internal
 }  // namespace protobuf
@@ -768,29 +831,6 @@ TEST_F(MapImplTest, EmplaceKeyOnly) {
 
 #else
 
-TEST_F(MapImplTest, ValueTypeNoImplicitConversion) {
-  using vt = typename Map<const char*, int>::value_type;
-
-  EXPECT_FALSE((std::is_convertible<
-                vt, std::pair<std::string, std::vector<std::string>>>::value));
-}
-
-enum class ConstructorType {
-  kDefault,
-  kCopy,
-  kMove,
-};
-
-struct ConstructorTag {
-  ConstructorTag() : invoked_constructor(ConstructorType::kDefault) {}
-  ConstructorTag(const ConstructorTag&)
-      : invoked_constructor(ConstructorType::kCopy) {}
-  ConstructorTag(ConstructorTag&&)
-      : invoked_constructor(ConstructorType::kMove) {}
-
-  ConstructorType invoked_constructor;
-};
-
 TEST_F(MapImplTest, ValueTypeHasMoveConstructor) {
   using vt = typename Map<ConstructorTag, ConstructorTag>::value_type;
   ConstructorTag l, r;
@@ -803,25 +843,6 @@ TEST_F(MapImplTest, ValueTypeHasMoveConstructor) {
 
 #endif  // !PROTOBUF_FUTURE_MAP_PAIR_UPGRADE
 
-struct CountedInstance {
-  CountedInstance() { ++num_created; }
-  CountedInstance(const CountedInstance&) : CountedInstance() {}
-  CountedInstance(CountedInstance&&) : CountedInstance() {}
-
-  CountedInstance& operator=(const CountedInstance&) {
-    ++num_assigned;
-    return *this;
-  }
-
-  explicit CountedInstance(int x) : CountedInstance() {}
-
-  static int num_created;
-  static int num_assigned;
-};
-
-int CountedInstance::num_created = 0;
-int CountedInstance::num_assigned = 0;
-
 TEST_F(MapImplTest, TryEmplaceExisting) {
   Map<int32_t, CountedInstance> m;
 
@@ -836,23 +857,6 @@ TEST_F(MapImplTest, TryEmplaceExisting) {
   EXPECT_EQ(CountedInstance::num_assigned, 0);
 }
 
-struct ArenaConstructible {
-  using InternalArenaConstructable_ = void;
-  using DestructorSkippable_ = void;
-
-  ArenaConstructible() = default;
-  ArenaConstructible(const ArenaConstructible&) = default;
-  ArenaConstructible(Arena*) : ArenaConstructible() {}
-
-  ArenaConstructible& operator=(const ArenaConstructible&) = default;
-
-  explicit ArenaConstructible(int) : ArenaConstructible() {}
-
-  Arena* arena() const { return nullptr; }
-
-  CountedInstance unused;
-};
-
 TEST_F(MapImplTest, TryEmplaceArenaConstructible) {
   ASSERT_TRUE(Arena::is_arena_constructable<ArenaConstructible>::value);
 

src/google/protobuf/port.h

@@ -39,15 +39,21 @@
 #include <cassert>
 #include <cstddef>
 #include <new>
+#include <string>
 #include <type_traits>
 
 
+#include "absl/meta/type_traits.h"
+
 // must be last
 #include "google/protobuf/port_def.inc"
 
 
 namespace google {
 namespace protobuf {
+
+class MessageLite;
+
 namespace internal {
 
 
@@ -110,6 +116,33 @@ inline ToRef DownCast(From& f) {
   return *static_cast<To*>(&f);
 }
 
+// Helpers for identifying our supported types.
+template <typename T>
+struct is_supported_integral_type
+    : absl::disjunction<std::is_same<T, int32_t>, std::is_same<T, uint32_t>,
+                        std::is_same<T, int64_t>, std::is_same<T, uint64_t>,
+                        std::is_same<T, bool>> {};
+
+template <typename T>
+struct is_supported_floating_point_type
+    : absl::disjunction<std::is_same<T, float>, std::is_same<T, double>> {};
+
+template <typename T>
+struct is_supported_string_type
+    : absl::disjunction<std::is_same<T, std::string>> {};
+
+template <typename T>
+struct is_supported_scalar_type
+    : absl::disjunction<is_supported_integral_type<T>,
+                        is_supported_floating_point_type<T>,
+                        is_supported_string_type<T>> {};
+
+template <typename T>
+struct is_supported_message_type
+    : absl::disjunction<std::is_base_of<MessageLite, T>> {
+  static constexpr auto force_complete_type = sizeof(T);
+};
+
 // To prevent sharing cache lines between threads
 #ifdef __cpp_aligned_new
 enum { kCacheAlignment = 64 };

src/google/protobuf/port_def.inc

@@ -204,6 +204,10 @@ static_assert(PROTOBUF_CPLUSPLUS_MIN(201402L), "Protobuf only supports C++14 and
 
 #ifdef PROTOBUF_FUTURE_BREAKING_CHANGES
 
+// Include static asserts to lock down assumptions about our containers.
+// Owner: mkruskal@
+#define PROTOBUF_FUTURE_CONTAINER_STATIC_ASSERTS 1
+
 // Used to upgrade google::protobuf::MapPair<K, V> to std::pair<const K, V>.
 // Owner: mordberg@
 #define PROTOBUF_FUTURE_MAP_PAIR_UPGRADE 1

src/google/protobuf/repeated_field.h

@@ -148,6 +148,16 @@ class RepeatedField final {
   static_assert(
       alignof(Arena) >= alignof(Element),
       "We only support types that have an alignment smaller than Arena");
+#ifndef PROTOBUF_FUTURE_CONTAINER_STATIC_ASSERTS
+  static_assert(!std::is_const<Element>::value,
+                "We do not support const value types.");
+  static_assert(!std::is_volatile<Element>::value,
+                "We do not support volatile value types.");
+  static_assert(!std::is_pointer<Element>::value,
+                "We do not support pointer value types.");
+  static_assert(!std::is_reference<Element>::value,
+                "We do not support reference value types.");
+#endif  // !PROTOBUF_FUTURE_CONTAINER_STATIC_ASSERTS
 
  public:
   constexpr RepeatedField();

src/google/protobuf/repeated_ptr_field.h

@@ -902,6 +902,16 @@ class StringTypeHandler {
 // Messages.
 template <typename Element>
 class RepeatedPtrField final : private internal::RepeatedPtrFieldBase {
+#ifndef PROTOBUF_FUTURE_CONTAINER_STATIC_ASSERTS
+  static_assert(!std::is_const<Element>::value,
+                "We do not support const value types.");
+  static_assert(!std::is_volatile<Element>::value,
+                "We do not support volatile value types.");
+  static_assert(!std::is_pointer<Element>::value,
+                "We do not support pointer value types.");
+  static_assert(!std::is_reference<Element>::value,
+                "We do not support reference value types.");
+#endif  // !PROTOBUF_FUTURE_CONTAINER_STATIC_ASSERTS
 
  public:
   constexpr RepeatedPtrField();