Merge pull request #18729 from Veykril/push-kyxtnozqzwkn

Clear flycheck diagnostics more granularly

Author: Veykril

src/tools/rust-analyzer/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<IntMap<usize, IntMap<FileId, Vec<Fix>>>>;
+pub(crate) type CheckFixes =
+    Arc<IntMap<usize, FxHashMap<Option<Arc<PackageId>>, IntMap<FileId, Vec<Fix>>>>>;
 
 #[derive(Debug, Default, Clone)]
 pub struct DiagnosticsMapConfig {
@@ -31,7 +33,10 @@ pub(crate) struct DiagnosticCollection {
     pub(crate) native_syntax: IntMap<FileId, (DiagnosticsGeneration, Vec<lsp_types::Diagnostic>)>,
     pub(crate) native_semantic: IntMap<FileId, (DiagnosticsGeneration, Vec<lsp_types::Diagnostic>)>,
     // FIXME: should be Vec<flycheck::Diagnostic>
-    pub(crate) check: IntMap<usize, IntMap<FileId, Vec<lsp_types::Diagnostic>>>,
+    pub(crate) check: IntMap<
+        usize,
+        FxHashMap<Option<Arc<PackageId>>, IntMap<FileId, Vec<lsp_types::Diagnostic>>>,
+    >,
     pub(crate) check_fixes: CheckFixes,
     changes: IntSet<FileId>,
     /// Counter for supplying a new generation number for diagnostics.
@@ -50,18 +55,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 +76,33 @@ impl DiagnosticCollection {
         self.changes.insert(file_id);
     }
 
+    pub(crate) fn clear_check_for_package(
+        &mut self,
+        flycheck_id: usize,
+        package_id: Arc<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<Arc<PackageId>>,
         file_id: FileId,
         diagnostic: lsp_types::Diagnostic,
         fix: Option<Box<Fix>>,
     ) {
-        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 +111,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 +167,12 @@ impl DiagnosticCollection {
     ) -> impl Iterator<Item = &lsp_types::Diagnostic> {
         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)
     }
 

src/tools/rust-analyzer/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;
@@ -13,6 +14,7 @@ pub(crate) use cargo_metadata::diagnostic::{
     Applicability, Diagnostic, DiagnosticCode, DiagnosticLevel, DiagnosticSpan,
 };
 use toolchain::Tool;
+use triomphe::Arc;
 
 use crate::command::{CommandHandle, ParseFromLine};
 
@@ -151,10 +153,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: Arc<AbsPathBuf>,
+        diagnostic: Diagnostic,
+        package_id: Option<Arc<PackageId>>,
+    },
 
-    /// Request clearing all previous diagnostics
-    ClearDiagnostics { id: usize },
+    /// Request clearing all outdated diagnostics.
+    ClearDiagnostics {
+        id: usize,
+        /// The package whose diagnostics to clear, or if unspecified, all diagnostics.
+        package_id: Option<Arc<PackageId>>,
+    },
 
     /// Request check progress notification to client
     Progress {
@@ -167,15 +178,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()
             }
@@ -201,12 +215,13 @@ enum StateChange {
 struct FlycheckActor {
     /// The workspace id of this flycheck instance.
     id: usize,
+
     sender: Sender<FlycheckMessage>,
     config: FlycheckConfig,
     manifest_path: Option<AbsPathBuf>,
     /// Either the workspace root of the workspace we are flychecking,
     /// or the project root of the project.
-    root: AbsPathBuf,
+    root: Arc<AbsPathBuf>,
     sysroot_root: Option<AbsPathBuf>,
     /// CargoHandle exists to wrap around the communication needed to be able to
     /// run `cargo check` without blocking. Currently the Rust standard library
@@ -216,8 +231,13 @@ struct FlycheckActor {
     command_handle: Option<CommandHandle<CargoCheckMessage>>,
     /// The receiver side of the channel mentioned above.
     command_receiver: Option<Receiver<CargoCheckMessage>>,
+    package_status: FxHashMap<Arc<PackageId>, DiagnosticReceived>,
+}
 
-    status: FlycheckStatus,
+#[derive(PartialEq, Eq, Copy, Clone, Debug)]
+enum DiagnosticReceived {
+    Yes,
+    No,
 }
 
 #[allow(clippy::large_enum_variant)]
@@ -226,13 +246,6 @@ enum Event {
     CheckEvent(Option<CargoCheckMessage>),
 }
 
-#[derive(PartialEq)]
-enum FlycheckStatus {
-    Started,
-    DiagnosticSent,
-    Finished,
-}
-
 pub(crate) const SAVED_FILE_PLACEHOLDER: &str = "$saved_file";
 
 impl FlycheckActor {
@@ -250,11 +263,11 @@ impl FlycheckActor {
             sender,
             config,
             sysroot_root,
-            root: workspace_root,
+            root: Arc::new(workspace_root),
             manifest_path,
             command_handle: None,
             command_receiver: None,
-            status: FlycheckStatus::Finished,
+            package_status: FxHashMap::default(),
         }
     }
 
@@ -307,13 +320,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;
                         }
                     }
                 }
@@ -333,11 +344,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) => {
@@ -347,23 +372,32 @@ impl FlycheckActor {
                             "artifact received"
                         );
                         self.report_progress(Progress::DidCheckCrate(msg.target.name));
+                        self.package_status
+                            .entry(Arc::new(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;
                     }
                 },
             }
@@ -381,7 +415,7 @@ impl FlycheckActor {
             command_handle.cancel();
             self.command_receiver.take();
             self.report_progress(Progress::DidCancel);
-            self.status = FlycheckStatus::Finished;
+            self.package_status.clear();
         }
     }
 
@@ -401,7 +435,7 @@ impl FlycheckActor {
                     cmd.env("RUSTUP_TOOLCHAIN", AsRef::<std::path::Path>::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),
@@ -443,11 +477,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);
                     }
                 }
 
@@ -487,7 +521,7 @@ impl FlycheckActor {
 #[allow(clippy::large_enum_variant)]
 enum CargoCheckMessage {
     CompilerArtifact(cargo_metadata::Artifact),
-    Diagnostic(Diagnostic),
+    Diagnostic { diagnostic: Diagnostic, package_id: Option<Arc<PackageId>> },
 }
 
 impl ParseFromLine for CargoCheckMessage {
@@ -502,11 +536,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(Arc::new(msg.package_id)),
+                        })
                     }
                     _ => None,
                 },
-                JsonMessage::Rustc(message) => Some(CargoCheckMessage::Diagnostic(message)),
+                JsonMessage::Rustc(message) => {
+                    Some(CargoCheckMessage::Diagnostic { diagnostic: message, package_id: None })
+                }
             };
         }