Introduce preliminary submessage_mut semantics

We've had access to views for submessages for a while: If you hit some_message.submsg().some_int(), you'll get a view for that int. Until now, there hasn't been a way to get some_message.submsg_mut(), so we introduce the mutational pathway here. We haven't added fully-functioning mutation, but this is a step towards that goal. subview was inaccurate, so I've refactored and renamed: { accessor_fns_for_views, accessor_fns_for_muts }. PiperOrigin-RevId: 581984371
1 year ago · b3cfff3478
parent a74398fbe6
commit b3cfff3478
9 changed files with 193 additions and 55 deletions
--- a/rust/cpp.rs
+++ b/rust/cpp.rs
@ -171,6 +171,14 @@ impl<'msg> MutatorMessageRef<'msg> {
        MutatorMessageRef { msg: msg.msg, _phantom: PhantomData }
    }

+    pub fn from_parent(
+        _private: Private,
+        _parent_msg: &'msg mut MessageInner,
+        message_field_ptr: RawMessage,
+    ) -> Self {
+        MutatorMessageRef { msg: message_field_ptr, _phantom: PhantomData }
+    }
+
    pub fn msg(&self) -> RawMessage {
        self.msg
    }
--- a/rust/test/nested.proto
+++ b/rust/test/nested.proto
@ -11,8 +11,19 @@ package nest;

 message Outer {
  message Inner {
-    optional int32 num = 1;
-    optional bool boolean = 2;
+    optional double double = 1;
+    optional float float = 2;
+    optional int32 int32 = 3;
+    optional int64 int64 = 4;
+    optional uint32 uint32 = 5;
+    optional uint64 uint64 = 6;
+    optional sint32 sint32 = 7;
+    optional sint64 sint64 = 8;
+    optional fixed32 fixed32 = 9;
+    optional fixed64 fixed64 = 10;
+    optional sfixed32 sfixed32 = 11;
+    optional sfixed64 sfixed64 = 12;
+    optional bool bool = 13;

    message SuperInner {
      message DuperInner {
--- a/rust/test/shared/BUILD
+++ b/rust/test/shared/BUILD
@ -261,7 +261,10 @@ rust_test(
        # TODO: Enable testing on arm once we support sanitizers for Rust on Arm.
        "not_build:arm",
    ],
-    deps = ["//rust/test:nested_cc_rust_proto"],
+    deps = [
+        "@crate_index//:googletest",
+        "//rust/test:nested_cc_rust_proto",
+    ],
 )

 rust_test(
@ -271,7 +274,10 @@ rust_test(
        # TODO: Enable testing on arm once we support sanitizers for Rust on Arm.
        "not_build:arm",
    ],
-    deps = ["//rust/test:nested_upb_rust_proto"],
+    deps = [
+        "@crate_index//:googletest",
+        "//rust/test:nested_upb_rust_proto",
+    ],
 )

 rust_test(
--- a/rust/test/shared/accessors_test.rs
+++ b/rust/test/shared/accessors_test.rs
@ -667,11 +667,16 @@ fn test_nonempty_default_string_accessors() {

 #[test]
 fn test_singular_msg_field() {
-    use crate::TestAllTypes_::NestedMessageView;
-    let msg = TestAllTypes::new();
-    // TODO: fetch the inner integer `bb`
-    // call should look like msg.optional_nested_message().bb()
-    let _msg: NestedMessageView = msg.optional_nested_message();
+    use crate::TestAllTypes_::{NestedMessageMut, NestedMessageView};
+
+    let mut msg = TestAllTypes::new();
+    let msg_view: NestedMessageView = msg.optional_nested_message();
+    // testing reading an int inside a view
+    assert_that!(msg_view.bb(), eq(0));
+
+    let msg_mut: NestedMessageMut = msg.optional_nested_message_mut();
+    // test reading an int inside a mut
+    assert_that!(msg_mut.bb(), eq(0));
 }

 #[test]
--- a/rust/test/shared/simple_nested_test.rs
+++ b/rust/test/shared/simple_nested_test.rs
@ -5,13 +5,49 @@
 // license that can be found in the LICENSE file or at
 // https://developers.google.com/open-source/licenses/bsd

+use googletest::prelude::*;
 use nested_proto::nest::Outer;
+use nested_proto::nest::Outer_::InnerMut;
+use nested_proto::nest::Outer_::InnerView;

 #[test]
-fn test_simple_nested_proto() {
+fn test_nested_views() {
    let outer_msg = Outer::new();
-    assert_eq!(outer_msg.inner().num(), 0);
-    assert!(!outer_msg.inner().boolean());
+    let inner_msg: InnerView<'_> = outer_msg.inner();
+    assert_that!(inner_msg.double(), eq(0.0));
+    assert_that!(inner_msg.float(), eq(0.0));
+    assert_that!(inner_msg.int32(), eq(0));
+    assert_that!(inner_msg.int64(), eq(0));
+    assert_that!(inner_msg.uint32(), eq(0));
+    assert_that!(inner_msg.uint64(), eq(0));
+    assert_that!(inner_msg.sint32(), eq(0));
+    assert_that!(inner_msg.sint64(), eq(0));
+    assert_that!(inner_msg.fixed32(), eq(0));
+    assert_that!(inner_msg.fixed64(), eq(0));
+    assert_that!(inner_msg.sfixed32(), eq(0));
+    assert_that!(inner_msg.sfixed64(), eq(0));
+    assert_that!(inner_msg.bool(), eq(false));
+}
+
+#[test]
+fn test_nested_muts() {
+    // TODO: add actual mutation logic, this just peeks at InnerMut at
+    // the moment
+    let mut outer_msg = Outer::new();
+    let inner_msg: InnerMut<'_> = outer_msg.inner_mut();
+    assert_that!(inner_msg.double(), eq(0.0));
+    assert_that!(inner_msg.float(), eq(0.0));
+    assert_that!(inner_msg.int32(), eq(0));
+    assert_that!(inner_msg.int64(), eq(0));
+    assert_that!(inner_msg.uint32(), eq(0));
+    assert_that!(inner_msg.uint64(), eq(0));
+    assert_that!(inner_msg.sint32(), eq(0));
+    assert_that!(inner_msg.sint64(), eq(0));
+    assert_that!(inner_msg.fixed32(), eq(0));
+    assert_that!(inner_msg.fixed64(), eq(0));
+    assert_that!(inner_msg.sfixed32(), eq(0));
+    assert_that!(inner_msg.sfixed64(), eq(0));
+    assert_that!(inner_msg.bool(), eq(false));
 }

 #[test]
--- a/rust/upb.rs
+++ b/rust/upb.rs
@ -261,6 +261,14 @@ impl<'msg> MutatorMessageRef<'msg> {
        MutatorMessageRef { msg: msg.msg, arena: &msg.arena }
    }

+    pub fn from_parent(
+        _private: Private,
+        parent_msg: &'msg mut MessageInner,
+        message_field_ptr: RawMessage,
+    ) -> Self {
+        MutatorMessageRef { msg: message_field_ptr, arena: &parent_msg.arena }
+    }
+
    pub fn msg(&self) -> RawMessage {
        self.msg
    }
--- a/src/google/protobuf/compiler/rust/accessors/singular_message.cc
+++ b/src/google/protobuf/compiler/rust/accessors/singular_message.cc
@ -22,42 +22,82 @@ void SingularMessage::InMsgImpl(Context<FieldDescriptor> field) const {

  auto prefix = "crate::" + GetCrateRelativeQualifiedPath(d);

-  if (field.is_cpp()) {
-    field.Emit({{"prefix", prefix},
-                {"field", field.desc().name()},
-                {"getter_thunk", Thunk(field, "get")}},
-               R"rs(
-          pub fn r#$field$(&self) -> $prefix$View {
-            // For C++ kernel, getters automatically return the
-            // default_instance if the field is unset.
-            let submsg = unsafe { $getter_thunk$(self.inner.msg) };
-            $prefix$View::new($pbi$::Private, submsg)
-          }
-        )rs");
-  } else {
-    field.Emit({{"prefix", prefix},
-                {"field", field.desc().name()},
-                {"getter_thunk", Thunk(field, "get")}},
-               R"rs(
-          pub fn r#$field$(&self) -> $prefix$View {
-            let submsg = unsafe { $getter_thunk$(self.inner.msg) };
-            // For upb, getters return null if the field is unset, so we need to
-            // check for null and return the default instance manually. Note that
-            // a null ptr received from upb manifests as Option::None
-            match submsg {
+  field.Emit(
+      {
+          {"prefix", prefix},
+          {"field", field.desc().name()},
+          {"getter_thunk", Thunk(field, "get")},
+          {"clearer_thunk", Thunk(field, "clear")},
+          {
+              "view_body",
+              [&] {
+                if (field.is_upb()) {
+                  field.Emit({}, R"rs(
+              let submsg = unsafe { $getter_thunk$(self.inner.msg) };
+              // For upb, getters return null if the field is unset, so we need
+              // to check for null and return the default instance manually.
+              // Note that a nullptr received from upb manifests as Option::None
+              match submsg {
                // TODO:(b/304357029)
-                None => $prefix$View::new($pbi$::Private, $pbr$::ScratchSpace::zeroed_block($pbi$::Private)),
+                None => $prefix$View::new($pbi$::Private,
+                        $pbr$::ScratchSpace::zeroed_block($pbi$::Private)),
                Some(field) => $prefix$View::new($pbi$::Private, field),
              }
-          }
        )rs");
-  }
+                } else {
+                  field.Emit({}, R"rs(
+              // For C++ kernel, getters automatically return the
+              // default_instance if the field is unset.
+              let submsg = unsafe { $getter_thunk$(self.inner.msg) };
+              $prefix$View::new($pbi$::Private, submsg)
+        )rs");
+                }
+              },
+          },
+          {"submessage_mut",
+           [&] {
+             if (field.is_upb()) {
+               field.Emit({}, R"rs(
+                 let submsg_opt = unsafe { $getter_thunk$(self.inner.msg) };
+                 match submsg_opt {
+                   None => {
+                     $prefix$Mut::new($pbi$::Private,
+                       &mut self.inner,
+                       $pbr$::ScratchSpace::zeroed_block($pbi$::Private))
+                   },
+                   Some(submsg) => {
+                     $prefix$Mut::new($pbi$::Private, &mut self.inner, submsg)
+                }
+              }
+        )rs");
+             } else {
+               field.Emit({}, R"rs(
+                    let submsg = unsafe { $getter_thunk$(self.inner.msg) };
+                    $prefix$Mut::new($pbi$::Private, &mut self.inner, submsg)
+                  )rs");
+             }
+           }},
+      },
+      R"rs(
+            pub fn r#$field$(&self) -> $prefix$View {
+              $view_body$
+            }
+
+            pub fn $field$_mut(&mut self) -> $prefix$Mut {
+              $submessage_mut$
+            }
+
+            pub fn $field$_clear(&mut self) {
+              unsafe { $clearer_thunk$(self.inner.msg) }
+            }
+        )rs");
 }

 void SingularMessage::InExternC(Context<FieldDescriptor> field) const {
  field.Emit(
      {
          {"getter_thunk", Thunk(field, "get")},
+          {"clearer_thunk", Thunk(field, "clear")},
          {"ReturnType",
           [&] {
             if (field.is_cpp()) {
@ -72,6 +112,7 @@ void SingularMessage::InExternC(Context<FieldDescriptor> field) const {
      },
      R"rs(
                  fn $getter_thunk$(raw_msg: $pbi$::RawMessage) -> $ReturnType$;
+                  fn $clearer_thunk$(raw_msg: $pbi$::RawMessage);
               )rs");
 }

@ -79,11 +120,13 @@ void SingularMessage::InThunkCc(Context<FieldDescriptor> field) const {
  field.Emit({{"QualifiedMsg",
               cpp::QualifiedClassName(field.desc().containing_type())},
              {"getter_thunk", Thunk(field, "get")},
+              {"clearer_thunk", Thunk(field, "clear")},
              {"field", cpp::FieldName(&field.desc())}},
             R"cc(
               const void* $getter_thunk$($QualifiedMsg$* msg) {
                 return static_cast<const void*>(&msg->$field$());
               }
+               void $clearer_thunk$($QualifiedMsg$* msg) { msg->clear_$field$(); }
             )cc");
 }

--- a/src/google/protobuf/compiler/rust/generator.cc
+++ b/src/google/protobuf/compiler/rust/generator.cc
@ -98,6 +98,7 @@ void EmitPubUseOfOwnMessages(Context<FileDescriptor>& primary_file,
                        pub use crate::$mod$::$Msg$;
                        // TODO Address use for imported crates
                        pub use crate::$mod$::$Msg$View;
+                        pub use crate::$mod$::$Msg$Mut;
                      )rs");
  }
 }
--- a/src/google/protobuf/compiler/rust/message.cc
+++ b/src/google/protobuf/compiler/rust/message.cc
@ -181,19 +181,33 @@ bool IsSimpleScalar(FieldDescriptor::Type type) {
         type == FieldDescriptor::TYPE_BOOL;
 }

-void GenerateSubView(Context<FieldDescriptor> field) {
+void GetterForViewOrMut(Context<FieldDescriptor> field, bool is_mut) {
+  // If we're dealing with a Mut, the getter must be supplied self.inner.msg()
+  // whereas a View has to be supplied self.msg
  field.Emit(
      {
          {"field", field.desc().name()},
          {"getter_thunk", Thunk(field, "get")},
+          {"self", is_mut ? "self.inner.msg()" : "self.msg"},
          {"Scalar", PrimitiveRsTypeName(field.desc())},
      },
      R"rs(
-      pub fn r#$field$(&self) -> $Scalar$ { unsafe {
-        $getter_thunk$(self.msg)
-      } }
-    )rs");
+    pub fn r#$field$(&self) -> $Scalar$ {
+      unsafe { $getter_thunk$($self$) }
+    }
+  )rs");
+}
+
+void AccessorsForViewOrMut(Context<Descriptor> msg, bool is_mut) {
+  for (int i = 0; i < msg.desc().field_count(); ++i) {
+    auto field = msg.WithDesc(*msg.desc().field(i));
+    if (field.desc().is_repeated()) continue;
+    if (!IsSimpleScalar(field.desc().type())) continue;
+    GetterForViewOrMut(field, is_mut);
+    msg.printer().PrintRaw("\n");
+  }
 }
+
 }  // namespace

 void GenerateRs(Context<Descriptor> msg) {
@ -278,17 +292,8 @@ void GenerateRs(Context<Descriptor> msg) {
                 }  // mod $Msg$_
                )rs");
           }},
-          {"subviews",
-           [&] {
-             for (int i = 0; i < msg.desc().field_count(); ++i) {
-               auto field = msg.WithDesc(*msg.desc().field(i));
-               if (field.desc().is_repeated()) continue;
-               if (!IsSimpleScalar(field.desc().type())) continue;
-               GenerateSubView(field);
-               msg.printer().PrintRaw("\n");
-             }
-           }},
-      },
+          {"accessor_fns_for_views", [&] {AccessorsForViewOrMut(msg, false);}},
+          {"accessor_fns_for_muts", [&] {AccessorsForViewOrMut(msg, true);}}},
      R"rs(
        #[allow(non_camel_case_types)]
        // TODO: Implement support for debug redaction
@ -321,7 +326,7 @@ void GenerateRs(Context<Descriptor> msg) {
          pub fn new(_private: $pbi$::Private, msg: $pbi$::RawMessage) -> Self {
            Self { msg, _phantom: std::marker::PhantomData }
          }
-          $subviews$
+          $accessor_fns_for_views$
        }

        // SAFETY:
@ -352,10 +357,25 @@ void GenerateRs(Context<Descriptor> msg) {

        #[derive(Debug)]
        #[allow(dead_code)]
+        #[allow(non_camel_case_types)]
        pub struct $Msg$Mut<'a> {
          inner: $pbr$::MutatorMessageRef<'a>,
        }

+        impl<'a> $Msg$Mut<'a> {
+          #[doc(hidden)]
+          pub fn new(_private: $pbi$::Private,
+                     parent: &'a mut $pbr$::MessageInner,
+                     msg: $pbi$::RawMessage)
+            -> Self {
+            Self {
+              inner: $pbr$::MutatorMessageRef::from_parent(
+                       $pbi$::Private, parent, msg)
+            }
+          }
+          $accessor_fns_for_muts$
+        }
+
        // SAFETY:
        // - `$Msg$Mut` does not perform any shared mutation.
        // - `$Msg$Mut` is not `Send`, and so even in the presence of mutator