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