internal: Add Cargo-style project discovery for Buck and Bazel Users #14307

davidbarsky · 2023-03-09T20:23:30Z

This feature requires the user to add a command that generates a rust-project.json from a set of files. Project discovery can be invoked in two ways:

At extension activation time, which includes the generated rust-project.json as part of the linkedProjects argument in InitializeParams.
Through a new command titled "rust-analyzer: Add current file to workspace", which makes use of a new, rust-analyzer-specific LSP request that adds the workspace without erasing any existing workspaces. Note that there is no mechanism to remove workspaces other than "quit the rust-analyzer server".

Few notes:

I think that the command-running functionality could merit being placed into its own extension (and expose it via extension contribution points) to provide build-system idiomatic progress reporting and status handling, but I haven't (yet) made an extension that does this nor does Buck expose this sort of functionality.
This approach would just work for Bazel. I'll try and get the tool that's responsible for Buck integration open-sourced soon.
On the testing side of things, I've used this in around my employer's Buck-powered monorepo and it's a nice experience. That being said, I can't think of an open-source repository where this can be tested in public, so you might need to trust me on this one.

I'd love to get feedback on:

Naming of LSP extensions/new commands. I'm not too pleased with how "rust-analyzer: Add current file to workspace" is named, in that it's creating a new workspace. I think that this command being added should be gated on rust-analyzer.discoverProjectCommand on being set, so I can add this in sequent commits.
My Typescript. It's not particularly good.
Suggestions on handling folders with both Cargo and non-Cargo build systems and if I make activation a bit better.

(I previously tried to add this functionality entirely within rust-analyzer-the-LSP server itself, but matklad was right—an extension side approach is much, much easier.)

crates/project-model/src/cfg_flag.rs

editors/code/package.json

crates/rust-analyzer/src/handlers.rs

editors/code/package.json

Veykril · 2023-03-10T13:52:54Z

I feel like an addProject extension isn't the right tool here, instead I think the client should track all linked projects (including those of virtual project-json's) itself and tell the server to reload with the fresh set of supplied projects on change of these projects.

cc #13446

davidbarsky · 2023-03-10T15:48:42Z

I feel like an addProject extension isn't the right tool here, instead I think the client should track all linked projects (including those of virtual project-json's) itself and tell the server to reload with the fresh set of supplied projects on change of these projects.

I can see how that approach is better. I think it'd allow for a Buck/Bazel extension to manage linked projects and (potentially) surface them through a UI and mapping each linked project to a BUILD/BUCK file. One question, though: linkedProjects only gets sent once, during the initial server setup as a part of InitializeParams. I don't think it's a good idea to fully reinitialize r-a on each reload, but I can change rust-analyzer/reload to accept linkedProjects (or even a full ConfigData!) instead of just (). Is that approach okay with you?

Veykril · 2023-03-10T16:09:41Z

Yes, I forgot to add that, we'd still need this to be a request, repurposing reloadWorkspace sounds good.

or even a full ConfigData

LSP already has a configuration changed notification so we could also re-use this and just update the linked projects key there. The main problem here is figuring out how to shove an "inline" rust project specification into there since right not this takes a list of file paths only

davidbarsky · 2023-03-10T16:28:42Z

Yes, I forgot to add that, we'd still need this to be a request, repurposing reloadWorkspace sounds good.

or even a full ConfigData

LSP already has a configuration changed notification so we could also re-use this and just update the linked projects key there. The main problem here is figuring out how to shove an "inline" rust project specification into there since right not this takes a list of file paths only

Thankfully, we don't need to figure this out! linkedProjects already accepts an inline rust-project. It's what I'm doing in this PR (sorry, I've spent a lot of time staring at this part of the codebase...)

Just to make sure I understand, we want to do:

Send a configuration changed notification to r-a.
Send the actual, full new configuration (with the updated linkedProjects).

davidbarsky · 2023-03-11T00:45:06Z

I've added a workspace configuration-based approach to reloading the workspace in cef2d62. I haven't removed the rust-analyzer/AddProject-based approach yet because the way I've implementing reloading feels like an abuse of vscode.WorkspaceConfiguration. Let me know what approach y'all prefer: I'm happy to change this PR accordingly.

This feature requires the user to add a command that generates a `rust-project.json` from a set of files. Project discovery can be invoked in two ways: 1. At extension activation time, which includes the generated `rust-project.json` as part of the linkedProjects argument in InitializeParams 2. Through a new command titled "Add current file to workspace", which makes use of a new, rust-analyzer specific LSP request that adds the workspace without erasing any existing workspaces. I think that the command-running functionality _could_ merit being placed into its own extension (and expose it via extension contribution points), if only provide build-system idiomatic progress reporting and status handling, but I haven't (yet) made an extension that does this.

Co-authored-by: Lukas Wirth <lukastw97@gmail.com>

…to avoid failing tests

…iguration has changed

davidbarsky · 2023-03-13T19:48:03Z

I think I've addressed all the comments:

I've rebased against the master branch.
I've removed rust-analyzer/addProject in favor of notifying the server that configuration has changed. Note that the rust-project.json is never written to disk; it's synthesized entirely in-memory and placed into the InitializeParams. Actually updating VS Code's configuration would result in writing to disk into the workspace-level .vscode/ folder, but I think exposing this detail to users is a poor UX.

Veykril · 2023-03-13T19:52:37Z

Will take a proper look tomorrow but

Actually updating VS Code's configuration would result in writing to disk into the workspace-level .vscode/ folder, but I think exposing this detail to users is a poor UX.

fully agree with this, we should not be changing the user config. We only need to synthesize the linkedProjects field when passing it to the server, this way we could in the future also expose an API for the vscode extension that other extensions can hook into (which I think is what matklad was describing in the other issue at some point)

davidbarsky · 2023-03-13T20:20:52Z

Will take a proper look tomorrow

No worries, take care!

fully agree with this, we should not be changing the user config. We only need to synthesize the linkedProjects field when passing it to the server, this way we could in the future also expose an API for the vscode extension that other extensions can hook into

Yup, that's the approach that I took here. It seems like the config was written out to the workspace-level .vscode/ when key was updated in vscode.WorkspaceConfiguration; there doesn't seem to be a way to keep that in the extension alone.

which I think is what matklad was describing in the other issue at some point

He was. I tried a different approach, and when that I didn't work out, I was able to get this approach working in about 2 days. I'm a bit annoyed with myself that I didn't listen to him first!

editors/code/src/config.ts

editors/code/src/client.ts

Veykril

One more thing

editors/code/src/client.ts

editors/code/src/ctx.ts

Veykril · 2023-03-14T17:06:27Z

I am not sure whether this is the right way we want to tackle this (that is having this command be part of r-a in long term), but for the time being I'd say that's okay.

davidbarsky · 2023-03-14T17:28:46Z

I am not sure whether this is the right way we want to tackle this (that is having this command be part of r-a in long term), but for the time being I'd say that's okay.

My feeling is that I'd like to get some user feedback first, but I strongly suspect that this should live in an external extension that rust-analyzer coordinates with. I don't expect rust-analyzer to worry about maintaining this for an extended period of time.

davidbarsky · 2023-03-14T17:45:45Z

Sorry about the back and forth, but thank you for your patience :)

Veykril · 2023-03-14T17:48:42Z

@bors r+

bors · 2023-03-14T17:48:44Z

📌 Commit 6e7bc07 has been approved by Veykril

It is now in the queue for this repository.

bors · 2023-03-14T17:48:52Z

⌛ Testing commit 6e7bc07 with merge c15335c...

bors · 2023-03-14T18:01:32Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing c15335c to master...

woody77 · 2023-04-07T23:35:12Z

When is the discovery command run? Is it run in lieu of opening a rust-project.json file?

davidbarsky · 2023-04-08T15:50:11Z

When is the discovery command run? Is it run in lieu of opening a rust-project.json file?

@woody77 At the moment, it can run in two situations:

when the extension is activating (from either an editor restart or a Rust file being opened for the first time), or
when the user runs the "Add Project to Current Workspace" command through the command palette.

To be entirely honest, I think I was a bit too conservative with the latter option. I think that we should be probably be running the discovery command each time the user opens a Rust file that isn't part of the current workspace—rust-analyzer is stupidly fast at loading projects, and getting the results from Buck takes a few seconds at worst.

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 9, 2023

bjorn3 reviewed Mar 9, 2023

View reviewed changes

crates/project-model/src/cfg_flag.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Mar 9, 2023

View reviewed changes

editors/code/package.json Outdated Show resolved Hide resolved

Veykril reviewed Mar 10, 2023

View reviewed changes

crates/rust-analyzer/src/handlers.rs Outdated Show resolved Hide resolved

editors/code/package.json Outdated Show resolved Hide resolved

Veykril reviewed Mar 10, 2023

View reviewed changes

editors/code/package.json Outdated Show resolved Hide resolved

davidbarsky and others added 8 commits March 13, 2023 13:30

fmt

46e0220

remove errant -- in executeDiscoverProject

1f5c535

fix typo in package.json.

68d3eae

Update crates/rust-analyzer/src/handlers.rs

7a6e111

Co-authored-by: Lukas Wirth <lukastw97@gmail.com>

move rust-analyzer.discoverProjectCommand above $generated-start …

9137149

…to avoid failing tests

Add a workspace config-based approach to reloading discovered projects.

8d9bff0

Remove rust-analyzer/addProject in favor of notifying r-a that conf…

56273b3

…iguration has changed

davidbarsky force-pushed the davidbarsky/add-cargo-style-project-discovery-for-buck-and-bazel-sickos branch from ecc1de5 to 56273b3 Compare March 13, 2023 17:30

davidbarsky added 2 commits March 13, 2023 13:33

fmt

cb93c12

rename "addProject" command

78aed30

davidbarsky requested a review from Veykril March 13, 2023 19:48

davidbarsky mentioned this pull request Mar 13, 2023

VSCode now supports SnippetTextEdits natively #13229

Closed

Veykril reviewed Mar 14, 2023

View reviewed changes

editors/code/src/config.ts Show resolved Hide resolved

editors/code/src/client.ts Outdated Show resolved Hide resolved

Address review comments

bd545a1

davidbarsky requested a review from Veykril March 14, 2023 16:51

Veykril requested changes Mar 14, 2023

View reviewed changes

editors/code/src/client.ts Show resolved Hide resolved

editors/code/src/ctx.ts Show resolved Hide resolved

fix: don't override linkedProjects if no workspace was discovered.

6e7bc07

Veykril approved these changes Mar 14, 2023

View reviewed changes

bors merged commit c15335c into rust-lang:master Mar 14, 2023

davidbarsky deleted the davidbarsky/add-cargo-style-project-discovery-for-buck-and-bazel-sickos branch March 14, 2023 18:19

lnicola changed the title ~~Add Cargo-style project discovery for Buck and Bazel Users~~ internal: Add Cargo-style project discovery for Buck and Bazel Users Mar 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal: Add Cargo-style project discovery for Buck and Bazel Users #14307

internal: Add Cargo-style project discovery for Buck and Bazel Users #14307

davidbarsky commented Mar 9, 2023

Veykril commented Mar 10, 2023 •

edited

Loading

davidbarsky commented Mar 10, 2023 •

edited

Loading

Veykril commented Mar 10, 2023

davidbarsky commented Mar 10, 2023 •

edited

Loading

davidbarsky commented Mar 11, 2023 •

edited

Loading

davidbarsky commented Mar 13, 2023

Veykril commented Mar 13, 2023

davidbarsky commented Mar 13, 2023 •

edited

Loading

Veykril left a comment

Veykril commented Mar 14, 2023

davidbarsky commented Mar 14, 2023

davidbarsky commented Mar 14, 2023

Veykril commented Mar 14, 2023

bors commented Mar 14, 2023

bors commented Mar 14, 2023

bors commented Mar 14, 2023

woody77 commented Apr 7, 2023

davidbarsky commented Apr 8, 2023

internal: Add Cargo-style project discovery for Buck and Bazel Users #14307

internal: Add Cargo-style project discovery for Buck and Bazel Users #14307

Conversation

davidbarsky commented Mar 9, 2023

Veykril commented Mar 10, 2023 • edited Loading

davidbarsky commented Mar 10, 2023 • edited Loading

Veykril commented Mar 10, 2023

davidbarsky commented Mar 10, 2023 • edited Loading

davidbarsky commented Mar 11, 2023 • edited Loading

davidbarsky commented Mar 13, 2023

Veykril commented Mar 13, 2023

davidbarsky commented Mar 13, 2023 • edited Loading

Veykril left a comment

Choose a reason for hiding this comment

Veykril commented Mar 14, 2023

davidbarsky commented Mar 14, 2023

davidbarsky commented Mar 14, 2023

Veykril commented Mar 14, 2023

bors commented Mar 14, 2023

bors commented Mar 14, 2023

bors commented Mar 14, 2023

woody77 commented Apr 7, 2023

davidbarsky commented Apr 8, 2023

Veykril commented Mar 10, 2023 •

edited

Loading

davidbarsky commented Mar 10, 2023 •

edited

Loading

davidbarsky commented Mar 10, 2023 •

edited

Loading

davidbarsky commented Mar 11, 2023 •

edited

Loading

davidbarsky commented Mar 13, 2023 •

edited

Loading