From 47a567b833b19a1e2f7e487e9a17d1e8da81b44e Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Tue, 28 Feb 2023 12:08:23 +0100 Subject: [PATCH] Deduplicate source roots that have overlapping include paths --- crates/flycheck/src/lib.rs | 41 ++++++++------ crates/project-model/src/workspace.rs | 42 ++++++++------- crates/rust-analyzer/src/config.rs | 28 +++++----- crates/rust-analyzer/src/reload.rs | 78 ++++++++++++++++++++++++--- 4 files changed, 135 insertions(+), 54 deletions(-) diff --git a/crates/flycheck/src/lib.rs b/crates/flycheck/src/lib.rs index 11f7b068ecb..19800224db6 100644 --- a/crates/flycheck/src/lib.rs +++ b/crates/flycheck/src/lib.rs @@ -76,7 +76,7 @@ impl fmt::Display for FlycheckConfig { #[derive(Debug)] pub struct FlycheckHandle { // XXX: drop order is significant - sender: Sender, + sender: Sender, _thread: jod_thread::JoinHandle, id: usize, } @@ -89,7 +89,7 @@ impl FlycheckHandle { workspace_root: AbsPathBuf, ) -> FlycheckHandle { let actor = FlycheckActor::new(id, sender, config, workspace_root); - let (sender, receiver) = unbounded::(); + let (sender, receiver) = unbounded::(); let thread = jod_thread::Builder::new() .name("Flycheck".to_owned()) .spawn(move || actor.run(receiver)) @@ -99,12 +99,12 @@ impl FlycheckHandle { /// Schedule a re-start of the cargo check worker. pub fn restart(&self) { - self.sender.send(Restart::Yes).unwrap(); + self.sender.send(StateChange::Restart).unwrap(); } /// Stop this cargo check worker. pub fn cancel(&self) { - self.sender.send(Restart::No).unwrap(); + self.sender.send(StateChange::Cancel).unwrap(); } pub fn id(&self) -> usize { @@ -149,9 +149,9 @@ pub enum Progress { DidFailToRestart(String), } -enum Restart { - Yes, - No, +enum StateChange { + Restart, + Cancel, } /// A [`FlycheckActor`] is a single check instance of a workspace. @@ -172,7 +172,7 @@ struct FlycheckActor { } enum Event { - Restart(Restart), + RequestStateChange(StateChange), CheckEvent(Option), } @@ -191,30 +191,31 @@ impl FlycheckActor { self.send(Message::Progress { id: self.id, progress }); } - fn next_event(&self, inbox: &Receiver) -> Option { + fn next_event(&self, inbox: &Receiver) -> Option { let check_chan = self.cargo_handle.as_ref().map(|cargo| &cargo.receiver); if let Ok(msg) = inbox.try_recv() { // give restarts a preference so check outputs don't block a restart or stop - return Some(Event::Restart(msg)); + return Some(Event::RequestStateChange(msg)); } select! { - recv(inbox) -> msg => msg.ok().map(Event::Restart), + recv(inbox) -> msg => msg.ok().map(Event::RequestStateChange), recv(check_chan.unwrap_or(&never())) -> msg => Some(Event::CheckEvent(msg.ok())), } } - fn run(mut self, inbox: Receiver) { + fn run(mut self, inbox: Receiver) { 'event: while let Some(event) = self.next_event(&inbox) { match event { - Event::Restart(Restart::No) => { + Event::RequestStateChange(StateChange::Cancel) => { + tracing::debug!(flycheck_id = self.id, "flycheck cancelled"); self.cancel_check_process(); } - Event::Restart(Restart::Yes) => { + Event::RequestStateChange(StateChange::Restart) => { // Cancel the previously spawned process self.cancel_check_process(); while let Ok(restart) = inbox.recv_timeout(Duration::from_millis(50)) { // restart chained with a stop, so just cancel - if let Restart::No = restart { + if let StateChange::Cancel = restart { continue 'event; } } @@ -255,10 +256,20 @@ impl FlycheckActor { } Event::CheckEvent(Some(message)) => match message { CargoMessage::CompilerArtifact(msg) => { + tracing::trace!( + flycheck_id = self.id, + artifact = msg.target.name, + "artifact received" + ); self.report_progress(Progress::DidCheckCrate(msg.target.name)); } CargoMessage::Diagnostic(msg) => { + tracing::trace!( + flycheck_id = self.id, + message = msg.message, + "diagnostic received" + ); self.send(Message::AddDiagnostic { id: self.id, workspace_root: self.root.clone(), diff --git a/crates/project-model/src/workspace.rs b/crates/project-model/src/workspace.rs index 2a11f1e8eb8..45cb8961961 100644 --- a/crates/project-model/src/workspace.rs +++ b/crates/project-model/src/workspace.rs @@ -237,7 +237,7 @@ impl ProjectWorkspace { }; if let Some(sysroot) = &sysroot { - tracing::info!(src_root = %sysroot.src_root().display(), root = %sysroot.root().display(), "Using sysroot"); + tracing::info!(workspace = %cargo_toml.display(), src_root = %sysroot.src_root().display(), root = %sysroot.root().display(), "Using sysroot"); } let rustc_dir = match &config.rustc_source { @@ -247,27 +247,31 @@ impl ProjectWorkspace { } None => None, }; - if let Some(rustc_dir) = &rustc_dir { - tracing::info!(rustc_dir = %rustc_dir.display(), "Using rustc source"); - } let rustc = match rustc_dir { - Some(rustc_dir) => match CargoWorkspace::fetch_metadata( - &rustc_dir, - cargo_toml.parent(), - config, - progress, - ) { - Ok(meta) => Some(CargoWorkspace::new(meta)), - Err(e) => { - tracing::error!( - %e, - "Failed to read Cargo metadata from rustc source at {}", - rustc_dir.display() - ); - None + Some(rustc_dir) if rustc_dir == cargo_toml => { + tracing::info!(rustc_dir = %rustc_dir.display(), "Workspace is the rustc workspace itself, not adding the rustc workspace separately"); + None + } + Some(rustc_dir) => { + tracing::info!(workspace = %cargo_toml.display(), rustc_dir = %rustc_dir.display(), "Using rustc source"); + match CargoWorkspace::fetch_metadata( + &rustc_dir, + cargo_toml.parent(), + config, + progress, + ) { + Ok(meta) => Some(CargoWorkspace::new(meta)), + Err(e) => { + tracing::error!( + %e, + "Failed to read Cargo metadata from rustc source at {}", + rustc_dir.display() + ); + None + } } - }, + } None => None, }; diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs index 48d3fd0e2b9..c643a218a07 100644 --- a/crates/rust-analyzer/src/config.rs +++ b/crates/rust-analyzer/src/config.rs @@ -852,27 +852,27 @@ impl Config { } pub fn linked_projects(&self) -> Vec { match self.data.linkedProjects.as_slice() { - [] => match self.discovered_projects.as_ref() { - Some(discovered_projects) => { - let exclude_dirs: Vec<_> = self - .data - .files_excludeDirs + [] => { + match self.discovered_projects.as_ref() { + Some(discovered_projects) => { + let exclude_dirs: Vec<_> = self + .data + .files_excludeDirs + .iter() + .map(|p| self.root_path.join(p)) + .collect(); + discovered_projects .iter() - .map(|p| self.root_path.join(p)) - .collect(); - discovered_projects - .iter() - .filter(|p| { - let (ProjectManifest::ProjectJson(path) - | ProjectManifest::CargoToml(path)) = p; + .filter(|(ProjectManifest::ProjectJson(path) | ProjectManifest::CargoToml(path))| { !exclude_dirs.iter().any(|p| path.starts_with(p)) }) .cloned() .map(LinkedProject::from) .collect() + } + None => Vec::new(), } - None => Vec::new(), - }, + } linked_projects => linked_projects .iter() .filter_map(|linked_project| match linked_project { diff --git a/crates/rust-analyzer/src/reload.rs b/crates/rust-analyzer/src/reload.rs index abce0d73782..1a396bb06a3 100644 --- a/crates/rust-analyzer/src/reload.rs +++ b/crates/rust-analyzer/src/reload.rs @@ -12,17 +12,21 @@ //! correct. Instead, we try to provide a best-effort service. Even if the //! project is currently loading and we don't have a full project model, we //! still want to respond to various requests. -use std::{mem, sync::Arc}; +use std::{collections::hash_map::Entry, mem, sync::Arc}; use flycheck::{FlycheckConfig, FlycheckHandle}; use hir::db::DefDatabase; use ide::Change; -use ide_db::base_db::{ - CrateGraph, Env, ProcMacro, ProcMacroExpander, ProcMacroExpansionError, ProcMacroKind, - ProcMacroLoadResult, SourceRoot, VfsPath, +use ide_db::{ + base_db::{ + CrateGraph, Env, ProcMacro, ProcMacroExpander, ProcMacroExpansionError, ProcMacroKind, + ProcMacroLoadResult, SourceRoot, VfsPath, + }, + FxHashMap, }; +use itertools::Itertools; use proc_macro_api::{MacroDylib, ProcMacroServer}; -use project_model::{ProjectWorkspace, WorkspaceBuildScripts}; +use project_model::{PackageRoot, ProjectWorkspace, WorkspaceBuildScripts}; use syntax::SmolStr; use vfs::{file_set::FileSetConfig, AbsPath, AbsPathBuf, ChangeKind}; @@ -494,7 +498,69 @@ impl ProjectFolders { let mut fsc = FileSetConfig::builder(); let mut local_filesets = vec![]; - for root in workspaces.iter().flat_map(|ws| ws.to_roots()) { + // Dedup source roots + // Depending on the project setup, we can have duplicated source roots, or for example in + // the case of the rustc workspace, we can end up with two source roots that are almost the + // same but not quite, like: + // PackageRoot { is_local: false, include: [AbsPathBuf(".../rust/src/tools/miri/cargo-miri")], exclude: [] } + // PackageRoot { + // is_local: true, + // include: [AbsPathBuf(".../rust/src/tools/miri/cargo-miri"), AbsPathBuf(".../rust/build/x86_64-pc-windows-msvc/stage0-tools/x86_64-pc-windows-msvc/release/build/cargo-miri-85801cd3d2d1dae4/out")], + // exclude: [AbsPathBuf(".../rust/src/tools/miri/cargo-miri/.git"), AbsPathBuf(".../rust/src/tools/miri/cargo-miri/target")] + // } + // + // The first one comes from the explicit rustc workspace which points to the rustc workspace itself + // The second comes from the rustc workspace that we load as the actual project workspace + // These `is_local` differing in this kind of way gives us problems, especially when trying to filter diagnostics as we don't report diagnostics for external libraries. + // So we need to deduplicate these, usually it would be enough to deduplicate by `include`, but as the rustc example shows here that doesn't work, + // so we need to also coalesce the includes if they overlap. + + let mut roots: Vec<_> = workspaces + .iter() + .flat_map(|ws| ws.to_roots()) + .update(|root| root.include.sort()) + .sorted_by(|a, b| a.include.cmp(&b.include)) + .collect(); + + // map that tracks indices of overlapping roots + let mut overlap_map = FxHashMap::<_, Vec<_>>::default(); + let mut done = false; + + while !mem::replace(&mut done, true) { + // maps include paths to indices of the corresponding root + let mut include_to_idx = FxHashMap::default(); + // Find and note down the indices of overlapping roots + for (idx, root) in roots.iter().filter(|it| !it.include.is_empty()).enumerate() { + for include in &root.include { + match include_to_idx.entry(include) { + Entry::Occupied(e) => { + overlap_map.entry(*e.get()).or_default().push(idx); + } + Entry::Vacant(e) => { + e.insert(idx); + } + } + } + } + for (k, v) in overlap_map.drain() { + done = false; + for v in v { + let r = mem::replace( + &mut roots[v], + PackageRoot { is_local: false, include: vec![], exclude: vec![] }, + ); + roots[k].is_local |= r.is_local; + roots[k].include.extend(r.include); + roots[k].exclude.extend(r.exclude); + } + roots[k].include.sort(); + roots[k].exclude.sort(); + roots[k].include.dedup(); + roots[k].exclude.dedup(); + } + } + + for root in roots.into_iter().filter(|it| !it.include.is_empty()) { let file_set_roots: Vec = root.include.iter().cloned().map(VfsPath::from).collect();