Add multi project support vscode with single server #3987

jannickj · 2020-04-15T21:39:17Z

Redesign based on discussion from: #3972

Redesigned multi project support entails:

Check all open .rs files for a Cargo.toml and start Rust-Analyzer with all their roots at activation time
Attach hooks on onDidOpenTextDocument and inform user to restart if a new Rust Project have been found (with option to auto restart Rust-Analyzer).
All the hassle with running multiple Language Servers is gone (thank god)

Some caveats

I couldn't find an option in VSCode to specify the root folders, so I had to add a CLI flag for Rust-Analyzer to override it, it could also be an ENV for a less intrusive design. (That said if someone finds the proper way git revert 53ed3831dd296edd660aeea7d6508e53f263c650 should get rid of this change)
"Check all open .rs files" only works on files the user have opened in that session, so starting a new VScode with 100s of files will only activate Rust-Analyzer for the file that is active. (see: API Access to "Open Editors" microsoft/vscode#15178)
This rust analyzer repo have a Cargo.toml in the root, with this implementation that would be ignored since every project in crates/* have their own Cargo.toml which would take priority, I did try adding both the projects Cargo.toml and the root's Cargo.toml. But that seems to make Rust-Analyzer generate double outputs on highlight.
For each newly found project to be added to the rust analyzer it will have to restart, making the load time O(n^2). That said you can always just open one file for each project and restart Rust-Analyzer once.

Future work

Finish Dynamic Project Load Better VFS #3715
One thing I found annoying is the lack of feedback when Rust-Analyzer is loading, this becomes even more of an issue when it loads more.

… I was unable to set workspace folders in vscode. can be removed if solution is found.

…hat the folder actually contains the file.

…new document is opened. One caveat is vscode.workspace.textDocuments only provides the documents the user have clicked on: Issue: microsoft/vscode#15178

jondo2010 · 2020-04-16T07:55:09Z

Does this do anything to address #2001?

jannickj · 2020-04-16T07:59:40Z

@jondo2010 It should fix #2001 but would be great to test which I haven't done :)
The reason why it should be fixed is that this will go look for Cargo.toml and only spin up the language server for folders that contains the Cargo.toml

davidolrik · 2020-04-16T09:34:42Z

I couldn't find an option in VSCode to specify the root folders

I believe that there are some variables for that, like ${workspaceFolder}, is that what you are looking for?

jannickj · 2020-04-16T10:30:15Z

@davidolrik it only takes a single folder the parameter I need to set is for multiple folders it's part of the language server spec (microsoft/language-server-protocol#281) but it's unavailable in the vscode api

bjorn3 · 2020-04-16T10:39:51Z

editors/code/package.json

                    "default": true,
                    "description": "Whether to ask for permission before downloading any files from the Internet"
                },
+                "rust-analyzer.server.autoRestartOnNew": {


Suggested change

"rust-analyzer.server.autoRestartOnNew": {

"rust-analyzer.server.autoRestartOnNewProjectFound": {

matklad

Took a look. I must say this feels like a pretty big change to how everything works, which departs a lot from what is already implemented/what I envision, so it was a bit hard to review for me :)

The big piece is that we should strive to keep almost everything in Rust. I think the only thing here which genuinely belongs to the editor is didChangeWorkspaceFolders event, for everything else, I don't see a fundamental reason why it can't be done at the editor's side.

I still don't fully understand what specific use-cases this covers, but here's my ideas about what we can do to make handing multi-projects better

our current discovery scheme is very limited. For each ws folder, we look for Cargo.toml only up to one level deep. This handles auto-discovery for some projects, but not for all of them. We don't do full auto-discovery primarily because our current VFS is limited; but I also think that unconditional crawl of all files in the project is probably a bad idea. So, the two action items here are:
- add a cargoTomls: [Path] configuration to completely disable auto-discovery of Cargo tomls in favor of specified values. This should be an option in Config, which is fetched from initialization options
- for the case where we open rust-file deep in the project (due to onLangauge activation event), I think we can pass these file to the server as well (via the same initialization options), such that the server can crawl upwards from the file. Crawling upwards should always be OK. This is roughtly the logic that is implemented here, but it absolutely should be on the server's side. I wonder if implementing this would allow us to remove the one-level crawl? Might be a good idea to return to the original PR and check if those use-cases are covered.
we currently don't notice workspace structure changes
- server-side, we should monitor for changes in Cargo.toml, Cargo.lock and implicit targets like src/bin/foo.rs (the correct definition of implicit targets can be copy-pasted from IntelliJ) and ask the clients to show reload messages
- client side, we should monitor for the changes in workspace folders, and show the "needs reload" message (or, ideally, some kind of birhgt-red persistent status in the status bar)

matklad · 2020-04-16T10:27:06Z

editors/code/src/main.ts

 import { activateTaskProvider } from './tasks';

 let ctx: Ctx | undefined;
+const foundProjects: Set<string> = new Set();


As a general comment, global singletons should not be added without very good reason.

100% agree,
I only used it here because it's not meant be shared outside main, and it didn't fit in Ctx since this is meant to live after a restart.

matklad · 2020-04-16T10:29:17Z

crates/rust-analyzer/src/bin/args.rs


 impl Args {
+    fn get_roots(mut matches: Arguments) -> Result<Option<Vec<String>>> {
+        if matches.contains("--roots") {


Adding a command line option, given that this is the first command line option, massively changes the interface of the program. It's much better to stick to existing conventions, which, in this case, are the initializationOptions (not sure about the naming) field of the initialize requst

massively changes the interface of the program

As I said I couldn't find a way to send it via the initialization options in vscode. It also shouldn't be a massive change to the interface since it's a completely optional parameter but I can see how it feels intrusive.

matklad · 2020-04-16T10:30:15Z

editors/code/src/main.ts

+
+                foundProjects.add(cargoRoot.fsPath);
+                if (isMissing) {
+                    if (config?.autoRestartOnNew) {


My experience in IntelliJ tells me that auto restart doesn't worth the trouble. The problem is that we should only restart in quiescent state, and only the user knows when this state is reached.

Ie, setting a status "needs restart" is absolutely necessary, and a big missing piece in rust-analyzer at the moment, but it should be the user who decides to click the restart button.

I added it after testing it a bit, since it got very old very quickly having to restart manually all the time, which is also why it's turned off by default.

matklad · 2020-04-16T10:31:24Z

editors/code/src/util.ts

+}
+
+// searches up the folder structure until it finds a Cargo.toml
+export async function nearestParentWithCargoToml(


We already have this implemented in rust (find_cargo_toml in ra_project_model), and I think we should strive for keeping as much as possible in rust, as it is sharable between the editors then.

@matklad

Took a look. I must say this feels like a pretty big change to how everything works, which departs a lot from what is already implemented/what I envision, so it was a bit hard to review for me :)

Not sure what it takes away from what is already implemented / what visions it's breaking?
Is it that nothing can be done on the client side?

The big piece is that we should strive to keep almost everything in Rust. I think the only thing here which genuinely belongs to the editor is didChangeWorkspaceFolders event, for everything else, I don't see a fundamental reason why it can't be done at the editor's side.

That's a good point, I saw this as a very minor change to improve QoL for vscode, but I can see how it might be better to do 100% server side for many other reasons. It took me a couple of hours to code up (mostly reading vscode issues). If that's what is ruining this pr for you I could redo it in the server, however there it will actually affect all users of it (but maybe that is what we want?).

still don't fully understand what specific use-cases this covers, but here's my ideas about what we can do to make handing multi-projects better

There are many use-cases

You have a large project where rust is in a deep subfolder

You open a random file from another project (outside your workspace)

You have many projects in one folder which depend on each other (ala crates)

jannickj · 2020-04-16T11:39:06Z

I can reimplement this PR on the Rust-Analyzer server(no more typescripting :D), but I am actually curious of is: Is this the behavior one would expect from a UX perspective?

matklad · 2020-04-16T12:59:13Z

Is this the behavior one would expect from a UX perspective?

Yeah, I think we want all of these things:

finding Cargo.toml as an ancestor of a file which brought the extension to life.
notification/status message that rust-analyzer might need a reload, if we detect that the project structure might have changed.
ability to explicitly configure the set of root Cargo.toml.

Implementation wise, I'd probably suggested to start with 3, adding rust-analyzer.cargoProjects: string[] config variable which is used instead of workspace folders, if specified. Should be the easiest one to add, and should give a feeling for all the moving parts involve.

Than it makes sense to tackle 1, as it is somewhat tricky, but also I think will solve the largest practical issue.

jannickj · 2020-04-16T15:05:59Z

start with 3, adding rust-analyzer.cargoProjects: string[] config variable which is used instead of workspace folders

I still haven't been able to find an option in vscode to override the workspace_roots which are being sent to the language server, and I expect it's not exposed anywhere. This was the reason I created the --roots flag on the Rust-Analyzer service, and you seemed to really dislike that so I'm not sure how to proceed here without an ability to set the roots on the server?

Also wouldn't it be cooler to have a file(ex. .rust-analyzer) which can override the settings, such that you can have your workspace setup for a specific project instead of having to update your plugin settings everytime? Then it would also be a server feature instead of a vscode feature(your vision)

matklad · 2020-04-16T16:48:28Z

I still haven't been able to find an option in vscode to override the workspace_roots which are being sent to the language server, and I expect it's not exposed anywhere.

I think VS Code workspace roots and the cargo projects need not to be one-to-one. For example, I can imagine a setup where a single VS Code workspace folder actually contains several Cargo workspaces. For that reason, we don't need to override the workspace folders, rather, we need a separate, rust-analyzer specific config for specifying the set of Cargo.toml.

This config needs to be added in three places:

The config value is available early at the server lifetime, and can be used to configure the initial workspaces: https://github.com/rust-analyzer/rust-analyzer/blob/fc0a47a0c19a1b58a586e30645cec4d80d700513/crates/rust-analyzer/src/main_loop.rs#L67

Also wouldn't it be cooler to have a file(ex. .rust-analyzer) which can override the settings, such that you can have your workspace setup for a specific project instead of having to update your plugin settings everytime?

yeah, that would be ideal, but this really should be a protocol feature. We rely on standard LSP way to pass configuration, and that is specked to be editor-specific.

jannickj · 2020-04-16T18:09:09Z

Either I completely misunderstand or what you're proposing would not to solve the problem I am trying to solve.

The way I initially understood your config proposal was that it would be super niche for people with very specific setup and not the normal way to use rust-analyzer. If the config is meant to be a sort of override then I completely agree.

However the bread and butter I want to achieve with this change is Dynamic Rust-Analyzer Project Loading, working in whatever shape the files are laid out. Once that is in place I don't mind making an override config.

I have written some user stories to make it more clear what problems I am trying to solve and why, I made them colorful so it's easier to feel the pain :)

Imagine this scenario:
I have a repository with

MegaRepo
|- [deep path 1]
   |- Rust Project A
   |- Rust Project B
|- [deep path 2]
   |- Python Project

and

[Some random repo I just pulled]
|- [deep path 3]
   |- Rust Project C

Situation A:

I am working on the python project and I want to quickly make some quick changes to the rust projects A & B.

Situation B:

I just pulled down some repo and want to quickly make some changes to it.

How it is today

Situation A:

With workspace set to MegaRoot, you open a file from project A and are met with
"rust-analyzer failed to load workspace: Failed to find Cargo.toml for path ..."
With workspace set to MegaRoot/[deep path 1], you open a file from project A and are met with
"rust-analyzer failed to load workspace: Failed to find Cargo.toml for path, due to multiple equally valid Cargo.toml files found: "
With workspace set to MegaRoot/[deep path 1]/Rust Project A, it works, but now you need to either keep two editors open, one for Project A and One for project B or go without tool. This is super Annoying.

Situation B:

(Same story as A)

How it is after this PR

Situation A:

Without auto restart, and with workspace set to MegaRoot, you open a file from project A and are met with "RA needs a restart", you type in Ctrl+P + "Rust-Analyzer: Restart" + Enter, 2 sec later it just works. you open a file from project B, same thing a before you type in Ctrl+P + "Rust-Analyzer: Restart" wait two seconds again.
With auto restart, and with workspace set to MegaRoot, you open a file from project A and are met with "RA restarting", 2 sec later it just works. you open a file from project B, 2 sec later it just works.

Situation B:

(Same story as B)

If config was the main way to work with Rust-Analyzer

Situation A:

With workspace set to MegaRoot, you open a file from project A and are met with
"rust-analyzer failed to load workspace: Failed to find Cargo.toml for path ...", you go in
to configs and add `cargoProjects: [...[tons of project..], [deep path 1]/Rust Project A/Cargo.toml", you type in Ctrl+P + "Rust-Analyzer: Restart" + Enter, 2 sec later it's ready. you open a file from project B, Same error again you scroll through configs again add this project path, restart the service another time. That said from now on your setup just works... until...

Situation B:

You haven't thought about rust-analyzer in a while because now it just works, ooh you have this new project you need to fix, "rust-analyzer failed to load workspace: Failed to find Cargo.toml for path ...", ohh why is this?.. 2 hours later scrolling through github issues, and other comments, ahh right there was this manual config I have to set for new project.. (Same story as A)

matklad · 2020-04-16T19:19:55Z

Yes, the config is for a very niche use-case, but it makes sense to start with it implementation wise, as it will clearly separate the discovery aspect from project loading aspect. Additionally, I suspect the config might turn out to be the right tool to persist workspace configuration between restarts. You probably don't want to repeat the whole reloading dance every time you open the project.

jannickj · 2020-04-16T19:40:52Z

Ah right, that's fair, I guess there are some outstanding issues around how to deal with persisting the project discoveries.

That said I am willing to slay that horse when I've created the "config setup" PR, and I've gained some credit.

matklad · 2020-04-16T20:34:15Z

Submitted #3995 which hopefully makes the control flow in this area a little clearer. It also removes the "multiple equally valid Cargo.toml files" error, which existed simply because code was split into functions across wrong boundaries.

matklad · 2020-04-17T13:14:17Z

сс #2649

matklad · 2020-06-02T08:41:49Z

Closing due to inactivity

jannickj · 2020-06-03T05:17:03Z

Yeah my bad been quite busy lately I will see if I can find some time soon to get this done :)

jannickj added 7 commits April 15, 2020 23:09

create helper to find the location of Cargo.toml root folder

da8099b

add an "override" roots flag to RA-server, shouldn't be necessary but…

53ed383

… I was unable to set workspace folders in vscode. can be removed if solution is found.

instead of returning root folder if it contains a Cargo.toml, check t…

a5da352

…hat the folder actually contains the file.

add functionally to locate rust projects on startup and every time a …

22d1495

…new document is opened. One caveat is vscode.workspace.textDocuments only provides the documents the user have clicked on: Issue: microsoft/vscode#15178

add option for automatically restart the rust-analyzer server

91e552b

fix tasks to work for all projects

d328a01

apply linting

b545a22

jannickj mentioned this pull request Apr 15, 2020

Add multi project support vscode #3972

Closed

bjorn3 reviewed Apr 16, 2020

View reviewed changes

matklad reviewed Apr 16, 2020

View reviewed changes

matklad closed this Jun 2, 2020

	"rust-analyzer.server.autoRestartOnNew": {
	"rust-analyzer.server.autoRestartOnNewProjectFound": {

Add multi project support vscode with single server #3987

Add multi project support vscode with single server #3987

Uh oh!

Conversation

jannickj commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Redesigned multi project support entails:

Some caveats

Future work

Uh oh!

jondo2010 commented Apr 16, 2020

Uh oh!

jannickj commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidolrik commented Apr 16, 2020

Uh oh!

jannickj commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matklad left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannickj Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannickj commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matklad commented Apr 16, 2020

Uh oh!

jannickj commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matklad commented Apr 16, 2020

Uh oh!

jannickj commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Situation A:

Situation B:

How it is today

Situation A:

Situation B:

How it is after this PR

Situation A:

Situation B:

If config was the main way to work with Rust-Analyzer

Situation A:

Situation B:

Uh oh!

matklad commented Apr 16, 2020

Uh oh!

jannickj commented Apr 16, 2020

Uh oh!

matklad commented Apr 16, 2020

Uh oh!

matklad commented Apr 17, 2020

Uh oh!

matklad commented Jun 2, 2020

Uh oh!

jannickj commented Apr 15, 2020 •

edited

Loading

jannickj commented Apr 16, 2020 •

edited

Loading

jannickj commented Apr 16, 2020 •

edited

Loading

matklad left a comment •

edited

Loading

jannickj Apr 16, 2020 •

edited

Loading

jannickj commented Apr 16, 2020 •

edited

Loading

jannickj commented Apr 16, 2020 •

edited

Loading

jannickj commented Apr 16, 2020 •

edited

Loading