Make it easy to write forward-compatible code: struct builders?

[wchargin]

In protobuf, adding a field to a message is considered a backward compatible change. It’s compatible at the wire format level and also at the source level for many sets of bindings, including the standard bindings for Python, Go, C++, and Java.

Prost makes it easy to directly initialize structs of message type:

// message Person { string name = 1; string email = 2; }
let person = proto::Person {
    name: "A. U. Thor".into(),
    email: "[email protected]".into(),
};

But then if a new type is added to the message, the Rust code fails to compile, because it’s no longer initializing all the fields.

One way to achieve compatibility is to spread in ..Default::default():

let person = proto::Person {
    name: "A. U. Thor".into(),
    email: "[email protected]".into(),
    ..Default::default() // more fields may be added later
};

This works, but (a) you have to remember to do it every time, and (b) it can get a bit spammy when there are multiple layers of nesting:

            ..Default::default()
        },
        ..Default::default()
    },
    ..Default::default()
}

It would be nice to have a way to guarantee better compatibility. One approach would be some kind of optional builder API:

let person = proto::Person::builder()
    .name("A. U. Thor".into())
    .email("[email protected]".into())
    .build();

I don’t know what exactly the right ergonomics are here, especially around nested fields. (I half-wonder if the builder should take a closure, but I think that that might get a bit hairy.)

A nice side-effect of such a builder pattern would be that the methods could take impl Into<FieldType>, so you could pass &str literals rather than having to write .to_string()/.into() everywhere.

An option to set #[non_exhaustive] on structs doesn’t sound quite right to me, since (a) that doesn’t do anything inside the same crate and (b) then you can’t use functional record updates (..default()).

It’s fine with me if the struct fields remain public; we can have a style recommendation that says “always use builders when building messages of a type that you don’t control”, or something. Though being able to control that via a codegen option would be nice, too.

This is an important consideration for using Prost in shared or large codebases, especially monorepos. It’s common to have a shared protobuf definition with autogenerated bindings that many teams depend on. Enabling the proto owners to make forward-compatible changes without breaking other teams’ builds would be a requirement to use Prost.

[danburkert]

I don't agree that adding a field to a message being a backwards compatible wire change is sufficient motivation to also guarantee that it's a source-compatible change. There are plenty of other guaranteed wire-compatible changes that typically are not source compatible in any (typed) language bindings, e.g. widening an integer, moving a field into a oneof, renaming a field, removing an optional field, etc.

I do think #[non_exhaustive] is the way forward for users who want to expose a prost-generated type in a public crate API that needs stability guarantees. I don't personally think this is a wise thing to do, but a lot of people seem to want to do it 🤷 . For workspace-style private codebase I find that the compiler pointing out where code needs to get updated is extremely helpful.

(b) then you can’t use functional record updates (..default())

This appears to work just fine: https://gist.github.com/a64fe9b1dd55d89ed8687381395f02d4

[danburkert]

This appears to work just fine

Maybe that's an artifact of the type being in the same crate? I haven't used #[non_exaustive] in anger since it's a relatively new feature, so I'm not exactly sure what its limitations are.

[wchargin]

Hi! Thanks for your response.

I don't agree that adding a field to a message being a backwards compatible wire change is sufficient motivation to also guarantee that it's a source-compatible change. There are plenty of other guaranteed wire-compatible changes that typically are not source compatible in any (typed) language bindings, e.g. widening an integer, moving a field into a oneof, renaming a field, removing an optional field, etc.

Agreed: not all wire-compatible changes are source-compatible. But adding a field specifically is meant to be both wire-compatible and source-compatible. The guidelines (also linked above) say that fields “may be added to existing APIs in the same version”, and also say a single version “must” be source- as well as wire- and semantic-compatible, which implies the conclusion.

Correspondingly, even though Go, Java, and C++ are all typed, their language bindings treat this as a source-compatible change. Java and C++ only expose getters and setters (and/or builders). Go has structs like Prost does, but omitted fields in Go struct literals are implicitly set to zero values.

For workspace-style private codebase I find that the compiler pointing out where code needs to get updated is extremely helpful.

Absolutely; I’m a huge fan of this workflow as well (“change one type, fix all the errors, it works first try”). To quote from Abseil’s “Use Exhaustive switch Statements Responsibly” (h/t @nfelt), for this to work, you need one of three things:

1. The owner of the enum guarantees no new enumerators will be added,
2. The owner of the enum is willing and able to fix our code when new enumerators are added (e.g. the enum definition is part of the same project),
3. The owner of the enum will not be blocked by breaking our build (e.g. their code is in a separate source control repository), and we’re willing to be forced to update our switch statements when updating to the latest version of the enum-owner’s code.

The “change one type, fix all the errors, it works first try” flow is basically picking option (2), but for published protos this doesn’t work. For protos that aren’t set in stone, this leaves (3), which may be impossible (monorepo) or undesirable (if you want to take the proto semantics).

This appears to work just fine

Maybe that's an artifact of the type being in the same crate?

Yeah, that’s right:

$ cat sub.rs
#[non_exhaustive]
#[derive(Debug, Default)]
pub struct S {
    pub a: i32,
    pub b: i32,
}
$ cat main.rs
extern crate sub;
fn main() {
    let s = sub::S {
        a: 1,
        b: 2,
        ..Default::default()
    };
    dbg!(s);
}
$ rustc sub.rs --crate-type=lib
$ rustc main.rs -L.
error[E0639]: cannot create non-exhaustive struct using struct expression
 --> main.rs:3:13
  |
3 |       let s = sub::S {
  |  _____________^
4 | |         a: 1,
5 | |         b: 2,
6 | |         ..Default::default()
7 | |     };
  | |_____^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0639`.

By the way, I played with sketching out an API for this, and was pleased to see that if you put the builder-related methods in traits, then the whole thing works fine even if the proto happens to have a field called build or something else pathological. (Maybe obvious to you, but it’s not too often that I see the tangible benefits of trait methods being namespaced as they are.)

[danburkert]

The guidelines (also linked above) say that fields “may be added to existing APIs in the same version”, and also says a single version “must” be source- as well as wire- and semantic-compatible.

That link appears to be to Google Cloud Client guidelines, that's not really relevant to the design of a general purpose protobuf library. I haven't seen any protobuf docs or guidelines that say anything about source compatibility of generated code in any context, and in practice there is no such guarantee as I pointed out.

WRT #[non_exhaustive] that's pretty unfortunate, it makes it a little less useful of a tool. It's still possible to use #[non_exhaustive] without a builder API though, doing something like:

let mut msg = MyMessage::default();
msg.foo = "bar".to_owned();
...

[danburkert]

Also note that you could in theory attach a general purpose builder derive macro to prost generated types, e.g. https://crates.io/crates/derive_builder. I haven't tried this so maybe there's some sticking points, but I believe all the hooks are available in prost-build to make it possible.

[Kixunil]

Combination of #[non_exhaustive] and builder sounds like a best option to me since then it's possible to match on the struct and access the fields directly without creating compatibility hazard.

I kinda like the idea of general-purpose builder although I'm not sure if there's a reason why prost-specific could be better. One thing that comes to my mind is avoiding naming conflicts (if someone funny names protobuf messages Foo and FooBuilder).

My use case is a library for quite large gRPC protocol that I don't have the time to write cleaner bindings for. (I wish I had.)

[damelLP]

https://lib.rs/crates/proto-builder-trait

This seems to do the job? Maybe it could be upstreamed?

[mzabaluev]

derive_builder does away with a lot of the boilerplate and is very flexible, but I think a code generator using specific knowledge of the protobuf type system would be able to provide a better API out of the box.

Consider repeated fields:

message HelloRequest {
    repeated string flags = 2;
}

The builder for HelloRequest could provide this setter method:

impl HelloRequestBuilder {
    pub fn flags<T>(mut self, flags: impl IntoIterator<Item = T>) -> Self
    where T: Into<String>,
    {
        self.inner.flags = flags.into_iter().map(Into::into).collect();
        self
    }
}

Stylistically, I don't like the builder type becoming a part of the API that you have to use by name. It's better to add an associated fn to the message type, and have the builder type tucked away in a submodule:

impl HelloRequest {
    pub fn builder() -> hello_request::HelloRequestBuilder {
        todo!()
    }
}

[mzabaluev]

As realized in #901, a protobuf-specific generator can provide much more ergonomic API than a general purpose derive.