Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an option to box oneof fields of kind message #420

Closed
wants to merge 4 commits into from
Closed

Add an option to box oneof fields of kind message #420

wants to merge 4 commits into from

Conversation

hurryabit
Copy link

@hurryabit hurryabit commented Jun 14, 2019

Currently, protoc-rust cannot handle mutually recursive messages with oneofs, even with singular_field_option_box enabled. For instance, the example

message A {
    oneof a {
        int32 i = 1;
        B b = 2;
    }
}

message B {
    oneof b {
        int32 i = 1;
        A a = 2;
    }
}

produces Rust code whose compilation fails with

error[E0072]: recursive type `common::v2::test_oneof_recursive_pb::A` has infinite size
   --> protobuf-test/src/common/v2/test_oneof_recursive_pb.rs:273:1
    |
273 | pub struct A {
    | ^^^^^^^^^^^^ recursive type has infinite size
274 |     // message oneof groups
275 |     pub a: ::std::option::Option<a::A>,
    |     ---------------------------------- recursive without indirection
    |
    = help: insert indirection (e.g., a `Box`, `Rc`, or `&`) at some point to make `common::v2::test_oneof_recursive_pb::A` representable

For recursive messages that do not go through other messages in the recursive cycle, this problem has been fixed by an ad-hoc check to detect these messages and adding Boxes where necessary:

// detecting recursion

Unfortunately, this recursion detection does not easily extend to mutually recursive messages.

This PR proposes to solve the problem by allowing the user to specify when oneof fields should be boxed. Therefore, we introduce an option oneof_field_box that works like singular_field_option_box but on oneof fields of kind message. Fortunately, the infrastructure for doing the boxing/unboxing has already been implemented for the existing ad-hoc recursion check. Thus, we only need to pass the option into the right place and add a few tests.


This change is Reviewable

@stepancheg
Copy link
Owner

Having an option is OK, but I think this case should work automatically without additional options specified.

Unfortunately, this recursion detection does not easily extend to mutually recursive messages.

I think it's doable.

I need to try to implement it, and if you want to try it yourself, go ahead.

@stepancheg
Copy link
Owner

I have implemented recursion detection in 5d70656.

Is oneof_field_box still necessary?

Keeping this PR open because this commit needs to be backported to the stable branch.

Thank you for the bugreport/PR!

@stepancheg
Copy link
Owner

Ported to 2.7 branch c4ca2e9.

2.7 will be released in ~1-2 weeks. Please let me know if this is critical and need to be released earlier.

@stepancheg stepancheg closed this Jun 19, 2019
@stepancheg
Copy link
Owner

I'd apprecate code review BTW, because there's a chance I overlooked something.

@hurryabit
Copy link
Author

@stepancheg I had a look at your fix. The code looks reasonable and simpler than I expected. Thanks a lot for fixing this so quickly!

I'm currently using a pinned version from the master branch. A quick release of 2.7 is hence not critical for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants