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