1 files changed, 143 insertions, 11 deletions

diff --git a/docs/dev/style.md b/docs/dev/style.md
index 21330948b..dd71e3932 100644
--- a/docs/dev/style.md
+++ b/docs/dev/style.md
@@ -6,6 +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.
 
+When reviewing pull requests prefer extending this document to leaving
+non-reusable comments on the pull request itself.
+
 # General
 
 ## Scale of Changes
@@ -38,7 +41,7 @@ For the second group, the change would be subjected to quite a bit of scrutiny a
 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 realise that you need to elevate to a change 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.
@@ -99,7 +102,7 @@ Of course, applying Clippy suggestions is welcome as long as they indeed improve
 ## 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.
+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.
@@ -139,13 +142,24 @@ There are many benefits to this:
 
 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
+[`mark::hit! / mark::check!`](https://github.com/rust-analyzer/rust-analyzer/blob/71fe719dd5247ed8615641d9303d7ca1aa201c2f/crates/test_utils/src/mark.rs)
+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.
+
 ## Function Preconditions
 
 Express function preconditions in types and force the caller to provide them (rather than checking in callee):
 
 ```rust
 // GOOD
-fn frbonicate(walrus: Walrus) {
+fn frobnicate(walrus: Walrus) {
     ...
 }
 
@@ -213,12 +227,12 @@ if idx >= len {
 }
 ```
 
-**Rationale:** its useful to see the invariant relied upon by the rest of the function clearly spelled out.
+**Rationale:** it's useful to see the invariant relied upon by the rest of the function clearly spelled out.
 
 ## Assertions
 
 Assert liberally.
-Prefer `stdx::assert_never!` to standard `assert!`.
+Prefer `stdx::never!` to standard `assert!`.
 
 ## Getters & Setters
 
@@ -253,6 +267,20 @@ 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.
 
+## Useless Types
+
+More generally, always prefer types on the left
+
+```rust
+// GOOD      BAD
+&[T]         &Vec<T>
+&str         &String
+Option<&T>   &Option<T>
+```
+
+**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
@@ -280,6 +308,10 @@ Prefer `Default` even it has to be implemented manually.
 
 **Rationale:** less typing in the common case, uniformity.
 
+Use `Vec::new` rather than `vec![]`.
+
+**Rationale:** uniformity, strength reduction.
+
 ## Functions Over Objects
 
 Avoid creating "doer" objects.
@@ -336,13 +368,73 @@ impl ThingDoer {
 
 **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> { ... }
+```
+
 ## Avoid Monomorphization
 
 Avoid making a lot of code type parametric, *especially* on the boundaries between crates.
 
 ```rust
 // GOOD
-fn frbonicate(f: impl FnMut()) {
+fn frobnicate(f: impl FnMut()) {
     frobnicate_impl(&mut f)
 }
 fn frobnicate_impl(f: &mut dyn FnMut()) {
@@ -350,7 +442,7 @@ fn frobnicate_impl(f: &mut dyn FnMut()) {
 }
 
 // BAD
-fn frbonicate(f: impl FnMut()) {
+fn frobnicate(f: impl FnMut()) {
     // lots of code
 }
 ```
@@ -359,11 +451,11 @@ Avoid `AsRef` polymorphism, it pays back only for widely used libraries:
 
 ```rust
 // GOOD
-fn frbonicate(f: &Path) {
+fn frobnicate(f: &Path) {
 }
 
 // BAD
-fn frbonicate(f: impl AsRef<Path>) {
+fn frobnicate(f: impl AsRef<Path>) {
 }
 ```
 
@@ -372,6 +464,14 @@ This allows for exceptionally good performance, but leads to increased compile t
 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.
 
+## 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
 
@@ -418,12 +518,44 @@ fn frobnicate(s: &str) {
 **Rationale:** reveals the costs.
 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`.
 
 **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.
+
 # Style
 
 ## Order of Imports
@@ -633,7 +765,7 @@ fn foo() -> Option<Bar> {
 }
 ```
 
-**Rationale:** reduce congnitive stack usage.
+**Rationale:** reduce cognitive stack usage.
 
 ## Comparisons