From fdf2f6226b4ce58a2407d7d3fa3b700f6e76e60a Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Wed, 7 Oct 2020 12:50:46 +0200 Subject: Reorg style --- docs/dev/style.md | 357 +++++++++++++++++++++++++++--------------------------- 1 file changed, 181 insertions(+), 176 deletions(-) (limited to 'docs') diff --git a/docs/dev/style.md b/docs/dev/style.md index fb407afcd..17626f3fd 100644 --- a/docs/dev/style.md +++ b/docs/dev/style.md @@ -6,7 +6,9 @@ Our approach to "clean code" is two-fold: 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. -# Scale of Changes +# 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. @@ -45,13 +47,35 @@ That said, adding an innocent-looking `pub use` is a very simple way to break en 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 +## Crates.io Dependencies We try to be very conservative with usage of crates.io dependencies. 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. -# Minimal Tests +## 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. + +## 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 `cargo xtask lint` command which runs a subset of low-FPR lints. +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. + +# Code + +## Minimal Tests 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. @@ -65,119 +89,7 @@ There are many benefits to this: 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. -# Order of Imports - -Separate import groups with blank lines. -Use one `use` per crate. - -```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::{} -``` - -Module declarations come before the imports. -Order them in "suggested reading order" for a person new to the code base. - -# Import Style - -Qualify items from `hir` and `ast`. - -```rust -// Good -use syntax::ast; - -fn frobnicate(func: hir::Function, strukt: ast::StructDef) {} - -// Not as good -use hir::Function; -use syntax::ast::StructDef; - -fn frobnicate(func: Function, strukt: StructDef) {} -``` - -Avoid local `use MyEnum::*` imports. - -Prefer `use crate::foo::bar` to `use super::bar`. - -When implementing `Debug` or `Display`, import `std::fmt`: - -```rust -// Good -use std::fmt; - -impl fmt::Display for RenameError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { .. } -} - -// Not as good -impl std::fmt::Display for RenameError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { .. } -} -``` - -# 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. - -Put `struct`s and `enum`s first, functions and impls last. - -Do - -```rust -// Good -struct Foo { - bars: Vec -} - -struct Bar; -``` - -rather than - -```rust -// Not as good -struct Bar; - -struct Foo { - bars: Vec -} -``` - -# Variable Naming - -Use boring and long names for local variables ([yay code completion](https://github.com/rust-analyzer/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`). - -Default names: - -* `res` -- "result of the function" local variable -* `it` -- I don't really care about the name -* `n_foo` -- number of foos -* `foo_idx` -- index of `foo` - -# Collection types - -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. - -# Preconditions +## Preconditions Express function preconditions in types and force the caller to provide them (rather than checking in callee): @@ -199,7 +111,6 @@ fn frobnicate(walrus: Option) { Avoid preconditions that span across function boundaries: - ```rust // Good fn string_literal_contents(s: &str) -> Option<&str> { @@ -233,31 +144,7 @@ fn 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. -# Early Returns - -Do use early returns - -```rust -// Good -fn foo() -> Option { - if !condition() { - return None; - } - - Some(...) -} - -// Not as good -fn foo() -> Option { - if condition() { - Some(...) - } else { - None - } -} -``` - -# Getters & Setters +## 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. @@ -285,6 +172,40 @@ impl Person { } ``` +## 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*. +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 +// Good +fn frbonicate(f: impl FnMut()) { + frobnicate_impl(&mut f) +} +fn frobnicate_impl(f: &mut dyn FnMut()) { + // lots of code +} + +// Not as good +fn frbonicate(f: impl FnMut()) { + // lots of code +} +``` + +Avoid `AsRef` polymorphism, it pays back only for widely used libraries: + +```rust +// Good +fn frbonicate(f: &Path) { +} + +// Not as good +fn frbonicate(f: impl AsRef) { +} +``` # Premature Pessimization @@ -322,62 +243,146 @@ fn frobnicate(s: &str) { } ``` -# Avoid Monomorphization +## Collection types -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. -For this reason, avoid making a lot of code type parametric, *especially* on the boundaries between crates. +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. + +# Style + +## Order of Imports + +Separate import groups with blank lines. +Use one `use` per crate. + +```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::{} +``` + +Module declarations come before the imports. +Order them in "suggested reading order" for a person new to the code base. + +## Import Style + +Qualify items from `hir` and `ast`. ```rust // Good -fn frbonicate(f: impl FnMut()) { - frobnicate_impl(&mut f) -} -fn frobnicate_impl(f: &mut dyn FnMut()) { - // lots of code +use syntax::ast; + +fn frobnicate(func: hir::Function, strukt: ast::StructDef) {} + +// Not as good +use hir::Function; +use syntax::ast::StructDef; + +fn frobnicate(func: Function, strukt: StructDef) {} +``` + +Avoid local `use MyEnum::*` imports. + +Prefer `use crate::foo::bar` to `use super::bar`. + +When implementing `Debug` or `Display`, import `std::fmt`: + +```rust +// Good +use std::fmt; + +impl fmt::Display for RenameError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { .. } } // Not as good -fn frbonicate(f: impl FnMut()) { - // lots of code +impl std::fmt::Display for RenameError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { .. } } ``` -Avoid `AsRef` polymorphism, it pays back only for widely used libraries: +## 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. + +Put `struct`s and `enum`s first, functions and impls last. + +Do ```rust // Good -fn frbonicate(f: &Path) { +struct Foo { + bars: Vec } +struct Bar; +``` + +rather than + +```rust // Not as good -fn frbonicate(f: impl AsRef) { +struct Bar; + +struct Foo { + bars: Vec } ``` -# Documentation +## Variable Naming -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 :-) +Use boring and long names for local variables ([yay code completion](https://github.com/rust-analyzer/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`). -# Commit Style +Default names: -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. +* `res` -- "result of the function" local variable +* `it` -- I don't really care about the name +* `n_foo` -- number of foos +* `foo_idx` -- index of `foo` -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. -# Clippy +## Early Returns -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 `cargo xtask lint` command which runs a subset of low-FPR lints. -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. +Do use early returns + +```rust +// Good +fn foo() -> Option { + if !condition() { + return None; + } + + Some(...) +} + +// Not as good +fn foo() -> Option { + if condition() { + Some(...) + } else { + None + } +} +``` + +## Documentation + +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 :-) -- cgit v1.2.3 From 6976494781f810193535eebc73e5bda73ba9eddf Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Wed, 7 Oct 2020 12:57:49 +0200 Subject: Add comparisons guideline to style --- docs/dev/style.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) (limited to 'docs') diff --git a/docs/dev/style.md b/docs/dev/style.md index 17626f3fd..cc06d4122 100644 --- a/docs/dev/style.md +++ b/docs/dev/style.md @@ -144,6 +144,20 @@ fn 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. +When checking a boolean precondition, prefer `if !invariant` to `if negated_invariant`: + +```rust +// Good +if !(idx < len) { + return None; +} + +// Not as good +if idx >= len { + return None; +} +``` + ## Getters & Setters If a field can have any value without breaking invariants, make the field public. @@ -382,6 +396,19 @@ fn foo() -> Option { } ``` +## Comparisons + +Use `<`/`<=`, avoid `>`/`>=`. +Less-then comparisons are more intuitive, they correspond spatially to [real line](https://en.wikipedia.org/wiki/Real_line) + +```rs +// Good +assert!(lo <= x && x <= hi); + +// Not as good +assert!(x >= lo && x <= hi>); +``` + ## Documentation For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. -- cgit v1.2.3 From 1688e481b31e0f67fba72beee4079adb6b95f83c Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Wed, 7 Oct 2020 13:03:13 +0200 Subject: minor --- docs/dev/style.md | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) (limited to 'docs') diff --git a/docs/dev/style.md b/docs/dev/style.md index cc06d4122..7aed7816e 100644 --- a/docs/dev/style.md +++ b/docs/dev/style.md @@ -113,6 +113,13 @@ Avoid preconditions that span across function boundaries: ```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]) @@ -121,24 +128,17 @@ fn string_literal_contents(s: &str) -> Option<&str> { } } -fn foo() { +// Not as good +fn main() { let s: &str = ...; - if let Some(contents) = string_literal_contents(s) { - + if is_string_literal(s) { + let contents = &s[1..s.len() - 1]; } } -// Not as good fn is_string_literal(s: &str) -> bool { s.starts_with('"') && s.ends_with('"') } - -fn foo() { - let s: &str = ...; - if is_string_literal(s) { - let contents = &s[1..s.len() - 1]; - } -} ``` 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`. -- cgit v1.2.3