Mirrors/rust-analyzer
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-analyzer synced 2024-12-26 13:03:31 +00:00
Code Issues Projects Releases Packages Wiki Activity

Styleguide readability

Browse source
This commit is contained in:
Aleksey Kladov 2021-01-07 20:11:55 +03:00
parent eb710a63ca
commit 5aed769afe
1 changed files with 115 additions and 63 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

178
docs/dev/style.md
View file

@ -53,6 +53,9 @@ We try to be very conservative with usage of crates.io dependencies.
Don't use small "helper" crates (exception: `itertools` is allowed). Don't use small "helper" crates (exception: `itertools` is allowed).
If there's some general reusable bit of code you need, consider adding it to the `stdx` crate. If there's some general reusable bit of code you need, consider adding it to the `stdx` crate.
**Rational:** keep compile times low, create ecosystem pressure for faster
compiles, reduce the number of things which might break.
## Commit Style ## Commit Style
We don't have specific rules around git history hygiene. We don't have specific rules around git history hygiene.
@ -66,15 +69,18 @@ Such messages create a lot of duplicate notification traffic during rebases.
If possible, write commit messages from user's perspective: If possible, write commit messages from user's perspective:
``` ```
# Good # GOOD
Goto definition works inside macros Goto definition works inside macros
# Not as good # BAD
Use original span for FileId Use original span for FileId
``` ```
This makes it easier to prepare a changelog. This makes it easier to prepare a changelog.
**Rational:** clean history is potentially useful, but rarely used.
But many users read changelogs.
## Clippy ## Clippy
We don't enforce Clippy. We don't enforce Clippy.
@ -82,21 +88,16 @@ 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. Selectively patching false-positives with `allow(clippy)` is considered worse than not using Clippy at all.
There's `cargo xtask lint` command which runs a subset of low-FPR lints. There's `cargo xtask lint` command which runs a subset of low-FPR lints.
Careful tweaking of `xtask lint` is welcome. Careful tweaking of `xtask lint` is welcome.
See also [rust-lang/clippy#5537](https://github.com/rust-lang/rust-clippy/issues/5537).
Of course, applying Clippy suggestions is welcome as long as they indeed improve the code. Of course, applying Clippy suggestions is welcome as long as they indeed improve the code.
**Rational:** see [rust-lang/clippy#5537](https://github.com/rust-lang/rust-clippy/issues/5537).
# Code # Code
## Minimal Tests ## Minimal Tests
Most tests in rust-analyzer start with a snippet of Rust code. Most tests in rust-analyzer start with a snippet of Rust code.
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. 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.
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
It also makes sense to format snippets more compactly (for example, by placing enum definitions like `enum E { Foo, Bar }` on a single line), 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. as long as they are still readable.
@ -125,19 +126,28 @@ fn main() {
} }
``` ```
That way, you can use your editor's "number of selected characters" feature to correlate offsets with test's source code. **Rational:**
## Preconditions 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.
## Function Preconditions
Express function preconditions in types and force the caller to provide them (rather than checking in callee): Express function preconditions in types and force the caller to provide them (rather than checking in callee):
```rust ```rust
// Good // GOOD
fn frbonicate(walrus: Walrus) { fn frbonicate(walrus: Walrus) {
... ...
} }
// Not as good // BAD
fn frobnicate(walrus: Option<Walrus>) { fn frobnicate(walrus: Option<Walrus>) {
let walrus = match walrus { let walrus = match walrus {
Some(it) => it, Some(it) => it,
@ -147,10 +157,13 @@ fn frobnicate(walrus: Option<Walrus>) {
} }
``` ```
Avoid preconditions that span across function boundaries: **Rational:** 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 ```rust
// Good // GOOD
fn main() { fn main() {
let s: &str = ...; let s: &str = ...;
if let Some(contents) = string_literal_contents(s) { if let Some(contents) = string_literal_contents(s) {
@ -166,7 +179,7 @@ fn string_literal_contents(s: &str) -> Option<&str> {
} }
} }
// Not as good // BAD
fn main() { fn main() {
let s: &str = ...; let s: &str = ...;
if is_string_literal(s) { if is_string_literal(s) {
@ -182,20 +195,24 @@ fn is_string_literal(s: &str) -> bool {
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 "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. In the "Good" version, the precondition check and usage are checked in the same block, and then encoded in the types.
**Rational:** non-local code properties degrade under change.
When checking a boolean precondition, prefer `if !invariant` to `if negated_invariant`: When checking a boolean precondition, prefer `if !invariant` to `if negated_invariant`:
```rust ```rust
// Good // GOOD
if !(idx < len) { if !(idx < len) {
return None; return None;
} }
// Not as good // BAD
if idx >= len { if idx >= len {
return None; return None;
} }
``` ```
**Rational:** its useful to see the invariant relied upon by the rest of the function clearly spelled out.
## Getters & Setters ## Getters & Setters
If a field can have any value without breaking invariants, make the field public. If a field can have any value without breaking invariants, make the field public.
@ -211,31 +228,36 @@ struct Person {
middle_name: Option<String> middle_name: Option<String>
} }
// Good // GOOD
impl Person { impl Person {
fn first_name(&self) -> &str { self.first_name.as_str() } fn first_name(&self) -> &str { self.first_name.as_str() }
fn middle_name(&self) -> Option<&str> { self.middle_name.as_ref() } fn middle_name(&self) -> Option<&str> { self.middle_name.as_ref() }
} }
// Not as good // BAD
impl Person { impl Person {
fn first_name(&self) -> String { self.first_name.clone() } fn first_name(&self) -> String { self.first_name.clone() }
fn middle_name(&self) -> &Option<String> { &self.middle_name } fn middle_name(&self) -> &Option<String> { &self.middle_name }
} }
``` ```
**Rational:** 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 own data discloses irrelevant details about origin of data.
Irrelevant (neither right nor wrong) things obscure correctness.
## Constructors ## Constructors
Prefer `Default` to zero-argument `new` function Prefer `Default` to zero-argument `new` function
```rust ```rust
// Good // GOOD
#[derive(Default)] #[derive(Default)]
struct Foo { struct Foo {
bar: Option<Bar> bar: Option<Bar>
} }
// Not as good // BAD
struct Foo { struct Foo {
bar: Option<Bar> bar: Option<Bar>
} }
@ -249,16 +271,18 @@ impl Foo {
Prefer `Default` even it has to be implemented manually. Prefer `Default` even it has to be implemented manually.
**Rational:** less typing in the common case, uniformity.
## Functions Over Objects ## Functions Over Objects
Avoid creating "doer" objects. Avoid creating "doer" objects.
That is, objects which are created only to execute a single action. That is, objects which are created only to execute a single action.
```rust ```rust
// Good // GOOD
do_thing(arg1, arg2); do_thing(arg1, arg2);
// Not as good // BAD
ThingDoer::new(arg1, arg2).do(); ThingDoer::new(arg1, arg2).do();
``` ```
@ -303,16 +327,14 @@ impl ThingDoer {
} }
``` ```
**Rational:** not bothering the caller with irrelevant details, not mixing user API with implementor API.
## Avoid Monomorphization ## Avoid Monomorphization
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*. Avoid making a lot of code type parametric, *especially* on the boundaries between crates.
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.
For this reason, avoid making a lot of code type parametric, *especially* on the boundaries between crates.
```rust ```rust
// Good // GOOD
fn frbonicate(f: impl FnMut()) { fn frbonicate(f: impl FnMut()) {
frobnicate_impl(&mut f) frobnicate_impl(&mut f)
} }
@ -320,7 +342,7 @@ fn frobnicate_impl(f: &mut dyn FnMut()) {
// lots of code // lots of code
} }
// Not as good // BAD
fn frbonicate(f: impl FnMut()) { fn frbonicate(f: impl FnMut()) {
// lots of code // lots of code
} }
@ -329,15 +351,21 @@ fn frbonicate(f: impl FnMut()) {
Avoid `AsRef` polymorphism, it pays back only for widely used libraries: Avoid `AsRef` polymorphism, it pays back only for widely used libraries:
```rust ```rust
// Good // GOOD
fn frbonicate(f: &Path) { fn frbonicate(f: &Path) {
} }
// Not as good // BAD
fn frbonicate(f: impl AsRef<Path>) { fn frbonicate(f: impl AsRef<Path>) {
} }
``` ```
**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*.
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.
# Premature Pessimization # Premature Pessimization
## Avoid Allocations ## Avoid Allocations
@ -346,7 +374,7 @@ 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. Don't allocate a `Vec` where an iterator would do, don't allocate strings needlessly.
```rust ```rust
// Good // GOOD
use itertools::Itertools; use itertools::Itertools;
let (first_word, second_word) = match text.split_ascii_whitespace().collect_tuple() { let (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
None => return, None => return,
} }
// Not as good // BAD
let words = text.split_ascii_whitespace().collect::<Vec<_>>(); let words = text.split_ascii_whitespace().collect::<Vec<_>>();
if words.len() != 2 { if words.len() != 2 {
return return
} }
``` ```
**Rational:** not allocating is almost often faster.
## Push Allocations to the Call Site ## Push Allocations to the Call Site
If allocation is inevitable, let the caller allocate the resource: If allocation is inevitable, let the caller allocate the resource:
```rust ```rust
// Good // GOOD
fn frobnicate(s: String) { fn frobnicate(s: String) {
... ...
} }
// Not as good // BAD
fn frobnicate(s: &str) { fn frobnicate(s: &str) {
let s = s.to_string(); let s = s.to_string();
... ...
} }
``` ```
This is better because it reveals the costs. **Rational:** reveals the costs.
It is also more efficient when the caller already owns the allocation. It is also more efficient when the caller already owns the allocation.
## Collection types ## Collection types
Prefer `rustc_hash::FxHashMap` and `rustc_hash::FxHashSet` instead of the ones in `std::collections`. Prefer `rustc_hash::FxHashMap` and `rustc_hash::FxHashSet` instead of the ones in `std::collections`.
They use a hasher that's slightly faster and using them consistently will reduce code size by some small amount.
**Rational:** they use a hasher that's significantly faster and using them consistently will reduce code size by some small amount.
# Style # Style
@ -393,6 +424,9 @@ They use a hasher that's slightly faster and using them consistently will reduce
Separate import groups with blank lines. Separate import groups with blank lines.
Use one `use` per crate. 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 ```rust
mod x; mod x;
mod y; mod y;
@ -411,46 +445,45 @@ use crate::{}
use super::{} use super::{}
``` ```
Module declarations come before the imports. **Rational:** consistency.
Order them in "suggested reading order" for a person new to the code base. Reading order is important for new contributors.
Grouping by crate allows to spot unwanted dependencies easier.
## Import Style ## Import Style
Qualify items from `hir` and `ast`. Qualify items from `hir` and `ast`.
```rust ```rust
// Good // GOOD
use syntax::ast; use syntax::ast;
fn frobnicate(func: hir::Function, strukt: ast::StructDef) {} fn frobnicate(func: hir::Function, strukt: ast::Struct) {}
// Not as good // BAD
use hir::Function; use hir::Function;
use syntax::ast::StructDef; use syntax::ast::Struct;
fn frobnicate(func: Function, strukt: StructDef) {} fn frobnicate(func: Function, strukt: Struct) {}
``` ```
Avoid local `use MyEnum::*` imports. **Rational:** avoids name clashes, makes the layer clear at a glance.
Prefer `use crate::foo::bar` to `use super::bar`.
When implementing traits from `std::fmt` or `std::ops`, import the module: When implementing traits from `std::fmt` or `std::ops`, import the module:
```rust ```rust
// Good // GOOD
use std::fmt; use std::fmt;
impl fmt::Display for RenameError { impl fmt::Display for RenameError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { .. } fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { .. }
} }
// Not as good // BAD
impl std::fmt::Display for RenameError { impl std::fmt::Display for RenameError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { .. } fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { .. }
} }
// Not as good // BAD
use std::ops::Deref; use std::ops::Deref;
impl Deref for Widget { impl Deref for Widget {
@ -459,6 +492,15 @@ impl Deref for Widget {
} }
``` ```
**Rational:** overall, less typing.
Makes it clear that a trait is implemented, rather than used.
Avoid local `use MyEnum::*` imports.
**Rational:** consistency.
Prefer `use crate::foo::bar` to `use super::bar` or `use self::bar::baz`.
**Rational:** consistency, this is the style which works in all cases.
## Order of Items ## 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. Optimize 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.
Specifically, if all items except one are private, always put the non-private item on top. Specifically, if all items except one are private, always put the non-private item on top.
```rust ```rust
// Good // GOOD
pub(crate) fn frobnicate() { pub(crate) fn frobnicate() {
Helper::act() Helper::act()
} }
@ -481,7 +523,7 @@ impl Helper {
} }
} }
// Not as good // BAD
#[derive(Default)] #[derive(Default)]
struct Helper { stuff: i32 } struct Helper { stuff: i32 }
@ -497,12 +539,11 @@ impl Helper {
``` ```
If there's a mixture of private and public items, put public items first. If there's a mixture of private and public items, put public items first.
If function bodies are folded in the editor, the source code should read as documentation for the public API.
Put `struct`s and `enum`s first, functions and impls last. Order types declarations in top-down manner. Put `struct`s and `enum`s first, functions and impls last. Order type declarations in top-down manner.
```rust ```rust
// Good // GOOD
struct Parent { struct Parent {
children: Vec<Child> children: Vec<Child>
} }
@ -515,7 +556,7 @@ impl Parent {
impl Child { impl Child {
} }
// Not as good // BAD
struct Child; struct Child;
impl Child { impl Child {
@ -529,6 +570,9 @@ impl Parent {
} }
``` ```
**Rational:** 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.
## Variable Naming ## Variable Naming
Use boring and long names for local variables ([yay code completion](https://github.com/rust-analyzer/rust-analyzer/pull/4162#discussion_r417130973)). Use 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_
mod -> module mod -> module
``` ```
**Rationale:** consistency.
## Early Returns ## Early Returns
Do use early returns Do use early returns
```rust ```rust
// Good // GOOD
fn foo() -> Option<Bar> { fn foo() -> Option<Bar> {
if !condition() { if !condition() {
return None; return None;
@ -570,7 +616,7 @@ fn foo() -> Option<Bar> {
Some(...) Some(...)
} }
// Not as good // BAD
fn foo() -> Option<Bar> { fn foo() -> Option<Bar> {
if condition() { if condition() {
Some(...) Some(...)
@ -580,20 +626,26 @@ fn foo() -> Option<Bar> {
} }
``` ```
**Rational:** reduce congnitive stack usage.
## Comparisons ## Comparisons
Use `<`/`<=`, avoid `>`/`>=`. Use `<`/`<=`, avoid `>`/`>=`.
Less-then comparisons are more intuitive, they correspond spatially to [real line](https://en.wikipedia.org/wiki/Real_line)
```rust ```rust
// Good // GOOD
assert!(lo <= x && x <= hi); assert!(lo <= x && x <= hi);
// Not as good // BAD
assert!(x >= lo && x <= hi>); assert!(x >= lo && x <= hi>);
``` ```
**Rational:** Less-then comparisons are more intuitive, they correspond spatially to [real line](https://en.wikipedia.org/wiki/Real_line).
## Documentation ## Documentation
For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. 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 :-) If the line is too long, you want to split the sentence in two :-)
**Rational:** much easier to edit the text and read the diff.