Auto merge of #17549 - Veykril:runnables-fix, r=Veykril

fix: Fix runnables being incorrectly constructed

I've misunderstood parts of the code here which caused runnables to arbitrarily break :) (I have yet to understand the conditions that made them break though, there is some odd caching involved I feel like ...)
Fixes #17402

editors/code/package.json

@@ -333,14 +333,6 @@
             {
                 "title": "general",
                 "properties": {
-                    "rust-analyzer.cargoRunner": {
-                        "type": [
-                            "null",
-                            "string"
-                        ],
-                        "default": null,
-                        "description": "Custom cargo runner extension ID."
-                    },
                     "rust-analyzer.restartServerOnConfigChange": {
                         "markdownDescription": "Whether to restart the server automatically when certain settings that require a restart are changed.",
                         "default": false,

editors/code/src/bootstrap.ts

@@ -36,14 +36,20 @@ async function getServer(
     config: Config,
     state: PersistentState,
 ): Promise<string | undefined> {
+    const packageJson: {
+        version: string;
+        releaseTag: string | null;
+        enableProposedApi: boolean | undefined;
+    } = context.extension.packageJSON;
+
     const explicitPath = process.env["__RA_LSP_SERVER_DEBUG"] ?? config.serverPath;
     if (explicitPath) {
         if (explicitPath.startsWith("~/")) {
             return os.homedir() + explicitPath.slice("~".length);
         }
         return explicitPath;
     }
-    if (config.package.releaseTag === null) return "rust-analyzer";
+    if (packageJson.releaseTag === null) return "rust-analyzer";
 
     const ext = process.platform === "win32" ? ".exe" : "";
     const bundled = vscode.Uri.joinPath(context.extensionUri, "server", `rust-analyzer${ext}`);
@@ -54,8 +60,15 @@ async function getServer(
     if (bundledExists) {
         let server = bundled;
         if (await isNixOs()) {
-            server = await getNixOsServer(config, ext, state, bundled, server);
-            await state.updateServerVersion(config.package.version);
+            server = await getNixOsServer(
+                context.globalStorageUri,
+                packageJson.version,
+                ext,
+                state,
+                bundled,
+                server,
+            );
+            await state.updateServerVersion(packageJson.version);
         }
         return server.fsPath;
     }
@@ -86,19 +99,20 @@ export function isValidExecutable(path: string, extraEnv: Env): boolean {
 }
 
 async function getNixOsServer(
-    config: Config,
+    globalStorageUri: vscode.Uri,
+    version: string,
     ext: string,
     state: PersistentState,
     bundled: vscode.Uri,
     server: vscode.Uri,
 ) {
-    await vscode.workspace.fs.createDirectory(config.globalStorageUri).then();
-    const dest = vscode.Uri.joinPath(config.globalStorageUri, `rust-analyzer${ext}`);
+    await vscode.workspace.fs.createDirectory(globalStorageUri).then();
+    const dest = vscode.Uri.joinPath(globalStorageUri, `rust-analyzer${ext}`);
     let exists = await vscode.workspace.fs.stat(dest).then(
         () => true,
         () => false,
     );
-    if (exists && config.package.version !== state.serverVersion) {
+    if (exists && version !== state.serverVersion) {
         await vscode.workspace.fs.delete(dest);
         exists = false;
     }

editors/code/src/config.ts

@@ -4,6 +4,7 @@ import * as path from "path";
 import * as vscode from "vscode";
 import { type Env, log, unwrapUndefinable, expectNotUndefined } from "./util";
 import type { JsonProject } from "./rust_project";
+import type { Disposable } from "./ctx";
 
 export type RunnableEnvCfgItem = {
     mask?: string;
@@ -29,22 +30,9 @@ export class Config {
         (opt) => `${this.rootSection}.${opt}`,
     );
 
-    readonly package: {
-        version: string;
-        releaseTag: string | null;
-        enableProposedApi: boolean | undefined;
-    } = vscode.extensions.getExtension(this.extensionId)!.packageJSON;
-
-    readonly globalStorageUri: vscode.Uri;
-
-    constructor(ctx: vscode.ExtensionContext) {
-        this.globalStorageUri = ctx.globalStorageUri;
+    constructor(disposables: Disposable[]) {
         this.discoveredWorkspaces = [];
-        vscode.workspace.onDidChangeConfiguration(
-            this.onDidChangeConfiguration,
-            this,
-            ctx.subscriptions,
-        );
+        vscode.workspace.onDidChangeConfiguration(this.onDidChangeConfiguration, this, disposables);
         this.refreshLogging();
         this.configureLanguage();
     }
@@ -55,7 +43,10 @@ export class Config {
 
     private refreshLogging() {
         log.setEnabled(this.traceExtension ?? false);
-        log.info("Extension version:", this.package.version);
+        log.info(
+            "Extension version:",
+            vscode.extensions.getExtension(this.extensionId)!.packageJSON.version,
+        );
 
         const cfg = Object.entries(this.cfg).filter(([_, val]) => !(val instanceof Function));
         log.info("Using configuration", Object.fromEntries(cfg));
@@ -277,10 +268,6 @@ export class Config {
         return this.get<string[]>("runnables.problemMatcher") || [];
     }
 
-    get cargoRunner() {
-        return this.get<string | undefined>("cargoRunner");
-    }
-
     get testExplorer() {
         return this.get<boolean | undefined>("testExplorer");
     }

editors/code/src/ctx.ts

@@ -118,7 +118,7 @@ export class Ctx implements RustAnalyzerExtensionApi {
         extCtx.subscriptions.push(this);
         this.version = extCtx.extension.packageJSON.version ?? "<unknown>";
         this._serverVersion = "<not running>";
-        this.config = new Config(extCtx);
+        this.config = new Config(extCtx.subscriptions);
         this.statusBar = vscode.window.createStatusBarItem(vscode.StatusBarAlignment.Left);
         if (this.config.testExplorer) {
             this.testController = vscode.tests.createTestController(

editors/code/src/run.ts

@@ -111,26 +111,31 @@ export async function createTaskFromRunnable(
     runnable: ra.Runnable,
     config: Config,
 ): Promise<vscode.Task> {
-    let definition: tasks.RustTargetDefinition;
+    const target = vscode.workspace.workspaceFolders?.[0];
+
+    let definition: tasks.TaskDefinition;
+    let options;
+    let cargo;
     if (runnable.kind === "cargo") {
         const runnableArgs = runnable.args;
         let args = createCargoArgs(runnableArgs);
 
-        let program: string;
         if (runnableArgs.overrideCargo) {
             // Split on spaces to allow overrides like "wrapper cargo".
             const cargoParts = runnableArgs.overrideCargo.split(" ");
 
-            program = unwrapUndefinable(cargoParts[0]);
+            cargo = unwrapUndefinable(cargoParts[0]);
             args = [...cargoParts.slice(1), ...args];
         } else {
-            program = await toolchain.cargoPath();
+            cargo = await toolchain.cargoPath();
         }
 
         definition = {
             type: tasks.CARGO_TASK_TYPE,
-            command: program,
-            args,
+            command: unwrapUndefinable(args[0]),
+            args: args.slice(1),
+        };
+        options = {
             cwd: runnableArgs.workspaceRoot || ".",
             env: prepareEnv(runnable.label, runnableArgs, config.runnablesExtraEnv),
         };
@@ -140,13 +145,14 @@ export async function createTaskFromRunnable(
             type: tasks.SHELL_TASK_TYPE,
             command: runnableArgs.program,
             args: runnableArgs.args,
+        };
+        options = {
             cwd: runnableArgs.cwd,
             env: prepareBaseEnv(),
         };
     }
 
-    const target = vscode.workspace.workspaceFolders?.[0];
-    const exec = await tasks.targetToExecution(definition, config.cargoRunner, true);
+    const exec = await tasks.targetToExecution(definition, options, cargo);
     const task = await tasks.buildRustTask(
         target,
         definition,

editors/code/src/tasks.ts

@@ -1,6 +1,5 @@
 import * as vscode from "vscode";
 import type { Config } from "./config";
-import { log, unwrapUndefinable } from "./util";
 import * as toolchain from "./toolchain";
 
 // This ends up as the `type` key in tasks.json. RLS also uses `cargo` and
@@ -10,21 +9,21 @@ export const SHELL_TASK_TYPE = "shell";
 
 export const RUST_TASK_SOURCE = "rust";
 
-export type RustTargetDefinition = {
+export type TaskDefinition = vscode.TaskDefinition & {
     readonly type: typeof CARGO_TASK_TYPE | typeof SHELL_TASK_TYPE;
-} & vscode.TaskDefinition &
-    RustTarget;
-export type RustTarget = {
-    // The command to run, usually `cargo`.
-    command: string;
-    // Additional arguments passed to the command.
     args?: string[];
-    // The working directory to run the command in.
-    cwd?: string;
-    // The shell environment.
-    env?: { [key: string]: string };
+    command: string;
 };
 
+export type CargoTaskDefinition = {
+    env?: Record<string, string>;
+    type: typeof CARGO_TASK_TYPE;
+} & TaskDefinition;
+
+function isCargoTask(definition: vscode.TaskDefinition): definition is CargoTaskDefinition {
+    return definition.type === CARGO_TASK_TYPE;
+}
+
 class RustTaskProvider implements vscode.TaskProvider {
     private readonly config: Config;
 
@@ -58,13 +57,13 @@ class RustTaskProvider implements vscode.TaskProvider {
         for (const workspaceTarget of vscode.workspace.workspaceFolders) {
             for (const def of defs) {
                 const definition = {
-                    command: cargo,
-                    args: [def.command],
-                };
-                const exec = await targetToExecution(definition, this.config.cargoRunner);
+                    command: def.command,
+                    type: CARGO_TASK_TYPE,
+                } as const;
+                const exec = await targetToExecution(definition, {}, cargo);
                 const vscodeTask = await buildRustTask(
                     workspaceTarget,
-                    { ...definition, type: CARGO_TASK_TYPE },
+                    definition,
                     `cargo ${def.command}`,
                     this.config.problemMatcher,
                     exec,
@@ -81,23 +80,13 @@ class RustTaskProvider implements vscode.TaskProvider {
         // VSCode calls this for every cargo task in the user's tasks.json,
         // we need to inform VSCode how to execute that command by creating
         // a ShellExecution for it.
-        if (task.definition.type === CARGO_TASK_TYPE) {
-            const taskDefinition = task.definition as RustTargetDefinition;
-            const cargo = await toolchain.cargoPath();
-            const exec = await targetToExecution(
-                {
-                    command: cargo,
-                    args: [taskDefinition.command].concat(taskDefinition.args || []),
-                    cwd: taskDefinition.cwd,
-                    env: taskDefinition.env,
-                },
-                this.config.cargoRunner,
-            );
-            return await buildRustTask(
+        if (isCargoTask(task.definition)) {
+            const exec = await targetToExecution(task.definition, { env: task.definition.env });
+            return buildRustTask(
                 task.scope,
-                taskDefinition,
+                task.definition,
                 task.name,
-                this.config.problemMatcher,
+                task.problemMatchers,
                 exec,
             );
         }
@@ -108,7 +97,7 @@ class RustTaskProvider implements vscode.TaskProvider {
 
 export async function buildRustTask(
     scope: vscode.WorkspaceFolder | vscode.TaskScope | undefined,
-    definition: RustTargetDefinition,
+    definition: TaskDefinition,
     name: string,
     problemMatcher: string[],
     exec: vscode.ProcessExecution | vscode.ShellExecution,
@@ -126,40 +115,23 @@ export async function buildRustTask(
 }
 
 export async function targetToExecution(
-    definition: RustTarget,
-    customRunner?: string,
-    throwOnError: boolean = false,
+    definition: TaskDefinition,
+    options?: {
+        env?: { [key: string]: string };
+        cwd?: string;
+    },
+    cargo?: string,
 ): Promise<vscode.ProcessExecution | vscode.ShellExecution> {
-    if (customRunner) {
-        const runnerCommand = `${customRunner}.buildShellExecution`;
-
-        try {
-            const runnerArgs = {
-                kind: CARGO_TASK_TYPE,
-                args: definition.args,
-                cwd: definition.cwd,
-                env: definition.env,
-            };
-            const customExec = await vscode.commands.executeCommand(runnerCommand, runnerArgs);
-            if (customExec) {
-                if (customExec instanceof vscode.ShellExecution) {
-                    return customExec;
-                } else {
-                    log.debug("Invalid cargo ShellExecution", customExec);
-                    throw "Invalid cargo ShellExecution.";
-                }
-            }
-            // fallback to default processing
-        } catch (e) {
-            if (throwOnError) throw `Cargo runner '${customRunner}' failed! ${e}`;
-            // fallback to default processing
-        }
+    let command, args;
+    if (isCargoTask(definition)) {
+        // FIXME: The server should provide cargo
+        command = cargo || (await toolchain.cargoPath());
+        args = [definition.command].concat(definition.args || []);
+    } else {
+        command = definition.command;
+        args = definition.args || [];
     }
-    const args = unwrapUndefinable(definition.args);
-    return new vscode.ProcessExecution(definition.command, args, {
-        cwd: definition.cwd,
-        env: definition.env,
-    });
+    return new vscode.ProcessExecution(command, args, options);
 }
 
 export function activateTaskProvider(config: Config): vscode.Disposable {

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

@@ -13,7 +13,7 @@ export async function getTests(ctx: Context) {
                 USING_MY_VAR: "test test test",
                 MY_VAR: "test",
             };
-            const actualEnv = await substituteVariablesInEnv(envJson);
+            const actualEnv = substituteVariablesInEnv(envJson);
             assert.deepStrictEqual(actualEnv, expectedEnv);
         });
 
@@ -34,7 +34,7 @@ export async function getTests(ctx: Context) {
                 E_IS_ISOLATED: "test",
                 F_USES_E: "test",
             };
-            const actualEnv = await substituteVariablesInEnv(envJson);
+            const actualEnv = substituteVariablesInEnv(envJson);
             assert.deepStrictEqual(actualEnv, expectedEnv);
         });
 
@@ -47,7 +47,7 @@ export async function getTests(ctx: Context) {
                 USING_EXTERNAL_VAR: "test test test",
             };
 
-            const actualEnv = await substituteVariablesInEnv(envJson);
+            const actualEnv = substituteVariablesInEnv(envJson);
             assert.deepStrictEqual(actualEnv, expectedEnv);
             delete process.env["TEST_VARIABLE"];
         });
@@ -56,7 +56,7 @@ export async function getTests(ctx: Context) {
             const envJson = {
                 USING_VSCODE_VAR: "${workspaceFolderBasename}",
             };
-            const actualEnv = await substituteVariablesInEnv(envJson);
+            const actualEnv = substituteVariablesInEnv(envJson);
             assert.deepStrictEqual(actualEnv["USING_VSCODE_VAR"], "code");
         });
     });