Merge #9701

9701: fix: correctly update diagnostics when files are opened and closed r=matklad a=matklad

bors r+
🤖

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

crates/rust-analyzer/src/document.rs

@@ -1,16 +0,0 @@
-//! In-memory document information.
-
-/// Information about a document that the Language Client
-/// knows about.
-/// Its lifetime is driven by the textDocument/didOpen and textDocument/didClose
-/// client notifications.
-#[derive(Debug, Clone)]
-pub(crate) struct DocumentData {
-    pub(crate) version: i32,
-}
-
-impl DocumentData {
-    pub(crate) fn new(version: i32) -> Self {
-        DocumentData { version }
-    }
-}

crates/rust-analyzer/src/global_state.rs

@@ -8,7 +8,7 @@
 use crossbeam_channel::{unbounded, Receiver, Sender};
 use flycheck::FlycheckHandle;
 use ide::{Analysis, AnalysisHost, Cancellable, Change, FileId};
-use ide_db::base_db::{CrateId, VfsPath};
+use ide_db::base_db::CrateId;
 use lsp_types::{SemanticTokens, Url};
 use parking_lot::{Mutex, RwLock};
 use project_model::{
@@ -20,11 +20,11 @@
 use crate::{
     config::Config,
     diagnostics::{CheckFixes, DiagnosticCollection},
-    document::DocumentData,
     from_proto,
     line_index::{LineEndings, LineIndex},
     lsp_ext,
     main_loop::Task,
+    mem_docs::MemDocs,
     op_queue::OpQueue,
     reload::SourceRootConfig,
     request_metrics::{LatestRequests, RequestMetrics},
@@ -57,7 +57,7 @@ pub(crate) struct GlobalState {
     pub(crate) config: Arc<Config>,
     pub(crate) analysis_host: AnalysisHost,
     pub(crate) diagnostics: DiagnosticCollection,
-    pub(crate) mem_docs: FxHashMap<VfsPath, DocumentData>,
+    pub(crate) mem_docs: MemDocs,
     pub(crate) semantic_tokens_cache: Arc<Mutex<FxHashMap<Url, SemanticTokens>>>,
     pub(crate) shutdown_requested: bool,
     pub(crate) last_reported_status: Option<lsp_ext::ServerStatusParams>,
@@ -115,7 +115,7 @@ pub(crate) struct GlobalStateSnapshot {
     pub(crate) analysis: Analysis,
     pub(crate) check_fixes: CheckFixes,
     pub(crate) latest_requests: Arc<RwLock<LatestRequests>>,
-    mem_docs: FxHashMap<VfsPath, DocumentData>,
+    mem_docs: MemDocs,
     pub(crate) semantic_tokens_cache: Arc<Mutex<FxHashMap<Url, SemanticTokens>>>,
     vfs: Arc<RwLock<(vfs::Vfs, FxHashMap<FileId, LineEndings>)>>,
     pub(crate) workspaces: Arc<Vec<ProjectWorkspace>>,
@@ -147,7 +147,7 @@ pub(crate) fn new(sender: Sender<lsp_server::Message>, config: Config) -> Global
             config: Arc::new(config.clone()),
             analysis_host,
             diagnostics: Default::default(),
-            mem_docs: FxHashMap::default(),
+            mem_docs: MemDocs::default(),
             semantic_tokens_cache: Arc::new(Default::default()),
             shutdown_requested: false,
             last_reported_status: None,

crates/rust-analyzer/src/lib.rs

@@ -33,7 +33,7 @@ macro_rules! eprintln {
 mod request_metrics;
 mod lsp_utils;
 mod thread_pool;
-mod document;
+mod mem_docs;
 mod diff;
 mod op_queue;
 pub mod lsp_ext;

crates/rust-analyzer/src/main_loop.rs

@@ -17,11 +17,11 @@
 use crate::{
     config::Config,
     dispatch::{NotificationDispatcher, RequestDispatcher},
-    document::DocumentData,
     from_proto,
     global_state::{file_id_to_url, url_to_file_id, GlobalState},
     handlers, lsp_ext,
     lsp_utils::{apply_document_changes, is_cancelled, notification_is, Progress},
+    mem_docs::DocumentData,
     reload::{BuildDataProgress, ProjectWorkspaceProgress},
     Result,
 };
@@ -305,7 +305,7 @@ fn handle_event(&mut self, event: Event) -> Result<()> {
                             let vfs = &mut self.vfs.write().0;
                             for (path, contents) in files {
                                 let path = VfsPath::from(path);
-                                if !self.mem_docs.contains_key(&path) {
+                                if !self.mem_docs.contains(&path) {
                                     vfs.set_file_contents(path, contents);
                                 }
                             }
@@ -406,15 +406,32 @@ fn handle_event(&mut self, event: Event) -> Result<()> {
         }
 
         let state_changed = self.process_changes();
+        let memdocs_added_or_removed = self.mem_docs.take_changes();
 
-        if self.is_quiescent() && !was_quiescent {
+        if self.is_quiescent() {
+            if !was_quiescent {
                 for flycheck in &self.flycheck {
                     flycheck.update();
                 }
             }
 
-        if self.is_quiescent() && (!was_quiescent || state_changed) {
-            self.update_file_notifications_on_threadpool();
+            if !was_quiescent || state_changed {
+                // Ensure that only one cache priming task can run at a time
+                self.prime_caches_queue.request_op();
+                if self.prime_caches_queue.should_start_op() {
+                    self.task_pool.handle.spawn_with_sender({
+                        let snap = self.snapshot();
+                        move |sender| {
+                            let cb = |progress| {
+                                sender.send(Task::PrimeCaches(progress)).unwrap();
+                            };
+                            match snap.analysis.prime_caches(cb) {
+                                Ok(()) => (),
+                                Err(_canceled) => (),
+                            }
+                        }
+                    });
+                }
 
                 // Refresh semantic tokens if the client supports it.
                 if self.config.semantic_tokens_refresh() {
@@ -428,6 +445,13 @@ fn handle_event(&mut self, event: Event) -> Result<()> {
                 }
             }
 
+            if !was_quiescent || state_changed || memdocs_added_or_removed {
+                if self.config.publish_diagnostics() {
+                    self.update_diagnostics()
+                }
+            }
+        }
+
         if let Some(diagnostic_changes) = self.diagnostics.take_changes() {
             for file_id in diagnostic_changes {
                 let url = file_id_to_url(&self.vfs.read().0, file_id);
@@ -582,53 +606,43 @@ fn on_notification(&mut self, not: Notification) -> Result<()> {
                     if this
                         .mem_docs
                         .insert(path.clone(), DocumentData::new(params.text_document.version))
-                        .is_some()
+                        .is_err()
                     {
                         log::error!("duplicate DidOpenTextDocument: {}", path)
                     }
-                    let changed = this
-                        .vfs
+                    this.vfs
                         .write()
                         .0
                         .set_file_contents(path, Some(params.text_document.text.into_bytes()));
-
-                    // If the VFS contents are unchanged, update diagnostics, since `handle_event`
-                    // won't see any changes. This avoids missing diagnostics when opening a file.
-                    //
-                    // If the file *was* changed, `handle_event` will already recompute and send
-                    // diagnostics. We can't do it here, since the *current* file contents might be
-                    // unset in salsa, since the VFS change hasn't been applied to the database yet.
-                    if !changed {
-                        this.maybe_update_diagnostics();
-                    }
                 }
                 Ok(())
             })?
             .on::<lsp_types::notification::DidChangeTextDocument>(|this, params| {
                 if let Ok(path) = from_proto::vfs_path(&params.text_document.uri) {
-                    let doc = match this.mem_docs.get_mut(&path) {
-                        Some(doc) => doc,
+                    match this.mem_docs.get_mut(&path) {
+                        Some(doc) => {
+                            // The version passed in DidChangeTextDocument is the version after all edits are applied
+                            // so we should apply it before the vfs is notified.
+                            doc.version = params.text_document.version;
+                        }
                         None => {
                             log::error!("expected DidChangeTextDocument: {}", path);
                             return Ok(());
                         }
                     };
+
                     let vfs = &mut this.vfs.write().0;
                     let file_id = vfs.file_id(&path).unwrap();
                     let mut text = String::from_utf8(vfs.file_contents(file_id).to_vec()).unwrap();
                     apply_document_changes(&mut text, params.content_changes);
 
-                    // The version passed in DidChangeTextDocument is the version after all edits are applied
-                    // so we should apply it before the vfs is notified.
-                    doc.version = params.text_document.version;
-
                     vfs.set_file_contents(path.clone(), Some(text.into_bytes()));
                 }
                 Ok(())
             })?
             .on::<lsp_types::notification::DidCloseTextDocument>(|this, params| {
                 if let Ok(path) = from_proto::vfs_path(&params.text_document.uri) {
-                    if this.mem_docs.remove(&path).is_none() {
+                    if this.mem_docs.remove(&path).is_err() {
                         log::error!("orphan DidCloseTextDocument: {}", path);
                     }
 
@@ -696,35 +710,16 @@ fn on_notification(&mut self, not: Notification) -> Result<()> {
             .finish();
         Ok(())
     }
-    fn update_file_notifications_on_threadpool(&mut self) {
-        self.maybe_update_diagnostics();
 
-        // Ensure that only one cache priming task can run at a time
-        self.prime_caches_queue.request_op();
-        if self.prime_caches_queue.should_start_op() {
-            self.task_pool.handle.spawn_with_sender({
-                let snap = self.snapshot();
-                move |sender| {
-                    let cb = |progress| {
-                        sender.send(Task::PrimeCaches(progress)).unwrap();
-                    };
-                    match snap.analysis.prime_caches(cb) {
-                        Ok(()) => (),
-                        Err(_canceled) => (),
-                    }
-                }
-            });
-        }
-    }
-    fn maybe_update_diagnostics(&mut self) {
+    fn update_diagnostics(&mut self) {
         let subscriptions = self
             .mem_docs
-            .keys()
+            .iter()
             .map(|path| self.vfs.read().0.file_id(path).unwrap())
             .collect::<Vec<_>>();
 
         log::trace!("updating notifications for {:?}", subscriptions);
-        if self.config.publish_diagnostics() {
+
         let snapshot = self.snapshot();
         self.task_pool.handle.spawn(move || {
             let diagnostics = subscriptions
@@ -744,5 +739,4 @@ fn maybe_update_diagnostics(&mut self) {
             Task::Diagnostics(diagnostics)
         })
     }
-    }
 }

crates/rust-analyzer/src/mem_docs.rs

@@ -0,0 +1,65 @@
+//! In-memory document information.
+
+use std::mem;
+
+use rustc_hash::FxHashMap;
+use vfs::VfsPath;
+
+/// Holds the set of in-memory documents.
+///
+/// For these document, there true contents is maintained by the client. It
+/// might be different from what's on disk.
+#[derive(Default, Clone)]
+pub(crate) struct MemDocs {
+    mem_docs: FxHashMap<VfsPath, DocumentData>,
+    added_or_removed: bool,
+}
+
+impl MemDocs {
+    pub(crate) fn contains(&self, path: &VfsPath) -> bool {
+        self.mem_docs.contains_key(path)
+    }
+    pub(crate) fn insert(&mut self, path: VfsPath, data: DocumentData) -> Result<(), ()> {
+        self.added_or_removed = true;
+        match self.mem_docs.insert(path, data) {
+            Some(_) => Err(()),
+            None => Ok(()),
+        }
+    }
+    pub(crate) fn remove(&mut self, path: &VfsPath) -> Result<(), ()> {
+        self.added_or_removed = true;
+        match self.mem_docs.remove(path) {
+            Some(_) => Ok(()),
+            None => Err(()),
+        }
+    }
+    pub(crate) fn get(&self, path: &VfsPath) -> Option<&DocumentData> {
+        self.mem_docs.get(path)
+    }
+    pub(crate) fn get_mut(&mut self, path: &VfsPath) -> Option<&mut DocumentData> {
+        // NB: don't set `self.added_or_removed` here, as that purposefully only
+        // tracks changes to the key set.
+        self.mem_docs.get_mut(path)
+    }
+    pub(crate) fn iter(&self) -> impl Iterator<Item = &VfsPath> {
+        self.mem_docs.keys()
+    }
+    pub(crate) fn take_changes(&mut self) -> bool {
+        mem::replace(&mut self.added_or_removed, false)
+    }
+}
+
+/// Information about a document that the Language Client
+/// knows about.
+/// Its lifetime is driven by the textDocument/didOpen and textDocument/didClose
+/// client notifications.
+#[derive(Debug, Clone)]
+pub(crate) struct DocumentData {
+    pub(crate) version: i32,
+}
+
+impl DocumentData {
+    pub(crate) fn new(version: i32) -> Self {
+        DocumentData { version }
+    }
+}

crates/rust-analyzer/src/reload.rs

@@ -403,7 +403,7 @@ fn eq_ignore_build_data<'a>(
             let mut load = |path: &AbsPath| {
                 let _p = profile::span("GlobalState::load");
                 let vfs_path = vfs::VfsPath::from(path.to_path_buf());
-                if !mem_docs.contains_key(&vfs_path) {
+                if !mem_docs.contains(&vfs_path) {
                     let contents = loader.handle.load_sync(path);
                     vfs.set_file_contents(vfs_path.clone(), contents);
                 }