Unnamed repository; edit this file 'description' to name the repository.
Merge pull request #20635 from iorizu/fix-double-flycheck
fix: Don't trigger two flychecks when saving files that are part of targets
| -rw-r--r-- | crates/rust-analyzer/src/flycheck.rs | 8 | ||||
| -rw-r--r-- | crates/rust-analyzer/src/handlers/notification.rs | 275 | ||||
| -rw-r--r-- | crates/rust-analyzer/src/reload.rs | 20 |
3 files changed, 170 insertions, 133 deletions
diff --git a/crates/rust-analyzer/src/flycheck.rs b/crates/rust-analyzer/src/flycheck.rs index 01ac75c09a..315c45d5b6 100644 --- a/crates/rust-analyzer/src/flycheck.rs +++ b/crates/rust-analyzer/src/flycheck.rs @@ -104,11 +104,11 @@ pub(crate) enum FlycheckConfig { } impl FlycheckConfig { - pub(crate) fn invocation_strategy_once(&self) -> bool { + pub(crate) fn invocation_strategy(&self) -> InvocationStrategy { match self { - FlycheckConfig::CargoCommand { .. } => false, + FlycheckConfig::CargoCommand { .. } => InvocationStrategy::PerWorkspace, FlycheckConfig::CustomCommand { invocation_strategy, .. } => { - *invocation_strategy == InvocationStrategy::Once + invocation_strategy.clone() } } } @@ -529,7 +529,7 @@ impl FlycheckActor { if let Some(command_handle) = self.command_handle.take() { tracing::debug!( command = ?command_handle, - "did cancel flycheck" + "did cancel flycheck" ); command_handle.cancel(); self.command_receiver.take(); diff --git a/crates/rust-analyzer/src/handlers/notification.rs b/crates/rust-analyzer/src/handlers/notification.rs index e193ff7774..68c91a6539 100644 --- a/crates/rust-analyzer/src/handlers/notification.rs +++ b/crates/rust-analyzer/src/handlers/notification.rs @@ -1,9 +1,11 @@ //! This module is responsible for implementing handlers for Language Server //! Protocol. This module specifically handles notifications. -use std::ops::{Deref, Not as _}; +use std::{ + ops::{Deref, Not as _}, + panic::UnwindSafe, +}; -use ide_db::base_db::salsa::Cancelled; use itertools::Itertools; use lsp_types::{ CancelParams, DidChangeConfigurationParams, DidChangeTextDocumentParams, @@ -16,7 +18,7 @@ use vfs::{AbsPathBuf, ChangeKind, VfsPath}; use crate::{ config::{Config, ConfigChange}, - flycheck::Target, + flycheck::{InvocationStrategy, Target}, global_state::{FetchWorkspaceRequest, GlobalState}, lsp::{from_proto, utils::apply_document_changes}, lsp_ext::{self, RunFlycheckParams}, @@ -301,124 +303,165 @@ fn run_flycheck(state: &mut GlobalState, vfs_path: VfsPath) -> bool { let file_id = state.vfs.read().0.file_id(&vfs_path); if let Some((file_id, vfs::FileExcluded::No)) = file_id { let world = state.snapshot(); - let invocation_strategy_once = state.config.flycheck(None).invocation_strategy_once(); + let invocation_strategy = state.config.flycheck(None).invocation_strategy(); let may_flycheck_workspace = state.config.flycheck_workspace(None); - let mut updated = false; - let task = move || -> std::result::Result<(), Cancelled> { - if invocation_strategy_once { - let saved_file = vfs_path.as_path().map(|p| p.to_owned()); - world.flycheck[0].restart_workspace(saved_file); - } - let target = TargetSpec::for_file(&world, file_id)?.and_then(|it| { - let tgt_kind = it.target_kind(); - let (tgt_name, root, package) = match it { - TargetSpec::Cargo(c) => (c.target, c.workspace_root, c.package), - _ => return None, - }; - - 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 Some((None, root, package)), - }; - - Some((Some(tgt), root, package)) - }); - tracing::debug!(?target, "flycheck target"); - // we have a specific non-library target, attempt to only check that target, nothing - // else will be affected - if let Some((target, root, package)) = target { - // trigger a package check if we have a non-library target as that can't affect - // anything else in the workspace OR if we're not allowed to check the workspace as - // the user opted into package checks then - let package_check_allowed = target.is_some() || !may_flycheck_workspace; - if package_check_allowed { - let workspace = world.workspaces.iter().position(|ws| match &ws.kind { - project_model::ProjectWorkspaceKind::Cargo { cargo, .. } - | project_model::ProjectWorkspaceKind::DetachedFile { - cargo: Some((cargo, _, _)), - .. - } => *cargo.workspace_root() == root, - _ => false, - }); - if let Some(idx) = workspace { - world.flycheck[idx].restart_for_package(package, target); - } + let task: Box<dyn FnOnce() -> ide::Cancellable<()> + Send + UnwindSafe> = + match invocation_strategy { + InvocationStrategy::Once => { + Box::new(move || { + // FIXME: Because triomphe::Arc's auto UnwindSafe impl requires that the inner type + // be UnwindSafe, and FlycheckHandle is not UnwindSafe, `word.flycheck` cannot + // be captured directly. std::sync::Arc has an UnwindSafe impl that only requires + // that the inner type be RefUnwindSafe, so if we were using that one we wouldn't + // have this problem. Remove the line below when triomphe::Arc has an UnwindSafe impl + // like std::sync::Arc's. + let world = world; + stdx::always!( + world.flycheck.len() == 1, + "should have exactly one flycheck handle when invocation strategy is once" + ); + let saved_file = vfs_path.as_path().map(ToOwned::to_owned); + world.flycheck[0].restart_workspace(saved_file); + Ok(()) + }) } - } - - if !may_flycheck_workspace { - return Ok(()); - } - - // Trigger flychecks for all workspaces that depend on the saved file - // Crates containing or depending on the saved file - let crate_ids = world - .analysis - .crates_for(file_id)? - .into_iter() - .flat_map(|id| world.analysis.transitive_rev_deps(id)) - .flatten() - .unique() - .collect::<Vec<_>>(); - tracing::debug!(?crate_ids, "flycheck crate ids"); - let crate_root_paths: Vec<_> = crate_ids - .iter() - .filter_map(|&crate_id| { - world - .analysis - .crate_root(crate_id) - .map(|file_id| { - world.file_id_to_file_path(file_id).as_path().map(ToOwned::to_owned) - }) - .transpose() - }) - .collect::<ide::Cancellable<_>>()?; - let crate_root_paths: Vec<_> = crate_root_paths.iter().map(Deref::deref).collect(); - tracing::debug!(?crate_root_paths, "flycheck crate roots"); - - // Find all workspaces that have at least one target containing the saved file - let workspace_ids = - world.workspaces.iter().enumerate().filter(|(_, ws)| match &ws.kind { - project_model::ProjectWorkspaceKind::Cargo { cargo, .. } - | project_model::ProjectWorkspaceKind::DetachedFile { - cargo: Some((cargo, _, _)), - .. - } => cargo.packages().any(|pkg| { - cargo[pkg] - .targets + InvocationStrategy::PerWorkspace => { + Box::new(move || { + let target = TargetSpec::for_file(&world, file_id)?.and_then(|it| { + let tgt_kind = it.target_kind(); + let (tgt_name, root, package) = match it { + TargetSpec::Cargo(c) => (c.target, c.workspace_root, c.package), + _ => return None, + }; + + 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 Some((None, root, package)), + }; + + Some((Some(tgt), root, package)) + }); + tracing::debug!(?target, "flycheck target"); + // we have a specific non-library target, attempt to only check that target, nothing + // else will be affected + let mut package_workspace_idx = None; + if let Some((target, root, package)) = target { + // trigger a package check if we have a non-library target as that can't affect + // anything else in the workspace OR if we're not allowed to check the workspace as + // the user opted into package checks then + let package_check_allowed = target.is_some() || !may_flycheck_workspace; + if package_check_allowed { + package_workspace_idx = + world.workspaces.iter().position(|ws| match &ws.kind { + project_model::ProjectWorkspaceKind::Cargo { + cargo, + .. + } + | project_model::ProjectWorkspaceKind::DetachedFile { + cargo: Some((cargo, _, _)), + .. + } => *cargo.workspace_root() == root, + _ => false, + }); + if let Some(idx) = package_workspace_idx { + world.flycheck[idx].restart_for_package(package, target); + } + } + } + + if !may_flycheck_workspace { + return Ok(()); + } + + // Trigger flychecks for all workspaces that depend on the saved file + // Crates containing or depending on the saved file + let crate_ids: Vec<_> = world + .analysis + .crates_for(file_id)? + .into_iter() + .flat_map(|id| world.analysis.transitive_rev_deps(id)) + .flatten() + .unique() + .collect(); + tracing::debug!(?crate_ids, "flycheck crate ids"); + let crate_root_paths: Vec<_> = crate_ids .iter() - .any(|&it| crate_root_paths.contains(&cargo[it].root.as_path())) - }), - project_model::ProjectWorkspaceKind::Json(project) => project - .crates() - .any(|(_, krate)| crate_root_paths.contains(&krate.root_module.as_path())), - project_model::ProjectWorkspaceKind::DetachedFile { .. } => false, - }); - - let saved_file = vfs_path.as_path().map(|p| p.to_owned()); - - // Find and trigger corresponding flychecks - 'flychecks: for flycheck in world.flycheck.iter() { - for (id, _) in workspace_ids.clone() { - if id == flycheck.id() { - updated = true; - flycheck.restart_workspace(saved_file.clone()); - continue 'flychecks; - } - } - } - // No specific flycheck was triggered, so let's trigger all of them. - if !updated { - for flycheck in world.flycheck.iter() { - flycheck.restart_workspace(saved_file.clone()); + .filter_map(|&crate_id| { + world + .analysis + .crate_root(crate_id) + .map(|file_id| { + world + .file_id_to_file_path(file_id) + .as_path() + .map(ToOwned::to_owned) + }) + .transpose() + }) + .collect::<ide::Cancellable<_>>()?; + let crate_root_paths: Vec<_> = + crate_root_paths.iter().map(Deref::deref).collect(); + tracing::debug!(?crate_root_paths, "flycheck crate roots"); + + // Find all workspaces that have at least one target containing the saved file + let workspace_ids = + world.workspaces.iter().enumerate().filter(|&(idx, ws)| { + let ws_contains_file = match &ws.kind { + project_model::ProjectWorkspaceKind::Cargo { + cargo, .. + } + | project_model::ProjectWorkspaceKind::DetachedFile { + cargo: Some((cargo, _, _)), + .. + } => cargo.packages().any(|pkg| { + cargo[pkg].targets.iter().any(|&it| { + crate_root_paths.contains(&cargo[it].root.as_path()) + }) + }), + project_model::ProjectWorkspaceKind::Json(project) => { + project.crates().any(|(_, krate)| { + crate_root_paths.contains(&krate.root_module.as_path()) + }) + } + project_model::ProjectWorkspaceKind::DetachedFile { + .. + } => false, + }; + let is_pkg_ws = match package_workspace_idx { + Some(pkg_idx) => pkg_idx == idx, + None => false, + }; + ws_contains_file && !is_pkg_ws + }); + + let saved_file = vfs_path.as_path().map(ToOwned::to_owned); + let mut workspace_check_triggered = false; + // Find and trigger corresponding flychecks + 'flychecks: for flycheck in world.flycheck.iter() { + for (id, _) in workspace_ids.clone() { + if id == flycheck.id() { + workspace_check_triggered = true; + flycheck.restart_workspace(saved_file.clone()); + continue 'flychecks; + } + } + } + + // No specific flycheck was triggered, so let's trigger all of them. + if !workspace_check_triggered && package_workspace_idx.is_none() { + for flycheck in world.flycheck.iter() { + flycheck.restart_workspace(saved_file.clone()); + } + } + Ok(()) + }) } - } - Ok(()) - }; + }; + state.task_pool.handle.spawn_with_sender(stdx::thread::ThreadIntent::Worker, move |_| { if let Err(e) = std::panic::catch_unwind(task) { tracing::error!("flycheck task panicked: {e:?}") diff --git a/crates/rust-analyzer/src/reload.rs b/crates/rust-analyzer/src/reload.rs index a8a54930c6..27738fee51 100644 --- a/crates/rust-analyzer/src/reload.rs +++ b/crates/rust-analyzer/src/reload.rs @@ -318,7 +318,7 @@ impl GlobalState { } } - let mut workspaces = linked_projects + let mut workspaces: Vec<_> = linked_projects .iter() .map(|project| match project { LinkedProject::ProjectManifest(manifest) => { @@ -339,7 +339,7 @@ impl GlobalState { Ok(workspace) } }) - .collect::<Vec<_>>(); + .collect(); let mut i = 0; while i < workspaces.len() { @@ -848,23 +848,17 @@ impl GlobalState { fn reload_flycheck(&mut self) { let _p = tracing::info_span!("GlobalState::reload_flycheck").entered(); let config = self.config.flycheck(None); - let sender = self.flycheck_sender.clone(); - let invocation_strategy = match config { - FlycheckConfig::CargoCommand { .. } => { - crate::flycheck::InvocationStrategy::PerWorkspace - } - FlycheckConfig::CustomCommand { ref invocation_strategy, .. } => { - invocation_strategy.clone() - } - }; - let next_gen = self.flycheck.iter().map(|f| f.generation() + 1).max().unwrap_or_default(); + let sender = &self.flycheck_sender; + let invocation_strategy = config.invocation_strategy(); + let next_gen = + self.flycheck.iter().map(FlycheckHandle::generation).max().unwrap_or_default() + 1; self.flycheck = match invocation_strategy { crate::flycheck::InvocationStrategy::Once => { vec![FlycheckHandle::spawn( 0, next_gen, - sender, + sender.clone(), config, None, self.config.root_path().clone(), |