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