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

feature: Substitute $saved_file in custom check commands #15476

Merged
merged 1 commit into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
59 changes: 48 additions & 11 deletions crates/flycheck/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use std::{

use command_group::{CommandGroup, GroupChild};
use crossbeam_channel::{never, select, unbounded, Receiver, Sender};
use paths::AbsPathBuf;
use paths::{AbsPath, AbsPathBuf};
use rustc_hash::FxHashMap;
use serde::Deserialize;
use stdx::process::streaming_output;
Expand Down Expand Up @@ -102,13 +102,15 @@ impl FlycheckHandle {
}

/// Schedule a re-start of the cargo check worker to do a workspace wide check.
pub fn restart_workspace(&self) {
self.sender.send(StateChange::Restart(None)).unwrap();
pub fn restart_workspace(&self, saved_file: Option<AbsPathBuf>) {
self.sender.send(StateChange::Restart { package: None, saved_file }).unwrap();
}

/// Schedule a re-start of the cargo check worker to do a package wide check.
pub fn restart_for_package(&self, package: String) {
self.sender.send(StateChange::Restart(Some(package))).unwrap();
self.sender
.send(StateChange::Restart { package: Some(package), saved_file: None })
.unwrap();
}

/// Stop this cargo check worker.
Expand Down Expand Up @@ -159,7 +161,7 @@ pub enum Progress {
}

enum StateChange {
Restart(Option<String>),
Restart { package: Option<String>, saved_file: Option<AbsPathBuf> },
Cancel,
}

Expand All @@ -186,6 +188,8 @@ enum Event {
CheckEvent(Option<CargoMessage>),
}

const SAVED_FILE_PLACEHOLDER: &str = "$saved_file";

impl FlycheckActor {
fn new(
id: usize,
Expand Down Expand Up @@ -221,7 +225,7 @@ impl FlycheckActor {
tracing::debug!(flycheck_id = self.id, "flycheck cancelled");
self.cancel_check_process();
}
Event::RequestStateChange(StateChange::Restart(package)) => {
Event::RequestStateChange(StateChange::Restart { package, saved_file }) => {
// Cancel the previously spawned process
self.cancel_check_process();
while let Ok(restart) = inbox.recv_timeout(Duration::from_millis(50)) {
Expand All @@ -231,7 +235,11 @@ impl FlycheckActor {
}
}

let command = self.check_command(package.as_deref());
let command =
match self.check_command(package.as_deref(), saved_file.as_deref()) {
Some(c) => c,
None => continue,
};
let formatted_command = format!("{:?}", command);

tracing::debug!(?command, "will restart flycheck");
Expand Down Expand Up @@ -305,7 +313,14 @@ impl FlycheckActor {
}
}

fn check_command(&self, package: Option<&str>) -> Command {
/// Construct a `Command` object for checking the user's code. If the user
/// has specified a custom command with placeholders that we cannot fill,
/// return None.
fn check_command(
&self,
package: Option<&str>,
saved_file: Option<&AbsPath>,
) -> Option<Command> {
let (mut cmd, args) = match &self.config {
FlycheckConfig::CargoCommand {
command,
Expand Down Expand Up @@ -358,7 +373,7 @@ impl FlycheckActor {
cmd.arg("--target-dir").arg(target_dir);
}
cmd.envs(extra_env);
(cmd, extra_args)
(cmd, extra_args.clone())
}
FlycheckConfig::CustomCommand {
command,
Expand Down Expand Up @@ -387,12 +402,34 @@ impl FlycheckActor {
}
}

(cmd, args)
if args.contains(&SAVED_FILE_PLACEHOLDER.to_owned()) {
davidbarsky marked this conversation as resolved.
Show resolved Hide resolved
// If the custom command has a $saved_file placeholder, and
// we're saving a file, replace the placeholder in the arguments.
if let Some(saved_file) = saved_file {
let args = args
.iter()
.map(|arg| {
if arg == SAVED_FILE_PLACEHOLDER {
saved_file.to_string()
} else {
arg.clone()
}
})
.collect();
(cmd, args)
} else {
// The custom command has a $saved_file placeholder,
// but we had an IDE event that wasn't a file save. Do nothing.
return None;
Copy link
Member

Choose a reason for hiding this comment

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

This means there's going to be no initial check when loading the project, but I don't see any way around it without configuring two check commands.

I guess this is not a problem for you, but Cargo users trying this might still be surprised (ref. #12882).

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose a workaround would be #15892, but yes, in this PR? it might confuse Cargo users.

(That being said, this is very much not a feature for Cargo, but I think that #15892 could make it a feature for Cargo users and resolve #12882 in the process).

Copy link
Contributor

Choose a reason for hiding this comment

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

An example this should work for is rust-lang/rust's cargo wrapper, ./x.py check.

I reckon: If the command has $saved_file in it, return None. If it doesn't, return Some, as it does not require substitution. That brings back the current behaviour until you start using $saved_file.

}
} else {
(cmd, args.clone())
}
}
};

cmd.args(args);
cmd
Some(cmd)
}

fn send(&self, check_task: Message) {
Expand Down
5 changes: 5 additions & 0 deletions crates/rust-analyzer/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,11 @@ config_data! {
/// by changing `#rust-analyzer.check.invocationStrategy#` and
/// `#rust-analyzer.check.invocationLocation#`.
///
/// If `$saved_file` is part of the command, rust-analyzer will pass
/// the absolute path of the saved file to the provided command. This is
/// intended to be used with non-Cargo build systems.
/// Note that `$saved_file` is experimental and may be removed in the futureg.
///
/// An example command would be:
///
/// ```bash
Expand Down
10 changes: 6 additions & 4 deletions crates/rust-analyzer/src/handlers/notification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ pub(crate) fn handle_did_save_text_document(
} else if state.config.check_on_save() {
// No specific flycheck was triggered, so let's trigger all of them.
for flycheck in state.flycheck.iter() {
flycheck.restart_workspace();
flycheck.restart_workspace(None);
}
}
Ok(())
Expand Down Expand Up @@ -314,14 +314,16 @@ fn run_flycheck(state: &mut GlobalState, vfs_path: VfsPath) -> bool {
Some((idx, package))
});

let saved_file = vfs_path.as_path().map(|p| p.to_owned());

// Find and trigger corresponding flychecks
for flycheck in world.flycheck.iter() {
for (id, package) in workspace_ids.clone() {
if id == flycheck.id() {
updated = true;
match package.filter(|_| !world.config.flycheck_workspace()) {
Some(package) => flycheck.restart_for_package(package),
None => flycheck.restart_workspace(),
None => flycheck.restart_workspace(saved_file.clone()),
}
continue;
}
Expand All @@ -330,7 +332,7 @@ fn run_flycheck(state: &mut GlobalState, vfs_path: VfsPath) -> bool {
// No specific flycheck was triggered, so let's trigger all of them.
if !updated {
for flycheck in world.flycheck.iter() {
flycheck.restart_workspace();
flycheck.restart_workspace(saved_file.clone());
}
}
Ok(())
Expand Down Expand Up @@ -372,7 +374,7 @@ pub(crate) fn handle_run_flycheck(
}
// No specific flycheck was triggered, so let's trigger all of them.
for flycheck in state.flycheck.iter() {
flycheck.restart_workspace();
flycheck.restart_workspace(None);
}
Ok(())
}
3 changes: 1 addition & 2 deletions crates/rust-analyzer/src/main_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ use std::{

use always_assert::always;
use crossbeam_channel::{select, Receiver};
use flycheck::FlycheckHandle;
use ide_db::base_db::{SourceDatabaseExt, VfsPath};
use lsp_server::{Connection, Notification, Request};
use lsp_types::notification::Notification as _;
Expand Down Expand Up @@ -337,7 +336,7 @@ impl GlobalState {
if became_quiescent {
if self.config.check_on_save() {
// Project has loaded properly, kick off initial flycheck
self.flycheck.iter().for_each(FlycheckHandle::restart_workspace);
self.flycheck.iter().for_each(|flycheck| flycheck.restart_workspace(None));
}
if self.config.prefill_caches() {
self.prime_caches_queue.request_op("became quiescent".to_owned(), ());
Expand Down
5 changes: 5 additions & 0 deletions docs/user/generated_config.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,11 @@ each of them, with the working directory being the workspace root
by changing `#rust-analyzer.check.invocationStrategy#` and
`#rust-analyzer.check.invocationLocation#`.

If `$saved_file` is part of the command, rust-analyzer will pass
the absolute path of the saved file to the provided command. This is
intended to be used with non-Cargo build systems.
Comment on lines +237 to +239
Copy link
Member

Choose a reason for hiding this comment

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

Given my comment, let's make clear that this is only a temporary thing and will likely be removed in the future

Note that `$saved_file` is experimental and may be removed in the futureg.

An example command would be:

```bash
Expand Down
2 changes: 1 addition & 1 deletion editors/code/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -775,7 +775,7 @@
]
},
"rust-analyzer.check.overrideCommand": {
"markdownDescription": "Override the command rust-analyzer uses instead of `cargo check` for\ndiagnostics on save. The command is required to output json and\nshould therefore include `--message-format=json` or a similar option\n(if your client supports the `colorDiagnosticOutput` experimental\ncapability, you can use `--message-format=json-diagnostic-rendered-ansi`).\n\nIf you're changing this because you're using some tool wrapping\nCargo, you might also want to change\n`#rust-analyzer.cargo.buildScripts.overrideCommand#`.\n\nIf there are multiple linked projects/workspaces, this command is invoked for\neach of them, with the working directory being the workspace root\n(i.e., the folder containing the `Cargo.toml`). This can be overwritten\nby changing `#rust-analyzer.check.invocationStrategy#` and\n`#rust-analyzer.check.invocationLocation#`.\n\nAn example command would be:\n\n```bash\ncargo check --workspace --message-format=json --all-targets\n```\n.",
"markdownDescription": "Override the command rust-analyzer uses instead of `cargo check` for\ndiagnostics on save. The command is required to output json and\nshould therefore include `--message-format=json` or a similar option\n(if your client supports the `colorDiagnosticOutput` experimental\ncapability, you can use `--message-format=json-diagnostic-rendered-ansi`).\n\nIf you're changing this because you're using some tool wrapping\nCargo, you might also want to change\n`#rust-analyzer.cargo.buildScripts.overrideCommand#`.\n\nIf there are multiple linked projects/workspaces, this command is invoked for\neach of them, with the working directory being the workspace root\n(i.e., the folder containing the `Cargo.toml`). This can be overwritten\nby changing `#rust-analyzer.check.invocationStrategy#` and\n`#rust-analyzer.check.invocationLocation#`.\n\nIf `$saved_file` is part of the command, rust-analyzer will pass\nthe absolute path of the saved file to the provided command. This is\nintended to be used with non-Cargo build systems.\nNote that `$saved_file` is experimental and may be removed in the futureg.\n\nAn example command would be:\n\n```bash\ncargo check --workspace --message-format=json --all-targets\n```\n.",
"default": null,
"type": [
"null",
Expand Down
Loading