[rami3l]

Closes #5133.

Background

In #4324, rustup tries to call rust-analyzer within the toolchain and, if it is not found, it will fall back to another candidate executable available in PATH.

When looking for candidates, rustup tries to prevent executables named rust-analyzer only by checking if it is called rust-analyzer and not a rustup proxy with same_file to avoid infinite recursion.

This works most of the time, but it doesn't take into account of the fact that a candidate executable can redirect to rustup without being itself a rustup proxy.

This is the case in #5133: Homebrew has shipped a wrapper script that it exposes as the proxy for rust-analyzer which redirects to rustup, instead of shipping the classical rustup proxy FS links. Thus, according to the old logic, rustup will try to call it not knowing the redirection exists, and infinite recursion will be triggered.

Proposed Solution

When calling rustup proxies, RUST_RECURSION_COUNT should be set to a value no larger than env_var::RUST_RECURSION_COUNT_MAX (currently 20).

Thus, if we set the value to env_var::RUST_RECURSION_COUNT_MAX + 1, any redirection to rustup will intentionally fail upon invocation. This way, we can prevent such redirecting executables from being a fallback candidate by calling rust-analyzer --version as a smoke test upfront.

Applying this test also replaces the previous Unix-only executable FS permissions check, so that the executability check becomes truly cross-platform.

Impact

Impact to the user will stay minimal. Only the proxy startup time of rust-analyzer will be slightly damaged due to the overhead introduced by the rust-analyzer --version test, but since rust-analyzer is expected to be a long-running process, its startup performance is not of great importance anyway.

cc @tamird for review.

[tamird]

Thanks for opening this! I'm not the right person to review it, though. :)

[rami3l]

Thanks for opening this! I'm not the right person to review it, though. :)

@tamird No worries!

Just to clarify, you are not expected to be able inspect the implementation details, but it'd be cool if you could check out the first commit and see if the test case is indeed describing what you've got with your Homebrew rustup distribution 🙏

[tamird]

AI-assisted reply:

Yep, that matches what I saw: the active toolchain has no rust-analyzer component, and Homebrew’s wrapper execs the rustup proxy, which falls back to the same wrapper.

My original setup also had a working standalone rust-analyzer later in PATH. Could we add that variant to verify that rustup skips the wrapper and reaches the real analyzer? The new case matches what I saw after uninstalling the standalone analyzer.

One question about the fix: should the --version probe have a timeout? Command::status() waits indefinitely, so a candidate hanging there would block the fallback search. There’s already timeout handling for rustc --version in the same file.

[rami3l]

@tamird Nice call! I've added the smoke test timeout.

That said I don't think a test involving 3 rust-analyzer executables would be necessary, because the current logic will only bail out if none of the candidates qualifies. Update: I managed to make it work.

[rami3l]

Looking back, probably I shouldn't have carried the old hack of copying rls to rust-analyzer over. I'll try to see if I can do this properly, after which I'll rebase this patch... Update: Addressed in #5141.

[djc]

I don't think the (to be fair, pre-existing) design of re-assigning is_external_ra is particularly clear. If we no longer need platform-specific stuff, it becomes easier to refactor that into something more clear.

View changes since the review

[djc]

I suppose an extra invocation is somewhat expensive, but we expect RA proxy calls not to happen that frequently anyway and/or be expensive anyway?

View changes since the review

[rami3l]

Yes, the overhead part is already explained in the PR description.

[ege-arhan]

If c.wait_timeout(...) times out (Ok(None)), c is dropped without terminating the child process. wait-timeout does not kill the child on timeout, and std::process::Child::drop does not kill or reap it either, leaving a hanging candidate binary running as an orphaned process in the background.

To prevent leaking processes (similar to the rustc version check earlier in src/toolchain.rs):

is_external_ra = match ra.spawn() {
    Ok(mut c) => match c.wait_timeout(Duration::from_secs(10)) {
        Ok(Some(s)) => s.success(),
        Ok(None) => {
            let _ = c.kill();
            let _ = c.wait();
            false
        }
        Err(_) => false,
    },
    Err(_) => false,
};

View changes since the review