feat: Use oldest rustup rust-analyzer when toolchain override is present #17705

huntc · 2024-07-26T05:36:22Z

Selects a rust-toolchain declared RA based on its date. The earliest (oldest) RA wins and becomes the one that the workspace uses as a whole.

In terms of precedence:

nightly > stable-with-version > stable

With stable-with-version, we invoke the RA with a --version arg and attempt to extract a date. Given the same date as a nightly, the nightly RA will win.

Fixes #17663

Veykril · 2024-07-26T07:08:43Z

editors/code/src/bootstrap.ts

+                });
+                if (!res.error && res.status === 0) {
+                    toolchainServerPath = earliestToolchainPath(toolchainServerPath, res.stdout.trim(), function (path) {
+                        const res = spawnSync(path, ["version"], {


Wouldn't it make more sense to query the rustup version for the workspace here?

This is a lambda where the path comes from within evaluating the toolchain path. It is invoked conditionally depending on the type of path to the RA it is i.e. if the RA isn't pointing to a nightly or stable one, then the path is deemed to be an explicit version of RA. In that instance we'll query its version for a date.

Also, this is a lambda for two reasons:

other spawnSync calls are made within this getServer function; and

so that I can write unit tests easily for earliestToolchainPath and orderFromPath.

I'm confused, rustup which rust-analyzer should only yield a rust toolchain version (if installed in the toolchain) or error no?

It appears to yield paths with stable, nightly and [some-version] in them. At least in my workspace. I’ll create some unit tests with sample paths to illustrate as I move forward.

Or may be I don’t understand what you’re asking… if we are talking about the lambda still, that path can be a path to a nightly, stable or versioned RA.

Oh I see, we can return a manual (non-toolchain component) rust-analyzer install from this. Okay then this handling does make sense

Or actually, what is being invoked here, path/to/rust-analyzer version? That wouldn't make sense as that is not valid command for r-a. The flag is --version. Can you add a comment above that explaining roughly whats being executed for visibility? Also for the regex part a comment showing an example input and what we disassemble that to via the regexes(regices?) would be helpful for readability

Absolutely, I’ll get on to some unit tests tomorrow which should make things a bit more obvious Also, the test infra is there already (npm test).

I just wanted to make sure I was on the right track etc. Thanks for the initial review.

huntc · 2024-07-26T09:31:16Z

@Veykril Can you please direct me to some doco in your to conveniently test our the extension?

Veykril · 2024-07-26T10:37:19Z

I don't think we can easily test this. We generally don't have a good testing infra setup for the vscode extension unfortunately.

huntc · 2024-07-27T00:48:02Z

I don't think we can easily test this. We generally don't have a good testing infra setup for the vscode extension unfortunately.

Sorry, I meant, how do I test this manually and locally? I can see an option to install the VSIX file, but I'm unsure if it updates an existing one.

huntc · 2024-07-27T02:23:15Z

OK, this is working, although I think we need to go a bit further...

At the moment, the code will identify RA's for each rust-toolchain.toml file found. This isn't exactly what we want though. We should only select an RA with a toolchain if it is explicitly set via components. I'll look at doing that next.

huntc · 2024-07-27T02:24:04Z

OK, this is working, although I think we need to go a bit further...

At the moment, the code will identify RA's for each rust-toolchain.toml file found. This isn't exactly what we want though. We should only select an RA with a toolchain if it is explicitly set via components. I'll look at doing that next.

BTW I figured this out by installing the extension from its VSIX. I realised that npm run package was also required.

huntc · 2024-07-28T06:07:38Z

This PR has now been tested in my environment that has a couple of projects pinned to old toolchains. Having added RA as a component to the relevant toolchain files causes the corresponding version of RA to be used. I can confirm this by using the "show RA version" command.

I am getting the following warnings reported and don't quite understand why:

[ERROR rust_analyzer::reload] Failed to run proc-macro server from path /Users/myuser/.rustup/toolchains/stable-aarch64-apple-darwin/libexec/rust-analyzer-proc-macro-srv, error: Custom { kind: Other, error: "proc-macro server's api version (4) is newer than rust-analyzer's (2)" }

Do we need to do something with the proc-macro server as well?

Selects a rust-toolchain declared RA based on its date. The earliest (oldest) RA wins and becomes the one that the workspace uses as a whole. In terms of precedence: nightly > stable-with-version > stable With stable-with-version, we invoke the RA with a `--version` arg and attempt to extract a date. Given the same date as a nightly, the nightly RA will win.

editors/code/src/bootstrap.ts

huntc · 2024-07-28T06:11:56Z

editors/code/src/bootstrap.ts

+export const _private = {
+    earliestToolchainPath,
+    orderFromPath,
+};


This appears to be the TS way of "hiding" things that need to be exported purely for testing.

Veykril · 2024-07-28T08:48:46Z

This PR has now been tested in my environment that has a couple of projects pinned to old toolchains. Having added RA as a component to the relevant toolchain files causes the corresponding version of RA to be used. I can confirm this by using the "show RA version" command.

I am getting the following warnings reported and don't quite understand why:
[ERROR rust_analyzer::reload] Failed to run proc-macro server from path /Users/myuser/.rustup/toolchains/stable-aarch64-apple-darwin/libexec/rust-analyzer-proc-macro-srv, error: Custom { kind: Other, error: "proc-macro server's api version (4) is newer than rust-analyzer's (2)" }
Do we need to do something with the proc-macro server as well?

That is a problem we can't work around unfortunately. The problem here is that the rust-analyzer is too old for one of the newer toolchains in the project being used, so it doesn't speak the proc-macro servers protocol anymore. We can't use a different proc-macro server though as it is required for the proc-macro server to built with the same toolchain as the proc-macros that are being loaded.

huntc · 2024-07-28T08:56:09Z

This PR has now been tested in my environment that has a couple of projects pinned to old toolchains. Having added RA as a component to the relevant toolchain files causes the corresponding version of RA to be used. I can confirm this by using the "show RA version" command.
I am getting the following warnings reported and don't quite understand why:
[ERROR rust_analyzer::reload] Failed to run proc-macro server from path /Users/myuser/.rustup/toolchains/stable-aarch64-apple-darwin/libexec/rust-analyzer-proc-macro-srv, error: Custom { kind: Other, error: "proc-macro server's api version (4) is newer than rust-analyzer's (2)" }
Do we need to do something with the proc-macro server as well?
That is a problem we can't work around unfortunately. The problem here is that the rust-analyzer is too old for one of the newer toolchains in the project being used, so it doesn't speak the proc-macro servers protocol anymore. We can't use a different proc-macro server though as it is required for the proc-macro server to built with the same toolchain as the proc-macros that are being loaded.

Ok. I guess we must live with that then. At least the changes here avoid broken development experiences.

lnicola · 2024-07-28T09:15:23Z

BTW I figured this out by installing the extension from its VSIX. I realised that npm run package was also required.

cargo xtask install --client should do the trick.

regexes (regices?)

Regexen, of course 🙃.

Veykril · 2024-07-29T11:57:31Z

editors/code/src/bootstrap.ts

+//  nightly   - /Users/myuser/.rustup/toolchains/nightly-2022-11-22-aarch64-apple-darwin/bin/rust-analyzer
+//  versioned - /Users/myuser/.rustup/toolchains/1.72.1-aarch64-apple-darwin/bin/rust-analyzer
+//  stable    - /Users/myuser/.rustup/toolchains/stable-aarch64-apple-darwin/bin/rust-analyzer
+function orderFromPath(


This will not handle non-versioned toolchains correctly. stable/beta/nightly can refer to some arbitrary old or new version.

For those we should invoke rustup show active-toolchain -v
which yields

nightly-x86_64-pc-windows-msvc (default) rustc 1.81.0-nightly (9c3bc805d 2024-06-27)

where we can fetch the date

Please note that we're not actually checking the path itself, but the RA that resides at that path is being asked for its version.

However, I've now actually enhanced the logic to always ask the RA for its version info, even if is stable, beta etc. The logic now stands that given the same date, nightly will take precedence over anything else. This actually simplifies the code.

Veykril · 2024-08-02T06:49:35Z

Looks great, thank you for implementing this!
@bors r+

bors · 2024-08-02T06:49:38Z

📌 Commit 77e6999 has been approved by Veykril

It is now in the queue for this repository.

bors · 2024-08-02T06:50:46Z

⌛ Testing commit 77e6999 with merge 670a5ab...

bors · 2024-08-02T07:03:42Z

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

huntc · 2024-08-02T07:30:04Z

Looks great, thank you for implementing this! @bors r+

My pleasure. Thanks for the guidance also.

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 26, 2024

huntc mentioned this pull request Jul 26, 2024

VSCode should honor toolchain overrides that contain a rust-analyzer component for server spawning #17663

Closed

Veykril reviewed Jul 26, 2024

View reviewed changes

huntc force-pushed the resolve-ra branch from e4181d4 to 5c8a320 Compare July 27, 2024 02:14

huntc force-pushed the resolve-ra branch from 5c8a320 to 887606b Compare July 28, 2024 06:03

huntc marked this pull request as ready for review July 28, 2024 06:07

huntc force-pushed the resolve-ra branch from 887606b to ec7451c Compare July 28, 2024 06:09

huntc commented Jul 28, 2024

View reviewed changes

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

huntc commented Jul 28, 2024

View reviewed changes

huntc mentioned this pull request Jul 28, 2024

Configure rust toolchain on a per-folder/project level zed-industries/zed#4883

Open

1 task

Veykril reviewed Jul 29, 2024

View reviewed changes

huntc added 5 commits July 30, 2024 16:26

Checks date for any RA

b05def0

Further simplifications

26691a1

Outdated comment removed

37d4cf0

Formatting

719631b

Corrected precedence position

77e6999

Veykril approved these changes Aug 2, 2024

View reviewed changes

Veykril changed the title ~~Determine toolchain RA on age~~ feat: Use oldest rustup rust-analyzer when toolchain override is present Aug 2, 2024

bors merged commit 670a5ab into rust-lang:master Aug 2, 2024
11 checks passed

huntc deleted the resolve-ra branch August 2, 2024 07:30

feat: Use oldest rustup rust-analyzer when toolchain override is present #17705

feat: Use oldest rustup rust-analyzer when toolchain override is present #17705

Uh oh!

Conversation

huntc commented Jul 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huntc Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huntc commented Jul 26, 2024

Uh oh!

Veykril commented Jul 26, 2024

Uh oh!

huntc commented Jul 27, 2024

Uh oh!

huntc commented Jul 27, 2024

Uh oh!

huntc commented Jul 27, 2024

Uh oh!

huntc commented Jul 28, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veykril commented Jul 28, 2024

Uh oh!

huntc commented Jul 28, 2024

Uh oh!

lnicola commented Jul 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veykril commented Aug 2, 2024

Uh oh!

bors commented Aug 2, 2024

Uh oh!

bors commented Aug 2, 2024

Uh oh!

bors commented Aug 2, 2024

Uh oh!

Uh oh!

huntc commented Aug 2, 2024

Uh oh!

Uh oh!

huntc Jul 26, 2024 •

edited

Loading