From 7aab6a5c12364a8f0a89a64a4d20df3f85c1b40b Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 20 Dec 2024 12:57:56 +0100 Subject: [PATCH 1/4] Clear flycheck diagnostics more granularly --- crates/rust-analyzer/src/diagnostics.rs | 51 ++++++++-- crates/rust-analyzer/src/flycheck.rs | 102 +++++++++++++------ crates/rust-analyzer/src/handlers/request.rs | 8 +- crates/rust-analyzer/src/main_loop.rs | 12 ++- 4 files changed, 125 insertions(+), 48 deletions(-) diff --git a/crates/rust-analyzer/src/diagnostics.rs b/crates/rust-analyzer/src/diagnostics.rs index 22910ee4c68a..03294e5ab31e 100644 --- a/crates/rust-analyzer/src/diagnostics.rs +++ b/crates/rust-analyzer/src/diagnostics.rs @@ -3,6 +3,7 @@ pub(crate) mod to_proto; use std::mem; +use cargo_metadata::PackageId; use ide::FileId; use ide_db::FxHashMap; use itertools::Itertools; @@ -13,7 +14,8 @@ use triomphe::Arc; use crate::{global_state::GlobalStateSnapshot, lsp, lsp_ext, main_loop::DiagnosticsTaskKind}; -pub(crate) type CheckFixes = Arc>>>; +pub(crate) type CheckFixes = + Arc, IntMap>>>>; #[derive(Debug, Default, Clone)] pub struct DiagnosticsMapConfig { @@ -31,7 +33,8 @@ pub(crate) struct DiagnosticCollection { pub(crate) native_syntax: IntMap)>, pub(crate) native_semantic: IntMap)>, // FIXME: should be Vec - pub(crate) check: IntMap>>, + pub(crate) check: + IntMap, IntMap>>>, pub(crate) check_fixes: CheckFixes, changes: IntSet, /// Counter for supplying a new generation number for diagnostics. @@ -50,18 +53,19 @@ pub(crate) struct Fix { impl DiagnosticCollection { pub(crate) fn clear_check(&mut self, flycheck_id: usize) { - if let Some(it) = Arc::make_mut(&mut self.check_fixes).get_mut(&flycheck_id) { + if let Some(it) = self.check.get_mut(&flycheck_id) { it.clear(); } - if let Some(it) = self.check.get_mut(&flycheck_id) { - self.changes.extend(it.drain().map(|(key, _value)| key)); + if let Some(fixes) = Arc::make_mut(&mut self.check_fixes).get_mut(&flycheck_id) { + fixes.clear(); } } pub(crate) fn clear_check_all(&mut self) { Arc::make_mut(&mut self.check_fixes).clear(); - self.changes - .extend(self.check.values_mut().flat_map(|it| it.drain().map(|(key, _value)| key))) + self.changes.extend( + self.check.values_mut().flat_map(|it| it.drain().flat_map(|(_, v)| v.into_keys())), + ) } pub(crate) fn clear_native_for(&mut self, file_id: FileId) { @@ -70,14 +74,29 @@ impl DiagnosticCollection { self.changes.insert(file_id); } + pub(crate) fn clear_check_for_package(&mut self, flycheck_id: usize, package_id: PackageId) { + let Some(check) = self.check.get_mut(&flycheck_id) else { + return; + }; + check.remove(&Some(package_id)); + } + pub(crate) fn add_check_diagnostic( &mut self, flycheck_id: usize, + package_id: &Option, file_id: FileId, diagnostic: lsp_types::Diagnostic, fix: Option>, ) { - let diagnostics = self.check.entry(flycheck_id).or_default().entry(file_id).or_default(); + let diagnostics = self + .check + .entry(flycheck_id) + .or_default() + .entry(package_id.clone()) + .or_default() + .entry(file_id) + .or_default(); for existing_diagnostic in diagnostics.iter() { if are_diagnostics_equal(existing_diagnostic, &diagnostic) { return; @@ -86,7 +105,14 @@ impl DiagnosticCollection { if let Some(fix) = fix { let check_fixes = Arc::make_mut(&mut self.check_fixes); - check_fixes.entry(flycheck_id).or_default().entry(file_id).or_default().push(*fix); + check_fixes + .entry(flycheck_id) + .or_default() + .entry(package_id.clone()) + .or_default() + .entry(file_id) + .or_default() + .push(*fix); } diagnostics.push(diagnostic); self.changes.insert(file_id); @@ -135,7 +161,12 @@ impl DiagnosticCollection { ) -> impl Iterator { let native_syntax = self.native_syntax.get(&file_id).into_iter().flat_map(|(_, d)| d); let native_semantic = self.native_semantic.get(&file_id).into_iter().flat_map(|(_, d)| d); - let check = self.check.values().filter_map(move |it| it.get(&file_id)).flatten(); + let check = self + .check + .values() + .flat_map(|it| it.values()) + .filter_map(move |it| it.get(&file_id)) + .flatten(); native_syntax.chain(native_semantic).chain(check) } diff --git a/crates/rust-analyzer/src/flycheck.rs b/crates/rust-analyzer/src/flycheck.rs index b035d779a7d5..4373d0529fcc 100644 --- a/crates/rust-analyzer/src/flycheck.rs +++ b/crates/rust-analyzer/src/flycheck.rs @@ -1,8 +1,9 @@ //! Flycheck provides the functionality needed to run `cargo check` to provide //! LSP diagnostics based on the output of the command. -use std::{fmt, io, process::Command, time::Duration}; +use std::{fmt, io, mem, process::Command, time::Duration}; +use cargo_metadata::PackageId; use crossbeam_channel::{select_biased, unbounded, Receiver, Sender}; use paths::{AbsPath, AbsPathBuf, Utf8PathBuf}; use rustc_hash::FxHashMap; @@ -150,10 +151,19 @@ impl FlycheckHandle { pub(crate) enum FlycheckMessage { /// Request adding a diagnostic with fixes included to a file - AddDiagnostic { id: usize, workspace_root: AbsPathBuf, diagnostic: Diagnostic }, + AddDiagnostic { + id: usize, + workspace_root: AbsPathBuf, + diagnostic: Diagnostic, + package_id: Option, + }, - /// Request clearing all previous diagnostics - ClearDiagnostics { id: usize }, + /// Request clearing all outdated diagnostics. + ClearDiagnostics { + id: usize, + /// The pacakge whose diagnostics to clear, or if unspecified, all diagnostics. + package_id: Option, + }, /// Request check progress notification to client Progress { @@ -166,15 +176,18 @@ pub(crate) enum FlycheckMessage { impl fmt::Debug for FlycheckMessage { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - FlycheckMessage::AddDiagnostic { id, workspace_root, diagnostic } => f + FlycheckMessage::AddDiagnostic { id, workspace_root, diagnostic, package_id } => f .debug_struct("AddDiagnostic") .field("id", id) .field("workspace_root", workspace_root) + .field("package_id", package_id) .field("diagnostic_code", &diagnostic.code.as_ref().map(|it| &it.code)) .finish(), - FlycheckMessage::ClearDiagnostics { id } => { - f.debug_struct("ClearDiagnostics").field("id", id).finish() - } + FlycheckMessage::ClearDiagnostics { id, package_id } => f + .debug_struct("ClearDiagnostics") + .field("id", id) + .field("package_id", package_id) + .finish(), FlycheckMessage::Progress { id, progress } => { f.debug_struct("Progress").field("id", id).field("progress", progress).finish() } @@ -200,6 +213,7 @@ enum StateChange { struct FlycheckActor { /// The workspace id of this flycheck instance. id: usize, + sender: Sender, config: FlycheckConfig, manifest_path: Option, @@ -215,8 +229,13 @@ struct FlycheckActor { command_handle: Option>, /// The receiver side of the channel mentioned above. command_receiver: Option>, + package_status: FxHashMap, +} - status: FlycheckStatus, +#[derive(PartialEq, Eq, Copy, Clone, Debug)] +enum DiagnosticReceived { + Yes, + No, } #[allow(clippy::large_enum_variant)] @@ -225,13 +244,6 @@ enum Event { CheckEvent(Option), } -#[derive(PartialEq)] -enum FlycheckStatus { - Started, - DiagnosticSent, - Finished, -} - pub(crate) const SAVED_FILE_PLACEHOLDER: &str = "$saved_file"; impl FlycheckActor { @@ -253,7 +265,7 @@ impl FlycheckActor { manifest_path, command_handle: None, command_receiver: None, - status: FlycheckStatus::Finished, + package_status: FxHashMap::default(), } } @@ -306,13 +318,11 @@ impl FlycheckActor { self.command_handle = Some(command_handle); self.command_receiver = Some(receiver); self.report_progress(Progress::DidStart); - self.status = FlycheckStatus::Started; } Err(error) => { self.report_progress(Progress::DidFailToRestart(format!( "Failed to run the following command: {formatted_command} error={error}" ))); - self.status = FlycheckStatus::Finished; } } } @@ -332,11 +342,25 @@ impl FlycheckActor { error ); } - if self.status == FlycheckStatus::Started { - self.send(FlycheckMessage::ClearDiagnostics { id: self.id }); + if self.package_status.is_empty() { + // We finished without receiving any diagnostics. + // That means all of them are stale. + self.send(FlycheckMessage::ClearDiagnostics { + id: self.id, + package_id: None, + }); + } else { + for (package_id, status) in mem::take(&mut self.package_status) { + if let DiagnosticReceived::No = status { + self.send(FlycheckMessage::ClearDiagnostics { + id: self.id, + package_id: Some(package_id), + }); + } + } } + self.report_progress(Progress::DidFinish(res)); - self.status = FlycheckStatus::Finished; } Event::CheckEvent(Some(message)) => match message { CargoCheckMessage::CompilerArtifact(msg) => { @@ -346,23 +370,30 @@ impl FlycheckActor { "artifact received" ); self.report_progress(Progress::DidCheckCrate(msg.target.name)); + self.package_status.entry(msg.package_id).or_insert(DiagnosticReceived::No); } - - CargoCheckMessage::Diagnostic(msg) => { + CargoCheckMessage::Diagnostic { diagnostic, package_id } => { tracing::trace!( flycheck_id = self.id, - message = msg.message, + message = diagnostic.message, "diagnostic received" ); - if self.status == FlycheckStatus::Started { - self.send(FlycheckMessage::ClearDiagnostics { id: self.id }); + if let Some(package_id) = &package_id { + if !self.package_status.contains_key(package_id) { + self.package_status + .insert(package_id.clone(), DiagnosticReceived::Yes); + self.send(FlycheckMessage::ClearDiagnostics { + id: self.id, + package_id: Some(package_id.clone()), + }); + } } self.send(FlycheckMessage::AddDiagnostic { id: self.id, + package_id, workspace_root: self.root.clone(), - diagnostic: msg, + diagnostic, }); - self.status = FlycheckStatus::DiagnosticSent; } }, } @@ -380,7 +411,7 @@ impl FlycheckActor { command_handle.cancel(); self.command_receiver.take(); self.report_progress(Progress::DidCancel); - self.status = FlycheckStatus::Finished; + self.package_status.clear(); } } @@ -486,7 +517,7 @@ impl FlycheckActor { #[allow(clippy::large_enum_variant)] enum CargoCheckMessage { CompilerArtifact(cargo_metadata::Artifact), - Diagnostic(Diagnostic), + Diagnostic { diagnostic: Diagnostic, package_id: Option }, } impl ParseFromLine for CargoCheckMessage { @@ -501,11 +532,16 @@ impl ParseFromLine for CargoCheckMessage { Some(CargoCheckMessage::CompilerArtifact(artifact)) } cargo_metadata::Message::CompilerMessage(msg) => { - Some(CargoCheckMessage::Diagnostic(msg.message)) + Some(CargoCheckMessage::Diagnostic { + diagnostic: msg.message, + package_id: Some(msg.package_id), + }) } _ => None, }, - JsonMessage::Rustc(message) => Some(CargoCheckMessage::Diagnostic(message)), + JsonMessage::Rustc(message) => { + Some(CargoCheckMessage::Diagnostic { diagnostic: message, package_id: None }) + } }; } diff --git a/crates/rust-analyzer/src/handlers/request.rs b/crates/rust-analyzer/src/handlers/request.rs index 23383e17fbc3..160481dd05fc 100644 --- a/crates/rust-analyzer/src/handlers/request.rs +++ b/crates/rust-analyzer/src/handlers/request.rs @@ -1441,7 +1441,13 @@ pub(crate) fn handle_code_action( } // Fixes from `cargo check`. - for fix in snap.check_fixes.values().filter_map(|it| it.get(&frange.file_id)).flatten() { + for fix in snap + .check_fixes + .values() + .flat_map(|it| it.values()) + .filter_map(|it| it.get(&frange.file_id)) + .flatten() + { // FIXME: this mapping is awkward and shouldn't exist. Refactor // `snap.check_fixes` to not convert to LSP prematurely. let intersect_fix_range = fix diff --git a/crates/rust-analyzer/src/main_loop.rs b/crates/rust-analyzer/src/main_loop.rs index a34f0a3c929a..5d6e9279805e 100644 --- a/crates/rust-analyzer/src/main_loop.rs +++ b/crates/rust-analyzer/src/main_loop.rs @@ -956,7 +956,7 @@ impl GlobalState { fn handle_flycheck_msg(&mut self, message: FlycheckMessage) { match message { - FlycheckMessage::AddDiagnostic { id, workspace_root, diagnostic } => { + FlycheckMessage::AddDiagnostic { id, workspace_root, diagnostic, package_id } => { let snap = self.snapshot(); let diagnostics = crate::diagnostics::to_proto::map_rust_diagnostic_to_lsp( &self.config.diagnostics_map(None), @@ -968,6 +968,7 @@ impl GlobalState { match url_to_file_id(&self.vfs.read().0, &diag.url) { Ok(file_id) => self.diagnostics.add_check_diagnostic( id, + &package_id, file_id, diag.diagnostic, diag.fix, @@ -981,9 +982,12 @@ impl GlobalState { }; } } - - FlycheckMessage::ClearDiagnostics { id } => self.diagnostics.clear_check(id), - + FlycheckMessage::ClearDiagnostics { id, package_id: None } => { + self.diagnostics.clear_check(id) + } + FlycheckMessage::ClearDiagnostics { id, package_id: Some(package_id) } => { + self.diagnostics.clear_check_for_package(id, package_id) + } FlycheckMessage::Progress { id, progress } => { let (state, message) = match progress { flycheck::Progress::DidStart => (Progress::Begin, None), From 7da17fe1959a8ac2595bbd4af714f871b405acc8 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 20 Dec 2024 13:07:27 +0100 Subject: [PATCH 2/4] Arc the workspace root flycheck --- crates/rust-analyzer/src/flycheck.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/crates/rust-analyzer/src/flycheck.rs b/crates/rust-analyzer/src/flycheck.rs index 4373d0529fcc..d178b8b1fdfd 100644 --- a/crates/rust-analyzer/src/flycheck.rs +++ b/crates/rust-analyzer/src/flycheck.rs @@ -1,7 +1,7 @@ //! Flycheck provides the functionality needed to run `cargo check` to provide //! LSP diagnostics based on the output of the command. -use std::{fmt, io, mem, process::Command, time::Duration}; +use std::{fmt, io, mem, process::Command, sync::Arc, time::Duration}; use cargo_metadata::PackageId; use crossbeam_channel::{select_biased, unbounded, Receiver, Sender}; @@ -153,7 +153,7 @@ pub(crate) enum FlycheckMessage { /// Request adding a diagnostic with fixes included to a file AddDiagnostic { id: usize, - workspace_root: AbsPathBuf, + workspace_root: Arc, diagnostic: Diagnostic, package_id: Option, }, @@ -161,7 +161,7 @@ pub(crate) enum FlycheckMessage { /// Request clearing all outdated diagnostics. ClearDiagnostics { id: usize, - /// The pacakge whose diagnostics to clear, or if unspecified, all diagnostics. + /// The package whose diagnostics to clear, or if unspecified, all diagnostics. package_id: Option, }, @@ -219,7 +219,7 @@ struct FlycheckActor { manifest_path: Option, /// Either the workspace root of the workspace we are flychecking, /// or the project root of the project. - root: AbsPathBuf, + root: Arc, sysroot_root: Option, /// CargoHandle exists to wrap around the communication needed to be able to /// run `cargo check` without blocking. Currently the Rust standard library @@ -261,7 +261,7 @@ impl FlycheckActor { sender, config, sysroot_root, - root: workspace_root, + root: Arc::new(workspace_root), manifest_path, command_handle: None, command_receiver: None, @@ -431,7 +431,7 @@ impl FlycheckActor { cmd.env("RUSTUP_TOOLCHAIN", AsRef::::as_ref(sysroot_root)); } cmd.arg(command); - cmd.current_dir(&self.root); + cmd.current_dir(&*self.root); match package { Some(pkg) => cmd.arg("-p").arg(pkg), @@ -473,11 +473,11 @@ impl FlycheckActor { match invocation_strategy { InvocationStrategy::Once => { - cmd.current_dir(&self.root); + cmd.current_dir(&*self.root); } InvocationStrategy::PerWorkspace => { // FIXME: cmd.current_dir(&affected_workspace); - cmd.current_dir(&self.root); + cmd.current_dir(&*self.root); } } From 4a8eb8c2298cee6c312a58019d2c7906fe05dbd6 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 20 Dec 2024 13:27:13 +0100 Subject: [PATCH 3/4] Arc the package ids coming from flycheck --- crates/rust-analyzer/src/diagnostics.rs | 16 +++++++++++----- crates/rust-analyzer/src/flycheck.rs | 17 ++++++++++------- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/crates/rust-analyzer/src/diagnostics.rs b/crates/rust-analyzer/src/diagnostics.rs index 03294e5ab31e..e64a15ae041e 100644 --- a/crates/rust-analyzer/src/diagnostics.rs +++ b/crates/rust-analyzer/src/diagnostics.rs @@ -15,7 +15,7 @@ use triomphe::Arc; use crate::{global_state::GlobalStateSnapshot, lsp, lsp_ext, main_loop::DiagnosticsTaskKind}; pub(crate) type CheckFixes = - Arc, IntMap>>>>; + Arc>, IntMap>>>>; #[derive(Debug, Default, Clone)] pub struct DiagnosticsMapConfig { @@ -33,8 +33,10 @@ pub(crate) struct DiagnosticCollection { pub(crate) native_syntax: IntMap)>, pub(crate) native_semantic: IntMap)>, // FIXME: should be Vec - pub(crate) check: - IntMap, IntMap>>>, + pub(crate) check: IntMap< + usize, + FxHashMap>, IntMap>>, + >, pub(crate) check_fixes: CheckFixes, changes: IntSet, /// Counter for supplying a new generation number for diagnostics. @@ -74,7 +76,11 @@ impl DiagnosticCollection { self.changes.insert(file_id); } - pub(crate) fn clear_check_for_package(&mut self, flycheck_id: usize, package_id: PackageId) { + pub(crate) fn clear_check_for_package( + &mut self, + flycheck_id: usize, + package_id: Arc, + ) { let Some(check) = self.check.get_mut(&flycheck_id) else { return; }; @@ -84,7 +90,7 @@ impl DiagnosticCollection { pub(crate) fn add_check_diagnostic( &mut self, flycheck_id: usize, - package_id: &Option, + package_id: &Option>, file_id: FileId, diagnostic: lsp_types::Diagnostic, fix: Option>, diff --git a/crates/rust-analyzer/src/flycheck.rs b/crates/rust-analyzer/src/flycheck.rs index d178b8b1fdfd..7ff36deb98b2 100644 --- a/crates/rust-analyzer/src/flycheck.rs +++ b/crates/rust-analyzer/src/flycheck.rs @@ -1,7 +1,7 @@ //! Flycheck provides the functionality needed to run `cargo check` to provide //! LSP diagnostics based on the output of the command. -use std::{fmt, io, mem, process::Command, sync::Arc, time::Duration}; +use std::{fmt, io, mem, process::Command, time::Duration}; use cargo_metadata::PackageId; use crossbeam_channel::{select_biased, unbounded, Receiver, Sender}; @@ -13,6 +13,7 @@ pub(crate) use cargo_metadata::diagnostic::{ Applicability, Diagnostic, DiagnosticCode, DiagnosticLevel, DiagnosticSpan, }; use toolchain::Tool; +use triomphe::Arc; use crate::command::{CommandHandle, ParseFromLine}; @@ -155,14 +156,14 @@ pub(crate) enum FlycheckMessage { id: usize, workspace_root: Arc, diagnostic: Diagnostic, - package_id: Option, + package_id: Option>, }, /// Request clearing all outdated diagnostics. ClearDiagnostics { id: usize, /// The package whose diagnostics to clear, or if unspecified, all diagnostics. - package_id: Option, + package_id: Option>, }, /// Request check progress notification to client @@ -229,7 +230,7 @@ struct FlycheckActor { command_handle: Option>, /// The receiver side of the channel mentioned above. command_receiver: Option>, - package_status: FxHashMap, + package_status: FxHashMap, DiagnosticReceived>, } #[derive(PartialEq, Eq, Copy, Clone, Debug)] @@ -370,7 +371,9 @@ impl FlycheckActor { "artifact received" ); self.report_progress(Progress::DidCheckCrate(msg.target.name)); - self.package_status.entry(msg.package_id).or_insert(DiagnosticReceived::No); + self.package_status + .entry(Arc::new(msg.package_id)) + .or_insert(DiagnosticReceived::No); } CargoCheckMessage::Diagnostic { diagnostic, package_id } => { tracing::trace!( @@ -517,7 +520,7 @@ impl FlycheckActor { #[allow(clippy::large_enum_variant)] enum CargoCheckMessage { CompilerArtifact(cargo_metadata::Artifact), - Diagnostic { diagnostic: Diagnostic, package_id: Option }, + Diagnostic { diagnostic: Diagnostic, package_id: Option> }, } impl ParseFromLine for CargoCheckMessage { @@ -534,7 +537,7 @@ impl ParseFromLine for CargoCheckMessage { cargo_metadata::Message::CompilerMessage(msg) => { Some(CargoCheckMessage::Diagnostic { diagnostic: msg.message, - package_id: Some(msg.package_id), + package_id: Some(Arc::new(msg.package_id)), }) } _ => None, From db8660410f11c3842e7405c23cb4e0beec2e7527 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 20 Dec 2024 13:32:03 +0100 Subject: [PATCH 4/4] Clear all check diagnostics when the workspace changes --- crates/rust-analyzer/src/main_loop.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/rust-analyzer/src/main_loop.rs b/crates/rust-analyzer/src/main_loop.rs index 5d6e9279805e..405860c55298 100644 --- a/crates/rust-analyzer/src/main_loop.rs +++ b/crates/rust-analyzer/src/main_loop.rs @@ -715,6 +715,7 @@ impl GlobalState { error!("FetchWorkspaceError: {e}"); } self.wants_to_switch = Some("fetched workspace".to_owned()); + self.diagnostics.clear_check_all(); (Progress::End, None) } };