Repository: tokio-rs/prost (language: rust)

Issue:
proto3 optional
In documentation you write:

`The table of field modifiers above applies to message fields, except that proto3 message fields without a modifier (the default) will be wrapped in an Option`

But that's not true in my case. Leaving all modifiers will not create an option for me. The only way to do so is use proto2 and optional modifier


Discussion / hints:
I believe the statement as written is correct.  What field type are you seeing this behavior with?
Hi. Actually it was just a message with a combination of required and optional int64
@danburkert I'm seeing something and I can't tell from the README text highlighted by @cemoktra if this is expected...

For this example:

```
syntax = "proto3";

message A { string val = 1; }
message B { string val = 1; }
message C { A a = 1; }
message Example {
  A a = 1;
  B b = 2;
  optional C c = 3;
}
```

Produces this output:

```
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct A {
    #[prost(string, tag = "1")]
    pub val: ::prost::alloc::string::String,
}
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct B {
    #[prost(string, tag = "1")]
    pub val: ::prost::alloc::string::String,
}
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct C {
    #[prost(message, optional, tag = "1")]
    pub a: ::core::option::Option<A>,
}
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct Example {
    #[prost(message, optional, tag = "1")]
    pub a: ::core::option::Option<A>,
    #[prost(message, optional, tag = "2")]
    pub b: ::core::option::Option<B>,
    #[prost(message, optional, tag = "3")]
    pub c: ::core::option::Option<C>,
}
```

The generated files have message fields wrapped as Option<T> for fields that aren't defined as `optional`. This only happens for message types that aren't defined as prost strings, though. Is this expected?


> The generated files have message fields wrapped as Option for fields that aren't defined as optional. This only happens for message types that aren't defined as prost strings, though. Is this expected?

I believe this is expected, but I'm not sure what you mean by 'message types that aren't defined as prost strings' - messages and strings are separate types.
@danburkert Whoops, sorry. This was supposed to be "message field types that aren't defined as strings". For example, they don't end up as `Option<::prost::alloc::string::String>` when they aren't defined as optional -- while `C.a`, which is also not defined as optional, does end up as `Option<A>`. Thanks for clarifying.
I see - yes it's expected.  As the README notes, proto3 message fields without a modifier are be wrapped in an `Option`.  The new proto3 `optional` modifier is for scalar fields, I'm kind of surprised `protoc` even lets you apply it to a message field.
> I'm kind of surprised `protoc` even lets you apply it to a message field.

Even if it doesn't change behavior in the actual generated code, it can document to developers which fields are actually expected to be sometimes unset and which are expected to always be present.
It looks like the new proto3 `optional` works just fine with prost 0.7. I think this can be closed?
Scratch that, prost_build panics with more complex message definitions using optional.

Fix patch:
diff --git a/prost-build/src/code_generator.rs b/prost-build/src/code_generator.rs
index af50c28c4..156f006dc 100644
--- a/prost-build/src/code_generator.rs
+++ b/prost-build/src/code_generator.rs
@@ -237,12 +237,12 @@ impl<'a> CodeGenerator<'a> {
 
             for (idx, oneof) in message.oneof_decl.into_iter().enumerate() {
                 let idx = idx as i32;
-                self.append_oneof(
-                    &fq_message_name,
-                    oneof,
-                    idx,
-                    oneof_fields.remove(&idx).unwrap(),
-                );
+                // optional fields create a synthetic oneof that we want to skip
+                let fields = match oneof_fields.remove(&idx) {
+                    Some(fields) => fields,
+                    None => continue,
+                };
+                self.append_oneof(&fq_message_name, oneof, idx, fields);
             }
 
             self.pop_mod();


Test patch:
diff --git a/tests/src/lib.rs b/tests/src/lib.rs
index b20bc49bc..f56d73e7a 100644
--- a/tests/src/lib.rs
+++ b/tests/src/lib.rs
@@ -597,7 +597,10 @@ mod tests {
 
     #[test]
     fn test_proto3_presence() {
-        let msg = proto3::presence::A { b: Some(42) };
+        let msg = proto3::presence::A {
+            b: Some(42),
+            foo: Some(proto3::presence::a::Foo::C(13)),
+        };
 
         check_message(&msg);
     }
diff --git a/tests/src/proto3_presence.proto b/tests/src/proto3_presence.proto
index d47decce3..1e12c9bcb 100644
--- a/tests/src/proto3_presence.proto
+++ b/tests/src/proto3_presence.proto
@@ -4,4 +4,8 @@ package proto3.presence;
 
 message A {
   optional int32 b = 1;
+
+  oneof foo {
+    int32 c = 2;
+  }
 }