Codegen: proto3 fields shouldn't have Option<> wrappers

[scottlamb]

As you might know, proto3 drops the concepts of default values and even of a field being present or absent at all. You can see the C++ API no longer has a has_foo accessor. The idea is that stuff can be represented with a simple struct.

I'd love for quick-protobuf to fully support this. In tests/rust_protobuf/v3/test_basic_pb.proto, this message:

message Test1 {
    int32 a = 1;
}

currently maps to this generated struct:

#[derive(Debug, Default, PartialEq, Clone)]
pub struct Test1 {
    pub a: Option<i32>,
}

the best proto3 representation leaves out that Option:

#[derive(Debug, Default, PartialEq, Clone)]
pub struct Test1 {
    pub a: i32,
}

with MessageRead and MessageWrite implementations simplified to match. A few reasons I prefer this representation:

• it better matches intended semantics / improves interoperability. Any time you're using the equivalent of has_*, you're assigning a meaning that can't be represented in other languages, so you're losing portability.
• it's simpler to deal with.
• it's more memory-compact. IIUC, even with Rust's fancy field reordering, the whole Option has to be together, so I think it basically doubles the size of the in-memory representation.

fwiw, rust-protobuf drops the Option<> as I've suggested (but doesn't have Cow support, which is what led me to quick-rs).

#[derive(PartialEq,Clone,Default)]
pub struct Test1 {
    // message fields
    pub a: i32,
    // special fields
    unknown_fields: ::protobuf::UnknownFields,
    cached_size: ::protobuf::CachedSize,
}

(I also prefer having unknown_fields, but that's a separate issue.)

[tafia]

Thanks for opening this issue and sorry for the late answer.

I think the best solution would be for pb-rs to have a dedicated flag so people can decide which solution best describes there files. And default to proto3 behaviour indeed.

Shouldn't be too hard. If you want to give it a try please do, else I will try to implement it on my side this month.

[nerdrew]

I have a branch I've been testing for this. Question: should Option<T> for proto3 fields be configurable? I know that proto3 fields always have a default, but having a Vec::new() that is empty seems a little wasteful (though maybe not a big deal).

I'm about to rebase my branch on https://github.com/tafia/quick-protobuf/tree/fixbuild because having OUT_DIR support seems nice.

[tafia]

great!
I'll be merging the fixbuild branch soon.

[scottlamb]

I have a branch I've been testing for this.

Yay!

should Option for proto3 fields be configurable?

IMHO, the only reason to do that is to ease migration from earlier versions of quick-protobuf and/or from proto2.

I know that proto3 fields always have a default, but having a Vec::new() that is empty seems a little wasteful (though maybe not a big deal).

Vec::new() doesn't allocate, so it's not wasteful. The allocation happens when the first value is pushed onto the Vec.

[nerdrew]

Ah, in that case (Vec::new() doesn't allocate) then I see little reason for proto3 fields to be optional.

[nerdrew]

I think this can be closed now, ya?