rust: add short style guide

[wchargin]

Summary:
Main purpose is to include an item about proto compatibility, since it’s
slightly non-obvious. Otherwise, there’s not too much to say: there are
other guidelines that we should follow, but I would prefer that they
come up naturally in code review rather than erecting barriers to entry.

Test Plan:
Click links. Links work.

wchargin-branch: rust-style-guide

[wchargin]

(Reviewer selection is because @nfelt and I already discussed the proto
business, but feel free to review anyway if you like.)

[psybuzz]

These are useful tips. I don't have any specific feedback, but lgtm

[nfelt]

Thanks for writing this up! LGTM aside from the one comment.

[nfelt]

I'm a little lukewarm about this rule, in particular when it comes to protobuf definitions that we clearly control ourselves (like the ones in tensorboard.data; and possibly in the future if we were to explicitly fork the event / summary protos and stop mirroring TF). Had responded on the thread, ICYMI: #4385 (comment)

I realize that it's a little hassle to have to go and update the code everywhere if we decide to change the proto, but that work isn't any different from if we'd made the proto definition a Rust struct natively and had decided to add a field, and yet we don't write all our struct literals with a trailing ..Default::default() in case they're expanded in the future. I think this is more or less a similar argument as described in https://abseil.io/tips/147 about exhaustive switch cases in C++; it seems okay to me to say that we will sign up for either 1 or 2 (3 wouldn't apply unless we were to include the mirrored TF protobufs in this as well).

I'd be more sympathetic here if we could at least make this a compile-time check or a linter check (or if Prost generated a proto-creation API that was future-proof, like a withFoo() builder pattern, that we could use instead), but if we can't, it seems like there's a decent chance eventually at least one case will slip through, and since we can't prevent that possibility we'll want to be aware of the possibility that a protobuf schema addition will break the Rust build even if we try to adhere to this rule.

Would be curious to hear thoughts from others and whether you think there's a particularly ironclad reason we should strive to provide protobuf changes with this kind of guarantee. (FWIW, the only backwards-compatibility guarantee I'm aware of with protobuf is at the wire level; I am not aware of any general guarantees about backwards-compatibility for generated code when the definition changes, though I'll concede that in other generated code I've worked with, at least adding a field has always been backwards-compatible.)

[wchargin]

Ah, I had indeed missed that; thanks for surfacing it!

It’s fine with me to waive this suggestion for protos that we clearly
control ourselves: i.e., those whose source of truth is in this repo.

My main goal with this suggestion was that running update.sh to pull
in new TensorFlow protos should be a safe operation, because (a) it may
be run by people who aren’t familiar with this code, and (b) it tends to
happen around release time, so we would prefer not to rush any changes.
That’s why I see it differently than changing a normal Rust struct: in
that case, you’re deliberately making a change to the Rust code rather
than just making what should be an automatable change.

it seems like there's a decent chance eventually at least one case will slip through, and since we can't prevent that possibility we'll want to be aware of the possibility

Agreed, hence my tips for handling such a situation in this document.

[from link] The other alternative that came to mind would be to initialize structs to default-empty, then mutate to populate fields; needing mutation is a downside although I think in some of the more convoluted multi-layer struct cases this may be significantly more concise.

I don’t mind the mutation, but the problem that you run into here is the
loss of type information:

        let req = Request::new(data::ReadScalarsRequest {
            experiment_id: "123".to_string(),
            plugin_filter: Some(data::PluginFilter {
                plugin_name: "scalars".to_string(),
                ..Default::default()
            }),
            downsample: Some(data::Downsample {
                num_points: 1000,
                ..Default::default()
            }),
            ..Default::default()
        });

        let mut req = data::ReadScalarsRequest::default();
        req.experiment_id = "123".to_string();
        req.plugin_filter = Some(data::PluginFilter::default());
        req.plugin_filter.as_mut().unwrap().plugin_name = "scalars".to_string();
        //               ^^^^^^^^^^^^^^^^^^
        req.downsample = Some(data::Downsample::default());
        req.downsample.as_mut().unwrap().num_points = 1000;
        //            ^^^^^^^^^^^^^^^^^^
        let req = Request::new(req);

…which gets worse with more structure (deeper nesting, repeated fields).
I guess you can kind of roll your own nested builders thanks to Rust’s
expression blocks:

        let req = Request::new({
            let mut req = data::ReadScalarsRequest::default();
            req.experiment_id = "123".to_string();
            req.plugin_filter = Some({
                let mut pf = data::PluginFilter::default();
                pf.plugin_name = "scalars".to_string();
                pf
            });
            req.downsample = Some({
                let mut ds = Some(data::Downsample::default());
                ds.num_points = 1000;
                ds
            });
            req
        })

…and while this does literally about the ..Default::default() spreads,
it doesn’t really seem to satisfy the spirit of your objection. That is:
it’s still a bit noisy.

$0.02: I personally don’t find the ..Default::default() spam that
bad, but I accept that reasonable people may, and if there were shorter
sugar then I would happily use it.

We could macro up some builders:

        let req = Request::new(proto!(data::ReadScalarsRequest {
            experiment_id: "123".to_string(),
            plugin_filter: proto!(data::PluginFilter, {
                plugin_name: "scalars".to_string(),
            }),
            downsample: proto!(data::Downsample, {
                num_points: 1000,
            }),
        }));

…and while I won’t run out and do this unilaterally, I also wouldn’t mind it
if it would make you happier and if it doesn’t break type-directed
autocompletion (which I think I can wrangle).

I am not aware of any general guarantees about backwards-compatibility for generated code when the definition changes

You kind of implicitly have this on any proto_library with public
visibility in Google’s monorepo, though, right? I agree that they don’t
apply to all wire-compatible changes (renames, most obviously), but my
impression has been that people feel free to add fields.

Also, https://google.aip.dev/180#adding-components says that adding
“components” (incl. fields) is permitted within the same major version,
which implies that they are source-compatible, since “[code] written
against a previous version must compile against a newer version”.

What do you think about keeping the recommendation just for protos whose
source of truth is not in this repo? i.e., those not affected by the
update.sh script?

[wchargin]

I tried the macro thing for 15 minutes. A bit harder than I thought. Not
pursuing further at this time.

[nfelt]

It's fine by me as you have it, and thanks for the pointer to https://google.aip.dev/180#adding-components - that does indeed formalize this rule. That being said, I think our problem is a bit more fundamental than one that could be patched up by an optional convention (whether macro-assisted or not): Prost generated code in combination with Rust struct literals having to be exhaustive just fundamentally isn't compatible with the API, since it's trivially possible to write code against the generated API that breaks when you add a field.

I think we would need rust-lang/rfcs#1806 (see also related rust-lang/rfcs#1458) to fix this, but it's been postponed :( Either that or some way that Prost structs could disallow use struct literal initialization entirely in favor of forcing you through some sort of builder API, but I'm guessing that eliminates a lot of the "idiomatic Rust flavor" that Prost was trying to offer in the first place.

[wchargin]

Okay; thanks! Agreed, I think; builders are fairly common in Rust
(stdlib examples: Command, thread::Builder, DirBuilder,
probably more), though certainly struct initialization is simpler.

I’ve opened a Prost feature request for builders. If maintainers
seem open to it, maybe I’ll take a stab at implementation.

[wchargin]

(oops, did not mean to re-request from @psybuzz; belay that)