From 91114633bbcf82f3212aff9d8a5413657f52622f Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Tue, 5 Sep 2023 09:22:12 -0700 Subject: [PATCH] Do not expose mutable methods in the CProxy. PiperOrigin-RevId: 562800851 --- upb/protos/protos.h | 47 +++++++------- upb/protos_generator/gen_accessors.cc | 16 +++-- upb/protos_generator/gen_messages.cc | 10 +-- upb/protos_generator/tests/test_generated.cc | 65 ++++++++++++++++++++ 4 files changed, 106 insertions(+), 32 deletions(-) diff --git a/upb/protos/protos.h b/upb/protos/protos.h index 68d8274997..1c0faf28e2 100644 --- a/upb/protos/protos.h +++ b/upb/protos/protos.h @@ -102,21 +102,6 @@ class Ptr final { Proxy p_; }; -namespace internal { -struct PrivateAccess { - template - static auto* GetInternalMsg(T&& message) { - return message->msg(); - } -}; - -template -auto* GetInternalMsg(T&& message) { - return PrivateAccess::GetInternalMsg(std::forward(message)); -} - -} // namespace internal - inline absl::string_view UpbStrToStringView(upb_StringView str) { return absl::string_view(str.data, str.size); } @@ -163,6 +148,26 @@ absl::Status MessageEncodeError(upb_EncodeStatus status, SourceLocation loc = SourceLocation::current()); namespace internal { +struct PrivateAccess { + template + static auto* GetInternalMsg(T&& message) { + return message->msg(); + } + template + static auto Proxy(void* p, upb_Arena* arena) { + return typename T::Proxy(p, arena); + } + template + static auto CProxy(const void* p, upb_Arena* arena) { + return typename T::CProxy(p, arena); + } +}; + +template +auto* GetInternalMsg(T&& message) { + return PrivateAccess::GetInternalMsg(std::forward(message)); +} + template T CreateMessage() { return T(); @@ -174,8 +179,8 @@ typename T::Proxy CreateMessageProxy(void* msg, upb_Arena* arena) { } template -typename T::CProxy CreateMessage(upb_Message* msg, upb_Arena* arena) { - return typename T::CProxy(msg, arena); +typename T::CProxy CreateMessage(const upb_Message* msg, upb_Arena* arena) { + return PrivateAccess::CProxy(msg, arena); } class ExtensionMiniTableProvider { @@ -269,11 +274,11 @@ void DeepCopy(Ptr source_message, Ptr target_message) { } template -typename T::Proxy CloneMessage(Ptr message, upb::Arena& arena) { - return typename T::Proxy( +typename T::Proxy CloneMessage(Ptr message, upb_Arena* arena) { + return internal::PrivateAccess::Proxy( ::protos::internal::DeepClone(internal::GetInternalMsg(message), - T::minitable(), arena.ptr()), - arena.ptr()); + T::minitable(), arena), + arena); } template diff --git a/upb/protos_generator/gen_accessors.cc b/upb/protos_generator/gen_accessors.cc index ac7186f158..9db49da278 100644 --- a/upb/protos_generator/gen_accessors.cc +++ b/upb/protos_generator/gen_accessors.cc @@ -447,18 +447,26 @@ void WriteUsingAccessorsInHeader(const protobuf::Descriptor* desc, // Generate hazzer (if any). if (field->has_presence()) { output("using $0Access::has_$1;\n", class_name, resolved_field_name); - output("using $0Access::clear_$1;\n", class_name, resolved_field_name); + if (!read_only) { + output("using $0Access::clear_$1;\n", class_name, resolved_field_name); + } } if (field->is_map()) { output( R"cc( using $0Access::$1_size; - using $0Access::clear_$1; - using $0Access::delete_$1; using $0Access::get_$1; - using $0Access::set_$1; )cc", class_name, resolved_field_name); + if (!read_only) { + output( + R"cc( + using $0Access::clear_$1; + using $0Access::delete_$1; + using $0Access::set_$1; + )cc", + class_name, resolved_field_name); + } } else if (desc->options().map_entry()) { // TODO(b/237399867) Implement map entry } else if (field->is_repeated()) { diff --git a/upb/protos_generator/gen_messages.cc b/upb/protos_generator/gen_messages.cc index bb886e9192..ca75ba70dd 100644 --- a/upb/protos_generator/gen_messages.cc +++ b/upb/protos_generator/gen_messages.cc @@ -283,9 +283,6 @@ void WriteModelProxyDeclaration(const protobuf::Descriptor* descriptor, ::protos::Ptr<$0Proxy> message); friend upb_Arena* ::protos::internal::GetArena<$2>($2* message); friend upb_Arena* ::protos::internal::GetArena<$2>(::protos::Ptr<$2> message); - friend $0Proxy(::protos::CloneMessage(::protos::Ptr<$2> message, - ::upb::Arena& arena)); - static void Rebind($0Proxy& lhs, const $0Proxy& rhs) { lhs.msg_ = rhs.msg_; lhs.arena_ = rhs.arena_; @@ -312,7 +309,7 @@ void WriteModelCProxyDeclaration(const protobuf::Descriptor* descriptor, )cc", ClassName(descriptor), MessageName(descriptor)); - WriteUsingAccessorsInHeader(descriptor, MessageClassType::kMessageProxy, + WriteUsingAccessorsInHeader(descriptor, MessageClassType::kMessageCProxy, output); output.Indent(1); @@ -322,9 +319,8 @@ void WriteModelCProxyDeclaration(const protobuf::Descriptor* descriptor, using AsNonConst = $0Proxy; const void* msg() const { return msg_; } - $0CProxy(void* msg, upb_Arena* arena) : internal::$0Access(($1*)msg, arena){}; - friend $0::CProxy(::protos::internal::CreateMessage<$0>( - upb_Message* msg, upb_Arena* arena)); + $0CProxy(const void* msg, upb_Arena* arena) + : internal::$0Access(($1*)msg, arena){}; friend struct ::protos::internal::PrivateAccess; friend class RepeatedFieldProxy; friend class ::protos::Ptr<$0>; diff --git a/upb/protos_generator/tests/test_generated.cc b/upb/protos_generator/tests/test_generated.cc index 4bc6ffaf7c..81e69567db 100644 --- a/upb/protos_generator/tests/test_generated.cc +++ b/upb/protos_generator/tests/test_generated.cc @@ -46,6 +46,8 @@ #include "protos_generator/tests/test_model.upb.proto.h" #include "upb/mem/arena.h" +namespace { + using ::protos_generator::test::protos::ChildModel1; using ::protos_generator::test::protos::other_ext; using ::protos_generator::test::protos::RED; @@ -60,6 +62,12 @@ using ::protos_generator::test::protos::ThemeExtension; using ::testing::ElementsAre; using ::testing::HasSubstr; +// C++17 port of C++20 `requires` +template +constexpr bool Requires(F) { + return std::is_invocable_v; +} + TEST(CppGeneratedCode, Constructor) { TestModel test_model; } TEST(CppGeneratedCode, MessageEnum) { EXPECT_EQ(5, TestModel_Category_IMAGES); } @@ -963,6 +971,61 @@ TEST(CppGeneratedCode, ProxyToCProxy) { (void)child2; } +TEST(CppGeneratedCode, MutableAccessorsAreHiddenInCProxy) { + TestModel model; + ::protos::Ptr proxy = &model; + ::protos::Ptr cproxy = proxy; + + const auto test_const_accessors = [](auto p) { + // We don't want to run it, just check it compiles. + if (false) { + (void)p->has_str1(); + (void)p->str1(); + (void)p->has_value(); + (void)p->value(); + (void)p->has_oneof_member1(); + (void)p->oneof_member1(); + (void)p->value_array(); + (void)p->value_array_size(); + (void)p->value_array(1); + (void)p->has_nested_child_1(); + (void)p->nested_child_1(); + (void)p->child_models(); + (void)p->child_models_size(); + (void)p->child_models(1); + (void)p->child_map_size(); + (void)p->get_child_map(1); + } + }; + + test_const_accessors(proxy); + test_const_accessors(cproxy); + + const auto test_mutable_accessors = [](auto p, bool expected) { + const auto r = [&](auto l) { return Requires(l) == expected; }; + EXPECT_TRUE(r([](auto p) -> decltype(p->set_str1("")) {})); + EXPECT_TRUE(r([](auto p) -> decltype(p->clear_str1()) {})); + EXPECT_TRUE(r([](auto p) -> decltype(p->set_value(1)) {})); + EXPECT_TRUE(r([](auto p) -> decltype(p->clear_value()) {})); + EXPECT_TRUE(r([](auto p) -> decltype(p->set_oneof_member1("")) {})); + EXPECT_TRUE(r([](auto p) -> decltype(p->clear_oneof_member1()) {})); + EXPECT_TRUE(r([](auto p) -> decltype(p->mutable_nested_child_1()) {})); + EXPECT_TRUE(r([](auto p) -> decltype(p->clear_nested_child_1()) {})); + EXPECT_TRUE(r([](auto p) -> decltype(p->add_value_array(1)) {})); + EXPECT_TRUE(r([](auto p) -> decltype(p->mutable_value_array()) {})); + EXPECT_TRUE(r([](auto p) -> decltype(p->resize_value_array(1)) {})); + EXPECT_TRUE(r([](auto p) -> decltype(p->set_value_array(1, 1)) {})); + EXPECT_TRUE(r([](auto p) -> decltype(p->add_child_models()) {})); + EXPECT_TRUE(r([](auto p) -> decltype(p->mutable_child_models(1)) {})); + EXPECT_TRUE(r([](auto p) -> decltype(p->clear_child_map()) {})); + EXPECT_TRUE(r([](auto p) -> decltype(p->delete_child_map(1)) {})); + EXPECT_TRUE(r( + [](auto p) -> decltype(p->set_child_map(1, *p->get_child_map(1))) {})); + }; + test_mutable_accessors(proxy, true); + test_mutable_accessors(cproxy, false); +} + bool ProxyToCProxyMethod(::protos::Ptr child) { return child->child_str1() == "text in child"; } @@ -1067,3 +1130,5 @@ TEST(CppGeneratedCode, ClearConstMessageShouldFail) { ::protos::ClearMessage(model.child_model_1()); } #endif + +} // namespace