From 443fc298072d6de9db9a044d78e57877c37ad719 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 4 Jan 2024 10:19:25 +0100 Subject: [PATCH] fix: Fix build scripts not being rebuilt in some occasions --- crates/hir-def/src/nameres/collector.rs | 2 +- crates/hir-expand/src/lib.rs | 4 +- crates/hir-expand/src/proc_macro.rs | 8 +++- crates/project-model/src/build_scripts.rs | 8 +++- crates/project-model/src/cargo_workspace.rs | 12 +++--- crates/project-model/src/workspace.rs | 38 +++++++++--------- crates/rust-analyzer/src/cargo_target_spec.rs | 2 +- crates/rust-analyzer/src/config.rs | 2 +- crates/rust-analyzer/src/global_state.rs | 39 +++++++++++++------ .../src/handlers/notification.rs | 12 +++--- crates/rust-analyzer/src/handlers/request.rs | 4 +- crates/rust-analyzer/src/main_loop.rs | 18 +++------ crates/rust-analyzer/src/reload.rs | 19 +++++++-- docs/user/generated_config.adoc | 2 +- editors/code/package.json | 2 +- 15 files changed, 102 insertions(+), 70 deletions(-) diff --git a/crates/hir-def/src/nameres/collector.rs b/crates/hir-def/src/nameres/collector.rs index 3763bfcbcfaf..fdd5dd5a156a 100644 --- a/crates/hir-def/src/nameres/collector.rs +++ b/crates/hir-def/src/nameres/collector.rs @@ -99,7 +99,7 @@ pub(super) fn collect_defs(db: &dyn DefDatabase, def_map: DefMap, tree_id: TreeI }; ( name.as_name(), - CustomProcMacroExpander::new(hir_expand::proc_macro::ProcMacroId( + CustomProcMacroExpander::new(hir_expand::proc_macro::ProcMacroId::new( idx as u32, )), ) diff --git a/crates/hir-expand/src/lib.rs b/crates/hir-expand/src/lib.rs index 6a122e0859cc..5fd13d895117 100644 --- a/crates/hir-expand/src/lib.rs +++ b/crates/hir-expand/src/lib.rs @@ -220,8 +220,8 @@ pub enum MacroCallKind { }, Attr { ast_id: AstId, - // FIXME: This is being interned, subtrees can very quickly differ just slightly causing - // leakage problems here + // FIXME: This shouldn't be here, we can derive this from `invoc_attr_index` + // but we need to fix the `cfg_attr` handling first. attr_args: Option>, /// Syntactical index of the invoking `#[attribute]`. /// diff --git a/crates/hir-expand/src/proc_macro.rs b/crates/hir-expand/src/proc_macro.rs index 25c78fade824..6cca928857eb 100644 --- a/crates/hir-expand/src/proc_macro.rs +++ b/crates/hir-expand/src/proc_macro.rs @@ -12,7 +12,13 @@ use syntax::SmolStr; use crate::{db::ExpandDatabase, tt, ExpandError, ExpandResult}; #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] -pub struct ProcMacroId(pub u32); +pub struct ProcMacroId(u32); + +impl ProcMacroId { + pub fn new(u32: u32) -> Self { + ProcMacroId(u32) + } +} #[derive(Copy, Clone, Eq, PartialEq, Debug, Hash)] pub enum ProcMacroKind { diff --git a/crates/project-model/src/build_scripts.rs b/crates/project-model/src/build_scripts.rs index 68cd40c040b3..afdfa4e602e7 100644 --- a/crates/project-model/src/build_scripts.rs +++ b/crates/project-model/src/build_scripts.rs @@ -23,7 +23,7 @@ use serde::Deserialize; use crate::{ cfg_flag::CfgFlag, utf8_stdout, CargoConfig, CargoFeatures, CargoWorkspace, InvocationLocation, - InvocationStrategy, Package, + InvocationStrategy, Package, TargetKind, }; #[derive(Debug, Default, Clone, PartialEq, Eq)] @@ -445,7 +445,11 @@ impl WorkspaceBuildScripts { .collect(); for p in rustc.packages() { let package = &rustc[p]; - if package.targets.iter().any(|&it| rustc[it].is_proc_macro) { + if package + .targets + .iter() + .any(|&it| matches!(rustc[it].kind, TargetKind::Lib { is_proc_macro: true })) + { if let Some((_, path)) = proc_macro_dylibs .iter() .find(|(name, _)| *name.trim_start_matches("lib") == package.name) diff --git a/crates/project-model/src/cargo_workspace.rs b/crates/project-model/src/cargo_workspace.rs index d89c4598afc7..d5e3665b82c1 100644 --- a/crates/project-model/src/cargo_workspace.rs +++ b/crates/project-model/src/cargo_workspace.rs @@ -186,8 +186,6 @@ pub struct TargetData { pub root: AbsPathBuf, /// Kind of target pub kind: TargetKind, - /// Is this target a proc-macro - pub is_proc_macro: bool, /// Required features of the target without which it won't build pub required_features: Vec, } @@ -196,7 +194,10 @@ pub struct TargetData { pub enum TargetKind { Bin, /// Any kind of Cargo lib crate-type (dylib, rlib, proc-macro, ...). - Lib, + Lib { + /// Is this target a proc-macro + is_proc_macro: bool, + }, Example, Test, Bench, @@ -213,8 +214,8 @@ impl TargetKind { "bench" => TargetKind::Bench, "example" => TargetKind::Example, "custom-build" => TargetKind::BuildScript, - "proc-macro" => TargetKind::Lib, - _ if kind.contains("lib") => TargetKind::Lib, + "proc-macro" => TargetKind::Lib { is_proc_macro: true }, + _ if kind.contains("lib") => TargetKind::Lib { is_proc_macro: false }, _ => continue, }; } @@ -366,7 +367,6 @@ impl CargoWorkspace { name, root: AbsPathBuf::assert(src_path.into()), kind: TargetKind::new(&kind), - is_proc_macro: &*kind == ["proc-macro"], required_features, }); pkg_data.targets.push(tgt); diff --git a/crates/project-model/src/workspace.rs b/crates/project-model/src/workspace.rs index 4057493fa3a6..bc1f5b2f8f9f 100644 --- a/crates/project-model/src/workspace.rs +++ b/crates/project-model/src/workspace.rs @@ -547,7 +547,7 @@ impl ProjectWorkspace { let extra_targets = cargo[pkg] .targets .iter() - .filter(|&&tgt| cargo[tgt].kind == TargetKind::Lib) + .filter(|&&tgt| matches!(cargo[tgt].kind, TargetKind::Lib { .. })) .filter_map(|&tgt| cargo[tgt].root.parent()) .map(|tgt| tgt.normalize().to_path_buf()) .filter(|path| !path.starts_with(&pkg_root)); @@ -912,7 +912,7 @@ fn cargo_to_crate_graph( let mut lib_tgt = None; for &tgt in cargo[pkg].targets.iter() { - if cargo[tgt].kind != TargetKind::Lib && !cargo[pkg].is_member { + if !matches!(cargo[tgt].kind, TargetKind::Lib { .. }) && !cargo[pkg].is_member { // For non-workspace-members, Cargo does not resolve dev-dependencies, so we don't // add any targets except the library target, since those will not work correctly if // they use dev-dependencies. @@ -920,9 +920,9 @@ fn cargo_to_crate_graph( // https://github.com/rust-lang/rust-analyzer/issues/11300 continue; } - let &TargetData { ref name, kind, is_proc_macro, ref root, .. } = &cargo[tgt]; + let &TargetData { ref name, kind, ref root, .. } = &cargo[tgt]; - if kind == TargetKind::Lib + if matches!(kind, TargetKind::Lib { .. }) && sysroot.map_or(false, |sysroot| root.starts_with(sysroot.src_root())) { if let Some(&(_, crate_id, _)) = @@ -947,19 +947,24 @@ fn cargo_to_crate_graph( cfg_options.clone(), file_id, name, - is_proc_macro, + kind, target_layout.clone(), false, toolchain.cloned(), ); - if kind == TargetKind::Lib { + if let TargetKind::Lib { .. } = kind { lib_tgt = Some((crate_id, name.clone())); pkg_to_lib_crate.insert(pkg, crate_id); } // Even crates that don't set proc-macro = true are allowed to depend on proc_macro // (just none of the APIs work when called outside of a proc macro). if let Some(proc_macro) = libproc_macro { - add_proc_macro_dep(crate_graph, crate_id, proc_macro, is_proc_macro); + add_proc_macro_dep( + crate_graph, + crate_id, + proc_macro, + matches!(kind, TargetKind::Lib { is_proc_macro: true }), + ); } pkg_crates.entry(pkg).or_insert_with(Vec::new).push((crate_id, kind)); @@ -1157,9 +1162,9 @@ fn handle_rustc_crates( }; for &tgt in rustc_workspace[pkg].targets.iter() { - if rustc_workspace[tgt].kind != TargetKind::Lib { + let kind @ TargetKind::Lib { is_proc_macro } = rustc_workspace[tgt].kind else { continue; - } + }; if let Some(file_id) = load(&rustc_workspace[tgt].root) { let crate_id = add_target_crate_root( crate_graph, @@ -1169,7 +1174,7 @@ fn handle_rustc_crates( cfg_options.clone(), file_id, &rustc_workspace[tgt].name, - rustc_workspace[tgt].is_proc_macro, + kind, target_layout.clone(), true, toolchain.cloned(), @@ -1178,12 +1183,7 @@ fn handle_rustc_crates( // Add dependencies on core / std / alloc for this crate public_deps.add_to_crate_graph(crate_graph, crate_id); if let Some(proc_macro) = libproc_macro { - add_proc_macro_dep( - crate_graph, - crate_id, - proc_macro, - rustc_workspace[tgt].is_proc_macro, - ); + add_proc_macro_dep(crate_graph, crate_id, proc_macro, is_proc_macro); } rustc_pkg_crates.entry(pkg).or_insert_with(Vec::new).push(crate_id); } @@ -1245,7 +1245,7 @@ fn add_target_crate_root( cfg_options: CfgOptions, file_id: FileId, cargo_name: &str, - is_proc_macro: bool, + kind: TargetKind, target_layout: TargetLayoutLoadResult, rustc_crate: bool, toolchain: Option, @@ -1295,7 +1295,7 @@ fn add_target_crate_root( cfg_options, potential_cfg_options, env, - is_proc_macro, + matches!(kind, TargetKind::Lib { is_proc_macro: true }), if rustc_crate { CrateOrigin::Rustc { name: pkg.name.clone() } } else if pkg.is_member { @@ -1306,7 +1306,7 @@ fn add_target_crate_root( target_layout, toolchain, ); - if is_proc_macro { + if let TargetKind::Lib { is_proc_macro: true } = kind { let proc_macro = match build_data.as_ref().map(|it| it.proc_macro_dylib_path.as_ref()) { Some(it) => it.cloned().map(|path| Ok((Some(cargo_name.to_owned()), path))), None => Some(Err("crate has not yet been built".to_owned())), diff --git a/crates/rust-analyzer/src/cargo_target_spec.rs b/crates/rust-analyzer/src/cargo_target_spec.rs index 0190ca3cab85..9a9357a53989 100644 --- a/crates/rust-analyzer/src/cargo_target_spec.rs +++ b/crates/rust-analyzer/src/cargo_target_spec.rs @@ -174,7 +174,7 @@ impl CargoTargetSpec { buf.push("--example".to_owned()); buf.push(self.target); } - TargetKind::Lib => { + TargetKind::Lib { is_proc_macro: _ } => { buf.push("--lib".to_owned()); } TargetKind::Other | TargetKind::BuildScript => (), diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs index 88fb3708449f..66a8aba006e5 100644 --- a/crates/rust-analyzer/src/config.rs +++ b/crates/rust-analyzer/src/config.rs @@ -111,7 +111,7 @@ config_data! { cargo_buildScripts_overrideCommand: Option> = "null", /// Rerun proc-macros building/build-scripts running when proc-macro /// or build-script sources change and are saved. - cargo_buildScripts_rebuildOnSave: bool = "false", + cargo_buildScripts_rebuildOnSave: bool = "true", /// Use `RUSTC_WRAPPER=rust-analyzer` when running build scripts to /// avoid checking unnecessary things. cargo_buildScripts_useRustcWrapper: bool = "true", diff --git a/crates/rust-analyzer/src/global_state.rs b/crates/rust-analyzer/src/global_state.rs index f57a27305f03..6d84820c1a02 100644 --- a/crates/rust-analyzer/src/global_state.rs +++ b/crates/rust-analyzer/src/global_state.rs @@ -9,7 +9,9 @@ use crossbeam_channel::{unbounded, Receiver, Sender}; use flycheck::FlycheckHandle; use hir::Change; use ide::{Analysis, AnalysisHost, Cancellable, FileId}; -use ide_db::base_db::{CrateId, FileLoader, ProcMacroPaths, SourceDatabase}; +use ide_db::base_db::{ + salsa::ParallelDatabase, CrateId, FileLoader, ProcMacroPaths, SourceDatabase, +}; use load_cargo::SourceRootConfig; use lsp_types::{SemanticTokens, Url}; use nohash_hasher::IntMap; @@ -74,9 +76,11 @@ pub(crate) struct GlobalState { pub(crate) last_reported_status: Option, // proc macros - pub(crate) proc_macro_changed: bool, pub(crate) proc_macro_clients: Arc<[anyhow::Result]>, + // build scripts + pub(crate) build_script_changed: bool, + // Flycheck pub(crate) flycheck: Arc<[FlycheckHandle]>, pub(crate) flycheck_sender: Sender, @@ -187,9 +191,10 @@ impl GlobalState { source_root_config: SourceRootConfig::default(), config_errors: Default::default(), - proc_macro_changed: false, proc_macro_clients: Arc::from_iter([]), + build_script_changed: false, + flycheck: Arc::from_iter([]), flycheck_sender, flycheck_receiver, @@ -287,6 +292,9 @@ impl GlobalState { if reload::should_refresh_for_change(&path, file.change_kind) { workspace_structure_change = Some((path.clone(), false)); } + if AsRef::::as_ref(&path).ends_with("build.rs") { + self.build_script_changed = true; + } if file.is_created_or_deleted() { has_structure_changes = true; workspace_structure_change = @@ -333,7 +341,6 @@ impl GlobalState { self.analysis_host.apply_change(change); { - let raw_database = self.analysis_host.raw_database(); // FIXME: ideally we should only trigger a workspace fetch for non-library changes // but something's going wrong with the source root business when we add a new local // crate see https://github.com/rust-lang/rust-analyzer/issues/13029 @@ -343,13 +350,23 @@ impl GlobalState { force_crate_graph_reload, ); } - self.proc_macro_changed = - changed_files.iter().filter(|file| !file.is_created_or_deleted()).any(|file| { - let crates = raw_database.relevant_crates(file.file_id); - let crate_graph = raw_database.crate_graph(); - - crates.iter().any(|&krate| crate_graph[krate].is_proc_macro) - }); + self.task_pool.handle.spawn(stdx::thread::ThreadIntent::Worker, { + let db = self.analysis_host.raw_database().snapshot(); + move || { + Task::BuildScriptsMayHaveChanged( + changed_files.iter().filter(|file| !file.is_created_or_deleted()).any( + |file| { + // FIXME: We should check for build script related changes as well + // but thats a lot more tricky to pull off. + let crates = db.relevant_crates(file.file_id); + let crate_graph = db.crate_graph(); + + crates.iter().any(|&krate| crate_graph[krate].is_proc_macro) + }, + ), + ) + } + }); } true diff --git a/crates/rust-analyzer/src/handlers/notification.rs b/crates/rust-analyzer/src/handlers/notification.rs index 7e6219991b66..377fb035a5ba 100644 --- a/crates/rust-analyzer/src/handlers/notification.rs +++ b/crates/rust-analyzer/src/handlers/notification.rs @@ -130,11 +130,9 @@ pub(crate) fn handle_did_save_text_document( state: &mut GlobalState, params: DidSaveTextDocumentParams, ) -> anyhow::Result<()> { - if state.config.script_rebuild_on_save() && state.proc_macro_changed { - // reset the flag - state.proc_macro_changed = false; - // rebuild the proc macros - state.fetch_build_data_queue.request_op("ScriptRebuildOnSave".to_owned(), ()); + if state.config.script_rebuild_on_save() && state.build_script_changed { + state.build_script_changed = false; + state.fetch_build_data_queue.request_op("buildScriptsRebuildOnSave".to_owned(), ()); } if let Ok(vfs_path) = from_proto::vfs_path(¶ms.text_document.uri) { @@ -143,7 +141,7 @@ pub(crate) fn handle_did_save_text_document( if reload::should_refresh_for_change(abs_path, ChangeKind::Modify) { state .fetch_workspaces_queue - .request_op(format!("DidSaveTextDocument {abs_path}"), false); + .request_op(format!("workspace vfs file change saved {abs_path}"), false); } } @@ -221,7 +219,7 @@ pub(crate) fn handle_did_change_workspace_folders( if !config.has_linked_projects() && config.detached_files().is_empty() { config.rediscover_workspaces(); - state.fetch_workspaces_queue.request_op("client workspaces changed".to_string(), false) + state.fetch_workspaces_queue.request_op("workspaces folders changed".to_string(), false) } Ok(()) diff --git a/crates/rust-analyzer/src/handlers/request.rs b/crates/rust-analyzer/src/handlers/request.rs index f1317ce2b401..90227c94ada7 100644 --- a/crates/rust-analyzer/src/handlers/request.rs +++ b/crates/rust-analyzer/src/handlers/request.rs @@ -52,7 +52,7 @@ use crate::{ pub(crate) fn handle_workspace_reload(state: &mut GlobalState, _: ()) -> anyhow::Result<()> { state.proc_macro_clients = Arc::from_iter([]); - state.proc_macro_changed = false; + state.build_script_changed = false; state.fetch_workspaces_queue.request_op("reload workspace request".to_string(), false); Ok(()) @@ -60,7 +60,7 @@ pub(crate) fn handle_workspace_reload(state: &mut GlobalState, _: ()) -> anyhow: pub(crate) fn handle_proc_macros_rebuild(state: &mut GlobalState, _: ()) -> anyhow::Result<()> { state.proc_macro_clients = Arc::from_iter([]); - state.proc_macro_changed = false; + state.build_script_changed = false; state.fetch_build_data_queue.request_op("rebuild proc macros request".to_string(), ()); Ok(()) diff --git a/crates/rust-analyzer/src/main_loop.rs b/crates/rust-analyzer/src/main_loop.rs index cdf41c955d26..9df0c9d30d9e 100644 --- a/crates/rust-analyzer/src/main_loop.rs +++ b/crates/rust-analyzer/src/main_loop.rs @@ -12,7 +12,6 @@ use ide_db::base_db::{SourceDatabaseExt, VfsPath}; use lsp_server::{Connection, Notification, Request}; use lsp_types::notification::Notification as _; use stdx::thread::ThreadIntent; -use triomphe::Arc; use vfs::FileId; use crate::{ @@ -69,6 +68,7 @@ pub(crate) enum Task { FetchWorkspace(ProjectWorkspaceProgress), FetchBuildData(BuildDataProgress), LoadProcMacros(ProcMacroProgress), + BuildScriptsMayHaveChanged(bool), } #[derive(Debug)] @@ -320,7 +320,7 @@ impl GlobalState { } // Refresh inlay hints if the client supports it. - if (self.send_hint_refresh_query || self.proc_macro_changed) + if (self.send_hint_refresh_query || self.build_script_changed) && self.config.inlay_hints_refresh() { self.send_request::((), |_, _| ()); @@ -513,17 +513,8 @@ impl GlobalState { .op_completed(Some((workspaces, force_reload_crate_graph))); if let Err(e) = self.fetch_workspace_error() { tracing::error!("FetchWorkspaceError:\n{e}"); - } - - let old = Arc::clone(&self.workspaces); + }; self.switch_workspaces("fetched workspace".to_string()); - let workspaces_updated = !Arc::ptr_eq(&old, &self.workspaces); - - if self.config.run_build_scripts() && workspaces_updated { - self.fetch_build_data_queue - .request_op(format!("workspace updated"), ()); - } - (Progress::End, None) } }; @@ -567,6 +558,9 @@ impl GlobalState { self.report_progress("Loading", state, msg, None, None); } } + Task::BuildScriptsMayHaveChanged(have_changed) => { + self.build_script_changed |= have_changed; + } } } diff --git a/crates/rust-analyzer/src/reload.rs b/crates/rust-analyzer/src/reload.rs index 91dc6c2e4b41..6a731a0af829 100644 --- a/crates/rust-analyzer/src/reload.rs +++ b/crates/rust-analyzer/src/reload.rs @@ -82,7 +82,7 @@ impl GlobalState { } if self.config.linked_or_discovered_projects() != old_config.linked_or_discovered_projects() { - self.fetch_workspaces_queue.request_op("linked projects changed".to_string(), false) + self.fetch_workspaces_queue.request_op("discovered projects changed".to_string(), false) } else if self.config.flycheck() != old_config.flycheck() { self.reload_flycheck(); } @@ -105,9 +105,10 @@ impl GlobalState { }; let mut message = String::new(); - if self.proc_macro_changed { + if self.build_script_changed { status.health = lsp_ext::Health::Warning; - message.push_str("Proc-macros have changed and need to be rebuilt.\n\n"); + message + .push_str("Proc-macros or build scripts have changed and need to be rebuilt.\n\n"); } if let Err(_) = self.fetch_build_data_error() { status.health = lsp_ext::Health::Warning; @@ -398,6 +399,13 @@ impl GlobalState { if *force_reload_crate_graph { self.recreate_crate_graph(cause); } + if self.build_script_changed { + if self.config.run_build_scripts() { + self.build_script_changed = false; + self.fetch_build_data_queue + .request_op(format!("build scripts updated"), ()); + } + } // Current build scripts do not match the version of the active // workspace, so there's nothing for us to update. return; @@ -409,6 +417,11 @@ impl GlobalState { // we don't care about build-script results, they are stale. // FIXME: can we abort the build scripts here? self.workspaces = Arc::new(workspaces); + + if self.config.run_build_scripts() { + self.build_script_changed = false; + self.fetch_build_data_queue.request_op(format!("workspace updated"), ()); + } } if let FilesWatcher::Client = self.config.files().watcher { diff --git a/docs/user/generated_config.adoc b/docs/user/generated_config.adoc index c3f249e0ce2c..05486b32d0b7 100644 --- a/docs/user/generated_config.adoc +++ b/docs/user/generated_config.adoc @@ -71,7 +71,7 @@ cargo check --quiet --workspace --message-format=json --all-targets ``` . -- -[[rust-analyzer.cargo.buildScripts.rebuildOnSave]]rust-analyzer.cargo.buildScripts.rebuildOnSave (default: `false`):: +[[rust-analyzer.cargo.buildScripts.rebuildOnSave]]rust-analyzer.cargo.buildScripts.rebuildOnSave (default: `true`):: + -- Rerun proc-macros building/build-scripts running when proc-macro diff --git a/editors/code/package.json b/editors/code/package.json index 27ed8ac502b5..17a22dc970d2 100644 --- a/editors/code/package.json +++ b/editors/code/package.json @@ -588,7 +588,7 @@ }, "rust-analyzer.cargo.buildScripts.rebuildOnSave": { "markdownDescription": "Rerun proc-macros building/build-scripts running when proc-macro\nor build-script sources change and are saved.", - "default": false, + "default": true, "type": "boolean" }, "rust-analyzer.cargo.buildScripts.useRustcWrapper": {