diff options
author | Aleksey Kladov <[email protected]> | 2020-08-02 13:37:27 +0100 |
---|---|---|
committer | Aleksey Kladov <[email protected]> | 2020-08-02 13:37:50 +0100 |
commit | edee52fa577ad3143777644d9b5e764b4e0a837d (patch) | |
tree | c35796b1504d01e792260145bb1a4ccd4b3d4858 | |
parent | e4d0f19b01e5ba5007073f126b7eb0276c79024f (diff) |
reorg docs
-rw-r--r-- | docs/dev/README.md | 320 | ||||
-rw-r--r-- | docs/dev/style.md | 211 |
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 | ||
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 | 79 | ||
193 | ```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. |
194 | mod x; | 81 | I usually just `cargo xtask install --server` and poke changes from my live environment. |
195 | mod y; | 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. | ||
196 | 86 | ||
197 | use std::{ ... } | 87 | If I need to fix something simultaneously in the server and in the client, I feel even more sad. |
198 | 88 | I don't have a specific workflow for this case. | |
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 | |||
250 | ```rust | ||
251 | // Not as good | ||
252 | struct Bar; | ||
253 | |||
254 | struct Foo { | ||
255 | bars: Vec<Bar> | ||
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 | ||
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 @@ | |||
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 | We separate import groups with blank lines | ||
71 | |||
72 | ```rust | ||
73 | mod x; | ||
74 | mod y; | ||
75 | |||
76 | // First std. | ||
77 | use std::{ ... } | ||
78 | |||
79 | // Second, external crates (both crates.io crates and other rust-analyzer crates). | ||
80 | use crate_foo::{ ... } | ||
81 | use crate_bar::{ ... } | ||
82 | |||
83 | // Then current crate. | ||
84 | use crate::{} | ||
85 | |||
86 | // Finally, parent and child modules, but prefer `use crate::`. | ||
87 | use super::{} | ||
88 | ``` | ||
89 | |||
90 | Module declarations come before the imports. | ||
91 | Order them in "suggested reading order" for a person new to the code base. | ||
92 | |||
93 | ## Import Style | ||
94 | |||
95 | Items from `hir` and `ast` should be used qualified: | ||
96 | |||
97 | ```rust | ||
98 | // Good | ||
99 | use ra_syntax::ast; | ||
100 | |||
101 | fn frobnicate(func: hir::Function, strukt: ast::StructDef) {} | ||
102 | |||
103 | // Not as good | ||
104 | use hir::Function; | ||
105 | use ra_syntax::ast::StructDef; | ||
106 | |||
107 | fn frobnicate(func: Function, strukt: StructDef) {} | ||
108 | ``` | ||
109 | |||
110 | Avoid local `use MyEnum::*` imports. | ||
111 | |||
112 | Prefer `use crate::foo::bar` to `use super::bar`. | ||
113 | |||
114 | ## Order of Items | ||
115 | |||
116 | Optimize for the reader who sees the file for the first time, and wants to get a general idea about what's going on. | ||
117 | People read things from top to bottom, so place most important things first. | ||
118 | |||
119 | Specifically, if all items except one are private, always put the non-private item on top. | ||
120 | |||
121 | Put `struct`s and `enum`s first, functions and impls last. | ||
122 | |||
123 | Do | ||
124 | |||
125 | ```rust | ||
126 | // Good | ||
127 | struct Foo { | ||
128 | bars: Vec<Bar> | ||
129 | } | ||
130 | |||
131 | struct Bar; | ||
132 | ``` | ||
133 | |||
134 | rather than | ||
135 | |||
136 | ```rust | ||
137 | // Not as good | ||
138 | struct Bar; | ||
139 | |||
140 | struct Foo { | ||
141 | bars: Vec<Bar> | ||
142 | } | ||
143 | ``` | ||
144 | |||
145 | ## Variable Naming | ||
146 | |||
147 | We generally use boring and long names for local variables ([yay code completion](https://github.com/rust-analyzer/rust-analyzer/pull/4162#discussion_r417130973)). | ||
148 | The default name is a lowercased name of the type: `global_state: GlobalState`. | ||
149 | Avoid ad-hoc acronyms and contractions, but use the ones that exist consistently (`db`, `ctx`, `acc`). | ||
150 | The default name for "result of the function" local variable is `res`. | ||
151 | The default name for "I don't really care about the name" variable is `it`. | ||
152 | |||
153 | ## Collection types | ||
154 | |||
155 | We prefer `rustc_hash::FxHashMap` and `rustc_hash::FxHashSet` instead of the ones in `std::collections`. | ||
156 | They use a hasher that's slightly faster and using them consistently will reduce code size by some small amount. | ||
157 | |||
158 | ## Preconditions | ||
159 | |||
160 | Function preconditions should generally be expressed in types and provided by the caller (rather than checked by callee): | ||
161 | |||
162 | ```rust | ||
163 | // Good | ||
164 | fn frbonicate(walrus: Walrus) { | ||
165 | ... | ||
166 | } | ||
167 | |||
168 | // Not as good | ||
169 | fn 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 | |||
180 | Avoid writing code which is slower than it needs to be. | ||
181 | Don't allocate a `Vec` where an iterator would do, don't allocate strings needlessly. | ||
182 | |||
183 | ```rust | ||
184 | // Good | ||
185 | use itertools::Itertools; | ||
186 | |||
187 | let (first_word, second_word) = match text.split_ascii_whitespace().collect_tuple() { | ||
188 | Some(it) => it, | ||
189 | None => return, | ||
190 | } | ||
191 | |||
192 | // Not as good | ||
193 | let words = text.split_ascii_whitespace().collect::<Vec<_>>(); | ||
194 | if words.len() != 2 { | ||
195 | return | ||
196 | } | ||
197 | ``` | ||
198 | |||
199 | ## Documentation | ||
200 | |||
201 | For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. | ||
202 | If the line is too long, you want to split the sentence in two :-) | ||
203 | |||
204 | ## Commit Style | ||
205 | |||
206 | We don't have specific rules around git history hygiene. | ||
207 | Maintaining clean git history is encouraged, but not enforced. | ||
208 | We use rebase workflow, it's OK to rewrite history during PR review process. | ||
209 | |||
210 | Avoid @mentioning people in commit messages and pull request descriptions(they are added to commit message by bors). | ||
211 | Such messages create a lot of duplicate notification traffic during rebases. | ||