From 35f33d9bf5b9dd1cdf4af96999c0f61b8e900579 Mon Sep 17 00:00:00 2001 From: Aria Desires Date: Thu, 17 Jul 2025 11:57:00 -0400 Subject: [PATCH] [ty] publish settings diagnostics (#19335) --- crates/ty_project/src/lib.rs | 8 + crates/ty_server/src/server.rs | 2 +- crates/ty_server/src/server/api.rs | 1 + .../ty_server/src/server/api/diagnostics.rs | 78 ++++++++ .../notifications/did_change_watched_files.rs | 6 +- crates/ty_server/src/session.rs | 177 +++++++++++++----- 6 files changed, 223 insertions(+), 49 deletions(-) diff --git a/crates/ty_project/src/lib.rs b/crates/ty_project/src/lib.rs index 377b3d1276..57cba59721 100644 --- a/crates/ty_project/src/lib.rs +++ b/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 { + self.settings_diagnostics(db) + .iter() + .map(OptionDiagnostic::to_diagnostic) + .collect() + } } #[salsa::tracked(returns(deref), heap_size=get_size2::GetSize::get_heap_size)] diff --git a/crates/ty_server/src/server.rs b/crates/ty_server/src/server.rs index cea779f9ff..03b7b83265 100644 --- a/crates/ty_server/src/server.rs +++ b/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 = std::result::Result; pub(crate) struct Server { diff --git a/crates/ty_server/src/server/api.rs b/crates/ty_server/src/server/api.rs index a07367ad6b..41d14cbf15 100644 --- a/crates/ty_server/src/server/api.rs +++ b/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. diff --git a/crates/ty_server/src/server/api/diagnostics.rs b/crates/ty_server/src/server/api/diagnostics.rs index 438dc1c172..11e39796ad 100644 --- a/crates/ty_server/src/server/api/diagnostics.rs +++ b/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> = 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::>(); + + client.send_notification::(PublishDiagnosticsParams { + uri: url, + diagnostics: lsp_diagnostics, + version: None, + }); + } +} + pub(super) fn compute_diagnostics( db: &ProjectDatabase, snapshot: &DocumentSnapshot, diff --git a/crates/ty_server/src/server/api/notifications/did_change_watched_files.rs b/crates/ty_server/src/server/api/notifications/did_change_watched_files.rs index 16fbbe2aee..41001ae8fc 100644 --- a/crates/ty_server/src/server/api/notifications/did_change_watched_files.rs +++ b/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) } diff --git a/crates/ty_server/src/session.rs b/crates/ty_server/src/session.rs index e0eac2d046..4ce3f0448c 100644 --- a/crates/ty_server/src/session.rs +++ b/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, + projects: BTreeMap, /// The project to use for files outside any workspace. For example, if the user /// opens the project `/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, } +/// 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, +} + 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 { - 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() - } - } + &self.project_state(path).db } /// 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 { - 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() - } - } + &mut self.project_state_mut(path).db } /// Returns a reference to the project's [`ProjectDatabase`] corresponding to the given path, if @@ -210,10 +207,70 @@ impl Session { &self, path: impl AsRef, ) -> 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, + ) -> 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 + '_ { + 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 + '_ { 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 { - Ok(project) => { - self.projects.insert(root, project); - } + let (root, db) = match project { + Ok(db) => (root, db), 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 + .project() + .root(&db_with_default_settings) + .to_path_buf(); - self.projects.insert( - db_with_default_settings - .project() - .root(&db_with_default_settings) - .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 { 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); +struct DefaultProject(std::sync::OnceLock); impl DefaultProject { pub(crate) fn new() -> Self { DefaultProject(std::sync::OnceLock::new()) } - pub(crate) fn get(&self, index: Option<&Arc>) -> &ProjectDatabase { + pub(crate) fn get(&self, index: Option<&Arc>) -> &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>) -> &mut ProjectDatabase { + pub(crate) fn get_mut(&mut self, index: Option<&Arc>) -> &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() } }