diff --git a/crates/base-db/src/input.rs b/crates/base-db/src/input.rs index 032e4a8e4a73..21e6dce32ed8 100644 --- a/crates/base-db/src/input.rs +++ b/crates/base-db/src/input.rs @@ -432,10 +432,15 @@ impl CrateGraph { /// Returns all transitive reverse dependencies of the given crate, /// including the crate itself. - pub fn transitive_rev_deps(&self, of: CrateId) -> impl Iterator { + /// + /// The result is topologically ordered, like [Self::crates_in_topological_order] -- dependencies + /// of a crate come before the crate itself. The crate you pass in is first. + pub fn transitive_rev_deps(&self, of: CrateId) -> Vec { + let mut rev_deps = Vec::new(); + let mut visited = FxHashSet::default(); + visited.insert(of); + rev_deps.push(of); let mut worklist = vec![of]; - let mut rev_deps = FxHashSet::default(); - rev_deps.insert(of); let mut inverted_graph = FxHashMap::<_, Vec<_>>::default(); self.arena.iter().for_each(|(krate, data)| { @@ -446,15 +451,16 @@ impl CrateGraph { while let Some(krate) = worklist.pop() { if let Some(krate_rev_deps) = inverted_graph.get(&krate) { - krate_rev_deps - .iter() - .copied() - .filter(|&rev_dep| rev_deps.insert(rev_dep)) - .for_each(|rev_dep| worklist.push(rev_dep)); + krate_rev_deps.iter().copied().filter(|&rev_dep| visited.insert(rev_dep)).for_each( + |rev_dep| { + rev_deps.push(rev_dep); + worklist.push(rev_dep); + }, + ); } } - rev_deps.into_iter() + rev_deps } /// Returns all crates in the graph, sorted in topological order (ie. dependencies of a crate diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index 6328a3c3d0d9..47d05491210e 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -215,7 +215,7 @@ impl Crate { self, db: &dyn HirDatabase, ) -> impl Iterator { - db.crate_graph().transitive_rev_deps(self.id).map(|id| Crate { id }) + db.crate_graph().transitive_rev_deps(self.id).into_iter().map(|id| Crate { id }) } pub fn root_module(self) -> Module { diff --git a/crates/ide/src/lib.rs b/crates/ide/src/lib.rs index cdadfeea4b9a..625429a110d6 100644 --- a/crates/ide/src/lib.rs +++ b/crates/ide/src/lib.rs @@ -577,7 +577,7 @@ impl Analysis { /// Returns crates this file belongs too. pub fn transitive_rev_deps(&self, crate_id: CrateId) -> Cancellable> { - self.with_db(|db| db.crate_graph().transitive_rev_deps(crate_id).collect()) + self.with_db(|db| db.crate_graph().transitive_rev_deps(crate_id)) } /// Returns crates this file *might* belong too. diff --git a/crates/project-model/src/project_json.rs b/crates/project-model/src/project_json.rs index a09c7a77abce..a9fb52fcfc99 100644 --- a/crates/project-model/src/project_json.rs +++ b/crates/project-model/src/project_json.rs @@ -183,12 +183,11 @@ impl ProjectJson { &self.project_root } - pub fn crate_by_root(&self, root: &AbsPath) -> Option { + pub fn crate_by_root(&self, root: &AbsPath) -> Option<&Crate> { self.crates .iter() .filter(|krate| krate.is_workspace_member) .find(|krate| krate.root_module == root) - .cloned() } /// Returns the path to the project's manifest, if it exists. @@ -219,13 +218,17 @@ impl ProjectJson { pub fn runnables(&self) -> &[Runnable] { &self.runnables } + + pub fn runnable_template(&self, kind: RunnableKind) -> Option<&Runnable> { + self.runnables().iter().find(|r| r.kind == kind) + } } /// A crate points to the root module of a crate and lists the dependencies of the crate. This is /// useful in creating the crate graph. #[derive(Clone, Debug, Eq, PartialEq)] pub struct Crate { - pub(crate) display_name: Option, + pub display_name: Option, pub root_module: AbsPathBuf, pub(crate) edition: Edition, pub(crate) version: Option, @@ -319,6 +322,10 @@ pub enum RunnableKind { /// Run a single test. TestOne, + + /// Template for checking a target, emitting rustc JSON diagnostics. + /// May include {label} which will get the label from the `build` section of a crate. + Flycheck, } #[derive(Serialize, Deserialize, Debug, Clone, Eq, PartialEq)] @@ -420,6 +427,7 @@ pub struct RunnableData { #[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)] #[serde(rename_all = "camelCase")] pub enum RunnableKindData { + Flycheck, Check, Run, TestOne, @@ -490,6 +498,7 @@ impl From for RunnableKind { RunnableKindData::Check => RunnableKind::Check, RunnableKindData::Run => RunnableKind::Run, RunnableKindData::TestOne => RunnableKind::TestOne, + RunnableKindData::Flycheck => RunnableKind::Flycheck, } } } diff --git a/crates/rust-analyzer/src/flycheck.rs b/crates/rust-analyzer/src/flycheck.rs index b035d779a7d5..8c85f23fb05f 100644 --- a/crates/rust-analyzer/src/flycheck.rs +++ b/crates/rust-analyzer/src/flycheck.rs @@ -5,6 +5,7 @@ use std::{fmt, io, process::Command, time::Duration}; use crossbeam_channel::{select_biased, unbounded, Receiver, Sender}; use paths::{AbsPath, AbsPathBuf, Utf8PathBuf}; +use project_model::{project_json, TargetKind}; use rustc_hash::FxHashMap; use serde::Deserialize; @@ -35,14 +36,63 @@ pub(crate) struct CargoOptions { pub(crate) target_dir: Option, } -#[derive(Clone)] -pub(crate) enum Target { +/// `--bin rust-analyzer`, `--example example-1`, `--bench microbenchmark`, `--test integrationtest2` +#[derive(Clone, Debug)] +pub(crate) enum BinTarget { + /// --bin rust-analyzer Bin(String), + /// --example example Example(String), - Benchmark(String), + /// -- bench microbenchmark + Bench(String), + /// --test integrationtest2 Test(String), } +impl BinTarget { + pub(crate) fn from_target_kind(kind: TargetKind, name: impl Into) -> Option { + let name = name.into(); + Some(match kind { + TargetKind::Bin => BinTarget::Bin(name), + TargetKind::Example => BinTarget::Example(name), + TargetKind::Bench => BinTarget::Bench(name), + TargetKind::Test => BinTarget::Test(name), + _ => return None, + }) + } + + /// For e.g. this crate, we have `rust-analyzer` as the package name, and + /// `rust-analyzer` as the binary target name. This is the latter, the + /// binary target name. + pub(crate) fn name(&self) -> &str { + match self { + BinTarget::Bin(it) + | BinTarget::Example(it) + | BinTarget::Bench(it) + | BinTarget::Test(it) => it, + } + } + + #[allow(unused)] + pub(crate) fn target_kind(&self) -> TargetKind { + match self { + BinTarget::Bin(_) => TargetKind::Bin, + BinTarget::Example(_) => TargetKind::Example, + BinTarget::Bench(_) => TargetKind::Bench, + BinTarget::Test(_) => TargetKind::Test, + } + } + + pub(crate) fn append_cargo_arg<'a>(&self, cmd: &'a mut Command) -> &'a mut Command { + match self { + BinTarget::Bin(it) => cmd.arg("--bin").arg(it), + BinTarget::Example(it) => cmd.arg("--example").arg(it), + BinTarget::Bench(it) => cmd.arg("--bench").arg(it), + BinTarget::Test(it) => cmd.arg("--test").arg(it), + } + } +} + impl CargoOptions { pub(crate) fn apply_on_command(&self, cmd: &mut Command) { for target in &self.target_triples { @@ -69,6 +119,28 @@ impl CargoOptions { } } +/// The flycheck config from a rust-project.json file +#[derive(Debug, Default)] +pub(crate) struct FlycheckConfigJson { + // XXX: unimplemented because not all that important> nobody + // doing custom rust-project.json needs it most likely + // pub workspace_template: Option, + // + /// The template with [project_json::RunnableKind::Flycheck] + pub single_template: Option, +} + +impl FlycheckConfigJson { + pub(crate) fn any_configured(&self) -> bool { + // self.workspace_template.is_some() || + self.single_template.is_some() + } +} + +/// The flycheck config from rust-analyzer's own configuration. +/// +/// We rely on this when rust-project.json does not specify a flycheck runnable +/// #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum FlycheckConfig { CargoCommand { @@ -105,6 +177,10 @@ pub(crate) struct FlycheckHandle { sender: Sender, _thread: stdx::thread::JoinHandle, id: usize, + + /// Bit hacky, but this lets us force the use of restart_for_package when the flycheck + /// configuration does not support restart_workspace. + cannot_run_workspace: bool, } impl FlycheckHandle { @@ -112,29 +188,49 @@ impl FlycheckHandle { id: usize, sender: Sender, config: FlycheckConfig, + config_json: FlycheckConfigJson, sysroot_root: Option, workspace_root: AbsPathBuf, manifest_path: Option, ) -> FlycheckHandle { - let actor = - FlycheckActor::new(id, sender, config, sysroot_root, workspace_root, manifest_path); + let actor = FlycheckActor::new( + id, + sender, + config, + config_json, + sysroot_root, + workspace_root, + manifest_path, + ); + + let cannot_run_workspace = actor.cannot_run_workspace(); + let (sender, receiver) = unbounded::(); let thread = stdx::thread::Builder::new(stdx::thread::ThreadIntent::Worker) .name("Flycheck".to_owned()) .spawn(move || actor.run(receiver)) .expect("failed to spawn thread"); - FlycheckHandle { id, sender, _thread: thread } + FlycheckHandle { id, sender, _thread: thread, cannot_run_workspace } } /// Schedule a re-start of the cargo check worker to do a workspace wide check. pub(crate) fn restart_workspace(&self, saved_file: Option) { - self.sender.send(StateChange::Restart { package: None, saved_file, target: None }).unwrap(); + self.sender + .send(StateChange::Restart { package: PackageToRestart::All, saved_file }) + .unwrap(); + } + + pub(crate) fn cannot_run_workspace(&self) -> bool { + self.cannot_run_workspace } /// Schedule a re-start of the cargo check worker to do a package wide check. - pub(crate) fn restart_for_package(&self, package: String, target: Option) { + pub(crate) fn restart_for_package(&self, package: PackageSpecifier) { self.sender - .send(StateChange::Restart { package: Some(package), saved_file: None, target }) + .send(StateChange::Restart { + package: PackageToRestart::Package(package), + saved_file: None, + }) .unwrap(); } @@ -184,15 +280,52 @@ impl fmt::Debug for FlycheckMessage { #[derive(Debug)] pub(crate) enum Progress { - DidStart, + DidStart { + /// The user sees this in VSCode, etc. May be a shortened version of the command we actually + /// executed, otherwise it is way too long. + user_facing_command: String, + }, DidCheckCrate(String), DidFinish(io::Result<()>), DidCancel, DidFailToRestart(String), } +#[derive(Debug, Clone)] +enum PackageToRestart { + All, + // Either a cargo package or a $label in rust-project.check.overrideCommand + Package(PackageSpecifier), +} + +#[derive(Debug)] +enum FlycheckCommandOrigin { + /// Regular cargo invocation + Cargo, + /// Configured via check_overrideCommand + CheckOverrideCommand, + /// From a runnable with [project_json::RunnableKind::Flycheck] + ProjectJsonRunnable, +} + +#[derive(Debug, Clone)] +pub(crate) enum PackageSpecifier { + Cargo { + /// The one in Cargo.toml, assumed to work with `cargo check -p {}` etc + /// + /// PackageData.name works. + cargo_canonical_name: String, + /// Ask cargo to build a specific --bin, --test, --bench + bin_target: Option, + }, + BuildInfo { + /// If a `build` field is present in rust-project.json, its label field + label: String, + }, +} + enum StateChange { - Restart { package: Option, saved_file: Option, target: Option }, + Restart { package: PackageToRestart, saved_file: Option }, Cancel, } @@ -202,6 +335,8 @@ struct FlycheckActor { id: usize, sender: Sender, config: FlycheckConfig, + config_json: FlycheckConfigJson, + /// If we are flychecking a cargo workspace, this will point to the workspace Cargo.toml manifest_path: Option, /// Either the workspace root of the workspace we are flychecking, /// or the project root of the project. @@ -232,13 +367,62 @@ enum FlycheckStatus { Finished, } -pub(crate) const SAVED_FILE_PLACEHOLDER: &str = "$saved_file"; +/// This is stable behaviour. Don't change. +const SAVED_FILE_PLACEHOLDER: &str = "$saved_file"; +const LABEL_INLINE: &str = "{label}"; + +struct Substitutions<'a> { + label: Option<&'a str>, + saved_file: Option<&'a str>, +} + +impl<'a> Substitutions<'a> { + /// If you have a runnable, and it has {label} in it somewhere, treat it as a template that + /// may be unsatisfied if you do not provide a label to substitute into it. Returns None in + /// that situation. Otherwise performs the requested substitutions. + /// + fn substitute(self, template: &project_json::Runnable) -> Option { + let mut cmd = Command::new(&template.program); + let mut label_satisfied = self.label.is_none(); + let mut saved_file_satisfied = self.saved_file.is_none(); + for arg in &template.args { + if let Some(ix) = arg.find(LABEL_INLINE) { + if let Some(label) = self.label { + let mut arg = arg.to_string(); + arg.replace_range(ix..ix + LABEL_INLINE.len(), label); + cmd.arg(arg); + label_satisfied = true; + continue; + } else { + return None; + } + } + if arg == SAVED_FILE_PLACEHOLDER { + if let Some(saved_file) = self.saved_file { + cmd.arg(saved_file); + saved_file_satisfied = true; + continue; + } else { + return None; + } + } + cmd.arg(arg); + } + if label_satisfied && saved_file_satisfied { + cmd.current_dir(&template.cwd); + Some(cmd) + } else { + None + } + } +} impl FlycheckActor { fn new( id: usize, sender: Sender, config: FlycheckConfig, + config_json: FlycheckConfigJson, sysroot_root: Option, workspace_root: AbsPathBuf, manifest_path: Option, @@ -248,6 +432,7 @@ impl FlycheckActor { id, sender, config, + config_json, sysroot_root, root: workspace_root, manifest_path, @@ -280,7 +465,7 @@ impl FlycheckActor { tracing::debug!(flycheck_id = self.id, "flycheck cancelled"); self.cancel_check_process(); } - Event::RequestStateChange(StateChange::Restart { package, saved_file, target }) => { + 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)) { @@ -290,27 +475,39 @@ impl FlycheckActor { } } - let Some(command) = - self.check_command(package.as_deref(), saved_file.as_deref(), target) + let Some((command, origin)) = + self.check_command(package.clone(), saved_file.as_deref()) else { + tracing::debug!(?package, "failed to build flycheck command"); continue; }; - let formatted_command = format!("{command:?}"); + let debug_command = format!("{command:?}"); + let user_facing_command = match origin { + // Don't show all the --format=json-with-blah-blah args, just the simple + // version + FlycheckCommandOrigin::Cargo => self.config.to_string(), + // show them the full command but pretty printed. advanced user + FlycheckCommandOrigin::ProjectJsonRunnable + | FlycheckCommandOrigin::CheckOverrideCommand => display_command( + &command, + Some(std::path::Path::new(self.root.as_path())), + ), + }; - tracing::debug!(?command, "will restart flycheck"); + tracing::debug!(?origin, ?command, "will restart flycheck"); let (sender, receiver) = unbounded(); match CommandHandle::spawn(command, sender) { Ok(command_handle) => { - tracing::debug!(command = formatted_command, "did restart flycheck"); + tracing::debug!(?origin, command = %debug_command, "did restart flycheck"); self.command_handle = Some(command_handle); self.command_receiver = Some(receiver); - self.report_progress(Progress::DidStart); + self.report_progress(Progress::DidStart { user_facing_command }); self.status = FlycheckStatus::Started; } Err(error) => { self.report_progress(Progress::DidFailToRestart(format!( - "Failed to run the following command: {formatted_command} error={error}" + "Failed to run the following command: {debug_command} origin={origin:?} error={error}" ))); self.status = FlycheckStatus::Finished; } @@ -384,17 +581,68 @@ impl FlycheckActor { } } + fn explicit_check_command( + &self, + package: PackageToRestart, + saved_file: Option<&AbsPath>, + ) -> Option { + match package { + PackageToRestart::All => { + // If the template doesn't contain {label}, then it works for restarting all. + // + // Might be nice to have: self.config_json.workspace_template.as_ref().map(|x| x.to_command()) + // But for now this works. + // + let template = self.config_json.single_template.as_ref()?; + let subs = Substitutions { + label: None, + saved_file: saved_file.map(|x| x.as_str()), + }; + subs.substitute(template) + } + PackageToRestart::Package( + PackageSpecifier::BuildInfo { label } + // Treat missing build_info as implicitly setting label = the cargo canonical name + // + // Not sure what to do about --bin etc with custom override commands. + | PackageSpecifier::Cargo { cargo_canonical_name: label, bin_target: _ }, + ) => { + let template = self.config_json.single_template.as_ref()?; + let subs = Substitutions { + label: Some(&label), + saved_file: saved_file.map(|x| x.as_str()), + }; + subs.substitute(template) + } + } + } + + fn cannot_run_workspace(&self) -> bool { + let fake_path = self.root.join("fake.rs"); + self.check_command(PackageToRestart::All, Some(&fake_path)).is_none() + } + /// 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>, + package: PackageToRestart, saved_file: Option<&AbsPath>, - target: Option, - ) -> Option { + ) -> Option<(Command, FlycheckCommandOrigin)> { match &self.config { FlycheckConfig::CargoCommand { command, options, ansi_color_output } => { + // Only use the rust-project.json's flycheck config when no check_overrideCommand + // is configured. In the other branch we will still do label substitution but on + // the overrideCommand instead. + if self.config_json.any_configured() { + // Completely handle according to rust-project.json. + // We don't consider this to be "using cargo" so we will not apply any of the + // CargoOptions to the command. + let cmd = self.explicit_check_command(package, saved_file)?; + return Some((cmd, FlycheckCommandOrigin::ProjectJsonRunnable)); + } + let mut cmd = Command::new(Tool::Cargo.path()); if let Some(sysroot_root) = &self.sysroot_root { cmd.env("RUSTUP_TOOLCHAIN", AsRef::::as_ref(sysroot_root)); @@ -403,19 +651,26 @@ impl FlycheckActor { cmd.current_dir(&self.root); match package { - Some(pkg) => cmd.arg("-p").arg(pkg), - None => cmd.arg("--workspace"), + PackageToRestart::Package(PackageSpecifier::Cargo { + cargo_canonical_name, + bin_target, + }) => { + cmd.arg("-p").arg(cargo_canonical_name); + if let Some(tgt) = bin_target { + tgt.append_cargo_arg(&mut cmd); + } + } + PackageToRestart::Package(PackageSpecifier::BuildInfo { label: _ }) => { + // No way to flycheck this single package. All we have is a build label. + // There's no way to really say whether this build label happens to be + // a cargo canonical name, so we won't try. + return None; + } + PackageToRestart::All => { + cmd.arg("--workspace"); + } }; - if let Some(tgt) = target { - match tgt { - Target::Bin(tgt) => cmd.arg("--bin").arg(tgt), - Target::Example(tgt) => cmd.arg("--example").arg(tgt), - Target::Test(tgt) => cmd.arg("--test").arg(tgt), - Target::Benchmark(tgt) => cmd.arg("--bench").arg(tgt), - }; - } - cmd.arg(if *ansi_color_output { "--message-format=json-diagnostic-rendered-ansi" } else { @@ -434,45 +689,43 @@ impl FlycheckActor { options.apply_on_command(&mut cmd); cmd.args(&options.extra_args); - Some(cmd) + Some((cmd, FlycheckCommandOrigin::Cargo)) } FlycheckConfig::CustomCommand { command, args, extra_env, invocation_strategy } => { let mut cmd = Command::new(command); cmd.envs(extra_env); - - match invocation_strategy { - InvocationStrategy::Once => { - cmd.current_dir(&self.root); - } + let root = match invocation_strategy { + InvocationStrategy::Once => self.root.as_path(), InvocationStrategy::PerWorkspace => { - // FIXME: cmd.current_dir(&affected_workspace); - cmd.current_dir(&self.root); + // FIXME: should run in the affected_workspace? + self.root.as_path() } - } + }; - // 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 { - for arg in args { - if arg == SAVED_FILE_PLACEHOLDER { - cmd.arg(saved_file); - } else { - cmd.arg(arg); - } - } - } else { - for arg in args { - if arg == SAVED_FILE_PLACEHOLDER { - // The custom command has a $saved_file placeholder, - // but we had an IDE event that wasn't a file save. Do nothing. - return None; - } + let runnable = project_json::Runnable { + program: command.clone(), + cwd: Utf8PathBuf::new(), + args: args.clone(), + kind: project_json::RunnableKind::Flycheck, + }; - cmd.arg(arg); + let label = match &package { + PackageToRestart::All => None, + PackageToRestart::Package(PackageSpecifier::BuildInfo { label }) => { + Some(label.as_ref()) } - } + PackageToRestart::Package(PackageSpecifier::Cargo { + cargo_canonical_name, + bin_target: _, + }) => Some(cargo_canonical_name.as_ref()), + }; - Some(cmd) + let subs = Substitutions { label, saved_file: saved_file.map(|x| x.as_str()) }; + let mut cmd = subs.substitute(&runnable)?; + cmd.envs(extra_env); + cmd.current_dir(root); + + Some((cmd, FlycheckCommandOrigin::CheckOverrideCommand)) } } } @@ -525,3 +778,73 @@ enum JsonMessage { Cargo(cargo_metadata::Message), Rustc(Diagnostic), } + +/// Not good enough to execute in a shell, but good enough to show the user without all the noisy +/// quotes +fn display_command(c: &Command, implicit_cwd: Option<&std::path::Path>) -> String { + let mut o = String::new(); + use std::fmt::Write; + let lossy = std::ffi::OsStr::to_string_lossy; + if let Some(dir) = c.get_current_dir() { + if Some(dir) == implicit_cwd.map(std::path::Path::new) { + // pass + } else if dir.to_string_lossy().contains(" ") { + write!(o, "cd {:?} && ", dir).unwrap(); + } else { + write!(o, "cd {} && ", dir.display()).unwrap(); + } + } + for (env, val) in c.get_envs() { + let (env, val) = (lossy(env), val.map(lossy).unwrap_or(std::borrow::Cow::Borrowed(""))); + if env.contains(" ") { + write!(o, "\"{}={}\" ", env, val).unwrap(); + } else if val.contains(" ") { + write!(o, "{}=\"{}\" ", env, val).unwrap(); + } else { + write!(o, "{}={} ", env, val).unwrap(); + } + } + let prog = lossy(c.get_program()); + if prog.contains(" ") { + write!(o, "{:?}", prog).unwrap(); + } else { + write!(o, "{}", prog).unwrap(); + } + for arg in c.get_args() { + let arg = lossy(arg); + if arg.contains(" ") { + write!(o, " \"{}\"", arg).unwrap(); + } else { + write!(o, " {}", arg).unwrap(); + } + } + o +} + +#[test] +fn test_display_command() { + use std::path::Path; + let mut cmd = Command::new("command"); + assert_eq!(display_command(cmd.arg("--arg"), None), "command --arg"); + assert_eq!(display_command(cmd.arg("spaced arg"), None), "command --arg \"spaced arg\""); + assert_eq!( + display_command(cmd.env("ENVIRON", "yeah"), None), + "ENVIRON=yeah command --arg \"spaced arg\"" + ); + assert_eq!( + display_command(cmd.env("OTHER", "spaced env"), None), + "ENVIRON=yeah OTHER=\"spaced env\" command --arg \"spaced arg\"" + ); + assert_eq!( + display_command(cmd.current_dir("/tmp"), None), + "cd /tmp && ENVIRON=yeah OTHER=\"spaced env\" command --arg \"spaced arg\"" + ); + assert_eq!( + display_command(cmd.current_dir("/tmp and/thing"), None), + "cd \"/tmp and/thing\" && ENVIRON=yeah OTHER=\"spaced env\" command --arg \"spaced arg\"" + ); + assert_eq!( + display_command(cmd.current_dir("/tmp and/thing"), Some(Path::new("/tmp and/thing"))), + "ENVIRON=yeah OTHER=\"spaced env\" command --arg \"spaced arg\"" + ); +} diff --git a/crates/rust-analyzer/src/global_state.rs b/crates/rust-analyzer/src/global_state.rs index 35e1da80bf67..25cd16070b83 100644 --- a/crates/rust-analyzer/src/global_state.rs +++ b/crates/rust-analyzer/src/global_state.rs @@ -93,6 +93,7 @@ pub(crate) struct GlobalState { pub(crate) flycheck_sender: Sender, pub(crate) flycheck_receiver: Receiver, pub(crate) last_flycheck_error: Option, + pub(crate) flycheck_formatted_commands: Vec, // Test explorer pub(crate) test_run_session: Option>, @@ -240,6 +241,7 @@ impl GlobalState { flycheck_sender, flycheck_receiver, last_flycheck_error: None, + flycheck_formatted_commands: vec![], test_run_session: None, test_run_sender, @@ -695,7 +697,7 @@ impl GlobalStateSnapshot { let Some(krate) = project.crate_by_root(path) else { continue; }; - let Some(build) = krate.build else { + let Some(build) = krate.build.clone() else { continue; }; diff --git a/crates/rust-analyzer/src/handlers/notification.rs b/crates/rust-analyzer/src/handlers/notification.rs index e8d1a7e4df68..f3a434344f55 100644 --- a/crates/rust-analyzer/src/handlers/notification.rs +++ b/crates/rust-analyzer/src/handlers/notification.rs @@ -3,6 +3,7 @@ use std::ops::{Deref, Not as _}; +use ide::CrateId; use itertools::Itertools; use lsp_types::{ CancelParams, DidChangeConfigurationParams, DidChangeTextDocumentParams, @@ -10,13 +11,13 @@ use lsp_types::{ DidOpenTextDocumentParams, DidSaveTextDocumentParams, WorkDoneProgressCancelParams, }; use paths::Utf8PathBuf; -use stdx::TupleExt; +use project_model::project_json; use triomphe::Arc; use vfs::{AbsPathBuf, ChangeKind, VfsPath}; use crate::{ config::{Config, ConfigChange}, - flycheck::Target, + flycheck::{self, BinTarget}, global_state::{FetchWorkspaceRequest, GlobalState}, lsp::{from_proto, utils::apply_document_changes}, lsp_ext::{self, RunFlycheckParams}, @@ -296,41 +297,68 @@ fn run_flycheck(state: &mut GlobalState, vfs_path: VfsPath) -> bool { let source_root_id = world.analysis.source_root_id(file_id).ok(); let mut updated = false; let task = move || -> std::result::Result<(), ide::Cancelled> { - // Is the target binary? If so we let flycheck run only for the workspace that contains the crate. - let target = TargetSpec::for_file(&world, file_id)?.and_then(|it| { - let tgt_kind = it.target_kind(); - let (tgt_name, crate_id) = match it { - TargetSpec::Cargo(c) => (c.target, c.crate_id), - TargetSpec::ProjectJson(p) => (p.label, p.crate_id), - }; + #[derive(Debug)] + enum FlycheckScope { + /// Cargo workspace but user edited a binary target. There should be no + /// downstream crates. We let flycheck run only for the workspace that + /// contains the crate. + Binary { package_name: Option, bin_target: BinTarget, crate_id: CrateId }, + /// Limit flycheck to crates actually containing the file_id, because the user does not want to + /// flycheck with --workspace. + NoDownstream, + /// Run on any affected workspace + Workspace, + } - let tgt = match tgt_kind { - project_model::TargetKind::Bin => Target::Bin(tgt_name), - project_model::TargetKind::Example => Target::Example(tgt_name), - project_model::TargetKind::Test => Target::Test(tgt_name), - project_model::TargetKind::Bench => Target::Benchmark(tgt_name), - _ => return None, - }; + let scope = TargetSpec::for_file(&world, file_id)? + .map(|it| { + let tgt_kind = it.target_kind(); + let (package_name, tgt_name, crate_id) = match it { + TargetSpec::Cargo(c) => (Some(c.package), c.target, c.crate_id), + TargetSpec::ProjectJson(p) => (None, p.label, p.crate_id), + }; - Some((tgt, crate_id)) - }); + if let Some(bin_target) = BinTarget::from_target_kind(tgt_kind, tgt_name) { + return FlycheckScope::Binary { package_name, bin_target, crate_id }; + } + if !world.config.flycheck_workspace(source_root_id) { + FlycheckScope::NoDownstream + } else { + FlycheckScope::Workspace + } + }) + // XXX: is this right? + .unwrap_or(FlycheckScope::Workspace); - let crate_ids = match target { - // Trigger flychecks for the only crate which the target belongs to - Some((_, krate)) => vec![krate], - None => { + tracing::debug!("flycheck scope: {:?}", scope); + + let crate_ids = match scope { + FlycheckScope::Workspace => { // Trigger flychecks for all workspaces that depend on the saved file - // Crates containing or depending on the saved file + // i.e. have crates containing or depending on the saved file world .analysis .crates_for(file_id)? .into_iter() + // These are topologically sorted. So `id` is first. .flat_map(|id| world.analysis.transitive_rev_deps(id)) + // FIXME: If there are multiple crates_for(file_id), once you flatten + // multiple transitive_rev_deps, it's no longer guaranteed to be toposort. .flatten() .unique() .collect::>() } + FlycheckScope::NoDownstream => { + // Trigger flychecks in all workspaces, but only for the exact crate that has + // this file, and not for any workspaces that don't have that file. + world.analysis.crates_for(file_id)? + } + // Trigger flychecks for the only crate which the target belongs to + FlycheckScope::Binary { crate_id, .. } => { + vec![crate_id] + } }; + let crate_root_paths: Vec<_> = crate_ids .iter() .filter_map(|&crate_id| { @@ -352,20 +380,51 @@ fn run_flycheck(state: &mut GlobalState, vfs_path: VfsPath) -> bool { | project_model::ProjectWorkspaceKind::DetachedFile { cargo: Some((cargo, _, _)), .. - } => cargo.packages().find_map(|pkg| { - let has_target_with_root = cargo[pkg] - .targets - .iter() - .any(|&it| crate_root_paths.contains(&cargo[it].root.as_path())); - has_target_with_root.then(|| cargo[pkg].name.clone()) - }), + } => { + // Iterate crate_root_paths first because it is in topological + // order^[1], and we can therefore find the actual crate your saved + // file was in rather than some random downstream dependency. + // Thus with `[check] workspace = false` we can flycheck the + // smallest number of crates (just A) instead of checking B and C + // in response to every file save in A. + // + // A <- B <- C + // + // [1]: But see FIXME above where we flatten. + crate_root_paths.iter().find_map(|root| { + let target = cargo.target_by_root(root)?; + let pkg = cargo[target].package; + let pkg_name = cargo[pkg].name.clone(); + Some(flycheck::PackageSpecifier::Cargo { + // This is very hacky. But we are iterating through a lot of + // crates, many of which are reverse deps, and it doesn't make + // sense to attach --bin XXX to some random downstream dep in a + // different workspace. + bin_target: match &scope { + FlycheckScope::Binary { + package_name: bin_pkg_name, + bin_target, + .. + } if bin_pkg_name.as_ref() == Some(&pkg_name) + && bin_target.name() == cargo[target].name => + { + Some(bin_target.clone()) + } + _ => None, + }, + cargo_canonical_name: pkg_name, + }) + }) + } project_model::ProjectWorkspaceKind::Json(project) => { - if !project.crates().any(|(_, krate)| { - crate_root_paths.contains(&krate.root_module.as_path()) - }) { - return None; - } - None + let krate_flycheck = crate_root_paths.iter().find_map(|root| { + let krate = project.crate_by_root(root)?; + project_json_flycheck(project, krate) + }); + + // If there is no matching crate, returns None and doesn't hit this + // workspace in the loop below. + Some(krate_flycheck?) } project_model::ProjectWorkspaceKind::DetachedFile { .. } => return None, }; @@ -379,14 +438,20 @@ fn run_flycheck(state: &mut GlobalState, vfs_path: VfsPath) -> bool { for (id, package) in workspace_ids.clone() { if id == flycheck.id() { updated = true; - match package.filter(|_| { - !world.config.flycheck_workspace(source_root_id) || target.is_some() + match package.filter(|spec| { + // Any of these cases, and we can't flycheck the whole workspace. + !world.config.flycheck_workspace(source_root_id) + || flycheck.cannot_run_workspace() + // No point flychecking the whole workspace when you edited a + // main.rs. It cannot have dependencies. + || matches!( + spec, + flycheck::PackageSpecifier::Cargo { bin_target: Some(_), .. } + ) }) { - Some(package) => flycheck - .restart_for_package(package, target.clone().map(TupleExt::head)), + Some(spec) => flycheck.restart_for_package(spec), None => flycheck.restart_workspace(saved_file.clone()), } - continue; } } } @@ -446,3 +511,22 @@ pub(crate) fn handle_abort_run_test(state: &mut GlobalState, _: ()) -> anyhow::R } Ok(()) } + +fn project_json_flycheck( + _project_json: &project_json::ProjectJson, + krate: &project_json::Crate, +) -> Option { + if let Some(build_info) = krate.build.as_ref() { + let label = build_info.label.clone(); + Some(flycheck::PackageSpecifier::BuildInfo { label }) + } else { + // No build_info field, so assume this is built by cargo. + let cargo_canonical_name = + krate.display_name.as_ref().map(|x| x.canonical_name().to_owned())?.to_string(); + Some(flycheck::PackageSpecifier::Cargo { + cargo_canonical_name, + // In JSON world, can we even describe crates that are checkable with `cargo check --bin XXX`? + bin_target: None, + }) + } +} diff --git a/crates/rust-analyzer/src/main_loop.rs b/crates/rust-analyzer/src/main_loop.rs index ec71b4a7a122..88225da03d1a 100644 --- a/crates/rust-analyzer/src/main_loop.rs +++ b/crates/rust-analyzer/src/main_loop.rs @@ -951,8 +951,24 @@ impl GlobalState { FlycheckMessage::ClearDiagnostics { id } => self.diagnostics.clear_check(id), FlycheckMessage::Progress { id, progress } => { + let format_with_id = |user_facing_command: String| { + if self.flycheck.len() == 1 { + user_facing_command + } else { + format!("{user_facing_command} (#{})", id + 1) + } + }; + + self.flycheck_formatted_commands + .resize_with(self.flycheck.len().max(id + 1), || { + format_with_id(self.config.flycheck(None).to_string()) + }); + let (state, message) = match progress { - flycheck::Progress::DidStart => (Progress::Begin, None), + flycheck::Progress::DidStart { user_facing_command } => { + self.flycheck_formatted_commands[id] = format_with_id(user_facing_command); + (Progress::Begin, None) + } flycheck::Progress::DidCheckCrate(target) => (Progress::Report, Some(target)), flycheck::Progress::DidCancel => { self.last_flycheck_error = None; @@ -970,13 +986,8 @@ impl GlobalState { } }; - // When we're running multiple flychecks, we have to include a disambiguator in - // the title, or the editor complains. Note that this is a user-facing string. - let title = if self.flycheck.len() == 1 { - format!("{}", self.config.flycheck(None)) - } else { - format!("{} (#{})", self.config.flycheck(None), id + 1) - }; + // Clone because we &mut self for report_progress + let title = self.flycheck_formatted_commands[id].clone(); self.report_progress( &title, state, diff --git a/crates/rust-analyzer/src/reload.rs b/crates/rust-analyzer/src/reload.rs index 12667e3c90de..2e4afb57eb6c 100644 --- a/crates/rust-analyzer/src/reload.rs +++ b/crates/rust-analyzer/src/reload.rs @@ -24,7 +24,9 @@ use itertools::Itertools; use load_cargo::{load_proc_macro, ProjectFolders}; use lsp_types::FileSystemWatcher; use proc_macro_api::ProcMacroServer; -use project_model::{ManifestPath, ProjectWorkspace, ProjectWorkspaceKind, WorkspaceBuildScripts}; +use project_model::{ + project_json, ManifestPath, ProjectWorkspace, ProjectWorkspaceKind, WorkspaceBuildScripts, +}; use stdx::{format_to, thread::ThreadIntent}; use triomphe::Arc; use vfs::{AbsPath, AbsPathBuf, ChangeKind}; @@ -808,6 +810,7 @@ impl GlobalState { 0, sender, config, + crate::flycheck::FlycheckConfigJson::default(), None, self.config.root_path().clone(), None, @@ -825,13 +828,25 @@ impl GlobalState { | ProjectWorkspaceKind::DetachedFile { cargo: Some((cargo, _, _)), .. - } => (cargo.workspace_root(), Some(cargo.manifest_path())), + } => ( + crate::flycheck::FlycheckConfigJson::default(), + cargo.workspace_root(), + Some(cargo.manifest_path()), + ), ProjectWorkspaceKind::Json(project) => { + let config_json = crate::flycheck::FlycheckConfigJson { + single_template: project + .runnable_template(project_json::RunnableKind::Flycheck) + .cloned(), + }; // Enable flychecks for json projects if a custom flycheck command was supplied // in the workspace configuration. match config { + _ if config_json.any_configured() => { + (config_json, project.path(), None) + } FlycheckConfig::CustomCommand { .. } => { - (project.path(), None) + (config_json, project.path(), None) } _ => return None, } @@ -841,11 +856,12 @@ impl GlobalState { ws.sysroot.root().map(ToOwned::to_owned), )) }) - .map(|(id, (root, manifest_path), sysroot_root)| { + .map(|(id, (config_json, root, manifest_path), sysroot_root)| { FlycheckHandle::spawn( id, sender.clone(), config.clone(), + config_json, sysroot_root, root.to_path_buf(), manifest_path.map(|it| it.to_path_buf()), @@ -855,6 +871,7 @@ impl GlobalState { } } .into(); + self.flycheck_formatted_commands = vec![]; } }