aboutsummaryrefslogtreecommitdiff
path: root/docs/dev/style.md
diff options
context:
space:
mode:
Diffstat (limited to 'docs/dev/style.md')
-rw-r--r--docs/dev/style.md212
1 files changed, 212 insertions, 0 deletions
diff --git a/docs/dev/style.md b/docs/dev/style.md
new file mode 100644
index 000000000..1c68f5702
--- /dev/null
+++ b/docs/dev/style.md
@@ -0,0 +1,212 @@
1Our approach to "clean code" is two-fold:
2
3* We generally don't block PRs on style changes.
4* At the same time, all code in rust-analyzer is constantly refactored.
5
6It 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.
7Sending small cleanup PRs (like renaming a single local variable) is encouraged.
8
9# Scale of Changes
10
11Everyone knows that it's better to send small & focused pull requests.
12The problem is, sometimes you *have* to, eg, rewrite the whole compiler, and that just doesn't fit into a set of isolated PRs.
13
14The main things to keep an eye on are the boundaries between various components.
15There are three kinds of changes:
16
171. Internals of a single component are changed.
18 Specifically, you don't change any `pub` items.
19 A good example here would be an addition of a new assist.
20
212. API of a component is expanded.
22 Specifically, you add a new `pub` function which wasn't there before.
23 A good example here would be expansion of assist API, for example, to implement lazy assists or assists groups.
24
253. A new dependency between components is introduced.
26 Specifically, you add a `pub use` reexport from another crate or you add a new line to the `[dependencies]` section of `Cargo.toml`.
27 A good example here would be adding reference search capability to the assists crates.
28
29For the first group, the change is generally merged as long as:
30
31* it works for the happy case,
32* it has tests,
33* it doesn't panic for the unhappy case.
34
35For the second group, the change would be subjected to quite a bit of scrutiny and iteration.
36The new API needs to be right (or at least easy to change later).
37The actual implementation doesn't matter that much.
38It's very important to minimize the amount of changed lines of code for changes of the second kind.
39Often, you start doing a change of the first kind, only to realise that you need to elevate to a change of the second kind.
40In this case, we'll probably ask you to split API changes into a separate PR.
41
42Changes of the third group should be pretty rare, so we don't specify any specific process for them.
43That said, adding an innocent-looking `pub use` is a very simple way to break encapsulation, keep an eye on it!
44
45Note: if you enjoyed this abstract hand-waving about boundaries, you might appreciate
46https://www.tedinski.com/2018/02/06/system-boundaries.html
47
48# Crates.io Dependencies
49
50We try to be very conservative with usage of crates.io dependencies.
51Don't use small "helper" crates (exception: `itertools` is allowed).
52If there's some general reusable bit of code you need, consider adding it to the `stdx` crate.
53
54# Minimal Tests
55
56Most tests in rust-analyzer start with a snippet of Rust code.
57This 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.
58There are many benefits to this:
59
60* less to read or to scroll past
61* easier to understand what exactly is tested
62* less stuff printed during printf-debugging
63* less time to run test
64
65It also makes sense to format snippets more compactly (for example, by placing enum definitions like `enum E { Foo, Bar }` on a single line),
66as long as they are still readable.
67
68## Order of Imports
69
70Separate import groups with blank lines.
71Use one `use` per crate.
72
73```rust
74mod x;
75mod y;
76
77// First std.
78use std::{ ... }
79
80// Second, external crates (both crates.io crates and other rust-analyzer crates).
81use crate_foo::{ ... }
82use crate_bar::{ ... }
83
84// Then current crate.
85use crate::{}
86
87// Finally, parent and child modules, but prefer `use crate::`.
88use super::{}
89```
90
91Module declarations come before the imports.
92Order them in "suggested reading order" for a person new to the code base.
93
94## Import Style
95
96Qualify items from `hir` and `ast`.
97
98```rust
99// Good
100use ra_syntax::ast;
101
102fn frobnicate(func: hir::Function, strukt: ast::StructDef) {}
103
104// Not as good
105use hir::Function;
106use ra_syntax::ast::StructDef;
107
108fn frobnicate(func: Function, strukt: StructDef) {}
109```
110
111Avoid local `use MyEnum::*` imports.
112
113Prefer `use crate::foo::bar` to `use super::bar`.
114
115## Order of Items
116
117Optimize for the reader who sees the file for the first time, and wants to get a general idea about what's going on.
118People read things from top to bottom, so place most important things first.
119
120Specifically, if all items except one are private, always put the non-private item on top.
121
122Put `struct`s and `enum`s first, functions and impls last.
123
124Do
125
126```rust
127// Good
128struct Foo {
129 bars: Vec<Bar>
130}
131
132struct Bar;
133```
134
135rather than
136
137```rust
138// Not as good
139struct Bar;
140
141struct Foo {
142 bars: Vec<Bar>
143}
144```
145
146## Variable Naming
147
148Use boring and long names for local variables ([yay code completion](https://github.com/rust-analyzer/rust-analyzer/pull/4162#discussion_r417130973)).
149The default name is a lowercased name of the type: `global_state: GlobalState`.
150Avoid ad-hoc acronyms and contractions, but use the ones that exist consistently (`db`, `ctx`, `acc`).
151The default name for "result of the function" local variable is `res`.
152The default name for "I don't really care about the name" variable is `it`.
153
154## Collection types
155
156Prefer `rustc_hash::FxHashMap` and `rustc_hash::FxHashSet` instead of the ones in `std::collections`.
157They use a hasher that's slightly faster and using them consistently will reduce code size by some small amount.
158
159## Preconditions
160
161Express function preconditions in types and force the caller to provide them (rather than checking in callee):
162
163```rust
164// Good
165fn frbonicate(walrus: Walrus) {
166 ...
167}
168
169// Not as good
170fn frobnicate(walrus: Option<Walrus>) {
171 let walrus = match walrus {
172 Some(it) => it,
173 None => return,
174 };
175 ...
176}
177```
178
179## Premature Pessimization
180
181Avoid writing code which is slower than it needs to be.
182Don't allocate a `Vec` where an iterator would do, don't allocate strings needlessly.
183
184```rust
185// Good
186use itertools::Itertools;
187
188let (first_word, second_word) = match text.split_ascii_whitespace().collect_tuple() {
189 Some(it) => it,
190 None => return,
191}
192
193// Not as good
194let words = text.split_ascii_whitespace().collect::<Vec<_>>();
195if words.len() != 2 {
196 return
197}
198```
199
200## Documentation
201
202For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines.
203If the line is too long, you want to split the sentence in two :-)
204
205## Commit Style
206
207We don't have specific rules around git history hygiene.
208Maintaining clean git history is encouraged, but not enforced.
209Use rebase workflow, it's OK to rewrite history during PR review process.
210
211Avoid @mentioning people in commit messages and pull request descriptions(they are added to commit message by bors).
212Such messages create a lot of duplicate notification traffic during rebases.