Option to use the rustup-provided proc-macro-srv

[fasterthanlime]

Since RA as a subtree looks like it's gonna land soon, I'm looking at next steps.

I'm already happily using it locally with a rust-toolchain of:

[toolchain]
channel = "nightly-2022-07-23"

Having installed RA like so:

cargo +nightly-2022-07-23 install --path crates/rust-analyzer --locked --force --features force-always-assert --features proc-macro-srv/sysroot-abi

And having VSCode workspace settings (.vscode/settings.json) of:

{
  "rust-analyzer.server.path": "rust-analyzer"
}

This is very close to something I can tell others to do.

The issue is, as soon as "RA as a subtree" lands and that we have the "rust-analyzer proc-macro always works with the rustc from the same toolchain+channel" guarantee, what I want instead is this:

{
  "rust-analyzer.procMacro.server": "/home/amos/.rustup/toolchains/nightly-2022-07-23-x86_64-unknown-linux-gnu/bin/rust-analyzer"
}

There's several issues with that:

• It doesn't even work as-is: rust-analyzer.procMacro.server wants a path relative to the workspace for some reason, so the actual path it tries to execute is /home/amos/bearcove/some-project/home/amos/.rustup/...
• I obviously can't commit an absolute path starting with /home/amos to a shared Git repository
• Having to re-state the channel here when it's in rust-toolchain.toml already is.. not very DRY. rust-analyzer already has code to discover the toolchain / various tools, I'd expect it to do the right thing here too.

But what is the right thing?

The dream scenario

In my dreams, when expanding a proc-macro, rust-analyzer:

• Finds out which toolchain it's been compiled with
• If it's provided by rustup, it adds the rust-analyzer component as needed
• Then it spawns that toolchain's rust-analyzer proc-macro subcommand and uses that, for this proc macro
• Repeat with any proc macros that need expanding (which may spawn several toolchains, if different cargo workspaces are opened in the same vscode window)

There's a bunch of moving pieces there - I don't know how hard the current codebase assumes that we have "a single proc-macro-srv binary" (and then that binary is responsible for handling all possible ABIs).

The short-term win

Because this is all relatively new, we might not want to mess with defaults just yet. Instead, we could add another option, say maybe:

  "rust-analyzer.procMacro.serverFromRustup": true

Under the hood, this executes rustup which rust-analyzer at the top of the workspace:

❯ rustup +nightly-2022-07-23 which rust-analyzer
/home/amos/.rustup/toolchains/nightly-2022-07-23-x86_64-unknown-linux-gnu/bin/rust-analyzer

And attempts rustup component add rust-analyzer once if that fails.

One alternative is to have a special value for rust-analyzer.procMacro.server that's just the "rustup" string, but using an in-band value / picking "rustup" as a niche feels odd.

The downside of having an option like rust-analyzer.procMacro.serverFromRustup is that we need to decide if it takes precedence over rust-analyzer.procMacro.server (I think it should).

Later on, if we're happy with that behavior, we can change the default to true (and folks can still opt out of it by setting it to false explicitly).

[Veykril]

It doesn't even work as-is: rust-analyzer.procMacro.server wants a path relative to the workspace for some reason, so the actual path it tries to execute is /home/amos/bearcove/some-project/home/amos/.rustup/...

This sounds very much wrong, there is no reason for the server to make that path relative to the project workspace... Though maybe this was always meant to be an internal setting so far.

Would it be an option (speaking short term at least) to expose a command override that is used to spawn the proc-macro server? Then I imagine you could set it to something like rustup +nightly-2022-07-23 rust-analyzer proc-macro right?

[fasterthanlime]

Would it be an option (speaking short term at least) to expose a command override that is used to spawn the proc-macro server? Then I imagine you could set it to something like rustup +nightly-2022-07-23 rust-analyzer proc-macro right?

There's a couple design considerations there. The command you're looking for, btw, is rustup run nightly-2022-07-23 rust-analyzer proc-macro.

To start with, rustup run doesn't support using "the ambient toolchain". It accepts "default" as a toolchain name, but that's often "stable" or "nightly", not "the toolchain that should be used for this workspace". Even if it did, we'd need to define which folder rust-analyzer runs that from - since that would impact toolchain selection.

I suppose for a start, we can accept that the channel (nightly-2022-07-23) will be duplicated in rust-toolchain.toml and .vscode/settings.json. That requires two changes for rust-analyzer.procMacro.server:

• Accept "full commands", not just "a path to an executable". Maybe it should accept either a String or an Array, to make things easier (no need to do split shell commands).
• Stop assuming what's passed is a relative path, or an absolute path for that matter. It should look at $PATH if the first item is not an absolut path.

That could be a viable step in the right direction!

[Veykril]

(possibly slight topic change but I just went on rustup documentation hunt)
I am not too familiar with rustup itself and how overrides work, but if I understand this right proxies in rustup make use of overrides as is. Given rust-analyzer is soon gonna be a component, and rls is apparently a proxy in rustup, couldn't we make rust-analyzer also a proxy for rust-up? That way overrides should just work and we should be able to drop the explicit toolchain and rust-analyzer won't have to figure that one out itself. That is assuming I understood how these things interact correctly.

That aside I think we should have a proper override command setting instead of just the server path nevertheless (as well as fixing the server path being relative).
And assuming what I dug up would work one would only need to set the command to rustup run rust-analyzer proc-macro and it should just work™️. (though r-a currently only spawns a single server process opposed to one per workspace I believe, though that is a thing we can dig into afterwards)

[fasterthanlime]

I am not too familiar with rustup itself and how overrides work, but if I understand this right proxies in rustup make use of overrides as is. Given rust-analyzer is soon gonna be a component, and rls is apparently a proxy in rustup, couldn't we make rust-analyzer also a proxy for rust-up? That way overrides should just work and we should be able to drop the explicit toolchain and rust-analyzer won't have to figure that one out itself. That is assuming I understood how these things interact correctly.

(nit: rust-analyzer is already a component - it just wasn't very useful as a component so far, because ABI breakage, yada yada)

Yes, that's another path forward. There's a PR to add a rust-analyzer proxy for rustup: rust-lang/rustup#3022

I think @kinnison is waiting for the go-ahead from more rust-analyzer folks to merge it?

After that, the override path can just be rust-analyzer (pointing to the proxy), and rust-analyzer just has to spawn that from the right path, so it uses the correct override.

Another concern we have in parallel to that, is that now we suddenly can have a mismatch between the rust-analyzer version that provides LSP / code intelligence, and the rust-analyzer version that provides proc-macro-srv, hence why I suggested versioning the proc-macro-api interface in some way (but that can be done later).

[Veykril]

Okay so to re-iterate then:

short term

• Create a procMacro.overrideCommand setting
• Fix procMacro.server to not be relative to the project root (though this is not really relevant to this anymore)
• Make rust-analyzer a rustup proxy component

long term:

• if the used r-a binary is a rustup distribution
  • check if the "current" toolchain has the r-a rustup component
    • if not, ask the user whether r-a should add it for them
  • if the "current" toolchain has the r-a component installed, spawn it via rustup run rust-analyzer proc-macro unless the override command or server path is set
• Have r-a spawn a proc-macro server per cargo workspace (maybe short term, depends on how simple this is not sure)

Does this capture everything from this issue or am I missing something here (except for the last concern you mentioned now)?

[fasterthanlime]

Yes to everything you said except this part:

• if the used r-a binary is a rustup distribution

This should happen even in the 99% case where the VSCode user is using the "VSCode Marketplace" version of the r-a binary. The proper condition is if the cargo/rustc being used is provided by rustup. (This isn't the case if they're installed via the distribution package manager, for example - another discussion topic for another time).

[Veykril]

Ah okay, ye that makes more sense actually :) So the question there is how to query that, is it enough to just check if rustup --version succeeds or something or is there a better way to check for that.

With that said, the short term items here are rather easy to fix then, the biggest problem there is getting the proxy component PR merged.

(And If I see this right, judging from a quick glance at the source, spawning a proc-macro server per workspace shouldn't be too tough if not hidden problems exist there)

[fasterthanlime]

So the question there is how to query that, is it enough to just check if rustup --version succeeds or something or is there a better way to check for that.

I'd do platform::cargo() and check if cargo_path.parent().join("rustup") exists. For typical installs they're both in something like ~/.cargo/bin/{rustup,cargo} - maybe the rustup team can give us a better solution.

the biggest problem there is getting the proxy component PR merged.

Yeah, I tried leaving encouraging comments on rust-lang/rustup#3022 but I think the concern is that it'll "break someone's workflow" if they've installed their own rust-analyzer binary with cargo xtask install or something. I think a comment from someone on the RA team could easily address that concern.

As far as I can tell, the "DUP" mechanism is designed exactly for that: it won't override anything you already have in your ~/.cargo/bin with a proxy binary.

[Veykril]

maybe the rustup team can give us a better solution.

I'll drop a topic in #t-devtools -> https://rust-lang.zulipchat.com/#narrow/stream/301329-t-devtools/topic/Best.20way.20for.20to.20query.20whether.20rustup.20is.20used

Yeah, I tried leaving encouraging comments on rust-lang/rustup#3022...

From the looks of it there shouldn't be any problems with that, as said the DUP mechanism should fix that problem.

(And If I see this right, judging from a quick glance at the source, spawning a proc-macro server per workspace shouldn't be too tough if not hidden problems exist there)

Just went and checked, did a quick implementation and it seems to work fine with multiple servers #12856

[fasterthanlime]

This PR moves us along the "separate proc-macro-srv binary" path:

• Add rust-analyzer-proc-macro-srv binary, use it if found in sysroot #12858

I've gotten initial positive feedback about shipping it as part of the rustc component, which means it would be installed for all toolchains (past a certain date), even when rustc isn't installed via rustup.

[fasterthanlime]

This is now done automatically and should work out of the box starting from rust-analyzer stable 2022-08-01 for rust nightly-2022-07-29 or greater (including betas and stables).

[lnicola]

To start with, rustup run doesn't support using "the ambient toolchain". It accepts "default" as a toolchain name, but that's often "stable" or "nightly", not "the toolchain that should be used for this workspace".

Well, doesn't that bring back memories? 😄