[ty] publish settings diagnostics (#19335)

crates/ty_project/src/lib.rs

@@ -469,6 +469,14 @@ impl Project {
             self.set_file_set(db).to(IndexedFiles::lazy());
         }
     }
+
+    /// Check if the project's settings have any issues
+    pub fn check_settings(&self, db: &dyn Db) -> Vec<Diagnostic> {
+        self.settings_diagnostics(db)
+            .iter()
+            .map(OptionDiagnostic::to_diagnostic)
+            .collect()
+    }
 }
 
 #[salsa::tracked(returns(deref), heap_size=get_size2::GetSize::get_heap_size)]

crates/ty_server/src/server.rs

@@ -21,8 +21,8 @@ mod schedule;
 
 use crate::session::client::Client;
 pub(crate) use api::Error;
+pub(crate) use api::publish_settings_diagnostics;
 pub(crate) use main_loop::{Action, ConnectionSender, Event, MainLoopReceiver, MainLoopSender};
-
 pub(crate) type Result<T> = std::result::Result<T, api::Error>;
 
 pub(crate) struct Server {

crates/ty_server/src/server/api.rs

@@ -17,6 +17,7 @@ mod traits;
 use self::traits::{NotificationHandler, RequestHandler};
 use super::{Result, schedule::BackgroundSchedule};
 use crate::session::client::Client;
+pub(crate) use diagnostics::publish_settings_diagnostics;
 use ruff_db::panic::PanicError;
 
 /// Processes a request from the client to the server.

crates/ty_server/src/server/api/diagnostics.rs

@@ -8,11 +8,13 @@ use rustc_hash::FxHashMap;
 use ruff_db::diagnostic::{Annotation, Severity, SubDiagnostic};
 use ruff_db::files::FileRange;
 use ruff_db::source::{line_index, source_text};
+use ruff_db::system::SystemPathBuf;
 use ty_project::{Db, ProjectDatabase};
 
 use crate::document::{DocumentKey, FileRangeExt, ToRangeExt};
 use crate::session::DocumentSnapshot;
 use crate::session::client::Client;
+use crate::system::{AnySystemPath, file_to_url};
 use crate::{PositionEncoding, Session};
 
 /// Represents the diagnostics for a text document or a notebook document.
@@ -109,6 +111,82 @@ pub(super) fn publish_diagnostics(session: &Session, key: &DocumentKey, client:
     }
 }
 
+/// Publishes settings diagnostics for all the project at the given path
+/// using the [publish diagnostics notification].
+///
+/// [publish diagnostics notification]: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_publishDiagnostics
+pub(crate) fn publish_settings_diagnostics(
+    session: &mut Session,
+    client: &Client,
+    path: SystemPathBuf,
+) {
+    // Don't publish settings diagnostics for workspace that are already doing full diagnostics.
+    //
+    // Note we DO NOT respect the fact that clients support pulls because these are
+    // files they *specifically* won't pull diagnostics from us for, because we don't
+    // claim to be an LSP for them.
+    let has_workspace_diagnostics = session
+        .workspaces()
+        .for_path(&path)
+        .map(|workspace| workspace.settings().diagnostic_mode().is_workspace())
+        .unwrap_or(false);
+    if has_workspace_diagnostics {
+        return;
+    }
+
+    let session_encoding = session.position_encoding();
+    let state = session.project_state_mut(&AnySystemPath::System(path));
+    let db = &state.db;
+    let project = db.project();
+    let settings_diagnostics = project.check_settings(db);
+
+    // We need to send diagnostics if we have non-empty ones, or we have ones to clear.
+    // These will both almost always be empty so this function will almost always be a no-op.
+    if settings_diagnostics.is_empty() && state.untracked_files_with_pushed_diagnostics.is_empty() {
+        return;
+    }
+
+    // Group diagnostics by URL
+    let mut diagnostics_by_url: FxHashMap<Url, Vec<_>> = FxHashMap::default();
+    for diagnostic in settings_diagnostics {
+        if let Some(span) = diagnostic.primary_span() {
+            let file = span.expect_ty_file();
+            let Some(url) = file_to_url(db, file) else {
+                tracing::debug!("Failed to convert file to URL at {}", file.path(db));
+                continue;
+            };
+            diagnostics_by_url.entry(url).or_default().push(diagnostic);
+        }
+    }
+
+    // Record the URLs we're sending non-empty diagnostics for, so we know to clear them
+    // the next time we publish settings diagnostics!
+    let old_untracked = std::mem::replace(
+        &mut state.untracked_files_with_pushed_diagnostics,
+        diagnostics_by_url.keys().cloned().collect(),
+    );
+
+    // Add empty diagnostics for any files that had diagnostics before but don't now.
+    // This will clear them (either the file is no longer relevant to us or fixed!)
+    for url in old_untracked {
+        diagnostics_by_url.entry(url).or_default();
+    }
+    // Send the settings diagnostics!
+    for (url, file_diagnostics) in diagnostics_by_url {
+        // Convert diagnostics to LSP format
+        let lsp_diagnostics = file_diagnostics
+            .into_iter()
+            .map(|diagnostic| to_lsp_diagnostic(db, &diagnostic, session_encoding))
+            .collect::<Vec<_>>();
+
+        client.send_notification::<PublishDiagnostics>(PublishDiagnosticsParams {
+            uri: url,
+            diagnostics: lsp_diagnostics,
+            version: None,
+        });
+    }
+}
+
 pub(super) fn compute_diagnostics(
     db: &ProjectDatabase,
     snapshot: &DocumentSnapshot,

crates/ty_server/src/server/api/notifications/did_change_watched_files.rs

@@ -1,5 +1,5 @@
 use crate::server::Result;
-use crate::server::api::diagnostics::publish_diagnostics;
+use crate::server::api::diagnostics::{publish_diagnostics, publish_settings_diagnostics};
 use crate::server::api::traits::{NotificationHandler, SyncNotificationHandler};
 use crate::session::Session;
 use crate::session::client::Client;
@@ -88,7 +88,8 @@ impl SyncNotificationHandler for DidChangeWatchedFiles {
         for (root, changes) in events_by_db {
             tracing::debug!("Applying changes to `{root}`");
 
-            let result = session.apply_changes(&AnySystemPath::System(root), changes);
+            let result = session.apply_changes(&AnySystemPath::System(root.clone()), changes);
+            publish_settings_diagnostics(session, client, root);
 
             project_changed |= result.project_changed();
         }
@@ -107,7 +108,6 @@ impl SyncNotificationHandler for DidChangeWatchedFiles {
                     publish_diagnostics(session, &key, client);
                 }
             }
-
             // TODO: always publish diagnostics for notebook files (since they don't use pull diagnostics)
         }
 

crates/ty_server/src/session.rs

@@ -21,6 +21,7 @@ pub(crate) use self::index::DocumentQuery;
 pub(crate) use self::options::{AllOptions, ClientOptions, DiagnosticMode};
 pub(crate) use self::settings::ClientSettings;
 use crate::document::{DocumentKey, DocumentVersion, NotebookDocument};
+use crate::server::publish_settings_diagnostics;
 use crate::session::client::Client;
 use crate::session::request_queue::RequestQueue;
 use crate::system::{AnySystemPath, LSPSystem};
@@ -49,7 +50,7 @@ pub(crate) struct Session {
     workspaces: Workspaces,
 
     /// The projects across all workspaces.
-    projects: BTreeMap<SystemPathBuf, ProjectDatabase>,
+    projects: BTreeMap<SystemPathBuf, ProjectState>,
 
     /// The project to use for files outside any workspace. For example, if the user
     /// opens the project `<home>/my_project` in VS code but they then opens a Python file from their Desktop.
@@ -73,6 +74,25 @@ pub(crate) struct Session {
     deferred_messages: VecDeque<Message>,
 }
 
+/// LSP State for a Project
+pub(crate) struct ProjectState {
+    pub(crate) db: ProjectDatabase,
+    /// Files that we have outstanding otherwise-untracked pushed diagnostics for.
+    ///
+    /// In `CheckMode::OpenFiles` we still read some files that the client hasn't
+    /// told us to open. Notably settings files like `pyproject.toml`. In this
+    /// mode the client will never pull diagnostics for that file, and because
+    /// the file isn't formally "open" we also don't have a reliable signal to
+    /// refresh diagnostics for it either.
+    ///
+    /// However diagnostics for those files include things like "you typo'd your
+    /// configuration for the LSP itself", so it's really important that we tell
+    /// the user about them! So we remember which ones we have emitted diagnostics
+    /// for so that we can clear the diagnostics for all of them before we go
+    /// to update any of them.
+    pub(crate) untracked_files_with_pushed_diagnostics: Vec<Url>,
+}
+
 impl Session {
     pub(crate) fn new(
         client_capabilities: &ClientCapabilities,
@@ -168,17 +188,7 @@ impl Session {
     ///
     /// If the path is a virtual path, it will return the first project database in the session.
     pub(crate) fn project_db(&self, path: &AnySystemPath) -> &ProjectDatabase {
-        match path {
+        &self.project_state(path).db
-            AnySystemPath::System(system_path) => self
-                .project_db_for_path(system_path)
-                .unwrap_or_else(|| self.default_project.get(self.index.as_ref())),
-            AnySystemPath::SystemVirtual(_virtual_path) => {
-                // TODO: Currently, ty only supports single workspace but we need to figure out
-                // which project should this virtual path belong to when there are multiple
-                // projects: https://github.com/astral-sh/ty/issues/794
-                self.projects.iter().next().map(|(_, db)| db).unwrap()
-            }
-        }
     }
 
     /// Returns a mutable reference to the project's [`ProjectDatabase`] in which the given `path`
@@ -188,20 +198,7 @@ impl Session {
     ///
     /// [`project_db`]: Session::project_db
     pub(crate) fn project_db_mut(&mut self, path: &AnySystemPath) -> &mut ProjectDatabase {
-        match path {
+        &mut self.project_state_mut(path).db
-            AnySystemPath::System(system_path) => self
-                .projects
-                .range_mut(..=system_path.to_path_buf())
-                .next_back()
-                .map(|(_, db)| db)
-                .unwrap_or_else(|| self.default_project.get_mut(self.index.as_ref())),
-            AnySystemPath::SystemVirtual(_virtual_path) => {
-                // TODO: Currently, ty only supports single workspace but we need to figure out
-                // which project should this virtual path belong to when there are multiple
-                // projects: https://github.com/astral-sh/ty/issues/794
-                self.projects.iter_mut().next().map(|(_, db)| db).unwrap()
-            }
-        }
     }
 
     /// Returns a reference to the project's [`ProjectDatabase`] corresponding to the given path, if
@@ -210,10 +207,70 @@ impl Session {
         &self,
         path: impl AsRef<SystemPath>,
     ) -> Option<&ProjectDatabase> {
+        self.project_state_for_path(path).map(|state| &state.db)
+    }
+
+    /// Returns a reference to the project's [`ProjectState`] in which the given `path` belongs.
+    ///
+    /// If the path is a system path, it will return the project database that is closest to the
+    /// given path, or the default project if no project is found for the path.
+    ///
+    /// If the path is a virtual path, it will return the first project database in the session.
+    pub(crate) fn project_state(&self, path: &AnySystemPath) -> &ProjectState {
+        match path {
+            AnySystemPath::System(system_path) => self
+                .project_state_for_path(system_path)
+                .unwrap_or_else(|| self.default_project.get(self.index.as_ref())),
+            AnySystemPath::SystemVirtual(_virtual_path) => {
+                // TODO: Currently, ty only supports single workspace but we need to figure out
+                // which project should this virtual path belong to when there are multiple
+                // projects: https://github.com/astral-sh/ty/issues/794
+                self.projects
+                    .iter()
+                    .next()
+                    .map(|(_, project)| project)
+                    .unwrap()
+            }
+        }
+    }
+
+    /// Returns a mutable reference to the project's [`ProjectState`] in which the given `path`
+    /// belongs.
+    ///
+    /// Refer to [`project_db`] for more details on how the project is selected.
+    ///
+    /// [`project_db`]: Session::project_db
+    pub(crate) fn project_state_mut(&mut self, path: &AnySystemPath) -> &mut ProjectState {
+        match path {
+            AnySystemPath::System(system_path) => self
+                .projects
+                .range_mut(..=system_path.to_path_buf())
+                .next_back()
+                .map(|(_, project)| project)
+                .unwrap_or_else(|| self.default_project.get_mut(self.index.as_ref())),
+            AnySystemPath::SystemVirtual(_virtual_path) => {
+                // TODO: Currently, ty only supports single workspace but we need to figure out
+                // which project should this virtual path belong to when there are multiple
+                // projects: https://github.com/astral-sh/ty/issues/794
+                self.projects
+                    .iter_mut()
+                    .next()
+                    .map(|(_, project)| project)
+                    .unwrap()
+            }
+        }
+    }
+
+    /// Returns a reference to the project's [`ProjectState`] corresponding to the given path, if
+    /// any.
+    pub(crate) fn project_state_for_path(
+        &self,
+        path: impl AsRef<SystemPath>,
+    ) -> Option<&ProjectState> {
         self.projects
             .range(..=path.as_ref().to_path_buf())
             .next_back()
-            .map(|(_, db)| db)
+            .map(|(_, project)| project)
     }
 
     pub(crate) fn apply_changes(
@@ -237,6 +294,13 @@ impl Session {
     ///
     /// This iterator will only yield the default project database if it has been used.
     fn projects_mut(&mut self) -> impl Iterator<Item = &'_ mut ProjectDatabase> + '_ {
+        self.project_states_mut().map(|project| &mut project.db)
+    }
+
+    /// Returns a mutable iterator over all projects that have been initialized to this point.
+    ///
+    /// This iterator will only yield the default project if it has been used.
+    pub(crate) fn project_states_mut(&mut self) -> impl Iterator<Item = &'_ mut ProjectState> + '_ {
         let default_project = self.default_project.try_get_mut();
         self.projects.values_mut().chain(default_project)
     }
@@ -282,10 +346,8 @@ impl Session {
                     ProjectDatabase::new(metadata, system.clone())
                 });
 
-            match project {
+            let (root, db) = match project {
-                Ok(project) => {
+                Ok(db) => (root, db),
-                    self.projects.insert(root, project);
-                }
                 Err(err) => {
                     tracing::error!(
                         "Failed to create project for `{root}`: {err:#}. Falling back to default settings"
@@ -300,16 +362,29 @@ impl Session {
                             .context("Failed to convert default options to metadata")
                             .and_then(|metadata| ProjectDatabase::new(metadata, system))
                             .expect("Default configuration to be valid");
-
+                    let default_root = db_with_default_settings
-                    self.projects.insert(
-                        db_with_default_settings
                         .project()
                         .root(&db_with_default_settings)
-                            .to_path_buf(),
+                        .to_path_buf();
-                        db_with_default_settings,
+
+                    (default_root, db_with_default_settings)
+                }
+            };
+
+            // Carry forward diagnostic state if any exists
+            let previous = self.projects.remove(&root);
+            let untracked = previous
+                .map(|state| state.untracked_files_with_pushed_diagnostics)
+                .unwrap_or_default();
+            self.projects.insert(
+                root.clone(),
+                ProjectState {
+                    db,
+                    untracked_files_with_pushed_diagnostics: untracked,
+                },
             );
-                }
+
-            }
+            publish_settings_diagnostics(self, client, root);
         }
 
         assert!(
@@ -343,7 +418,12 @@ impl Session {
     /// Creates a snapshot of the current state of the [`Session`].
     pub(crate) fn take_session_snapshot(&self) -> SessionSnapshot {
         SessionSnapshot {
-            projects: self.projects.values().cloned().collect(),
+            projects: self
+                .projects
+                .values()
+                .map(|project| &project.db)
+                .cloned()
+                .collect(),
             index: self.index.clone().unwrap(),
             position_encoding: self.position_encoding,
         }
@@ -437,6 +517,10 @@ impl Session {
     pub(crate) fn global_settings(&self) -> Arc<ClientSettings> {
         self.index().global_settings()
     }
+
+    pub(crate) fn position_encoding(&self) -> PositionEncoding {
+        self.position_encoding
+    }
 }
 
 /// A guard that holds the only reference to the index and allows modifying it.
@@ -654,14 +738,14 @@ impl Workspace {
 ///    We really want that to be the actual project database and not our fallback database.
 /// 2. The logs when the server starts can be confusing if it once shows it uses Python X (for the default db)
 ///    but then has another log that it uses Python Y (for the actual project db).
-struct DefaultProject(std::sync::OnceLock<ProjectDatabase>);
+struct DefaultProject(std::sync::OnceLock<ProjectState>);
 
 impl DefaultProject {
     pub(crate) fn new() -> Self {
         DefaultProject(std::sync::OnceLock::new())
     }
 
-    pub(crate) fn get(&self, index: Option<&Arc<Index>>) -> &ProjectDatabase {
+    pub(crate) fn get(&self, index: Option<&Arc<Index>>) -> &ProjectState {
         self.0.get_or_init(|| {
             tracing::info!("Initialize default project");
 
@@ -672,11 +756,14 @@ impl DefaultProject {
                 None,
             )
             .unwrap();
-            ProjectDatabase::new(metadata, system).unwrap()
+            ProjectState {
+                db: ProjectDatabase::new(metadata, system).unwrap(),
+                untracked_files_with_pushed_diagnostics: Vec::new(),
+            }
         })
     }
 
-    pub(crate) fn get_mut(&mut self, index: Option<&Arc<Index>>) -> &mut ProjectDatabase {
+    pub(crate) fn get_mut(&mut self, index: Option<&Arc<Index>>) -> &mut ProjectState {
         let _ = self.get(index);
 
         // SAFETY: The `OnceLock` is guaranteed to be initialized at this point because
@@ -684,7 +771,7 @@ impl DefaultProject {
         self.0.get_mut().unwrap()
     }
 
-    pub(crate) fn try_get_mut(&mut self) -> Option<&mut ProjectDatabase> {
+    pub(crate) fn try_get_mut(&mut self) -> Option<&mut ProjectState> {
         self.0.get_mut()
     }
 }