aboutsummaryrefslogtreecommitdiff
path: root/docs/dev/style.md
diff options
context:
space:
mode:
authorAleksey Kladov <[email protected]>2020-10-07 11:50:46 +0100
committerAleksey Kladov <[email protected]>2020-10-07 11:50:46 +0100
commitfdf2f6226b4ce58a2407d7d3fa3b700f6e76e60a (patch)
tree1478cad9075a17a9d2b25f2823062e7de4647443 /docs/dev/style.md
parent2aa46034c2eeb3d994b2760878ac1969487542e3 (diff)
Reorg style
Diffstat (limited to 'docs/dev/style.md')
-rw-r--r--docs/dev/style.md357
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:
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,7 +111,6 @@ 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
205fn string_literal_contents(s: &str) -> Option<&str> { 116fn string_literal_contents(s: &str) -> Option<&str> {
@@ -233,31 +144,7 @@ fn foo() {
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 147## Getters & Setters
237
238Do use early returns
239
240```rust
241// Good
242fn foo() -> Option<Bar> {
243 if !condition() {
244 return None;
245 }
246
247 Some(...)
248}
249
250// Not as good
251fn foo() -> Option<Bar> {
252 if condition() {
253 Some(...)
254 } else {
255 None
256 }
257}
258```
259
260# Getters & Setters
261 148
262If a field can have any value without breaking invariants, make the field public. 149If 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. 150Conversely, 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
177Rust 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*.
178This allows for exceptionally good performance, but leads to increased compile times.
179Runtime performance obeys 80%/20% rule -- only a small fraction of code is hot.
180Compile time **does not** obey this rule -- all code has to be compiled.
181For this reason, avoid making a lot of code type parametric, *especially* on the boundaries between crates.
182
183```rust
184// Good
185fn frbonicate(f: impl FnMut()) {
186 frobnicate_impl(&mut f)
187}
188fn frobnicate_impl(f: &mut dyn FnMut()) {
189 // lots of code
190}
191
192// Not as good
193fn frbonicate(f: impl FnMut()) {
194 // lots of code
195}
196```
197
198Avoid `AsRef` polymorphism, it pays back only for widely used libraries:
199
200```rust
201// Good
202fn frbonicate(f: &Path) {
203}
204
205// Not as good
206fn 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
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*. 248Prefer `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. 249They 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. 250
330Compile time **does not** obey this rule -- all code has to be compiled. 251# Style
331For this reason, avoid making a lot of code type parametric, *especially* on the boundaries between crates. 252
253## Order of Imports
254
255Separate import groups with blank lines.
256Use one `use` per crate.
257
258```rust
259mod x;
260mod y;
261
262// First std.
263use std::{ ... }
264
265// Second, external crates (both crates.io crates and other rust-analyzer crates).
266use crate_foo::{ ... }
267use crate_bar::{ ... }
268
269// Then current crate.
270use crate::{}
271
272// Finally, parent and child modules, but prefer `use crate::`.
273use super::{}
274```
275
276Module declarations come before the imports.
277Order them in "suggested reading order" for a person new to the code base.
278
279## Import Style
280
281Qualify items from `hir` and `ast`.
332 282
333```rust 283```rust
334// Good 284// Good
335fn frbonicate(f: impl FnMut()) { 285use syntax::ast;
336 frobnicate_impl(&mut f) 286
337} 287fn frobnicate(func: hir::Function, strukt: ast::StructDef) {}
338fn frobnicate_impl(f: &mut dyn FnMut()) { 288
339 // lots of code 289// Not as good
290use hir::Function;
291use syntax::ast::StructDef;
292
293fn frobnicate(func: Function, strukt: StructDef) {}
294```
295
296Avoid local `use MyEnum::*` imports.
297
298Prefer `use crate::foo::bar` to `use super::bar`.
299
300When implementing `Debug` or `Display`, import `std::fmt`:
301
302```rust
303// Good
304use std::fmt;
305
306impl 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
343fn frbonicate(f: impl FnMut()) { 311impl 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
348Avoid `AsRef` polymorphism, it pays back only for widely used libraries: 316## Order of Items
317
318Optimize for the reader who sees the file for the first time, and wants to get a general idea about what's going on.
319People read things from top to bottom, so place most important things first.
320
321Specifically, if all items except one are private, always put the non-private item on top.
322
323Put `struct`s and `enum`s first, functions and impls last.
324
325Do
349 326
350```rust 327```rust
351// Good 328// Good
352fn frbonicate(f: &Path) { 329struct Foo {
330 bars: Vec<Bar>
353} 331}
354 332
333struct Bar;
334```
335
336rather than
337
338```rust
355// Not as good 339// Not as good
356fn frbonicate(f: impl AsRef<Path>) { 340struct Bar;
341
342struct Foo {
343 bars: Vec<Bar>
357} 344}
358``` 345```
359 346
360# Documentation 347## Variable Naming
361 348
362For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. 349Use 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 :-) 350The default name is a lowercased name of the type: `global_state: GlobalState`.
351Avoid ad-hoc acronyms and contractions, but use the ones that exist consistently (`db`, `ctx`, `acc`).
364 352
365# Commit Style 353Default names:
366 354
367We don't have specific rules around git history hygiene. 355* `res` -- "result of the function" local variable
368Maintaining clean git history is strongly encouraged, but not enforced. 356* `it` -- I don't really care about the name
369Use rebase workflow, it's OK to rewrite history during PR review process. 357* `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. 358* `foo_idx` -- index of `foo`
371 359
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 360
375# Clippy 361## Early Returns
376 362
377We don't enforce Clippy. 363Do use early returns
378A number of default lints have high false positive rate. 364
379Selectively patching false-positives with `allow(clippy)` is considered worse than not using Clippy at all. 365```rust
380There's `cargo xtask lint` command which runs a subset of low-FPR lints. 366// Good
381Careful tweaking of `xtask lint` is welcome. 367fn foo() -> Option<Bar> {
382See also [rust-lang/clippy#5537](https://github.com/rust-lang/rust-clippy/issues/5537). 368 if !condition() {
383Of 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
376fn foo() -> Option<Bar> {
377 if condition() {
378 Some(...)
379 } else {
380 None
381 }
382}
383```
384
385## Documentation
386
387For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines.
388If the line is too long, you want to split the sentence in two :-)