Skip to content

Conversation

@jannickj
Copy link

This pr adds multi project support for vscode.

Some time ago I added this for the RLS vscode plugin: rust-lang/vscode-rust#638
And since I had some time I decided to implement it for rust-analyzer aswell.

The design is simple (can be expanded upon):

  • every time you open a document it will locate the Cargo.toml file, and create an RA-server, if another document shares the same Cargo.toml then it will also share that RA-server, otherwise a new will be spawned.
  • Commands are shared but it will depend on the active Ctx.
  • The status display will also be created for each Ctx and will just be hidden/shown depending on which Ctx is in focus.
  • Tasks (Cargo build etc..) will run depending on the file you have open.

Some caveats / things that could be done better or different:

  • I added a CLI flag to RA-server to force it to use cwd
  • If a project contains other projects then there might be double reporting (needs more advanced document filter)
  • the current multi workspace impl is effectively not active anymore (for vscode)
  • I don't know if it would be favorable to run just 1 RA-server, but in that case there needs support for dynamic loading / unloading of workspaces.
  • This a lot, so I expect there to be some things I have overlooked, when I did it for the RLS I put this behind a feature flag, that said I've learned a lot from all the little things that came up on that project. But I am sure there are some things I've overlooked. And the design for this was much deeper given how much more complex RA is, so adding a 1000+ conditions on whether this is active or not is probably a bad Idea.

Also I haven't been part of any design discussions so if this had already been decided against for other reasons that's fair :)

@Ten0
Copy link

Ten0 commented Apr 14, 2020

Hmm... I was browsing through the PRs after opening my issue. Does this fix #3975 through the --use-cwd parameter, or is the forwarding-to-workspace done later ?

@jannickj
Copy link
Author

jannickj commented Apr 14, 2020

Hi @Ten0, I'll try to outline how it will work in more detail, hope this will clarify it, anyway here goes:

workspace with multiple projects side by side

If your structure is:

my_workspace
|- project_a
   |- Cargo.toml
   |- src
|- project_b
   |- Cargo.toml
   |- src

Then it would work, by spinning up 2 RA-severs one with cwd=my_workspace/project_a and one with cwd=my_workspace/project_b, which should achieve what you want.

workspace with a project nested inside another project

If your structure is:

my_workspace
|- project_a
   |- Cargo.toml
   |- src
   |- project_b
      |- Cargo.toml
      |- src

Then it would work, by spinning up 2 RA-severs on with cwd=my_workspace/project_a and one with cwd=my_workspace/project_a/project_b, which would produce the double reporting i mentioned (which might be annoying, and look a little buggy :/, I think I have a solution for it but I want some feedback on this PR before I make any changes)

A workspace with it's own Cargo.toml and with other projects inside it (How the RA repo is structured)

Maybe this mode should just be removed

mega_project
|- Cargo.toml
|- crates
  |- project_a
    |- Cargo.toml
    |- src
  |- project_b
    |- Cargo.toml
    |- src
  ...

Then it would work, by spinning up 1 RA-severs with cwd=project_a, doing the same as the current RA-server implementation does today.
That said this design might be not preferable and a bit subtle, and I did think about removing this mode after I posted the PR but I wanted some feedback on it first.
The reason why this is a bit subtle is that if you for instance just open another folder in vscode such that the workspace structure is like:

my_workspace
|- mega_project
  |- Cargo.toml
  |- crates
    |- project_a
      |- Cargo.toml
      |- src
    |- project_b
      |- Cargo.toml
      |- src
    ...

Then it would see as just another form of the second setup and work by spinning up 3 RA-severs one with cwd=workspace/mega_project, one with cwd=workspace/mega_project/crates/project_a and the last withcwd=workspace/mega_project/crates/project_b .

@Ten0
Copy link

Ten0 commented Apr 14, 2020

My structure is a bit different ^^ :

mega_project
|- .cargo/workspace
  |- Cargo.toml
|- crates
  |- project_a
    |- Cargo.toml <-- These point to `../../.cargo/workspace` as project workspace
    |- src
  |- project_b
    |- Cargo.toml
    |- src

How would it behave in this scenario ? Optionally I could add the .cargo/workspace to the ignore path.

Somewhat-unrelated: I feel like the double reporting problem may be linked to this discussion: #2792

Somewhat-unrelated again: I feel like when opening a workspace root, we could spin up a single workspace check, because when the workspace is large, spinning up completely separate RA processes would eat an insane amount of physical resources.

@matklad
Copy link
Contributor

matklad commented Apr 15, 2020

I'll try to review this closely some time later today, but, generally, I think we should pursue a different, harder but more long-term viable approach (If I understood correctly what this PR doese).

We should support loading several workspaces into the single rust-analyzer process.

Hm, actually, looking at the code we do seem to support this?

https://github.com/rust-analyzer/rust-analyzer/blob/b495e56b0d3f7a72494d002a69440563e8394574/crates/rust-analyzer/src/main_loop.rs#L94-L121

https://github.com/rust-analyzer/rust-analyzer/blob/b495e56b0d3f7a72494d002a69440563e8394574/crates/rust-analyzer/src/bin/main.rs#L76-L82

Note that I don't use this funcitonality myslf, so it might very well be broken, but in that case, I think we should fix it (and maybe add a heavy test).

@jannickj
Copy link
Author

@matklad I actually do agree, I just saw the rewrite of RA's project selector as being too big a task for a one night project, but now that I've worked with it a bit maybe it's not too bad ;).

Anyway going back to topic, there are a lot of annoyances with multiple language servers in vscode, that said this does provide the kind of functionality one would expect, and it was quite easy to implement. But I completely understand if it's preferred to wait with this until it's available inside the language server.

@matklad
Copy link
Contributor

matklad commented Apr 15, 2020

Yeah, I guess there are two orthogonal problems here:

  • mulit-project setup (and I think rust-analyzer already fully supports that?)
  • dynamic reconfiguration support (which rust-analyzer lacks at the moment)

Dynamic reconfiguration, on the one hand, is a pretty thorny issue, but, on the other hand, the solution of "just" reloading the might not be that bad at the end of the day. The true reconfiguration is blocked on #3715 (or, more generally, on the investiagation of what dynamic root set means in the first place). However, I think we can get 80% there if we just show user a message "hey, we've noticed that you've edited Cargo.toml / added new workspace folder, you might want to reload rust-anlayzer".

Is this a fair summary of the situation? Or is there something I am overlooking, which makes multi-server setup a significantly better UX?

@jannickj
Copy link
Author

jannickj commented Apr 15, 2020

@matklad

Is this a fair summary of the situation? Or is there something I am overlooking, which makes multi-server setup a significantly better UX?

Yes, quite fair, with a caveat while multi-project is supported it's effectively not used in vscode (multi workspace is not very nice) that said I think it should be easy to get there. Also the multi-server solution proposed in this PR is really not a "good" solution other than it providing the "dynamic project loading", without much headache I agree 100% that the "would you like to restart?" is good enough and it expands better once dynamic project loading is ready :).

That said I agree that your proposal is a bit cleaner, I don't know the design process but I wouldn't mind implementing what you proposed:

  • Load Projects in workspace (not only root)
  • Dynamically detect new projects (and prompt user to reload RA)

@jannickj
Copy link
Author

I am closing this to make way for a new redesign on:
#3987

My reasoning is that there is no code overlap and instead of force pushing and making it more confusing I instead opened a new pr.

@jannickj jannickj closed this Apr 15, 2020
@jannickj jannickj deleted the add-multi-project-support-vscode branch April 15, 2020 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants