From 6a42d7f627958d20587c8a137ed1756465d13b1e Mon Sep 17 00:00:00 2001 From: David Barsky Date: Tue, 28 Mar 2023 09:17:16 -0400 Subject: [PATCH 1/3] fix: allow new, subsequent `rust-project.json`-based workspaces to get proc macro expansion. --- crates/project-model/src/workspace.rs | 8 +++++ crates/rust-analyzer/src/reload.rs | 43 +++++++++++++++++++++------ 2 files changed, 42 insertions(+), 9 deletions(-) diff --git a/crates/project-model/src/workspace.rs b/crates/project-model/src/workspace.rs index 916447fdffa..5f23d9fe826 100644 --- a/crates/project-model/src/workspace.rs +++ b/crates/project-model/src/workspace.rs @@ -672,6 +672,14 @@ pub fn eq_ignore_build_data(&self, other: &Self) -> bool { _ => false, } } + + /// Returns `true` if the project workspace is [`Json`]. + /// + /// [`Json`]: ProjectWorkspace::Json + #[must_use] + pub fn is_json(&self) -> bool { + matches!(self, Self::Json { .. }) + } } fn project_json_to_crate_graph( diff --git a/crates/rust-analyzer/src/reload.rs b/crates/rust-analyzer/src/reload.rs index 7b27a067062..0c76ac8b925 100644 --- a/crates/rust-analyzer/src/reload.rs +++ b/crates/rust-analyzer/src/reload.rs @@ -296,11 +296,25 @@ pub(crate) fn switch_workspaces(&mut self, cause: Cause) { let workspaces = workspaces.iter().filter_map(|res| res.as_ref().ok().cloned()).collect::>(); - let same_workspaces = workspaces.len() == self.workspaces.len() - && workspaces - .iter() - .zip(self.workspaces.iter()) - .all(|(l, r)| l.eq_ignore_build_data(r)); + // `different_workspaces` is used to spawn a new proc macro server for a newly-added + // rust workspace (most commonly sourced from a `rust-project.json`). While the algorithm + // to find the new workspaces is quadratic, we generally expect that the number of total + // workspaces to remain in the low single digits. the `cloned_workspace` is needed for borrowck + // reasons. + let cloned_workspaces = workspaces.clone(); + let different_workspaces = cloned_workspaces + .iter() + .filter(|ws| { + !self + .workspaces + .iter() + .find(|existing_ws| ws.eq_ignore_build_data(&existing_ws)) + .is_some() + }) + .collect::>(); + let same_workspaces = different_workspaces.is_empty(); + + tracing::debug!(current_workspaces = ?self.workspaces, new_workspaces = ?workspaces, ?same_workspaces, "comparing workspaces"); if same_workspaces { let (workspaces, build_scripts) = self.fetch_build_data_queue.last_op_result(); @@ -370,11 +384,10 @@ pub(crate) fn switch_workspaces(&mut self, cause: Cause) { let files_config = self.config.files(); let project_folders = ProjectFolders::new(&self.workspaces, &files_config.exclude); - if self.proc_macro_clients.is_empty() { + if self.proc_macro_clients.is_empty() || !different_workspaces.is_empty() { if let Some((path, path_manually_set)) = self.config.proc_macro_srv() { tracing::info!("Spawning proc-macro servers"); - self.proc_macro_clients = self - .workspaces + self.proc_macro_clients = different_workspaces .iter() .map(|ws| { let (path, args): (_, &[_]) = if path_manually_set { @@ -448,7 +461,19 @@ pub(crate) fn switch_workspaces(&mut self, cause: Cause) { }; let mut change = Change::new(); - if same_workspaces { + // `self.fetch_proc_macros_queue.request_op(cause, proc_macro_paths)` is only called in + // when `switch_workspaces` is called _without_ changing workspaces. This typically occurs + // when build scripts have finishing building, but when rust-analyzer is used with a + // rust-project.json, the build scripts have already been built by the external build system + // that generated the `rust-project.json`. + + // Therefore, in order to allow _new_ workspaces added via rust-project.json (e.g., after + // a workspace was already added), we check whether this is the same workspace _or_ + // if any of the new workspaces is a `rust-project.json`. + // + // The else branch is used to provide better diagnostics to users while procedural macros + // are still being built. + if same_workspaces || different_workspaces.iter().any(|ws| ws.is_json()) { if self.config.expand_proc_macros() { self.fetch_proc_macros_queue.request_op(cause, proc_macro_paths); } From e5bfd7ef0ac992e25716a4bb2646b331a5a31fc4 Mon Sep 17 00:00:00 2001 From: David Barsky Date: Tue, 28 Mar 2023 09:56:01 -0400 Subject: [PATCH 2/3] it, uh, turns out that we should be spawning for new servers. oops. --- crates/rust-analyzer/src/reload.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/crates/rust-analyzer/src/reload.rs b/crates/rust-analyzer/src/reload.rs index 0c76ac8b925..6bd28498d57 100644 --- a/crates/rust-analyzer/src/reload.rs +++ b/crates/rust-analyzer/src/reload.rs @@ -296,9 +296,9 @@ pub(crate) fn switch_workspaces(&mut self, cause: Cause) { let workspaces = workspaces.iter().filter_map(|res| res.as_ref().ok().cloned()).collect::>(); - // `different_workspaces` is used to spawn a new proc macro server for a newly-added - // rust workspace (most commonly sourced from a `rust-project.json`). While the algorithm - // to find the new workspaces is quadratic, we generally expect that the number of total + // `different_workspaces` is used to determine whether to spawn a a new proc macro server for + // a newly-added rust workspace (most commonly sourced from a `rust-project.json`). While the + // algorithm to find the new workspaces is quadratic, we generally expect that the number of total // workspaces to remain in the low single digits. the `cloned_workspace` is needed for borrowck // reasons. let cloned_workspaces = workspaces.clone(); @@ -387,7 +387,8 @@ pub(crate) fn switch_workspaces(&mut self, cause: Cause) { if self.proc_macro_clients.is_empty() || !different_workspaces.is_empty() { if let Some((path, path_manually_set)) = self.config.proc_macro_srv() { tracing::info!("Spawning proc-macro servers"); - self.proc_macro_clients = different_workspaces + self.proc_macro_clients = self + .workspaces .iter() .map(|ws| { let (path, args): (_, &[_]) = if path_manually_set { From 25c59b8e92da205d14a94c0c86c2c729b3e66f1c Mon Sep 17 00:00:00 2001 From: David Barsky Date: Wed, 29 Mar 2023 15:29:32 -0400 Subject: [PATCH 3/3] address PR comments --- crates/base-db/src/input.rs | 2 +- crates/project-model/src/workspace.rs | 18 ++++--- crates/rust-analyzer/src/cli/load_cargo.rs | 2 +- crates/rust-analyzer/src/reload.rs | 55 ++++------------------ 4 files changed, 19 insertions(+), 58 deletions(-) diff --git a/crates/base-db/src/input.rs b/crates/base-db/src/input.rs index c43941d6ac1..3a8c0c385a4 100644 --- a/crates/base-db/src/input.rs +++ b/crates/base-db/src/input.rs @@ -17,7 +17,7 @@ // Map from crate id to the name of the crate and path of the proc-macro. If the value is `None`, // then the crate for the proc-macro hasn't been build yet as the build data is missing. -pub type ProcMacroPaths = FxHashMap, AbsPathBuf)>>; +pub type ProcMacroPaths = FxHashMap, AbsPathBuf), String>>; pub type ProcMacros = FxHashMap; /// Files are grouped into source roots. A source root is a directory on the diff --git a/crates/project-model/src/workspace.rs b/crates/project-model/src/workspace.rs index 5f23d9fe826..b6f79b46227 100644 --- a/crates/project-model/src/workspace.rs +++ b/crates/project-model/src/workspace.rs @@ -691,7 +691,7 @@ fn project_json_to_crate_graph( target_layout: TargetLayoutLoadResult, ) -> (CrateGraph, ProcMacroPaths) { let mut crate_graph = CrateGraph::default(); - let mut proc_macros = FxHashMap::default(); + let mut proc_macros: ProcMacroPaths = FxHashMap::default(); let sysroot_deps = sysroot.as_ref().map(|sysroot| { sysroot_to_crate_graph( &mut crate_graph, @@ -743,13 +743,11 @@ fn project_json_to_crate_graph( ); if krate.is_proc_macro { if let Some(path) = krate.proc_macro_dylib_path.clone() { - proc_macros.insert( - crate_graph_crate_id, - Some(( - krate.display_name.as_ref().map(|it| it.canonical_name().to_owned()), - path, - )), - ); + let node = Ok(( + krate.display_name.as_ref().map(|it| it.canonical_name().to_owned()), + path, + )); + proc_macros.insert(crate_graph_crate_id, node); } } (crate_id, crate_graph_crate_id) @@ -1193,8 +1191,8 @@ fn add_target_crate_root( ); if is_proc_macro { let proc_macro = match build_data.as_ref().map(|it| it.proc_macro_dylib_path.as_ref()) { - Some(it) => it.cloned().map(|path| Some((Some(cargo_name.to_owned()), path))), - None => Some(None), + Some(it) => it.cloned().map(|path| Ok((Some(cargo_name.to_owned()), path))), + None => Some(Err("crate has not yet been build".to_owned())), }; if let Some(proc_macro) = proc_macro { proc_macros.insert(crate_id, proc_macro); diff --git a/crates/rust-analyzer/src/cli/load_cargo.rs b/crates/rust-analyzer/src/cli/load_cargo.rs index 268f59e7e4b..f644bdc7b18 100644 --- a/crates/rust-analyzer/src/cli/load_cargo.rs +++ b/crates/rust-analyzer/src/cli/load_cargo.rs @@ -102,7 +102,7 @@ pub fn load_workspace( ( crate_id, path.map_or_else( - || Err("proc macro crate is missing dylib".to_owned()), + |_| Err("proc macro crate is missing dylib".to_owned()), |(_, path)| load_proc_macro(proc_macro_server, &path, &[]), ), ) diff --git a/crates/rust-analyzer/src/reload.rs b/crates/rust-analyzer/src/reload.rs index 6bd28498d57..c6bd3f0d9cc 100644 --- a/crates/rust-analyzer/src/reload.rs +++ b/crates/rust-analyzer/src/reload.rs @@ -251,7 +251,7 @@ pub(crate) fn fetch_proc_macros(&mut self, cause: Cause, paths: Vec>(); - // `different_workspaces` is used to determine whether to spawn a a new proc macro server for - // a newly-added rust workspace (most commonly sourced from a `rust-project.json`). While the - // algorithm to find the new workspaces is quadratic, we generally expect that the number of total - // workspaces to remain in the low single digits. the `cloned_workspace` is needed for borrowck - // reasons. - let cloned_workspaces = workspaces.clone(); - let different_workspaces = cloned_workspaces - .iter() - .filter(|ws| { - !self - .workspaces - .iter() - .find(|existing_ws| ws.eq_ignore_build_data(&existing_ws)) - .is_some() - }) - .collect::>(); - let same_workspaces = different_workspaces.is_empty(); - - tracing::debug!(current_workspaces = ?self.workspaces, new_workspaces = ?workspaces, ?same_workspaces, "comparing workspaces"); + let same_workspaces = workspaces.len() == self.workspaces.len() + && workspaces + .iter() + .zip(self.workspaces.iter()) + .all(|(l, r)| l.eq_ignore_build_data(r)); if same_workspaces { let (workspaces, build_scripts) = self.fetch_build_data_queue.last_op_result(); @@ -384,7 +370,7 @@ pub(crate) fn switch_workspaces(&mut self, cause: Cause) { let files_config = self.config.files(); let project_folders = ProjectFolders::new(&self.workspaces, &files_config.exclude); - if self.proc_macro_clients.is_empty() || !different_workspaces.is_empty() { + if self.proc_macro_clients.is_empty() || !same_workspaces { if let Some((path, path_manually_set)) = self.config.proc_macro_srv() { tracing::info!("Spawning proc-macro servers"); self.proc_macro_clients = self @@ -462,31 +448,8 @@ pub(crate) fn switch_workspaces(&mut self, cause: Cause) { }; let mut change = Change::new(); - // `self.fetch_proc_macros_queue.request_op(cause, proc_macro_paths)` is only called in - // when `switch_workspaces` is called _without_ changing workspaces. This typically occurs - // when build scripts have finishing building, but when rust-analyzer is used with a - // rust-project.json, the build scripts have already been built by the external build system - // that generated the `rust-project.json`. - - // Therefore, in order to allow _new_ workspaces added via rust-project.json (e.g., after - // a workspace was already added), we check whether this is the same workspace _or_ - // if any of the new workspaces is a `rust-project.json`. - // - // The else branch is used to provide better diagnostics to users while procedural macros - // are still being built. - if same_workspaces || different_workspaces.iter().any(|ws| ws.is_json()) { - if self.config.expand_proc_macros() { - self.fetch_proc_macros_queue.request_op(cause, proc_macro_paths); - } - } else { - // Set up errors for proc-macros upfront that we haven't run build scripts yet - let mut proc_macros = FxHashMap::default(); - for paths in proc_macro_paths { - proc_macros.extend(paths.into_iter().map(move |(crate_id, _)| { - (crate_id, Err("crate has not yet been build".to_owned())) - })); - } - change.set_proc_macros(proc_macros); + if self.config.expand_proc_macros() { + self.fetch_proc_macros_queue.request_op(cause, proc_macro_paths); } change.set_crate_graph(crate_graph); self.analysis_host.apply_change(change);