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

code: expose workspaces to other extensions; remove addProject command #15830

Conversation

davidbarsky
Copy link
Contributor

This (mostly red) PR does three things:

  • Exposes two methods to companion extensions (setWorkspaces and notifyRustAnalyzer).
    • setWorkspaces is needed to update linkedProjects without writing workspace/global configuration.
    • notifyRustAnalyzer to get the server to pull the new configuration.
  • Makes Ctx implement RustAnalyzerExtensionApi to prevent accidental regressions.
  • Remove rust-analyzer.addProject, as that will live in a buck2 companion extension. No need for that to be in rust-analyzer!

I can see the utility of combining notifyRustAnalyzer and setWorkspaces into a single method (updateWorkspacesAndNotify()?), but I don't feel strongly about this. My feeling is that this API could be easily changed in the future.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 3, 2023
@davidbarsky
Copy link
Contributor Author

cc: @Wilfred @P1n3appl3.

@davidbarsky davidbarsky force-pushed the davidbarsky/allow-companion-extension-to-call-rust-analyzer branch from 2269c36 to 497c019 Compare November 7, 2023 17:48
@davidbarsky
Copy link
Contributor Author

@lnicola don't land this yet, sorry! I'm working on a (quick) change that modifies discoverWorkspaceCommand to accept a array of vscode.Uri. I think I'd prefer to reduce the churn on y'all by sneaking that in here!

(I should've commented sooner!)

@lnicola
Copy link
Member

lnicola commented Nov 7, 2023

Yeah, no worries. Veykril might also want to take a look when he's back.

@@ -190,7 +191,7 @@ export class Ctx {
const command = `${this.config.discoverProjectRunner}.discoverWorkspaceCommand`;
log.info(`running command: ${command}`);
const project: JsonProject = await vscode.commands.executeCommand(command);
this.addToDiscoveredWorkspaces([project]);
this.setWorkspaces([project]);

Choose a reason for hiding this comment

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

Maybe I'm not reading this correctly (it's quite possible I am) - but is the implication of this that if I am currently working in project1/foo.rs and then open a new .rs file in project2/bar.rs, I will lose r-a support in project1?

Copy link
Contributor Author

@davidbarsky davidbarsky Nov 8, 2023

Choose a reason for hiding this comment

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

You will not lose IDE functionality when switching targets.

The relationship between the rust-analyzer extension and the companion extension is a little weird. At startup, the rust-analyzer extension will pull data from the companion extension, but in the steady state, the companion extension will push changes to the rust-analyzer extension. The code you're commenting on is responsible for pulling changes at startup.

I need to cherry-pick changes I made on a branch that would make the companion extension fully control rust-analyzer's view of linkedProjects, which is what I was referring to when I asked Laurențiu not to land this PR.

Relatedly, rust-project will managed each workspace (the generated rust-project.json) based on the target label or the contents of _workspace (as set by with_rust_workspace).

@davidbarsky
Copy link
Contributor Author

@lnicola depending on your interest level, i've updated this PR to include the changes I wanted to.

(let me know if I shouldn't @ you!)

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.

lgtm with a the comment nit

@Veykril
Copy link
Member

Veykril commented Nov 15, 2023

@bors delegate+

@bors
Copy link
Contributor

bors commented Nov 15, 2023

✌️ @davidbarsky, you can now approve this pull request!

If @Veykril told you to "r=me" after making some further change, please make that change, then do @bors r=@Veykril

@davidbarsky davidbarsky force-pushed the davidbarsky/allow-companion-extension-to-call-rust-analyzer branch from 2625495 to 0cd68bf Compare November 16, 2023 17:38
@davidbarsky
Copy link
Contributor Author

@bors r+

@bors
Copy link
Contributor

bors commented Nov 16, 2023

📌 Commit 0cd68bf has been approved by davidbarsky

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 16, 2023

⌛ Testing commit 0cd68bf with merge 37a8790...

@bors
Copy link
Contributor

bors commented Nov 16, 2023

☀️ Test successful - checks-actions
Approved by: davidbarsky
Pushing 37a8790 to master...

@bors bors merged commit 37a8790 into rust-lang:master Nov 16, 2023
10 checks passed
@davidbarsky davidbarsky deleted the davidbarsky/allow-companion-extension-to-call-rust-analyzer branch November 19, 2023 00:57
bors added a commit that referenced this pull request Feb 8, 2024
…, r=Veykril

feature: Create `UnindexedProject` notification to be sent to the client

(Note that this branch contains commits from #15830, which I'll rebase atop of as needed.)

Based on the discussion in #15837, I've added a notification and off-by-default toggle to send that notification from `handle_did_open_text_document`. I'm happy to rename/tweak this as needed.

I've been using this for a little bit, and it does seem to cause a little bit more indexing/work in rust-analyzer, but it's something that I'll profile as needed, I think.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants