diff options
Diffstat (limited to 'docs/dev')
-rw-r--r-- | docs/dev/README.md | 351 | ||||
-rw-r--r-- | docs/dev/style.md | 212 | ||||
-rw-r--r-- | docs/dev/syntax.md | 2 |
3 files changed, 296 insertions, 269 deletions
diff --git a/docs/dev/README.md b/docs/dev/README.md index 2896d333e..51cf716b3 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 | ||
51 | You can run `cargo xtask install-pre-commit-hook` to install git-hook to run rustfmt on commit. | 51 | You can run `cargo xtask install-pre-commit-hook` to install git-hook to run rustfmt on commit. |
52 | 52 | ||
53 | # Code organization | ||
54 | |||
55 | All Rust code lives in the `crates` top-level directory, and is organized as a | ||
56 | single Cargo workspace. The `editors` top-level directory contains code for | ||
57 | integrating with editors. Currently, it contains the plugin for VS Code (in | ||
58 | TypeScript). The `docs` top-level directory contains both developer and user | ||
59 | documentation. | ||
60 | |||
61 | We have some automation infra in Rust in the `xtask` package. It contains | ||
62 | stuff like formatting checking, code generation and powers `cargo xtask install`. | ||
63 | The latter syntax is achieved with the help of cargo aliases (see `.cargo` | ||
64 | directory). | ||
65 | |||
66 | # Launching rust-analyzer | 53 | # Launching rust-analyzer |
67 | 54 | ||
68 | Debugging the language server can be tricky: LSP is rather chatty, so driving it | 55 | Debugging the language server can be tricky. |
69 | from the command line is not really feasible, driving it via VS Code requires | 56 | LSP is rather chatty, so driving it from the command line is not really feasible, driving it via VS Code requires interacting with two processes. |
70 | interacting with two processes. | ||
71 | 57 | ||
72 | For this reason, the best way to see how rust-analyzer works is to find a | 58 | For this reason, the best way to see how rust-analyzer works is to find a relevant test and execute it. |
73 | relevant test and execute it (VS Code includes an action for running a single | 59 | VS Code & Emacs include an action for running a single test. |
74 | test). | ||
75 | 60 | ||
76 | However, launching a VS Code instance with a locally built language server is | 61 | Launching a VS Code instance with a locally built language server is also possible. |
77 | possible. There's **"Run Extension (Debug Build)"** launch configuration for this. | 62 | There's **"Run Extension (Debug Build)"** launch configuration for this in VS Code. |
78 | 63 | ||
79 | In general, I use one of the following workflows for fixing bugs and | 64 | In general, I use one of the following workflows for fixing bugs and implementing features: |
80 | implementing features. | ||
81 | 65 | ||
82 | If the problem concerns only internal parts of rust-analyzer (i.e. I don't need | 66 | If 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. |
83 | to touch the `rust-analyzer` crate or TypeScript code), there is a unit-test for it. | 67 | So, I use **Rust Analyzer: Run** action in VS Code to run this single test, and then just do printf-driven development/debugging. |
84 | So, I use **Rust Analyzer: Run** action in VS Code to run this single test, and | 68 | As 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. |
85 | then just do printf-driven development/debugging. As a sanity check after I'm | ||
86 | done, I use `cargo xtask install --server` and **Reload Window** action in VS | ||
87 | Code to sanity check that the thing works as I expect. | ||
88 | 69 | ||
89 | If the problem concerns only the VS Code extension, I use **Run Installed Extension** | 70 | If the problem concerns only the VS Code extension, I use **Run Installed Extension** launch configuration from `launch.json`. |
90 | launch configuration from `launch.json`. Notably, this uses the usual | 71 | Notably, this uses the usual `rust-analyzer` binary from `PATH`. |
91 | `rust-analyzer` binary from `PATH`. For this, it is important to have the following | 72 | For this, it is important to have the following in your `settings.json` file: |
92 | in 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 | ``` |
98 | After I am done with the fix, I use `cargo | 78 | After I am done with the fix, I use `cargo xtask install --client-code` to try the new extension for real. |
99 | xtask install --client-code` to try the new extension for real. | ||
100 | |||
101 | If I need to fix something in the `rust-analyzer` crate, I feel sad because it's | ||
102 | on the boundary between the two processes, and working there is slow. I usually | ||
103 | just `cargo xtask install --server` and poke changes from my live environment. | ||
104 | Note that this uses `--release`, which is usually faster overall, because | ||
105 | loading stdlib into debug version of rust-analyzer takes a lot of time. To speed | ||
106 | things up, sometimes I open a temporary hello-world project which has | ||
107 | `"rust-analyzer.withSysroot": false` in `.code/settings.json`. This flag causes | ||
108 | rust-analyzer to skip loading the sysroot, which greatly reduces the amount of | ||
109 | things rust-analyzer needs to do, and makes printf's more useful. Note that you | ||
110 | should only use the `eprint!` family of macros for debugging: stdout is used for LSP | ||
111 | communication, and `print!` would break it. | ||
112 | |||
113 | If I need to fix something simultaneously in the server and in the client, I | ||
114 | feel even more sad. I don't have a specific workflow for this case. | ||
115 | |||
116 | Additionally, I use `cargo run --release -p rust-analyzer -- analysis-stats | ||
117 | path/to/some/rust/crate` to run a batch analysis. This is primarily useful for | ||
118 | performance optimizations, or for bug minimization. | ||
119 | |||
120 | # Code Style & Review Process | ||
121 | |||
122 | Our 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 | |||
127 | 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. | ||
128 | Sending small cleanup PRs (like renaming a single local variable) is encouraged. | ||
129 | |||
130 | ## Scale of Changes | ||
131 | |||
132 | Everyone knows that it's better to send small & focused pull requests. | ||
133 | The problem is, sometimes you *have* to, eg, rewrite the whole compiler, and that just doesn't fit into a set of isolated PRs. | ||
134 | |||
135 | The main things to keep an eye on are the boundaries between various components. | ||
136 | There are three kinds of changes: | ||
137 | |||
138 | 1. 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 | |||
142 | 2. 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 | |||
146 | 3. 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 | |||
150 | For 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 | |||
156 | For the second group, the change would be subjected to quite a bit of scrutiny and iteration. | ||
157 | The new API needs to be right (or at least easy to change later). | ||
158 | The actual implementation doesn't matter that much. | ||
159 | It's very important to minimize the amount of changed lines of code for changes of the second kind. | ||
160 | 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. | ||
161 | In this case, we'll probably ask you to split API changes into a separate PR. | ||
162 | |||
163 | Changes of the third group should be pretty rare, so we don't specify any specific process for them. | ||
164 | That said, adding an innocent-looking `pub use` is a very simple way to break encapsulation, keep an eye on it! | ||
165 | |||
166 | Note: if you enjoyed this abstract hand-waving about boundaries, you might appreciate | ||
167 | https://www.tedinski.com/2018/02/06/system-boundaries.html | ||
168 | |||
169 | ## Crates.io Dependencies | ||
170 | |||
171 | We try to be very conservative with usage of crates.io dependencies. | ||
172 | Don't use small "helper" crates (exception: `itertools` is allowed). | ||
173 | If there's some general reusable bit of code you need, consider adding it to the `stdx` crate. | ||
174 | |||
175 | ## Minimal Tests | ||
176 | |||
177 | Most tests in rust-analyzer start with a snippet of Rust code. | ||
178 | 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. | ||
179 | There 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 | |||
186 | It also makes sense to format snippets more compactly (for example, by placing enum defitions like `enum E { Foo, Bar }` on a single line), | ||
187 | as long as they are still readable. | ||
188 | |||
189 | ## Order of Imports | ||
190 | |||
191 | We separate import groups with blank lines | ||
192 | |||
193 | ```rust | ||
194 | mod x; | ||
195 | mod y; | ||
196 | |||
197 | use std::{ ... } | ||
198 | |||
199 | use crate_foo::{ ... } | ||
200 | use crate_bar::{ ... } | ||
201 | |||
202 | use crate::{} | ||
203 | |||
204 | use super::{} // but prefer `use crate::` | ||
205 | ``` | ||
206 | |||
207 | ## Import Style | ||
208 | |||
209 | Items from `hir` and `ast` should be used qualified: | ||
210 | |||
211 | ```rust | ||
212 | // Good | ||
213 | use ra_syntax::ast; | ||
214 | |||
215 | fn frobnicate(func: hir::Function, strukt: ast::StructDef) {} | ||
216 | |||
217 | // Not as good | ||
218 | use hir::Function; | ||
219 | use ra_syntax::ast::StructDef; | ||
220 | |||
221 | fn frobnicate(func: Function, strukt: StructDef) {} | ||
222 | ``` | ||
223 | |||
224 | Avoid local `use MyEnum::*` imports. | ||
225 | |||
226 | Prefer `use crate::foo::bar` to `use super::bar`. | ||
227 | |||
228 | ## Order of Items | ||
229 | |||
230 | Optimize for the reader who sees the file for the first time, and wants to get the general idea about what's going on. | ||
231 | People read things from top to bottom, so place most important things first. | ||
232 | |||
233 | Specifically, if all items except one are private, always put the non-private item on top. | ||
234 | |||
235 | Put `struct`s and `enum`s first, functions and impls last. | ||
236 | |||
237 | Do | ||
238 | |||
239 | ```rust | ||
240 | // Good | ||
241 | struct Foo { | ||
242 | bars: Vec<Bar> | ||
243 | } | ||
244 | |||
245 | struct Bar; | ||
246 | ``` | ||
247 | |||
248 | rather than | ||
249 | 79 | ||
250 | ```rust | 80 | If 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. |
251 | // Not as good | 81 | I usually just `cargo xtask install --server` and poke changes from my live environment. |
252 | struct Bar; | 82 | Note that this uses `--release`, which is usually faster overall, because loading stdlib into debug version of rust-analyzer takes a lot of time. |
83 | To speed things up, sometimes I open a temporary hello-world project which has `"rust-analyzer.withSysroot": false` in `.code/settings.json`. | ||
84 | This 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. | ||
85 | Note that you should only use the `eprint!` family of macros for debugging: stdout is used for LSP communication, and `print!` would break it. | ||
253 | 86 | ||
254 | struct Foo { | 87 | If I need to fix something simultaneously in the server and in the client, I feel even more sad. |
255 | bars: Vec<Bar> | 88 | I don't have a specific workflow for this case. |
256 | } | ||
257 | ``` | ||
258 | 89 | ||
259 | ## Variable Naming | 90 | Additionally, I use `cargo run --release -p rust-analyzer -- analysis-stats path/to/some/rust/crate` to run a batch analysis. |
91 | This is primarily useful for performance optimizations, or for bug minimization. | ||
260 | 92 | ||
261 | We 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 |
262 | The default name is a lowercased name of the type: `global_state: GlobalState`. | ||
263 | Avoid ad-hoc acronyms and contractions, but use the ones that exist consistently (`db`, `ctx`, `acc`). | ||
264 | The default name for "result of the function" local variable is `res`. | ||
265 | |||
266 | ## Collection types | ||
267 | 94 | ||
268 | We prefer `rustc_hash::FxHashMap` and `rustc_hash::FxHashSet` instead of the ones in `std::collections`. | 95 | Tests for the parser (`ra_parser`) live in the `ra_syntax` crate (see `test_data` directory). |
269 | They use a hasher that's slightly faster and using them consistently will reduce code size by some small amount. | 96 | There 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 | ||
273 | Function preconditions should generally be expressed in types and provided by the caller (rather than checked by callee): | 101 | The 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. |
102 | If 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 | 104 | To update test data, run with `UPDATE_EXPECT` variable: |
276 | // Good | ||
277 | fn frbonicate(walrus: Walrus) { | ||
278 | ... | ||
279 | } | ||
280 | 105 | ||
281 | // Not as good | 106 | ```bash |
282 | fn frobnicate(walrus: Option<Walrus>) { | 107 | env 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 | 110 | After adding a new inline test you need to run `cargo xtest codegen` and also update the test data as described above. |
292 | |||
293 | While we don't specifically optimize code yet, avoid writing code which is slower than it needs to be. | ||
294 | Don't allocate a `Vec` where an iterator would do, don't allocate strings needlessly. | ||
295 | 111 | ||
296 | ```rust | 112 | ## TypeScript Tests |
297 | // Good | ||
298 | use itertools::Itertools; | ||
299 | 113 | ||
300 | let (first_word, second_word) = match text.split_ascii_whitespace().collect_tuple() { | 114 | If 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 |
306 | let words = text.split_ascii_whitespace().collect::<Vec<_>>(); | 117 | cd editors/code |
307 | if words.len() != 2 { | 118 | npm ci |
308 | return | 119 | npm run lint |
309 | } | ||
310 | ``` | 120 | ``` |
311 | 121 | ||
312 | ## Documentation | 122 | # Code organization |
313 | |||
314 | For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. | ||
315 | If the line is too long, you want to split the sentence in two :-) | ||
316 | |||
317 | ## Commit Style | ||
318 | 123 | ||
319 | We don't have specific rules around git history hygiene. | 124 | All Rust code lives in the `crates` top-level directory, and is organized as a single Cargo workspace. |
320 | Maintaining clean git history is encouraged, but not enforced. | 125 | The `editors` top-level directory contains code for integrating with editors. |
321 | We use rebase workflow, it's OK to rewrite history during PR review process. | 126 | Currently, it contains the plugin for VS Code (in TypeScript). |
127 | The `docs` top-level directory contains both developer and user documentation. | ||
322 | 128 | ||
323 | Avoid @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. | 129 | We have some automation infra in Rust in the `xtask` package. |
130 | It contains stuff like formatting checking, code generation and powers `cargo xtask install`. | ||
131 | The 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. | |||
355 | Rather than talking in definitions and references, it talks in Strings and textual offsets. | 163 | Rather than talking in definitions and references, it talks in Strings and textual offsets. |
356 | In 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. | 164 | In 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. |
357 | The results are 100% Rust specific though. | 165 | The results are 100% Rust specific though. |
166 | Shout 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 | |||
361 | Tests for the parser (`ra_parser`) live in the `ra_syntax` crate (see `test_data` directory). | ||
362 | There 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 | |||
367 | The 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. | ||
368 | If 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 | |||
370 | To update test data, run with `UPDATE_EXPECT` variable: | ||
371 | |||
372 | ```bash | ||
373 | env UPDATE_EXPECT=1 cargo qt | ||
374 | ``` | ||
375 | |||
376 | After 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 | |||
380 | If you change files under `editors/code` and would like to run the tests and linter, install npm and run: | ||
381 | 169 | ||
382 | ```bash | 170 | Do see [./style.md](./style.md). |
383 | cd editors/code | ||
384 | npm ci | ||
385 | npm run lint | ||
386 | ``` | ||
387 | 171 | ||
388 | # Logging | 172 | # Logging |
389 | 173 | ||
@@ -451,3 +235,34 @@ For measuring time of incremental analysis, use either of these: | |||
451 | $ cargo run --release -p rust-analyzer -- analysis-bench ../chalk/ --highlight ../chalk/chalk-engine/src/logic.rs | 235 | $ cargo run --release -p rust-analyzer -- analysis-bench ../chalk/ --highlight ../chalk/chalk-engine/src/logic.rs |
452 | $ cargo run --release -p rust-analyzer -- analysis-bench ../chalk/ --complete ../chalk/chalk-engine/src/logic.rs:94:0 | 236 | $ cargo run --release -p rust-analyzer -- analysis-bench ../chalk/ --complete ../chalk/chalk-engine/src/logic.rs:94:0 |
453 | ``` | 237 | ``` |
238 | |||
239 | # Release Process | ||
240 | |||
241 | Release process is handled by `release`, `dist` and `promote` xtasks, `release` being the main one. | ||
242 | |||
243 | `release` assumes that you have checkouts of `rust-analyzer`, `rust-analyzer.github.io`, and `rust-lang/rust` in the same directory: | ||
244 | |||
245 | ``` | ||
246 | ./rust-analyzer | ||
247 | ./rust-analyzer.github.io | ||
248 | ./rust-rust-analyzer # Note the name! | ||
249 | ``` | ||
250 | |||
251 | Additionally, it assumes that remote for `rust-analyzer` is called `upstream` (I use `origin` to point to my fork). | ||
252 | |||
253 | Release steps: | ||
254 | |||
255 | 1. Inside rust-analyzer, run `cargo xtask release`. This will: | ||
256 | * checkout the `release` branch | ||
257 | * reset it to `upstream/nightly` | ||
258 | * push it to `upstream`. This triggers GitHub Actions which: | ||
259 | * runs `cargo xtask dist` to package binaries and VS Code extension | ||
260 | * makes a GitHub release | ||
261 | * pushes VS Code extension to the marketplace | ||
262 | * create new changelog in `rust-analyzer.github.io` | ||
263 | * create `rust-analyzer.github.io/git.log` file with the log of merge commits since last release | ||
264 | 2. While the release is in progress, fill-in the changelog using `git.log` | ||
265 | 3. Commit & push the changelog | ||
266 | 4. Tweet | ||
267 | 5. Inside `rust-analyzer`, run `cargo xtask promote` -- this will create a PR to rust-lang/rust updating rust-analyzer's submodule. | ||
268 | Self-approve the PR. | ||
diff --git a/docs/dev/style.md b/docs/dev/style.md new file mode 100644 index 000000000..1c68f5702 --- /dev/null +++ b/docs/dev/style.md | |||
@@ -0,0 +1,212 @@ | |||
1 | Our 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 | |||
6 | 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. | ||
7 | Sending small cleanup PRs (like renaming a single local variable) is encouraged. | ||
8 | |||
9 | # Scale of Changes | ||
10 | |||
11 | Everyone knows that it's better to send small & focused pull requests. | ||
12 | The problem is, sometimes you *have* to, eg, rewrite the whole compiler, and that just doesn't fit into a set of isolated PRs. | ||
13 | |||
14 | The main things to keep an eye on are the boundaries between various components. | ||
15 | There are three kinds of changes: | ||
16 | |||
17 | 1. 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 | |||
21 | 2. 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 | |||
25 | 3. 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 | |||
29 | For 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 | |||
35 | For the second group, the change would be subjected to quite a bit of scrutiny and iteration. | ||
36 | The new API needs to be right (or at least easy to change later). | ||
37 | The actual implementation doesn't matter that much. | ||
38 | It's very important to minimize the amount of changed lines of code for changes of the second kind. | ||
39 | 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. | ||
40 | In this case, we'll probably ask you to split API changes into a separate PR. | ||
41 | |||
42 | Changes of the third group should be pretty rare, so we don't specify any specific process for them. | ||
43 | That said, adding an innocent-looking `pub use` is a very simple way to break encapsulation, keep an eye on it! | ||
44 | |||
45 | Note: if you enjoyed this abstract hand-waving about boundaries, you might appreciate | ||
46 | https://www.tedinski.com/2018/02/06/system-boundaries.html | ||
47 | |||
48 | # Crates.io Dependencies | ||
49 | |||
50 | We try to be very conservative with usage of crates.io dependencies. | ||
51 | Don't use small "helper" crates (exception: `itertools` is allowed). | ||
52 | If there's some general reusable bit of code you need, consider adding it to the `stdx` crate. | ||
53 | |||
54 | # Minimal Tests | ||
55 | |||
56 | Most tests in rust-analyzer start with a snippet of Rust code. | ||
57 | 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. | ||
58 | There 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 | |||
65 | It also makes sense to format snippets more compactly (for example, by placing enum definitions like `enum E { Foo, Bar }` on a single line), | ||
66 | as long as they are still readable. | ||
67 | |||
68 | ## Order of Imports | ||
69 | |||
70 | Separate import groups with blank lines. | ||
71 | Use one `use` per crate. | ||
72 | |||
73 | ```rust | ||
74 | mod x; | ||
75 | mod y; | ||
76 | |||
77 | // First std. | ||
78 | use std::{ ... } | ||
79 | |||
80 | // Second, external crates (both crates.io crates and other rust-analyzer crates). | ||
81 | use crate_foo::{ ... } | ||
82 | use crate_bar::{ ... } | ||
83 | |||
84 | // Then current crate. | ||
85 | use crate::{} | ||
86 | |||
87 | // Finally, parent and child modules, but prefer `use crate::`. | ||
88 | use super::{} | ||
89 | ``` | ||
90 | |||
91 | Module declarations come before the imports. | ||
92 | Order them in "suggested reading order" for a person new to the code base. | ||
93 | |||
94 | ## Import Style | ||
95 | |||
96 | Qualify items from `hir` and `ast`. | ||
97 | |||
98 | ```rust | ||
99 | // Good | ||
100 | use ra_syntax::ast; | ||
101 | |||
102 | fn frobnicate(func: hir::Function, strukt: ast::StructDef) {} | ||
103 | |||
104 | // Not as good | ||
105 | use hir::Function; | ||
106 | use ra_syntax::ast::StructDef; | ||
107 | |||
108 | fn frobnicate(func: Function, strukt: StructDef) {} | ||
109 | ``` | ||
110 | |||
111 | Avoid local `use MyEnum::*` imports. | ||
112 | |||
113 | Prefer `use crate::foo::bar` to `use super::bar`. | ||
114 | |||
115 | ## Order of Items | ||
116 | |||
117 | Optimize for the reader who sees the file for the first time, and wants to get a general idea about what's going on. | ||
118 | People read things from top to bottom, so place most important things first. | ||
119 | |||
120 | Specifically, if all items except one are private, always put the non-private item on top. | ||
121 | |||
122 | Put `struct`s and `enum`s first, functions and impls last. | ||
123 | |||
124 | Do | ||
125 | |||
126 | ```rust | ||
127 | // Good | ||
128 | struct Foo { | ||
129 | bars: Vec<Bar> | ||
130 | } | ||
131 | |||
132 | struct Bar; | ||
133 | ``` | ||
134 | |||
135 | rather than | ||
136 | |||
137 | ```rust | ||
138 | // Not as good | ||
139 | struct Bar; | ||
140 | |||
141 | struct Foo { | ||
142 | bars: Vec<Bar> | ||
143 | } | ||
144 | ``` | ||
145 | |||
146 | ## Variable Naming | ||
147 | |||
148 | Use boring and long names for local variables ([yay code completion](https://github.com/rust-analyzer/rust-analyzer/pull/4162#discussion_r417130973)). | ||
149 | The default name is a lowercased name of the type: `global_state: GlobalState`. | ||
150 | Avoid ad-hoc acronyms and contractions, but use the ones that exist consistently (`db`, `ctx`, `acc`). | ||
151 | The default name for "result of the function" local variable is `res`. | ||
152 | The default name for "I don't really care about the name" variable is `it`. | ||
153 | |||
154 | ## Collection types | ||
155 | |||
156 | Prefer `rustc_hash::FxHashMap` and `rustc_hash::FxHashSet` instead of the ones in `std::collections`. | ||
157 | They use a hasher that's slightly faster and using them consistently will reduce code size by some small amount. | ||
158 | |||
159 | ## Preconditions | ||
160 | |||
161 | Express function preconditions in types and force the caller to provide them (rather than checking in callee): | ||
162 | |||
163 | ```rust | ||
164 | // Good | ||
165 | fn frbonicate(walrus: Walrus) { | ||
166 | ... | ||
167 | } | ||
168 | |||
169 | // Not as good | ||
170 | fn frobnicate(walrus: Option<Walrus>) { | ||
171 | let walrus = match walrus { | ||
172 | Some(it) => it, | ||
173 | None => return, | ||
174 | }; | ||
175 | ... | ||
176 | } | ||
177 | ``` | ||
178 | |||
179 | ## Premature Pessimization | ||
180 | |||
181 | Avoid writing code which is slower than it needs to be. | ||
182 | Don't allocate a `Vec` where an iterator would do, don't allocate strings needlessly. | ||
183 | |||
184 | ```rust | ||
185 | // Good | ||
186 | use itertools::Itertools; | ||
187 | |||
188 | let (first_word, second_word) = match text.split_ascii_whitespace().collect_tuple() { | ||
189 | Some(it) => it, | ||
190 | None => return, | ||
191 | } | ||
192 | |||
193 | // Not as good | ||
194 | let words = text.split_ascii_whitespace().collect::<Vec<_>>(); | ||
195 | if words.len() != 2 { | ||
196 | return | ||
197 | } | ||
198 | ``` | ||
199 | |||
200 | ## Documentation | ||
201 | |||
202 | For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. | ||
203 | If the line is too long, you want to split the sentence in two :-) | ||
204 | |||
205 | ## Commit Style | ||
206 | |||
207 | We don't have specific rules around git history hygiene. | ||
208 | Maintaining clean git history is encouraged, but not enforced. | ||
209 | Use rebase workflow, it's OK to rewrite history during PR review process. | ||
210 | |||
211 | Avoid @mentioning people in commit messages and pull request descriptions(they are added to commit message by bors). | ||
212 | Such messages create a lot of duplicate notification traffic during rebases. | ||
diff --git a/docs/dev/syntax.md b/docs/dev/syntax.md index d4bc4b07c..f1bcdc4af 100644 --- a/docs/dev/syntax.md +++ b/docs/dev/syntax.md | |||
@@ -74,7 +74,7 @@ Points of note: | |||
74 | * The original text can be recovered by concatenating the texts of all tokens in order. | 74 | * The original text can be recovered by concatenating the texts of all tokens in order. |
75 | * Accessing a child of particular type (for example, parameter list of a function) generally involves linerary traversing the children, looking for a specific `kind`. | 75 | * Accessing a child of particular type (for example, parameter list of a function) generally involves linerary traversing the children, looking for a specific `kind`. |
76 | * Modifying the tree is roughly `O(depth)`. | 76 | * Modifying the tree is roughly `O(depth)`. |
77 | We don't make special efforts to guarantree that the depth is not liner, but, in practice, syntax trees are branchy and shallow. | 77 | We don't make special efforts to guarantee that the depth is not linear, but, in practice, syntax trees are branchy and shallow. |
78 | * If mandatory (grammar wise) node is missing from the input, it's just missing from the tree. | 78 | * If mandatory (grammar wise) node is missing from the input, it's just missing from the tree. |
79 | * If an extra erroneous input is present, it is wrapped into a node with `ERROR` kind, and treated just like any other node. | 79 | * If an extra erroneous input is present, it is wrapped into a node with `ERROR` kind, and treated just like any other node. |
80 | * Parser errors are not a part of syntax tree. | 80 | * Parser errors are not a part of syntax tree. |