aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--docs/dev/README.md320
-rw-r--r--docs/dev/style.md211
2 files changed, 263 insertions, 268 deletions
diff --git a/docs/dev/README.md b/docs/dev/README.md
index 2896d333e..18c53d5c0 100644
--- a/docs/dev/README.md
+++ b/docs/dev/README.md
@@ -50,277 +50,85 @@ We use bors-ng to enforce the [not rocket science](https://graydon2.dreamwidth.o
50 50
51You can run `cargo xtask install-pre-commit-hook` to install git-hook to run rustfmt on commit. 51You can run `cargo xtask install-pre-commit-hook` to install git-hook to run rustfmt on commit.
52 52
53# Code organization
54
55All Rust code lives in the `crates` top-level directory, and is organized as a
56single Cargo workspace. The `editors` top-level directory contains code for
57integrating with editors. Currently, it contains the plugin for VS Code (in
58TypeScript). The `docs` top-level directory contains both developer and user
59documentation.
60
61We have some automation infra in Rust in the `xtask` package. It contains
62stuff like formatting checking, code generation and powers `cargo xtask install`.
63The latter syntax is achieved with the help of cargo aliases (see `.cargo`
64directory).
65
66# Launching rust-analyzer 53# Launching rust-analyzer
67 54
68Debugging the language server can be tricky: LSP is rather chatty, so driving it 55Debugging the language server can be tricky.
69from the command line is not really feasible, driving it via VS Code requires 56LSP is rather chatty, so driving it from the command line is not really feasible, driving it via VS Code requires interacting with two processes.
70interacting with two processes.
71 57
72For this reason, the best way to see how rust-analyzer works is to find a 58For this reason, the best way to see how rust-analyzer works is to find a relevant test and execute it.
73relevant test and execute it (VS Code includes an action for running a single 59VS Code & Emacs include an action for running a single test.
74test).
75 60
76However, launching a VS Code instance with a locally built language server is 61Launching a VS Code instance with a locally built language server is also possible.
77possible. There's **"Run Extension (Debug Build)"** launch configuration for this. 62There's **"Run Extension (Debug Build)"** launch configuration for this in VS Code.
78 63
79In general, I use one of the following workflows for fixing bugs and 64In general, I use one of the following workflows for fixing bugs and implementing features:
80implementing features.
81 65
82If the problem concerns only internal parts of rust-analyzer (i.e. I don't need 66If the problem concerns only internal parts of rust-analyzer (i.e. I don't need to touch the `rust-analyzer` crate or TypeScript code), there is a unit-test for it.
83to touch the `rust-analyzer` crate or TypeScript code), there is a unit-test for it. 67So, I use **Rust Analyzer: Run** action in VS Code to run this single test, and then just do printf-driven development/debugging.
84So, I use **Rust Analyzer: Run** action in VS Code to run this single test, and 68As a sanity check after I'm done, I use `cargo xtask install --server` and **Reload Window** action in VS Code to verify that the thing works as I expect.
85then just do printf-driven development/debugging. As a sanity check after I'm
86done, I use `cargo xtask install --server` and **Reload Window** action in VS
87Code to sanity check that the thing works as I expect.
88 69
89If the problem concerns only the VS Code extension, I use **Run Installed Extension** 70If the problem concerns only the VS Code extension, I use **Run Installed Extension** launch configuration from `launch.json`.
90launch configuration from `launch.json`. Notably, this uses the usual 71Notably, this uses the usual `rust-analyzer` binary from `PATH`.
91`rust-analyzer` binary from `PATH`. For this, it is important to have the following 72For this, it is important to have the following in your `settings.json` file:
92in your `settings.json` file:
93```json 73```json
94{ 74{
95 "rust-analyzer.serverPath": "rust-analyzer" 75 "rust-analyzer.serverPath": "rust-analyzer"
96} 76}
97``` 77```
98After I am done with the fix, I use `cargo 78After I am done with the fix, I use `cargo xtask install --client-code` to try the new extension for real.
99xtask install --client-code` to try the new extension for real.
100
101If I need to fix something in the `rust-analyzer` crate, I feel sad because it's
102on the boundary between the two processes, and working there is slow. I usually
103just `cargo xtask install --server` and poke changes from my live environment.
104Note that this uses `--release`, which is usually faster overall, because
105loading stdlib into debug version of rust-analyzer takes a lot of time. To speed
106things up, sometimes I open a temporary hello-world project which has
107`"rust-analyzer.withSysroot": false` in `.code/settings.json`. This flag causes
108rust-analyzer to skip loading the sysroot, which greatly reduces the amount of
109things rust-analyzer needs to do, and makes printf's more useful. Note that you
110should only use the `eprint!` family of macros for debugging: stdout is used for LSP
111communication, and `print!` would break it.
112
113If I need to fix something simultaneously in the server and in the client, I
114feel even more sad. I don't have a specific workflow for this case.
115
116Additionally, I use `cargo run --release -p rust-analyzer -- analysis-stats
117path/to/some/rust/crate` to run a batch analysis. This is primarily useful for
118performance optimizations, or for bug minimization.
119
120# Code Style & Review Process
121
122Our approach to "clean code" is two-fold:
123
124* We generally don't block PRs on style changes.
125* At the same time, all code in rust-analyzer is constantly refactored.
126
127It 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.
128Sending small cleanup PRs (like renaming a single local variable) is encouraged.
129
130## Scale of Changes
131
132Everyone knows that it's better to send small & focused pull requests.
133The problem is, sometimes you *have* to, eg, rewrite the whole compiler, and that just doesn't fit into a set of isolated PRs.
134
135The main things to keep an eye on are the boundaries between various components.
136There are three kinds of changes:
137
1381. Internals of a single component are changed.
139 Specifically, you don't change any `pub` items.
140 A good example here would be an addition of a new assist.
141
1422. API of a component is expanded.
143 Specifically, you add a new `pub` function which wasn't there before.
144 A good example here would be expansion of assist API, for example, to implement lazy assists or assists groups.
145
1463. A new dependency between components is introduced.
147 Specifically, you add a `pub use` reexport from another crate or you add a new line to the `[dependencies]` section of `Cargo.toml`.
148 A good example here would be adding reference search capability to the assists crates.
149
150For the first group, the change is generally merged as long as:
151
152* it works for the happy case,
153* it has tests,
154* it doesn't panic for the unhappy case.
155
156For the second group, the change would be subjected to quite a bit of scrutiny and iteration.
157The new API needs to be right (or at least easy to change later).
158The actual implementation doesn't matter that much.
159It's very important to minimize the amount of changed lines of code for changes of the second kind.
160Often, you start doing a change of the first kind, only to realise that you need to elevate to a change of the second kind.
161In this case, we'll probably ask you to split API changes into a separate PR.
162
163Changes of the third group should be pretty rare, so we don't specify any specific process for them.
164That said, adding an innocent-looking `pub use` is a very simple way to break encapsulation, keep an eye on it!
165
166Note: if you enjoyed this abstract hand-waving about boundaries, you might appreciate
167https://www.tedinski.com/2018/02/06/system-boundaries.html
168
169## Crates.io Dependencies
170
171We try to be very conservative with usage of crates.io dependencies.
172Don't use small "helper" crates (exception: `itertools` is allowed).
173If there's some general reusable bit of code you need, consider adding it to the `stdx` crate.
174
175## Minimal Tests
176
177Most tests in rust-analyzer start with a snippet of Rust code.
178This 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.
179There are many benefits to this:
180
181* less to read or to scroll past
182* easier to understand what exactly is tested
183* less stuff printed during printf-debugging
184* less time to run test
185
186It also makes sense to format snippets more compactly (for example, by placing enum defitions like `enum E { Foo, Bar }` on a single line),
187as long as they are still readable.
188
189## Order of Imports
190
191We separate import groups with blank lines
192 79
193```rust 80If I need to fix something in the `rust-analyzer` crate, I feel sad because it's on the boundary between the two processes, and working there is slow.
194mod x; 81I usually just `cargo xtask install --server` and poke changes from my live environment.
195mod y; 82Note that this uses `--release`, which is usually faster overall, because loading stdlib into debug version of rust-analyzer takes a lot of time.
83To speed things up, sometimes I open a temporary hello-world project which has `"rust-analyzer.withSysroot": false` in `.code/settings.json`.
84This flag causes rust-analyzer to skip loading the sysroot, which greatly reduces the amount of things rust-analyzer needs to do, and makes printf's more useful.
85Note that you should only use the `eprint!` family of macros for debugging: stdout is used for LSP communication, and `print!` would break it.
196 86
197use std::{ ... } 87If I need to fix something simultaneously in the server and in the client, I feel even more sad.
198 88I don't have a specific workflow for this case.
199use crate_foo::{ ... }
200use crate_bar::{ ... }
201
202use crate::{}
203
204use super::{} // but prefer `use crate::`
205```
206
207## Import Style
208
209Items from `hir` and `ast` should be used qualified:
210
211```rust
212// Good
213use ra_syntax::ast;
214
215fn frobnicate(func: hir::Function, strukt: ast::StructDef) {}
216
217// Not as good
218use hir::Function;
219use ra_syntax::ast::StructDef;
220
221fn frobnicate(func: Function, strukt: StructDef) {}
222```
223
224Avoid local `use MyEnum::*` imports.
225
226Prefer `use crate::foo::bar` to `use super::bar`.
227
228## Order of Items
229
230Optimize for the reader who sees the file for the first time, and wants to get the general idea about what's going on.
231People read things from top to bottom, so place most important things first.
232
233Specifically, if all items except one are private, always put the non-private item on top.
234
235Put `struct`s and `enum`s first, functions and impls last.
236
237Do
238
239```rust
240// Good
241struct Foo {
242 bars: Vec<Bar>
243}
244
245struct Bar;
246```
247
248rather than
249
250```rust
251// Not as good
252struct Bar;
253
254struct Foo {
255 bars: Vec<Bar>
256}
257```
258 89
259## Variable Naming 90Additionally, I use `cargo run --release -p rust-analyzer -- analysis-stats path/to/some/rust/crate` to run a batch analysis.
91This is primarily useful for performance optimizations, or for bug minimization.
260 92
261We generally use boring and long names for local variables ([yay code completion](https://github.com/rust-analyzer/rust-analyzer/pull/4162#discussion_r417130973)). 93## Parser Tests
262The default name is a lowercased name of the type: `global_state: GlobalState`.
263Avoid ad-hoc acronyms and contractions, but use the ones that exist consistently (`db`, `ctx`, `acc`).
264The default name for "result of the function" local variable is `res`.
265
266## Collection types
267 94
268We prefer `rustc_hash::FxHashMap` and `rustc_hash::FxHashSet` instead of the ones in `std::collections`. 95Tests for the parser (`ra_parser`) live in the `ra_syntax` crate (see `test_data` directory).
269They use a hasher that's slightly faster and using them consistently will reduce code size by some small amount. 96There are two kinds of tests:
270 97
271## Preconditions 98* Manually written test cases in `parser/ok` and `parser/err`
99* "Inline" tests in `parser/inline` (these are generated) from comments in `ra_parser` crate.
272 100
273Function preconditions should generally be expressed in types and provided by the caller (rather than checked by callee): 101The purpose of inline tests is not to achieve full coverage by test cases, but to explain to the reader of the code what each particular `if` and `match` is responsible for.
102If you are tempted to add a large inline test, it might be a good idea to leave only the simplest example in place, and move the test to a manual `parser/ok` test.
274 103
275```rust 104To update test data, run with `UPDATE_EXPECT` variable:
276// Good
277fn frbonicate(walrus: Walrus) {
278 ...
279}
280 105
281// Not as good 106```bash
282fn frobnicate(walrus: Option<Walrus>) { 107env UPDATE_EXPECT=1 cargo qt
283 let walrus = match walrus {
284 Some(it) => it,
285 None => return,
286 };
287 ...
288}
289``` 108```
290 109
291## Premature Pessimization 110After adding a new inline test you need to run `cargo xtest codegen` and also update the test data as described above.
292
293While we don't specifically optimize code yet, avoid writing code which is slower than it needs to be.
294Don't allocate a `Vec` where an iterator would do, don't allocate strings needlessly.
295 111
296```rust 112## TypeScript Tests
297// Good
298use itertools::Itertools;
299 113
300let (first_word, second_word) = match text.split_ascii_whitespace().collect_tuple() { 114If you change files under `editors/code` and would like to run the tests and linter, install npm and run:
301 Some(it) => it,
302 None => return,
303}
304 115
305// Not as good 116```bash
306let words = text.split_ascii_whitespace().collect::<Vec<_>>(); 117cd editors/code
307if words.len() != 2 { 118npm ci
308 return 119npm run lint
309}
310``` 120```
311 121
312## Documentation 122# Code organization
313
314For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines.
315If the line is too long, you want to split the sentence in two :-)
316
317## Commit Style
318 123
319We don't have specific rules around git history hygiene. 124All Rust code lives in the `crates` top-level directory, and is organized as a single Cargo workspace.
320Maintaining clean git history is encouraged, but not enforced. 125The `editors` top-level directory contains code for integrating with editors.
321We use rebase workflow, it's OK to rewrite history during PR review process. 126Currently, it contains the plugin for VS Code (in TypeScript).
127The `docs` top-level directory contains both developer and user documentation.
322 128
323Avoid @mentioning people in commit messages and pull request descriptions (they are added to commit message by bors), as such messages create a lot of duplicate notification traffic during rebases. 129We have some automation infra in Rust in the `xtask` package.
130It contains stuff like formatting checking, code generation and powers `cargo xtask install`.
131The latter syntax is achieved with the help of cargo aliases (see `.cargo` directory).
324 132
325# Architecture Invariants 133# Architecture Invariants
326 134
@@ -355,35 +163,11 @@ The main IDE crate (`ra_ide`) uses "Plain Old Data" for the API.
355Rather than talking in definitions and references, it talks in Strings and textual offsets. 163Rather than talking in definitions and references, it talks in Strings and textual offsets.
356In general, API is centered around UI concerns -- the result of the call is what the user sees in the editor, and not what the compiler sees underneath. 164In general, API is centered around UI concerns -- the result of the call is what the user sees in the editor, and not what the compiler sees underneath.
357The results are 100% Rust specific though. 165The results are 100% Rust specific though.
166Shout outs to LSP developers for popularizing the idea that "UI" is a good place to draw a boundary at.
358 167
359## Parser Tests 168# Code Style & Review Process
360
361Tests for the parser (`ra_parser`) live in the `ra_syntax` crate (see `test_data` directory).
362There are two kinds of tests:
363
364* Manually written test cases in `parser/ok` and `parser/err`
365* "Inline" tests in `parser/inline` (these are generated) from comments in `ra_parser` crate.
366
367The purpose of inline tests is not to achieve full coverage by test cases, but to explain to the reader of the code what each particular `if` and `match` is responsible for.
368If you are tempted to add a large inline test, it might be a good idea to leave only the simplest example in place, and move the test to a manual `parser/ok` test.
369
370To update test data, run with `UPDATE_EXPECT` variable:
371
372```bash
373env UPDATE_EXPECT=1 cargo qt
374```
375
376After adding a new inline test you need to run `cargo xtest codegen` and also update the test data as described above.
377
378## TypeScript Tests
379
380If you change files under `editors/code` and would like to run the tests and linter, install npm and run:
381 169
382```bash 170Do see [./style.md](./style.md).
383cd editors/code
384npm ci
385npm run lint
386```
387 171
388# Logging 172# Logging
389 173
diff --git a/docs/dev/style.md b/docs/dev/style.md
new file mode 100644
index 000000000..0a85b4a55
--- /dev/null
+++ b/docs/dev/style.md
@@ -0,0 +1,211 @@
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
9# Scale of Changes
10
11Everyone knows that it's better to send small & focused pull requests.
12The problem is, sometimes you *have* to, eg, rewrite the whole compiler, and that just doesn't fit into a set of isolated PRs.
13
14The main things to keep an eye on are the boundaries between various components.
15There are three kinds of changes:
16
171. Internals of a single component are changed.
18 Specifically, you don't change any `pub` items.
19 A good example here would be an addition of a new assist.
20
212. API of a component is expanded.
22 Specifically, you add a new `pub` function which wasn't there before.
23 A good example here would be expansion of assist API, for example, to implement lazy assists or assists groups.
24
253. A new dependency between components is introduced.
26 Specifically, you add a `pub use` reexport from another crate or you add a new line to the `[dependencies]` section of `Cargo.toml`.
27 A good example here would be adding reference search capability to the assists crates.
28
29For the first group, the change is generally merged as long as:
30
31* it works for the happy case,
32* it has tests,
33* it doesn't panic for the unhappy case.
34
35For the second group, the change would be subjected to quite a bit of scrutiny and iteration.
36The new API needs to be right (or at least easy to change later).
37The actual implementation doesn't matter that much.
38It's very important to minimize the amount of changed lines of code for changes of the second kind.
39Often, you start doing a change of the first kind, only to realise that you need to elevate to a change of the second kind.
40In this case, we'll probably ask you to split API changes into a separate PR.
41
42Changes of the third group should be pretty rare, so we don't specify any specific process for them.
43That said, adding an innocent-looking `pub use` is a very simple way to break encapsulation, keep an eye on it!
44
45Note: if you enjoyed this abstract hand-waving about boundaries, you might appreciate
46https://www.tedinski.com/2018/02/06/system-boundaries.html
47
48# Crates.io Dependencies
49
50We try to be very conservative with usage of crates.io dependencies.
51Don't use small "helper" crates (exception: `itertools` is allowed).
52If there's some general reusable bit of code you need, consider adding it to the `stdx` crate.
53
54# Minimal Tests
55
56Most tests in rust-analyzer start with a snippet of Rust code.
57This 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.
58There are many benefits to this:
59
60* less to read or to scroll past
61* easier to understand what exactly is tested
62* less stuff printed during printf-debugging
63* less time to run test
64
65It also makes sense to format snippets more compactly (for example, by placing enum definitions like `enum E { Foo, Bar }` on a single line),
66as long as they are still readable.
67
68## Order of Imports
69
70We separate import groups with blank lines
71
72```rust
73mod x;
74mod y;
75
76// First std.
77use std::{ ... }
78
79// Second, external crates (both crates.io crates and other rust-analyzer crates).
80use crate_foo::{ ... }
81use crate_bar::{ ... }
82
83// Then current crate.
84use crate::{}
85
86// Finally, parent and child modules, but prefer `use crate::`.
87use super::{}
88```
89
90Module declarations come before the imports.
91Order them in "suggested reading order" for a person new to the code base.
92
93## Import Style
94
95Items from `hir` and `ast` should be used qualified:
96
97```rust
98// Good
99use ra_syntax::ast;
100
101fn frobnicate(func: hir::Function, strukt: ast::StructDef) {}
102
103// Not as good
104use hir::Function;
105use ra_syntax::ast::StructDef;
106
107fn frobnicate(func: Function, strukt: StructDef) {}
108```
109
110Avoid local `use MyEnum::*` imports.
111
112Prefer `use crate::foo::bar` to `use super::bar`.
113
114## Order of Items
115
116Optimize for the reader who sees the file for the first time, and wants to get a general idea about what's going on.
117People read things from top to bottom, so place most important things first.
118
119Specifically, if all items except one are private, always put the non-private item on top.
120
121Put `struct`s and `enum`s first, functions and impls last.
122
123Do
124
125```rust
126// Good
127struct Foo {
128 bars: Vec<Bar>
129}
130
131struct Bar;
132```
133
134rather than
135
136```rust
137// Not as good
138struct Bar;
139
140struct Foo {
141 bars: Vec<Bar>
142}
143```
144
145## Variable Naming
146
147We generally use boring and long names for local variables ([yay code completion](https://github.com/rust-analyzer/rust-analyzer/pull/4162#discussion_r417130973)).
148The default name is a lowercased name of the type: `global_state: GlobalState`.
149Avoid ad-hoc acronyms and contractions, but use the ones that exist consistently (`db`, `ctx`, `acc`).
150The default name for "result of the function" local variable is `res`.
151The default name for "I don't really care about the name" variable is `it`.
152
153## Collection types
154
155We prefer `rustc_hash::FxHashMap` and `rustc_hash::FxHashSet` instead of the ones in `std::collections`.
156They use a hasher that's slightly faster and using them consistently will reduce code size by some small amount.
157
158## Preconditions
159
160Function preconditions should generally be expressed in types and provided by the caller (rather than checked by callee):
161
162```rust
163// Good
164fn frbonicate(walrus: Walrus) {
165 ...
166}
167
168// Not as good
169fn frobnicate(walrus: Option<Walrus>) {
170 let walrus = match walrus {
171 Some(it) => it,
172 None => return,
173 };
174 ...
175}
176```
177
178## Premature Pessimization
179
180Avoid writing code which is slower than it needs to be.
181Don't allocate a `Vec` where an iterator would do, don't allocate strings needlessly.
182
183```rust
184// Good
185use itertools::Itertools;
186
187let (first_word, second_word) = match text.split_ascii_whitespace().collect_tuple() {
188 Some(it) => it,
189 None => return,
190}
191
192// Not as good
193let words = text.split_ascii_whitespace().collect::<Vec<_>>();
194if words.len() != 2 {
195 return
196}
197```
198
199## Documentation
200
201For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines.
202If the line is too long, you want to split the sentence in two :-)
203
204## Commit Style
205
206We don't have specific rules around git history hygiene.
207Maintaining clean git history is encouraged, but not enforced.
208We use rebase workflow, it's OK to rewrite history during PR review process.
209
210Avoid @mentioning people in commit messages and pull request descriptions(they are added to commit message by bors).
211Such messages create a lot of duplicate notification traffic during rebases.