getter/setter generated for optional enum fields should use Option

[jmhain]

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]

I agree. Are you willing to do the work?

[giangndm]

@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? https://github.com/giangndm/prost/pull/1

[caspermeijn]

Would this PR also address your problem? https://github.com/tokio-rs/prost/pull/1079

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

[mzabaluev]

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.