diff options
author | Aleksey Kladov <[email protected]> | 2020-10-07 11:50:46 +0100 |
---|---|---|
committer | Aleksey Kladov <[email protected]> | 2020-10-07 11:50:46 +0100 |
commit | fdf2f6226b4ce58a2407d7d3fa3b700f6e76e60a (patch) | |
tree | 1478cad9075a17a9d2b25f2823062e7de4647443 | |
parent | 2aa46034c2eeb3d994b2760878ac1969487542e3 (diff) |
Reorg style
-rw-r--r-- | docs/dev/style.md | 357 |
1 files changed, 181 insertions, 176 deletions
diff --git a/docs/dev/style.md b/docs/dev/style.md index fb407afcd..17626f3fd 100644 --- a/docs/dev/style.md +++ b/docs/dev/style.md | |||
@@ -6,7 +6,9 @@ Our approach to "clean code" is two-fold: | |||
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. | 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. | 7 | Sending small cleanup PRs (like renaming a single local variable) is encouraged. |
8 | 8 | ||
9 | # Scale of Changes | 9 | # General |
10 | |||
11 | ## Scale of Changes | ||
10 | 12 | ||
11 | Everyone knows that it's better to send small & focused pull requests. | 13 | 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. | 14 | The problem is, sometimes you *have* to, eg, rewrite the whole compiler, and that just doesn't fit into a set of isolated PRs. |
@@ -45,13 +47,35 @@ That said, adding an innocent-looking `pub use` is a very simple way to break en | |||
45 | Note: if you enjoyed this abstract hand-waving about boundaries, you might appreciate | 47 | Note: if you enjoyed this abstract hand-waving about boundaries, you might appreciate |
46 | https://www.tedinski.com/2018/02/06/system-boundaries.html | 48 | https://www.tedinski.com/2018/02/06/system-boundaries.html |
47 | 49 | ||
48 | # Crates.io Dependencies | 50 | ## Crates.io Dependencies |
49 | 51 | ||
50 | We try to be very conservative with usage of crates.io dependencies. | 52 | We try to be very conservative with usage of crates.io dependencies. |
51 | Don't use small "helper" crates (exception: `itertools` is allowed). | 53 | 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. | 54 | If there's some general reusable bit of code you need, consider adding it to the `stdx` crate. |
53 | 55 | ||
54 | # Minimal Tests | 56 | ## Commit Style |
57 | |||
58 | We don't have specific rules around git history hygiene. | ||
59 | Maintaining clean git history is strongly encouraged, but not enforced. | ||
60 | Use rebase workflow, it's OK to rewrite history during PR review process. | ||
61 | 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. | ||
62 | |||
63 | Avoid @mentioning people in commit messages and pull request descriptions(they are added to commit message by bors). | ||
64 | Such messages create a lot of duplicate notification traffic during rebases. | ||
65 | |||
66 | ## Clippy | ||
67 | |||
68 | We don't enforce Clippy. | ||
69 | A number of default lints have high false positive rate. | ||
70 | Selectively patching false-positives with `allow(clippy)` is considered worse than not using Clippy at all. | ||
71 | There's `cargo xtask lint` command which runs a subset of low-FPR lints. | ||
72 | Careful tweaking of `xtask lint` is welcome. | ||
73 | See also [rust-lang/clippy#5537](https://github.com/rust-lang/rust-clippy/issues/5537). | ||
74 | Of course, applying Clippy suggestions is welcome as long as they indeed improve the code. | ||
75 | |||
76 | # Code | ||
77 | |||
78 | ## Minimal Tests | ||
55 | 79 | ||
56 | Most tests in rust-analyzer start with a snippet of Rust code. | 80 | 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. | 81 | 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. |
@@ -65,119 +89,7 @@ There are many benefits to this: | |||
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), | 89 | 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. | 90 | as long as they are still readable. |
67 | 91 | ||
68 | # Order of Imports | 92 | ## Preconditions |
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 syntax::ast; | ||
101 | |||
102 | fn frobnicate(func: hir::Function, strukt: ast::StructDef) {} | ||
103 | |||
104 | // Not as good | ||
105 | use hir::Function; | ||
106 | use 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 | When implementing `Debug` or `Display`, import `std::fmt`: | ||
116 | |||
117 | ```rust | ||
118 | // Good | ||
119 | use std::fmt; | ||
120 | |||
121 | impl fmt::Display for RenameError { | ||
122 | fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { .. } | ||
123 | } | ||
124 | |||
125 | // Not as good | ||
126 | impl std::fmt::Display for RenameError { | ||
127 | fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { .. } | ||
128 | } | ||
129 | ``` | ||
130 | |||
131 | # Order of Items | ||
132 | |||
133 | Optimize for the reader who sees the file for the first time, and wants to get a general idea about what's going on. | ||
134 | People read things from top to bottom, so place most important things first. | ||
135 | |||
136 | Specifically, if all items except one are private, always put the non-private item on top. | ||
137 | |||
138 | Put `struct`s and `enum`s first, functions and impls last. | ||
139 | |||
140 | Do | ||
141 | |||
142 | ```rust | ||
143 | // Good | ||
144 | struct Foo { | ||
145 | bars: Vec<Bar> | ||
146 | } | ||
147 | |||
148 | struct Bar; | ||
149 | ``` | ||
150 | |||
151 | rather than | ||
152 | |||
153 | ```rust | ||
154 | // Not as good | ||
155 | struct Bar; | ||
156 | |||
157 | struct Foo { | ||
158 | bars: Vec<Bar> | ||
159 | } | ||
160 | ``` | ||
161 | |||
162 | # Variable Naming | ||
163 | |||
164 | Use boring and long names for local variables ([yay code completion](https://github.com/rust-analyzer/rust-analyzer/pull/4162#discussion_r417130973)). | ||
165 | The default name is a lowercased name of the type: `global_state: GlobalState`. | ||
166 | Avoid ad-hoc acronyms and contractions, but use the ones that exist consistently (`db`, `ctx`, `acc`). | ||
167 | |||
168 | Default names: | ||
169 | |||
170 | * `res` -- "result of the function" local variable | ||
171 | * `it` -- I don't really care about the name | ||
172 | * `n_foo` -- number of foos | ||
173 | * `foo_idx` -- index of `foo` | ||
174 | |||
175 | # Collection types | ||
176 | |||
177 | Prefer `rustc_hash::FxHashMap` and `rustc_hash::FxHashSet` instead of the ones in `std::collections`. | ||
178 | They use a hasher that's slightly faster and using them consistently will reduce code size by some small amount. | ||
179 | |||
180 | # Preconditions | ||
181 | 93 | ||
182 | Express function preconditions in types and force the caller to provide them (rather than checking in callee): | 94 | Express function preconditions in types and force the caller to provide them (rather than checking in callee): |
183 | 95 | ||
@@ -199,7 +111,6 @@ fn frobnicate(walrus: Option<Walrus>) { | |||
199 | 111 | ||
200 | Avoid preconditions that span across function boundaries: | 112 | Avoid preconditions that span across function boundaries: |
201 | 113 | ||
202 | |||
203 | ```rust | 114 | ```rust |
204 | // Good | 115 | // Good |
205 | fn string_literal_contents(s: &str) -> Option<&str> { | 116 | fn string_literal_contents(s: &str) -> Option<&str> { |
@@ -233,31 +144,7 @@ fn foo() { | |||
233 | 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`. | 144 | 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`. |
234 | In the "Good" version, the precondition check and usage are checked in the same block, and then encoded in the types. | 145 | In the "Good" version, the precondition check and usage are checked in the same block, and then encoded in the types. |
235 | 146 | ||
236 | # Early Returns | 147 | ## Getters & Setters |
237 | |||
238 | Do use early returns | ||
239 | |||
240 | ```rust | ||
241 | // Good | ||
242 | fn foo() -> Option<Bar> { | ||
243 | if !condition() { | ||
244 | return None; | ||
245 | } | ||
246 | |||
247 | Some(...) | ||
248 | } | ||
249 | |||
250 | // Not as good | ||
251 | fn foo() -> Option<Bar> { | ||
252 | if condition() { | ||
253 | Some(...) | ||
254 | } else { | ||
255 | None | ||
256 | } | ||
257 | } | ||
258 | ``` | ||
259 | |||
260 | # Getters & Setters | ||
261 | 148 | ||
262 | If a field can have any value without breaking invariants, make the field public. | 149 | If a field can have any value without breaking invariants, make the field public. |
263 | Conversely, if there is an invariant, document it, enforce it in the "constructor" function, make the field private, and provide a getter. | 150 | Conversely, if there is an invariant, document it, enforce it in the "constructor" function, make the field private, and provide a getter. |
@@ -285,6 +172,40 @@ impl Person { | |||
285 | } | 172 | } |
286 | ``` | 173 | ``` |
287 | 174 | ||
175 | ## Avoid Monomorphization | ||
176 | |||
177 | 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*. | ||
178 | This allows for exceptionally good performance, but leads to increased compile times. | ||
179 | Runtime performance obeys 80%/20% rule -- only a small fraction of code is hot. | ||
180 | Compile time **does not** obey this rule -- all code has to be compiled. | ||
181 | For this reason, avoid making a lot of code type parametric, *especially* on the boundaries between crates. | ||
182 | |||
183 | ```rust | ||
184 | // Good | ||
185 | fn frbonicate(f: impl FnMut()) { | ||
186 | frobnicate_impl(&mut f) | ||
187 | } | ||
188 | fn frobnicate_impl(f: &mut dyn FnMut()) { | ||
189 | // lots of code | ||
190 | } | ||
191 | |||
192 | // Not as good | ||
193 | fn frbonicate(f: impl FnMut()) { | ||
194 | // lots of code | ||
195 | } | ||
196 | ``` | ||
197 | |||
198 | Avoid `AsRef` polymorphism, it pays back only for widely used libraries: | ||
199 | |||
200 | ```rust | ||
201 | // Good | ||
202 | fn frbonicate(f: &Path) { | ||
203 | } | ||
204 | |||
205 | // Not as good | ||
206 | fn frbonicate(f: impl AsRef<Path>) { | ||
207 | } | ||
208 | ``` | ||
288 | 209 | ||
289 | # Premature Pessimization | 210 | # Premature Pessimization |
290 | 211 | ||
@@ -322,62 +243,146 @@ fn frobnicate(s: &str) { | |||
322 | } | 243 | } |
323 | ``` | 244 | ``` |
324 | 245 | ||
325 | # Avoid Monomorphization | 246 | ## Collection types |
326 | 247 | ||
327 | 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*. | 248 | Prefer `rustc_hash::FxHashMap` and `rustc_hash::FxHashSet` instead of the ones in `std::collections`. |
328 | This allows for exceptionally good performance, but leads to increased compile times. | 249 | They use a hasher that's slightly faster and using them consistently will reduce code size by some small amount. |
329 | Runtime performance obeys 80%/20% rule -- only a small fraction of code is hot. | 250 | |
330 | Compile time **does not** obey this rule -- all code has to be compiled. | 251 | # Style |
331 | For this reason, avoid making a lot of code type parametric, *especially* on the boundaries between crates. | 252 | |
253 | ## Order of Imports | ||
254 | |||
255 | Separate import groups with blank lines. | ||
256 | Use one `use` per crate. | ||
257 | |||
258 | ```rust | ||
259 | mod x; | ||
260 | mod y; | ||
261 | |||
262 | // First std. | ||
263 | use std::{ ... } | ||
264 | |||
265 | // Second, external crates (both crates.io crates and other rust-analyzer crates). | ||
266 | use crate_foo::{ ... } | ||
267 | use crate_bar::{ ... } | ||
268 | |||
269 | // Then current crate. | ||
270 | use crate::{} | ||
271 | |||
272 | // Finally, parent and child modules, but prefer `use crate::`. | ||
273 | use super::{} | ||
274 | ``` | ||
275 | |||
276 | Module declarations come before the imports. | ||
277 | Order them in "suggested reading order" for a person new to the code base. | ||
278 | |||
279 | ## Import Style | ||
280 | |||
281 | Qualify items from `hir` and `ast`. | ||
332 | 282 | ||
333 | ```rust | 283 | ```rust |
334 | // Good | 284 | // Good |
335 | fn frbonicate(f: impl FnMut()) { | 285 | use syntax::ast; |
336 | frobnicate_impl(&mut f) | 286 | |
337 | } | 287 | fn frobnicate(func: hir::Function, strukt: ast::StructDef) {} |
338 | fn frobnicate_impl(f: &mut dyn FnMut()) { | 288 | |
339 | // lots of code | 289 | // Not as good |
290 | use hir::Function; | ||
291 | use syntax::ast::StructDef; | ||
292 | |||
293 | fn frobnicate(func: Function, strukt: StructDef) {} | ||
294 | ``` | ||
295 | |||
296 | Avoid local `use MyEnum::*` imports. | ||
297 | |||
298 | Prefer `use crate::foo::bar` to `use super::bar`. | ||
299 | |||
300 | When implementing `Debug` or `Display`, import `std::fmt`: | ||
301 | |||
302 | ```rust | ||
303 | // Good | ||
304 | use std::fmt; | ||
305 | |||
306 | impl fmt::Display for RenameError { | ||
307 | fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { .. } | ||
340 | } | 308 | } |
341 | 309 | ||
342 | // Not as good | 310 | // Not as good |
343 | fn frbonicate(f: impl FnMut()) { | 311 | impl std::fmt::Display for RenameError { |
344 | // lots of code | 312 | fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { .. } |
345 | } | 313 | } |
346 | ``` | 314 | ``` |
347 | 315 | ||
348 | Avoid `AsRef` polymorphism, it pays back only for widely used libraries: | 316 | ## Order of Items |
317 | |||
318 | Optimize for the reader who sees the file for the first time, and wants to get a general idea about what's going on. | ||
319 | People read things from top to bottom, so place most important things first. | ||
320 | |||
321 | Specifically, if all items except one are private, always put the non-private item on top. | ||
322 | |||
323 | Put `struct`s and `enum`s first, functions and impls last. | ||
324 | |||
325 | Do | ||
349 | 326 | ||
350 | ```rust | 327 | ```rust |
351 | // Good | 328 | // Good |
352 | fn frbonicate(f: &Path) { | 329 | struct Foo { |
330 | bars: Vec<Bar> | ||
353 | } | 331 | } |
354 | 332 | ||
333 | struct Bar; | ||
334 | ``` | ||
335 | |||
336 | rather than | ||
337 | |||
338 | ```rust | ||
355 | // Not as good | 339 | // Not as good |
356 | fn frbonicate(f: impl AsRef<Path>) { | 340 | struct Bar; |
341 | |||
342 | struct Foo { | ||
343 | bars: Vec<Bar> | ||
357 | } | 344 | } |
358 | ``` | 345 | ``` |
359 | 346 | ||
360 | # Documentation | 347 | ## Variable Naming |
361 | 348 | ||
362 | For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. | 349 | Use boring and long names for local variables ([yay code completion](https://github.com/rust-analyzer/rust-analyzer/pull/4162#discussion_r417130973)). |
363 | If the line is too long, you want to split the sentence in two :-) | 350 | The default name is a lowercased name of the type: `global_state: GlobalState`. |
351 | Avoid ad-hoc acronyms and contractions, but use the ones that exist consistently (`db`, `ctx`, `acc`). | ||
364 | 352 | ||
365 | # Commit Style | 353 | Default names: |
366 | 354 | ||
367 | We don't have specific rules around git history hygiene. | 355 | * `res` -- "result of the function" local variable |
368 | Maintaining clean git history is strongly encouraged, but not enforced. | 356 | * `it` -- I don't really care about the name |
369 | Use rebase workflow, it's OK to rewrite history during PR review process. | 357 | * `n_foo` -- number of foos |
370 | 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. | 358 | * `foo_idx` -- index of `foo` |
371 | 359 | ||
372 | Avoid @mentioning people in commit messages and pull request descriptions(they are added to commit message by bors). | ||
373 | Such messages create a lot of duplicate notification traffic during rebases. | ||
374 | 360 | ||
375 | # Clippy | 361 | ## Early Returns |
376 | 362 | ||
377 | We don't enforce Clippy. | 363 | Do use early returns |
378 | A number of default lints have high false positive rate. | 364 | |
379 | Selectively patching false-positives with `allow(clippy)` is considered worse than not using Clippy at all. | 365 | ```rust |
380 | There's `cargo xtask lint` command which runs a subset of low-FPR lints. | 366 | // Good |
381 | Careful tweaking of `xtask lint` is welcome. | 367 | fn foo() -> Option<Bar> { |
382 | See also [rust-lang/clippy#5537](https://github.com/rust-lang/rust-clippy/issues/5537). | 368 | if !condition() { |
383 | Of course, applying Clippy suggestions is welcome as long as they indeed improve the code. | 369 | return None; |
370 | } | ||
371 | |||
372 | Some(...) | ||
373 | } | ||
374 | |||
375 | // Not as good | ||
376 | fn foo() -> Option<Bar> { | ||
377 | if condition() { | ||
378 | Some(...) | ||
379 | } else { | ||
380 | None | ||
381 | } | ||
382 | } | ||
383 | ``` | ||
384 | |||
385 | ## Documentation | ||
386 | |||
387 | For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. | ||
388 | If the line is too long, you want to split the sentence in two :-) | ||