Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: teach rust-analyzer to discover linked_projects #17246

Conversation

davidbarsky
Copy link
Contributor

@davidbarsky davidbarsky commented May 16, 2024

This PR's been a long-time coming, but like the title says, it introduces server-side project discovery and removes the extension hooks I previously introduced. I don't think this PR is ready to land, but here are the things I'm feeling squishy about:

  • I don't think I like the idea of introducing the cargo-metadata command-but-for-everything-else in the flycheck module, but the progress reporting infrastructure was too convenient to pass up. Happy to move it elsewhere.

Here are the things I know I need to change:

  • For progress reporting, I'm extracting from a serde_json::Value that corresponds to tracing_subsciber::fmt::Layer's JSON output. I'd like to make this a bit more structured/documented than the current nonsense I wrote.
  • The progress reporting currently hardcodes "Buck"; it should be deriving that from the previously mentioned more-structured-output.
  • This doesn't handle reloading when a corresponding buildfile is changed. It should be doing that.
Anyway, here's a video of rust-analyzer discovering a Buck target.
rust-project.mov

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 16, 2024
@davidbarsky davidbarsky force-pushed the david/move-rust-project-generation-to-server branch 4 times, most recently from 378ef57 to e62932f Compare May 22, 2024 16:26
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't looked at the main_loop.rs and reload.rs changes yet, will do that tomorrow

if self.config.linked_or_discovered_projects() != old_config.linked_or_discovered_projects()
{
self.fetch_workspaces_queue.request_op("discovered projects changed".to_owned(), false)
self.fetch_workspaces_queue.request_op("discovered projects changed".to_owned(), true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing this we should probably make this an enum (I'm at fault for introducing a bool here and being lazy)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change this to an enum, since I'm doing a bunch of surgery here anyways: would something like (bad namesincoming!) ReloadBehavior { Force, Done } be a decent starting point?

crates/project-model/Cargo.toml Outdated Show resolved Hide resolved
crates/rust-analyzer/src/config.rs Outdated Show resolved Hide resolved
crates/rust-analyzer/src/config.rs Outdated Show resolved Hide resolved
crates/rust-analyzer/src/config.rs Outdated Show resolved Hide resolved
crates/rust-analyzer/src/lsp/utils.rs Outdated Show resolved Hide resolved
@Veykril
Copy link
Member

Veykril commented May 22, 2024

I don't think I like the idea of introducing the cargo-metadata command-but-for-everything-else in the flycheck module, but the progress reporting infrastructure was too convenient to pass up. Happy to move it elsewhere.

The flycheck crate is a mess now (at least name wise) anyways as it also does stuff for the test explorer, so that doesn't really make things a lot worse now...

@davidbarsky davidbarsky force-pushed the david/move-rust-project-generation-to-server branch 2 times, most recently from ace6d7d to 1003808 Compare May 22, 2024 21:22
@bors
Copy link
Collaborator

bors commented May 23, 2024

☔ The latest upstream changes (presumably #17287) made this pull request unmergeable. Please resolve the merge conflicts.

@davidbarsky davidbarsky force-pushed the david/move-rust-project-generation-to-server branch from 1003808 to 57fa458 Compare May 23, 2024 22:35
@bors
Copy link
Collaborator

bors commented May 24, 2024

☔ The latest upstream changes (presumably #17275) made this pull request unmergeable. Please resolve the merge conflicts.

@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 27, 2024
@davidbarsky davidbarsky force-pushed the david/move-rust-project-generation-to-server branch from 57fa458 to 1404d02 Compare May 29, 2024 20:58
@davidbarsky davidbarsky force-pushed the david/move-rust-project-generation-to-server branch from 1404d02 to ce2c55c Compare May 30, 2024 23:08
@bors
Copy link
Collaborator

bors commented Jun 1, 2024

☔ The latest upstream changes (presumably #17278) made this pull request unmergeable. Please resolve the merge conflicts.

@davidbarsky
Copy link
Contributor Author

For "reload rust-analyzer reasons when a TARGETS/BUCK file changes"; this PR is based atop of #16840.

facebook-github-bot pushed a commit to facebook/buck2 that referenced this pull request Jun 6, 2024
Summary: To support rust-lang/rust-analyzer#17246, I've added an argument for logging using JSON to that rust-analyzer can use these fields for progress reporting.

Reviewed By: Wilfred

Differential Revision: D57924551

fbshipit-source-id: 992e9e096fdea1eefe2b199d8dd84be7b247a732
@bors
Copy link
Collaborator

bors commented Jun 6, 2024

☔ The latest upstream changes (presumably #17346) made this pull request unmergeable. Please resolve the merge conflicts.

@davidbarsky davidbarsky force-pushed the david/move-rust-project-generation-to-server branch 3 times, most recently from 93e6138 to 7d6408a Compare June 10, 2024 20:51
@bors
Copy link
Collaborator

bors commented Jun 11, 2024

☔ The latest upstream changes (presumably #17374) made this pull request unmergeable. Please resolve the merge conflicts.

@davidbarsky
Copy link
Contributor Author

Ugh I forgot about this sorry

@bors delegate+

Merge after you rebase it once more (though do wait until monday's release just to be safe should this break anything accidentally)

No worries! I've got a four-day weekend, so I don't expect to open GitHub on my computer :). I'll add some proper serde deserialization that is more independent of tracing's output and then merge this on Monday or Tuesday.


#[derive(PartialEq, Clone, Debug, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
pub enum Arguments {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest DiscoverArgument or similar. This gets used below as project_json::Arguments which is not entirely self-explanatory. And since this is the interface that other build systems will program their project discovery tools against, needs some docs -- what does RA want them to do, roughly, for Path vs Label?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i'll add those docs today. thanks!

(the tl;dr is that it assumes a somewhat expansive client like Buck's rust-project because the smarts can't live in rust-analyzer.)

crates/flycheck/src/project_json.rs Outdated Show resolved Hide resolved
crates/flycheck/src/project_json.rs Outdated Show resolved Hide resolved
crates/rust-analyzer/src/reload.rs Show resolved Hide resolved
docs/user/generated_config.adoc Outdated Show resolved Hide resolved
@davidbarsky davidbarsky force-pushed the david/move-rust-project-generation-to-server branch 3 times, most recently from acf5f3c to f3ee31f Compare July 18, 2024 13:46
@davidbarsky davidbarsky force-pushed the david/move-rust-project-generation-to-server branch from f3ee31f to db43a5a Compare July 18, 2024 16:01
@davidbarsky
Copy link
Contributor Author

@bors r=@Veykril

@bors
Copy link
Collaborator

bors commented Jul 18, 2024

📌 Commit db43a5a has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jul 18, 2024

⌛ Testing commit db43a5a with merge 52143a5...

@bors
Copy link
Collaborator

bors commented Jul 18, 2024

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

@bors bors merged commit 52143a5 into rust-lang:master Jul 18, 2024
11 checks passed
@davidbarsky davidbarsky deleted the david/move-rust-project-generation-to-server branch July 18, 2024 17:16
bors added a commit that referenced this pull request Jul 23, 2024
…ification, r=Veykril

chore: remove `UnindexinedProject` notification

This PR is split out from #17246 (and contains its changes, which is a little annoying from a review perspective...). I'd like to land this change a week or so after #17246 lands in order to give any users of the unindexed project notification time to adopt migrate.
@levino levino mentioned this pull request Jul 24, 2024
lnicola pushed a commit to lnicola/rust that referenced this pull request Jul 28, 2024
…oject-notification, r=Veykril

chore: remove `UnindexinedProject` notification

This PR is split out from rust-lang/rust-analyzer#17246 (and contains its changes, which is a little annoying from a review perspective...). I'd like to land this change a week or so after rust-lang#17246 lands in order to give any users of the unindexed project notification time to adopt migrate.
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Aug 1, 2024
…oject-notification, r=Veykril

chore: remove `UnindexinedProject` notification

This PR is split out from rust-lang/rust-analyzer#17246 (and contains its changes, which is a little annoying from a review perspective...). I'd like to land this change a week or so after rust-lang#17246 lands in order to give any users of the unindexed project notification time to adopt migrate.
Wilfred added a commit to Wilfred/rust-analyzer that referenced this pull request Sep 5, 2024
`linkedProjects` is owned by the user's configuration, so when users
update this setting, `linkedProjects` is reset. This is problematic when
`linkedProjects` also contains projects discovered with `discoverCommand`.

The buggy behaviour occurred when:

(1) The user configures `discoverCommand` and loads a Rust project.

(2) The user changes any setting in VS Code, so rust-analyzer receives
`workspace/didChangeConfiguration`.

(3) `handle_did_change_configuration` ultimately calls
`Client::apply_change_with_sink()`, which updates `config.user_config`
and discards any items we added in `linkedProjects`.

Instead, separate out `discovered_projects_from_filesystem` and
`discovered_projects_from_command` from user configuration, so user
settings cannot affect any type of discovered project.

This fixes the subtle issue mentioned here:
rust-lang#17246 (comment)
Wilfred added a commit to Wilfred/rust-analyzer that referenced this pull request Sep 5, 2024
`linkedProjects` is owned by the user's configuration, so when users
update this setting, `linkedProjects` is reset. This is problematic when
`linkedProjects` also contains projects discovered with `discoverCommand`.

The buggy behaviour occurred when:

(1) The user configures `discoverCommand` and loads a Rust project.

(2) The user changes any setting in VS Code, so rust-analyzer receives
`workspace/didChangeConfiguration`.

(3) `handle_did_change_configuration` ultimately calls
`Client::apply_change_with_sink()`, which updates `config.user_config`
and discards any items we added in `linkedProjects`.

Instead, separate out `discovered_projects_from_filesystem` and
`discovered_projects_from_command` from user configuration, so user
settings cannot affect any type of discovered project.

This fixes the subtle issue mentioned here:
rust-lang#17246 (comment)
Wilfred added a commit to Wilfred/rust-analyzer that referenced this pull request Sep 5, 2024
`linkedProjects` is owned by the user's configuration, so when users
update this setting, `linkedProjects` is reset. This is problematic when
`linkedProjects` also contains projects discovered with `discoverCommand`.

The buggy behaviour occurred when:

(1) The user configures `discoverCommand` and loads a Rust project.

(2) The user changes any setting in VS Code, so rust-analyzer receives
`workspace/didChangeConfiguration`.

(3) `handle_did_change_configuration` ultimately calls
`Client::apply_change_with_sink()`, which updates `config.user_config`
and discards any items we added in `linkedProjects`.

Instead, separate out `discovered_projects_from_filesystem` and
`discovered_projects_from_command` from user configuration, so user
settings cannot affect any type of discovered project.

This fixes the subtle issue mentioned here:
rust-lang#17246 (comment)
bors added a commit that referenced this pull request Sep 6, 2024
fix: Updating settings should not clobber discovered projects

`linkedProjects` is owned by the user's configuration, so when users update this setting, `linkedProjects` is reset. This is problematic when `linkedProjects` also contains projects discovered with `discoverCommand`.

The buggy behaviour occurred when:

(1) The user configures `discoverCommand` and loads a Rust project.

(2) The user changes any setting in VS Code, so rust-analyzer receives `workspace/didChangeConfiguration`.

(3) `handle_did_change_configuration` ultimately calls `Client::apply_change_with_sink()`, which updates
`config.user_config` and discards any items we added in `linkedProjects`.

Instead, separate out `discovered_projects_from_filesystem` and `discovered_projects_from_command` from user configuration, so user settings cannot affect any type of discovered project.

This fixes the subtle issue mentioned here: #17246 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants