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

getter/setter generated for optional enum fields should use Option #1027

Open
jmhain opened this issue Apr 13, 2024 · 4 comments
Open

getter/setter generated for optional enum fields should use Option #1027

jmhain opened this issue Apr 13, 2024 · 4 comments

Comments

@jmhain
Copy link

jmhain commented Apr 13, 2024

Currently if you have an enum field marked as optional, prost wraps the i32 field in an Option as with other scalar values. However, the generated getter in this case still returns MyEnum instead of Option<MyEnum>, silently converting a None into the default value. This burned me recently, and it's also inconsistent with the fact that the i32 fields are wrapped in Option.

@caspermeijn
Copy link
Collaborator

I agree. Are you willing to do the work?

@giangndm
Copy link

giangndm commented May 6, 2024

@caspermeijn I have the same issues, which make me have to use a trick to convert from Option. I checked the source code and found that it has some strange behavior in

https://github.com/tokio-rs/prost/blob/baddf9828596bb7e2b908f5d4997542ebe59a7c3/prost-derive/src/field/scalar.rs#L308-326 and https://github.com/tokio-rs/prost/blob/baddf9828596bb7e2b908f5d4997542ebe59a7c3/prost-derive/src/field/scalar.rs#L354-377

It returns a default value with Kind::Optional. I think that with Kind::Optional, we should return None if it's not set.
I tried to change the above code, but it affected a lot of other code, because #[prost(optional)] is used a lot.

For example, with

pub struct FieldDescriptorProto {
    #[prost(string, optional, tag = "1")]
    pub name: ::core::option::Option<::prost::alloc::string::String>,

Instead of checking if name is Some or None, some other code uses a function from a code macro like:

impl OneofField {
    fn new(descriptor: OneofDescriptorProto, fields: Vec<Field>, path_index: i32) -> Self {
        Self {
            descriptor,
            fields,
            path_index,
        }
    }

    fn rust_name(&self) -> String {
        to_snake(self.descriptor.name())
    }
}

I think we should have another function if we need to get name with a fallback to a default value, like name_fallback_default. As I'm new to working with Prost source code, I'm not familiar with it enough to know the best direction. Could you please give me some advice on this case?

Ps: I just implemented the patch in my fork. Can you take a look?
giangndm#1

@caspermeijn
Copy link
Collaborator

Would this PR also address your problem? #1079

It changes the field type to Option<OpenEnum<MyEnum>>. OpenEnum had a known() helper function that returns Option<MyEnum>.

@mzabaluev
Copy link
Contributor

#1079 does not address this issue, as I did not change the behavior of prost-derive in other aspects than the value type of the enum fields.

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

No branches or pull requests

4 participants