Skip to content

Commit

Permalink
Introduce submessage view accessors for arbitrary depths
Browse files Browse the repository at this point in the history
Before this CL, we weren't generating view accessors for messages with depth > 1.
We weren't getting to message::GetterForViewOrMut because we were detecting FieldDescriptor::TYPE_MESSAGE and bailing out.

That check has now been expunged, and we now properly emit the right view, even for messages embedded within messages.

Added tests for:
- another level of submsg access depth
- accessing a message declared outside of the current message (that is, not a direct nested message within the same message)
- accessing the accessor of the accessor of the accessor of a recursively defined message

PiperOrigin-RevId: 588460599
  • Loading branch information
honglooker authored and copybara-github committed Dec 6, 2023
1 parent 17b8dd6 commit a74c3bb
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 4 deletions.
16 changes: 16 additions & 0 deletions rust/test/nested.proto
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ package nest;

message Outer {
message Inner {
message InnerSubMsg {
optional bool flag = 1;
}

optional double double = 1;
optional float float = 2;
optional int32 int32 = 3;
Expand All @@ -26,6 +30,7 @@ message Outer {
optional bool bool = 13;
optional string string = 14;
optional bytes bytes = 15;
optional InnerSubMsg innersubmsg = 16;

message SuperInner {
message DuperInner {
Expand All @@ -40,4 +45,15 @@ message Outer {
optional Inner inner = 1;
optional .nest.Outer.Inner.SuperInner.DuperInner.EvenMoreInner
.CantBelieveItsSoInner deep = 2;

optional NotInside notinside = 3;
}

message NotInside {
optional int32 num = 1;
}

message Recursive {
optional Recursive rec = 1;
optional int32 num = 2;
}
18 changes: 18 additions & 0 deletions rust/test/shared/simple_nested_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ 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));
}

#[test]
Expand Down Expand Up @@ -96,3 +97,20 @@ fn test_nested_muts() {
);
// TODO: add mutation tests for strings and bytes
}

#[test]
fn test_msg_from_outside() {
// let's make sure that we're not just working for messages nested inside
// messages, messages from without and within should work
let outer = nested_proto::nest::Outer::new();
assert_that!(outer.notinside().num(), eq(0));
}

#[test]
fn test_recursive_msg() {
let rec = nested_proto::nest::Recursive::new();
assert_that!(rec.num(), eq(0));
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
}
49 changes: 45 additions & 4 deletions src/google/protobuf/compiler/rust/message.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

#include "absl/log/absl_check.h"
#include "absl/log/absl_log.h"
#include "absl/strings/str_cat.h"
#include "absl/strings/match.h"
#include "absl/strings/string_view.h"
#include "google/protobuf/compiler/cpp/helpers.h"
#include "google/protobuf/compiler/cpp/names.h"
Expand Down Expand Up @@ -173,8 +173,50 @@ 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
auto self = is_mut ? "self.inner.msg()" : "self.msg";
auto rsType = PrimitiveRsTypeName(field.desc());
if (fieldType == FieldDescriptor::TYPE_MESSAGE) {
Context<Descriptor> d = field.WithDesc(field.desc().message_type());
auto prefix = "crate::" + GetCrateRelativeQualifiedPath(d);
// TODO: investigate imports breaking submsg accessors
if (absl::StrContains(prefix, "import")) {
return;
}
field.Emit(
{
{"prefix", prefix},
{"field", fieldName},
{"self", self},
{"getter_thunk", getter_thunk},
// TODO: dedupe with singular_message.cc
{
"view_body",
[&] {
if (field.is_upb()) {
field.Emit({}, R"rs(
let submsg = unsafe { $getter_thunk$($self$) };
match submsg {
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(
let submsg = unsafe { $getter_thunk$($self$) };
$prefix$View::new($pbi$::Private, submsg)
)rs");
}
},
},
},
R"rs(
pub fn r#$field$(&self) -> $prefix$View {
$view_body$
}
)rs");
return;
}

auto rsType = PrimitiveRsTypeName(field.desc());
if (fieldType == FieldDescriptor::TYPE_STRING) {
field.Emit(
{
Expand Down Expand Up @@ -250,8 +292,7 @@ void AccessorsForViewOrMut(Context<Descriptor> msg, bool is_mut) {
// TODO - add cord support
if (field.desc().options().has_ctype()) continue;
// TODO
if (field.desc().type() == FieldDescriptor::TYPE_MESSAGE ||
field.desc().type() == FieldDescriptor::TYPE_ENUM ||
if (field.desc().type() == FieldDescriptor::TYPE_ENUM ||
field.desc().type() == FieldDescriptor::TYPE_GROUP)
continue;
GetterForViewOrMut(field, is_mut);
Expand Down

0 comments on commit a74c3bb

Please sign in to comment.