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

fix: boxed oneof field #1210

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

xxchan
Copy link

@xxchan xxchan commented Dec 20, 2024

fix #1159

Signed-off-by: xxchan [email protected]

Copy link
Collaborator

@caspermeijn caspermeijn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test will pass without the other changes. CodeGenerator already makes the OneOf field a Box<T> if self.boxed() == true on line 667.

Can you explain why the other changes are needed?

tests/src/boxed_field.rs Outdated Show resolved Hide resolved
@@ -15,6 +15,8 @@ impl Field {
pub fn new(attrs: &[Meta]) -> Result<Option<Field>, Error> {
let mut ty = None;
let mut tags = None;
let mut boxed = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see where this variable is used. Can you explain what this does?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember I see some unknown attribute error while developing, so I copied the code from message.rs.
https://github.com/xxchan/prost/blob/56b96023a17a0b15026fedc63ec9a5ea8b128ddf/prost-derive/src/field/message.rs#L26-L27

But you are right, this seems to a unnecessary mistake.

Signed-off-by: xxchan <[email protected]>
Signed-off-by: xxchan <[email protected]>
Signed-off-by: xxchan <[email protected]>
Signed-off-by: xxchan <[email protected]>
Copy link
Author

@xxchan xxchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @caspermeijn, thanks for the prompt review! Reverted unnecessary changes, and made CI Green.

I think this test will pass without the other changes.

I'm not sure what you mean by "other changes" here.

But anyway, here's how it works:

As mentioned in #1159, the type for the oneof field was correct. What was wrong is the generated merge function.

I noticed there are #[prost(boxed)] on normal messages fields, but not on the oneof field. After adding it, it works.

pub struct Foo {
    #[prost(message, optional, boxed, tag = "1")]
    pub bar: ::core::option::Option<::prost::alloc::boxed::Box<Bar>>,
    #[prost(oneof = "foo::OneofField", tags = "2, 3")]
    pub oneof_field: ::core::option::Option<foo::OneofField>,
}
/// Nested message and enum types in `Foo`.
    pub mod foo {
        pub enum OneofField {
            #[prost(string, tag = "2")]
            Baz(::prost::alloc::string::String),
// This PR added "boxed" here 
            #[prost(message, boxed, tag = "3")]  
            BoxQux(::prost::alloc::boxed::Box<super::Bar>),
        }
	}

@@ -15,6 +15,8 @@ impl Field {
pub fn new(attrs: &[Meta]) -> Result<Option<Field>, Error> {
let mut ty = None;
let mut tags = None;
let mut boxed = false;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember I see some unknown attribute error while developing, so I copied the code from message.rs.
https://github.com/xxchan/prost/blob/56b96023a17a0b15026fedc63ec9a5ea8b128ddf/prost-derive/src/field/message.rs#L26-L27

But you are right, this seems to a unnecessary mistake.

@xxchan xxchan requested a review from caspermeijn December 23, 2024 05:04
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.

prost_config::Config::boxed generates code doesn't compile
2 participants