From 994006585be6850b250b21aac76a58f1324cad5d Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Tue, 2 Jun 2020 11:16:49 +0200 Subject: Start documenting review process --- docs/dev/README.md | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 104 insertions(+), 1 deletion(-) (limited to 'docs/dev/README.md') diff --git a/docs/dev/README.md b/docs/dev/README.md index 65cc9fc12..1de5a2aab 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 * [good-first-issue](https://github.com/rust-analyzer/rust-analyzer/labels/good%20first%20issue) are good issues to get into the project. -* [E-mentor](https://github.com/rust-analyzer/rust-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3AE-mentor) +* [E-has-instructions](https://github.com/rust-analyzer/rust-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3AE-has-instructions) issues have links to the code in question and tests. * [E-easy](https://github.com/rust-analyzer/rust-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3AE-easy), [E-medium](https://github.com/rust-analyzer/rust-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3AE-medium), @@ -117,6 +117,109 @@ Additionally, I use `cargo run --release -p rust-analyzer -- analysis-stats path/to/some/rust/crate` to run a batch analysis. This is primarily useful for performance optimizations, or for bug minimization. +# Code Style & Review Process + +Our approach to "clean code" is two fold: + +* We generally don't block PRs on style changes. +* At the same time, all code in rust-analyzer is constantly refactored. + +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. +Sending small cleanup PRs (like rename a single local variable) is encouraged. + +## Scale of Changes + +Everyone knows that it's better to send small & focused pull requests. +The problem is, sometimes you *have* to, eg, rewrite the whole compiler, and that just doesn't fit into a set of isolated PRs. + +The main thing too keep an eye on is the boundaries between various components. +There are three kinds of changes: + +1. Internals of a single component are changed. + Specifically, you don't change any `pub` items. + A good example here would be an addition of a new assist. + +2. API of a component is expanded. + Specifically, you add a new `pub` function which wasn't there before. + A good example here would be expansion of assist API, for example, to implement lazy assists or assists groups. + +3. A new dependency between components is introduced. + Specifically, you add a `pub use` reexport from another crate or you add a new line to `[dependencies]` section of `Cargo.toml`. + A good example here would be adding reference search capability to the assists crates. + +For the first group, the change is generally merged as long as: + +* it works for the happy case, +* it has tests, +* it doesn't panic for unhappy case. + +For the second group, the change would be subjected to quite a bit of scrutiny and iteration. +The new API needs to be right (or at least easy to change later). +The actual implementation doesn't matter that much. +It's very important to minimize the amount of changed lines of code for changes of the second kind. +Often, you start doing change of the first kind, only to realise that you need to elevate to a change of the second kind. +In this case, we'll probably ask you to split API changes into a separate PR. + +Changes of the third group should be pretty rare, so we don't specify any specific process for them. +That said, adding an innocent-looking `pub use` is a very simple way to break encapsulation, keep an eye on it! + +Note: if you enjoyed this abstract hand-waving about boundaries, you might appreciate +https://www.tedinski.com/2018/02/06/system-boundaries.html + +## Order of Imports + +We separate import groups with blank lines + +``` +mod x; +mod y; + +use std::{ ... } + +use crate_foo::{ ... } +use crate_bar::{ ... } + +use crate::{} + +use super::{} // but prefer `use crate::` +``` + +## Order of Items + +Optimize for the reader who sees the file for the first time, and wants to get the general idea about what's going on. +People read things from top to bottom, so place most important things first. + +Specifically, if all items except one are private, always put the non-private item on top. + +Put `struct`s and `enum`s first, functions and impls last. + +Do + +``` +// Good +struct Foo { + bars: Vec +} + +struct Bar; +``` + +rather than + +``` +// Not as good +struct Bar; + +struct Foo { + bars: Vec +} +``` + +## Documentation + +For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. +If the line is too long, you want to split the sentence in two :-) + # Logging Logging is done by both rust-analyzer and VS Code, so it might be tricky to -- cgit v1.2.3 From 41ae7ed79f75d52179e3553e30e47709a82e693b Mon Sep 17 00:00:00 2001 From: Veetaha Date: Thu, 4 Jun 2020 01:48:47 +0300 Subject: Bufgix --- docs/dev/README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'docs/dev/README.md') diff --git a/docs/dev/README.md b/docs/dev/README.md index 1de5a2aab..194a40e15 100644 --- a/docs/dev/README.md +++ b/docs/dev/README.md @@ -170,7 +170,7 @@ https://www.tedinski.com/2018/02/06/system-boundaries.html We separate import groups with blank lines -``` +```rust mod x; mod y; @@ -195,7 +195,7 @@ Put `struct`s and `enum`s first, functions and impls last. Do -``` +```rust // Good struct Foo { bars: Vec @@ -206,7 +206,7 @@ struct Bar; rather than -``` +```rust // Not as good struct Bar; -- cgit v1.2.3 From ae1acbd09c8e98e4e23f01f633ad551dabd5c578 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 6 Jun 2020 19:32:45 +0200 Subject: Document import style --- docs/dev/README.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) (limited to 'docs/dev/README.md') diff --git a/docs/dev/README.md b/docs/dev/README.md index 194a40e15..6f74d4223 100644 --- a/docs/dev/README.md +++ b/docs/dev/README.md @@ -184,6 +184,27 @@ use crate::{} use super::{} // but prefer `use crate::` ``` +## Import Style + +Items from `hir` and `ast` should be used qualified: + +```rust +// Good +use ra_syntax::ast; + +fn frobnicate(func: hir::Function, strukt: ast::StructDef) {} + +// Not as good +use hir::Function; +use ra_syntax::ast::StructDef; + +fn frobnicate(func: Function, strukt: StructDef) {} +``` + +Avoid local `use MyEnum::*` imports. + +Prefer `use crate::foo::bar` to `use super::bar`. + ## Order of Items Optimize for the reader who sees the file for the first time, and wants to get the general idea about what's going on. -- cgit v1.2.3 From 81ffe973ac265507419024048c166bbeef9aa275 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 6 Jun 2020 19:54:41 +0200 Subject: Document certain invariants --- docs/dev/README.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) (limited to 'docs/dev/README.md') diff --git a/docs/dev/README.md b/docs/dev/README.md index 6f74d4223..903cb4055 100644 --- a/docs/dev/README.md +++ b/docs/dev/README.md @@ -241,6 +241,33 @@ struct Foo { For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. If the line is too long, you want to split the sentence in two :-) +# Architecture Invariants + +This section tries to document high-level design constraints, which are not +always obvious from the low-level code. + +## Incomplete syntax trees + +Syntax trees are by design incomplete and do not enforce well-formedness. +If ast method returns an `Option`, it *can* be `None` at runtime, even if this is forbidden by the grammar. + +## LSP indenpendence + +rust-analyzer is independent from LSP. +It provides features for a hypothetical perfect Rust-specific IDE client. +Internal representations are lowered to LSP in the `rust-analyzer` crate (the only crate which is allowed to use LSP types). + +## IDE/Compiler split + +There's a semi-hard split between "compiler" and "IDE", at the `ra_hir` crate. +Compiler derives new facts about source code. +It explicitly acknowledges that not all info is available (ie, you can't look at types during name resolution). + +IDE assumes that all information is available at all times. + +IDE should use only types from `ra_hir`, and should not depend on the underling compiler types. +`ra_hir` is a facade. + # Logging Logging is done by both rust-analyzer and VS Code, so it might be tricky to -- cgit v1.2.3 From ee8dec5dc11cfecf219b6510b0eadd9691a82ba5 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Mon, 8 Jun 2020 12:52:28 +0200 Subject: IDE API --- docs/dev/README.md | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'docs/dev/README.md') diff --git a/docs/dev/README.md b/docs/dev/README.md index 903cb4055..64d595b68 100644 --- a/docs/dev/README.md +++ b/docs/dev/README.md @@ -268,6 +268,13 @@ IDE assumes that all information is available at all times. IDE should use only types from `ra_hir`, and should not depend on the underling compiler types. `ra_hir` is a facade. +## IDE API + +The main IDE crate (`ra_ide`) uses "Plain Old Data" for the API. +Rather than talking in definitions and references, it talks in Strings and textual offsets. +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. +The results are 100% Rust specific though. + # Logging Logging is done by both rust-analyzer and VS Code, so it might be tricky to -- cgit v1.2.3 From cc07c82fefb2affc1772e12b8357471cccc8d578 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Mon, 8 Jun 2020 12:54:48 +0200 Subject: Preconditions style --- docs/dev/README.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) (limited to 'docs/dev/README.md') diff --git a/docs/dev/README.md b/docs/dev/README.md index 64d595b68..5a9c0a148 100644 --- a/docs/dev/README.md +++ b/docs/dev/README.md @@ -241,6 +241,26 @@ struct Foo { For `.md` and `.adoc` files, prefer a sentence-per-line format, don't wrap lines. If the line is too long, you want to split the sentence in two :-) +## Preconditions + +Function preconditions should generally be expressed in types and provided by the caller (rather than checked by callee): + +```rust +// Good +fn frbonicate(walrus: Walrus) { + ... +} + +// Not as good +fn frobnicate(walrus: Option) { + let walrus = match walrus { + Some(it) => it, + None => return, + }; + ... +} +``` + # Architecture Invariants This section tries to document high-level design constraints, which are not -- cgit v1.2.3 From 4968321706fc5c24d9f2c35cdbbd5e9047c97c96 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Mon, 8 Jun 2020 13:19:32 +0200 Subject: Don't @ people in commit messages --- docs/dev/README.md | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'docs/dev/README.md') diff --git a/docs/dev/README.md b/docs/dev/README.md index 5a9c0a148..46ee030fc 100644 --- a/docs/dev/README.md +++ b/docs/dev/README.md @@ -261,6 +261,14 @@ fn frobnicate(walrus: Option) { } ``` +## Commit Style + +We don't have specific rules around git history hygiene. +Maintaining clean git history is encouraged, but not enforced. +We use rebase workflow, it's OK to rewrite history during PR review process. + +Avoid @mentioning people in commit messages, as such messages create a lot of duplicate notification traffic during rebases. + # Architecture Invariants This section tries to document high-level design constraints, which are not -- cgit v1.2.3 From e3663d60bfe1e8505e6db8bffab997b87be03913 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauren=C8=9Biu=20Nicola?= Date: Mon, 8 Jun 2020 18:50:27 +0300 Subject: Dev docs nits --- docs/dev/README.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'docs/dev/README.md') diff --git a/docs/dev/README.md b/docs/dev/README.md index 46ee030fc..0330939b6 100644 --- a/docs/dev/README.md +++ b/docs/dev/README.md @@ -55,7 +55,7 @@ You can run `cargo xtask install-pre-commit-hook` to install git-hook to run rus All Rust code lives in the `crates` top-level directory, and is organized as a single Cargo workspace. The `editors` top-level directory contains code for integrating with editors. Currently, it contains the plugin for VS Code (in -typescript). The `docs` top-level directory contains both developer and user +TypeScript). The `docs` top-level directory contains both developer and user documentation. 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 In general, I use one of the following workflows for fixing bugs and implementing features. -If the problem concerns only internal parts of rust-analyzer (ie, I don't need -to touch `rust-analyzer` crate or typescript code), there is a unit-test for it. +If the problem concerns only internal parts of rust-analyzer (i.e. I don't need +to touch `rust-analyzer` crate or TypeScript code), there is a unit-test for it. So, I use **Rust Analyzer: Run** action in VS Code to run this single test, and then just do printf-driven development/debugging. As a sanity check after I'm done, I use `cargo xtask install --server` and **Reload Window** action in VS @@ -279,7 +279,7 @@ always obvious from the low-level code. Syntax trees are by design incomplete and do not enforce well-formedness. If ast method returns an `Option`, it *can* be `None` at runtime, even if this is forbidden by the grammar. -## LSP indenpendence +## LSP independence rust-analyzer is independent from LSP. It provides features for a hypothetical perfect Rust-specific IDE client. @@ -289,7 +289,7 @@ Internal representations are lowered to LSP in the `rust-analyzer` crate (the on There's a semi-hard split between "compiler" and "IDE", at the `ra_hir` crate. Compiler derives new facts about source code. -It explicitly acknowledges that not all info is available (ie, you can't look at types during name resolution). +It explicitly acknowledges that not all info is available (i.e. you can't look at types during name resolution). IDE assumes that all information is available at all times. @@ -345,8 +345,8 @@ There's also two VS Code commands which might be of interest: rust code that it refers to and the rust editor will also highlight the proper text range. - If you press Ctrl (i.e. trigger goto definition) in the inspected - Rust source file the syntax tree read-only editor should scroll to and select the + If you trigger Go to Definition in the inspected Rust source file, + the syntax tree read-only editor should scroll to and select the appropriate syntax node token. ![demo](https://user-images.githubusercontent.com/36276403/78225773-6636a480-74d3-11ea-9d9f-1c9d42da03b0.png) -- cgit v1.2.3 From 1538206609dbb88f10ff2525523096bab0c65740 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Wed, 10 Jun 2020 14:12:08 +0200 Subject: Explain inline tests --- docs/dev/README.md | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'docs/dev/README.md') diff --git a/docs/dev/README.md b/docs/dev/README.md index 0330939b6..cf93135ee 100644 --- a/docs/dev/README.md +++ b/docs/dev/README.md @@ -303,6 +303,17 @@ Rather than talking in definitions and references, it talks in Strings and textu 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. The results are 100% Rust specific though. +## Parser Tests + +Test for parser (`ra_parser`) live in `ra_syntax` crate (see `test_data` direcotory). +There are two kinds of tests: + +* Manually written test cases in `parser/ok` and `parser/error` +* "Inline" tests in `parser/inline` (these are generated) from comments in `ra_parser` crate. + +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. +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. + # Logging Logging is done by both rust-analyzer and VS Code, so it might be tricky to -- cgit v1.2.3 From f280407d7350c2abc4cc35389d389acdc5d93e28 Mon Sep 17 00:00:00 2001 From: Jacek Generowicz Date: Wed, 10 Jun 2020 20:32:29 +0200 Subject: Fix parser test directory name in dev docs --- docs/dev/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'docs/dev/README.md') diff --git a/docs/dev/README.md b/docs/dev/README.md index cf93135ee..ef5ffbf59 100644 --- a/docs/dev/README.md +++ b/docs/dev/README.md @@ -308,7 +308,7 @@ The results are 100% Rust specific though. Test for parser (`ra_parser`) live in `ra_syntax` crate (see `test_data` direcotory). There are two kinds of tests: -* Manually written test cases in `parser/ok` and `parser/error` +* Manually written test cases in `parser/ok` and `parser/err` * "Inline" tests in `parser/inline` (these are generated) from comments in `ra_parser` crate. 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. -- cgit v1.2.3