Skip to content

Commit 13000da

Browse files
authored
rust: add short style guide (#4399)
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
1 parent 81fc828 commit 13000da

File tree

1 file changed

+77
-0
lines changed

1 file changed

+77
-0
lines changed

tensorboard/data/server/DEVELOPMENT.md

+77
Original file line numberDiff line numberDiff line change
@@ -135,3 +135,80 @@ You should know:
135135
[`grpc_cli`]: https://github.com/grpc/grpc/blob/master/doc/command_line_tool.md
136136
[grpc/grpc#24734]: https://github.com/grpc/grpc/issues/24374
137137
[reflection]: https://github.com/hyperium/tonic/pull/340
138+
139+
## Style
140+
141+
Google has no official Rust style guide. In lieu of an exhaustive list of rules,
142+
we offer the following advice:
143+
144+
- There is an official [Rust API Guidelines Checklist][cklist]. (Don’t miss
145+
all the prose behind the items, and see also the “External links”
146+
reference.) This can sometimes be helpful to answer questions like, “how
147+
should I name this method?” or “should I validate this invariant, and, if
148+
so, how?”. The standard library and toolchains are also great references for
149+
questions of documentation, interfaces, or implementation: you can always
150+
just ”go see what `String` does”.
151+
152+
These sources provide _guidelines_. Our code can probably afford to be less
153+
completely documented than the standard library.
154+
155+
- Listen to Clippy and bias toward taking its advice. It’s okay to disable
156+
lints by using `#[allow(clippy::some_lint_name)]`: preferably on just a
157+
single item (rather than a module), and preferably with a brief comment.
158+
159+
- Standard correctness guidelines: prefer making it structurally impossible to
160+
panic (avoid `unwrap`/`expect` when possible); use newtypes and visibility
161+
to enforce invariants where appropriate; do not use `unsafe` unless you have
162+
a good reason that you are prepared to justify; etc.
163+
164+
- Write Rust code that will still compile if backward-compatible changes are
165+
made to protobuf definitions whose source of truth is not in this repo
166+
(i.e., those updated by `tensorboard/compat/proto/update.sh`). This means:
167+
168+
- When you construct a protobuf message, spread in `Default::default` even
169+
if you populate all the fields, in case more fields are added:
170+
171+
```rust
172+
let plugin_data = pb::summary_metadata::PluginData {
173+
plugin_name: String::from("scalars"),
174+
content: Vec::new(),
175+
..Default::default()
176+
};
177+
```
178+
179+
This [contradicts `clippy::needless_update`][clippy-nu], so we disable
180+
that lint at the crate level.
181+
182+
- When you consume a protobuf enum, include a default case:
183+
184+
```rust
185+
let class_name = match data_class {
186+
DataClass::Scalar => "scalar",
187+
DataClass::Tensor => "tensor",
188+
DataClass::BlobSequence => "blob sequence",
189+
_ => "unknown", // matching against `_`, not `DataClass::Unknown`
190+
};
191+
```
192+
193+
This way, whoever is updating our copies of the protobuf definitions doesn’t
194+
also have to context-switch in to updating Rust code. This rule doesn’t
195+
apply to `tensorboard.data` protos, since we own those.
196+
197+
We don’t have a compile-time check for this, so just try to be careful.
198+
199+
If you’re reading this because a protobuf change _has_ caused Rust failures,
200+
here are some tips for fixing them:
201+
202+
- For struct initializers: add `..Default::default()` (no trailing comma),
203+
as in the example above.
204+
- For `match` expressions: add a `_ => unimplemented!()` branch, as in the
205+
example above, which will panic at runtime if it’s hit.
206+
- For other contexts: if there’s not an obvious fix, run `git blame` and
207+
ask the original author, or ask another Rust developer.
208+
209+
[cklist]: https://rust-lang.github.io/api-guidelines/checklist.html
210+
[clippy-nu]: https://github.com/rust-lang/rust-clippy/issues/6323
211+
212+
When in doubt, you can always ask a colleague or the internet. Stack Overflow
213+
responses on Rust tend to be both fast and helpful, especially for carefully
214+
posed questions.

0 commit comments

Comments
 (0)