From d605ec9c321392d9c7ee4b440c560e1e405d92e6 Mon Sep 17 00:00:00 2001 From: veetaha Date: Sun, 31 May 2020 05:13:08 +0300 Subject: Change Runnable.bin -> Runnable.kind As per matklad, we now pass the responsibility for finding the binary to the frontend. Also, added caching for finding the binary path to reduce the amount of filesystem interactions. --- docs/dev/lsp-extensions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'docs/dev') diff --git a/docs/dev/lsp-extensions.md b/docs/dev/lsp-extensions.md index dbc95be38..d06da355d 100644 --- a/docs/dev/lsp-extensions.md +++ b/docs/dev/lsp-extensions.md @@ -417,7 +417,7 @@ interface Runnable { /// The label to show in the UI. label: string; /// The following fields describe a process to spawn. - bin: string; + kind: "cargo" | "rustc" | "rustup"; args: string[]; /// Args for cargo after `--`. extraArgs: string[]; -- cgit v1.2.3 From a83ab820a4633bac718ee0fd11f06d1b3142be6b Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Tue, 2 Jun 2020 17:34:18 +0200 Subject: Spec better runnables --- docs/dev/lsp-extensions.md | 80 +++++++++++++++++++++++++--------------------- 1 file changed, 44 insertions(+), 36 deletions(-) (limited to 'docs/dev') diff --git a/docs/dev/lsp-extensions.md b/docs/dev/lsp-extensions.md index b7237ee90..647cf6107 100644 --- a/docs/dev/lsp-extensions.md +++ b/docs/dev/lsp-extensions.md @@ -311,6 +311,50 @@ Moreover, it would be cool if editors didn't need to implement even basic langua This is how `SelectionRange` request works. * Alternatively, should we perhaps flag certain `SelectionRange`s as being brace pairs? +## Runnables + +**Issue:** https://github.com/microsoft/language-server-protocol/issues/944 + +**Server Capability:** `{ "runnables": { "kinds": string[] } }` + +This request is send from client to server to get the list of things that can be run (tests, binaries, `cargo check -p`). + +**Method:** `experimental/runnables` + +**Request:** + +```typescript +interface RunnablesParams { + textDocument: TextDocumentIdentifier; + /// If null, compute runnables for the whole file. + position?: Position; +} +``` + +**Response:** `Runnable[]` + +```typescript +interface Runnable { + label: string; + /// If this Runnable is associated with a specific function/module, etc, the location of this item + location?: LocationLink; + /// Running things is necessary technology specific, `kind` needs to be advertised via server capabilities, + // the type of `args` is specific to `kind`. The actual running is handled by the client. + kind: string; + args: any; +} +``` + +rust-analyzer supports only one `kind`, `"cargo"`. The `args` for `"cargo"` look like this: + +```typescript +{ + workspaceRoot?: string; + cargoArgs: string[]; + executableArgs: string[]; +} +``` + ## Analyzer Status **Method:** `rust-analyzer/analyzerStatus` @@ -399,39 +443,3 @@ interface InlayHint { label: string, } ``` - -## Runnables - -**Method:** `rust-analyzer/runnables` - -This request is send from client to server to get the list of things that can be run (tests, binaries, `cargo check -p`). -Note that we plan to move this request to `experimental/runnables`, as it is not really Rust-specific, but the current API is not necessary the right one. -Upstream issue: https://github.com/microsoft/language-server-protocol/issues/944 - -**Request:** - -```typescript -interface RunnablesParams { - textDocument: TextDocumentIdentifier; - /// If null, compute runnables for the whole file. - position?: Position; -} -``` - -**Response:** `Runnable[]` - -```typescript -interface Runnable { - /// The range this runnable is applicable for. - range: lc.Range; - /// The label to show in the UI. - label: string; - /// The following fields describe a process to spawn. - kind: "cargo" | "rustc" | "rustup"; - args: string[]; - /// Args for cargo after `--`. - extraArgs: string[]; - env: { [key: string]: string }; - cwd: string | null; -} -``` -- cgit v1.2.3 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') 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 1f7de306f547ecb394a34445fd6ac1d6bc8ab439 Mon Sep 17 00:00:00 2001 From: Mikhail Rakhmanov Date: Wed, 3 Jun 2020 18:57:50 +0200 Subject: Add documentation --- docs/dev/lsp-extensions.md | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) (limited to 'docs/dev') diff --git a/docs/dev/lsp-extensions.md b/docs/dev/lsp-extensions.md index 647cf6107..7f7940d0b 100644 --- a/docs/dev/lsp-extensions.md +++ b/docs/dev/lsp-extensions.md @@ -97,6 +97,30 @@ Invoking code action at this position will yield two code actions for importing * Is a fixed two-level structure enough? * Should we devise a general way to encode custom interaction protocols for GUI refactorings? +## Lazy assists with `ResolveCodeAction` + +**Issue:** https://github.com/microsoft/language-server-protocol/issues/787 + +**Client Capability** `{ "resolveCodeAction": boolean }` + +If this capability is set, the assists will be computed lazily. Thus `CodeAction` returned from the server will only contain `id` but not `edit` or `command` fields. The only exclusion from the rule is the diagnostic edits. + +After the client got the id, it should then call `experimental/resolveCodeAction` command on the server and provide the following payload: + +```typescript +interface ResolveCodeActionParams { + id: string; + codeActionParams: lc.CodeActionParams; +} +``` + +As a result of the command call the client will get the respective workspace edit (`lc.WorkspaceEdit`). + +### Unresolved Questions + +* Apply smarter filtering for ids? +* Upon `resolveCodeAction` command only call the assits which should be resolved and not all of them? + ## Parent Module **Issue:** https://github.com/microsoft/language-server-protocol/issues/1002 -- 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') 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 da7ec4b3398ffaf672a755bf57066e17ac42303a Mon Sep 17 00:00:00 2001 From: vsrs Date: Wed, 3 Jun 2020 14:34:11 +0300 Subject: Add hover actions LSP extension documentation. --- docs/dev/lsp-extensions.md | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) (limited to 'docs/dev') diff --git a/docs/dev/lsp-extensions.md b/docs/dev/lsp-extensions.md index 7f7940d0b..a0847dad3 100644 --- a/docs/dev/lsp-extensions.md +++ b/docs/dev/lsp-extensions.md @@ -467,3 +467,41 @@ interface InlayHint { label: string, } ``` + +## Hover Actions + +**Client Capability:** `{ "hoverActions": boolean }` + +If this capability is set, `Hover` request returned from the server might contain an additional field, `actions`: + +```typescript +interface Hover { + ... + actions?: CommandLinkGroup[]; +} + +interface CommandLink extends Command { + /** + * A tooltip for the command, when represented in the UI. + */ + tooltip?: string; +} + +interface CommandLinkGroup { + title?: string; + commands: CommandLink[]; +} +``` + +Such actions on the client side are appended to a hover bottom as command links: +``` + +-----------------------------+ + | Hover content | + | | + +-----------------------------+ + | _Action1_ | _Action2_ | <- first group, no TITLE + +-----------------------------+ + | TITLE _Action1_ | _Action2_ | <- second group + +-----------------------------+ + ... +``` \ No newline at end of file -- 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') 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') 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') 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') 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') 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') 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') 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') 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