1 Our approach to "clean code" is two-fold:
3 * We generally don't block PRs on style changes.
4 * At the same time, all code in rust-analyzer is constantly refactored.
6 It is explicitly OK for a reviewer to flag only some nits in the PR, and then send a follow-up cleanup PR for things which are easier to explain by example, cc-ing the original author.
7 Sending small cleanup PRs (like renaming a single local variable) is encouraged.
9 When reviewing pull requests prefer extending this document to leaving
10 non-reusable comments on the pull request itself.
16 Everyone knows that it's better to send small & focused pull requests.
17 The problem is, sometimes you *have* to, eg, rewrite the whole compiler, and that just doesn't fit into a set of isolated PRs.
19 The main things to keep an eye on are the boundaries between various components.
20 There are three kinds of changes:
22 1. Internals of a single component are changed.
23 Specifically, you don't change any `pub` items.
24 A good example here would be an addition of a new assist.
26 2. API of a component is expanded.
27 Specifically, you add a new `pub` function which wasn't there before.
28 A good example here would be expansion of assist API, for example, to implement lazy assists or assists groups.
30 3. A new dependency between components is introduced.
31 Specifically, you add a `pub use` reexport from another crate or you add a new line to the `[dependencies]` section of `Cargo.toml`.
32 A good example here would be adding reference search capability to the assists crates.
34 For the first group, the change is generally merged as long as:
36 * it works for the happy case,
38 * it doesn't panic for the unhappy case.
40 For the second group, the change would be subjected to quite a bit of scrutiny and iteration.
41 The new API needs to be right (or at least easy to change later).
42 The actual implementation doesn't matter that much.
43 It's very important to minimize the amount of changed lines of code for changes of the second kind.
44 Often, you start doing a change of the first kind, only to realise that you need to elevate to a change of the second kind.
45 In this case, we'll probably ask you to split API changes into a separate PR.
47 Changes of the third group should be pretty rare, so we don't specify any specific process for them.
48 That said, adding an innocent-looking `pub use` is a very simple way to break encapsulation, keep an eye on it!
50 Note: if you enjoyed this abstract hand-waving about boundaries, you might appreciate
51 https://www.tedinski.com/2018/02/06/system-boundaries.html
53 ## Crates.io Dependencies
55 We try to be very conservative with usage of crates.io dependencies.
56 Don't use small "helper" crates (exception: `itertools` is allowed).
57 If there's some general reusable bit of code you need, consider adding it to the `stdx` crate.
59 **Rationale:** keep compile times low, create ecosystem pressure for faster
60 compiles, reduce the number of things which might break.
64 We don't have specific rules around git history hygiene.
65 Maintaining clean git history is strongly encouraged, but not enforced.
66 Use rebase workflow, it's OK to rewrite history during PR review process.
67 After you are happy with the state of the code, please use [interactive rebase](https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History) to squash fixup commits.
69 Avoid @mentioning people in commit messages and pull request descriptions(they are added to commit message by bors).
70 Such messages create a lot of duplicate notification traffic during rebases.
72 If possible, write commit messages from user's perspective:
76 Goto definition works inside macros
79 Use original span for FileId
82 This makes it easier to prepare a changelog.
84 If the change adds a new user-visible functionality, consider recording a GIF with [peek](https://github.com/phw/peek) and pasting it into the PR description.
86 **Rationale:** clean history is potentially useful, but rarely used.
87 But many users read changelogs.
91 We don't enforce Clippy.
92 A number of default lints have high false positive rate.
93 Selectively patching false-positives with `allow(clippy)` is considered worse than not using Clippy at all.
94 There's `cargo xtask lint` command which runs a subset of low-FPR lints.
95 Careful tweaking of `xtask lint` is welcome.
96 Of course, applying Clippy suggestions is welcome as long as they indeed improve the code.
98 **Rationale:** see [rust-lang/clippy#5537](https://github.com/rust-lang/rust-clippy/issues/5537).
104 Most tests in rust-analyzer start with a snippet of Rust code.
105 This snippets should be minimal -- if you copy-paste a snippet of real code into the tests, make sure to remove everything which could be removed.
107 It also makes sense to format snippets more compactly (for example, by placing enum definitions like `enum E { Foo, Bar }` on a single line),
108 as long as they are still readable.
110 When using multiline fixtures, use unindented raw string literals:
114 fn inline_field_shorthand() {
116 inline_local_variable,
136 There are many benefits to this:
138 * less to read or to scroll past
139 * easier to understand what exactly is tested
140 * less stuff printed during printf-debugging
141 * less time to run test
143 Formatting ensures that you can use your editor's "number of selected characters" feature to correlate offsets with test's source code.
145 ## Function Preconditions
147 Express function preconditions in types and force the caller to provide them (rather than checking in callee):
151 fn frbonicate(walrus: Walrus) {
156 fn frobnicate(walrus: Option<Walrus>) {
157 let walrus = match walrus {
165 **Rationale:** this makes control flow explicit at the call site.
166 Call-site has more context, it often happens that the precondition falls out naturally or can be bubbled up higher in the stack.
168 Avoid splitting precondition check and precondition use across functions:
174 if let Some(contents) = string_literal_contents(s) {
179 fn string_literal_contents(s: &str) -> Option<&str> {
180 if s.starts_with('"') && s.ends_with('"') {
181 Some(&s[1..s.len() - 1])
190 if is_string_literal(s) {
191 let contents = &s[1..s.len() - 1];
195 fn is_string_literal(s: &str) -> bool {
196 s.starts_with('"') && s.ends_with('"')
200 In the "Not as good" version, the precondition that `1` is a valid char boundary is checked in `is_string_literal` and used in `foo`.
201 In the "Good" version, the precondition check and usage are checked in the same block, and then encoded in the types.
203 **Rationale:** non-local code properties degrade under change.
205 When checking a boolean precondition, prefer `if !invariant` to `if negated_invariant`:
219 **Rationale:** its useful to see the invariant relied upon by the rest of the function clearly spelled out.
224 Prefer `stdx::assert_never!` to standard `assert!`.
228 If a field can have any value without breaking invariants, make the field public.
229 Conversely, if there is an invariant, document it, enforce it in the "constructor" function, make the field private, and provide a getter.
230 Never provide setters.
232 Getters should return borrowed data:
236 // Invariant: never empty
238 middle_name: Option<String>
243 fn first_name(&self) -> &str { self.first_name.as_str() }
244 fn middle_name(&self) -> Option<&str> { self.middle_name.as_ref() }
249 fn first_name(&self) -> String { self.first_name.clone() }
250 fn middle_name(&self) -> &Option<String> { &self.middle_name }
254 **Rationale:** we don't provide public API, it's cheaper to refactor than to pay getters rent.
255 Non-local code properties degrade under change, privacy makes invariant local.
256 Borrowed own data discloses irrelevant details about origin of data.
257 Irrelevant (neither right nor wrong) things obscure correctness.
261 Prefer `Default` to zero-argument `new` function
282 Prefer `Default` even it has to be implemented manually.
284 **Rationale:** less typing in the common case, uniformity.
286 Use `Vec::new` rather than `vec![]`. **Rationale:** uniformity, strength
289 ## Functions Over Objects
291 Avoid creating "doer" objects.
292 That is, objects which are created only to execute a single action.
296 do_thing(arg1, arg2);
299 ThingDoer::new(arg1, arg2).do();
302 Note that this concerns only outward API.
303 When implementing `do_thing`, it might be very useful to create a context object.
306 pub fn do_thing(arg1: Arg1, arg2: Arg2) -> Res {
307 let mut ctx = Ctx { arg1, arg2 }
312 arg1: Arg1, arg2: Arg2
316 fn run(self) -> Res {
322 The difference is that `Ctx` is an impl detail here.
324 Sometimes a middle ground is acceptable if this can save some busywork:
327 ThingDoer::do(arg1, arg2);
329 pub struct ThingDoer {
330 arg1: Arg1, arg2: Arg2,
334 pub fn do(arg1: Arg1, arg2: Arg2) -> Res {
335 ThingDoer { arg1, arg2 }.run()
337 fn run(self) -> Res {
343 **Rationale:** not bothering the caller with irrelevant details, not mixing user API with implementor API.
345 ## Avoid Monomorphization
347 Avoid making a lot of code type parametric, *especially* on the boundaries between crates.
351 fn frbonicate(f: impl FnMut()) {
352 frobnicate_impl(&mut f)
354 fn frobnicate_impl(f: &mut dyn FnMut()) {
359 fn frbonicate(f: impl FnMut()) {
364 Avoid `AsRef` polymorphism, it pays back only for widely used libraries:
368 fn frbonicate(f: &Path) {
372 fn frbonicate(f: impl AsRef<Path>) {
376 **Rationale:** Rust uses monomorphization to compile generic code, meaning that for each instantiation of a generic functions with concrete types, the function is compiled afresh, *per crate*.
377 This allows for exceptionally good performance, but leads to increased compile times.
378 Runtime performance obeys 80%/20% rule -- only a small fraction of code is hot.
379 Compile time **does not** obey this rule -- all code has to be compiled.
381 ## Appropriate String Types
383 When interfacing with OS APIs, use `OsString`, even if the original source of
384 data is utf-8 encoded. **Rationale:** cleanly delineates the boundary when the
385 data goes into the OS-land.
387 Use `AbsPathBuf` and `AbsPath` over `std::Path`. **Rationale:** rust-analyzer is
388 a long-lived process which handles several projects at the same time. It is
389 important not to leak cwd by accident.
391 # Premature Pessimization
395 Avoid writing code which is slower than it needs to be.
396 Don't allocate a `Vec` where an iterator would do, don't allocate strings needlessly.
400 use itertools::Itertools;
402 let (first_word, second_word) = match text.split_ascii_whitespace().collect_tuple() {
408 let words = text.split_ascii_whitespace().collect::<Vec<_>>();
409 if words.len() != 2 {
414 **Rationale:** not allocating is almost often faster.
416 ## Push Allocations to the Call Site
418 If allocation is inevitable, let the caller allocate the resource:
422 fn frobnicate(s: String) {
427 fn frobnicate(s: &str) {
428 let s = s.to_string();
433 **Rationale:** reveals the costs.
434 It is also more efficient when the caller already owns the allocation.
438 Prefer `rustc_hash::FxHashMap` and `rustc_hash::FxHashSet` instead of the ones in `std::collections`.
440 **Rationale:** they use a hasher that's significantly faster and using them consistently will reduce code size by some small amount.
442 ## Avoid Intermediate Collections
444 When writing a recursive function to compute a sets of things, use an accumulator parameter instead of returning a fresh collection.
445 Accumulator goes first in the list of arguments.
449 pub fn reachable_nodes(node: Node) -> FxHashSet<Node> {
450 let mut res = FxHashSet::default();
454 fn go(acc: &mut FxHashSet<Node>, node: Node) {
456 for n in node.neighbors() {
462 pub fn reachable_nodes(node: Node) -> FxHashSet<Node> {
463 let mut res = FxHashSet::default();
465 for n in node.neighbors() {
466 res.extend(reachable_nodes(n));
472 **Rational:** re-use allocations, accumulator style is more concise for complex cases.
478 Separate import groups with blank lines.
479 Use one `use` per crate.
481 Module declarations come before the imports.
482 Order them in "suggested reading order" for a person new to the code base.
491 // Second, external crates (both crates.io crates and other rust-analyzer crates).
492 use crate_foo::{ ... }
493 use crate_bar::{ ... }
495 // Then current crate.
498 // Finally, parent and child modules, but prefer `use crate::`.
502 **Rationale:** consistency.
503 Reading order is important for new contributors.
504 Grouping by crate allows to spot unwanted dependencies easier.
508 Qualify items from `hir` and `ast`.
514 fn frobnicate(func: hir::Function, strukt: ast::Struct) {}
518 use syntax::ast::Struct;
520 fn frobnicate(func: Function, strukt: Struct) {}
523 **Rationale:** avoids name clashes, makes the layer clear at a glance.
525 When implementing traits from `std::fmt` or `std::ops`, import the module:
531 impl fmt::Display for RenameError {
532 fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { .. }
536 impl std::fmt::Display for RenameError {
537 fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { .. }
543 impl Deref for Widget {
545 fn deref(&self) -> &str { .. }
549 **Rationale:** overall, less typing.
550 Makes it clear that a trait is implemented, rather than used.
552 Avoid local `use MyEnum::*` imports.
553 **Rationale:** consistency.
555 Prefer `use crate::foo::bar` to `use super::bar` or `use self::bar::baz`.
556 **Rationale:** consistency, this is the style which works in all cases.
560 Optimize for the reader who sees the file for the first time, and wants to get a general idea about what's going on.
561 People read things from top to bottom, so place most important things first.
563 Specifically, if all items except one are private, always put the non-private item on top.
567 pub(crate) fn frobnicate() {
572 struct Helper { stuff: i32 }
582 struct Helper { stuff: i32 }
584 pub(crate) fn frobnicate() {
595 If there's a mixture of private and public items, put public items first.
597 Put `struct`s and `enum`s first, functions and impls last. Order type declarations in top-down manner.
627 **Rationale:** easier to get the sense of the API by visually scanning the file.
628 If function bodies are folded in the editor, the source code should read as documentation for the public API.
632 Use boring and long names for local variables ([yay code completion](https://github.com/rust-analyzer/rust-analyzer/pull/4162#discussion_r417130973)).
633 The default name is a lowercased name of the type: `global_state: GlobalState`.
634 Avoid ad-hoc acronyms and contractions, but use the ones that exist consistently (`db`, `ctx`, `acc`).
635 Prefer American spelling (color, behavior).
639 * `res` -- "result of the function" local variable
640 * `it` -- I don't really care about the name
641 * `n_foo` -- number of foos
642 * `foo_idx` -- index of `foo`
644 Many names in rust-analyzer conflict with keywords.
645 We use mangled names instead of `r#ident` syntax:
657 **Rationale:** consistency.
665 fn foo() -> Option<Bar> {
674 fn foo() -> Option<Bar> {
683 **Rationale:** reduce congnitive stack usage.
687 Use `<`/`<=`, avoid `>`/`>=`.
691 assert!(lo <= x && x <= hi);
694 assert!(x >= lo && x <= hi>);
697 **Rationale:** Less-then comparisons are more intuitive, they correspond spatially to [real line](https://en.wikipedia.org/wiki/Real_line).
702 Use `T![foo]` instead of `SyntaxKind::FOO_KW`.
707 T![true] | T![false] => true,
714 SyntaxKind::TRUE_KW | SyntaxKind::FALSE_KW => true,
719 **Rationale:** The macro uses the familiar Rust syntax, avoiding ambiguities like "is this a brace or bracket?".
723 For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines.
724 If the line is too long, you want to split the sentence in two :-)
726 **Rationale:** much easier to edit the text and read the diff.