[ty] Avoid stale I/O diagnostics when closing deleted files (#28988)

Author: MichaReiser

crates/ty_server/src/session.rs

@@ -19,11 +19,11 @@ use lsp_types::{
 use lsp_types::{ExitNotification, Notification};
 use ruff_db::Db;
 use ruff_db::files::{File, system_path_to_file, vendored_path_to_file};
-use ruff_db::system::{System, SystemPath, SystemPathBuf};
+use ruff_db::system::{System, SystemPath, SystemPathBuf, SystemVirtualPath};
 use ruff_python_ast::PySourceType;
 use ty_combine::Combine;
 use ty_project::metadata::Options;
-use ty_project::watch::ChangeEvent;
+use ty_project::watch::{ChangeEvent, DeletedKind};
 use ty_project::{
     ChangeResult, Db as _, ProjectDatabase, ProjectMetadata, ProjectReloadResult,
     ScriptEnvironmentAvailability, UseUv, UvSyncChanges,
@@ -1958,7 +1958,6 @@ impl OpenDocumentHandle {
         }
     }
 
-    #[expect(unused)]
     fn file_path(&self) -> Option<&AnySystemPath> {
         match self {
             Self::Text { path, .. } | Self::Notebook { path, .. } => Some(path),
@@ -2084,21 +2083,29 @@ impl OpenDocumentHandle {
     }
 
     fn update_in_databases(&self, session: &mut Session, client: &Client) {
-        let path = self.notebook_or_file_path();
-        let (containing_workspace, is_virtual, changes) = match path {
-            AnySystemPath::System(system_path) => (
-                session.workspaces().for_path(system_path),
-                false,
-                [ChangeEvent::file_content_changed(system_path.clone())],
-            ),
-            AnySystemPath::SystemVirtual(virtual_path) => (
-                None,
-                true,
-                [ChangeEvent::ChangedVirtual(virtual_path.clone())],
-            ),
+        let change = match self.notebook_or_file_path() {
+            AnySystemPath::System(system_path) => {
+                ChangeEvent::file_content_changed(system_path.clone())
+            }
+            AnySystemPath::SystemVirtual(virtual_path) => {
+                ChangeEvent::ChangedVirtual(virtual_path.clone())
+            }
         };
+        self.apply_change_in_databases(session, client, change);
+    }
+
+    fn apply_change_in_databases(
+        &self,
+        session: &mut Session,
+        client: &Client,
+        change: ChangeEvent,
+    ) {
+        let path = self.notebook_or_file_path();
+        let changes = [change];
+        let is_external = matches!(path, AnySystemPath::System(system_path)
+            if session.workspaces().for_path(system_path).is_none());
 
-        if containing_workspace.is_some() || is_virtual {
+        if !is_external {
             // A containing workspace determines the project for a system file, while virtual
             // documents select a single, arbitrary project. Neither selection depends on import
             // search paths, so update only the selected database.
@@ -2135,80 +2142,114 @@ impl OpenDocumentHandle {
     /// This can return an error when the document does not exist in the
     /// session index.
     pub(crate) fn close(&self, session: &mut Session, client: &Client) -> crate::Result<bool> {
-        let is_cell = self.is_cell();
-        let path = self.notebook_or_file_path();
-
         let removed_document = session.index_mut().close_document(&self.key())?;
 
-        // Close the text or notebook file in the database but skip this
-        // step for cells because closing a cell doesn't close its notebook.
-        let requires_clear_diagnostics = if is_cell {
-            true
-        } else {
-            let db = session.project_db_mut(path);
-
-            match path {
-                AnySystemPath::System(system_path) => {
-                    if let Some(file) = db.files().try_system(db, system_path) {
-                        db.project().close_file(db, file);
-
-                        // In case we preferred the language given by the Client
-                        // over the one detected by the file extension, remove the file
-                        // from the project to handle cases where a user changes the language
-                        // of a file (which results in a didClose and didOpen for the same path but with different languages).
-                        if removed_document.language_id().is_some()
-                            && system_path
-                                .extension()
-                                .and_then(PySourceType::try_from_extension)
-                                .is_none()
-                        {
-                            db.project().remove_file(db, file);
-                        }
-
-                        // Restore file and script membership from the saved contents. Discarding
-                        // unsaved script metadata can bring a file back into the project when
-                        // `exclude-scripts` is enabled. Also request synchronization for saved
-                        // metadata changes that were skipped while the editor overlay was present.
-                        self.update_in_databases(session, client);
-                    } else {
-                        // This can only fail when the path is a directory or it doesn't exists but the
-                        // file should exists for this handler in this branch. This is because every
-                        // close call is preceded by an open call, which ensures that the file is
-                        // interned in the lookup table (`Files`).
-                        tracing::warn!("Salsa file does not exists for {}", system_path);
-                    }
-
-                    // For non-virtual files, we clear diagnostics if:
-                    //
-                    // 1. The file does not belong to any workspace e.g., opening a random file from
-                    //    outside the workspace because closing it acts like the file doesn't exists
-                    // 2. The diagnostic mode is set to open-files only
-                    session.workspaces().for_path(system_path).is_none()
-                        || session
-                            .global_settings()
-                            .diagnostic_mode()
-                            .is_open_files_only()
-                }
-                AnySystemPath::SystemVirtual(virtual_path) => {
-                    if let Some(virtual_file) = db.files().try_virtual_file(virtual_path) {
-                        db.project().close_file(db, virtual_file.file());
-                        virtual_file.close(db);
-                        // Bump the file's revision back to using the file system's revision.
-                        virtual_file.sync(db);
-                    } else {
-                        tracing::warn!("Salsa virtual file does not exists for {}", virtual_path);
-                    }
+        let should_clear_diagnostics = match self.file_path() {
+            // Closing a notebook cell clears its diagnostics but leaves its notebook open.
+            None => true,
+            Some(AnySystemPath::System(path)) => {
+                let is_deleted = self.close_system_file(session, client, path, &removed_document);
+
+                // For non-virtual files, we clear diagnostics if:
+                //
+                // 1. The file was deleted before it was closed
+                // 2. The file does not belong to any workspace e.g., opening a random file from
+                //    outside the workspace because closing it acts like the file doesn't exist
+                // 3. The diagnostic mode is set to open-files only
+                is_deleted
+                    || session.workspaces().for_path(path).is_none()
+                    || session
+                        .global_settings()
+                        .diagnostic_mode()
+                        .is_open_files_only()
+            }
+            Some(AnySystemPath::SystemVirtual(path)) => {
+                self.close_virtual_file(session, path);
 
-                    // Always clear diagnostics for virtual files, as they don't really exist on disk
-                    // which means closing them is like deleting the file.
-                    true
-                }
+                // Always clear diagnostics for virtual files, as they don't really exist on disk
+                // which means closing them is like deleting the file.
+                true
             }
         };
 
         session.bump_revision();
 
-        Ok(requires_clear_diagnostics)
+        Ok(should_clear_diagnostics)
+    }
+
+    /// Closes a system file and returns whether it was deleted from disk.
+    fn close_system_file(
+        &self,
+        session: &mut Session,
+        client: &Client,
+        system_path: &SystemPath,
+        removed_document: &Document,
+    ) -> bool {
+        let db = session.project_db_mut(self.notebook_or_file_path());
+
+        // The editor can delete the file before closing it and before its watcher
+        // reports the deletion. Check disk state after removing the editor overlay.
+        let is_deleted = db
+            .system()
+            .path_metadata(system_path)
+            .is_err_and(|error| error.kind() == std::io::ErrorKind::NotFound);
+
+        if let Some(file) = db.files().try_system(db, system_path) {
+            db.project().close_file(db, file);
+
+            // In case we preferred the language given by the Client
+            // over the one detected by the file extension, remove the file
+            // from the project to handle cases where a user changes the language
+            // of a file (which results in a didClose and didOpen for the same path but with different languages).
+            if removed_document.language_id().is_some()
+                && system_path
+                    .extension()
+                    .and_then(PySourceType::try_from_extension)
+                    .is_none()
+            {
+                db.project().remove_file(db, file);
+            }
+
+            if is_deleted {
+                // A content change leaves missing files in the project index, allowing
+                // workspace diagnostics to report an I/O error until the watcher fires.
+                self.apply_change_in_databases(
+                    session,
+                    client,
+                    ChangeEvent::Deleted {
+                        path: system_path.to_path_buf(),
+                        kind: DeletedKind::File,
+                    },
+                );
+            } else {
+                // Restore file and script membership from the saved contents. Discarding
+                // unsaved script metadata can bring a file back into the project when
+                // `exclude-scripts` is enabled. Also request synchronization for saved
+                // metadata changes that were skipped while the editor overlay was present.
+                self.update_in_databases(session, client);
+            }
+        } else {
+            // This can only fail when the path is a directory or it doesn't exists but the
+            // file should exists for this handler in this branch. This is because every
+            // close call is preceded by an open call, which ensures that the file is
+            // interned in the lookup table (`Files`).
+            tracing::warn!("Salsa file does not exists for {}", system_path);
+        }
+
+        is_deleted
+    }
+
+    fn close_virtual_file(&self, session: &mut Session, virtual_path: &SystemVirtualPath) {
+        let db = session.project_db_mut(self.notebook_or_file_path());
+
+        if let Some(virtual_file) = db.files().try_virtual_file(virtual_path) {
+            db.project().close_file(db, virtual_file.file());
+            virtual_file.close(db);
+            // Bump the file's revision back to using the file system's revision.
+            virtual_file.sync(db);
+        } else {
+            tracing::warn!("Salsa virtual file does not exists for {}", virtual_path);
+        }
     }
 }
 

crates/ty_server/tests/e2e/pull_diagnostics.rs

@@ -4,16 +4,18 @@ use anyhow::Result;
 use insta::{assert_compact_json_snapshot, assert_debug_snapshot};
 use lsp_server::RequestId;
 use lsp_types::{
-    DocumentDiagnosticReport, PartialResultParams, PreviousResultId, ProgressNotification, Uri,
-    WorkDoneProgressBegin, WorkDoneProgressEnd, WorkDoneProgressParams, WorkspaceDiagnosticParams,
-    WorkspaceDiagnosticReport, WorkspaceDiagnosticReportPartialResult,
+    DocumentDiagnosticReport, FileChangeType, PartialResultParams, PreviousResultId,
+    ProgressNotification, Uri, WorkDoneProgressBegin, WorkDoneProgressEnd, WorkDoneProgressParams,
+    WorkspaceDiagnosticParams, WorkspaceDiagnosticReport, WorkspaceDiagnosticReportPartialResult,
     WorkspaceDocumentDiagnosticReport,
 };
 use lsp_types::{TextDocumentContentChangeWholeDocument, WorkspaceDiagnosticRequest};
 use ruff_db::system::SystemPath;
 use ty_server::{ClientOptions, DiagnosticMode};
 
-use crate::diagnostic_snapshots::condensed_document_diagnostic_snapshot;
+use crate::diagnostic_snapshots::{
+    condensed_document_diagnostic_snapshot, condensed_workspace_diagnostic_snapshot,
+};
 use crate::{AwaitResponseError, TestServer, TestServerBuilder};
 
 #[test]
@@ -1420,6 +1422,59 @@ def foo() -> str:
     Ok(())
 }
 
+#[test]
+fn closing_deleted_file_clears_diagnostics_before_watcher_notification() -> Result<()> {
+    let path = SystemPath::new("src/deleted.py");
+    let source = "value: int = 'wrong'\n";
+    let mut server = create_workspace_server_with_file(SystemPath::new("src"), path, source)?;
+    server.open_text_document(path, source, 1);
+
+    let initial = server.workspace_diagnostic_request(None, None);
+    let previous_result_ids = extract_result_ids_from_response(&initial);
+    insta::assert_snapshot!(condensed_workspace_diagnostic_snapshot(initial), @r#"
+    file://<temp_dir>/src/deleted.py
+    	0:13..0:20[ERROR]: Object of type `Literal["wrong"]` is not assignable to `int`
+    "#);
+
+    let request_id = server.send_request::<WorkspaceDiagnosticRequest>(WorkspaceDiagnosticParams {
+        previous_result_ids: previous_result_ids.clone(),
+        ..WorkspaceDiagnosticParams::default()
+    });
+    assert_workspace_diagnostics_suspends_for_long_polling(&mut server, &request_id);
+
+    // Editors can delete the file and close it before the filesystem watcher reports deletion.
+    std::fs::remove_file(server.file_path(path))?;
+    server.close_text_document(path);
+    insta::assert_snapshot!(
+        condensed_document_diagnostic_snapshot(server.document_diagnostic_request(path, None)),
+        @"",
+    );
+
+    let after_close = server.await_response::<WorkspaceDiagnosticRequest>(&request_id);
+    insta::assert_snapshot!(
+        condensed_workspace_diagnostic_snapshot(after_close),
+        @"file://<temp_dir>/src/deleted.py",
+    );
+
+    // The later watcher notification must preserve the empty report.
+    server.did_change_watched_file(path, FileChangeType::Deleted);
+    let after_watcher = server.workspace_diagnostic_request(None, Some(previous_result_ids));
+    insta::assert_snapshot!(
+        condensed_workspace_diagnostic_snapshot(after_watcher),
+        @"file://<temp_dir>/src/deleted.py",
+    );
+
+    server.write_file(path, source)?;
+    server.did_change_watched_file(path, FileChangeType::Created);
+    let recreated = server.workspace_diagnostic_request(None, None);
+    insta::assert_snapshot!(condensed_workspace_diagnostic_snapshot(recreated), @r#"
+    file://<temp_dir>/src/deleted.py
+    	0:13..0:20[ERROR]: Object of type `Literal["wrong"]` is not assignable to `int`
+    "#);
+
+    Ok(())
+}
+
 // Helper functions for long-polling tests
 fn create_workspace_server_with_file(
     workspace_root: &SystemPath,