diff options
author | Dmitry <[email protected]> | 2020-08-09 14:35:51 +0100 |
---|---|---|
committer | Dmitry <[email protected]> | 2020-08-09 14:39:32 +0100 |
commit | 8068302fefc75440b823f4bf1731a5f347d7c767 (patch) | |
tree | 251b967182e79bc82a58c2fb208c688f6152df1f /docs/dev/style.md | |
parent | 1a43a0f63e0008787225abb6fb2baef97b6a39e0 (diff) | |
parent | 8a57afe5a4bfab40072a83f7dc4ca560bf860919 (diff) |
Merge remote-tracking branch 'origin/master'
Diffstat (limited to 'docs/dev/style.md')
-rw-r--r-- | docs/dev/style.md | 212 |
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 @@ | |||
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 | Separate import groups with blank lines. | ||
71 | Use one `use` per crate. | ||
72 | |||
73 | ```rust | ||
74 | mod x; | ||
75 | mod y; | ||
76 | |||
77 | // First std. | ||
78 | use std::{ ... } | ||
79 | |||
80 | // Second, external crates (both crates.io crates and other rust-analyzer crates). | ||
81 | use crate_foo::{ ... } | ||
82 | use crate_bar::{ ... } | ||
83 | |||
84 | // Then current crate. | ||
85 | use crate::{} | ||
86 | |||
87 | // Finally, parent and child modules, but prefer `use crate::`. | ||
88 | use super::{} | ||
89 | ``` | ||
90 | |||
91 | Module declarations come before the imports. | ||
92 | Order them in "suggested reading order" for a person new to the code base. | ||
93 | |||
94 | ## Import Style | ||
95 | |||
96 | Qualify items from `hir` and `ast`. | ||
97 | |||
98 | ```rust | ||
99 | // Good | ||
100 | use ra_syntax::ast; | ||
101 | |||
102 | fn frobnicate(func: hir::Function, strukt: ast::StructDef) {} | ||
103 | |||
104 | // Not as good | ||
105 | use hir::Function; | ||
106 | use ra_syntax::ast::StructDef; | ||
107 | |||
108 | fn frobnicate(func: Function, strukt: StructDef) {} | ||
109 | ``` | ||
110 | |||
111 | Avoid local `use MyEnum::*` imports. | ||
112 | |||
113 | Prefer `use crate::foo::bar` to `use super::bar`. | ||
114 | |||
115 | ## Order of Items | ||
116 | |||
117 | Optimize for the reader who sees the file for the first time, and wants to get a general idea about what's going on. | ||
118 | People read things from top to bottom, so place most important things first. | ||
119 | |||
120 | Specifically, if all items except one are private, always put the non-private item on top. | ||
121 | |||
122 | Put `struct`s and `enum`s first, functions and impls last. | ||
123 | |||
124 | Do | ||
125 | |||
126 | ```rust | ||
127 | // Good | ||
128 | struct Foo { | ||
129 | bars: Vec<Bar> | ||
130 | } | ||
131 | |||
132 | struct Bar; | ||
133 | ``` | ||
134 | |||
135 | rather than | ||
136 | |||
137 | ```rust | ||
138 | // Not as good | ||
139 | struct Bar; | ||
140 | |||
141 | struct Foo { | ||
142 | bars: Vec<Bar> | ||
143 | } | ||
144 | ``` | ||
145 | |||
146 | ## Variable Naming | ||
147 | |||
148 | Use boring and long names for local variables ([yay code completion](https://github.com/rust-analyzer/rust-analyzer/pull/4162#discussion_r417130973)). | ||
149 | The default name is a lowercased name of the type: `global_state: GlobalState`. | ||
150 | Avoid ad-hoc acronyms and contractions, but use the ones that exist consistently (`db`, `ctx`, `acc`). | ||
151 | The default name for "result of the function" local variable is `res`. | ||
152 | The default name for "I don't really care about the name" variable is `it`. | ||
153 | |||
154 | ## Collection types | ||
155 | |||
156 | Prefer `rustc_hash::FxHashMap` and `rustc_hash::FxHashSet` instead of the ones in `std::collections`. | ||
157 | They use a hasher that's slightly faster and using them consistently will reduce code size by some small amount. | ||
158 | |||
159 | ## Preconditions | ||
160 | |||
161 | Express function preconditions in types and force the caller to provide them (rather than checking in callee): | ||
162 | |||
163 | ```rust | ||
164 | // Good | ||
165 | fn frbonicate(walrus: Walrus) { | ||
166 | ... | ||
167 | } | ||
168 | |||
169 | // Not as good | ||
170 | fn 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 | |||
181 | Avoid writing code which is slower than it needs to be. | ||
182 | Don't allocate a `Vec` where an iterator would do, don't allocate strings needlessly. | ||
183 | |||
184 | ```rust | ||
185 | // Good | ||
186 | use itertools::Itertools; | ||
187 | |||
188 | let (first_word, second_word) = match text.split_ascii_whitespace().collect_tuple() { | ||
189 | Some(it) => it, | ||
190 | None => return, | ||
191 | } | ||
192 | |||
193 | // Not as good | ||
194 | let words = text.split_ascii_whitespace().collect::<Vec<_>>(); | ||
195 | if words.len() != 2 { | ||
196 | return | ||
197 | } | ||
198 | ``` | ||
199 | |||
200 | ## Documentation | ||
201 | |||
202 | For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. | ||
203 | If the line is too long, you want to split the sentence in two :-) | ||
204 | |||
205 | ## Commit Style | ||
206 | |||
207 | We don't have specific rules around git history hygiene. | ||
208 | Maintaining clean git history is encouraged, but not enforced. | ||
209 | Use rebase workflow, it's OK to rewrite history during PR review process. | ||
210 | |||
211 | Avoid @mentioning people in commit messages and pull request descriptions(they are added to commit message by bors). | ||
212 | Such messages create a lot of duplicate notification traffic during rebases. | ||