Skip to content

flycheck: initial implementation of $saved_file #15381

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

Closed
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
129 changes: 93 additions & 36 deletions crates/flycheck/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@
#![warn(rust_2018_idioms, unused_lifetimes, semicolon_in_expressions_from_macros)]

use std::{
cell::OnceCell,
fmt, io,
process::{ChildStderr, ChildStdout, Command, Stdio},
time::Duration,
};

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 @@ -55,6 +56,7 @@ pub enum FlycheckConfig {
extra_env: FxHashMap<String, String>,
invocation_strategy: InvocationStrategy,
invocation_location: InvocationLocation,
invoke_with_saved_file: bool,
},
}

Expand All @@ -69,6 +71,15 @@ impl fmt::Display for FlycheckConfig {
}
}

impl FlycheckConfig {
pub fn invoke_with_saved_file(&self) -> bool {
match self {
FlycheckConfig::CustomCommand { invoke_with_saved_file, .. } => *invoke_with_saved_file,
_ => false,
}
}
Comment on lines +76 to +80
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
match self {
FlycheckConfig::CustomCommand { invoke_with_saved_file, .. } => *invoke_with_saved_file,
_ => false,
}
}
matches!(self, FlycheckConfig::CustomCommand { invoke_with_saved_file: true, .. })
}

}

/// Flycheck wraps the shared state and communication machinery used for
/// running `cargo check` (or other compatible command) and providing
/// diagnostics based on the output.
Expand Down Expand Up @@ -98,8 +109,8 @@ impl FlycheckHandle {
}

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

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

enum StateChange {
Restart,
Restart { saved_file: Option<AbsPathBuf> },
Cancel,
}

Expand All @@ -163,6 +174,7 @@ struct FlycheckActor {
/// Either the workspace root of the workspace we are flychecking,
/// or the project root of the project.
root: AbsPathBuf,
state: OnceCell<FlycheckState>,
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for having this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It solves a cancellation-related bug: a flycheck would be started, canceled before completion, and restarted. The restarted flycheck would then not interpolate the $saved_file value.

I think with some deeper changes to Flycheck, this can be replaced by a last_saved_file, but this introduction of statefulness one reason I have some misgivings about this approach.

/// CargoHandle exists to wrap around the communication needed to be able to
/// run `cargo check` without blocking. Currently the Rust standard library
/// doesn't provide a way to read sub-process output without blocking, so we
Expand All @@ -171,6 +183,11 @@ struct FlycheckActor {
cargo_handle: Option<CargoHandle>,
}

#[derive(Debug)]
struct FlycheckState {
command: Command,
}

enum Event {
RequestStateChange(StateChange),
CheckEvent(Option<CargoMessage>),
Expand All @@ -184,7 +201,14 @@ impl FlycheckActor {
workspace_root: AbsPathBuf,
) -> FlycheckActor {
tracing::info!(%id, ?workspace_root, "Spawning flycheck");
FlycheckActor { id, sender, config, root: workspace_root, cargo_handle: None }
FlycheckActor {
id,
sender,
config,
root: workspace_root,
state: OnceCell::new(),
cargo_handle: None,
}
}

fn report_progress(&self, progress: Progress) {
Expand All @@ -210,7 +234,7 @@ impl FlycheckActor {
tracing::debug!(flycheck_id = self.id, "flycheck cancelled");
self.cancel_check_process();
}
Event::RequestStateChange(StateChange::Restart) => {
Event::RequestStateChange(StateChange::Restart { saved_file }) => {
// Cancel the previously spawned process
self.cancel_check_process();
while let Ok(restart) = inbox.recv_timeout(Duration::from_millis(50)) {
Expand All @@ -220,22 +244,34 @@ impl FlycheckActor {
}
}

let command = self.check_command();
tracing::debug!(?command, "will restart flycheck");
match CargoHandle::spawn(command) {
let command = self.make_check_command(saved_file.as_deref());
let state = FlycheckState { command };
match self.state.get_mut() {
Some(old_state) => *old_state = state,
None => {
self.state.set(state).expect(
"Unreachable code, as the state of the OnceCell was checked.",
);
}
};

tracing::debug!(state = ?self.config, "restarting flycheck");

let command = self.state.get_mut().unwrap();

match CargoHandle::spawn(&mut command.command) {
Ok(cargo_handle) => {
tracing::debug!(
command = ?self.check_command(),
"did restart flycheck"
command = ?self.state,
"did restart flycheck"
);
self.cargo_handle = Some(cargo_handle);
self.report_progress(Progress::DidStart);
}
Err(error) => {
self.report_progress(Progress::DidFailToRestart(format!(
"Failed to run the following command: {:?} error={}",
self.check_command(),
error
&self.state, error
)));
}
}
Expand All @@ -249,7 +285,7 @@ impl FlycheckActor {
if res.is_err() {
tracing::error!(
"Flycheck failed to run the following command: {:?}",
self.check_command()
self.config
);
}
self.report_progress(Progress::DidFinish(res));
Expand Down Expand Up @@ -285,16 +321,13 @@ impl FlycheckActor {

fn cancel_check_process(&mut self) {
if let Some(cargo_handle) = self.cargo_handle.take() {
tracing::debug!(
command = ?self.check_command(),
"did cancel flycheck"
);
tracing::debug!(command = ?self.config, "did cancel flycheck");
cargo_handle.cancel();
self.report_progress(Progress::DidCancel);
}
}

fn check_command(&self) -> Command {
fn make_check_command(&self, saved_file: Option<&AbsPath>) -> Command {
let (mut cmd, args) = match &self.config {
FlycheckConfig::CargoCommand {
command,
Expand Down Expand Up @@ -339,14 +372,15 @@ impl FlycheckActor {
}
}
cmd.envs(extra_env);
(cmd, extra_args)
(cmd, extra_args.clone())
}
FlycheckConfig::CustomCommand {
command,
args,
extra_env,
invocation_strategy,
invocation_location,
invoke_with_saved_file,
} => {
let mut cmd = Command::new(command);
cmd.envs(extra_env);
Expand All @@ -368,11 +402,29 @@ impl FlycheckActor {
}
}

(cmd, args)
if *invoke_with_saved_file {
match (args.iter().position(|arg| arg == "$saved_file"), saved_file) {
(Some(i), Some(saved_file)) => {
let mut args = args.clone();
args[i] = saved_file.to_string();
(cmd, args)
}
_ => {
tracing::error!(
?saved_file,
"the saved file is missing. This is likely a bug."
);
(cmd, args.clone())
}
}
} else {
(cmd, args.clone())
}
}
};

cmd.args(args);

cmd
}

Expand Down Expand Up @@ -400,7 +452,7 @@ struct CargoHandle {
}

impl CargoHandle {
fn spawn(mut command: Command) -> std::io::Result<CargoHandle> {
fn spawn(command: &mut Command) -> std::io::Result<CargoHandle> {
command.stdout(Stdio::piped()).stderr(Stdio::piped()).stdin(Stdio::null());
let mut child = command.group_spawn().map(JodGroupChild)?;

Expand Down Expand Up @@ -464,23 +516,28 @@ impl CargoActor {
// Try to deserialize a message from Cargo or Rustc.
let mut deserializer = serde_json::Deserializer::from_str(line);
deserializer.disable_recursion_limit();
if let Ok(message) = JsonMessage::deserialize(&mut deserializer) {
match message {
// Skip certain kinds of messages to only spend time on what's useful
JsonMessage::Cargo(message) => match message {
cargo_metadata::Message::CompilerArtifact(artifact) if !artifact.fresh => {
self.sender.send(CargoMessage::CompilerArtifact(artifact)).unwrap();
}
cargo_metadata::Message::CompilerMessage(msg) => {
self.sender.send(CargoMessage::Diagnostic(msg.message)).unwrap();
match JsonMessage::deserialize(&mut deserializer) {
Ok(message) => {
match message {
// Skip certain kinds of messages to only spend time on what's useful
JsonMessage::Cargo(message) => match message {
cargo_metadata::Message::CompilerArtifact(artifact)
if !artifact.fresh =>
{
self.sender.send(CargoMessage::CompilerArtifact(artifact)).unwrap();
}
cargo_metadata::Message::CompilerMessage(msg) => {
self.sender.send(CargoMessage::Diagnostic(msg.message)).unwrap();
}
_ => (),
},
JsonMessage::Rustc(message) => {
self.sender.send(CargoMessage::Diagnostic(message)).unwrap();
}
_ => (),
},
JsonMessage::Rustc(message) => {
self.sender.send(CargoMessage::Diagnostic(message)).unwrap();
}
return true;
}
return true;
Err(e) => tracing::error!(?e, "unable to deserialize message"),
}

error.push_str(line);
Expand Down
8 changes: 8 additions & 0 deletions crates/rust-analyzer/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,10 @@ config_data! {
/// each of them, with the working directory being the project root
/// (i.e., the folder containing the `Cargo.toml`).
///
/// 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.
///
/// An example command would be:
///
/// ```bash
Expand Down Expand Up @@ -1264,6 +1268,9 @@ impl Config {
Some(args) if !args.is_empty() => {
let mut args = args.clone();
let command = args.remove(0);

let use_saved_file = args.contains(&"$saved_file".to_string());

FlycheckConfig::CustomCommand {
command,
args,
Expand All @@ -1280,6 +1287,7 @@ impl Config {
}
InvocationLocation::Workspace => flycheck::InvocationLocation::Workspace,
},
invoke_with_saved_file: use_saved_file,
}
}
Some(_) | None => FlycheckConfig::CargoCommand {
Expand Down
13 changes: 9 additions & 4 deletions crates/rust-analyzer/src/handlers/notification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,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();
flycheck.restart(None);
}
}
Ok(())
Expand Down Expand Up @@ -273,20 +273,25 @@ fn run_flycheck(state: &mut GlobalState, vfs_path: VfsPath) -> bool {
project_model::ProjectWorkspace::DetachedFiles { .. } => false,
});

let path = vfs_path
.as_path()
.expect("unable to convert to a path; this is a bug in rust-analyzer")
.to_owned();

// Find and trigger corresponding flychecks
for flycheck in world.flycheck.iter() {
for (id, _) in workspace_ids.clone() {
if id == flycheck.id() {
updated = true;
flycheck.restart();
flycheck.restart(Some(path.clone()));
continue;
}
}
}
// No specific flycheck was triggered, so let's trigger all of them.
if !updated {
for flycheck in world.flycheck.iter() {
flycheck.restart();
flycheck.restart(None);
}
}
Ok(())
Expand Down Expand Up @@ -328,7 +333,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();
flycheck.restart(None);
}
Ok(())
}
5 changes: 3 additions & 2 deletions crates/rust-analyzer/src/main_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,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 @@ -296,7 +295,9 @@ 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);
if !self.config.flycheck().invoke_with_saved_file() {
self.flycheck.iter().for_each(|flycheck| flycheck.restart(None));
}
}
if self.config.prefill_caches() {
self.prime_caches_queue.request_op("became quiescent".to_string(), ());
Expand Down
1 change: 1 addition & 0 deletions crates/rust-analyzer/src/reload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,7 @@ impl GlobalState {
fn reload_flycheck(&mut self) {
let _p = profile::span("GlobalState::reload_flycheck");
let config = self.config.flycheck();

let sender = self.flycheck_sender.clone();
let invocation_strategy = match config {
FlycheckConfig::CargoCommand { .. } => flycheck::InvocationStrategy::PerWorkspace,
Expand Down
4 changes: 4 additions & 0 deletions docs/user/generated_config.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,10 @@ If there are multiple linked projects, this command is invoked for
each of them, with the working directory being the project root
(i.e., the folder containing the `Cargo.toml`).

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.

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 @@ -735,7 +735,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, this command is invoked for\neach of them, with the working directory being the project root\n(i.e., the folder containing the `Cargo.toml`).\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, this command is invoked for\neach of them, with the working directory being the project root\n(i.e., the folder containing the `Cargo.toml`).\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.\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