Pull diagnostics fixes (#32242)
Follow-up of https://github.com/zed-industries/zed/pull/19230 * starts to send `result_id` in pull requests to allow servers to reply with non-full results * fixes a bug where disk-based diagnostics were offset after pulling the diagnostics * fixes a bug due to which pull diagnostics could not be disabled * uses better names and comments for the workspace pull diagnostics part Release Notes: - N/A
This commit is contained in:
Kirill Bulatov
GitHub
parent
508b604b67
commit
380d8c5662
15 changed files with 272 additions and 109 deletions
|
|
@ -125,7 +125,7 @@ use markdown::Markdown;
|
|||
use mouse_context_menu::MouseContextMenu;
|
||||
use persistence::DB;
|
||||
use project::{
|
||||
BreakpointWithPosition, CompletionResponse, LspPullDiagnostics, ProjectPath,
|
||||
BreakpointWithPosition, CompletionResponse, LspPullDiagnostics, ProjectPath, PulledDiagnostics,
|
||||
debugger::{
|
||||
breakpoint_store::{
|
||||
BreakpointEditAction, BreakpointSessionState, BreakpointState, BreakpointStore,
|
||||
|
|
@ -1700,7 +1700,7 @@ impl Editor {
|
|||
}
|
||||
editor.pull_diagnostics(window, cx);
|
||||
}
|
||||
project::Event::RefreshDocumentsDiagnostics => {
|
||||
project::Event::PullWorkspaceDiagnostics => {
|
||||
editor.pull_diagnostics(window, cx);
|
||||
}
|
||||
project::Event::SnippetEdit(id, snippet_edits) => {
|
||||
|
|
@ -15966,11 +15966,13 @@ impl Editor {
|
|||
|
||||
fn pull_diagnostics(&mut self, window: &Window, cx: &mut Context<Self>) -> Option<()> {
|
||||
let project = self.project.as_ref()?.downgrade();
|
||||
let debounce = Duration::from_millis(
|
||||
ProjectSettings::get_global(cx)
|
||||
.diagnostics
|
||||
.lsp_pull_diagnostics_debounce_ms?,
|
||||
);
|
||||
let pull_diagnostics_settings = ProjectSettings::get_global(cx)
|
||||
.diagnostics
|
||||
.lsp_pull_diagnostics;
|
||||
if !pull_diagnostics_settings.enabled {
|
||||
return None;
|
||||
}
|
||||
let debounce = Duration::from_millis(pull_diagnostics_settings.debounce_ms);
|
||||
let buffers = self.buffer.read(cx).all_buffers();
|
||||
|
||||
self.pull_diagnostics_task = cx.spawn_in(window, async move |editor, cx| {
|
||||
|
|
@ -18733,13 +18735,16 @@ impl Editor {
|
|||
}
|
||||
if let Some(project) = self.project.as_ref() {
|
||||
project.update(cx, |project, cx| {
|
||||
// Diagnostics are not local: an edit within one file (`pub mod foo()` -> `pub mod bar()`), may cause errors in another files with `foo()`.
|
||||
// Hence, emit a project-wide event to pull for every buffer's diagnostics that has an open editor.
|
||||
if edited_buffer
|
||||
.as_ref()
|
||||
.is_some_and(|buffer| buffer.read(cx).file().is_some())
|
||||
{
|
||||
cx.emit(project::Event::RefreshDocumentsDiagnostics);
|
||||
// Diagnostics are not local: an edit within one file (`pub mod foo()` -> `pub mod bar()`), may cause errors in another files with `foo()`.
|
||||
// Hence, emit a project-wide event to pull for every buffer's diagnostics that has an open editor.
|
||||
// TODO: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#diagnostic_refresh explains the flow how
|
||||
// diagnostics should be pulled: instead of pulling every open editor's buffer's diagnostics (which happens effectively due to emitting this event),
|
||||
// we should only pull for the current buffer's diagnostics and get the rest via the workspace diagnostics LSP request — this is not implemented yet.
|
||||
cx.emit(project::Event::PullWorkspaceDiagnostics);
|
||||
}
|
||||
|
||||
if let Some(buffer) = edited_buffer {
|
||||
|
|
@ -20990,7 +20995,7 @@ impl SemanticsProvider for Entity<Project> {
|
|||
let LspPullDiagnostics::Response {
|
||||
server_id,
|
||||
uri,
|
||||
diagnostics: project::PulledDiagnostics::Changed { diagnostics, .. },
|
||||
diagnostics,
|
||||
} = diagnostics_set
|
||||
else {
|
||||
continue;
|
||||
|
|
@ -21001,25 +21006,49 @@ impl SemanticsProvider for Entity<Project> {
|
|||
.as_ref()
|
||||
.map(|adapter| adapter.disk_based_diagnostic_sources.as_slice())
|
||||
.unwrap_or(&[]);
|
||||
lsp_store
|
||||
.merge_diagnostics(
|
||||
server_id,
|
||||
lsp::PublishDiagnosticsParams {
|
||||
uri: uri.clone(),
|
||||
diagnostics,
|
||||
version: None,
|
||||
},
|
||||
DiagnosticSourceKind::Pulled,
|
||||
disk_based_sources,
|
||||
|old_diagnostic, _| match old_diagnostic.source_kind {
|
||||
DiagnosticSourceKind::Pulled => false,
|
||||
DiagnosticSourceKind::Other | DiagnosticSourceKind::Pushed => {
|
||||
true
|
||||
}
|
||||
},
|
||||
cx,
|
||||
)
|
||||
.log_err();
|
||||
match diagnostics {
|
||||
PulledDiagnostics::Unchanged { result_id } => {
|
||||
lsp_store
|
||||
.merge_diagnostics(
|
||||
server_id,
|
||||
lsp::PublishDiagnosticsParams {
|
||||
uri: uri.clone(),
|
||||
diagnostics: Vec::new(),
|
||||
version: None,
|
||||
},
|
||||
Some(result_id),
|
||||
DiagnosticSourceKind::Pulled,
|
||||
disk_based_sources,
|
||||
|_, _| true,
|
||||
cx,
|
||||
)
|
||||
.log_err();
|
||||
}
|
||||
PulledDiagnostics::Changed {
|
||||
diagnostics,
|
||||
result_id,
|
||||
} => {
|
||||
lsp_store
|
||||
.merge_diagnostics(
|
||||
server_id,
|
||||
lsp::PublishDiagnosticsParams {
|
||||
uri: uri.clone(),
|
||||
diagnostics,
|
||||
version: None,
|
||||
},
|
||||
result_id,
|
||||
DiagnosticSourceKind::Pulled,
|
||||
disk_based_sources,
|
||||
|old_diagnostic, _| match old_diagnostic.source_kind {
|
||||
DiagnosticSourceKind::Pulled => false,
|
||||
DiagnosticSourceKind::Other
|
||||
| DiagnosticSourceKind::Pushed => true,
|
||||
},
|
||||
cx,
|
||||
)
|
||||
.log_err();
|
||||
}
|
||||
}
|
||||
}
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -13940,6 +13940,7 @@ async fn go_to_prev_overlapping_diagnostic(executor: BackgroundExecutor, cx: &mu
|
|||
},
|
||||
],
|
||||
},
|
||||
None,
|
||||
DiagnosticSourceKind::Pushed,
|
||||
&[],
|
||||
cx,
|
||||
|
|
@ -21854,7 +21855,7 @@ fn assert_hunk_revert(
|
|||
assert_eq!(actual_hunk_statuses_before, expected_hunk_statuses_before);
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
#[gpui::test(iterations = 10)]
|
||||
async fn test_pulling_diagnostics(cx: &mut TestAppContext) {
|
||||
init_test(cx, |_| {});
|
||||
|
||||
|
|
@ -21912,7 +21913,8 @@ async fn test_pulling_diagnostics(cx: &mut TestAppContext) {
|
|||
let fake_server = fake_servers.next().await.unwrap();
|
||||
let mut first_request = fake_server
|
||||
.set_request_handler::<lsp::request::DocumentDiagnosticRequest, _, _>(move |params, _| {
|
||||
counter.fetch_add(1, atomic::Ordering::Release);
|
||||
let new_result_id = counter.fetch_add(1, atomic::Ordering::Release) + 1;
|
||||
let result_id = Some(new_result_id.to_string());
|
||||
assert_eq!(
|
||||
params.text_document.uri,
|
||||
lsp::Url::from_file_path(path!("/a/first.rs")).unwrap()
|
||||
|
|
@ -21923,13 +21925,27 @@ async fn test_pulling_diagnostics(cx: &mut TestAppContext) {
|
|||
related_documents: None,
|
||||
full_document_diagnostic_report: lsp::FullDocumentDiagnosticReport {
|
||||
items: Vec::new(),
|
||||
result_id: None,
|
||||
result_id,
|
||||
},
|
||||
}),
|
||||
))
|
||||
}
|
||||
});
|
||||
|
||||
let ensure_result_id = |expected: Option<String>, cx: &mut TestAppContext| {
|
||||
editor.update(cx, |editor, cx| {
|
||||
let buffer_result_id = editor
|
||||
.buffer()
|
||||
.read(cx)
|
||||
.as_singleton()
|
||||
.expect("created a singleton buffer")
|
||||
.read(cx)
|
||||
.result_id();
|
||||
assert_eq!(expected, buffer_result_id);
|
||||
});
|
||||
};
|
||||
|
||||
ensure_result_id(None, cx);
|
||||
cx.executor().advance_clock(Duration::from_millis(60));
|
||||
cx.executor().run_until_parked();
|
||||
assert_eq!(
|
||||
|
|
@ -21941,6 +21957,7 @@ async fn test_pulling_diagnostics(cx: &mut TestAppContext) {
|
|||
.next()
|
||||
.await
|
||||
.expect("should have sent the first diagnostics pull request");
|
||||
ensure_result_id(Some("1".to_string()), cx);
|
||||
|
||||
// Editing should trigger diagnostics
|
||||
editor.update_in(cx, |editor, window, cx| {
|
||||
|
|
@ -21953,6 +21970,7 @@ async fn test_pulling_diagnostics(cx: &mut TestAppContext) {
|
|||
2,
|
||||
"Editing should trigger diagnostic request"
|
||||
);
|
||||
ensure_result_id(Some("2".to_string()), cx);
|
||||
|
||||
// Moving cursor should not trigger diagnostic request
|
||||
editor.update_in(cx, |editor, window, cx| {
|
||||
|
|
@ -21967,6 +21985,7 @@ async fn test_pulling_diagnostics(cx: &mut TestAppContext) {
|
|||
2,
|
||||
"Cursor movement should not trigger diagnostic request"
|
||||
);
|
||||
ensure_result_id(Some("2".to_string()), cx);
|
||||
|
||||
// Multiple rapid edits should be debounced
|
||||
for _ in 0..5 {
|
||||
|
|
@ -21980,7 +21999,7 @@ async fn test_pulling_diagnostics(cx: &mut TestAppContext) {
|
|||
let final_requests = diagnostic_requests.load(atomic::Ordering::Acquire);
|
||||
assert!(
|
||||
final_requests <= 4,
|
||||
"Multiple rapid edits should be debounced (got {} requests)",
|
||||
final_requests
|
||||
"Multiple rapid edits should be debounced (got {final_requests} requests)",
|
||||
);
|
||||
ensure_result_id(Some(final_requests.to_string()), cx);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue