docs/dev/style.md

   1 Our approach to "clean code" is two-fold:
   2
   3 * We generally don't block PRs on style changes.
   4 * At the same time, all code in rust-analyzer is constantly refactored.
   5
   6 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.
   7 Sending small cleanup PRs (like renaming a single local variable) is encouraged.
   8
   9 # General
  10
  11 ## Scale of Changes
  12
  13 Everyone knows that it's better to send small & focused pull requests.
  14 The problem is, sometimes you *have* to, eg, rewrite the whole compiler, and that just doesn't fit into a set of isolated PRs.
  15
  16 The main things to keep an eye on are the boundaries between various components.
  17 There are three kinds of changes:
  18
  19 1. Internals of a single component are changed.
  20    Specifically, you don't change any `pub` items.
  21    A good example here would be an addition of a new assist.
  22
  23 2. API of a component is expanded.
  24    Specifically, you add a new `pub` function which wasn't there before.
  25    A good example here would be expansion of assist API, for example, to implement lazy assists or assists groups.
  26
  27 3. A new dependency between components is introduced.
  28    Specifically, you add a `pub use` reexport from another crate or you add a new line to the `[dependencies]` section of `Cargo.toml`.
  29    A good example here would be adding reference search capability to the assists crates.
  30
  31 For the first group, the change is generally merged as long as:
  32
  33 * it works for the happy case,
  34 * it has tests,
  35 * it doesn't panic for the unhappy case.
  36
  37 For the second group, the change would be subjected to quite a bit of scrutiny and iteration.
  38 The new API needs to be right (or at least easy to change later).
  39 The actual implementation doesn't matter that much.
  40 It's very important to minimize the amount of changed lines of code for changes of the second kind.
  41 Often, you start doing a change of the first kind, only to realise that you need to elevate to a change of the second kind.
  42 In this case, we'll probably ask you to split API changes into a separate PR.
  43
  44 Changes of the third group should be pretty rare, so we don't specify any specific process for them.
  45 That said, adding an innocent-looking `pub use` is a very simple way to break encapsulation, keep an eye on it!
  46
  47 Note: if you enjoyed this abstract hand-waving about boundaries, you might appreciate
  48 https://www.tedinski.com/2018/02/06/system-boundaries.html
  49
  50 ## Crates.io Dependencies
  51
  52 We try to be very conservative with usage of crates.io dependencies.
  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.
  55
  56 **Rationale:** keep compile times low, create ecosystem pressure for faster
  57 compiles, reduce the number of things which might break.
  58
  59 ## Commit Style
  60
  61 We don't have specific rules around git history hygiene.
  62 Maintaining clean git history is strongly encouraged, but not enforced.
  63 Use rebase workflow, it's OK to rewrite history during PR review process.
  64 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.
  65
  66 Avoid @mentioning people in commit messages and pull request descriptions(they are added to commit message by bors).
  67 Such messages create a lot of duplicate notification traffic during rebases.
  68
  69 If possible, write commit messages from user's perspective:
  70
  71 ```
  72 # GOOD
  73 Goto definition works inside macros
  74
  75 # BAD
  76 Use original span for FileId
  77 ```
  78
  79 This makes it easier to prepare a changelog.
  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
  86 ## Clippy
  87
  88 We don't enforce Clippy.
  89 A number of default lints have high false positive rate.
  90 Selectively patching false-positives with `allow(clippy)` is considered worse than not using Clippy at all.
  91 There's `cargo xtask lint` command which runs a subset of low-FPR lints.
  92 Careful tweaking of `xtask lint` is welcome.
  93 Of course, applying Clippy suggestions is welcome as long as they indeed improve the code.
  94
  95 **Rationale:** see [rust-lang/clippy#5537](https://github.com/rust-lang/rust-clippy/issues/5537).
  96
  97 # Code
  98
  99 ## Minimal Tests
 100
 101 Most tests in rust-analyzer start with a snippet of Rust code.
 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.
 103
 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),
 105 as long as they are still readable.
 106
 107 When using multiline fixtures, use unindented raw string literals:
 108
 109 ```rust
 110     #[test]
 111     fn inline_field_shorthand() {
 112         check_assist(
 113             inline_local_variable,
 114             r#"
 115 struct S { foo: i32}
 116 fn main() {
 117     let $0foo = 92;
 118     S { foo }
 119 }
 120 "#,
 121             r#"
 122 struct S { foo: i32}
 123 fn main() {
 124     S { foo: 92 }
 125 }
 126 "#,
 127         );
 128     }
 129 ```
 130
 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
 139
 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
 143
 144 Express function preconditions in types and force the caller to provide them (rather than checking in callee):
 145
 146 ```rust
 147 // GOOD
 148 fn frbonicate(walrus: Walrus) {
 149     ...
 150 }
 151
 152 // BAD
 153 fn frobnicate(walrus: Option<Walrus>) {
 154     let walrus = match walrus {
 155         Some(it) => it,
 156         None => return,
 157     };
 158     ...
 159 }
 160 ```
 161
 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:
 166
 167 ```rust
 168 // GOOD
 169 fn main() {
 170     let s: &str = ...;
 171     if let Some(contents) = string_literal_contents(s) {
 172
 173     }
 174 }
 175
 176 fn string_literal_contents(s: &str) -> Option<&str> {
 177     if s.starts_with('"') && s.ends_with('"') {
 178         Some(&s[1..s.len() - 1])
 179     } else {
 180         None
 181     }
 182 }
 183
 184 // BAD
 185 fn main() {
 186     let s: &str = ...;
 187     if is_string_literal(s) {
 188         let contents = &s[1..s.len() - 1];
 189     }
 190 }
 191
 192 fn is_string_literal(s: &str) -> bool {
 193     s.starts_with('"') && s.ends_with('"')
 194 }
 195 ```
 196
 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`.
 198 In the "Good" version, the precondition check and usage are checked in the same block, and then encoded in the types.
 199
 200 **Rationale:** non-local code properties degrade under change.
 201
 202 When checking a boolean precondition, prefer `if !invariant` to `if negated_invariant`:
 203
 204 ```rust
 205 // GOOD
 206 if !(idx < len) {
 207     return None;
 208 }
 209
 210 // BAD
 211 if idx >= len {
 212     return None;
 213 }
 214 ```
 215
 216 **Rationale:** its useful to see the invariant relied upon by the rest of the function clearly spelled out.
 217
 218 ## Assertions
 219
 220 Assert liberally.
 221 Prefer `stdx::assert_never!` to standard `assert!`.
 222
 223 ## Getters & Setters
 224
 225 If a field can have any value without breaking invariants, make the field public.
 226 Conversely, if there is an invariant, document it, enforce it in the "constructor" function, make the field private, and provide a getter.
 227 Never provide setters.
 228
 229 Getters should return borrowed data:
 230
 231 ```rust
 232 struct Person {
 233     // Invariant: never empty
 234     first_name: String,
 235     middle_name: Option<String>
 236 }
 237
 238 // GOOD
 239 impl Person {
 240     fn first_name(&self) -> &str { self.first_name.as_str() }
 241     fn middle_name(&self) -> Option<&str> { self.middle_name.as_ref() }
 242 }
 243
 244 // BAD
 245 impl Person {
 246     fn first_name(&self) -> String { self.first_name.clone() }
 247     fn middle_name(&self) -> &Option<String> { &self.middle_name }
 248 }
 249 ```
 250
 251 **Rationale:** we don't provide public API, it's cheaper to refactor than to pay getters rent.
 252 Non-local code properties degrade under change, privacy makes invariant local.
 253 Borrowed own data discloses irrelevant details about origin of data.
 254 Irrelevant (neither right nor wrong) things obscure correctness.
 255
 256 ## Constructors
 257
 258 Prefer `Default` to zero-argument `new` function
 259
 260 ```rust
 261 // GOOD
 262 #[derive(Default)]
 263 struct Foo {
 264     bar: Option<Bar>
 265 }
 266
 267 // BAD
 268 struct Foo {
 269     bar: Option<Bar>
 270 }
 271
 272 impl Foo {
 273     fn new() -> Foo {
 274         Foo { bar: None }
 275     }
 276 }
 277 ```
 278
 279 Prefer `Default` even it has to be implemented manually.
 280
 281 **Rationale:** less typing in the common case, uniformity.
 282
 283 Use `Vec::new` rather than `vec![]`. **Rationale:** uniformity, strength
 284 reduction.
 285
 286 ## Functions Over Objects
 287
 288 Avoid creating "doer" objects.
 289 That is, objects which are created only to execute a single action.
 290
 291 ```rust
 292 // GOOD
 293 do_thing(arg1, arg2);
 294
 295 // BAD
 296 ThingDoer::new(arg1, arg2).do();
 297 ```
 298
 299 Note that this concerns only outward API.
 300 When implementing `do_thing`, it might be very useful to create a context object.
 301
 302 ```rust
 303 pub fn do_thing(arg1: Arg1, arg2: Arg2) -> Res {
 304     let mut ctx = Ctx { arg1, arg2 }
 305     ctx.run()
 306 }
 307
 308 struct Ctx {
 309     arg1: Arg1, arg2: Arg2
 310 }
 311
 312 impl Ctx {
 313     fn run(self) -> Res {
 314         ...
 315     }
 316 }
 317 ```
 318
 319 The difference is that `Ctx` is an impl detail here.
 320
 321 Sometimes a middle ground is acceptable if this can save some busywork:
 322
 323 ```rust
 324 ThingDoer::do(arg1, arg2);
 325
 326 pub struct ThingDoer {
 327     arg1: Arg1, arg2: Arg2,
 328 }
 329
 330 impl ThingDoer {
 331     pub fn do(arg1: Arg1, arg2: Arg2) -> Res {
 332         ThingDoer { arg1, arg2 }.run()
 333     }
 334     fn run(self) -> Res {
 335         ...
 336     }
 337 }
 338 ```
 339
 340 **Rationale:** not bothering the caller with irrelevant details, not mixing user API with implementor API.
 341
 342 ## Avoid Monomorphization
 343
 344 Avoid making a lot of code type parametric, *especially* on the boundaries between crates.
 345
 346 ```rust
 347 // GOOD
 348 fn frbonicate(f: impl FnMut()) {
 349     frobnicate_impl(&mut f)
 350 }
 351 fn frobnicate_impl(f: &mut dyn FnMut()) {
 352     // lots of code
 353 }
 354
 355 // BAD
 356 fn frbonicate(f: impl FnMut()) {
 357     // lots of code
 358 }
 359 ```
 360
 361 Avoid `AsRef` polymorphism, it pays back only for widely used libraries:
 362
 363 ```rust
 364 // GOOD
 365 fn frbonicate(f: &Path) {
 366 }
 367
 368 // BAD
 369 fn frbonicate(f: impl AsRef<Path>) {
 370 }
 371 ```
 372
 373 **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*.
 374 This allows for exceptionally good performance, but leads to increased compile times.
 375 Runtime performance obeys 80%/20% rule -- only a small fraction of code is hot.
 376 Compile time **does not** obey this rule -- all code has to be compiled.
 377
 378
 379 # Premature Pessimization
 380
 381 ## Avoid Allocations
 382
 383 Avoid writing code which is slower than it needs to be.
 384 Don't allocate a `Vec` where an iterator would do, don't allocate strings needlessly.
 385
 386 ```rust
 387 // GOOD
 388 use itertools::Itertools;
 389
 390 let (first_word, second_word) = match text.split_ascii_whitespace().collect_tuple() {
 391     Some(it) => it,
 392     None => return,
 393 }
 394
 395 // BAD
 396 let words = text.split_ascii_whitespace().collect::<Vec<_>>();
 397 if words.len() != 2 {
 398     return
 399 }
 400 ```
 401
 402 **Rationale:** not allocating is almost often faster.
 403
 404 ## Push Allocations to the Call Site
 405
 406 If allocation is inevitable, let the caller allocate the resource:
 407
 408 ```rust
 409 // GOOD
 410 fn frobnicate(s: String) {
 411     ...
 412 }
 413
 414 // BAD
 415 fn frobnicate(s: &str) {
 416     let s = s.to_string();
 417     ...
 418 }
 419 ```
 420
 421 **Rationale:** reveals the costs.
 422 It is also more efficient when the caller already owns the allocation.
 423
 424 ## Collection types
 425
 426 Prefer `rustc_hash::FxHashMap` and `rustc_hash::FxHashSet` instead of the ones in `std::collections`.
 427
 428 **Rationale:** they use a hasher that's significantly faster and using them consistently will reduce code size by some small amount.
 429
 430 # Style
 431
 432 ## Order of Imports
 433
 434 Separate import groups with blank lines.
 435 Use one `use` per crate.
 436
 437 Module declarations come before the imports.
 438 Order them in "suggested reading order" for a person new to the code base.
 439
 440 ```rust
 441 mod x;
 442 mod y;
 443
 444 // First std.
 445 use std::{ ... }
 446
 447 // Second, external crates (both crates.io crates and other rust-analyzer crates).
 448 use crate_foo::{ ... }
 449 use crate_bar::{ ... }
 450
 451 // Then current crate.
 452 use crate::{}
 453
 454 // Finally, parent and child modules, but prefer `use crate::`.
 455 use super::{}
 456 ```
 457
 458 **Rationale:** consistency.
 459 Reading order is important for new contributors.
 460 Grouping by crate allows to spot unwanted dependencies easier.
 461
 462 ## Import Style
 463
 464 Qualify items from `hir` and `ast`.
 465
 466 ```rust
 467 // GOOD
 468 use syntax::ast;
 469
 470 fn frobnicate(func: hir::Function, strukt: ast::Struct) {}
 471
 472 // BAD
 473 use hir::Function;
 474 use syntax::ast::Struct;
 475
 476 fn frobnicate(func: Function, strukt: Struct) {}
 477 ```
 478
 479 **Rationale:** avoids name clashes, makes the layer clear at a glance.
 480
 481 When implementing traits from `std::fmt` or `std::ops`, import the module:
 482
 483 ```rust
 484 // GOOD
 485 use std::fmt;
 486
 487 impl fmt::Display for RenameError {
 488     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { .. }
 489 }
 490
 491 // BAD
 492 impl std::fmt::Display for RenameError {
 493     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { .. }
 494 }
 495
 496 // BAD
 497 use std::ops::Deref;
 498
 499 impl Deref for Widget {
 500     type Target = str;
 501     fn deref(&self) -> &str { .. }
 502 }
 503 ```
 504
 505 **Rationale:** overall, less typing.
 506 Makes it clear that a trait is implemented, rather than used.
 507
 508 Avoid local `use MyEnum::*` imports.
 509 **Rationale:** consistency.
 510
 511 Prefer `use crate::foo::bar` to `use super::bar` or `use self::bar::baz`.
 512 **Rationale:** consistency, this is the style which works in all cases.
 513
 514 ## Order of Items
 515
 516 Optimize for the reader who sees the file for the first time, and wants to get a general idea about what's going on.
 517 People read things from top to bottom, so place most important things first.
 518
 519 Specifically, if all items except one are private, always put the non-private item on top.
 520
 521 ```rust
 522 // GOOD
 523 pub(crate) fn frobnicate() {
 524     Helper::act()
 525 }
 526
 527 #[derive(Default)]
 528 struct Helper { stuff: i32 }
 529
 530 impl Helper {
 531     fn act(&self) {
 532
 533     }
 534 }
 535
 536 // BAD
 537 #[derive(Default)]
 538 struct Helper { stuff: i32 }
 539
 540 pub(crate) fn frobnicate() {
 541     Helper::act()
 542 }
 543
 544 impl Helper {
 545     fn act(&self) {
 546
 547     }
 548 }
 549 ```
 550
 551 If there's a mixture of private and public items, put public items first.
 552
 553 Put `struct`s and `enum`s first, functions and impls last. Order type declarations in top-down manner.
 554
 555 ```rust
 556 // GOOD
 557 struct Parent {
 558     children: Vec<Child>
 559 }
 560
 561 struct Child;
 562
 563 impl Parent {
 564 }
 565
 566 impl Child {
 567 }
 568
 569 // BAD
 570 struct Child;
 571
 572 impl Child {
 573 }
 574
 575 struct Parent {
 576     children: Vec<Child>
 577 }
 578
 579 impl Parent {
 580 }
 581 ```
 582
 583 **Rationale:** easier to get the sense of the API by visually scanning the file.
 584 If function bodies are folded in the editor, the source code should read as documentation for the public API.
 585
 586 ## Variable Naming
 587
 588 Use boring and long names for local variables ([yay code completion](https://github.com/rust-analyzer/rust-analyzer/pull/4162#discussion_r417130973)).
 589 The default name is a lowercased name of the type: `global_state: GlobalState`.
 590 Avoid ad-hoc acronyms and contractions, but use the ones that exist consistently (`db`, `ctx`, `acc`).
 591 Prefer American spelling (color, behavior).
 592
 593 Default names:
 594
 595 * `res` -- "result of the function" local variable
 596 * `it` -- I don't really care about the name
 597 * `n_foo` -- number of foos
 598 * `foo_idx` -- index of `foo`
 599
 600 Many names in rust-analyzer conflict with keywords.
 601 We use mangled names instead of `r#ident` syntax:
 602
 603 ```
 604 struct -> strukt
 605 crate  -> krate
 606 impl   -> imp
 607 trait  -> trait_
 608 fn     -> func
 609 enum   -> enum_
 610 mod    -> module
 611 ```
 612
 613 **Rationale:** consistency.
 614
 615 ## Early Returns
 616
 617 Do use early returns
 618
 619 ```rust
 620 // GOOD
 621 fn foo() -> Option<Bar> {
 622     if !condition() {
 623         return None;
 624     }
 625
 626     Some(...)
 627 }
 628
 629 // BAD
 630 fn foo() -> Option<Bar> {
 631     if condition() {
 632         Some(...)
 633     } else {
 634         None
 635     }
 636 }
 637 ```
 638
 639 **Rationale:** reduce congnitive stack usage.
 640
 641 ## Comparisons
 642
 643 Use `<`/`<=`, avoid `>`/`>=`.
 644
 645 ```rust
 646 // GOOD
 647 assert!(lo <= x && x <= hi);
 648
 649 // BAD
 650 assert!(x >= lo && x <= hi>);
 651 ```
 652
 653 **Rationale:** Less-then comparisons are more intuitive, they correspond spatially to [real line](https://en.wikipedia.org/wiki/Real_line).
 654
 655
 656 ## Token names
 657
 658 Use `T![foo]` instead of `SyntaxKind::FOO_KW`.
 659
 660 ```rust
 661 // GOOD
 662 match p.current() {
 663     T![true] | T![false] => true,
 664     _ => false,
 665 }
 666
 667 // BAD
 668
 669 match p.current() {
 670     SyntaxKind::TRUE_KW | SyntaxKind::FALSE_KW => true,
 671     _ => false,
 672 }
 673 ```
 674
 675 **Rationale:** The macro uses the familiar Rust syntax, avoiding ambiguities like "is this a brace or bracket?".
 676
 677 ## Documentation
 678
 679 For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines.
 680 If the line is too long, you want to split the sentence in two :-)
 681
 682 **Rationale:** much easier to edit the text and read the diff.