From 691b3ca238f6eb68e3fb42b94321855801b03f44 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Tue, 29 Jul 2025 09:51:58 +0300 Subject: [PATCH] Cache LSP code lens requests (#35207) --- crates/editor/src/editor.rs | 6 +- crates/editor/src/editor_tests.rs | 108 ++++++++++++-- crates/editor/src/lsp_colors.rs | 6 +- crates/project/src/lsp_store.rs | 233 ++++++++++++++++++++++-------- crates/project/src/project.rs | 27 ++-- 5 files changed, 291 insertions(+), 89 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 6bbd1a409d..3c877873a0 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -21834,11 +21834,11 @@ impl CodeActionProvider for Entity { cx: &mut App, ) -> Task>> { self.update(cx, |project, cx| { - let code_lens = project.code_lens(buffer, range.clone(), cx); + let code_lens_actions = project.code_lens_actions(buffer, range.clone(), cx); let code_actions = project.code_actions(buffer, range, None, cx); cx.background_spawn(async move { - let (code_lens, code_actions) = join(code_lens, code_actions).await; - Ok(code_lens + let (code_lens_actions, code_actions) = join(code_lens_actions, code_actions).await; + Ok(code_lens_actions .context("code lens fetch")? .into_iter() .chain(code_actions.context("code action fetch")?) diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index a0333bb494..a13708c580 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -10072,8 +10072,14 @@ async fn test_autosave_with_dirty_buffers(cx: &mut TestAppContext) { ); } -#[gpui::test] -async fn test_range_format_during_save(cx: &mut TestAppContext) { +async fn setup_range_format_test( + cx: &mut TestAppContext, +) -> ( + Entity, + Entity, + &mut gpui::VisualTestContext, + lsp::FakeLanguageServer, +) { init_test(cx, |_| {}); let fs = FakeFs::new(cx.executor()); @@ -10088,9 +10094,9 @@ async fn test_range_format_during_save(cx: &mut TestAppContext) { FakeLspAdapter { capabilities: lsp::ServerCapabilities { document_range_formatting_provider: Some(lsp::OneOf::Left(true)), - ..Default::default() + ..lsp::ServerCapabilities::default() }, - ..Default::default() + ..FakeLspAdapter::default() }, ); @@ -10105,14 +10111,22 @@ async fn test_range_format_during_save(cx: &mut TestAppContext) { let (editor, cx) = cx.add_window_view(|window, cx| { build_editor_with_project(project.clone(), buffer, window, cx) }); + + cx.executor().start_waiting(); + let fake_server = fake_servers.next().await.unwrap(); + + (project, editor, cx, fake_server) +} + +#[gpui::test] +async fn test_range_format_on_save_success(cx: &mut TestAppContext) { + let (project, editor, cx, fake_server) = setup_range_format_test(cx).await; + editor.update_in(cx, |editor, window, cx| { editor.set_text("one\ntwo\nthree\n", window, cx) }); assert!(cx.read(|cx| editor.is_dirty(cx))); - cx.executor().start_waiting(); - let fake_server = fake_servers.next().await.unwrap(); - let save = editor .update_in(cx, |editor, window, cx| { editor.save( @@ -10147,13 +10161,18 @@ async fn test_range_format_during_save(cx: &mut TestAppContext) { "one, two\nthree\n" ); assert!(!cx.read(|cx| editor.is_dirty(cx))); +} + +#[gpui::test] +async fn test_range_format_on_save_timeout(cx: &mut TestAppContext) { + let (project, editor, cx, fake_server) = setup_range_format_test(cx).await; editor.update_in(cx, |editor, window, cx| { editor.set_text("one\ntwo\nthree\n", window, cx) }); assert!(cx.read(|cx| editor.is_dirty(cx))); - // Ensure we can still save even if formatting hangs. + // Test that save still works when formatting hangs fake_server.set_request_handler::( move |params, _| async move { assert_eq!( @@ -10185,8 +10204,13 @@ async fn test_range_format_during_save(cx: &mut TestAppContext) { "one\ntwo\nthree\n" ); assert!(!cx.read(|cx| editor.is_dirty(cx))); +} - // For non-dirty buffer, no formatting request should be sent +#[gpui::test] +async fn test_range_format_not_called_for_clean_buffer(cx: &mut TestAppContext) { + let (project, editor, cx, fake_server) = setup_range_format_test(cx).await; + + // Buffer starts clean, no formatting should be requested let save = editor .update_in(cx, |editor, window, cx| { editor.save( @@ -10207,6 +10231,12 @@ async fn test_range_format_during_save(cx: &mut TestAppContext) { .next(); cx.executor().start_waiting(); save.await; + cx.run_until_parked(); +} + +#[gpui::test] +async fn test_range_format_respects_language_tab_size_override(cx: &mut TestAppContext) { + let (project, editor, cx, fake_server) = setup_range_format_test(cx).await; // Set Rust language override and assert overridden tabsize is sent to language server update_test_language_settings(cx, |settings| { @@ -10220,7 +10250,7 @@ async fn test_range_format_during_save(cx: &mut TestAppContext) { }); editor.update_in(cx, |editor, window, cx| { - editor.set_text("somehting_new\n", window, cx) + editor.set_text("something_new\n", window, cx) }); assert!(cx.read(|cx| editor.is_dirty(cx))); let save = editor @@ -21310,16 +21340,32 @@ async fn test_apply_code_lens_actions_with_commands(cx: &mut gpui::TestAppContex }, ); - let (buffer, _handle) = project - .update(cx, |p, cx| { - p.open_local_buffer_with_lsp(path!("/dir/a.ts"), cx) + let editor = workspace + .update(cx, |workspace, window, cx| { + workspace.open_abs_path( + PathBuf::from(path!("/dir/a.ts")), + OpenOptions::default(), + window, + cx, + ) }) + .unwrap() .await + .unwrap() + .downcast::() .unwrap(); cx.executor().run_until_parked(); let fake_server = fake_language_servers.next().await.unwrap(); + let buffer = editor.update(cx, |editor, cx| { + editor + .buffer() + .read(cx) + .as_singleton() + .expect("have opened a single file by path") + }); + let buffer_snapshot = buffer.update(cx, |buffer, _| buffer.snapshot()); let anchor = buffer_snapshot.anchor_at(0, text::Bias::Left); drop(buffer_snapshot); @@ -21377,7 +21423,7 @@ async fn test_apply_code_lens_actions_with_commands(cx: &mut gpui::TestAppContex assert_eq!( actions.len(), 1, - "Should have only one valid action for the 0..0 range" + "Should have only one valid action for the 0..0 range, got: {actions:#?}" ); let action = actions[0].clone(); let apply = project.update(cx, |project, cx| { @@ -21423,7 +21469,7 @@ async fn test_apply_code_lens_actions_with_commands(cx: &mut gpui::TestAppContex .into_iter() .collect(), ), - ..Default::default() + ..lsp::WorkspaceEdit::default() }, }, ) @@ -21446,6 +21492,38 @@ async fn test_apply_code_lens_actions_with_commands(cx: &mut gpui::TestAppContex buffer.undo(cx); assert_eq!(buffer.text(), "a"); }); + + let actions_after_edits = cx + .update_window(*workspace, |_, window, cx| { + project.code_actions(&buffer, anchor..anchor, window, cx) + }) + .unwrap() + .await + .unwrap(); + assert_eq!( + actions, actions_after_edits, + "For the same selection, same code lens actions should be returned" + ); + + let _responses = + fake_server.set_request_handler::(|_, _| async move { + panic!("No more code lens requests are expected"); + }); + editor.update_in(cx, |editor, window, cx| { + editor.select_all(&SelectAll, window, cx); + }); + cx.executor().run_until_parked(); + let new_actions = cx + .update_window(*workspace, |_, window, cx| { + project.code_actions(&buffer, anchor..anchor, window, cx) + }) + .unwrap() + .await + .unwrap(); + assert_eq!( + actions, new_actions, + "Code lens are queried for the same range and should get the same set back, but without additional LSP queries now" + ); } #[gpui::test] diff --git a/crates/editor/src/lsp_colors.rs b/crates/editor/src/lsp_colors.rs index ce07dd43fe..08cf9078f2 100644 --- a/crates/editor/src/lsp_colors.rs +++ b/crates/editor/src/lsp_colors.rs @@ -6,7 +6,7 @@ use gpui::{Hsla, Rgba}; use itertools::Itertools; use language::point_from_lsp; use multi_buffer::Anchor; -use project::{DocumentColor, lsp_store::ColorFetchStrategy}; +use project::{DocumentColor, lsp_store::LspFetchStrategy}; use settings::Settings as _; use text::{Bias, BufferId, OffsetRangeExt as _}; use ui::{App, Context, Window}; @@ -180,9 +180,9 @@ impl Editor { .filter_map(|buffer| { let buffer_id = buffer.read(cx).remote_id(); let fetch_strategy = if ignore_cache { - ColorFetchStrategy::IgnoreCache + LspFetchStrategy::IgnoreCache } else { - ColorFetchStrategy::UseCache { + LspFetchStrategy::UseCache { known_cache_version: self.colors.as_ref().and_then(|colors| { Some(colors.buffer_colors.get(&buffer_id)?.cache_version_used) }), diff --git a/crates/project/src/lsp_store.rs b/crates/project/src/lsp_store.rs index 3645839271..defe056dd8 100644 --- a/crates/project/src/lsp_store.rs +++ b/crates/project/src/lsp_store.rs @@ -3556,7 +3556,8 @@ pub struct LspStore { _maintain_buffer_languages: Task<()>, diagnostic_summaries: HashMap, HashMap>>, - lsp_data: HashMap, + lsp_document_colors: HashMap, + lsp_code_lens: HashMap, } #[derive(Debug, Default, Clone)] @@ -3566,6 +3567,7 @@ pub struct DocumentColors { } type DocumentColorTask = Shared>>>; +type CodeLensTask = Shared, Arc>>>; #[derive(Debug, Default)] struct DocumentColorData { @@ -3575,8 +3577,15 @@ struct DocumentColorData { colors_update: Option<(Global, DocumentColorTask)>, } +#[derive(Debug, Default)] +struct CodeLensData { + lens_for_version: Global, + lens: HashMap>, + update: Option<(Global, CodeLensTask)>, +} + #[derive(Debug, PartialEq, Eq, Clone, Copy)] -pub enum ColorFetchStrategy { +pub enum LspFetchStrategy { IgnoreCache, UseCache { known_cache_version: Option }, } @@ -3809,7 +3818,8 @@ impl LspStore { language_server_statuses: Default::default(), nonce: StdRng::from_entropy().r#gen(), diagnostic_summaries: HashMap::default(), - lsp_data: HashMap::default(), + lsp_document_colors: HashMap::default(), + lsp_code_lens: HashMap::default(), active_entry: None, _maintain_workspace_config, _maintain_buffer_languages: Self::maintain_buffer_languages(languages, cx), @@ -3866,7 +3876,8 @@ impl LspStore { language_server_statuses: Default::default(), nonce: StdRng::from_entropy().r#gen(), diagnostic_summaries: HashMap::default(), - lsp_data: HashMap::default(), + lsp_document_colors: HashMap::default(), + lsp_code_lens: HashMap::default(), active_entry: None, toolchain_store, _maintain_workspace_config, @@ -4167,7 +4178,8 @@ impl LspStore { *refcount }; if refcount == 0 { - lsp_store.lsp_data.remove(&buffer_id); + lsp_store.lsp_document_colors.remove(&buffer_id); + lsp_store.lsp_code_lens.remove(&buffer_id); let local = lsp_store.as_local_mut().unwrap(); local.registered_buffers.remove(&buffer_id); local.buffers_opened_in_servers.remove(&buffer_id); @@ -5707,69 +5719,168 @@ impl LspStore { } } - pub fn code_lens( + pub fn code_lens_actions( &mut self, - buffer_handle: &Entity, + buffer: &Entity, cx: &mut Context, - ) -> Task>> { + ) -> CodeLensTask { + let version_queried_for = buffer.read(cx).version(); + let buffer_id = buffer.read(cx).remote_id(); + + if let Some(cached_data) = self.lsp_code_lens.get(&buffer_id) { + if !version_queried_for.changed_since(&cached_data.lens_for_version) { + let has_different_servers = self.as_local().is_some_and(|local| { + local + .buffers_opened_in_servers + .get(&buffer_id) + .cloned() + .unwrap_or_default() + != cached_data.lens.keys().copied().collect() + }); + if !has_different_servers { + return Task::ready(Ok(cached_data.lens.values().flatten().cloned().collect())) + .shared(); + } + } + } + + let lsp_data = self.lsp_code_lens.entry(buffer_id).or_default(); + if let Some((updating_for, running_update)) = &lsp_data.update { + if !version_queried_for.changed_since(&updating_for) { + return running_update.clone(); + } + } + let buffer = buffer.clone(); + let query_version_queried_for = version_queried_for.clone(); + let new_task = cx + .spawn(async move |lsp_store, cx| { + cx.background_executor() + .timer(Duration::from_millis(30)) + .await; + let fetched_lens = lsp_store + .update(cx, |lsp_store, cx| lsp_store.fetch_code_lens(&buffer, cx)) + .map_err(Arc::new)? + .await + .context("fetching code lens") + .map_err(Arc::new); + let fetched_lens = match fetched_lens { + Ok(fetched_lens) => fetched_lens, + Err(e) => { + lsp_store + .update(cx, |lsp_store, _| { + lsp_store.lsp_code_lens.entry(buffer_id).or_default().update = None; + }) + .ok(); + return Err(e); + } + }; + + lsp_store + .update(cx, |lsp_store, _| { + let lsp_data = lsp_store.lsp_code_lens.entry(buffer_id).or_default(); + if lsp_data.lens_for_version == query_version_queried_for { + lsp_data.lens.extend(fetched_lens.clone()); + } else if !lsp_data + .lens_for_version + .changed_since(&query_version_queried_for) + { + lsp_data.lens_for_version = query_version_queried_for; + lsp_data.lens = fetched_lens.clone(); + } + lsp_data.update = None; + lsp_data.lens.values().flatten().cloned().collect() + }) + .map_err(Arc::new) + }) + .shared(); + lsp_data.update = Some((version_queried_for, new_task.clone())); + new_task + } + + fn fetch_code_lens( + &mut self, + buffer: &Entity, + cx: &mut Context, + ) -> Task>>> { if let Some((upstream_client, project_id)) = self.upstream_client() { let request_task = upstream_client.request(proto::MultiLspQuery { - buffer_id: buffer_handle.read(cx).remote_id().into(), - version: serialize_version(&buffer_handle.read(cx).version()), + buffer_id: buffer.read(cx).remote_id().into(), + version: serialize_version(&buffer.read(cx).version()), project_id, strategy: Some(proto::multi_lsp_query::Strategy::All( proto::AllLanguageServers {}, )), request: Some(proto::multi_lsp_query::Request::GetCodeLens( - GetCodeLens.to_proto(project_id, buffer_handle.read(cx)), + GetCodeLens.to_proto(project_id, buffer.read(cx)), )), }); - let buffer = buffer_handle.clone(); - cx.spawn(async move |weak_project, cx| { - let Some(project) = weak_project.upgrade() else { - return Ok(Vec::new()); + let buffer = buffer.clone(); + cx.spawn(async move |weak_lsp_store, cx| { + let Some(lsp_store) = weak_lsp_store.upgrade() else { + return Ok(HashMap::default()); }; let responses = request_task.await?.responses; - let code_lens = join_all( + let code_lens_actions = join_all( responses .into_iter() - .filter_map(|lsp_response| match lsp_response.response? { - proto::lsp_response::Response::GetCodeLensResponse(response) => { - Some(response) - } - unexpected => { - debug_panic!("Unexpected response: {unexpected:?}"); - None - } + .filter_map(|lsp_response| { + let response = match lsp_response.response? { + proto::lsp_response::Response::GetCodeLensResponse(response) => { + Some(response) + } + unexpected => { + debug_panic!("Unexpected response: {unexpected:?}"); + None + } + }?; + let server_id = LanguageServerId::from_proto(lsp_response.server_id); + Some((server_id, response)) }) - .map(|code_lens_response| { - GetCodeLens.response_from_proto( - code_lens_response, - project.clone(), - buffer.clone(), - cx.clone(), - ) + .map(|(server_id, code_lens_response)| { + let lsp_store = lsp_store.clone(); + let buffer = buffer.clone(); + let cx = cx.clone(); + async move { + ( + server_id, + GetCodeLens + .response_from_proto( + code_lens_response, + lsp_store, + buffer, + cx, + ) + .await, + ) + } }), ) .await; - Ok(code_lens + let mut has_errors = false; + let code_lens_actions = code_lens_actions .into_iter() - .collect::>>>()? - .into_iter() - .flatten() - .collect()) + .filter_map(|(server_id, code_lens)| match code_lens { + Ok(code_lens) => Some((server_id, code_lens)), + Err(e) => { + has_errors = true; + log::error!("{e:#}"); + None + } + }) + .collect::>(); + anyhow::ensure!( + !has_errors || !code_lens_actions.is_empty(), + "Failed to fetch code lens" + ); + Ok(code_lens_actions) }) } else { - let code_lens_task = - self.request_multiple_lsp_locally(buffer_handle, None::, GetCodeLens, cx); - cx.spawn(async move |_, _| { - Ok(code_lens_task - .await - .into_iter() - .flat_map(|(_, code_lens)| code_lens) - .collect()) - }) + let code_lens_actions_task = + self.request_multiple_lsp_locally(buffer, None::, GetCodeLens, cx); + cx.background_spawn( + async move { Ok(code_lens_actions_task.await.into_iter().collect()) }, + ) } } @@ -6602,7 +6713,7 @@ impl LspStore { pub fn document_colors( &mut self, - fetch_strategy: ColorFetchStrategy, + fetch_strategy: LspFetchStrategy, buffer: Entity, cx: &mut Context, ) -> Option { @@ -6610,11 +6721,11 @@ impl LspStore { let buffer_id = buffer.read(cx).remote_id(); match fetch_strategy { - ColorFetchStrategy::IgnoreCache => {} - ColorFetchStrategy::UseCache { + LspFetchStrategy::IgnoreCache => {} + LspFetchStrategy::UseCache { known_cache_version, } => { - if let Some(cached_data) = self.lsp_data.get(&buffer_id) { + if let Some(cached_data) = self.lsp_document_colors.get(&buffer_id) { if !version_queried_for.changed_since(&cached_data.colors_for_version) { let has_different_servers = self.as_local().is_some_and(|local| { local @@ -6647,7 +6758,7 @@ impl LspStore { } } - let lsp_data = self.lsp_data.entry(buffer_id).or_default(); + let lsp_data = self.lsp_document_colors.entry(buffer_id).or_default(); if let Some((updating_for, running_update)) = &lsp_data.colors_update { if !version_queried_for.changed_since(&updating_for) { return Some(running_update.clone()); @@ -6661,14 +6772,14 @@ impl LspStore { .await; let fetched_colors = lsp_store .update(cx, |lsp_store, cx| { - lsp_store.fetch_document_colors_for_buffer(buffer.clone(), cx) + lsp_store.fetch_document_colors_for_buffer(&buffer, cx) })? .await .context("fetching document colors") .map_err(Arc::new); let fetched_colors = match fetched_colors { Ok(fetched_colors) => { - if fetch_strategy != ColorFetchStrategy::IgnoreCache + if fetch_strategy != LspFetchStrategy::IgnoreCache && Some(true) == buffer .update(cx, |buffer, _| { @@ -6684,7 +6795,7 @@ impl LspStore { lsp_store .update(cx, |lsp_store, _| { lsp_store - .lsp_data + .lsp_document_colors .entry(buffer_id) .or_default() .colors_update = None; @@ -6696,7 +6807,7 @@ impl LspStore { lsp_store .update(cx, |lsp_store, _| { - let lsp_data = lsp_store.lsp_data.entry(buffer_id).or_default(); + let lsp_data = lsp_store.lsp_document_colors.entry(buffer_id).or_default(); if lsp_data.colors_for_version == query_version_queried_for { lsp_data.colors.extend(fetched_colors.clone()); @@ -6730,7 +6841,7 @@ impl LspStore { fn fetch_document_colors_for_buffer( &mut self, - buffer: Entity, + buffer: &Entity, cx: &mut Context, ) -> Task>>> { if let Some((client, project_id)) = self.upstream_client() { @@ -6745,6 +6856,7 @@ impl LspStore { GetDocumentColor {}.to_proto(project_id, buffer.read(cx)), )), }); + let buffer = buffer.clone(); cx.spawn(async move |project, cx| { let Some(project) = project.upgrade() else { return Ok(HashMap::default()); @@ -6790,7 +6902,7 @@ impl LspStore { }) } else { let document_colors_task = - self.request_multiple_lsp_locally(&buffer, None::, GetDocumentColor, cx); + self.request_multiple_lsp_locally(buffer, None::, GetDocumentColor, cx); cx.spawn(async move |_, _| { Ok(document_colors_task .await @@ -11283,9 +11395,12 @@ impl LspStore { } fn cleanup_lsp_data(&mut self, for_server: LanguageServerId) { - for buffer_lsp_data in self.lsp_data.values_mut() { - buffer_lsp_data.colors.remove(&for_server); - buffer_lsp_data.cache_version += 1; + for buffer_colors in self.lsp_document_colors.values_mut() { + buffer_colors.colors.remove(&for_server); + buffer_colors.cache_version += 1; + } + for buffer_lens in self.lsp_code_lens.values_mut() { + buffer_lens.lens.remove(&for_server); } if let Some(local) = self.as_local_mut() { local.buffer_pull_diagnostics_result_ids.remove(&for_server); diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index f9c59d2e95..a4e76ed475 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -113,7 +113,7 @@ use std::{ use task_store::TaskStore; use terminals::Terminals; -use text::{Anchor, BufferId, Point}; +use text::{Anchor, BufferId, OffsetRangeExt, Point}; use toolchain_store::EmptyToolchainStore; use util::{ ResultExt as _, @@ -590,7 +590,7 @@ pub(crate) struct CoreCompletion { } /// A code action provided by a language server. -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq)] pub struct CodeAction { /// The id of the language server that produced this code action. pub server_id: LanguageServerId, @@ -604,7 +604,7 @@ pub struct CodeAction { } /// An action sent back by a language server. -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq)] pub enum LspAction { /// An action with the full data, may have a command or may not. /// May require resolving. @@ -3607,20 +3607,29 @@ impl Project { }) } - pub fn code_lens( + pub fn code_lens_actions( &mut self, - buffer_handle: &Entity, + buffer: &Entity, range: Range, cx: &mut Context, ) -> Task>> { - let snapshot = buffer_handle.read(cx).snapshot(); - let range = snapshot.anchor_before(range.start)..snapshot.anchor_after(range.end); + let snapshot = buffer.read(cx).snapshot(); + let range = range.clone().to_owned().to_point(&snapshot); + let range_start = snapshot.anchor_before(range.start); + let range_end = if range.start == range.end { + range_start + } else { + snapshot.anchor_after(range.end) + }; + let range = range_start..range_end; let code_lens_actions = self .lsp_store - .update(cx, |lsp_store, cx| lsp_store.code_lens(buffer_handle, cx)); + .update(cx, |lsp_store, cx| lsp_store.code_lens_actions(buffer, cx)); cx.background_spawn(async move { - let mut code_lens_actions = code_lens_actions.await?; + let mut code_lens_actions = code_lens_actions + .await + .map_err(|e| anyhow!("code lens fetch failed: {e:#}"))?; code_lens_actions.retain(|code_lens_action| { range .start