diff options
author | Mikhail Rakhmanov <[email protected]> | 2020-06-13 07:42:15 +0100 |
---|---|---|
committer | Mikhail Rakhmanov <[email protected]> | 2020-06-13 07:42:15 +0100 |
commit | 16bbf4ab7f132e6e5e5318dccdef9a5d71afdd7f (patch) | |
tree | 4b79fa8c046be56b02427ba843e70cdf3ac05767 /docs/dev/README.md | |
parent | eeb8b9e236796da8734ba81a49164864497f7226 (diff) | |
parent | b56ad148db0c69eb279c225f45d324b4e80e7367 (diff) |
Merge branch 'master' into keyword_completion
# Conflicts:
# docs/user/generated_features.adoc
Diffstat (limited to 'docs/dev/README.md')
-rw-r--r-- | docs/dev/README.md | 209 |
1 files changed, 203 insertions, 6 deletions
diff --git a/docs/dev/README.md b/docs/dev/README.md index 65cc9fc12..ef5ffbf59 100644 --- a/docs/dev/README.md +++ b/docs/dev/README.md | |||
@@ -30,7 +30,7 @@ https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Fwg-rls-2.2E0 | |||
30 | 30 | ||
31 | * [good-first-issue](https://github.com/rust-analyzer/rust-analyzer/labels/good%20first%20issue) | 31 | * [good-first-issue](https://github.com/rust-analyzer/rust-analyzer/labels/good%20first%20issue) |
32 | are good issues to get into the project. | 32 | are good issues to get into the project. |
33 | * [E-mentor](https://github.com/rust-analyzer/rust-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3AE-mentor) | 33 | * [E-has-instructions](https://github.com/rust-analyzer/rust-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3AE-has-instructions) |
34 | issues have links to the code in question and tests. | 34 | issues have links to the code in question and tests. |
35 | * [E-easy](https://github.com/rust-analyzer/rust-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3AE-easy), | 35 | * [E-easy](https://github.com/rust-analyzer/rust-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3AE-easy), |
36 | [E-medium](https://github.com/rust-analyzer/rust-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3AE-medium), | 36 | [E-medium](https://github.com/rust-analyzer/rust-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3AE-medium), |
@@ -55,7 +55,7 @@ You can run `cargo xtask install-pre-commit-hook` to install git-hook to run rus | |||
55 | All Rust code lives in the `crates` top-level directory, and is organized as a | 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 | 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 | 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 | 58 | TypeScript). The `docs` top-level directory contains both developer and user |
59 | documentation. | 59 | documentation. |
60 | 60 | ||
61 | We have some automation infra in Rust in the `xtask` package. It contains | 61 | We have some automation infra in Rust in the `xtask` package. It contains |
@@ -79,8 +79,8 @@ possible. There's **"Run Extension (Debug Build)"** launch configuration for thi | |||
79 | In general, I use one of the following workflows for fixing bugs and | 79 | In general, I use one of the following workflows for fixing bugs and |
80 | implementing features. | 80 | implementing features. |
81 | 81 | ||
82 | If the problem concerns only internal parts of rust-analyzer (ie, I don't need | 82 | If the problem concerns only internal parts of rust-analyzer (i.e. I don't need |
83 | to touch `rust-analyzer` crate or typescript code), there is a unit-test for it. | 83 | to touch `rust-analyzer` crate or TypeScript code), there is a unit-test for it. |
84 | So, I use **Rust Analyzer: Run** action in VS Code to run this single test, and | 84 | So, I use **Rust Analyzer: Run** action in VS Code to run this single test, and |
85 | then just do printf-driven development/debugging. As a sanity check after I'm | 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 | 86 | done, I use `cargo xtask install --server` and **Reload Window** action in VS |
@@ -117,6 +117,203 @@ 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 | 117 | path/to/some/rust/crate` to run a batch analysis. This is primarily useful for |
118 | performance optimizations, or for bug minimization. | 118 | performance optimizations, or for bug minimization. |
119 | 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 reviewer to flag only some nits in the PR, and than 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 rename 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 thing too keep an eye on is 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 `[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 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 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 | ## Order of Imports | ||
170 | |||
171 | We separate import groups with blank lines | ||
172 | |||
173 | ```rust | ||
174 | mod x; | ||
175 | mod y; | ||
176 | |||
177 | use std::{ ... } | ||
178 | |||
179 | use crate_foo::{ ... } | ||
180 | use crate_bar::{ ... } | ||
181 | |||
182 | use crate::{} | ||
183 | |||
184 | use super::{} // but prefer `use crate::` | ||
185 | ``` | ||
186 | |||
187 | ## Import Style | ||
188 | |||
189 | Items from `hir` and `ast` should be used qualified: | ||
190 | |||
191 | ```rust | ||
192 | // Good | ||
193 | use ra_syntax::ast; | ||
194 | |||
195 | fn frobnicate(func: hir::Function, strukt: ast::StructDef) {} | ||
196 | |||
197 | // Not as good | ||
198 | use hir::Function; | ||
199 | use ra_syntax::ast::StructDef; | ||
200 | |||
201 | fn frobnicate(func: Function, strukt: StructDef) {} | ||
202 | ``` | ||
203 | |||
204 | Avoid local `use MyEnum::*` imports. | ||
205 | |||
206 | Prefer `use crate::foo::bar` to `use super::bar`. | ||
207 | |||
208 | ## Order of Items | ||
209 | |||
210 | Optimize for the reader who sees the file for the first time, and wants to get the general idea about what's going on. | ||
211 | People read things from top to bottom, so place most important things first. | ||
212 | |||
213 | Specifically, if all items except one are private, always put the non-private item on top. | ||
214 | |||
215 | Put `struct`s and `enum`s first, functions and impls last. | ||
216 | |||
217 | Do | ||
218 | |||
219 | ```rust | ||
220 | // Good | ||
221 | struct Foo { | ||
222 | bars: Vec<Bar> | ||
223 | } | ||
224 | |||
225 | struct Bar; | ||
226 | ``` | ||
227 | |||
228 | rather than | ||
229 | |||
230 | ```rust | ||
231 | // Not as good | ||
232 | struct Bar; | ||
233 | |||
234 | struct Foo { | ||
235 | bars: Vec<Bar> | ||
236 | } | ||
237 | ``` | ||
238 | |||
239 | ## Documentation | ||
240 | |||
241 | For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. | ||
242 | If the line is too long, you want to split the sentence in two :-) | ||
243 | |||
244 | ## Preconditions | ||
245 | |||
246 | Function preconditions should generally be expressed in types and provided by the caller (rather than checked by callee): | ||
247 | |||
248 | ```rust | ||
249 | // Good | ||
250 | fn frbonicate(walrus: Walrus) { | ||
251 | ... | ||
252 | } | ||
253 | |||
254 | // Not as good | ||
255 | fn frobnicate(walrus: Option<Walrus>) { | ||
256 | let walrus = match walrus { | ||
257 | Some(it) => it, | ||
258 | None => return, | ||
259 | }; | ||
260 | ... | ||
261 | } | ||
262 | ``` | ||
263 | |||
264 | ## Commit Style | ||
265 | |||
266 | We don't have specific rules around git history hygiene. | ||
267 | Maintaining clean git history is encouraged, but not enforced. | ||
268 | We use rebase workflow, it's OK to rewrite history during PR review process. | ||
269 | |||
270 | Avoid @mentioning people in commit messages, as such messages create a lot of duplicate notification traffic during rebases. | ||
271 | |||
272 | # Architecture Invariants | ||
273 | |||
274 | This section tries to document high-level design constraints, which are not | ||
275 | always obvious from the low-level code. | ||
276 | |||
277 | ## Incomplete syntax trees | ||
278 | |||
279 | Syntax trees are by design incomplete and do not enforce well-formedness. | ||
280 | If ast method returns an `Option`, it *can* be `None` at runtime, even if this is forbidden by the grammar. | ||
281 | |||
282 | ## LSP independence | ||
283 | |||
284 | rust-analyzer is independent from LSP. | ||
285 | It provides features for a hypothetical perfect Rust-specific IDE client. | ||
286 | Internal representations are lowered to LSP in the `rust-analyzer` crate (the only crate which is allowed to use LSP types). | ||
287 | |||
288 | ## IDE/Compiler split | ||
289 | |||
290 | There's a semi-hard split between "compiler" and "IDE", at the `ra_hir` crate. | ||
291 | Compiler derives new facts about source code. | ||
292 | It explicitly acknowledges that not all info is available (i.e. you can't look at types during name resolution). | ||
293 | |||
294 | IDE assumes that all information is available at all times. | ||
295 | |||
296 | IDE should use only types from `ra_hir`, and should not depend on the underling compiler types. | ||
297 | `ra_hir` is a facade. | ||
298 | |||
299 | ## IDE API | ||
300 | |||
301 | The main IDE crate (`ra_ide`) uses "Plain Old Data" for the API. | ||
302 | Rather than talking in definitions and references, it talks in Strings and textual offsets. | ||
303 | 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. | ||
304 | The results are 100% Rust specific though. | ||
305 | |||
306 | ## Parser Tests | ||
307 | |||
308 | Test for parser (`ra_parser`) live in `ra_syntax` crate (see `test_data` direcotory). | ||
309 | There are two kinds of tests: | ||
310 | |||
311 | * Manually written test cases in `parser/ok` and `parser/err` | ||
312 | * "Inline" tests in `parser/inline` (these are generated) from comments in `ra_parser` crate. | ||
313 | |||
314 | 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. | ||
315 | 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. | ||
316 | |||
120 | # Logging | 317 | # Logging |
121 | 318 | ||
122 | Logging is done by both rust-analyzer and VS Code, so it might be tricky to | 319 | Logging is done by both rust-analyzer and VS Code, so it might be tricky to |
@@ -159,8 +356,8 @@ There's also two VS Code commands which might be of interest: | |||
159 | rust code that it refers to and the rust editor will also highlight the proper | 356 | rust code that it refers to and the rust editor will also highlight the proper |
160 | text range. | 357 | text range. |
161 | 358 | ||
162 | If you press <kbd>Ctrl</kbd> (i.e. trigger goto definition) in the inspected | 359 | If you trigger Go to Definition in the inspected Rust source file, |
163 | Rust source file the syntax tree read-only editor should scroll to and select the | 360 | the syntax tree read-only editor should scroll to and select the |
164 | appropriate syntax node token. | 361 | appropriate syntax node token. |
165 | 362 | ||
166 | ![demo](https://user-images.githubusercontent.com/36276403/78225773-6636a480-74d3-11ea-9d9f-1c9d42da03b0.png) | 363 | ![demo](https://user-images.githubusercontent.com/36276403/78225773-6636a480-74d3-11ea-9d9f-1c9d42da03b0.png) |