Merge #4580

4580: Fix invoking cargo without consulting CARGO env var or standard installation paths r=matklad a=Veetaha

Followup for #4329

The pr essentially fixes [this bug](https://youtu.be/EzQ7YIIo1rY?t=2189)

cc @lefticus

Co-authored-by: veetaha <veetaha2@gmail.com>

Cargo.lock

@@ -1119,6 +1119,7 @@ dependencies = [
  "memmap",
  "ra_mbe",
  "ra_proc_macro",
+ "ra_toolchain",
  "ra_tt",
  "serde_derive",
  "test_utils",

crates/ra_proc_macro_srv/Cargo.toml

@@ -22,3 +22,4 @@ cargo_metadata = "0.10.0"
 difference = "2.0.0"
 # used as proc macro test target
 serde_derive = "1.0.106"
+ra_toolchain = { path = "../ra_toolchain" }

crates/ra_proc_macro_srv/src/tests/utils.rs

@@ -2,7 +2,6 @@
 
 use crate::dylib;
 use crate::ProcMacroSrv;
-pub use difference::Changeset as __Changeset;
 use ra_proc_macro::ListMacrosTask;
 use std::str::FromStr;
 use test_utils::assert_eq_text;
@@ -13,7 +12,7 @@ mod fixtures {
 
     // Use current project metadata to get the proc-macro dylib path
     pub fn dylib_path(crate_name: &str, version: &str) -> std::path::PathBuf {
-        let command = Command::new("cargo")
+        let command = Command::new(ra_toolchain::cargo())
             .args(&["check", "--message-format", "json"])
             .output()
             .unwrap()

crates/rust-analyzer/src/lsp_ext.rs

@@ -121,12 +121,21 @@ pub struct RunnablesParams {
     pub position: Option<Position>,
 }
 
+// Must strictly correspond to the executable name
+#[derive(Serialize, Deserialize, Debug)]
+#[serde(rename_all = "lowercase")]
+pub enum RunnableKind {
+    Cargo,
+    Rustc,
+    Rustup,
+}
+
 #[derive(Deserialize, Serialize, Debug)]
 #[serde(rename_all = "camelCase")]
 pub struct Runnable {
     pub range: Range,
     pub label: String,
-    pub bin: String,
+    pub kind: RunnableKind,
     pub args: Vec<String>,
     pub extra_args: Vec<String>,
     pub env: FxHashMap<String, String>,

crates/rust-analyzer/src/main_loop/handlers.rs

@@ -426,7 +426,7 @@ pub fn handle_runnables(
                 res.push(lsp_ext::Runnable {
                     range: Default::default(),
                     label: format!("cargo {} -p {}", cmd, spec.package),
-                    bin: "cargo".to_string(),
+                    kind: lsp_ext::RunnableKind::Cargo,
                     args: vec![cmd.to_string(), "--package".to_string(), spec.package.clone()],
                     extra_args: Vec::new(),
                     env: FxHashMap::default(),
@@ -438,7 +438,7 @@ pub fn handle_runnables(
             res.push(lsp_ext::Runnable {
                 range: Default::default(),
                 label: "cargo check --workspace".to_string(),
-                bin: "cargo".to_string(),
+                kind: lsp_ext::RunnableKind::Cargo,
                 args: vec!["check".to_string(), "--workspace".to_string()],
                 extra_args: Vec::new(),
                 env: FxHashMap::default(),
@@ -982,10 +982,11 @@ fn to_lsp_runnable(
             target.map_or_else(|| "run binary".to_string(), |t| format!("run {}", t))
         }
     };
+
     Ok(lsp_ext::Runnable {
         range: to_proto::range(&line_index, runnable.range),
         label,
-        bin: "cargo".to_string(),
+        kind: lsp_ext::RunnableKind::Cargo,
         args,
         extra_args,
         env: {

crates/rust-analyzer/tests/heavy_tests/main.rs

@@ -79,7 +79,7 @@ fn foo() {
           {
             "args": [ "test" ],
             "extraArgs": [ "foo", "--nocapture" ],
-            "bin": "cargo",
+            "kind": "cargo",
             "env": { "RUST_BACKTRACE": "short" },
             "cwd": null,
             "label": "test foo",
@@ -91,7 +91,7 @@ fn foo() {
           {
             "args": ["check", "--workspace"],
             "extraArgs": [],
-            "bin": "cargo",
+            "kind": "cargo",
             "env": {},
             "cwd": null,
             "label": "cargo check --workspace",
@@ -141,7 +141,7 @@ fn main() {}
             {
               "args": [ "test", "--package", "foo", "--test", "spam" ],
               "extraArgs": [ "test_eggs", "--exact", "--nocapture" ],
-              "bin": "cargo",
+              "kind": "cargo",
               "env": { "RUST_BACKTRACE": "short" },
               "label": "test test_eggs",
               "range": {
@@ -153,7 +153,7 @@ fn main() {}
             {
               "args": [ "check", "--package", "foo" ],
               "extraArgs": [],
-              "bin": "cargo",
+              "kind": "cargo",
               "env": {},
               "label": "cargo check -p foo",
               "range": {
@@ -165,7 +165,7 @@ fn main() {}
             {
               "args": [ "test", "--package", "foo" ],
               "extraArgs": [],
-              "bin": "cargo",
+              "kind": "cargo",
               "env": {},
               "label": "cargo test -p foo",
               "range": {

docs/dev/lsp-extensions.md

@@ -427,7 +427,7 @@ interface Runnable {
     /// The label to show in the UI.
     label: string;
     /// The following fields describe a process to spawn.
-    bin: string;
+    kind: "cargo" | "rustc" | "rustup";
     args: string[];
     /// Args for cargo after `--`.
     extraArgs: string[];

editors/code/src/debug.ts

@@ -3,7 +3,7 @@ import * as vscode from 'vscode';
 import * as path from 'path';
 import * as ra from './lsp_ext';
 
-import { Cargo } from './cargo';
+import { Cargo } from './toolchain';
 import { Ctx } from "./ctx";
 
 const debugOutput = vscode.window.createOutputChannel("Debug");

editors/code/src/lsp_ext.ts

@@ -45,10 +45,13 @@ export interface RunnablesParams {
     textDocument: lc.TextDocumentIdentifier;
     position: lc.Position | null;
 }
+
+export type RunnableKind = "cargo" | "rustc" | "rustup";
+
 export interface Runnable {
     range: lc.Range;
     label: string;
-    bin: string;
+    kind: RunnableKind;
     args: string[];
     extraArgs: string[];
     env: { [key: string]: string };

editors/code/src/run.ts

@@ -1,6 +1,7 @@
 import * as vscode from 'vscode';
 import * as lc from 'vscode-languageclient';
 import * as ra from './lsp_ext';
+import * as toolchain from "./toolchain";
 
 import { Ctx, Cmd } from './ctx';
 import { startDebugSession, getDebugConfiguration } from './debug';
@@ -175,7 +176,7 @@ export function createTask(spec: ra.Runnable): vscode.Task {
     const definition: CargoTaskDefinition = {
         type: 'cargo',
         label: spec.label,
-        command: spec.bin,
+        command: toolchain.getPathForExecutable(spec.kind),
         args: spec.extraArgs ? [...spec.args, '--', ...spec.extraArgs] : spec.args,
         env: Object.assign({}, process.env, spec.env),
     };

editors/code/src/tasks.ts

@@ -1,4 +1,5 @@
 import * as vscode from 'vscode';
+import * as toolchain from "./toolchain";
 
 // This ends up as the `type` key in tasks.json. RLS also uses `cargo` and
 // our configuration should be compatible with it so use the same key.
@@ -24,6 +25,8 @@ class CargoTaskProvider implements vscode.TaskProvider {
         // set of tasks that always exist. These tasks cannot be removed in
         // tasks.json - only tweaked.
 
+        const cargoPath = toolchain.cargoPath();
+
         return [
             { command: 'build', group: vscode.TaskGroup.Build },
             { command: 'check', group: vscode.TaskGroup.Build },
@@ -46,7 +49,7 @@ class CargoTaskProvider implements vscode.TaskProvider {
                     `cargo ${command}`,
                     'rust',
                     // What to do when this command is executed.
-                    new vscode.ShellExecution('cargo', [command]),
+                    new vscode.ShellExecution(cargoPath, [command]),
                     // Problem matchers.
                     ['$rustc'],
                 );

editors/code/src/toolchain.ts

@@ -1,9 +1,10 @@
 import * as cp from 'child_process';
 import * as os from 'os';
 import * as path from 'path';
+import * as fs from 'fs';
 import * as readline from 'readline';
 import { OutputChannel } from 'vscode';
-import { isValidExecutable } from './util';
+import { log, memoize } from './util';
 
 interface CompilationArtifact {
     fileName: string;
@@ -17,34 +18,35 @@ export interface ArtifactSpec {
     filter?: (artifacts: CompilationArtifact[]) => CompilationArtifact[];
 }
 
-export function artifactSpec(args: readonly string[]): ArtifactSpec {
-    const cargoArgs = [...args, "--message-format=json"];
-
-    // arguments for a runnable from the quick pick should be updated.
-    // see crates\rust-analyzer\src\main_loop\handlers.rs, handle_code_lens
-    switch (cargoArgs[0]) {
-        case "run": cargoArgs[0] = "build"; break;
-        case "test": {
-            if (!cargoArgs.includes("--no-run")) {
-                cargoArgs.push("--no-run");
-            }
-            break;
-        }
-    }
-
-    const result: ArtifactSpec = { cargoArgs: cargoArgs };
-    if (cargoArgs[0] === "test") {
-        // for instance, `crates\rust-analyzer\tests\heavy_tests\main.rs` tests
-        // produce 2 artifacts: {"kind": "bin"} and {"kind": "test"}
-        result.filter = (artifacts) => artifacts.filter(it => it.isTest);
-    }
-
-    return result;
-}
-
 export class Cargo {
     constructor(readonly rootFolder: string, readonly output: OutputChannel) { }
 
+    // Made public for testing purposes
+    static artifactSpec(args: readonly string[]): ArtifactSpec {
+        const cargoArgs = [...args, "--message-format=json"];
+
+        // arguments for a runnable from the quick pick should be updated.
+        // see crates\rust-analyzer\src\main_loop\handlers.rs, handle_code_lens
+        switch (cargoArgs[0]) {
+            case "run": cargoArgs[0] = "build"; break;
+            case "test": {
+                if (!cargoArgs.includes("--no-run")) {
+                    cargoArgs.push("--no-run");
+                }
+                break;
+            }
+        }
+
+        const result: ArtifactSpec = { cargoArgs: cargoArgs };
+        if (cargoArgs[0] === "test") {
+            // for instance, `crates\rust-analyzer\tests\heavy_tests\main.rs` tests
+            // produce 2 artifacts: {"kind": "bin"} and {"kind": "test"}
+            result.filter = (artifacts) => artifacts.filter(it => it.isTest);
+        }
+
+        return result;
+    }
+
     private async getArtifacts(spec: ArtifactSpec): Promise<CompilationArtifact[]> {
         const artifacts: CompilationArtifact[] = [];
 
@@ -77,7 +79,7 @@ export class Cargo {
     }
 
     async executableFromArgs(args: readonly string[]): Promise<string> {
-        const artifacts = await this.getArtifacts(artifactSpec(args));
+        const artifacts = await this.getArtifacts(Cargo.artifactSpec(args));
 
         if (artifacts.length === 0) {
             throw new Error('No compilation artifacts');
@@ -94,14 +96,7 @@ export class Cargo {
         onStderrString: (data: string) => void
     ): Promise<number> {
         return new Promise((resolve, reject) => {
-            let cargoPath;
-            try {
-                cargoPath = getCargoPathOrFail();
-            } catch (err) {
-                return reject(err);
-            }
-
-            const cargo = cp.spawn(cargoPath, cargoArgs, {
+            const cargo = cp.spawn(cargoPath(), cargoArgs, {
                 stdio: ['ignore', 'pipe', 'pipe'],
                 cwd: this.rootFolder
             });
@@ -126,26 +121,54 @@ export class Cargo {
     }
 }
 
-// Mirrors `ra_env::get_path_for_executable` implementation
-function getCargoPathOrFail(): string {
-    const envVar = process.env.CARGO;
-    const executableName = "cargo";
-
-    if (envVar) {
-        if (isValidExecutable(envVar)) return envVar;
-
-        throw new Error(`\`${envVar}\` environment variable points to something that's not a valid executable`);
-    }
-
-    if (isValidExecutable(executableName)) return executableName;
-
-    const standardLocation = path.join(os.homedir(), '.cargo', 'bin', executableName);
-
-    if (isValidExecutable(standardLocation)) return standardLocation;
-
-    throw new Error(
-        `Failed to find \`${executableName}\` executable. ` +
-        `Make sure \`${executableName}\` is in \`$PATH\`, ` +
-        `or set \`${envVar}\` to point to a valid executable.`
-    );
+/** Mirrors `ra_toolchain::cargo()` implementation */
+export function cargoPath(): string {
+    return getPathForExecutable("cargo");
+}
+
+/** Mirrors `ra_toolchain::get_path_for_executable()` implementation */
+export const getPathForExecutable = memoize(
+    // We apply caching to decrease file-system interactions
+    (executableName: "cargo" | "rustc" | "rustup"): string => {
+        {
+            const envVar = process.env[executableName.toUpperCase()];
+            if (envVar) return envVar;
+        }
+
+        if (lookupInPath(executableName)) return executableName;
+
+        try {
+            // hmm, `os.homedir()` seems to be infallible
+            // it is not mentioned in docs and cannot be infered by the type signature...
+            const standardPath = path.join(os.homedir(), ".cargo", "bin", executableName);
+
+            if (isFile(standardPath)) return standardPath;
+        } catch (err) {
+            log.error("Failed to read the fs info", err);
+        }
+        return executableName;
+    }
+);
+
+function lookupInPath(exec: string): boolean {
+    const paths = process.env.PATH ?? "";;
+
+    const candidates = paths.split(path.delimiter).flatMap(dirInPath => {
+        const candidate = path.join(dirInPath, exec);
+        return os.type() === "Windows_NT"
+            ? [candidate, `${candidate}.exe`]
+            : [candidate];
+    });
+
+    return candidates.some(isFile);
+}
+
+function isFile(suspectPath: string): boolean {
+    // It is not mentionned in docs, but `statSync()` throws an error when
+    // the path doesn't exist
+    try {
+        return fs.statSync(suspectPath).isFile();
+    } catch {
+        return false;
+    }
 }

editors/code/src/util.ts

@@ -99,3 +99,21 @@ export function isValidExecutable(path: string): boolean {
 export function setContextValue(key: string, value: any): Thenable<void> {
     return vscode.commands.executeCommand('setContext', key, value);
 }
+
+/**
+ * Returns a higher-order function that caches the results of invoking the
+ * underlying function.
+ */
+export function memoize<Ret, TThis, Param extends string>(func: (this: TThis, arg: Param) => Ret) {
+    const cache = new Map<string, Ret>();
+
+    return function(this: TThis, arg: Param) {
+        const cached = cache.get(arg);
+        if (cached) return cached;
+
+        const result = func.call(this, arg);
+        cache.set(arg, result);
+
+        return result;
+    };
+}

editors/code/tests/unit/launch_config.test.ts

@@ -1,25 +1,25 @@
 import * as assert from 'assert';
-import * as cargo from '../../src/cargo';
+import { Cargo } from '../../src/toolchain';
 
 suite('Launch configuration', () => {
 
     suite('Lens', () => {
         test('A binary', async () => {
-            const args = cargo.artifactSpec(["build", "--package", "pkg_name", "--bin", "pkg_name"]);
+            const args = Cargo.artifactSpec(["build", "--package", "pkg_name", "--bin", "pkg_name"]);
 
             assert.deepEqual(args.cargoArgs, ["build", "--package", "pkg_name", "--bin", "pkg_name", "--message-format=json"]);
             assert.deepEqual(args.filter, undefined);
         });
 
         test('One of Multiple Binaries', async () => {
-            const args = cargo.artifactSpec(["build", "--package", "pkg_name", "--bin", "bin1"]);
+            const args = Cargo.artifactSpec(["build", "--package", "pkg_name", "--bin", "bin1"]);
 
             assert.deepEqual(args.cargoArgs, ["build", "--package", "pkg_name", "--bin", "bin1", "--message-format=json"]);
             assert.deepEqual(args.filter, undefined);
         });
 
         test('A test', async () => {
-            const args = cargo.artifactSpec(["test", "--package", "pkg_name", "--lib", "--no-run"]);
+            const args = Cargo.artifactSpec(["test", "--package", "pkg_name", "--lib", "--no-run"]);
 
             assert.deepEqual(args.cargoArgs, ["test", "--package", "pkg_name", "--lib", "--no-run", "--message-format=json"]);
             assert.notDeepEqual(args.filter, undefined);
@@ -28,7 +28,7 @@ suite('Launch configuration', () => {
 
     suite('QuickPick', () => {
         test('A binary', async () => {
-            const args = cargo.artifactSpec(["run", "--package", "pkg_name", "--bin", "pkg_name"]);
+            const args = Cargo.artifactSpec(["run", "--package", "pkg_name", "--bin", "pkg_name"]);
 
             assert.deepEqual(args.cargoArgs, ["build", "--package", "pkg_name", "--bin", "pkg_name", "--message-format=json"]);
             assert.deepEqual(args.filter, undefined);
@@ -36,14 +36,14 @@ suite('Launch configuration', () => {
 
 
         test('One of Multiple Binaries', async () => {
-            const args = cargo.artifactSpec(["run", "--package", "pkg_name", "--bin", "bin2"]);
+            const args = Cargo.artifactSpec(["run", "--package", "pkg_name", "--bin", "bin2"]);
 
             assert.deepEqual(args.cargoArgs, ["build", "--package", "pkg_name", "--bin", "bin2", "--message-format=json"]);
             assert.deepEqual(args.filter, undefined);
         });
 
         test('A test', async () => {
-            const args = cargo.artifactSpec(["test", "--package", "pkg_name", "--lib"]);
+            const args = Cargo.artifactSpec(["test", "--package", "pkg_name", "--lib"]);
 
             assert.deepEqual(args.cargoArgs, ["test", "--package", "pkg_name", "--lib", "--message-format=json", "--no-run"]);
             assert.notDeepEqual(args.filter, undefined);