Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

internal: Clean up runnable lsp extension #17547

Merged
merged 3 commits into from
Jul 6, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions crates/rust-analyzer/src/handlers/request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -847,7 +847,7 @@ pub(crate) fn handle_runnables(
if expect_test {
if let lsp_ext::RunnableArgs::Cargo(r) = &mut runnable.args {
runnable.label = format!("{} + expect", runnable.label);
r.expect_test = Some(true);
r.environment.insert("UPDATE_EXPECT".to_owned(), "1".to_owned());
}
}
res.push(runnable);
Expand All @@ -874,6 +874,7 @@ pub(crate) fn handle_runnables(
if all_targets {
cargo_args.push("--all-targets".to_owned());
}
cargo_args.extend(config.cargo_extra_args.iter().cloned());
res.push(lsp_ext::Runnable {
label: format!(
"cargo {cmd} -p {}{all_targets}",
Expand All @@ -884,31 +885,31 @@ pub(crate) fn handle_runnables(
kind: lsp_ext::RunnableKind::Cargo,
args: lsp_ext::RunnableArgs::Cargo(lsp_ext::CargoRunnableArgs {
workspace_root: Some(spec.workspace_root.clone().into()),
cwd: Some(cwd.into()),
cwd: cwd.into(),
override_cargo: config.override_cargo.clone(),
cargo_args,
cargo_extra_args: config.cargo_extra_args.clone(),
executable_args: Vec::new(),
expect_test: None,
environment: Default::default(),
}),
})
}
}
Some(TargetSpec::ProjectJson(_)) => {}
None => {
if !snap.config.linked_or_discovered_projects().is_empty() {
let mut cargo_args = vec!["check".to_owned(), "--workspace".to_owned()];
cargo_args.extend(config.cargo_extra_args.iter().cloned());
res.push(lsp_ext::Runnable {
label: "cargo check --workspace".to_owned(),
location: None,
kind: lsp_ext::RunnableKind::Cargo,
args: lsp_ext::RunnableArgs::Cargo(lsp_ext::CargoRunnableArgs {
workspace_root: None,
cwd: None,
cwd: ".".into(),
override_cargo: config.override_cargo,
cargo_args: vec!["check".to_owned(), "--workspace".to_owned()],
cargo_extra_args: config.cargo_extra_args,
cargo_args,
executable_args: Vec::new(),
expect_test: None,
environment: Default::default(),
}),
});
}
Expand Down
15 changes: 7 additions & 8 deletions crates/rust-analyzer/src/lsp/ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -460,28 +460,27 @@ pub enum RunnableKind {
#[derive(Deserialize, Serialize, Debug)]
#[serde(rename_all = "camelCase")]
pub struct CargoRunnableArgs {
// command to be executed instead of cargo
#[serde(skip_serializing_if = "FxHashMap::is_empty")]
pub environment: FxHashMap<String, String>,
pub cwd: Utf8PathBuf,
/// Command to be executed instead of cargo
pub override_cargo: Option<String>,
#[serde(skip_serializing_if = "Option::is_none")]
pub workspace_root: Option<Utf8PathBuf>,
#[serde(skip_serializing_if = "Option::is_none")]
pub cwd: Option<Utf8PathBuf>,
// command, --package and --lib stuff
pub cargo_args: Vec<String>,
// user-specified additional cargo args, like `--release`.
pub cargo_extra_args: Vec<String>,
// stuff after --
pub executable_args: Vec<String>,
#[serde(skip_serializing_if = "Option::is_none")]
pub expect_test: Option<bool>,
}

#[derive(Deserialize, Serialize, Debug)]
#[serde(rename_all = "camelCase")]
pub struct ShellRunnableArgs {
#[serde(skip_serializing_if = "FxHashMap::is_empty")]
pub environment: FxHashMap<String, String>,
pub cwd: Utf8PathBuf,
pub program: String,
pub args: Vec<String>,
pub cwd: Utf8PathBuf,
}

pub enum RelatedTests {}
Expand Down
13 changes: 6 additions & 7 deletions crates/rust-analyzer/src/lsp/to_proto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use ide::{
};
use ide_db::{rust_doc::format_docs, FxHasher};
use itertools::Itertools;
use paths::{Utf8Component, Utf8Prefix};
use paths::{Utf8Component, Utf8PathBuf, Utf8Prefix};
use semver::VersionReq;
use serde_json::to_value;
use vfs::AbsPath;
Expand Down Expand Up @@ -1390,10 +1390,9 @@ pub(crate) fn runnable(
workspace_root: Some(workspace_root.into()),
override_cargo: config.override_cargo,
cargo_args,
cwd: Some(cwd.into()),
cargo_extra_args: config.cargo_extra_args,
cwd: cwd.into(),
executable_args,
expect_test: None,
environment: Default::default(),
}),
}))
}
Expand All @@ -1407,6 +1406,7 @@ pub(crate) fn runnable(
program: json_shell_runnable_args.program,
args: json_shell_runnable_args.args,
cwd: json_shell_runnable_args.cwd,
environment: Default::default(),
};
Ok(Some(lsp_ext::Runnable {
label,
Expand All @@ -1433,10 +1433,9 @@ pub(crate) fn runnable(
workspace_root: None,
override_cargo: config.override_cargo,
cargo_args,
cwd: None,
cargo_extra_args: config.cargo_extra_args,
cwd: Utf8PathBuf::from("."),
executable_args,
expect_test: None,
environment: Default::default(),
}),
}))
}
Expand Down
4 changes: 3 additions & 1 deletion crates/rust-analyzer/src/target_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ impl CargoTargetSpec {
kind: &RunnableKind,
cfg: &Option<CfgExpr>,
) -> (Vec<String>, Vec<String>) {
let extra_test_binary_args = snap.config.runnables().extra_test_binary_args;
let config = snap.config.runnables();
let extra_test_binary_args = config.extra_test_binary_args;

let mut cargo_args = Vec::new();
let mut executable_args = Vec::new();
Expand Down Expand Up @@ -196,6 +197,7 @@ impl CargoTargetSpec {
}
}
}
cargo_args.extend(config.cargo_extra_args.iter().cloned());
(cargo_args, executable_args)
}

Expand Down
7 changes: 0 additions & 7 deletions crates/rust-analyzer/tests/slow-tests/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,6 @@ fn main() {}
"args": {
"cargoArgs": ["test", "--package", "foo", "--test", "spam"],
"executableArgs": ["test_eggs", "--exact", "--show-output"],
"cargoExtraArgs": [],
"overrideCargo": null,
"cwd": server.path().join("foo"),
"workspaceRoot": server.path().join("foo")
Expand Down Expand Up @@ -289,7 +288,6 @@ fn main() {}
"--test",
"spam"
],
"cargoExtraArgs": [],
"executableArgs": [
"",
"--show-output"
Expand Down Expand Up @@ -325,7 +323,6 @@ fn main() {}
"args": {
"cargoArgs": ["check", "--package", "foo", "--all-targets"],
"executableArgs": [],
"cargoExtraArgs": [],
"overrideCargo": null,
"cwd": server.path().join("foo"),
"workspaceRoot": server.path().join("foo")
Expand All @@ -337,7 +334,6 @@ fn main() {}
"args": {
"cargoArgs": ["test", "--package", "foo", "--all-targets"],
"executableArgs": [],
"cargoExtraArgs": [],
"overrideCargo": null,
"cwd": server.path().join("foo"),
"workspaceRoot": server.path().join("foo")
Expand Down Expand Up @@ -426,7 +422,6 @@ mod tests {
runnable,
"--all-targets"
],
"cargoExtraArgs": [],
"executableArgs": []
},
},
Expand Down Expand Up @@ -489,7 +484,6 @@ fn otherpkg() {}
"mainpkg",
"--all-targets"
],
"cargoExtraArgs": [],
"executableArgs": []
},
},
Expand All @@ -515,7 +509,6 @@ fn otherpkg() {}
"otherpkg",
"--all-targets"
],
"cargoExtraArgs": [],
"executableArgs": []
},
},
Expand Down
34 changes: 29 additions & 5 deletions docs/dev/lsp-extensions.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<!---
lsp/ext.rs hash: 8e6e340f2899b5e9
lsp/ext.rs hash: a0867710490bf8da

If you need to change the above hash to make the test pass, please check if you
need to adjust this doc as well and ping this issue:
Expand Down Expand Up @@ -376,12 +376,29 @@ rust-analyzer supports two `kind`s of runnables, `"cargo"` and `"shell"`. The `a

```typescript
{
/**
* Environment variables to set before running the command.
*/
environment?: Record<string, string>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to alternative names, should we be explicit with environmentVariables?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could also call it envVariables?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, I'll stick with environment I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough! not terribly attached.

/**
* The working directory to run the command in.
*/
cwd: string;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this being mandatory gives a more unique experience across clients. We should not leave this decision to clients as that could result in runnables sometimes working sometimes not. (notable it is required by the shell version already)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, reducing the number of possible options is great. the combinatorial explosion of options made this really difficult to reason about.

/**
* The workspace root directory of the cargo project.
*/
workspaceRoot?: string;
cwd?: string;
/**
* The cargo command to run.
*/
cargoArgs: string[];
cargoExtraArgs: string[];
/**
* Arguments to pass to the executable, these will be passed to the command after a `--` argument.
*/
executableArgs: string[];
expectTest?: boolean;
/**
* Command to execute instead of `cargo`.
*/
overrideCargo?: string;
}
```
Expand All @@ -390,10 +407,17 @@ The args for `"shell"` look like this:

```typescript
{
/**
* Environment variables to set before running the command.
*/
environment?: Record<string, string>;
/**
* The working directory to run the command in.
*/
cwd: string;
kind: string;
program: string;
args: string[];
cwd: string;
}
```

Expand Down
35 changes: 28 additions & 7 deletions editors/code/src/lsp_ext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -235,22 +235,43 @@ type RunnableShell = {
args: ShellRunnableArgs;
};

export type CommonRunnableArgs = {
/**
* Environment variables to set before running the command.
*/
environment?: Record<string, string>;
/**
* The working directory to run the command in.
*/
cwd: string;
};

export type ShellRunnableArgs = {
kind: string;
program: string;
args: string[];
cwd: string;
};
} & CommonRunnableArgs;

export type CargoRunnableArgs = {
/**
* The workspace root directory of the cargo project.
*/
workspaceRoot?: string;
cargoArgs: string[];
cwd: string;
cargoExtraArgs: string[];
/**
* Arguments to pass to the executable, these will be passed to the command after a `--` argument.
*/
executableArgs: string[];
expectTest?: boolean;
/**
* Arguments to pass to cargo.
*/
cargoArgs: string[];
/**
* Command to execute instead of `cargo`.
*/
// This is supplied by the user via config. We could pull this through the client config in the
// extension directly, but that would prevent us from honoring the rust-analyzer.toml for it.
overrideCargo?: string;
};
} & CommonRunnableArgs;

export type RunnablesParams = {
textDocument: lc.TextDocumentIdentifier;
Expand Down
17 changes: 6 additions & 11 deletions editors/code/src/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,12 @@ export class RunnableQuickPick implements vscode.QuickPickItem {
}
}

export function prepareBaseEnv(): Record<string, string> {
export function prepareBaseEnv(base?: Record<string, string>): Record<string, string> {
const env: Record<string, string> = { RUST_BACKTRACE: "short" };
Object.assign(env, process.env as { [key: string]: string });
Object.assign(env, process.env);
if (base) {
Object.assign(env, base);
}
return env;
}

Expand All @@ -77,12 +80,7 @@ export function prepareEnv(
runnableArgs: ra.CargoRunnableArgs,
runnableEnvCfg: RunnableEnvCfg,
): Record<string, string> {
const env = prepareBaseEnv();

if (runnableArgs.expectTest) {
env["UPDATE_EXPECT"] = "1";
}

const env = prepareBaseEnv(runnableArgs.environment);
const platform = process.platform;

const checkPlatform = (it: RunnableEnvCfgItem) => {
Expand Down Expand Up @@ -167,9 +165,6 @@ export async function createTaskFromRunnable(

export function createCargoArgs(runnableArgs: ra.CargoRunnableArgs): string[] {
const args = [...runnableArgs.cargoArgs]; // should be a copy!
if (runnableArgs.cargoExtraArgs) {
args.push(...runnableArgs.cargoExtraArgs); // Append user-specified cargo options.
}
if (runnableArgs.executableArgs.length > 0) {
args.push("--", ...runnableArgs.executableArgs);
}
Expand Down
1 change: 0 additions & 1 deletion editors/code/tests/unit/runnable_env.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ function makeRunnable(label: string): ra.Runnable {
cargoArgs: [],
cwd: ".",
executableArgs: [],
cargoExtraArgs: [],
},
};
}
Expand Down