diff options
9 files changed, 186 insertions, 143 deletions
diff --git a/src/tools/rust-analyzer/crates/project-model/src/cargo_workspace.rs b/src/tools/rust-analyzer/crates/project-model/src/cargo_workspace.rs index 40ab8c53fae..1a9501c0638 100644 --- a/src/tools/rust-analyzer/crates/project-model/src/cargo_workspace.rs +++ b/src/tools/rust-analyzer/crates/project-model/src/cargo_workspace.rs @@ -224,6 +224,7 @@ pub enum TargetKind { Example, Test, Bench, + /// Cargo calls this kind `custom-build` BuildScript, Other, } @@ -252,6 +253,22 @@ impl TargetKind { pub fn is_proc_macro(self) -> bool { matches!(self, TargetKind::Lib { is_proc_macro: true }) } + + /// If this is a valid cargo target, returns the name cargo uses in command line arguments + /// and output, otherwise None. + /// https://docs.rs/cargo_metadata/latest/cargo_metadata/enum.TargetKind.html + pub fn as_cargo_target(self) -> Option<&'static str> { + match self { + TargetKind::Bin => Some("bin"), + TargetKind::Lib { is_proc_macro: true } => Some("proc-macro"), + TargetKind::Lib { is_proc_macro: false } => Some("lib"), + TargetKind::Example => Some("example"), + TargetKind::Test => Some("test"), + TargetKind::Bench => Some("bench"), + TargetKind::BuildScript => Some("custom-build"), + TargetKind::Other => None, + } + } } #[derive(Default, Clone, Debug, PartialEq, Eq)] diff --git a/src/tools/rust-analyzer/crates/rust-analyzer/src/command.rs b/src/tools/rust-analyzer/crates/rust-analyzer/src/command.rs index b19a1b8d167..8e2d3d52b87 100644 --- a/src/tools/rust-analyzer/crates/rust-analyzer/src/command.rs +++ b/src/tools/rust-analyzer/crates/rust-analyzer/src/command.rs @@ -13,24 +13,33 @@ use crossbeam_channel::Sender; use process_wrap::std::{StdChildWrapper, StdCommandWrap}; use stdx::process::streaming_output; -/// Cargo output is structured as a one JSON per line. This trait abstracts parsing one line of -/// cargo output into a Rust data type. -pub(crate) trait ParseFromLine: Sized + Send + 'static { - fn from_line(line: &str, error: &mut String) -> Option<Self>; - fn from_eof() -> Option<Self>; +/// Cargo output is structured as one JSON per line. This trait abstracts parsing one line of +/// cargo output into a Rust data type +pub(crate) trait CargoParser<T>: Send + 'static { + fn from_line(&self, line: &str, error: &mut String) -> Option<T>; + fn from_eof(&self) -> Option<T>; } struct CargoActor<T> { + parser: Box<dyn CargoParser<T>>, sender: Sender<T>, stdout: ChildStdout, stderr: ChildStderr, } -impl<T: ParseFromLine> CargoActor<T> { - fn new(sender: Sender<T>, stdout: ChildStdout, stderr: ChildStderr) -> Self { - CargoActor { sender, stdout, stderr } +impl<T: Sized + Send + 'static> CargoActor<T> { + fn new( + parser: impl CargoParser<T>, + sender: Sender<T>, + stdout: ChildStdout, + stderr: ChildStderr, + ) -> Self { + let parser = Box::new(parser); + CargoActor { parser, sender, stdout, stderr } } +} +impl<T: Sized + Send + 'static> CargoActor<T> { fn run(self) -> io::Result<(bool, String)> { // We manually read a line at a time, instead of using serde's // stream deserializers, because the deserializer cannot recover @@ -47,7 +56,7 @@ impl<T: ParseFromLine> CargoActor<T> { let mut read_at_least_one_stderr_message = false; let process_line = |line: &str, error: &mut String| { // Try to deserialize a message from Cargo or Rustc. - if let Some(t) = T::from_line(line, error) { + if let Some(t) = self.parser.from_line(line, error) { self.sender.send(t).unwrap(); true } else { @@ -68,7 +77,7 @@ impl<T: ParseFromLine> CargoActor<T> { } }, &mut || { - if let Some(t) = T::from_eof() { + if let Some(t) = self.parser.from_eof() { self.sender.send(t).unwrap(); } }, @@ -116,8 +125,12 @@ impl<T> fmt::Debug for CommandHandle<T> { } } -impl<T: ParseFromLine> CommandHandle<T> { - pub(crate) fn spawn(mut command: Command, sender: Sender<T>) -> std::io::Result<Self> { +impl<T: Sized + Send + 'static> CommandHandle<T> { + pub(crate) fn spawn( + mut command: Command, + parser: impl CargoParser<T>, + sender: Sender<T>, + ) -> std::io::Result<Self> { command.stdout(Stdio::piped()).stderr(Stdio::piped()).stdin(Stdio::null()); let program = command.get_program().into(); @@ -134,7 +147,7 @@ impl<T: ParseFromLine> CommandHandle<T> { let stdout = child.0.stdout().take().unwrap(); let stderr = child.0.stderr().take().unwrap(); - let actor = CargoActor::<T>::new(sender, stdout, stderr); + let actor = CargoActor::<T>::new(parser, sender, stdout, stderr); let thread = stdx::thread::Builder::new(stdx::thread::ThreadIntent::Worker) .name("CommandHandle".to_owned()) .spawn(move || actor.run()) diff --git a/src/tools/rust-analyzer/crates/rust-analyzer/src/discover.rs b/src/tools/rust-analyzer/crates/rust-analyzer/src/discover.rs index 0c111319bb4..09de309ce95 100644 --- a/src/tools/rust-analyzer/crates/rust-analyzer/src/discover.rs +++ b/src/tools/rust-analyzer/crates/rust-analyzer/src/discover.rs @@ -9,7 +9,7 @@ use serde::{Deserialize, Serialize}; use serde_json::Value; use tracing::{info_span, span::EnteredSpan}; -use crate::command::{CommandHandle, ParseFromLine}; +use crate::command::{CargoParser, CommandHandle}; pub(crate) const ARG_PLACEHOLDER: &str = "{arg}"; @@ -66,7 +66,7 @@ impl DiscoverCommand { cmd.args(args); Ok(DiscoverHandle { - _handle: CommandHandle::spawn(cmd, self.sender.clone())?, + _handle: CommandHandle::spawn(cmd, DiscoverProjectParser, self.sender.clone())?, span: info_span!("discover_command").entered(), }) } @@ -115,8 +115,10 @@ impl DiscoverProjectMessage { } } -impl ParseFromLine for DiscoverProjectMessage { - fn from_line(line: &str, _error: &mut String) -> Option<Self> { +struct DiscoverProjectParser; + +impl CargoParser<DiscoverProjectMessage> for DiscoverProjectParser { + fn from_line(&self, line: &str, _error: &mut String) -> Option<DiscoverProjectMessage> { // can the line even be deserialized as JSON? let Ok(data) = serde_json::from_str::<Value>(line) else { let err = DiscoverProjectData::Error { error: line.to_owned(), source: None }; @@ -131,7 +133,7 @@ impl ParseFromLine for DiscoverProjectMessage { Some(msg) } - fn from_eof() -> Option<Self> { + fn from_eof(&self) -> Option<DiscoverProjectMessage> { None } } diff --git a/src/tools/rust-analyzer/crates/rust-analyzer/src/flycheck.rs b/src/tools/rust-analyzer/crates/rust-analyzer/src/flycheck.rs index 2309f94a742..1e46d9ba56d 100644 --- a/src/tools/rust-analyzer/crates/rust-analyzer/src/flycheck.rs +++ b/src/tools/rust-analyzer/crates/rust-analyzer/src/flycheck.rs @@ -17,7 +17,7 @@ pub(crate) use cargo_metadata::diagnostic::{ use toolchain::Tool; use triomphe::Arc; -use crate::command::{CommandHandle, ParseFromLine}; +use crate::command::{CargoParser, CommandHandle}; #[derive(Clone, Debug, Default, PartialEq, Eq)] pub(crate) enum InvocationStrategy { @@ -324,7 +324,7 @@ impl FlycheckActor { tracing::debug!(?command, "will restart flycheck"); let (sender, receiver) = unbounded(); - match CommandHandle::spawn(command, sender) { + match CommandHandle::spawn(command, CargoCheckParser, sender) { Ok(command_handle) => { tracing::debug!(command = formatted_command, "did restart flycheck"); self.command_handle = Some(command_handle); @@ -550,8 +550,10 @@ enum CargoCheckMessage { Diagnostic { diagnostic: Diagnostic, package_id: Option<Arc<PackageId>> }, } -impl ParseFromLine for CargoCheckMessage { - fn from_line(line: &str, error: &mut String) -> Option<Self> { +struct CargoCheckParser; + +impl CargoParser<CargoCheckMessage> for CargoCheckParser { + fn from_line(&self, line: &str, error: &mut String) -> Option<CargoCheckMessage> { let mut deserializer = serde_json::Deserializer::from_str(line); deserializer.disable_recursion_limit(); if let Ok(message) = JsonMessage::deserialize(&mut deserializer) { @@ -580,7 +582,7 @@ impl ParseFromLine for CargoCheckMessage { None } - fn from_eof() -> Option<Self> { + fn from_eof(&self) -> Option<CargoCheckMessage> { None } } diff --git a/src/tools/rust-analyzer/crates/rust-analyzer/src/hack_recover_crate_name.rs b/src/tools/rust-analyzer/crates/rust-analyzer/src/hack_recover_crate_name.rs deleted file mode 100644 index d7285653c5f..00000000000 --- a/src/tools/rust-analyzer/crates/rust-analyzer/src/hack_recover_crate_name.rs +++ /dev/null @@ -1,25 +0,0 @@ -//! Currently cargo does not emit crate name in the `cargo test --format=json`, which needs to be changed. This -//! module contains a way to recover crate names in a very hacky and wrong way. - -// FIXME(hack_recover_crate_name): Remove this module. - -use std::sync::{Mutex, MutexGuard, OnceLock}; - -use ide_db::FxHashMap; - -static STORAGE: OnceLock<Mutex<FxHashMap<String, String>>> = OnceLock::new(); - -fn get_storage() -> MutexGuard<'static, FxHashMap<String, String>> { - STORAGE.get_or_init(|| Mutex::new(FxHashMap::default())).lock().unwrap() -} - -pub(crate) fn insert_name(name_with_crate: String) { - let Some((_, name_without_crate)) = name_with_crate.split_once("::") else { - return; - }; - get_storage().insert(name_without_crate.to_owned(), name_with_crate); -} - -pub(crate) fn lookup_name(name_without_crate: String) -> Option<String> { - get_storage().get(&name_without_crate).cloned() -} diff --git a/src/tools/rust-analyzer/crates/rust-analyzer/src/handlers/request.rs b/src/tools/rust-analyzer/crates/rust-analyzer/src/handlers/request.rs index b91a5dbd416..1410138da82 100644 --- a/src/tools/rust-analyzer/crates/rust-analyzer/src/handlers/request.rs +++ b/src/tools/rust-analyzer/crates/rust-analyzer/src/handlers/request.rs @@ -36,7 +36,6 @@ use crate::{ config::{Config, RustfmtConfig, WorkspaceSymbolConfig}, diagnostics::convert_diagnostic, global_state::{FetchWorkspaceRequest, GlobalState, GlobalStateSnapshot}, - hack_recover_crate_name, line_index::LineEndings, lsp::{ ext::{ @@ -196,20 +195,48 @@ pub(crate) fn handle_view_item_tree( Ok(res) } -// cargo test requires the real package name which might contain hyphens but -// the test identifier passed to this function is the namespace form where hyphens -// are replaced with underscores so we have to reverse this and find the real package name -fn find_package_name(namespace_root: &str, cargo: &CargoWorkspace) -> Option<String> { +// cargo test requires: +// - the package name - the root of the test identifier supplied to this handler can be +// a package or a target inside a package. +// - the target name - if the test identifier is a target, it's needed in addition to the +// package name to run the right test +// - real names - the test identifier uses the namespace form where hyphens are replaced with +// underscores. cargo test requires the real name. +// - the target kind e.g. bin or lib +fn find_test_target(namespace_root: &str, cargo: &CargoWorkspace) -> Option<TestTarget> { cargo.packages().find_map(|p| { let package_name = &cargo[p].name; - if package_name.replace('-', "_") == namespace_root { - Some(package_name.clone()) - } else { - None + for target in cargo[p].targets.iter() { + let target_name = &cargo[*target].name; + if target_name.replace('-', "_") == namespace_root { + return Some(TestTarget { + package: package_name.clone(), + target: target_name.clone(), + kind: cargo[*target].kind, + }); + } } + None }) } +fn get_all_targets(cargo: &CargoWorkspace) -> Vec<TestTarget> { + cargo + .packages() + .flat_map(|p| { + let package_name = &cargo[p].name; + cargo[p].targets.iter().map(|target| { + let target_name = &cargo[*target].name; + TestTarget { + package: package_name.clone(), + target: target_name.clone(), + kind: cargo[*target].kind, + } + }) + }) + .collect() +} + pub(crate) fn handle_run_test( state: &mut GlobalState, params: lsp_ext::RunTestParams, @@ -217,53 +244,41 @@ pub(crate) fn handle_run_test( if let Some(_session) = state.test_run_session.take() { state.send_notification::<lsp_ext::EndRunTest>(()); } - // We detect the lowest common ancestor of all included tests, and - // run it. We ignore excluded tests for now, the client will handle - // it for us. - let lca = match params.include { - Some(tests) => tests - .into_iter() - .reduce(|x, y| { - let mut common_prefix = "".to_owned(); - for (xc, yc) in x.chars().zip(y.chars()) { - if xc != yc { - break; - } - common_prefix.push(xc); - } - common_prefix - }) - .unwrap_or_default(), - None => "".to_owned(), - }; - let (namespace_root, test_path) = if lca.is_empty() { - (None, None) - } else if let Some((namespace_root, path)) = lca.split_once("::") { - (Some(namespace_root), Some(path)) - } else { - (Some(lca.as_str()), None) - }; + let mut handles = vec![]; for ws in &*state.workspaces { if let ProjectWorkspaceKind::Cargo { cargo, .. } = &ws.kind { - let test_target = if let Some(namespace_root) = namespace_root { - if let Some(package_name) = find_package_name(namespace_root, cargo) { - TestTarget::Package(package_name) - } else { - TestTarget::Workspace - } - } else { - TestTarget::Workspace + // need to deduplicate `include` to avoid redundant test runs + let tests = match params.include { + Some(ref include) => include + .iter() + .unique() + .filter_map(|test| { + let (root, remainder) = match test.split_once("::") { + Some((root, remainder)) => (root.to_owned(), Some(remainder)), + None => (test.clone(), None), + }; + if let Some(target) = find_test_target(&root, cargo) { + Some((target, remainder)) + } else { + tracing::error!("Test target not found for: {test}"); + None + } + }) + .collect_vec(), + None => get_all_targets(cargo).into_iter().map(|target| (target, None)).collect(), }; - let handle = CargoTestHandle::new( - test_path, - state.config.cargo_test_options(None), - cargo.workspace_root(), - test_target, - state.test_run_sender.clone(), - )?; - handles.push(handle); + for (target, path) in tests { + let handle = CargoTestHandle::new( + path, + state.config.cargo_test_options(None), + cargo.workspace_root(), + target, + state.test_run_sender.clone(), + )?; + handles.push(handle); + } } } // Each process send finished signal twice, once for stdout and once for stderr @@ -287,9 +302,7 @@ pub(crate) fn handle_discover_test( } None => (snap.analysis.discover_test_roots()?, None), }; - for t in &tests { - hack_recover_crate_name::insert_name(t.id.clone()); - } + Ok(lsp_ext::DiscoverTestResults { tests: tests .into_iter() diff --git a/src/tools/rust-analyzer/crates/rust-analyzer/src/lib.rs b/src/tools/rust-analyzer/crates/rust-analyzer/src/lib.rs index 27d6225cdb7..daf75e86e64 100644 --- a/src/tools/rust-analyzer/crates/rust-analyzer/src/lib.rs +++ b/src/tools/rust-analyzer/crates/rust-analyzer/src/lib.rs @@ -15,7 +15,6 @@ mod command; mod diagnostics; mod discover; mod flycheck; -mod hack_recover_crate_name; mod line_index; mod main_loop; mod mem_docs; diff --git a/src/tools/rust-analyzer/crates/rust-analyzer/src/main_loop.rs b/src/tools/rust-analyzer/crates/rust-analyzer/src/main_loop.rs index f5d9469f262..e63572d81a5 100644 --- a/src/tools/rust-analyzer/crates/rust-analyzer/src/main_loop.rs +++ b/src/tools/rust-analyzer/crates/rust-analyzer/src/main_loop.rs @@ -26,7 +26,6 @@ use crate::{ file_id_to_url, url_to_file_id, FetchBuildDataResponse, FetchWorkspaceRequest, FetchWorkspaceResponse, GlobalState, }, - hack_recover_crate_name, handlers::{ dispatch::{NotificationDispatcher, RequestDispatcher}, request::empty_diagnostic_report, @@ -37,7 +36,7 @@ use crate::{ }, lsp_ext, reload::{BuildDataProgress, ProcMacroProgress, ProjectWorkspaceProgress}, - test_runner::{CargoTestMessage, TestState}, + test_runner::{CargoTestMessage, CargoTestOutput, TestState}, }; pub fn main_loop(config: Config, connection: Connection) -> anyhow::Result<()> { @@ -659,9 +658,7 @@ impl GlobalState { .filter_map(|f| snapshot.analysis.discover_tests_in_file(f).ok()) .flatten() .collect::<Vec<_>>(); - for t in &tests { - hack_recover_crate_name::insert_name(t.id.clone()); - } + Task::DiscoverTest(lsp_ext::DiscoverTestResults { tests: tests .into_iter() @@ -958,30 +955,29 @@ impl GlobalState { } fn handle_cargo_test_msg(&mut self, message: CargoTestMessage) { - match message { - CargoTestMessage::Test { name, state } => { + match message.output { + CargoTestOutput::Test { name, state } => { let state = match state { TestState::Started => lsp_ext::TestState::Started, TestState::Ignored => lsp_ext::TestState::Skipped, TestState::Ok => lsp_ext::TestState::Passed, TestState::Failed { stdout } => lsp_ext::TestState::Failed { message: stdout }, }; - let Some(test_id) = hack_recover_crate_name::lookup_name(name) else { - return; - }; + let test_id = format!("{}::{name}", message.target.target); + self.send_notification::<lsp_ext::ChangeTestState>( lsp_ext::ChangeTestStateParams { test_id, state }, ); } - CargoTestMessage::Suite => (), - CargoTestMessage::Finished => { + CargoTestOutput::Suite => (), + CargoTestOutput::Finished => { self.test_run_remaining_jobs = self.test_run_remaining_jobs.saturating_sub(1); if self.test_run_remaining_jobs == 0 { self.send_notification::<lsp_ext::EndRunTest>(()); self.test_run_session = None; } } - CargoTestMessage::Custom { text } => { + CargoTestOutput::Custom { text } => { self.send_notification::<lsp_ext::AppendOutputToRunTest>(text); } } diff --git a/src/tools/rust-analyzer/crates/rust-analyzer/src/test_runner.rs b/src/tools/rust-analyzer/crates/rust-analyzer/src/test_runner.rs index 3edfb812cf5..f245c6ac4bf 100644 --- a/src/tools/rust-analyzer/crates/rust-analyzer/src/test_runner.rs +++ b/src/tools/rust-analyzer/crates/rust-analyzer/src/test_runner.rs @@ -3,12 +3,13 @@ use crossbeam_channel::Sender; use paths::AbsPath; +use project_model::TargetKind; use serde::Deserialize as _; use serde_derive::Deserialize; use toolchain::Tool; use crate::{ - command::{CommandHandle, ParseFromLine}, + command::{CargoParser, CommandHandle}, flycheck::CargoOptions, }; @@ -25,9 +26,15 @@ pub(crate) enum TestState { }, } +#[derive(Debug)] +pub(crate) struct CargoTestMessage { + pub target: TestTarget, + pub output: CargoTestOutput, +} + #[derive(Debug, Deserialize)] #[serde(tag = "type", rename_all = "camelCase")] -pub(crate) enum CargoTestMessage { +pub(crate) enum CargoTestOutput { Test { name: String, #[serde(flatten)] @@ -40,19 +47,33 @@ pub(crate) enum CargoTestMessage { }, } -impl ParseFromLine for CargoTestMessage { - fn from_line(line: &str, _: &mut String) -> Option<Self> { +pub(crate) struct CargoTestOutputParser { + pub target: TestTarget, +} + +impl CargoTestOutputParser { + pub(crate) fn new(test_target: &TestTarget) -> Self { + Self { target: test_target.clone() } + } +} + +impl CargoParser<CargoTestMessage> for CargoTestOutputParser { + fn from_line(&self, line: &str, _error: &mut String) -> Option<CargoTestMessage> { let mut deserializer = serde_json::Deserializer::from_str(line); deserializer.disable_recursion_limit(); - if let Ok(message) = CargoTestMessage::deserialize(&mut deserializer) { - return Some(message); - } - Some(CargoTestMessage::Custom { text: line.to_owned() }) + Some(CargoTestMessage { + target: self.target.clone(), + output: if let Ok(message) = CargoTestOutput::deserialize(&mut deserializer) { + message + } else { + CargoTestOutput::Custom { text: line.to_owned() } + }, + }) } - fn from_eof() -> Option<Self> { - Some(CargoTestMessage::Finished) + fn from_eof(&self) -> Option<CargoTestMessage> { + Some(CargoTestMessage { target: self.target.clone(), output: CargoTestOutput::Finished }) } } @@ -62,14 +83,14 @@ pub(crate) struct CargoTestHandle { } // Example of a cargo test command: -// cargo test --workspace --no-fail-fast -- -Z unstable-options --format=json -// or -// cargo test --package my-package --no-fail-fast -- module::func -Z unstable-options --format=json +// +// cargo test --package my-package --bin my_bin --no-fail-fast -- module::func -Z unstable-options --format=json -#[derive(Debug)] -pub(crate) enum TestTarget { - Workspace, - Package(String), +#[derive(Debug, Clone)] +pub(crate) struct TestTarget { + pub package: String, + pub target: String, + pub kind: TargetKind, } impl CargoTestHandle { @@ -84,15 +105,18 @@ impl CargoTestHandle { cmd.env("RUSTC_BOOTSTRAP", "1"); cmd.arg("test"); - match &test_target { - TestTarget::Package(package) => { - cmd.arg("--package"); - cmd.arg(package); - } - TestTarget::Workspace => { - cmd.arg("--workspace"); - } - }; + cmd.arg("--package"); + cmd.arg(&test_target.package); + + if let TargetKind::Lib { .. } = test_target.kind { + // no name required with lib because there can only be one lib target per package + cmd.arg("--lib"); + } else if let Some(cargo_target) = test_target.kind.as_cargo_target() { + cmd.arg(format!("--{cargo_target}")); + cmd.arg(&test_target.target); + } else { + tracing::warn!("Running test for unknown cargo target {:?}", test_target.kind); + } // --no-fail-fast is needed to ensure that all requested tests will run cmd.arg("--no-fail-fast"); @@ -110,6 +134,8 @@ impl CargoTestHandle { cmd.arg(extra_arg); } - Ok(Self { _handle: CommandHandle::spawn(cmd, sender)? }) + Ok(Self { + _handle: CommandHandle::spawn(cmd, CargoTestOutputParser::new(&test_target), sender)?, + }) } } |