Fix some of the reflection based algorithms to handle Map entries properly.
They should ignore presence and always process key and value.
Remove codegen for methods in MapEntry and use the ones from Message.
Delete dead code in MapTypeHandler.
PiperOrigin-RevId: 588826780
diff --git a/src/google/protobuf/compiler/cpp/message.cc b/src/google/protobuf/compiler/cpp/message.cc
index d5fb7af..765935a 100644
--- a/src/google/protobuf/compiler/cpp/message.cc
+++ b/src/google/protobuf/compiler/cpp/message.cc
@@ -1160,19 +1160,18 @@
Formatter format(p);
absl::flat_hash_map<absl::string_view, std::string> vars;
CollectMapInfo(options_, descriptor_, &vars);
- vars["lite"] =
- HasDescriptorMethods(descriptor_->file(), options_) ? "" : "Lite";
+ ABSL_CHECK(HasDescriptorMethods(descriptor_->file(), options_));
auto v = p->WithVars(std::move(vars));
// Templatize constexpr constructor as a workaround for a bug in gcc 12
// (warning in gcc 13).
p->Emit(R"cc(
class $classname$ final
- : public ::$proto_ns$::internal::MapEntry$lite$<
+ : public ::$proto_ns$::internal::MapEntry<
$classname$, $key_cpp$, $val_cpp$,
::$proto_ns$::internal::WireFormatLite::$key_wire_type$,
::$proto_ns$::internal::WireFormatLite::$val_wire_type$> {
public:
- using SuperType = ::$proto_ns$::internal::MapEntry$lite$<
+ using SuperType = ::$proto_ns$::internal::MapEntry<
$classname$, $key_cpp$, $val_cpp$,
::$proto_ns$::internal::WireFormatLite::$key_wire_type$,
::$proto_ns$::internal::WireFormatLite::$val_wire_type$>;
diff --git a/src/google/protobuf/map_entry.h b/src/google/protobuf/map_entry.h
index 1bc1fc3..dead40a 100644
--- a/src/google/protobuf/map_entry.h
+++ b/src/google/protobuf/map_entry.h
@@ -139,12 +139,6 @@
// accessors ======================================================
- inline const auto& key() const {
- return KeyTypeHandler::GetExternalReference(key_);
- }
- inline const auto& value() const {
- return ValueTypeHandler::DefaultIfNotInitialized(value_);
- }
inline auto* mutable_key() {
_has_bits_[0] |= 0x00000001u;
return KeyTypeHandler::EnsureMutable(&key_, GetArena());
@@ -153,7 +147,6 @@
_has_bits_[0] |= 0x00000002u;
return ValueTypeHandler::EnsureMutable(&value_, GetArena());
}
-
// TODO: These methods currently differ in behavior from the ones
// implemented via reflection. This means that a MapEntry does not behave the
// same as an equivalent object made via DynamicMessage.
@@ -185,24 +178,6 @@
return ptr;
}
- size_t ByteSizeLong() const final {
- size_t size = 0;
- size += kTagSize + static_cast<size_t>(KeyTypeHandler::ByteSize(key()));
- size += kTagSize + static_cast<size_t>(ValueTypeHandler::ByteSize(value()));
- _cached_size_.Set(ToCachedSize(size));
- return size;
- }
-
- ::uint8_t* _InternalSerialize(::uint8_t* ptr,
- io::EpsCopyOutputStream* stream) const final {
- ptr = KeyTypeHandler::Write(kKeyFieldNumber, key(), ptr, stream);
- return ValueTypeHandler::Write(kValueFieldNumber, value(), ptr, stream);
- }
-
- bool IsInitialized() const final {
- return ValueTypeHandler::IsInitialized(value_);
- }
-
Message* New(Arena* arena) const final {
return Arena::CreateMessage<Derived>(arena);
}
diff --git a/src/google/protobuf/map_proto3_unittest.proto b/src/google/protobuf/map_proto3_unittest.proto
index 1181b29..29a3b36 100644
--- a/src/google/protobuf/map_proto3_unittest.proto
+++ b/src/google/protobuf/map_proto3_unittest.proto
@@ -21,3 +21,7 @@
map<int32, bytes> map_bytes = 1;
map<int32, string> map_string = 2;
}
+
+message TestI32StrMap {
+ map<int32, string> m_32_str = 1;
+}
diff --git a/src/google/protobuf/map_test.cc b/src/google/protobuf/map_test.cc
index f5188f5..7e9a80d 100644
--- a/src/google/protobuf/map_test.cc
+++ b/src/google/protobuf/map_test.cc
@@ -10,6 +10,7 @@
#include <array>
#include <cstddef>
#include <cstdint>
+#include <memory>
#include <string>
#include <type_traits>
#include <utility>
@@ -170,6 +171,48 @@
EXPECT_EQ(map1["2"].GetArena(), &arena);
}
+TEST(MapTest, AlwaysSerializesBothEntries) {
+ for (const Message* prototype :
+ {static_cast<const Message*>(
+ &protobuf_unittest::TestI32StrMap::default_instance()),
+ static_cast<const Message*>(
+ &proto3_unittest::TestI32StrMap::default_instance())}) {
+ const FieldDescriptor* map_field =
+ prototype->GetDescriptor()->FindFieldByName("m_32_str");
+ const FieldDescriptor* map_key = map_field->message_type()->map_key();
+ const FieldDescriptor* map_value = map_field->message_type()->map_value();
+ for (bool add_key : {true, false}) {
+ for (bool add_value : {true, false}) {
+ std::unique_ptr<Message> message(prototype->New());
+ Message* entry_message =
+ message->GetReflection()->AddMessage(message.get(), map_field);
+ // Add the fields, but leave them as the default to make it easier to
+ // match.
+ if (add_key) {
+ entry_message->GetReflection()->SetInt32(entry_message, map_key, 0);
+ }
+ if (add_value) {
+ entry_message->GetReflection()->SetString(entry_message, map_value,
+ "");
+ }
+ ASSERT_EQ(4, entry_message->ByteSizeLong());
+ EXPECT_EQ(entry_message->SerializeAsString(),
+ std::string({
+ '\010', '\0', // key, VARINT, value=0
+ '\022', '\0', // value, LEN, size=0
+ }));
+ ASSERT_EQ(6, message->ByteSizeLong());
+ EXPECT_EQ(message->SerializeAsString(),
+ std::string({
+ '\012', '\04', // field=1, LEN, size=4
+ '\010', '\0', // key, VARINT, value=0
+ '\022', '\0', // value, LEN, size=0
+ }));
+ }
+ }
+ }
+}
+
TEST(MapTest, LoadFactorCalculationWorks) {
// Three stages:
// - empty
diff --git a/src/google/protobuf/map_type_handler.h b/src/google/protobuf/map_type_handler.h
index 7475ab0..7429dae 100644
--- a/src/google/protobuf/map_type_handler.h
+++ b/src/google/protobuf/map_type_handler.h
@@ -93,16 +93,10 @@
uint8_t* ptr, io::EpsCopyOutputStream* stream);
// Functions to manipulate data on memory. ========================
- static inline const Type& GetExternalReference(const Type* value);
static inline void DeleteNoArena(const Type* x);
static constexpr TypeOnMemory Constinit();
static inline Type* EnsureMutable(Type** value, Arena* arena);
- // Return default instance if value is not initialized when calling const
- // reference accessor.
- static inline const Type& DefaultIfNotInitialized(const Type* value);
- // Check if all required fields have values set.
- static inline bool IsInitialized(Type* value);
};
#define MAP_HANDLER(FieldType) \
@@ -126,12 +120,7 @@
static inline uint8_t* Write(int field, const MapEntryAccessorType& value, \
uint8_t* ptr, \
io::EpsCopyOutputStream* stream); \
- static inline const MapEntryAccessorType& GetExternalReference( \
- const TypeOnMemory& value); \
static inline void DeleteNoArena(const TypeOnMemory& x); \
- static inline const MapEntryAccessorType& DefaultIfNotInitialized( \
- const TypeOnMemory& value); \
- static inline bool IsInitialized(const TypeOnMemory& value); \
static void DeleteNoArena(TypeOnMemory& value); \
static constexpr TypeOnMemory Constinit(); \
static inline MapEntryAccessorType* EnsureMutable(TypeOnMemory* value, \
@@ -421,13 +410,6 @@
// Definition for message handler
template <typename Type>
-inline const Type&
-MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type>::GetExternalReference(
- const Type* value) {
- return *value;
-}
-
-template <typename Type>
void MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type>::DeleteNoArena(
const Type* ptr) {
delete ptr;
@@ -448,30 +430,10 @@
return *value;
}
-template <typename Type>
-inline const Type&
-MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type>::DefaultIfNotInitialized(
- const Type* value) {
- return value != nullptr ? *value : *Type::internal_default_instance();
-}
-
-template <typename Type>
-inline bool MapTypeHandler<WireFormatLite::TYPE_MESSAGE, Type>::IsInitialized(
- Type* value) {
- return value ? value->IsInitialized() : false;
-}
-
// Definition for string/bytes handler
#define STRING_OR_BYTES_HANDLER_FUNCTIONS(FieldType) \
template <typename Type> \
- inline const typename MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
- Type>::MapEntryAccessorType& \
- MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
- Type>::GetExternalReference(const TypeOnMemory& value) { \
- return value.Get(); \
- } \
- template <typename Type> \
void MapTypeHandler<WireFormatLite::TYPE_##FieldType, Type>::DeleteNoArena( \
TypeOnMemory& value) { \
value.Destroy(); \
@@ -479,7 +441,7 @@
template <typename Type> \
constexpr auto \
MapTypeHandler<WireFormatLite::TYPE_##FieldType, Type>::Constinit() \
- ->TypeOnMemory { \
+ -> TypeOnMemory { \
return TypeOnMemory(&internal::fixed_address_empty_string, \
ConstantInitialized{}); \
} \
@@ -489,19 +451,6 @@
MapTypeHandler<WireFormatLite::TYPE_##FieldType, Type>::EnsureMutable( \
TypeOnMemory* value, Arena* arena) { \
return value->Mutable(arena); \
- } \
- template <typename Type> \
- inline const typename MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
- Type>::MapEntryAccessorType& \
- MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
- Type>::DefaultIfNotInitialized(const TypeOnMemory& value) { \
- return value.Get(); \
- } \
- template <typename Type> \
- inline bool \
- MapTypeHandler<WireFormatLite::TYPE_##FieldType, Type>::IsInitialized( \
- const TypeOnMemory& /* value */) { \
- return true; \
}
STRING_OR_BYTES_HANDLER_FUNCTIONS(STRING)
STRING_OR_BYTES_HANDLER_FUNCTIONS(BYTES)
@@ -509,13 +458,6 @@
#define PRIMITIVE_HANDLER_FUNCTIONS(FieldType) \
template <typename Type> \
- inline const typename MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
- Type>::MapEntryAccessorType& \
- MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
- Type>::GetExternalReference(const TypeOnMemory& value) { \
- return value; \
- } \
- template <typename Type> \
inline void MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
Type>::DeleteNoArena(TypeOnMemory& /* x */) {} \
template <typename Type> \
@@ -530,19 +472,6 @@
MapTypeHandler<WireFormatLite::TYPE_##FieldType, Type>::EnsureMutable( \
TypeOnMemory* value, Arena* /* arena */) { \
return value; \
- } \
- template <typename Type> \
- inline const typename MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
- Type>::MapEntryAccessorType& \
- MapTypeHandler<WireFormatLite::TYPE_##FieldType, \
- Type>::DefaultIfNotInitialized(const TypeOnMemory& value) { \
- return value; \
- } \
- template <typename Type> \
- inline bool \
- MapTypeHandler<WireFormatLite::TYPE_##FieldType, Type>::IsInitialized( \
- const TypeOnMemory& /* value */) { \
- return true; \
}
PRIMITIVE_HANDLER_FUNCTIONS(INT64)
PRIMITIVE_HANDLER_FUNCTIONS(UINT64)
diff --git a/src/google/protobuf/map_unittest.proto b/src/google/protobuf/map_unittest.proto
index 11be808..5a648d2 100644
--- a/src/google/protobuf/map_unittest.proto
+++ b/src/google/protobuf/map_unittest.proto
@@ -101,3 +101,7 @@
message TestRecursiveMapMessage {
map<string, TestRecursiveMapMessage> a = 1;
}
+
+message TestI32StrMap {
+ map<int32, string> m_32_str = 1;
+}
diff --git a/src/google/protobuf/reflection_ops.cc b/src/google/protobuf/reflection_ops.cc
index 7e125e2..233d258 100644
--- a/src/google/protobuf/reflection_ops.cc
+++ b/src/google/protobuf/reflection_ops.cc
@@ -262,7 +262,13 @@
std::vector<const FieldDescriptor*> fields;
// Should be safe to skip stripped fields because required fields are not
// stripped.
- reflection->ListFields(message, &fields);
+ if (descriptor->options().map_entry()) {
+ // MapEntry objects always check the value regardless of has bit.
+ // We don't need to bother with the key.
+ fields = {descriptor->map_value()};
+ } else {
+ reflection->ListFields(message, &fields);
+ }
for (const FieldDescriptor* field : fields) {
if (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) {
