Skip to content

Commit dc56c33

Browse files
authored
[ty] Initial support for workspace diagnostics (#18939)
## Summary This PR adds initial support for workspace diagnostics in the ty server. Reference spec: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_diagnostic This is currently implemented via the **pull diagnostics method** which was added in the current version (3.17) and the server advertises it via the `diagnosticProvider.workspaceDiagnostics` server capability. **Note:** This might be a bit confusing but a workspace diagnostics is not for a single workspace but for all the workspaces that the server handles. These are the ones that the server received during initialization. Currently, the ty server doesn't support multiple workspaces so this capability is also limited to provide diagnostics only for a single workspace (the first one if the client provided multiple). A new `ty.diagnosticMode` server setting is added which can be either `workspace` (for workspace diagnostics) or `openFilesOnly` (for checking only open files) (default). This is same as `python.analysis.diagnosticMode` that Pyright / Pylance utilizes. In the future, we could use the value under `python.*` namespace as fallback to improve the experience on user side to avoid setting the value multiple times. Part of: astral-sh/ty#81 ## Test Plan This capability was introduced in the current LSP version (~3 years) and the way it's implemented by various clients are a bit different. I've provided notes on what I've noticed and what would need to be done on our side to further improve the experience. ### VS Code VS Code sends the `workspace/diagnostic` requests every ~2 second: ``` [Trace - 12:12:32 PM] Sending request 'workspace/diagnostic - (403)'. [Trace - 12:12:32 PM] Received response 'workspace/diagnostic - (403)' in 2ms. [Trace - 12:12:34 PM] Sending request 'workspace/diagnostic - (404)'. [Trace - 12:12:34 PM] Received response 'workspace/diagnostic - (404)' in 2ms. [Trace - 12:12:36 PM] Sending request 'workspace/diagnostic - (405)'. [Trace - 12:12:36 PM] Received response 'workspace/diagnostic - (405)' in 2ms. [Trace - 12:12:38 PM] Sending request 'workspace/diagnostic - (406)'. [Trace - 12:12:38 PM] Received response 'workspace/diagnostic - (406)' in 3ms. [Trace - 12:12:40 PM] Sending request 'workspace/diagnostic - (407)'. [Trace - 12:12:40 PM] Received response 'workspace/diagnostic - (407)' in 2ms. ... ``` I couldn't really find any resource that explains this behavior. But, this does mean that we'd need to implement the caching layer via the previous result ids sooner. This will allow the server to avoid sending all the diagnostics on every request and instead just send a response stating that the diagnostics hasn't changed yet. This could possibly be achieved by using the salsa ID. If we switch from workspace diagnostics to open-files diagnostics, the server would send the diagnostics only via the `textDocument/diagnostic` endpoint. Here, when a document containing the diagnostic is closed, the server would send a publish diagnostics notification with an empty list of diagnostics to clear the diagnostics from that document. The issue is the VS Code doesn't seem to be clearing the diagnostics in this case even though it receives the notification. (I'm going to open an issue on VS Code side for this today.) https://github.com/user-attachments/assets/b0c0833d-386c-49f5-8a15-0ac9133e15ed ### Zed Zed's implementation works by refreshing the workspace diagnostics whenever the content of the documents are changed. This seems like a very reasonable behavior and I was a bit surprised that VS Code didn't use this heuristic. https://github.com/user-attachments/assets/71c7b546-7970-434a-9ba0-4fa620647f6c ### Neovim Neovim only recently added support for workspace diagnostics (neovim/neovim#34262, merged ~3 weeks ago) so it's only available on nightly versions. The initial support is limited and requires fetching the workspace diagnostics manually as demonstrated in the video. It doesn't support refreshing the workspace diagnostics either, so that would need to be done manually as well. I'm assuming that these are just a temporary limitation and will be implemented before the stable release. https://github.com/user-attachments/assets/25b4a0e5-9833-4877-88ad-279904fffaf9
1 parent a95c18a commit dc56c33

File tree

11 files changed

+182
-10
lines changed

11 files changed

+182
-10
lines changed

crates/ty_project/src/db.rs

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -83,15 +83,20 @@ impl ProjectDatabase {
8383

8484
/// Checks all open files in the project and its dependencies.
8585
pub fn check(&self) -> Vec<Diagnostic> {
86-
let mut reporter = DummyReporter;
87-
let reporter = AssertUnwindSafe(&mut reporter as &mut dyn Reporter);
88-
self.project().check(self, reporter)
86+
self.check_with_mode(CheckMode::OpenFiles)
8987
}
9088

9189
/// Checks all open files in the project and its dependencies, using the given reporter.
9290
pub fn check_with_reporter(&self, reporter: &mut dyn Reporter) -> Vec<Diagnostic> {
9391
let reporter = AssertUnwindSafe(reporter);
94-
self.project().check(self, reporter)
92+
self.project().check(self, CheckMode::OpenFiles, reporter)
93+
}
94+
95+
/// Check the project with the given mode.
96+
pub fn check_with_mode(&self, mode: CheckMode) -> Vec<Diagnostic> {
97+
let mut reporter = DummyReporter;
98+
let reporter = AssertUnwindSafe(&mut reporter as &mut dyn Reporter);
99+
self.project().check(self, mode, reporter)
95100
}
96101

97102
#[tracing::instrument(level = "debug", skip(self))]
@@ -157,6 +162,17 @@ impl std::fmt::Debug for ProjectDatabase {
157162
}
158163
}
159164

165+
#[derive(Debug, Clone, Copy, PartialEq)]
166+
pub enum CheckMode {
167+
/// Checks only the open files in the project.
168+
OpenFiles,
169+
170+
/// Checks all files in the project, ignoring the open file set.
171+
///
172+
/// This includes virtual files, such as those created by the language server.
173+
AllFiles,
174+
}
175+
160176
/// Stores memory usage information.
161177
pub struct SalsaMemoryDump {
162178
total_fields: usize,

crates/ty_project/src/lib.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use crate::glob::{GlobFilterCheckMode, IncludeResult};
22
use crate::metadata::options::{OptionDiagnostic, ToSettingsError};
33
use crate::walk::{ProjectFilesFilter, ProjectFilesWalker};
4-
pub use db::{Db, ProjectDatabase, SalsaMemoryDump};
4+
pub use db::{CheckMode, Db, ProjectDatabase, SalsaMemoryDump};
55
use files::{Index, Indexed, IndexedFiles};
66
use metadata::settings::Settings;
77
pub use metadata::{ProjectMetadata, ProjectMetadataError};
@@ -214,6 +214,7 @@ impl Project {
214214
pub(crate) fn check(
215215
self,
216216
db: &ProjectDatabase,
217+
mode: CheckMode,
217218
mut reporter: AssertUnwindSafe<&mut dyn Reporter>,
218219
) -> Vec<Diagnostic> {
219220
let project_span = tracing::debug_span!("Project::check");
@@ -228,7 +229,11 @@ impl Project {
228229
.map(OptionDiagnostic::to_diagnostic),
229230
);
230231

231-
let files = ProjectFiles::new(db, self);
232+
let files = match mode {
233+
CheckMode::OpenFiles => ProjectFiles::new(db, self),
234+
// TODO: Consider open virtual files as well
235+
CheckMode::AllFiles => ProjectFiles::Indexed(self.files(db)),
236+
};
232237
reporter.set_files(files.len());
233238

234239
diagnostics.extend(

crates/ty_server/src/server.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,7 @@ impl Server {
173173
diagnostic_provider: Some(DiagnosticServerCapabilities::Options(DiagnosticOptions {
174174
identifier: Some(crate::DIAGNOSTIC_NAME.into()),
175175
inter_file_dependencies: true,
176+
workspace_diagnostics: true,
176177
..Default::default()
177178
})),
178179
text_document_sync: Some(TextDocumentSyncCapability::Options(

crates/ty_server/src/server/api.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ pub(super) fn request(req: server::Request) -> Task {
3333
>(
3434
req, BackgroundSchedule::Worker
3535
),
36+
requests::WorkspaceDiagnosticRequestHandler::METHOD => background_request_task::<
37+
requests::WorkspaceDiagnosticRequestHandler,
38+
>(
39+
req, BackgroundSchedule::Worker
40+
),
3641
requests::GotoTypeDefinitionRequestHandler::METHOD => background_document_request_task::<
3742
requests::GotoTypeDefinitionRequestHandler,
3843
>(
@@ -135,7 +140,6 @@ where
135140
}))
136141
}
137142

138-
#[expect(dead_code)]
139143
fn background_request_task<R: traits::BackgroundRequestHandler>(
140144
req: server::Request,
141145
schedule: BackgroundSchedule,

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ pub(super) fn compute_diagnostics(
166166

167167
/// Converts the tool specific [`Diagnostic`][ruff_db::diagnostic::Diagnostic] to an LSP
168168
/// [`Diagnostic`].
169-
fn to_lsp_diagnostic(
169+
pub(super) fn to_lsp_diagnostic(
170170
db: &dyn Db,
171171
diagnostic: &ruff_db::diagnostic::Diagnostic,
172172
encoding: PositionEncoding,

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,12 @@ impl SyncNotificationHandler for DidCloseTextDocumentHandler {
4141
);
4242
}
4343

44-
clear_diagnostics(&key, client);
44+
if !session.global_settings().diagnostic_mode().is_workspace() {
45+
// The server needs to clear the diagnostics regardless of whether the client supports
46+
// pull diagnostics or not. This is because the client only has the capability to fetch
47+
// the diagnostics but does not automatically clear them when a document is closed.
48+
clear_diagnostics(&key, client);
49+
}
4550

4651
Ok(())
4752
}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,12 @@ mod goto_type_definition;
44
mod hover;
55
mod inlay_hints;
66
mod shutdown;
7+
mod workspace_diagnostic;
78

89
pub(super) use completion::CompletionRequestHandler;
910
pub(super) use diagnostic::DocumentDiagnosticRequestHandler;
1011
pub(super) use goto_type_definition::GotoTypeDefinitionRequestHandler;
1112
pub(super) use hover::HoverRequestHandler;
1213
pub(super) use inlay_hints::InlayHintRequestHandler;
1314
pub(super) use shutdown::ShutdownHandler;
15+
pub(super) use workspace_diagnostic::WorkspaceDiagnosticRequestHandler;
Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
use lsp_types::request::WorkspaceDiagnosticRequest;
2+
use lsp_types::{
3+
FullDocumentDiagnosticReport, Url, WorkspaceDiagnosticParams, WorkspaceDiagnosticReport,
4+
WorkspaceDiagnosticReportResult, WorkspaceDocumentDiagnosticReport,
5+
WorkspaceFullDocumentDiagnosticReport,
6+
};
7+
use rustc_hash::FxHashMap;
8+
use ty_project::CheckMode;
9+
10+
use crate::server::Result;
11+
use crate::server::api::diagnostics::to_lsp_diagnostic;
12+
use crate::server::api::traits::{
13+
BackgroundRequestHandler, RequestHandler, RetriableRequestHandler,
14+
};
15+
use crate::session::WorkspaceSnapshot;
16+
use crate::session::client::Client;
17+
use crate::system::file_to_url;
18+
19+
pub(crate) struct WorkspaceDiagnosticRequestHandler;
20+
21+
impl RequestHandler for WorkspaceDiagnosticRequestHandler {
22+
type RequestType = WorkspaceDiagnosticRequest;
23+
}
24+
25+
impl BackgroundRequestHandler for WorkspaceDiagnosticRequestHandler {
26+
fn run(
27+
snapshot: WorkspaceSnapshot,
28+
_client: &Client,
29+
_params: WorkspaceDiagnosticParams,
30+
) -> Result<WorkspaceDiagnosticReportResult> {
31+
let index = snapshot.index();
32+
33+
if !index.global_settings().diagnostic_mode().is_workspace() {
34+
tracing::debug!("Workspace diagnostics is disabled; returning empty report");
35+
return Ok(WorkspaceDiagnosticReportResult::Report(
36+
WorkspaceDiagnosticReport { items: vec![] },
37+
));
38+
}
39+
40+
let mut items = Vec::new();
41+
42+
for db in snapshot.projects() {
43+
let diagnostics = db.check_with_mode(CheckMode::AllFiles);
44+
45+
// Group diagnostics by URL
46+
let mut diagnostics_by_url: FxHashMap<Url, Vec<_>> = FxHashMap::default();
47+
48+
for diagnostic in diagnostics {
49+
if let Some(span) = diagnostic.primary_span() {
50+
let file = span.expect_ty_file();
51+
let Some(url) = file_to_url(db, file) else {
52+
tracing::debug!("Failed to convert file to URL at {}", file.path(db));
53+
continue;
54+
};
55+
diagnostics_by_url.entry(url).or_default().push(diagnostic);
56+
}
57+
}
58+
59+
items.reserve(diagnostics_by_url.len());
60+
61+
// Convert to workspace diagnostic report format
62+
for (url, file_diagnostics) in diagnostics_by_url {
63+
let version = index
64+
.key_from_url(url.clone())
65+
.ok()
66+
.and_then(|key| index.make_document_ref(&key))
67+
.map(|doc| i64::from(doc.version()));
68+
69+
// Convert diagnostics to LSP format
70+
let lsp_diagnostics = file_diagnostics
71+
.into_iter()
72+
.map(|diagnostic| {
73+
to_lsp_diagnostic(db, &diagnostic, snapshot.position_encoding())
74+
})
75+
.collect::<Vec<_>>();
76+
77+
items.push(WorkspaceDocumentDiagnosticReport::Full(
78+
WorkspaceFullDocumentDiagnosticReport {
79+
uri: url,
80+
version,
81+
full_document_diagnostic_report: FullDocumentDiagnosticReport {
82+
// TODO: We don't implement result ID caching yet
83+
result_id: None,
84+
items: lsp_diagnostics,
85+
},
86+
},
87+
));
88+
}
89+
}
90+
91+
Ok(WorkspaceDiagnosticReportResult::Report(
92+
WorkspaceDiagnosticReport { items },
93+
))
94+
}
95+
}
96+
97+
impl RetriableRequestHandler for WorkspaceDiagnosticRequestHandler {
98+
fn salsa_cancellation_error() -> lsp_server::ResponseError {
99+
lsp_server::ResponseError {
100+
code: lsp_server::ErrorCode::ServerCancelled as i32,
101+
message: "server cancelled the request".to_owned(),
102+
data: serde_json::to_value(lsp_types::DiagnosticServerCancellationData {
103+
retrigger_request: true,
104+
})
105+
.ok(),
106+
}
107+
}
108+
}

crates/ty_server/src/session.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -383,6 +383,10 @@ impl Session {
383383
pub(crate) fn client_capabilities(&self) -> &ResolvedClientCapabilities {
384384
&self.resolved_client_capabilities
385385
}
386+
387+
pub(crate) fn global_settings(&self) -> Arc<ClientSettings> {
388+
self.index().global_settings()
389+
}
386390
}
387391

388392
/// A guard that holds the only reference to the index and allows modifying it.
@@ -469,7 +473,6 @@ pub(crate) struct WorkspaceSnapshot {
469473
position_encoding: PositionEncoding,
470474
}
471475

472-
#[expect(dead_code)]
473476
impl WorkspaceSnapshot {
474477
pub(crate) fn projects(&self) -> &[ProjectDatabase] {
475478
&self.projects

crates/ty_server/src/session/options.rs

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,26 @@ pub(crate) struct ClientOptions {
4646
/// Settings under the `python.*` namespace in VS Code that are useful for the ty language
4747
/// server.
4848
python: Option<Python>,
49+
/// Diagnostic mode for the language server.
50+
diagnostic_mode: Option<DiagnosticMode>,
51+
}
52+
53+
/// Diagnostic mode for the language server.
54+
#[derive(Clone, Copy, Debug, Default, Deserialize)]
55+
#[cfg_attr(test, derive(PartialEq, Eq))]
56+
#[serde(rename_all = "camelCase")]
57+
pub(crate) enum DiagnosticMode {
58+
/// Check only currently open files.
59+
#[default]
60+
OpenFilesOnly,
61+
/// Check all files in the workspace.
62+
Workspace,
63+
}
64+
65+
impl DiagnosticMode {
66+
pub(crate) fn is_workspace(self) -> bool {
67+
matches!(self, DiagnosticMode::Workspace)
68+
}
4969
}
5070

5171
impl ClientOptions {
@@ -57,6 +77,7 @@ impl ClientOptions {
5777
.and_then(|python| python.ty)
5878
.and_then(|ty| ty.disable_language_services)
5979
.unwrap_or_default(),
80+
diagnostic_mode: self.diagnostic_mode.unwrap_or_default(),
6081
}
6182
}
6283
}

0 commit comments

Comments
 (0)