From ab8b762941e7f97f008654bffa713c8bd167bc64 Mon Sep 17 00:00:00 2001 From: Alyssa Haroldsen Date: Wed, 17 Jan 2024 16:29:20 -0800 Subject: [PATCH] Use `self` for all methods on views, return `'msg` View is Copy, and so it can preserve the lifetime of itself when accessing any fields. PiperOrigin-RevId: 599323122 --- rust/test/nested.proto | 4 ++- rust/test/shared/simple_nested_test.rs | 34 +++++++++++++++++- src/google/protobuf/compiler/rust/BUILD.bazel | 1 + .../compiler/rust/accessors/accessor_case.cc | 35 +++++++++++++++++++ .../compiler/rust/accessors/accessor_case.h | 22 ++++++++++++ .../compiler/rust/accessors/accessors.cc | 1 + .../compiler/rust/accessors/repeated_field.cc | 6 ++-- .../rust/accessors/singular_message.cc | 4 ++- .../rust/accessors/singular_scalar.cc | 5 +-- .../rust/accessors/singular_string.cc | 7 ++-- src/google/protobuf/compiler/rust/message.cc | 10 +++--- 11 files changed, 115 insertions(+), 14 deletions(-) create mode 100644 src/google/protobuf/compiler/rust/accessors/accessor_case.cc diff --git a/rust/test/nested.proto b/rust/test/nested.proto index aeed9bae3b..e26f998ba1 100644 --- a/rust/test/nested.proto +++ b/rust/test/nested.proto @@ -35,8 +35,10 @@ message Outer { optional bool bool = 13; optional string string = 14; optional bytes bytes = 15; - optional InnerSubMsg innersubmsg = 16; + optional InnerSubMsg inner_submsg = 16; optional InnerEnum inner_enum = 17; + repeated int32 repeated_int32 = 18 [packed = true]; + repeated InnerSubMsg repeated_inner_submsg = 19; message SuperInner { message DuperInner { diff --git a/rust/test/shared/simple_nested_test.rs b/rust/test/shared/simple_nested_test.rs index 0a7e2600e6..c56312427a 100644 --- a/rust/test/shared/simple_nested_test.rs +++ b/rust/test/shared/simple_nested_test.rs @@ -50,10 +50,33 @@ fn test_nested_views() { assert_that!(inner_msg.bool(), eq(false)); assert_that!(*inner_msg.string().as_bytes(), empty()); assert_that!(*inner_msg.bytes(), empty()); - assert_that!(inner_msg.innersubmsg().flag(), eq(false)); + assert_that!(inner_msg.inner_submsg().flag(), eq(false)); assert_that!(inner_msg.inner_enum(), eq(InnerEnum::Unspecified)); } +#[test] +fn test_nested_view_lifetimes() { + // Ensure that views have the lifetime of the first layer of borrow, and don't + // create intermediate borrows through nested accessors. + + let outer_msg = Outer::new(); + + let string = outer_msg.inner().string(); + assert_that!(string, eq("")); + + let bytes = outer_msg.inner().bytes(); + assert_that!(bytes, eq(b"")); + + let inner_submsg = outer_msg.inner().inner_submsg(); + assert_that!(inner_submsg.flag(), eq(false)); + + let repeated_int32 = outer_msg.inner().repeated_int32(); + assert_that!(repeated_int32, empty()); + + let repeated_inner_submsg = outer_msg.inner().repeated_inner_submsg(); + assert_that!(repeated_inner_submsg, empty()); +} + #[test] fn test_nested_muts() { // Covers the setting of a mut and the following assertion @@ -129,6 +152,10 @@ fn test_recursive_view() { assert_that!(rec.rec().num(), eq(0)); assert_that!(rec.rec().rec().num(), eq(0)); // turtles all the way down... assert_that!(rec.rec().rec().rec().num(), eq(0)); // ... ad infinitum + + // Test that intermediate borrows are not created. + let nested = rec.rec().rec().rec(); + assert_that!(nested.num(), eq(0)); } #[test] @@ -145,4 +172,9 @@ fn test_recursive_mut() { assert_that!(rec.num(), eq(0)); assert_that!(rec.rec().rec().num(), eq(0)); assert_that!(rec.rec().rec().rec().rec().num(), eq(1)); + + // This fails since `RecursiveMut` has `&mut self` methods. + // See b/314989133. + // let nested = rec.rec_mut().rec_mut().rec_mut(); + // assert_that!(nested.num(), eq(0)); } diff --git a/src/google/protobuf/compiler/rust/BUILD.bazel b/src/google/protobuf/compiler/rust/BUILD.bazel index 0e408aa0dd..c209b8df43 100644 --- a/src/google/protobuf/compiler/rust/BUILD.bazel +++ b/src/google/protobuf/compiler/rust/BUILD.bazel @@ -101,6 +101,7 @@ cc_library( cc_library( name = "accessors", srcs = [ + "accessors/accessor_case.cc", "accessors/accessors.cc", "accessors/map.cc", "accessors/repeated_field.cc", diff --git a/src/google/protobuf/compiler/rust/accessors/accessor_case.cc b/src/google/protobuf/compiler/rust/accessors/accessor_case.cc new file mode 100644 index 0000000000..470deba4f7 --- /dev/null +++ b/src/google/protobuf/compiler/rust/accessors/accessor_case.cc @@ -0,0 +1,35 @@ +#include "google/protobuf/compiler/rust/accessors/accessor_case.h" + +#include "absl/strings/string_view.h" + +namespace google { +namespace protobuf { +namespace compiler { +namespace rust { + +absl::string_view ViewReceiver(AccessorCase accessor_case) { + switch (accessor_case) { + case AccessorCase::VIEW: + return "self"; + case AccessorCase::OWNED: + case AccessorCase::MUT: + return "&self"; + } + return ""; +} + +absl::string_view ViewLifetime(AccessorCase accessor_case) { + switch (accessor_case) { + case AccessorCase::VIEW: + return "'msg"; + case AccessorCase::OWNED: + case AccessorCase::MUT: + return "'_"; + } + return ""; +} + +} // namespace rust +} // namespace compiler +} // namespace protobuf +} // namespace google diff --git a/src/google/protobuf/compiler/rust/accessors/accessor_case.h b/src/google/protobuf/compiler/rust/accessors/accessor_case.h index a68c55a6b9..57906e320e 100644 --- a/src/google/protobuf/compiler/rust/accessors/accessor_case.h +++ b/src/google/protobuf/compiler/rust/accessors/accessor_case.h @@ -8,6 +8,13 @@ #ifndef GOOGLE_PROTOBUF_COMPILER_RUST_ACCESSORS_ACCESSOR_CASE_H__ #define GOOGLE_PROTOBUF_COMPILER_RUST_ACCESSORS_ACCESSOR_CASE_H__ +#include "absl/strings/string_view.h" + +namespace google { +namespace protobuf { +namespace compiler { +namespace rust { + // GenerateAccessorMsgImpl is reused for all three types of $Msg$, $Msg$Mut and // $Msg$View; this enum signifies which case we are handling so corresponding // adjustments can be made (for example: to not emit any mutation accessors @@ -18,4 +25,19 @@ enum class AccessorCase { VIEW, }; +// Returns the `self` receiver type for a subfield view accessor. +absl::string_view ViewReceiver(AccessorCase accessor_case); + +// Returns the lifetime of a subfield view accessor. +// Views are Copy, and so the full `'msg` lifetime can be used. +// Any `&self` or `&mut self` accessors need to use the lifetime of that +// borrow, which is referenced via `'_`. +// See b/314989133 for _mut accessors. +absl::string_view ViewLifetime(AccessorCase accessor_case); + +} // namespace rust +} // namespace compiler +} // namespace protobuf +} // namespace google + #endif // GOOGLE_PROTOBUF_COMPILER_RUST_ACCESSORS_ACCESSOR_CASE_H__ diff --git a/src/google/protobuf/compiler/rust/accessors/accessors.cc b/src/google/protobuf/compiler/rust/accessors/accessors.cc index 92bb30cd1c..6a1e00c1e8 100644 --- a/src/google/protobuf/compiler/rust/accessors/accessors.cc +++ b/src/google/protobuf/compiler/rust/accessors/accessors.cc @@ -10,6 +10,7 @@ #include #include "absl/log/absl_log.h" +#include "google/protobuf/compiler/rust/accessors/accessor_case.h" #include "google/protobuf/compiler/rust/accessors/accessor_generator.h" #include "google/protobuf/compiler/rust/context.h" #include "google/protobuf/descriptor.h" diff --git a/src/google/protobuf/compiler/rust/accessors/repeated_field.cc b/src/google/protobuf/compiler/rust/accessors/repeated_field.cc index 6fd286e408..97055c5403 100644 --- a/src/google/protobuf/compiler/rust/accessors/repeated_field.cc +++ b/src/google/protobuf/compiler/rust/accessors/repeated_field.cc @@ -24,13 +24,15 @@ void RepeatedField::InMsgImpl(Context& ctx, const FieldDescriptor& field, AccessorCase accessor_case) const { ctx.Emit({{"field", RsSafeName(field.name())}, {"RsType", RsTypePath(ctx, field)}, + {"view_lifetime", ViewLifetime(accessor_case)}, + {"view_self", ViewReceiver(accessor_case)}, {"getter_thunk", ThunkName(ctx, field, "get")}, {"getter_mut_thunk", ThunkName(ctx, field, "get_mut")}, {"getter", [&] { if (ctx.is_upb()) { ctx.Emit({}, R"rs( - pub fn $field$(&self) -> $pb$::RepeatedView<'_, $RsType$> { + pub fn $field$($view_self$) -> $pb$::RepeatedView<$view_lifetime$, $RsType$> { unsafe { $getter_thunk$( self.raw_msg(), @@ -46,7 +48,7 @@ void RepeatedField::InMsgImpl(Context& ctx, const FieldDescriptor& field, )rs"); } else { ctx.Emit({}, R"rs( - pub fn $field$(&self) -> $pb$::RepeatedView<'_, $RsType$> { + pub fn $field$($view_self$) -> $pb$::RepeatedView<$view_lifetime$, $RsType$> { unsafe { $pb$::RepeatedView::from_raw( $pbi$::Private, diff --git a/src/google/protobuf/compiler/rust/accessors/singular_message.cc b/src/google/protobuf/compiler/rust/accessors/singular_message.cc index 7d8e6b8cbd..06a57d54e4 100644 --- a/src/google/protobuf/compiler/rust/accessors/singular_message.cc +++ b/src/google/protobuf/compiler/rust/accessors/singular_message.cc @@ -26,6 +26,8 @@ void SingularMessage::InMsgImpl(Context& ctx, const FieldDescriptor& field, std::string msg_type = RsTypePath(ctx, field); ctx.Emit({{"msg_type", msg_type}, {"field", RsSafeName(field.name())}, + {"view_lifetime", ViewLifetime(accessor_case)}, + {"view_self", ViewReceiver(accessor_case)}, {"getter_thunk", ThunkName(ctx, field, "get")}, {"getter_mut_thunk", ThunkName(ctx, field, "get_mut")}, {"clearer_thunk", ThunkName(ctx, field, "clear")}, @@ -58,7 +60,7 @@ void SingularMessage::InMsgImpl(Context& ctx, const FieldDescriptor& field, {"getter", [&] { ctx.Emit({}, R"rs( - pub fn $field$(&self) -> $msg_type$View { + pub fn $field$($view_self$) -> $msg_type$View<$view_lifetime$> { $getter_body$ } )rs"); diff --git a/src/google/protobuf/compiler/rust/accessors/singular_scalar.cc b/src/google/protobuf/compiler/rust/accessors/singular_scalar.cc index c26c79493c..5301fbc41b 100644 --- a/src/google/protobuf/compiler/rust/accessors/singular_scalar.cc +++ b/src/google/protobuf/compiler/rust/accessors/singular_scalar.cc @@ -26,13 +26,14 @@ void SingularScalar::InMsgImpl(Context& ctx, const FieldDescriptor& field, ctx.Emit( { {"field", RsSafeName(field.name())}, + {"view_self", ViewReceiver(accessor_case)}, {"Scalar", RsTypePath(ctx, field)}, {"hazzer_thunk", ThunkName(ctx, field, "has")}, {"default_value", DefaultValue(ctx, field)}, {"getter", [&] { ctx.Emit({}, R"rs( - pub fn $field$(&self) -> $Scalar$ { + pub fn $field$($view_self$) -> $Scalar$ { unsafe { $getter_thunk$(self.raw_msg()) } } )rs"); @@ -42,7 +43,7 @@ void SingularScalar::InMsgImpl(Context& ctx, const FieldDescriptor& field, if (!field.is_optional()) return; if (!field.has_presence()) return; ctx.Emit({}, R"rs( - pub fn $field$_opt(&self) -> $pb$::Optional<$Scalar$> { + pub fn $field$_opt($view_self$) -> $pb$::Optional<$Scalar$> { if !unsafe { $hazzer_thunk$(self.raw_msg()) } { return $pb$::Optional::Unset($default_value$); } diff --git a/src/google/protobuf/compiler/rust/accessors/singular_string.cc b/src/google/protobuf/compiler/rust/accessors/singular_string.cc index da83e4a260..69fd53958e 100644 --- a/src/google/protobuf/compiler/rust/accessors/singular_string.cc +++ b/src/google/protobuf/compiler/rust/accessors/singular_string.cc @@ -27,6 +27,7 @@ void SingularString::InMsgImpl(Context& ctx, const FieldDescriptor& field, std::string getter_thunk = ThunkName(ctx, field, "get"); std::string setter_thunk = ThunkName(ctx, field, "set"); std::string proxied_type = RsTypePath(ctx, field); + auto transform_view = [&] { if (field.type() == FieldDescriptor::TYPE_STRING) { ctx.Emit(R"rs( @@ -45,6 +46,8 @@ void SingularString::InMsgImpl(Context& ctx, const FieldDescriptor& field, {"setter_thunk", setter_thunk}, {"proxied_type", proxied_type}, {"transform_view", transform_view}, + {"view_lifetime", ViewLifetime(accessor_case)}, + {"view_self", ViewReceiver(accessor_case)}, {"field_optional_getter", [&] { if (!field.is_optional()) return; @@ -53,7 +56,7 @@ void SingularString::InMsgImpl(Context& ctx, const FieldDescriptor& field, {"getter_thunk", getter_thunk}, {"transform_view", transform_view}}, R"rs( - pub fn $field$_opt(&self) -> $pb$::Optional<&$proxied_type$> { + pub fn $field$_opt($view_self$) -> $pb$::Optional<&$view_lifetime$ $proxied_type$> { let view = unsafe { $getter_thunk$(self.raw_msg()).as_ref() }; $pb$::Optional::new( $transform_view$ , @@ -143,7 +146,7 @@ void SingularString::InMsgImpl(Context& ctx, const FieldDescriptor& field, }}, }, R"rs( - pub fn $field$(&self) -> &$proxied_type$ { + pub fn $field$($view_self$) -> &$view_lifetime$ $proxied_type$ { let view = unsafe { $getter_thunk$(self.raw_msg()).as_ref() }; $transform_view$ } diff --git a/src/google/protobuf/compiler/rust/message.cc b/src/google/protobuf/compiler/rust/message.cc index 3d02da0a2b..9339e35749 100644 --- a/src/google/protobuf/compiler/rust/message.cc +++ b/src/google/protobuf/compiler/rust/message.cc @@ -549,7 +549,7 @@ void GenerateRs(Context& ctx, const Descriptor& msg) { } #[allow(dead_code)] - impl<'a> $Msg$View<'a> { + impl<'msg> $Msg$View<'msg> { #[doc(hidden)] pub fn new(_private: $pbi$::Private, msg: $pbi$::RawMessage) -> Self { Self { msg, _phantom: std::marker::PhantomData } @@ -634,11 +634,11 @@ void GenerateRs(Context& ctx, const Descriptor& msg) { } #[allow(dead_code)] - impl<'a> $Msg$Mut<'a> { + impl<'msg> $Msg$Mut<'msg> { #[doc(hidden)] pub fn from_parent( _private: $pbi$::Private, - parent: $pbr$::MutatorMessageRef<'a>, + parent: $pbr$::MutatorMessageRef<'msg>, msg: $pbi$::RawMessage) -> Self { Self { @@ -648,7 +648,7 @@ void GenerateRs(Context& ctx, const Descriptor& msg) { } #[doc(hidden)] - pub fn new(_private: $pbi$::Private, msg: &'a mut $pbr$::MessageInner) -> Self { + pub fn new(_private: $pbi$::Private, msg: &'msg mut $pbr$::MessageInner) -> Self { Self{ inner: $pbr$::MutatorMessageRef::new(_private, msg) } } @@ -656,7 +656,7 @@ void GenerateRs(Context& ctx, const Descriptor& msg) { self.inner.msg() } - fn as_mutator_message_ref(&mut self) -> $pbr$::MutatorMessageRef<'a> { + fn as_mutator_message_ref(&mut self) -> $pbr$::MutatorMessageRef<'msg> { self.inner }