summaryrefslogtreecommitdiffstats
path: root/src/tools/rust-analyzer/docs/dev/style.md
blob: a80eebd632961a6ef468658dce53f755fce95c07 (plain)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
467
468
469
470
471
472
473
474
475
476
477
478
479
480
481
482
483
484
485
486
487
488
489
490
491
492
493
494
495
496
497
498
499
500
501
502
503
504
505
506
507
508
509
510
511
512
513
514
515
516
517
518
519
520
521
522
523
524
525
526
527
528
529
530
531
532
533
534
535
536
537
538
539
540
541
542
543
544
545
546
547
548
549
550
551
552
553
554
555
556
557
558
559
560
561
562
563
564
565
566
567
568
569
570
571
572
573
574
575
576
577
578
579
580
581
582
583
584
585
586
587
588
589
590
591
592
593
594
595
596
597
598
599
600
601
602
603
604
605
606
607
608
609
610
611
612
613
614
615
616
617
618
619
620
621
622
623
624
625
626
627
628
629
630
631
632
633
634
635
636
637
638
639
640
641
642
643
644
645
646
647
648
649
650
651
652
653
654
655
656
657
658
659
660
661
662
663
664
665
666
667
668
669
670
671
672
673
674
675
676
677
678
679
680
681
682
683
684
685
686
687
688
689
690
691
692
693
694
695
696
697
698
699
700
701
702
703
704
705
706
707
708
709
710
711
712
713
714
715
716
717
718
719
720
721
722
723
724
725
726
727
728
729
730
731
732
733
734
735
736
737
738
739
740
741
742
743
744
745
746
747
748
749
750
751
752
753
754
755
756
757
758
759
760
761
762
763
764
765
766
767
768
769
770
771
772
773
774
775
776
777
778
779
780
781
782
783
784
785
786
787
788
789
790
791
792
793
794
795
796
797
798
799
800
801
802
803
804
805
806
807
808
809
810
811
812
813
814
815
816
817
818
819
820
821
822
823
824
825
826
827
828
829
830
831
832
833
834
835
836
837
838
839
840
841
842
843
844
845
846
847
848
849
850
851
852
853
854
855
856
857
858
859
860
861
862
863
864
865
866
867
868
869
870
871
872
873
874
875
876
877
878
879
880
881
882
883
884
885
886
887
888
889
890
891
892
893
894
895
896
897
898
899
900
901
902
903
904
905
906
907
908
909
910
911
912
913
914
915
916
917
918
919
920
921
922
923
924
925
926
927
928
929
930
931
932
933
934
935
936
937
938
939
940
941
942
943
944
945
946
947
948
949
950
951
952
953
954
955
956
957
958
959
960
961
962
963
964
965
966
967
968
969
970
971
972
973
974
975
976
977
978
979
980
981
982
983
984
985
986
987
988
989
990
991
992
993
994
995
996
997
998
999
1000
1001
1002
1003
1004
1005
1006
1007
1008
1009
1010
1011
1012
1013
1014
1015
1016
1017
1018
1019
1020
1021
1022
1023
1024
1025
1026
1027
1028
1029
1030
1031
1032
1033
1034
1035
1036
1037
1038
1039
1040
1041
1042
1043
1044
1045
1046
1047
1048
1049
1050
1051
1052
1053
1054
1055
1056
1057
1058
1059
1060
1061
1062
1063
1064
1065
1066
1067
1068
1069
1070
1071
1072
1073
1074
1075
1076
1077
1078
1079
1080
1081
1082
1083
1084
1085
1086
1087
1088
1089
1090
1091
1092
1093
1094
1095
1096
1097
1098
1099
1100
1101
1102
1103
1104
1105
1106
1107
1108
1109
1110
1111
1112
1113
1114
1115
1116
1117
1118
1119
1120
1121
1122
1123
1124
1125
1126
1127
1128
1129
1130
1131
1132
1133
1134
1135
1136
1137
1138
1139
1140
1141
1142
1143
1144
1145
1146
1147
1148
1149
1150
1151
1152
1153
1154
1155
1156
1157
1158
1159
1160
1161
1162
1163
1164
1165
1166
1167
1168
1169
1170
1171
1172
Our approach to "clean code" is two-fold:

* We generally don't block PRs on style changes.
* At the same time, all code in rust-analyzer is constantly refactored.

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.
Sending small cleanup PRs (like renaming a single local variable) is encouraged.

When reviewing pull requests prefer extending this document to leaving
non-reusable comments on the pull request itself.

# General

## Scale of Changes

Everyone knows that it's better to send small & focused pull requests.
The problem is, sometimes you *have* to, eg, rewrite the whole compiler, and that just doesn't fit into a set of isolated PRs.

The main things to keep an eye on are the boundaries between various components.
There are three kinds of changes:

1. Internals of a single component are changed.
   Specifically, you don't change any `pub` items.
   A good example here would be an addition of a new assist.

2. API of a component is expanded.
   Specifically, you add a new `pub` function which wasn't there before.
   A good example here would be expansion of assist API, for example, to implement lazy assists or assists groups.

3. A new dependency between components is introduced.
   Specifically, you add a `pub use` reexport from another crate or you add a new line to the `[dependencies]` section of `Cargo.toml`.
   A good example here would be adding reference search capability to the assists crates.

For the first group, the change is generally merged as long as:

* it works for the happy case,
* it has tests,
* it doesn't panic for the unhappy case.

For the second group, the change would be subjected to quite a bit of scrutiny and iteration.
The new API needs to be right (or at least easy to change later).
The actual implementation doesn't matter that much.
It's very important to minimize the amount of changed lines of code for changes of the second kind.
Often, you start doing a change of the first kind, only to realize that you need to elevate to a change of the second kind.
In this case, we'll probably ask you to split API changes into a separate PR.

Changes of the third group should be pretty rare, so we don't specify any specific process for them.
That said, adding an innocent-looking `pub use` is a very simple way to break encapsulation, keep an eye on it!

Note: if you enjoyed this abstract hand-waving about boundaries, you might appreciate
https://www.tedinski.com/2018/02/06/system-boundaries.html

## Crates.io Dependencies

We try to be very conservative with usage of crates.io dependencies.
Don't use small "helper" crates (exception: `itertools` and `either` are allowed).
If there's some general reusable bit of code you need, consider adding it to the `stdx` crate.
A useful exercise is to read Cargo.lock and see if some *transitive* dependencies do not make sense for rust-analyzer.

**Rationale:** keep compile times low, create ecosystem pressure for faster compiles, reduce the number of things which might break.

## Commit Style

We don't have specific rules around git history hygiene.
Maintaining clean git history is strongly encouraged, but not enforced.
Use rebase workflow, it's OK to rewrite history during PR review process.
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.

Avoid @mentioning people in commit messages and pull request descriptions(they are added to commit message by bors).
Such messages create a lot of duplicate notification traffic during rebases.

If possible, write Pull Request titles and descriptions from the user's perspective:

```
# GOOD
Make goto definition work inside macros

# BAD
Use original span for FileId
```

This makes it easier to prepare a changelog.

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.

To make writing the release notes easier, you can mark a pull request as a feature, fix, internal change, or minor.
Minor changes are excluded from the release notes, while the other types are distributed in their corresponding sections.
There are two ways to mark this:

* use a `feat: `, `feature: `, `fix: `, `internal: ` or `minor: ` prefix in the PR title
* write `changelog [feature|fix|internal|skip] [description]` in a comment or in the PR description; the description is optional, and will replace the title if included.

These comments don't have to be added by the PR author.
Editing a comment or the PR description or title is also fine, as long as it happens before the release.

**Rationale:** clean history is potentially useful, but rarely used.
But many users read changelogs.
Including a description and GIF suitable for the changelog means less work for the maintainers on the release day.

## Clippy

We don't enforce Clippy.
A number of default lints have high false positive rate.
Selectively patching false-positives with `allow(clippy)` is considered worse than not using Clippy at all.
There's a `cargo lint` command which runs a subset of low-FPR lints.
Careful tweaking of `lint` is welcome.
Of course, applying Clippy suggestions is welcome as long as they indeed improve the code.

**Rationale:** see [rust-lang/clippy#5537](https://github.com/rust-lang/rust-clippy/issues/5537).

# Code

## Minimal Tests

Most tests in rust-analyzer start with a snippet of Rust code.
These 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.

It also makes sense to format snippets more compactly (for example, by placing enum definitions like `enum E { Foo, Bar }` on a single line),
as long as they are still readable.

When using multiline fixtures, use unindented raw string literals:

```rust
    #[test]
    fn inline_field_shorthand() {
        check_assist(
            inline_local_variable,
            r#"
struct S { foo: i32}
fn main() {
    let $0foo = 92;
    S { foo }
}
"#,
            r#"
struct S { foo: i32}
fn main() {
    S { foo: 92 }
}
"#,
        );
    }
```

**Rationale:**

There are many benefits to this:

* less to read or to scroll past
* easier to understand what exactly is tested
* less stuff printed during printf-debugging
* less time to run test

Formatting ensures that you can use your editor's "number of selected characters" feature to correlate offsets with test's source code.

## Marked Tests

Use
[`cov_mark::hit! / cov_mark::check!`](https://github.com/matklad/cov-mark)
when testing specific conditions.
Do not place several marks into a single test or condition.
Do not reuse marks between several tests.

**Rationale:** marks provide an easy way to find the canonical test for each bit of code.
This makes it much easier to understand.
More than one mark per test / code branch doesn't add significantly to understanding.

## `#[should_panic]`

Do not use `#[should_panic]` tests.
Instead, explicitly check for `None`, `Err`, etc.

**Rationale:** `#[should_panic]` is a tool for library authors to make sure that the API does not fail silently when misused.
`rust-analyzer` is not a library, we don't need to test for API misuse, and we have to handle any user input without panics.
Panic messages in the logs from the `#[should_panic]` tests are confusing.

## `#[ignore]`

Do not `#[ignore]` tests.
If the test currently does not work, assert the wrong behavior and add a fixme explaining why it is wrong.

**Rationale:** noticing when the behavior is fixed, making sure that even the wrong behavior is acceptable (ie, not a panic).

## Function Preconditions

Express function preconditions in types and force the caller to provide them (rather than checking in callee):

```rust
// GOOD
fn frobnicate(walrus: Walrus) {
    ...
}

// BAD
fn frobnicate(walrus: Option<Walrus>) {
    let walrus = match walrus {
        Some(it) => it,
        None => return,
    };
    ...
}
```

**Rationale:** this makes control flow explicit at the call site.
Call-site has more context, it often happens that the precondition falls out naturally or can be bubbled up higher in the stack.

Avoid splitting precondition check and precondition use across functions:

```rust
// GOOD
fn main() {
    let s: &str = ...;
    if let Some(contents) = string_literal_contents(s) {

    }
}

fn string_literal_contents(s: &str) -> Option<&str> {
    if s.starts_with('"') && s.ends_with('"') {
        Some(&s[1..s.len() - 1])
    } else {
        None
    }
}

// BAD
fn main() {
    let s: &str = ...;
    if is_string_literal(s) {
        let contents = &s[1..s.len() - 1];
    }
}

fn is_string_literal(s: &str) -> bool {
    s.starts_with('"') && s.ends_with('"')
}
```

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`.
In the "Good" version, the precondition check and usage are checked in the same block, and then encoded in the types.

**Rationale:** non-local code properties degrade under change.

When checking a boolean precondition, prefer `if !invariant` to `if negated_invariant`:

```rust
// GOOD
if !(idx < len) {
    return None;
}

// BAD
if idx >= len {
    return None;
}
```

**Rationale:** it's useful to see the invariant relied upon by the rest of the function clearly spelled out.

## Control Flow

As a special case of the previous rule, do not hide control flow inside functions, push it to the caller:

```rust
// GOOD
if cond {
    f()
}

// BAD
fn f() {
    if !cond {
        return;
    }
    ...
}
```

## Assertions

Assert liberally.
Prefer [`stdx::never!`](https://docs.rs/always-assert/0.1.2/always_assert/macro.never.html) to standard `assert!`.

**Rationale:** See [cross cutting concern: error handling](https://github.com/rust-lang/rust-analyzer/blob/master/docs/dev/architecture.md#error-handling).

## Getters & Setters

If a field can have any value without breaking invariants, make the field public.
Conversely, if there is an invariant, document it, enforce it in the "constructor" function, make the field private, and provide a getter.
Never provide setters.

Getters should return borrowed data:

```rust
struct Person {
    // Invariant: never empty
    first_name: String,
    middle_name: Option<String>
}

// GOOD
impl Person {
    fn first_name(&self) -> &str { self.first_name.as_str() }
    fn middle_name(&self) -> Option<&str> { self.middle_name.as_ref() }
}

// BAD
impl Person {
    fn first_name(&self) -> String { self.first_name.clone() }
    fn middle_name(&self) -> &Option<String> { &self.middle_name }
}
```

**Rationale:** we don't provide public API, it's cheaper to refactor than to pay getters rent.
Non-local code properties degrade under change, privacy makes invariant local.
Borrowed owned types (`&String`) disclose irrelevant details about internal representation.
Irrelevant (neither right nor wrong) things obscure correctness.

## Useless Types

More generally, always prefer types on the left

```rust
// GOOD      BAD
&[T]         &Vec<T>
&str         &String
Option<&T>   &Option<T>
&Path        &PathBuf
```

**Rationale:** types on the left are strictly more general.
Even when generality is not required, consistency is important.

## Constructors

Prefer `Default` to zero-argument `new` function.

```rust
// GOOD
#[derive(Default)]
struct Foo {
    bar: Option<Bar>
}

// BAD
struct Foo {
    bar: Option<Bar>
}

impl Foo {
    fn new() -> Foo {
        Foo { bar: None }
    }
}
```

Prefer `Default` even if it has to be implemented manually.

**Rationale:** less typing in the common case, uniformity.

Use `Vec::new` rather than `vec![]`.

**Rationale:** uniformity, strength reduction.

Avoid using "dummy" states to implement a `Default`.
If a type doesn't have a sensible default, empty value, don't hide it.
Let the caller explicitly decide what the right initial state is.

## Functions Over Objects

Avoid creating "doer" objects.
That is, objects which are created only to execute a single action.

```rust
// GOOD
do_thing(arg1, arg2);

// BAD
ThingDoer::new(arg1, arg2).do();
```

Note that this concerns only outward API.
When implementing `do_thing`, it might be very useful to create a context object.

```rust
pub fn do_thing(arg1: Arg1, arg2: Arg2) -> Res {
    let mut ctx = Ctx { arg1, arg2 };
    ctx.run()
}

struct Ctx {
    arg1: Arg1, arg2: Arg2
}

impl Ctx {
    fn run(self) -> Res {
        ...
    }
}
```

The difference is that `Ctx` is an impl detail here.

Sometimes a middle ground is acceptable if this can save some busywork:

```rust
ThingDoer::do(arg1, arg2);

pub struct ThingDoer {
    arg1: Arg1, arg2: Arg2,
}

impl ThingDoer {
    pub fn do(arg1: Arg1, arg2: Arg2) -> Res {
        ThingDoer { arg1, arg2 }.run()
    }
    fn run(self) -> Res {
        ...
    }
}
```

**Rationale:** not bothering the caller with irrelevant details, not mixing user API with implementor API.

## Functions with many parameters

Avoid creating functions with many optional or boolean parameters.
Introduce a `Config` struct instead.

```rust
// GOOD
pub struct AnnotationConfig {
    pub binary_target: bool,
    pub annotate_runnables: bool,
    pub annotate_impls: bool,
}

pub fn annotations(
    db: &RootDatabase,
    file_id: FileId,
    config: AnnotationConfig
) -> Vec<Annotation> {
    ...
}

// BAD
pub fn annotations(
    db: &RootDatabase,
    file_id: FileId,
    binary_target: bool,
    annotate_runnables: bool,
    annotate_impls: bool,
) -> Vec<Annotation> {
    ...
}
```

**Rationale:** reducing churn.
If the function has many parameters, they most likely change frequently.
By packing them into a struct we protect all intermediary functions from changes.

Do not implement `Default` for the `Config` struct, the caller has more context to determine better defaults.
Do not store `Config` as a part of the `state`, pass it explicitly.
This gives more flexibility for the caller.

If there is variation not only in the input parameters, but in the return type as well, consider introducing a `Command` type.

```rust
// MAYBE GOOD
pub struct Query {
    pub name: String,
    pub case_sensitive: bool,
}

impl Query {
    pub fn all(self) -> Vec<Item> { ... }
    pub fn first(self) -> Option<Item> { ... }
}

// MAYBE BAD
fn query_all(name: String, case_sensitive: bool) -> Vec<Item> { ... }
fn query_first(name: String, case_sensitive: bool) -> Option<Item> { ... }
```

## Prefer Separate Functions Over Parameters

If a function has a `bool` or an `Option` parameter, and it is always called with `true`, `false`, `Some` and `None` literals, split the function in two.

```rust
// GOOD
fn caller_a() {
    foo()
}

fn caller_b() {
    foo_with_bar(Bar::new())
}

fn foo() { ... }
fn foo_with_bar(bar: Bar) { ... }

// BAD
fn caller_a() {
    foo(None)
}

fn caller_b() {
    foo(Some(Bar::new()))
}

fn foo(bar: Option<Bar>) { ... }
```

**Rationale:** more often than not, such functions display "`false sharing`" -- they have additional `if` branching inside for two different cases.
Splitting the two different control flows into two functions simplifies each path, and remove cross-dependencies between the two paths.
If there's common code between `foo` and `foo_with_bar`, extract *that* into a common helper.

## Appropriate String Types

When interfacing with OS APIs, use `OsString`, even if the original source of data is utf-8 encoded.
**Rationale:** cleanly delineates the boundary when the data goes into the OS-land.

Use `AbsPathBuf` and `AbsPath` over `std::Path`.
**Rationale:** rust-analyzer is a long-lived process which handles several projects at the same time.
It is important not to leak cwd by accident.

# Premature Pessimization

## Avoid Allocations

Avoid writing code which is slower than it needs to be.
Don't allocate a `Vec` where an iterator would do, don't allocate strings needlessly.

```rust
// GOOD
use itertools::Itertools;

let (first_word, second_word) = match text.split_ascii_whitespace().collect_tuple() {
    Some(it) => it,
    None => return,
}

// BAD
let words = text.split_ascii_whitespace().collect::<Vec<_>>();
if words.len() != 2 {
    return
}
```

**Rationale:** not allocating is almost always faster.

## Push Allocations to the Call Site

If allocation is inevitable, let the caller allocate the resource:

```rust
// GOOD
fn frobnicate(s: String) {
    ...
}

// BAD
fn frobnicate(s: &str) {
    let s = s.to_string();
    ...
}
```

**Rationale:** reveals the costs.
It is also more efficient when the caller already owns the allocation.

## Collection Types

Prefer `rustc_hash::FxHashMap` and `rustc_hash::FxHashSet` instead of the ones in `std::collections`.

**Rationale:** they use a hasher that's significantly faster and using them consistently will reduce code size by some small amount.

## Avoid Intermediate Collections

When writing a recursive function to compute a sets of things, use an accumulator parameter instead of returning a fresh collection.
Accumulator goes first in the list of arguments.

```rust
// GOOD
pub fn reachable_nodes(node: Node) -> FxHashSet<Node> {
    let mut res = FxHashSet::default();
    go(&mut res, node);
    res
}
fn go(acc: &mut FxHashSet<Node>, node: Node) {
    acc.insert(node);
    for n in node.neighbors() {
        go(acc, n);
    }
}

// BAD
pub fn reachable_nodes(node: Node) -> FxHashSet<Node> {
    let mut res = FxHashSet::default();
    res.insert(node);
    for n in node.neighbors() {
        res.extend(reachable_nodes(n));
    }
    res
}
```

**Rationale:** re-use allocations, accumulator style is more concise for complex cases.

## Avoid Monomorphization

Avoid making a lot of code type parametric, *especially* on the boundaries between crates.

```rust
// GOOD
fn frobnicate(f: impl FnMut()) {
    frobnicate_impl(&mut f)
}
fn frobnicate_impl(f: &mut dyn FnMut()) {
    // lots of code
}

// BAD
fn frobnicate(f: impl FnMut()) {
    // lots of code
}
```

Avoid `AsRef` polymorphism, it pays back only for widely used libraries:

```rust
// GOOD
fn frobnicate(f: &Path) {
}

// BAD
fn frobnicate(f: impl AsRef<Path>) {
}
```

**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*.
This allows for exceptionally good performance, but leads to increased compile times.
Runtime performance obeys 80%/20% rule -- only a small fraction of code is hot.
Compile time **does not** obey this rule -- all code has to be compiled.

# Style

## Order of Imports

Separate import groups with blank lines.
Use one `use` per crate.

Module declarations come before the imports.
Order them in "suggested reading order" for a person new to the code base.

```rust
mod x;
mod y;

// First std.
use std::{ ... }

// Second, external crates (both crates.io crates and other rust-analyzer crates).
use crate_foo::{ ... }
use crate_bar::{ ... }

// Then current crate.
use crate::{}

// Finally, parent and child modules, but prefer `use crate::`.
use super::{}

// Re-exports are treated as item definitions rather than imports, so they go
// after imports and modules. Use them sparingly.
pub use crate::x::Z;
```

**Rationale:** consistency.
Reading order is important for new contributors.
Grouping by crate allows spotting unwanted dependencies easier.

## Import Style

Qualify items from `hir` and `ast`.

```rust
// GOOD
use syntax::ast;

fn frobnicate(func: hir::Function, strukt: ast::Struct) {}

// BAD
use hir::Function;
use syntax::ast::Struct;

fn frobnicate(func: Function, strukt: Struct) {}
```

**Rationale:** avoids name clashes, makes the layer clear at a glance.

When implementing traits from `std::fmt` or `std::ops`, import the module:

```rust
// GOOD
use std::fmt;

impl fmt::Display for RenameError {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { .. }
}

// BAD
impl std::fmt::Display for RenameError {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { .. }
}

// BAD
use std::ops::Deref;

impl Deref for Widget {
    type Target = str;
    fn deref(&self) -> &str { .. }
}
```

**Rationale:** overall, less typing.
Makes it clear that a trait is implemented, rather than used.

Avoid local `use MyEnum::*` imports.
**Rationale:** consistency.

Prefer `use crate::foo::bar` to `use super::bar` or `use self::bar::baz`.
**Rationale:** consistency, this is the style which works in all cases.

By default, avoid re-exports.
**Rationale:** for non-library code, re-exports introduce two ways to use something and allow for inconsistency.

## Order of Items

Optimize for the reader who sees the file for the first time, and wants to get a general idea about what's going on.
People read things from top to bottom, so place most important things first.

Specifically, if all items except one are private, always put the non-private item on top.

```rust
// GOOD
pub(crate) fn frobnicate() {
    Helper::act()
}

#[derive(Default)]
struct Helper { stuff: i32 }

impl Helper {
    fn act(&self) {

    }
}

// BAD
#[derive(Default)]
struct Helper { stuff: i32 }

pub(crate) fn frobnicate() {
    Helper::act()
}

impl Helper {
    fn act(&self) {

    }
}
```

If there's a mixture of private and public items, put public items first.

Put `struct`s and `enum`s first, functions and impls last. Order type declarations in top-down manner.

```rust
// GOOD
struct Parent {
    children: Vec<Child>
}

struct Child;

impl Parent {
}

impl Child {
}

// BAD
struct Child;

impl Child {
}

struct Parent {
    children: Vec<Child>
}

impl Parent {
}
```

**Rationale:** easier to get the sense of the API by visually scanning the file.
If function bodies are folded in the editor, the source code should read as documentation for the public API.

## Context Parameters

Some parameters are threaded unchanged through many function calls.
They determine the "context" of the operation.
Pass such parameters first, not last.
If there are several context parameters, consider packing them into a `struct Ctx` and passing it as `&self`.

```rust
// GOOD
fn dfs(graph: &Graph, v: Vertex) -> usize {
    let mut visited = FxHashSet::default();
    return go(graph, &mut visited, v);

    fn go(graph: &Graph, visited: &mut FxHashSet<Vertex>, v: usize) -> usize {
        ...
    }
}

// BAD
fn dfs(v: Vertex, graph: &Graph) -> usize {
    fn go(v: usize, graph: &Graph, visited: &mut FxHashSet<Vertex>) -> usize {
        ...
    }

    let mut visited = FxHashSet::default();
    go(v, graph, &mut visited)
}
```

**Rationale:** consistency.
Context-first works better when non-context parameter is a lambda.

## Variable Naming

Use boring and long names for local variables ([yay code completion](https://github.com/rust-lang/rust-analyzer/pull/4162#discussion_r417130973)).
The default name is a lowercased name of the type: `global_state: GlobalState`.
Avoid ad-hoc acronyms and contractions, but use the ones that exist consistently (`db`, `ctx`, `acc`).
Prefer American spelling (color, behavior).

Default names:

* `res` -- "result of the function" local variable
* `it` -- I don't really care about the name
* `n_foos` -- number of foos (prefer this to `foo_count`)
* `foo_idx` -- index of `foo`

Many names in rust-analyzer conflict with keywords.
We use mangled names instead of `r#ident` syntax:

```
crate  -> krate
enum   -> enum_
fn     -> func
impl   -> imp
macro  -> mac
mod    -> module
struct -> strukt
trait  -> trait_
type   -> ty
```

**Rationale:** consistency.

## Early Returns

Do use early returns

```rust
// GOOD
fn foo() -> Option<Bar> {
    if !condition() {
        return None;
    }

    Some(...)
}

// BAD
fn foo() -> Option<Bar> {
    if condition() {
        Some(...)
    } else {
        None
    }
}
```

**Rationale:** reduce cognitive stack usage.

Use `return Err(err)` to throw an error:

```rust
// GOOD
fn f() -> Result<(), ()> {
    if condition {
        return Err(());
    }
    Ok(())
}

// BAD
fn f() -> Result<(), ()> {
    if condition {
        Err(())?;
    }
    Ok(())
}
```

**Rationale:** `return` has type `!`, which allows the compiler to flag dead
code (`Err(...)?` is of unconstrained generic type `T`).

## Comparisons

When doing multiple comparisons use `<`/`<=`, avoid `>`/`>=`.

```rust
// GOOD
assert!(lo <= x && x <= hi);
assert!(r1 < l2 || r2 < l1);
assert!(x < y);
assert!(0 < x);

// BAD
assert!(x >= lo && x <= hi);
assert!(r1 < l2 || l1 > r2);
assert!(y > x);
assert!(x > 0);
```

**Rationale:** Less-then comparisons are more intuitive, they correspond spatially to [real line](https://en.wikipedia.org/wiki/Real_line).

## If-let

Avoid `if let ... { } else { }` construct, use `match` instead.

```rust
// GOOD
match ctx.expected_type.as_ref() {
    Some(expected_type) => completion_ty == expected_type && !expected_type.is_unit(),
    None => false,
}

// BAD
if let Some(expected_type) = ctx.expected_type.as_ref() {
    completion_ty == expected_type && !expected_type.is_unit()
} else {
    false
}
```

**Rationale:** `match` is almost always more compact.
The `else` branch can get a more precise pattern: `None` or `Err(_)` instead of `_`.

## Match Ergonomics

Don't use the `ref` keyword.

**Rationale:** consistency & simplicity.
`ref` was required before [match ergonomics](https://github.com/rust-lang/rfcs/blob/master/text/2005-match-ergonomics.md).
Today, it is redundant.
Between `ref` and mach ergonomics, the latter is more ergonomic in most cases, and is simpler (does not require a keyword).

## Empty Match Arms

Use `=> (),` when a match arm is intentionally empty:

```rust
// GOOD
match result {
    Ok(_) => (),
    Err(err) => error!("{}", err),
}

// BAD
match result {
    Ok(_) => {}
    Err(err) => error!("{}", err),
}
```

**Rationale:** consistency.

## Functional Combinators

Use high order monadic combinators like `map`, `then` when they are a natural choice; don't bend the code to fit into some combinator.
If writing a chain of combinators creates friction, replace them with control flow constructs: `for`, `if`, `match`.
Mostly avoid `bool::then` and `Option::filter`.

```rust
// GOOD
if !x.cond() {
    return None;
}
Some(x)

// BAD
Some(x).filter(|it| it.cond())
```

This rule is more "soft" then others, and boils down mostly to taste.
The guiding principle behind this rule is that code should be dense in computation, and sparse in the number of expressions per line.
The second example contains *less* computation -- the `filter` function is an indirection for `if`, it doesn't do any useful work by itself.
At the same time, it is more crowded -- it takes more time to visually scan it.

**Rationale:** consistency, playing to language's strengths.
Rust has first-class support for imperative control flow constructs like `for` and `if`, while functions are less first-class due to lack of universal function type, currying, and non-first-class effects (`?`, `.await`).

## Turbofish

Prefer type ascription over the turbofish.
When ascribing types, avoid `_`

```rust
// GOOD
let mutable: Vec<T> = old.into_iter().map(|it| builder.make_mut(it)).collect();

// BAD
let mutable: Vec<_> = old.into_iter().map(|it| builder.make_mut(it)).collect();

// BAD
let mutable = old.into_iter().map(|it| builder.make_mut(it)).collect::<Vec<_>>();
```

**Rationale:** consistency, readability.
If compiler struggles to infer the type, the human would as well.
Having the result type specified up-front helps with understanding what the chain of iterator methods is doing.

## Helper Functions

Avoid creating single-use helper functions:

```rust
// GOOD
let buf = {
    let mut buf = get_empty_buf(&mut arena);
    buf.add_item(item);
    buf
};

// BAD
let buf = prepare_buf(&mut arena, item);

...

fn prepare_buf(arena: &mut Arena, item: Item) -> ItemBuf {
    let mut res = get_empty_buf(&mut arena);
    res.add_item(item);
    res
}
```

Exception: if you want to make use of `return` or `?`.

**Rationale:** single-use functions change frequently, adding or removing parameters adds churn.
A block serves just as well to delineate a bit of logic, but has access to all the context.
Re-using originally single-purpose function often leads to bad coupling.

## Local Helper Functions

Put nested helper functions at the end of the enclosing functions
(this requires using return statement).
Don't nest more than one level deep.

```rust
// GOOD
fn dfs(graph: &Graph, v: Vertex) -> usize {
    let mut visited = FxHashSet::default();
    return go(graph, &mut visited, v);

    fn go(graph: &Graph, visited: &mut FxHashSet<Vertex>, v: usize) -> usize {
        ...
    }
}

// BAD
fn dfs(graph: &Graph, v: Vertex) -> usize {
    fn go(graph: &Graph, visited: &mut FxHashSet<Vertex>, v: usize) -> usize {
        ...
    }

    let mut visited = FxHashSet::default();
    go(graph, &mut visited, v)
}
```

**Rationale:** consistency, improved top-down readability.

## Helper Variables

Introduce helper variables freely, especially for multiline conditions:

```rust
// GOOD
let rustfmt_not_installed =
    captured_stderr.contains("not installed") || captured_stderr.contains("not available");

match output.status.code() {
    Some(1) if !rustfmt_not_installed => Ok(None),
    _ => Err(format_err!("rustfmt failed:\n{}", captured_stderr)),
};

// BAD
match output.status.code() {
    Some(1)
        if !captured_stderr.contains("not installed")
           && !captured_stderr.contains("not available") => Ok(None),
    _ => Err(format_err!("rustfmt failed:\n{}", captured_stderr)),
};
```

**Rationale:** Like blocks, single-use variables are a cognitively cheap abstraction, as they have access to all the context.
Extra variables help during debugging, they make it easy to print/view important intermediate results.
Giving a name to a condition inside an `if` expression often improves clarity and leads to nicely formatted code.

## Token names

Use `T![foo]` instead of `SyntaxKind::FOO_KW`.

```rust
// GOOD
match p.current() {
    T![true] | T![false] => true,
    _ => false,
}

// BAD

match p.current() {
    SyntaxKind::TRUE_KW | SyntaxKind::FALSE_KW => true,
    _ => false,
}
```

**Rationale:** The macro uses the familiar Rust syntax, avoiding ambiguities like "is this a brace or bracket?".

## Documentation

Style inline code comments as proper sentences.
Start with a capital letter, end with a dot.

```rust
// GOOD

// Only simple single segment paths are allowed.
MergeBehavior::Last => {
    tree.use_tree_list().is_none() && tree.path().map(path_len) <= Some(1)
}

// BAD

// only simple single segment paths are allowed
MergeBehavior::Last => {
    tree.use_tree_list().is_none() && tree.path().map(path_len) <= Some(1)
}
```

**Rationale:** writing a sentence (or maybe even a paragraph) rather just "a comment" creates a more appropriate frame of mind.
It tricks you into writing down more of the context you keep in your head while coding.

For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines.
If the line is too long, you want to split the sentence in two :-)

**Rationale:** much easier to edit the text and read the diff, see [this link](https://asciidoctor.org/docs/asciidoc-recommended-practices/#one-sentence-per-line).