From 2b45018be4f2eca6a8cad7d76afcde54ea3f088a Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Thu, 7 Dec 2023 09:58:24 -0800 Subject: [PATCH] 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 --- src/google/protobuf/compiler/cpp/message.cc | 7 +- src/google/protobuf/map_entry.h | 25 ------- src/google/protobuf/map_proto3_unittest.proto | 4 + src/google/protobuf/map_test.cc | 43 +++++++++++ src/google/protobuf/map_type_handler.h | 73 +------------------ src/google/protobuf/map_unittest.proto | 4 + src/google/protobuf/reflection_ops.cc | 8 +- 7 files changed, 62 insertions(+), 102 deletions(-) diff --git a/src/google/protobuf/compiler/cpp/message.cc b/src/google/protobuf/compiler/cpp/message.cc index d5fb7afb93..765935ae8e 100644 --- a/src/google/protobuf/compiler/cpp/message.cc +++ b/src/google/protobuf/compiler/cpp/message.cc @@ -1160,19 +1160,18 @@ void MessageGenerator::GenerateMapEntryClassDefinition(io::Printer* p) { Formatter format(p); absl::flat_hash_map 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 1bc1fc3c8b..dead40ac02 100644 --- a/src/google/protobuf/map_entry.h +++ b/src/google/protobuf/map_entry.h @@ -139,12 +139,6 @@ class MapEntry : public MapEntryBase { // 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 @@ class MapEntry : public MapEntryBase { _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 @@ class MapEntry : public MapEntryBase { return ptr; } - size_t ByteSizeLong() const final { - size_t size = 0; - size += kTagSize + static_cast(KeyTypeHandler::ByteSize(key())); - size += kTagSize + static_cast(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(arena); } diff --git a/src/google/protobuf/map_proto3_unittest.proto b/src/google/protobuf/map_proto3_unittest.proto index 1181b29988..29a3b367a2 100644 --- a/src/google/protobuf/map_proto3_unittest.proto +++ b/src/google/protobuf/map_proto3_unittest.proto @@ -21,3 +21,7 @@ message TestProto3BytesMap { map map_bytes = 1; map map_string = 2; } + +message TestI32StrMap { + map m_32_str = 1; +} diff --git a/src/google/protobuf/map_test.cc b/src/google/protobuf/map_test.cc index f5188f559f..7e9a80df42 100644 --- a/src/google/protobuf/map_test.cc +++ b/src/google/protobuf/map_test.cc @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -170,6 +171,48 @@ TEST(MapTest, CopyConstructMessagesWithArena) { EXPECT_EQ(map1["2"].GetArena(), &arena); } +TEST(MapTest, AlwaysSerializesBothEntries) { + for (const Message* prototype : + {static_cast( + &protobuf_unittest::TestI32StrMap::default_instance()), + static_cast( + &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(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 7475ab04a3..7429dae7de 100644 --- a/src/google/protobuf/map_type_handler.h +++ b/src/google/protobuf/map_type_handler.h @@ -93,16 +93,10 @@ class MapTypeHandler { 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 @@ class MapTypeHandler { 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, \ @@ -420,13 +409,6 @@ READ_METHOD(BOOL) // Definition for message handler -template -inline const Type& -MapTypeHandler::GetExternalReference( - const Type* value) { - return *value; -} - template void MapTypeHandler::DeleteNoArena( const Type* ptr) { @@ -448,29 +430,9 @@ inline Type* MapTypeHandler::EnsureMutable( return *value; } -template -inline const Type& -MapTypeHandler::DefaultIfNotInitialized( - const Type* value) { - return value != nullptr ? *value : *Type::internal_default_instance(); -} - -template -inline bool MapTypeHandler::IsInitialized( - Type* value) { - return value ? value->IsInitialized() : false; -} - // Definition for string/bytes handler #define STRING_OR_BYTES_HANDLER_FUNCTIONS(FieldType) \ - template \ - inline const typename MapTypeHandler::MapEntryAccessorType& \ - MapTypeHandler::GetExternalReference(const TypeOnMemory& value) { \ - return value.Get(); \ - } \ template \ void MapTypeHandler::DeleteNoArena( \ TypeOnMemory& value) { \ @@ -479,7 +441,7 @@ inline bool MapTypeHandler::IsInitialized( template \ constexpr auto \ MapTypeHandler::Constinit() \ - ->TypeOnMemory { \ + -> TypeOnMemory { \ return TypeOnMemory(&internal::fixed_address_empty_string, \ ConstantInitialized{}); \ } \ @@ -489,32 +451,12 @@ inline bool MapTypeHandler::IsInitialized( MapTypeHandler::EnsureMutable( \ TypeOnMemory* value, Arena* arena) { \ return value->Mutable(arena); \ - } \ - template \ - inline const typename MapTypeHandler::MapEntryAccessorType& \ - MapTypeHandler::DefaultIfNotInitialized(const TypeOnMemory& value) { \ - return value.Get(); \ - } \ - template \ - inline bool \ - MapTypeHandler::IsInitialized( \ - const TypeOnMemory& /* value */) { \ - return true; \ } STRING_OR_BYTES_HANDLER_FUNCTIONS(STRING) STRING_OR_BYTES_HANDLER_FUNCTIONS(BYTES) #undef STRING_OR_BYTES_HANDLER_FUNCTIONS #define PRIMITIVE_HANDLER_FUNCTIONS(FieldType) \ - template \ - inline const typename MapTypeHandler::MapEntryAccessorType& \ - MapTypeHandler::GetExternalReference(const TypeOnMemory& value) { \ - return value; \ - } \ template \ inline void MapTypeHandler::DeleteNoArena(TypeOnMemory& /* x */) {} \ @@ -530,19 +472,6 @@ STRING_OR_BYTES_HANDLER_FUNCTIONS(BYTES) MapTypeHandler::EnsureMutable( \ TypeOnMemory* value, Arena* /* arena */) { \ return value; \ - } \ - template \ - inline const typename MapTypeHandler::MapEntryAccessorType& \ - MapTypeHandler::DefaultIfNotInitialized(const TypeOnMemory& value) { \ - return value; \ - } \ - template \ - inline bool \ - MapTypeHandler::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 11be80836e..5a648d202f 100644 --- a/src/google/protobuf/map_unittest.proto +++ b/src/google/protobuf/map_unittest.proto @@ -101,3 +101,7 @@ message MessageContainingMapCalledEntry { message TestRecursiveMapMessage { map a = 1; } + +message TestI32StrMap { + map m_32_str = 1; +} diff --git a/src/google/protobuf/reflection_ops.cc b/src/google/protobuf/reflection_ops.cc index 7e125e2656..233d25847c 100644 --- a/src/google/protobuf/reflection_ops.cc +++ b/src/google/protobuf/reflection_ops.cc @@ -262,7 +262,13 @@ bool ReflectionOps::IsInitialized(const Message& message) { std::vector 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) {