1Our 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 6It is explicitly OK for a reviewer to flag only some nits in the PR, and then send a follow-up cleanup PR for things which are easier to explain by example, cc-ing the original author. 7Sending small cleanup PRs (like renaming a single local variable) is encouraged. 8 9When reviewing pull requests prefer extending this document to leaving 10non-reusable comments on the pull request itself. 11 12# General 13 14## Scale of Changes 15 16Everyone knows that it's better to send small & focused pull requests. 17The problem is, sometimes you *have* to, eg, rewrite the whole compiler, and that just doesn't fit into a set of isolated PRs. 18 19The main things to keep an eye on are the boundaries between various components. 20There are three kinds of changes: 21 221. Internals of a single component are changed. 23 Specifically, you don't change any `pub` items. 24 A good example here would be an addition of a new assist. 25 262. API of a component is expanded. 27 Specifically, you add a new `pub` function which wasn't there before. 28 A good example here would be expansion of assist API, for example, to implement lazy assists or assists groups. 29 303. A new dependency between components is introduced. 31 Specifically, you add a `pub use` reexport from another crate or you add a new line to the `[dependencies]` section of `Cargo.toml`. 32 A good example here would be adding reference search capability to the assists crates. 33 34For the first group, the change is generally merged as long as: 35 36* it works for the happy case, 37* it has tests, 38* it doesn't panic for the unhappy case. 39 40For the second group, the change would be subjected to quite a bit of scrutiny and iteration. 41The new API needs to be right (or at least easy to change later). 42The actual implementation doesn't matter that much. 43It's very important to minimize the amount of changed lines of code for changes of the second kind. 44Often, you start doing a change of the first kind, only to realize that you need to elevate to a change of the second kind. 45In this case, we'll probably ask you to split API changes into a separate PR. 46 47Changes of the third group should be pretty rare, so we don't specify any specific process for them. 48That said, adding an innocent-looking `pub use` is a very simple way to break encapsulation, keep an eye on it! 49 50Note: if you enjoyed this abstract hand-waving about boundaries, you might appreciate 51https://www.tedinski.com/2018/02/06/system-boundaries.html 52 53## Crates.io Dependencies 54 55We try to be very conservative with usage of crates.io dependencies. 56Don't use small "helper" crates (exception: `itertools` and `either` are allowed). 57If there's some general reusable bit of code you need, consider adding it to the `stdx` crate. 58A useful exercise is to read Cargo.lock and see if some *transitive* dependencies do not make sense for rust-analyzer. 59 60**Rationale:** keep compile times low, create ecosystem pressure for faster compiles, reduce the number of things which might break. 61 62## Commit Style 63 64We don't have specific rules around git history hygiene. 65Maintaining clean git history is strongly encouraged, but not enforced. 66Use rebase workflow, it's OK to rewrite history during PR review process. 67After 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. 68 69Avoid @mentioning people in commit messages and pull request descriptions(they are added to commit message by bors). 70Such messages create a lot of duplicate notification traffic during rebases. 71 72If possible, write commit messages from user's perspective: 73 74``` 75# GOOD 76Goto definition works inside macros 77 78# BAD 79Use original span for FileId 80``` 81 82This makes it easier to prepare a changelog. 83 84If 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. 85 86To make writing the release notes easier, you can mark a pull request as a feature, fix, internal change, or minor. 87Minor changes are excluded from the release notes, while the other types are distributed in their corresponding sections. 88There are two ways to mark this: 89 90* use a `feat: `, `feature: `, `fix: `, `internal: ` or `minor: ` prefix in the PR title 91* 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. 92 93These comments don't have to be added by the PR author. 94Editing a comment or the PR description or title is also fine, as long as it happens before the release. 95 96**Rationale:** clean history is potentially useful, but rarely used. 97But many users read changelogs. 98Including a description and GIF suitable for the changelog means less work for the maintainers on the release day. 99 100## Clippy 101 102We don't enforce Clippy. 103A number of default lints have high false positive rate. 104Selectively patching false-positives with `allow(clippy)` is considered worse than not using Clippy at all. 105There's a `cargo lint` command which runs a subset of low-FPR lints. 106Careful tweaking of `lint` is welcome. 107Of course, applying Clippy suggestions is welcome as long as they indeed improve the code. 108 109**Rationale:** see [rust-lang/clippy#5537](https://github.com/rust-lang/rust-clippy/issues/5537). 110 111# Code 112 113## Minimal Tests 114 115Most tests in rust-analyzer start with a snippet of Rust code. 116These 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. 117 118It also makes sense to format snippets more compactly (for example, by placing enum definitions like `enum E { Foo, Bar }` on a single line), 119as long as they are still readable. 120 121When using multiline fixtures, use unindented raw string literals: 122 123```rust 124 #[test] 125 fn inline_field_shorthand() { 126 check_assist( 127 inline_local_variable, 128 r#" 129struct S { foo: i32} 130fn main() { 131 let $0foo = 92; 132 S { foo } 133} 134"#, 135 r#" 136struct S { foo: i32} 137fn main() { 138 S { foo: 92 } 139} 140"#, 141 ); 142 } 143``` 144 145**Rationale:** 146 147There are many benefits to this: 148 149* less to read or to scroll past 150* easier to understand what exactly is tested 151* less stuff printed during printf-debugging 152* less time to run test 153 154Formatting ensures that you can use your editor's "number of selected characters" feature to correlate offsets with test's source code. 155 156## Marked Tests 157 158Use 159[`cov_mark::hit! / cov_mark::check!`](https://github.com/matklad/cov-mark) 160when testing specific conditions. 161Do not place several marks into a single test or condition. 162Do not reuse marks between several tests. 163 164**Rationale:** marks provide an easy way to find the canonical test for each bit of code. 165This makes it much easier to understand. 166More than one mark per test / code branch doesn't add significantly to understanding. 167 168## `#[should_panic]` 169 170Do not use `#[should_panic]` tests. 171Instead, explicitly check for `None`, `Err`, etc. 172 173**Rationale:** `#[should_panic]` is a tool for library authors to make sure that the API does not fail silently when misused. 174`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. 175Panic messages in the logs from the `#[should_panic]` tests are confusing. 176 177## `#[ignore]` 178 179Do not `#[ignore]` tests. 180If the test currently does not work, assert the wrong behavior and add a fixme explaining why it is wrong. 181 182**Rationale:** noticing when the behavior is fixed, making sure that even the wrong behavior is acceptable (ie, not a panic). 183 184## Function Preconditions 185 186Express function preconditions in types and force the caller to provide them (rather than checking in callee): 187 188```rust 189// GOOD 190fn frobnicate(walrus: Walrus) { 191 ... 192} 193 194// BAD 195fn frobnicate(walrus: Option<Walrus>) { 196 let walrus = match walrus { 197 Some(it) => it, 198 None => return, 199 }; 200 ... 201} 202``` 203 204**Rationale:** this makes control flow explicit at the call site. 205Call-site has more context, it often happens that the precondition falls out naturally or can be bubbled up higher in the stack. 206 207Avoid splitting precondition check and precondition use across functions: 208 209```rust 210// GOOD 211fn main() { 212 let s: &str = ...; 213 if let Some(contents) = string_literal_contents(s) { 214 215 } 216} 217 218fn string_literal_contents(s: &str) -> Option<&str> { 219 if s.starts_with('"') && s.ends_with('"') { 220 Some(&s[1..s.len() - 1]) 221 } else { 222 None 223 } 224} 225 226// BAD 227fn main() { 228 let s: &str = ...; 229 if is_string_literal(s) { 230 let contents = &s[1..s.len() - 1]; 231 } 232} 233 234fn is_string_literal(s: &str) -> bool { 235 s.starts_with('"') && s.ends_with('"') 236} 237``` 238 239In the "Not as good" version, the precondition that `1` is a valid char boundary is checked in `is_string_literal` and used in `foo`. 240In the "Good" version, the precondition check and usage are checked in the same block, and then encoded in the types. 241 242**Rationale:** non-local code properties degrade under change. 243 244When checking a boolean precondition, prefer `if !invariant` to `if negated_invariant`: 245 246```rust 247// GOOD 248if !(idx < len) { 249 return None; 250} 251 252// BAD 253if idx >= len { 254 return None; 255} 256``` 257 258**Rationale:** it's useful to see the invariant relied upon by the rest of the function clearly spelled out. 259 260## Control Flow 261 262As a special case of the previous rule, do not hide control flow inside functions, push it to the caller: 263 264```rust 265// GOOD 266if cond { 267 f() 268} 269 270// BAD 271fn f() { 272 if !cond { 273 return; 274 } 275 ... 276} 277``` 278 279## Assertions 280 281Assert liberally. 282Prefer [`stdx::never!`](https://docs.rs/always-assert/0.1.2/always_assert/macro.never.html) to standard `assert!`. 283 284**Rationale:** See [cross cutting concern: error handling](https://github.com/rust-analyzer/rust-analyzer/blob/master/docs/dev/architecture.md#error-handling). 285 286## Getters & Setters 287 288If a field can have any value without breaking invariants, make the field public. 289Conversely, if there is an invariant, document it, enforce it in the "constructor" function, make the field private, and provide a getter. 290Never provide setters. 291 292Getters should return borrowed data: 293 294```rust 295struct Person { 296 // Invariant: never empty 297 first_name: String, 298 middle_name: Option<String> 299} 300 301// GOOD 302impl Person { 303 fn first_name(&self) -> &str { self.first_name.as_str() } 304 fn middle_name(&self) -> Option<&str> { self.middle_name.as_ref() } 305} 306 307// BAD 308impl Person { 309 fn first_name(&self) -> String { self.first_name.clone() } 310 fn middle_name(&self) -> &Option<String> { &self.middle_name } 311} 312``` 313 314**Rationale:** we don't provide public API, it's cheaper to refactor than to pay getters rent. 315Non-local code properties degrade under change, privacy makes invariant local. 316Borrowed owned types (`&String`) disclose irrelevant details about internal representation. 317Irrelevant (neither right nor wrong) things obscure correctness. 318 319## Useless Types 320 321More generally, always prefer types on the left 322 323```rust 324// GOOD BAD 325&[T] &Vec<T> 326&str &String 327Option<&T> &Option<T> 328&Path &PathBuf 329``` 330 331**Rationale:** types on the left are strictly more general. 332Even when generality is not required, consistency is important. 333 334## Constructors 335 336Prefer `Default` to zero-argument `new` function. 337 338```rust 339// GOOD 340#[derive(Default)] 341struct Foo { 342 bar: Option<Bar> 343} 344 345// BAD 346struct Foo { 347 bar: Option<Bar> 348} 349 350impl Foo { 351 fn new() -> Foo { 352 Foo { bar: None } 353 } 354} 355``` 356 357Prefer `Default` even if it has to be implemented manually. 358 359**Rationale:** less typing in the common case, uniformity. 360 361Use `Vec::new` rather than `vec![]`. 362 363**Rationale:** uniformity, strength reduction. 364 365Avoid using "dummy" states to implement a `Default`. 366If a type doesn't have a sensible default, empty value, don't hide it. 367Let the caller explicitly decide what the right initial state is. 368 369## Functions Over Objects 370 371Avoid creating "doer" objects. 372That is, objects which are created only to execute a single action. 373 374```rust 375// GOOD 376do_thing(arg1, arg2); 377 378// BAD 379ThingDoer::new(arg1, arg2).do(); 380``` 381 382Note that this concerns only outward API. 383When implementing `do_thing`, it might be very useful to create a context object. 384 385```rust 386pub fn do_thing(arg1: Arg1, arg2: Arg2) -> Res { 387 let mut ctx = Ctx { arg1, arg2 }; 388 ctx.run() 389} 390 391struct Ctx { 392 arg1: Arg1, arg2: Arg2 393} 394 395impl Ctx { 396 fn run(self) -> Res { 397 ... 398 } 399} 400``` 401 402The difference is that `Ctx` is an impl detail here. 403 404Sometimes a middle ground is acceptable if this can save some busywork: 405 406```rust 407ThingDoer::do(arg1, arg2); 408 409pub struct ThingDoer { 410 arg1: Arg1, arg2: Arg2, 411} 412 413impl ThingDoer { 414 pub fn do(arg1: Arg1, arg2: Arg2) -> Res { 415 ThingDoer { arg1, arg2 }.run() 416 } 417 fn run(self) -> Res { 418 ... 419 } 420} 421``` 422 423**Rationale:** not bothering the caller with irrelevant details, not mixing user API with implementor API. 424 425## Functions with many parameters 426 427Avoid creating functions with many optional or boolean parameters. 428Introduce a `Config` struct instead. 429 430```rust 431// GOOD 432pub struct AnnotationConfig { 433 pub binary_target: bool, 434 pub annotate_runnables: bool, 435 pub annotate_impls: bool, 436} 437 438pub fn annotations( 439 db: &RootDatabase, 440 file_id: FileId, 441 config: AnnotationConfig 442) -> Vec<Annotation> { 443 ... 444} 445 446// BAD 447pub fn annotations( 448 db: &RootDatabase, 449 file_id: FileId, 450 binary_target: bool, 451 annotate_runnables: bool, 452 annotate_impls: bool, 453) -> Vec<Annotation> { 454 ... 455} 456``` 457 458**Rationale:** reducing churn. 459If the function has many parameters, they most likely change frequently. 460By packing them into a struct we protect all intermediary functions from changes. 461 462Do not implement `Default` for the `Config` struct, the caller has more context to determine better defaults. 463Do not store `Config` as a part of the `state`, pass it explicitly. 464This gives more flexibility for the caller. 465 466If there is variation not only in the input parameters, but in the return type as well, consider introducing a `Command` type. 467 468```rust 469// MAYBE GOOD 470pub struct Query { 471 pub name: String, 472 pub case_sensitive: bool, 473} 474 475impl Query { 476 pub fn all(self) -> Vec<Item> { ... } 477 pub fn first(self) -> Option<Item> { ... } 478} 479 480// MAYBE BAD 481fn query_all(name: String, case_sensitive: bool) -> Vec<Item> { ... } 482fn query_first(name: String, case_sensitive: bool) -> Option<Item> { ... } 483``` 484 485## Prefer Separate Functions Over Parameters 486 487If 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. 488 489```rust 490// GOOD 491fn caller_a() { 492 foo() 493} 494 495fn caller_b() { 496 foo_with_bar(Bar::new()) 497} 498 499fn foo() { ... } 500fn foo_with_bar(bar: Bar) { ... } 501 502// BAD 503fn caller_a() { 504 foo(None) 505} 506 507fn caller_b() { 508 foo(Some(Bar::new())) 509} 510 511fn foo(bar: Option<Bar>) { ... } 512``` 513 514**Rationale:** more often than not, such functions display "`false sharing`" -- they have additional `if` branching inside for two different cases. 515Splitting the two different control flows into two functions simplifies each path, and remove cross-dependencies between the two paths. 516If there's common code between `foo` and `foo_with_bar`, extract *that* into a common helper. 517 518## Appropriate String Types 519 520When interfacing with OS APIs, use `OsString`, even if the original source of data is utf-8 encoded. 521**Rationale:** cleanly delineates the boundary when the data goes into the OS-land. 522 523Use `AbsPathBuf` and `AbsPath` over `std::Path`. 524**Rationale:** rust-analyzer is a long-lived process which handles several projects at the same time. 525It is important not to leak cwd by accident. 526 527# Premature Pessimization 528 529## Avoid Allocations 530 531Avoid writing code which is slower than it needs to be. 532Don't allocate a `Vec` where an iterator would do, don't allocate strings needlessly. 533 534```rust 535// GOOD 536use itertools::Itertools; 537 538let (first_word, second_word) = match text.split_ascii_whitespace().collect_tuple() { 539 Some(it) => it, 540 None => return, 541} 542 543// BAD 544let words = text.split_ascii_whitespace().collect::<Vec<_>>(); 545if words.len() != 2 { 546 return 547} 548``` 549 550**Rationale:** not allocating is almost always faster. 551 552## Push Allocations to the Call Site 553 554If allocation is inevitable, let the caller allocate the resource: 555 556```rust 557// GOOD 558fn frobnicate(s: String) { 559 ... 560} 561 562// BAD 563fn frobnicate(s: &str) { 564 let s = s.to_string(); 565 ... 566} 567``` 568 569**Rationale:** reveals the costs. 570It is also more efficient when the caller already owns the allocation. 571 572## Collection Types 573 574Prefer `rustc_hash::FxHashMap` and `rustc_hash::FxHashSet` instead of the ones in `std::collections`. 575 576**Rationale:** they use a hasher that's significantly faster and using them consistently will reduce code size by some small amount. 577 578## Avoid Intermediate Collections 579 580When writing a recursive function to compute a sets of things, use an accumulator parameter instead of returning a fresh collection. 581Accumulator goes first in the list of arguments. 582 583```rust 584// GOOD 585pub fn reachable_nodes(node: Node) -> FxHashSet<Node> { 586 let mut res = FxHashSet::default(); 587 go(&mut res, node); 588 res 589} 590fn go(acc: &mut FxHashSet<Node>, node: Node) { 591 acc.insert(node); 592 for n in node.neighbors() { 593 go(acc, n); 594 } 595} 596 597// BAD 598pub fn reachable_nodes(node: Node) -> FxHashSet<Node> { 599 let mut res = FxHashSet::default(); 600 res.insert(node); 601 for n in node.neighbors() { 602 res.extend(reachable_nodes(n)); 603 } 604 res 605} 606``` 607 608**Rationale:** re-use allocations, accumulator style is more concise for complex cases. 609 610## Avoid Monomorphization 611 612Avoid making a lot of code type parametric, *especially* on the boundaries between crates. 613 614```rust 615// GOOD 616fn frobnicate(f: impl FnMut()) { 617 frobnicate_impl(&mut f) 618} 619fn frobnicate_impl(f: &mut dyn FnMut()) { 620 // lots of code 621} 622 623// BAD 624fn frobnicate(f: impl FnMut()) { 625 // lots of code 626} 627``` 628 629Avoid `AsRef` polymorphism, it pays back only for widely used libraries: 630 631```rust 632// GOOD 633fn frobnicate(f: &Path) { 634} 635 636// BAD 637fn frobnicate(f: impl AsRef<Path>) { 638} 639``` 640 641**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*. 642This allows for exceptionally good performance, but leads to increased compile times. 643Runtime performance obeys 80%/20% rule -- only a small fraction of code is hot. 644Compile time **does not** obey this rule -- all code has to be compiled. 645 646# Style 647 648## Order of Imports 649 650Separate import groups with blank lines. 651Use one `use` per crate. 652 653Module declarations come before the imports. 654Order them in "suggested reading order" for a person new to the code base. 655 656```rust 657mod x; 658mod y; 659 660// First std. 661use std::{ ... } 662 663// Second, external crates (both crates.io crates and other rust-analyzer crates). 664use crate_foo::{ ... } 665use crate_bar::{ ... } 666 667// Then current crate. 668use crate::{} 669 670// Finally, parent and child modules, but prefer `use crate::`. 671use super::{} 672 673// Re-exports are treated as item definitions rather than imports, so they go 674// after imports and modules. Use them sparingly. 675pub use crate::x::Z; 676``` 677 678**Rationale:** consistency. 679Reading order is important for new contributors. 680Grouping by crate allows spotting unwanted dependencies easier. 681 682## Import Style 683 684Qualify items from `hir` and `ast`. 685 686```rust 687// GOOD 688use syntax::ast; 689 690fn frobnicate(func: hir::Function, strukt: ast::Struct) {} 691 692// BAD 693use hir::Function; 694use syntax::ast::Struct; 695 696fn frobnicate(func: Function, strukt: Struct) {} 697``` 698 699**Rationale:** avoids name clashes, makes the layer clear at a glance. 700 701When implementing traits from `std::fmt` or `std::ops`, import the module: 702 703```rust 704// GOOD 705use std::fmt; 706 707impl fmt::Display for RenameError { 708 fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { .. } 709} 710 711// BAD 712impl std::fmt::Display for RenameError { 713 fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { .. } 714} 715 716// BAD 717use std::ops::Deref; 718 719impl Deref for Widget { 720 type Target = str; 721 fn deref(&self) -> &str { .. } 722} 723``` 724 725**Rationale:** overall, less typing. 726Makes it clear that a trait is implemented, rather than used. 727 728Avoid local `use MyEnum::*` imports. 729**Rationale:** consistency. 730 731Prefer `use crate::foo::bar` to `use super::bar` or `use self::bar::baz`. 732**Rationale:** consistency, this is the style which works in all cases. 733 734By default, avoid re-exports. 735**Rationale:** for non-library code, re-exports introduce two ways to use something and allow for inconsistency. 736 737## Order of Items 738 739Optimize for the reader who sees the file for the first time, and wants to get a general idea about what's going on. 740People read things from top to bottom, so place most important things first. 741 742Specifically, if all items except one are private, always put the non-private item on top. 743 744```rust 745// GOOD 746pub(crate) fn frobnicate() { 747 Helper::act() 748} 749 750#[derive(Default)] 751struct Helper { stuff: i32 } 752 753impl Helper { 754 fn act(&self) { 755 756 } 757} 758 759// BAD 760#[derive(Default)] 761struct Helper { stuff: i32 } 762 763pub(crate) fn frobnicate() { 764 Helper::act() 765} 766 767impl Helper { 768 fn act(&self) { 769 770 } 771} 772``` 773 774If there's a mixture of private and public items, put public items first. 775 776Put `struct`s and `enum`s first, functions and impls last. Order type declarations in top-down manner. 777 778```rust 779// GOOD 780struct Parent { 781 children: Vec<Child> 782} 783 784struct Child; 785 786impl Parent { 787} 788 789impl Child { 790} 791 792// BAD 793struct Child; 794 795impl Child { 796} 797 798struct Parent { 799 children: Vec<Child> 800} 801 802impl Parent { 803} 804``` 805 806**Rationale:** easier to get the sense of the API by visually scanning the file. 807If function bodies are folded in the editor, the source code should read as documentation for the public API. 808 809## Context Parameters 810 811Some parameters are threaded unchanged through many function calls. 812They determine the "context" of the operation. 813Pass such parameters first, not last. 814If there are several context parameters, consider packing them into a `struct Ctx` and passing it as `&self`. 815 816```rust 817// GOOD 818fn dfs(graph: &Graph, v: Vertex) -> usize { 819 let mut visited = FxHashSet::default(); 820 return go(graph, &mut visited, v); 821 822 fn go(graph: &Graph, visited: &mut FxHashSet<Vertex>, v: usize) -> usize { 823 ... 824 } 825} 826 827// BAD 828fn dfs(v: Vertex, graph: &Graph) -> usize { 829 fn go(v: usize, graph: &Graph, visited: &mut FxHashSet<Vertex>) -> usize { 830 ... 831 } 832 833 let mut visited = FxHashSet::default(); 834 go(v, graph, &mut visited) 835} 836``` 837 838**Rationale:** consistency. 839Context-first works better when non-context parameter is a lambda. 840 841## Variable Naming 842 843Use boring and long names for local variables ([yay code completion](https://github.com/rust-analyzer/rust-analyzer/pull/4162#discussion_r417130973)). 844The default name is a lowercased name of the type: `global_state: GlobalState`. 845Avoid ad-hoc acronyms and contractions, but use the ones that exist consistently (`db`, `ctx`, `acc`). 846Prefer American spelling (color, behavior). 847 848Default names: 849 850* `res` -- "result of the function" local variable 851* `it` -- I don't really care about the name 852* `n_foos` -- number of foos (prefer this to `foo_count`) 853* `foo_idx` -- index of `foo` 854 855Many names in rust-analyzer conflict with keywords. 856We use mangled names instead of `r#ident` syntax: 857 858``` 859crate -> krate 860enum -> enum_ 861fn -> func 862impl -> imp 863macro -> mac 864mod -> module 865struct -> strukt 866trait -> trait_ 867type -> ty 868``` 869 870**Rationale:** consistency. 871 872## Early Returns 873 874Do use early returns 875 876```rust 877// GOOD 878fn foo() -> Option<Bar> { 879 if !condition() { 880 return None; 881 } 882 883 Some(...) 884} 885 886// BAD 887fn foo() -> Option<Bar> { 888 if condition() { 889 Some(...) 890 } else { 891 None 892 } 893} 894``` 895 896**Rationale:** reduce cognitive stack usage. 897 898Use `return Err(err)` to throw an error: 899 900```rust 901// GOOD 902fn f() -> Result<(), ()> { 903 if condition { 904 return Err(()); 905 } 906 Ok(()) 907} 908 909// BAD 910fn f() -> Result<(), ()> { 911 if condition { 912 Err(())?; 913 } 914 Ok(()) 915} 916``` 917 918**Rationale:** `return` has type `!`, which allows the compiler to flag dead 919code (`Err(...)?` is of unconstrained generic type `T`). 920 921## Comparisons 922 923When doing multiple comparisons use `<`/`<=`, avoid `>`/`>=`. 924 925```rust 926// GOOD 927assert!(lo <= x && x <= hi); 928assert!(r1 < l2 || r2 < l1); 929assert!(x < y); 930assert!(x > 0); 931 932// BAD 933assert!(x >= lo && x <= hi); 934assert!(r1 < l2 || l1 > r2); 935assert!(y > x); 936assert!(0 > x); 937``` 938 939**Rationale:** Less-then comparisons are more intuitive, they correspond spatially to [real line](https://en.wikipedia.org/wiki/Real_line). 940 941## If-let 942 943Avoid `if let ... { } else { }` construct, use `match` instead. 944 945```rust 946// GOOD 947match ctx.expected_type.as_ref() { 948 Some(expected_type) => completion_ty == expected_type && !expected_type.is_unit(), 949 None => false, 950} 951 952// BAD 953if let Some(expected_type) = ctx.expected_type.as_ref() { 954 completion_ty == expected_type && !expected_type.is_unit() 955} else { 956 false 957} 958``` 959 960**Rationale:** `match` is almost always more compact. 961The `else` branch can get a more precise pattern: `None` or `Err(_)` instead of `_`. 962 963## Match Ergonomics 964 965Don't use the `ref` keyword. 966 967**Rationale:** consistency & simplicity. 968`ref` was required before [match ergonomics](https://github.com/rust-lang/rfcs/blob/master/text/2005-match-ergonomics.md). 969Today, it is redundant. 970Between `ref` and mach ergonomics, the latter is more ergonomic in most cases, and is simpler (does not require a keyword). 971 972## Empty Match Arms 973 974Ues `=> (),` when a match arm is intentionally empty: 975 976```rust 977// GOOD 978match result { 979 Ok(_) => (), 980 Err(err) => error!("{}", err), 981} 982 983// BAD 984match result { 985 Ok(_) => {} 986 Err(err) => error!("{}", err), 987} 988``` 989 990**Rationale:** consistency. 991 992## Functional Combinators 993 994Use high order monadic combinators like `map`, `then` when they are a natural choice; don't bend the code to fit into some combinator. 995If writing a chain of combinators creates friction, replace them with control flow constructs: `for`, `if`, `match`. 996Mostly avoid `bool::then` and `Option::filter`. 997 998```rust 999// GOOD 1000if !x.cond() { 1001 return None; 1002} 1003Some(x) 1004 1005// BAD 1006Some(x).filter(|it| it.cond()) 1007``` 1008 1009This rule is more "soft" then others, and boils down mostly to taste. 1010The guiding principle behind this rule is that code should be dense in computation, and sparse in the number of expressions per line. 1011The second example contains *less* computation -- the `filter` function is an indirection for `if`, it doesn't do any useful work by itself. 1012At the same time, it is more crowded -- it takes more time to visually scan it. 1013 1014**Rationale:** consistency, playing to language's strengths. 1015Rust 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`). 1016 1017## Turbofish 1018 1019Prefer type ascription over the turbofish. 1020When ascribing types, avoid `_` 1021 1022```rust 1023// GOOD 1024let mutable: Vec<T> = old.into_iter().map(|it| builder.make_mut(it)).collect(); 1025 1026// BAD 1027let mutable: Vec<_> = old.into_iter().map(|it| builder.make_mut(it)).collect(); 1028 1029// BAD 1030let mutable = old.into_iter().map(|it| builder.make_mut(it)).collect::<Vec<_>>(); 1031``` 1032 1033**Rationale:** consistency, readability. 1034If compiler struggles to infer the type, the human would as well. 1035Having the result type specified up-front helps with understanding what the chain of iterator methods is doing. 1036 1037## Helper Functions 1038 1039Avoid creating singe-use helper functions: 1040 1041```rust 1042// GOOD 1043let buf = { 1044 let mut buf = get_empty_buf(&mut arena); 1045 buf.add_item(item); 1046 buf 1047}; 1048 1049// BAD 1050let buf = prepare_buf(&mut arena, item); 1051 1052... 1053 1054fn prepare_buf(arena: &mut Arena, item: Item) -> ItemBuf { 1055 let mut res = get_empty_buf(&mut arena); 1056 res.add_item(item); 1057 res 1058} 1059``` 1060 1061Exception: if you want to make use of `return` or `?`. 1062 1063**Rationale:** single-use functions change frequently, adding or removing parameters adds churn. 1064A block serves just as well to delineate a bit of logic, but has access to all the context. 1065Re-using originally single-purpose function often leads to bad coupling. 1066 1067## Local Helper Functions 1068 1069Put nested helper functions at the end of the enclosing functions 1070(this requires using return statement). 1071Don't nest more than one level deep. 1072 1073```rust 1074// GOOD 1075fn dfs(graph: &Graph, v: Vertex) -> usize { 1076 let mut visited = FxHashSet::default(); 1077 return go(graph, &mut visited, v); 1078 1079 fn go(graph: &Graph, visited: &mut FxHashSet<Vertex>, v: usize) -> usize { 1080 ... 1081 } 1082} 1083 1084// BAD 1085fn dfs(graph: &Graph, v: Vertex) -> usize { 1086 fn go(graph: &Graph, visited: &mut FxHashSet<Vertex>, v: usize) -> usize { 1087 ... 1088 } 1089 1090 let mut visited = FxHashSet::default(); 1091 go(graph, &mut visited, v) 1092} 1093``` 1094 1095**Rationale:** consistency, improved top-down readability. 1096 1097## Helper Variables 1098 1099Introduce helper variables freely, especially for multiline conditions: 1100 1101```rust 1102// GOOD 1103let rustfmt_not_installed = 1104 captured_stderr.contains("not installed") || captured_stderr.contains("not available"); 1105 1106match output.status.code() { 1107 Some(1) if !rustfmt_not_installed => Ok(None), 1108 _ => Err(format_err!("rustfmt failed:\n{}", captured_stderr)), 1109}; 1110 1111// BAD 1112match output.status.code() { 1113 Some(1) 1114 if !captured_stderr.contains("not installed") 1115 && !captured_stderr.contains("not available") => Ok(None), 1116 _ => Err(format_err!("rustfmt failed:\n{}", captured_stderr)), 1117}; 1118``` 1119 1120**Rationale:** Like blocks, single-use variables are a cognitively cheap abstraction, as they have access to all the context. 1121Extra variables help during debugging, they make it easy to print/view important intermediate results. 1122Giving a name to a condition inside an `if` expression often improves clarity and leads to nicely formatted code. 1123 1124## Token names 1125 1126Use `T![foo]` instead of `SyntaxKind::FOO_KW`. 1127 1128```rust 1129// GOOD 1130match p.current() { 1131 T![true] | T![false] => true, 1132 _ => false, 1133} 1134 1135// BAD 1136 1137match p.current() { 1138 SyntaxKind::TRUE_KW | SyntaxKind::FALSE_KW => true, 1139 _ => false, 1140} 1141``` 1142 1143**Rationale:** The macro uses the familiar Rust syntax, avoiding ambiguities like "is this a brace or bracket?". 1144 1145## Documentation 1146 1147Style inline code comments as proper sentences. 1148Start with a capital letter, end with a dot. 1149 1150```rust 1151// GOOD 1152 1153// Only simple single segment paths are allowed. 1154MergeBehavior::Last => { 1155 tree.use_tree_list().is_none() && tree.path().map(path_len) <= Some(1) 1156} 1157 1158// BAD 1159 1160// only simple single segment paths are allowed 1161MergeBehavior::Last => { 1162 tree.use_tree_list().is_none() && tree.path().map(path_len) <= Some(1) 1163} 1164``` 1165 1166**Rationale:** writing a sentence (or maybe even a paragraph) rather just "a comment" creates a more appropriate frame of mind. 1167It tricks you into writing down more of the context you keep in your head while coding. 1168 1169For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. 1170If the line is too long, you want to split the sentence in two :-) 1171 1172**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). 1173