diff options
Diffstat (limited to 'docs/dev')
-rw-r--r-- | docs/dev/README.md | 4 | ||||
-rw-r--r-- | docs/dev/lsp-extensions.md | 13 | ||||
-rw-r--r-- | docs/dev/style.md | 211 |
3 files changed, 162 insertions, 66 deletions
diff --git a/docs/dev/README.md b/docs/dev/README.md index 4a2f9feb3..dd2bfc493 100644 --- a/docs/dev/README.md +++ b/docs/dev/README.md | |||
@@ -77,7 +77,7 @@ Notably, this uses the usual `rust-analyzer` binary from `PATH`. | |||
77 | For this, it is important to have the following in your `settings.json` file: | 77 | For this, it is important to have the following in your `settings.json` file: |
78 | ```json | 78 | ```json |
79 | { | 79 | { |
80 | "rust-analyzer.serverPath": "rust-analyzer" | 80 | "rust-analyzer.server.path": "rust-analyzer" |
81 | } | 81 | } |
82 | ``` | 82 | ``` |
83 | After I am done with the fix, I use `cargo xtask install --client` to try the new extension for real. | 83 | After I am done with the fix, I use `cargo xtask install --client` to try the new extension for real. |
@@ -227,6 +227,8 @@ There are also two VS Code commands which might be of interest: | |||
227 | 227 | ||
228 | * `Rust Analyzer: Syntax Tree` shows syntax tree of the current file/selection. | 228 | * `Rust Analyzer: Syntax Tree` shows syntax tree of the current file/selection. |
229 | 229 | ||
230 | * `Rust Analyzer: View Hir` shows the HIR expressions within the function containing the cursor. | ||
231 | |||
230 | You can hover over syntax nodes in the opened text file to see the appropriate | 232 | You can hover over syntax nodes in the opened text file to see the appropriate |
231 | rust code that it refers to and the rust editor will also highlight the proper | 233 | rust code that it refers to and the rust editor will also highlight the proper |
232 | text range. | 234 | text range. |
diff --git a/docs/dev/lsp-extensions.md b/docs/dev/lsp-extensions.md index 8c01db07c..78d86f060 100644 --- a/docs/dev/lsp-extensions.md +++ b/docs/dev/lsp-extensions.md | |||
@@ -1,5 +1,5 @@ | |||
1 | <!--- | 1 | <!--- |
2 | lsp_ext.rs hash: 203fdf79b21b5987 | 2 | lsp_ext.rs hash: 91f2c62457e0a20f |
3 | 3 | ||
4 | If you need to change the above hash to make the test pass, please check if you | 4 | If you need to change the above hash to make the test pass, please check if you |
5 | need to adjust this doc as well and ping this issue: | 5 | need to adjust this doc as well and ping this issue: |
@@ -449,6 +449,17 @@ interface SyntaxTeeParams { | |||
449 | Returns textual representation of a parse tree for the file/selected region. | 449 | Returns textual representation of a parse tree for the file/selected region. |
450 | Primarily for debugging, but very useful for all people working on rust-analyzer itself. | 450 | Primarily for debugging, but very useful for all people working on rust-analyzer itself. |
451 | 451 | ||
452 | ## View Hir | ||
453 | |||
454 | **Method:** `rust-analyzer/viewHir` | ||
455 | |||
456 | **Request:** `TextDocumentPositionParams` | ||
457 | |||
458 | **Response:** `string` | ||
459 | |||
460 | Returns a textual representation of the HIR of the function containing the cursor. | ||
461 | For debugging or when working on rust-analyzer itself. | ||
462 | |||
452 | ## Expand Macro | 463 | ## Expand Macro |
453 | 464 | ||
454 | **Method:** `rust-analyzer/expandMacro` | 465 | **Method:** `rust-analyzer/expandMacro` |
diff --git a/docs/dev/style.md b/docs/dev/style.md index 58b309379..9859f6148 100644 --- a/docs/dev/style.md +++ b/docs/dev/style.md | |||
@@ -53,6 +53,9 @@ We try to be very conservative with usage of crates.io dependencies. | |||
53 | Don't use small "helper" crates (exception: `itertools` is allowed). | 53 | Don't use small "helper" crates (exception: `itertools` is allowed). |
54 | 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. |
55 | 55 | ||
56 | **Rationale:** keep compile times low, create ecosystem pressure for faster | ||
57 | compiles, reduce the number of things which might break. | ||
58 | |||
56 | ## Commit Style | 59 | ## Commit Style |
57 | 60 | ||
58 | We don't have specific rules around git history hygiene. | 61 | We don't have specific rules around git history hygiene. |
@@ -66,15 +69,20 @@ Such messages create a lot of duplicate notification traffic during rebases. | |||
66 | If possible, write commit messages from user's perspective: | 69 | If possible, write commit messages from user's perspective: |
67 | 70 | ||
68 | ``` | 71 | ``` |
69 | # Good | 72 | # GOOD |
70 | Goto definition works inside macros | 73 | Goto definition works inside macros |
71 | 74 | ||
72 | # Not as good | 75 | # BAD |
73 | Use original span for FileId | 76 | Use original span for FileId |
74 | ``` | 77 | ``` |
75 | 78 | ||
76 | This makes it easier to prepare a changelog. | 79 | This makes it easier to prepare a changelog. |
77 | 80 | ||
81 | If the change adds a new user-visible functionality, consider recording a GIF with [peek](https://github.com/phw/peek) and pasting it into the PR description. | ||
82 | |||
83 | **Rationale:** clean history is potentially useful, but rarely used. | ||
84 | But many users read changelogs. | ||
85 | |||
78 | ## Clippy | 86 | ## Clippy |
79 | 87 | ||
80 | We don't enforce Clippy. | 88 | We don't enforce Clippy. |
@@ -82,21 +90,16 @@ A number of default lints have high false positive rate. | |||
82 | Selectively patching false-positives with `allow(clippy)` is considered worse than not using Clippy at all. | 90 | Selectively patching false-positives with `allow(clippy)` is considered worse than not using Clippy at all. |
83 | There's `cargo xtask lint` command which runs a subset of low-FPR lints. | 91 | There's `cargo xtask lint` command which runs a subset of low-FPR lints. |
84 | Careful tweaking of `xtask lint` is welcome. | 92 | Careful tweaking of `xtask lint` is welcome. |
85 | See also [rust-lang/clippy#5537](https://github.com/rust-lang/rust-clippy/issues/5537). | ||
86 | Of course, applying Clippy suggestions is welcome as long as they indeed improve the code. | 93 | Of course, applying Clippy suggestions is welcome as long as they indeed improve the code. |
87 | 94 | ||
95 | **Rationale:** see [rust-lang/clippy#5537](https://github.com/rust-lang/rust-clippy/issues/5537). | ||
96 | |||
88 | # Code | 97 | # Code |
89 | 98 | ||
90 | ## Minimal Tests | 99 | ## Minimal Tests |
91 | 100 | ||
92 | Most tests in rust-analyzer start with a snippet of Rust code. | 101 | Most tests in rust-analyzer start with a snippet of Rust code. |
93 | 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. | 102 | 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. |
94 | There are many benefits to this: | ||
95 | |||
96 | * less to read or to scroll past | ||
97 | * easier to understand what exactly is tested | ||
98 | * less stuff printed during printf-debugging | ||
99 | * less time to run test | ||
100 | 103 | ||
101 | It also makes sense to format snippets more compactly (for example, by placing enum definitions like `enum E { Foo, Bar }` on a single line), | 104 | It also makes sense to format snippets more compactly (for example, by placing enum definitions like `enum E { Foo, Bar }` on a single line), |
102 | as long as they are still readable. | 105 | as long as they are still readable. |
@@ -111,7 +114,7 @@ When using multiline fixtures, use unindented raw string literals: | |||
111 | r#" | 114 | r#" |
112 | struct S { foo: i32} | 115 | struct S { foo: i32} |
113 | fn main() { | 116 | fn main() { |
114 | let <|>foo = 92; | 117 | let $0foo = 92; |
115 | S { foo } | 118 | S { foo } |
116 | } | 119 | } |
117 | "#, | 120 | "#, |
@@ -125,19 +128,28 @@ fn main() { | |||
125 | } | 128 | } |
126 | ``` | 129 | ``` |
127 | 130 | ||
128 | That way, you can use your editor's "number of selected characters" feature to correlate offsets with test's source code. | 131 | **Rationale:** |
132 | |||
133 | There are many benefits to this: | ||
134 | |||
135 | * less to read or to scroll past | ||
136 | * easier to understand what exactly is tested | ||
137 | * less stuff printed during printf-debugging | ||
138 | * less time to run test | ||
129 | 139 | ||
130 | ## Preconditions | 140 | Formatting ensures that you can use your editor's "number of selected characters" feature to correlate offsets with test's source code. |
141 | |||
142 | ## Function Preconditions | ||
131 | 143 | ||
132 | Express function preconditions in types and force the caller to provide them (rather than checking in callee): | 144 | Express function preconditions in types and force the caller to provide them (rather than checking in callee): |
133 | 145 | ||
134 | ```rust | 146 | ```rust |
135 | // Good | 147 | // GOOD |
136 | fn frbonicate(walrus: Walrus) { | 148 | fn frbonicate(walrus: Walrus) { |
137 | ... | 149 | ... |
138 | } | 150 | } |
139 | 151 | ||
140 | // Not as good | 152 | // BAD |
141 | fn frobnicate(walrus: Option<Walrus>) { | 153 | fn frobnicate(walrus: Option<Walrus>) { |
142 | let walrus = match walrus { | 154 | let walrus = match walrus { |
143 | Some(it) => it, | 155 | Some(it) => it, |
@@ -147,10 +159,13 @@ fn frobnicate(walrus: Option<Walrus>) { | |||
147 | } | 159 | } |
148 | ``` | 160 | ``` |
149 | 161 | ||
150 | Avoid preconditions that span across function boundaries: | 162 | **Rationale:** this makes control flow explicit at the call site. |
163 | Call-site has more context, it often happens that the precondition falls out naturally or can be bubbled up higher in the stack. | ||
164 | |||
165 | Avoid splitting precondition check and precondition use across functions: | ||
151 | 166 | ||
152 | ```rust | 167 | ```rust |
153 | // Good | 168 | // GOOD |
154 | fn main() { | 169 | fn main() { |
155 | let s: &str = ...; | 170 | let s: &str = ...; |
156 | if let Some(contents) = string_literal_contents(s) { | 171 | if let Some(contents) = string_literal_contents(s) { |
@@ -166,7 +181,7 @@ fn string_literal_contents(s: &str) -> Option<&str> { | |||
166 | } | 181 | } |
167 | } | 182 | } |
168 | 183 | ||
169 | // Not as good | 184 | // BAD |
170 | fn main() { | 185 | fn main() { |
171 | let s: &str = ...; | 186 | let s: &str = ...; |
172 | if is_string_literal(s) { | 187 | if is_string_literal(s) { |
@@ -182,20 +197,24 @@ fn is_string_literal(s: &str) -> bool { | |||
182 | 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`. | 197 | 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`. |
183 | In the "Good" version, the precondition check and usage are checked in the same block, and then encoded in the types. | 198 | In the "Good" version, the precondition check and usage are checked in the same block, and then encoded in the types. |
184 | 199 | ||
200 | **Rationale:** non-local code properties degrade under change. | ||
201 | |||
185 | When checking a boolean precondition, prefer `if !invariant` to `if negated_invariant`: | 202 | When checking a boolean precondition, prefer `if !invariant` to `if negated_invariant`: |
186 | 203 | ||
187 | ```rust | 204 | ```rust |
188 | // Good | 205 | // GOOD |
189 | if !(idx < len) { | 206 | if !(idx < len) { |
190 | return None; | 207 | return None; |
191 | } | 208 | } |
192 | 209 | ||
193 | // Not as good | 210 | // BAD |
194 | if idx >= len { | 211 | if idx >= len { |
195 | return None; | 212 | return None; |
196 | } | 213 | } |
197 | ``` | 214 | ``` |
198 | 215 | ||
216 | **Rationale:** its useful to see the invariant relied upon by the rest of the function clearly spelled out. | ||
217 | |||
199 | ## Getters & Setters | 218 | ## Getters & Setters |
200 | 219 | ||
201 | If a field can have any value without breaking invariants, make the field public. | 220 | If a field can have any value without breaking invariants, make the field public. |
@@ -211,31 +230,36 @@ struct Person { | |||
211 | middle_name: Option<String> | 230 | middle_name: Option<String> |
212 | } | 231 | } |
213 | 232 | ||
214 | // Good | 233 | // GOOD |
215 | impl Person { | 234 | impl Person { |
216 | fn first_name(&self) -> &str { self.first_name.as_str() } | 235 | fn first_name(&self) -> &str { self.first_name.as_str() } |
217 | fn middle_name(&self) -> Option<&str> { self.middle_name.as_ref() } | 236 | fn middle_name(&self) -> Option<&str> { self.middle_name.as_ref() } |
218 | } | 237 | } |
219 | 238 | ||
220 | // Not as good | 239 | // BAD |
221 | impl Person { | 240 | impl Person { |
222 | fn first_name(&self) -> String { self.first_name.clone() } | 241 | fn first_name(&self) -> String { self.first_name.clone() } |
223 | fn middle_name(&self) -> &Option<String> { &self.middle_name } | 242 | fn middle_name(&self) -> &Option<String> { &self.middle_name } |
224 | } | 243 | } |
225 | ``` | 244 | ``` |
226 | 245 | ||
246 | **Rationale:** we don't provide public API, it's cheaper to refactor than to pay getters rent. | ||
247 | Non-local code properties degrade under change, privacy makes invariant local. | ||
248 | Borrowed own data discloses irrelevant details about origin of data. | ||
249 | Irrelevant (neither right nor wrong) things obscure correctness. | ||
250 | |||
227 | ## Constructors | 251 | ## Constructors |
228 | 252 | ||
229 | Prefer `Default` to zero-argument `new` function | 253 | Prefer `Default` to zero-argument `new` function |
230 | 254 | ||
231 | ```rust | 255 | ```rust |
232 | // Good | 256 | // GOOD |
233 | #[derive(Default)] | 257 | #[derive(Default)] |
234 | struct Foo { | 258 | struct Foo { |
235 | bar: Option<Bar> | 259 | bar: Option<Bar> |
236 | } | 260 | } |
237 | 261 | ||
238 | // Not as good | 262 | // BAD |
239 | struct Foo { | 263 | struct Foo { |
240 | bar: Option<Bar> | 264 | bar: Option<Bar> |
241 | } | 265 | } |
@@ -249,16 +273,18 @@ impl Foo { | |||
249 | 273 | ||
250 | Prefer `Default` even it has to be implemented manually. | 274 | Prefer `Default` even it has to be implemented manually. |
251 | 275 | ||
276 | **Rationale:** less typing in the common case, uniformity. | ||
277 | |||
252 | ## Functions Over Objects | 278 | ## Functions Over Objects |
253 | 279 | ||
254 | Avoid creating "doer" objects. | 280 | Avoid creating "doer" objects. |
255 | That is, objects which are created only to execute a single action. | 281 | That is, objects which are created only to execute a single action. |
256 | 282 | ||
257 | ```rust | 283 | ```rust |
258 | // Good | 284 | // GOOD |
259 | do_thing(arg1, arg2); | 285 | do_thing(arg1, arg2); |
260 | 286 | ||
261 | // Not as good | 287 | // BAD |
262 | ThingDoer::new(arg1, arg2).do(); | 288 | ThingDoer::new(arg1, arg2).do(); |
263 | ``` | 289 | ``` |
264 | 290 | ||
@@ -303,16 +329,14 @@ impl ThingDoer { | |||
303 | } | 329 | } |
304 | ``` | 330 | ``` |
305 | 331 | ||
332 | **Rationale:** not bothering the caller with irrelevant details, not mixing user API with implementor API. | ||
333 | |||
306 | ## Avoid Monomorphization | 334 | ## Avoid Monomorphization |
307 | 335 | ||
308 | 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*. | 336 | Avoid making a lot of code type parametric, *especially* on the boundaries between crates. |
309 | This allows for exceptionally good performance, but leads to increased compile times. | ||
310 | Runtime performance obeys 80%/20% rule -- only a small fraction of code is hot. | ||
311 | Compile time **does not** obey this rule -- all code has to be compiled. | ||
312 | For this reason, avoid making a lot of code type parametric, *especially* on the boundaries between crates. | ||
313 | 337 | ||
314 | ```rust | 338 | ```rust |
315 | // Good | 339 | // GOOD |
316 | fn frbonicate(f: impl FnMut()) { | 340 | fn frbonicate(f: impl FnMut()) { |
317 | frobnicate_impl(&mut f) | 341 | frobnicate_impl(&mut f) |
318 | } | 342 | } |
@@ -320,7 +344,7 @@ fn frobnicate_impl(f: &mut dyn FnMut()) { | |||
320 | // lots of code | 344 | // lots of code |
321 | } | 345 | } |
322 | 346 | ||
323 | // Not as good | 347 | // BAD |
324 | fn frbonicate(f: impl FnMut()) { | 348 | fn frbonicate(f: impl FnMut()) { |
325 | // lots of code | 349 | // lots of code |
326 | } | 350 | } |
@@ -329,15 +353,21 @@ fn frbonicate(f: impl FnMut()) { | |||
329 | Avoid `AsRef` polymorphism, it pays back only for widely used libraries: | 353 | Avoid `AsRef` polymorphism, it pays back only for widely used libraries: |
330 | 354 | ||
331 | ```rust | 355 | ```rust |
332 | // Good | 356 | // GOOD |
333 | fn frbonicate(f: &Path) { | 357 | fn frbonicate(f: &Path) { |
334 | } | 358 | } |
335 | 359 | ||
336 | // Not as good | 360 | // BAD |
337 | fn frbonicate(f: impl AsRef<Path>) { | 361 | fn frbonicate(f: impl AsRef<Path>) { |
338 | } | 362 | } |
339 | ``` | 363 | ``` |
340 | 364 | ||
365 | **Rationale:** 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*. | ||
366 | This allows for exceptionally good performance, but leads to increased compile times. | ||
367 | Runtime performance obeys 80%/20% rule -- only a small fraction of code is hot. | ||
368 | Compile time **does not** obey this rule -- all code has to be compiled. | ||
369 | |||
370 | |||
341 | # Premature Pessimization | 371 | # Premature Pessimization |
342 | 372 | ||
343 | ## Avoid Allocations | 373 | ## Avoid Allocations |
@@ -346,7 +376,7 @@ Avoid writing code which is slower than it needs to be. | |||
346 | Don't allocate a `Vec` where an iterator would do, don't allocate strings needlessly. | 376 | Don't allocate a `Vec` where an iterator would do, don't allocate strings needlessly. |
347 | 377 | ||
348 | ```rust | 378 | ```rust |
349 | // Good | 379 | // GOOD |
350 | use itertools::Itertools; | 380 | use itertools::Itertools; |
351 | 381 | ||
352 | let (first_word, second_word) = match text.split_ascii_whitespace().collect_tuple() { | 382 | let (first_word, second_word) = match text.split_ascii_whitespace().collect_tuple() { |
@@ -354,37 +384,40 @@ let (first_word, second_word) = match text.split_ascii_whitespace().collect_tupl | |||
354 | None => return, | 384 | None => return, |
355 | } | 385 | } |
356 | 386 | ||
357 | // Not as good | 387 | // BAD |
358 | let words = text.split_ascii_whitespace().collect::<Vec<_>>(); | 388 | let words = text.split_ascii_whitespace().collect::<Vec<_>>(); |
359 | if words.len() != 2 { | 389 | if words.len() != 2 { |
360 | return | 390 | return |
361 | } | 391 | } |
362 | ``` | 392 | ``` |
363 | 393 | ||
394 | **Rationale:** not allocating is almost often faster. | ||
395 | |||
364 | ## Push Allocations to the Call Site | 396 | ## Push Allocations to the Call Site |
365 | 397 | ||
366 | If allocation is inevitable, let the caller allocate the resource: | 398 | If allocation is inevitable, let the caller allocate the resource: |
367 | 399 | ||
368 | ```rust | 400 | ```rust |
369 | // Good | 401 | // GOOD |
370 | fn frobnicate(s: String) { | 402 | fn frobnicate(s: String) { |
371 | ... | 403 | ... |
372 | } | 404 | } |
373 | 405 | ||
374 | // Not as good | 406 | // BAD |
375 | fn frobnicate(s: &str) { | 407 | fn frobnicate(s: &str) { |
376 | let s = s.to_string(); | 408 | let s = s.to_string(); |
377 | ... | 409 | ... |
378 | } | 410 | } |
379 | ``` | 411 | ``` |
380 | 412 | ||
381 | This is better because it reveals the costs. | 413 | **Rationale:** reveals the costs. |
382 | It is also more efficient when the caller already owns the allocation. | 414 | It is also more efficient when the caller already owns the allocation. |
383 | 415 | ||
384 | ## Collection types | 416 | ## Collection types |
385 | 417 | ||
386 | Prefer `rustc_hash::FxHashMap` and `rustc_hash::FxHashSet` instead of the ones in `std::collections`. | 418 | Prefer `rustc_hash::FxHashMap` and `rustc_hash::FxHashSet` instead of the ones in `std::collections`. |
387 | They use a hasher that's slightly faster and using them consistently will reduce code size by some small amount. | 419 | |
420 | **Rationale:** they use a hasher that's significantly faster and using them consistently will reduce code size by some small amount. | ||
388 | 421 | ||
389 | # Style | 422 | # Style |
390 | 423 | ||
@@ -393,6 +426,9 @@ They use a hasher that's slightly faster and using them consistently will reduce | |||
393 | Separate import groups with blank lines. | 426 | Separate import groups with blank lines. |
394 | Use one `use` per crate. | 427 | Use one `use` per crate. |
395 | 428 | ||
429 | Module declarations come before the imports. | ||
430 | Order them in "suggested reading order" for a person new to the code base. | ||
431 | |||
396 | ```rust | 432 | ```rust |
397 | mod x; | 433 | mod x; |
398 | mod y; | 434 | mod y; |
@@ -411,46 +447,62 @@ use crate::{} | |||
411 | use super::{} | 447 | use super::{} |
412 | ``` | 448 | ``` |
413 | 449 | ||
414 | Module declarations come before the imports. | 450 | **Rationale:** consistency. |
415 | Order them in "suggested reading order" for a person new to the code base. | 451 | Reading order is important for new contributors. |
452 | Grouping by crate allows to spot unwanted dependencies easier. | ||
416 | 453 | ||
417 | ## Import Style | 454 | ## Import Style |
418 | 455 | ||
419 | Qualify items from `hir` and `ast`. | 456 | Qualify items from `hir` and `ast`. |
420 | 457 | ||
421 | ```rust | 458 | ```rust |
422 | // Good | 459 | // GOOD |
423 | use syntax::ast; | 460 | use syntax::ast; |
424 | 461 | ||
425 | fn frobnicate(func: hir::Function, strukt: ast::StructDef) {} | 462 | fn frobnicate(func: hir::Function, strukt: ast::Struct) {} |
426 | 463 | ||
427 | // Not as good | 464 | // BAD |
428 | use hir::Function; | 465 | use hir::Function; |
429 | use syntax::ast::StructDef; | 466 | use syntax::ast::Struct; |
430 | 467 | ||
431 | fn frobnicate(func: Function, strukt: StructDef) {} | 468 | fn frobnicate(func: Function, strukt: Struct) {} |
432 | ``` | 469 | ``` |
433 | 470 | ||
434 | Avoid local `use MyEnum::*` imports. | 471 | **Rationale:** avoids name clashes, makes the layer clear at a glance. |
435 | |||
436 | Prefer `use crate::foo::bar` to `use super::bar`. | ||
437 | 472 | ||
438 | When implementing `Debug` or `Display`, import `std::fmt`: | 473 | When implementing traits from `std::fmt` or `std::ops`, import the module: |
439 | 474 | ||
440 | ```rust | 475 | ```rust |
441 | // Good | 476 | // GOOD |
442 | use std::fmt; | 477 | use std::fmt; |
443 | 478 | ||
444 | impl fmt::Display for RenameError { | 479 | impl fmt::Display for RenameError { |
445 | fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { .. } | 480 | fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { .. } |
446 | } | 481 | } |
447 | 482 | ||
448 | // Not as good | 483 | // BAD |
449 | impl std::fmt::Display for RenameError { | 484 | impl std::fmt::Display for RenameError { |
450 | fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { .. } | 485 | fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { .. } |
451 | } | 486 | } |
487 | |||
488 | // BAD | ||
489 | use std::ops::Deref; | ||
490 | |||
491 | impl Deref for Widget { | ||
492 | type Target = str; | ||
493 | fn deref(&self) -> &str { .. } | ||
494 | } | ||
452 | ``` | 495 | ``` |
453 | 496 | ||
497 | **Rationale:** overall, less typing. | ||
498 | Makes it clear that a trait is implemented, rather than used. | ||
499 | |||
500 | Avoid local `use MyEnum::*` imports. | ||
501 | **Rationale:** consistency. | ||
502 | |||
503 | Prefer `use crate::foo::bar` to `use super::bar` or `use self::bar::baz`. | ||
504 | **Rationale:** consistency, this is the style which works in all cases. | ||
505 | |||
454 | ## Order of Items | 506 | ## Order of Items |
455 | 507 | ||
456 | Optimize for the reader who sees the file for the first time, and wants to get a general idea about what's going on. | 508 | Optimize for the reader who sees the file for the first time, and wants to get a general idea about what's going on. |
@@ -459,7 +511,7 @@ People read things from top to bottom, so place most important things first. | |||
459 | Specifically, if all items except one are private, always put the non-private item on top. | 511 | Specifically, if all items except one are private, always put the non-private item on top. |
460 | 512 | ||
461 | ```rust | 513 | ```rust |
462 | // Good | 514 | // GOOD |
463 | pub(crate) fn frobnicate() { | 515 | pub(crate) fn frobnicate() { |
464 | Helper::act() | 516 | Helper::act() |
465 | } | 517 | } |
@@ -473,7 +525,7 @@ impl Helper { | |||
473 | } | 525 | } |
474 | } | 526 | } |
475 | 527 | ||
476 | // Not as good | 528 | // BAD |
477 | #[derive(Default)] | 529 | #[derive(Default)] |
478 | struct Helper { stuff: i32 } | 530 | struct Helper { stuff: i32 } |
479 | 531 | ||
@@ -489,12 +541,11 @@ impl Helper { | |||
489 | ``` | 541 | ``` |
490 | 542 | ||
491 | If there's a mixture of private and public items, put public items first. | 543 | If there's a mixture of private and public items, put public items first. |
492 | If function bodies are folded in the editor, the source code should read as documentation for the public API. | ||
493 | 544 | ||
494 | Put `struct`s and `enum`s first, functions and impls last. Order types declarations in top-down manner. | 545 | Put `struct`s and `enum`s first, functions and impls last. Order type declarations in top-down manner. |
495 | 546 | ||
496 | ```rust | 547 | ```rust |
497 | // Good | 548 | // GOOD |
498 | struct Parent { | 549 | struct Parent { |
499 | children: Vec<Child> | 550 | children: Vec<Child> |
500 | } | 551 | } |
@@ -507,7 +558,7 @@ impl Parent { | |||
507 | impl Child { | 558 | impl Child { |
508 | } | 559 | } |
509 | 560 | ||
510 | // Not as good | 561 | // BAD |
511 | struct Child; | 562 | struct Child; |
512 | 563 | ||
513 | impl Child { | 564 | impl Child { |
@@ -521,6 +572,9 @@ impl Parent { | |||
521 | } | 572 | } |
522 | ``` | 573 | ``` |
523 | 574 | ||
575 | **Rationale:** easier to get the sense of the API by visually scanning the file. | ||
576 | If function bodies are folded in the editor, the source code should read as documentation for the public API. | ||
577 | |||
524 | ## Variable Naming | 578 | ## Variable Naming |
525 | 579 | ||
526 | Use boring and long names for local variables ([yay code completion](https://github.com/rust-analyzer/rust-analyzer/pull/4162#discussion_r417130973)). | 580 | Use boring and long names for local variables ([yay code completion](https://github.com/rust-analyzer/rust-analyzer/pull/4162#discussion_r417130973)). |
@@ -548,12 +602,14 @@ enum -> enum_ | |||
548 | mod -> module | 602 | mod -> module |
549 | ``` | 603 | ``` |
550 | 604 | ||
605 | **Rationale:** consistency. | ||
606 | |||
551 | ## Early Returns | 607 | ## Early Returns |
552 | 608 | ||
553 | Do use early returns | 609 | Do use early returns |
554 | 610 | ||
555 | ```rust | 611 | ```rust |
556 | // Good | 612 | // GOOD |
557 | fn foo() -> Option<Bar> { | 613 | fn foo() -> Option<Bar> { |
558 | if !condition() { | 614 | if !condition() { |
559 | return None; | 615 | return None; |
@@ -562,7 +618,7 @@ fn foo() -> Option<Bar> { | |||
562 | Some(...) | 618 | Some(...) |
563 | } | 619 | } |
564 | 620 | ||
565 | // Not as good | 621 | // BAD |
566 | fn foo() -> Option<Bar> { | 622 | fn foo() -> Option<Bar> { |
567 | if condition() { | 623 | if condition() { |
568 | Some(...) | 624 | Some(...) |
@@ -572,20 +628,47 @@ fn foo() -> Option<Bar> { | |||
572 | } | 628 | } |
573 | ``` | 629 | ``` |
574 | 630 | ||
631 | **Rationale:** reduce congnitive stack usage. | ||
632 | |||
575 | ## Comparisons | 633 | ## Comparisons |
576 | 634 | ||
577 | Use `<`/`<=`, avoid `>`/`>=`. | 635 | Use `<`/`<=`, avoid `>`/`>=`. |
578 | Less-then comparisons are more intuitive, they correspond spatially to [real line](https://en.wikipedia.org/wiki/Real_line) | ||
579 | 636 | ||
580 | ```rust | 637 | ```rust |
581 | // Good | 638 | // GOOD |
582 | assert!(lo <= x && x <= hi); | 639 | assert!(lo <= x && x <= hi); |
583 | 640 | ||
584 | // Not as good | 641 | // BAD |
585 | assert!(x >= lo && x <= hi>); | 642 | assert!(x >= lo && x <= hi>); |
586 | ``` | 643 | ``` |
587 | 644 | ||
645 | **Rationale:** Less-then comparisons are more intuitive, they correspond spatially to [real line](https://en.wikipedia.org/wiki/Real_line). | ||
646 | |||
647 | |||
648 | ## Token names | ||
649 | |||
650 | Use `T![foo]` instead of `SyntaxKind::FOO_KW`. | ||
651 | |||
652 | ```rust | ||
653 | // GOOD | ||
654 | match p.current() { | ||
655 | T![true] | T![false] => true, | ||
656 | _ => false, | ||
657 | } | ||
658 | |||
659 | // BAD | ||
660 | |||
661 | match p.current() { | ||
662 | SyntaxKind::TRUE_KW | SyntaxKind::FALSE_KW => true, | ||
663 | _ => false, | ||
664 | } | ||
665 | ``` | ||
666 | |||
667 | **Rationale:** The macro uses the familiar Rust syntax, avoiding ambiguities like "is this a brace or bracket?". | ||
668 | |||
588 | ## Documentation | 669 | ## Documentation |
589 | 670 | ||
590 | For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. | 671 | For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. |
591 | If the line is too long, you want to split the sentence in two :-) | 672 | If the line is too long, you want to split the sentence in two :-) |
673 | |||
674 | **Rationale:** much easier to edit the text and read the diff. | ||