Skip to content

Commit

Permalink
Fix generated code for mutually recursive messages with oneof fields
Browse files Browse the repository at this point in the history
```
message A {
    oneof a {
        int32 i = 1;
        B b = 2;
    }
}

message B {
    oneof b {
        int32 i = 1;
        A a = 2;
    }
}
```
  • Loading branch information
stepancheg committed Jun 19, 2019
1 parent c50ba4b commit 5d70656
Show file tree
Hide file tree
Showing 6 changed files with 66 additions and 14 deletions.
2 changes: 1 addition & 1 deletion protobuf-codegen/src/field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,7 @@ impl<'a> FieldGen<'a> {
}),
}
} else if let Some(oneof) = field.oneof() {
FieldKind::Oneof(OneofField::parse(&oneof, &field, elem))
FieldKind::Oneof(OneofField::parse(&oneof, &field, elem, root_scope))
} else {
let flag = if field.message.scope.file_scope.syntax() == Syntax::PROTO3
&& field.field.get_field_type() != field_descriptor_proto::Type::TYPE_MESSAGE
Expand Down
2 changes: 1 addition & 1 deletion protobuf-codegen/src/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use crate::serde;
/// Message info for codegen
pub(crate) struct MessageGen<'a> {
pub message: &'a MessageWithScope<'a>,
root_scope: &'a RootScope<'a>,
pub root_scope: &'a RootScope<'a>,
type_name: RustIdentWithPath,
pub fields: Vec<FieldGen<'a>>,
pub lite_runtime: bool,
Expand Down
54 changes: 44 additions & 10 deletions protobuf-codegen/src/oneof.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@ use crate::message::MessageGen;
use crate::rust_name::{RustIdent, RustIdentWithPath, RustPath};
use crate::rust_types_values::make_path;
use crate::rust_types_values::RustType;
use crate::scope::OneofWithContext;
use crate::scope::WithScope;
use crate::scope::{FieldWithContext, OneofVariantWithContext};
use crate::serde;
use crate::scope::{OneofWithContext, RootScope};
use crate::{serde, ProtobufAbsolutePath};
use protobuf::descriptor::field_descriptor_proto;
use std::collections::HashSet;

// oneof one { ... }
#[derive(Clone)]
Expand All @@ -27,17 +28,39 @@ pub(crate) struct OneofField<'a> {
}

impl<'a> OneofField<'a> {
// Detecting recursion: if oneof fields contains a self-reference
// or another message which has a reference to self,
// put oneof variant into a box.
fn need_boxed(
field: &FieldWithContext,
root_scope: &RootScope,
owner_name: &ProtobufAbsolutePath,
) -> bool {
let mut visited_messages = HashSet::new();
let mut fields = vec![field.clone()];
while let Some(field) = fields.pop() {
if field.field.get_field_type() == field_descriptor_proto::Type::TYPE_MESSAGE {
let message_name = ProtobufAbsolutePath::from(field.field.get_type_name());
if !visited_messages.insert(message_name.clone()) {
continue;
}
if message_name == *owner_name {
return true;
}
let message = root_scope.find_message(&message_name);
fields.extend(message.fields().into_iter().filter(|f| f.is_oneof()));
}
}
false
}

pub fn parse(
oneof: &OneofWithContext<'a>,
field: &FieldWithContext<'a>,
elem: FieldElem<'a>,
root_scope: &RootScope,
) -> OneofField<'a> {
// detecting recursion
let boxed = if let &FieldElem::Message(ref m) = &elem {
m.message.name_absolute() == oneof.message.name_absolute()
} else {
false
};
let boxed = OneofField::need_boxed(field, root_scope, &oneof.message.name_absolute());

OneofField {
elem,
Expand Down Expand Up @@ -83,6 +106,7 @@ impl<'a> OneofVariantGen<'a> {
oneof: &'a OneofGen<'a>,
variant: OneofVariantWithContext<'a>,
field: &'a FieldGen,
root_scope: &RootScope,
) -> OneofVariantGen<'a> {
OneofVariantGen {
oneof,
Expand All @@ -101,7 +125,12 @@ impl<'a> OneofVariantGen<'a> {
),
field.rust_name
),
oneof_field: OneofField::parse(variant.oneof, &field.proto_field, field.elem().clone()),
oneof_field: OneofField::parse(
variant.oneof,
&field.proto_field,
field.elem().clone(),
oneof.message.root_scope,
),
}
}

Expand Down Expand Up @@ -160,7 +189,12 @@ impl<'a> OneofGen<'a> {
.expect(&format!("field not found by name: {}", v.field.get_name()));
match field.proto_type {
field_descriptor_proto::Type::TYPE_GROUP => None,
_ => Some(OneofVariantGen::parse(self, v, field)),
_ => Some(OneofVariantGen::parse(
self,
v,
field,
self.message.root_scope,
)),
}
})
.collect()
Expand Down
2 changes: 1 addition & 1 deletion protobuf-codegen/src/protobuf_name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ mod relative_path_test {
}
}

#[derive(Clone, Eq, PartialEq, Debug)]
#[derive(Clone, Eq, PartialEq, Debug, Hash)]
pub struct ProtobufAbsolutePath {
pub path: String,
}
Expand Down
2 changes: 1 addition & 1 deletion protobuf-codegen/src/scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ impl<'a> RootScope<'a> {
}

// find message by fully qualified name
pub fn _find_message(&'a self, fqn: &ProtobufAbsolutePath) -> MessageWithScope<'a> {
pub fn find_message(&'a self, fqn: &ProtobufAbsolutePath) -> MessageWithScope<'a> {
match self.find_message_or_enum(fqn) {
MessageOrEnumWithScope::Message(m) => m,
_ => panic!("not a message: {}", fqn),
Expand Down
18 changes: 18 additions & 0 deletions protobuf-test/src/common/v2/test_oneof_recursive_pb.proto
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,21 @@ message LinkedList {
LinkedList node = 2;
}
}

message RecursiveA {
oneof x {
RecursiveB b = 1;
}
}

message RecursiveB {
oneof x {
RecursiveC c = 1;
}
}

message RecursiveC {
oneof x {
RecursiveA a = 1;
}
}

0 comments on commit 5d70656

Please sign in to comment.