From 3058a96deeac15d3ccb1c17d14eb7712ae397e4b Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Wed, 19 Jul 2023 14:06:00 +0300 Subject: [PATCH] Clean up stale conflicting hints --- crates/editor/src/editor.rs | 9 +- crates/editor/src/inlay_hint_cache.rs | 375 ++++++++++++++++++++++---- 2 files changed, 323 insertions(+), 61 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index b8a7853a93..379f43e2e5 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -2659,11 +2659,16 @@ impl Editor { InlayRefreshReason::RefreshRequested => (InvalidationStrategy::RefreshRequested, None), }; - self.inlay_hint_cache.refresh_inlay_hints( + if let Some(InlaySplice { + to_remove, + to_insert, + }) = self.inlay_hint_cache.spawn_hint_refresh( self.excerpt_visible_offsets(required_languages.as_ref(), cx), invalidate_cache, cx, - ) + ) { + self.splice_inlay_hints(to_remove, to_insert, cx); + } } fn visible_inlay_hints(&self, cx: &ViewContext<'_, '_, Editor>) -> Vec { diff --git a/crates/editor/src/inlay_hint_cache.rs b/crates/editor/src/inlay_hint_cache.rs index 52473f9971..63076ba234 100644 --- a/crates/editor/src/inlay_hint_cache.rs +++ b/crates/editor/src/inlay_hint_cache.rs @@ -195,20 +195,41 @@ impl InlayHintCache { } } - pub fn refresh_inlay_hints( + pub fn spawn_hint_refresh( &mut self, mut excerpts_to_query: HashMap, Global, Range)>, invalidate: InvalidationStrategy, cx: &mut ViewContext, - ) { - if !self.enabled || excerpts_to_query.is_empty() { - return; + ) -> Option { + if !self.enabled { + return None; } + let update_tasks = &mut self.update_tasks; + let mut invalidated_hints = Vec::new(); if invalidate.should_invalidate() { - update_tasks - .retain(|task_excerpt_id, _| excerpts_to_query.contains_key(task_excerpt_id)); + let mut changed = false; + update_tasks.retain(|task_excerpt_id, _| { + let retain = excerpts_to_query.contains_key(task_excerpt_id); + changed |= !retain; + retain + }); + self.hints.retain(|cached_excerpt, cached_hints| { + let retain = excerpts_to_query.contains_key(cached_excerpt); + changed |= !retain; + if !retain { + invalidated_hints.extend(cached_hints.read().hints.iter().map(|&(id, _)| id)); + } + retain + }); + if changed { + self.version += 1; + } } + if excerpts_to_query.is_empty() && invalidated_hints.is_empty() { + return None; + } + let cache_version = self.version; excerpts_to_query.retain(|visible_excerpt_id, _| { match update_tasks.entry(*visible_excerpt_id) { @@ -229,6 +250,15 @@ impl InlayHintCache { .ok(); }) .detach(); + + if invalidated_hints.is_empty() { + None + } else { + Some(InlaySplice { + to_remove: invalidated_hints, + to_insert: Vec::new(), + }) + } } fn new_allowed_hint_kinds_splice( @@ -684,7 +714,7 @@ async fn fetch_and_update_hints( if query.invalidate.should_invalidate() { let mut outdated_excerpt_caches = HashSet::default(); - for (excerpt_id, excerpt_hints) in editor.inlay_hint_cache().hints.iter() { + for (excerpt_id, excerpt_hints) in &editor.inlay_hint_cache().hints { let excerpt_hints = excerpt_hints.read(); if excerpt_hints.buffer_id == query.buffer_id && excerpt_id != &query.excerpt_id @@ -1022,9 +1052,9 @@ mod tests { "Should get its first hints when opening the editor" ); assert_eq!(expected_layers, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); assert_eq!( - inlay_cache.version, edits_made, + editor.inlay_hint_cache().version, + edits_made, "The editor update the cache version after every cache/view change" ); }); @@ -1053,9 +1083,9 @@ mod tests { "Should not update hints while the work task is running" ); assert_eq!(expected_layers, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); assert_eq!( - inlay_cache.version, edits_made, + editor.inlay_hint_cache().version, + edits_made, "Should not update the cache while the work task is running" ); }); @@ -1077,9 +1107,9 @@ mod tests { "New hints should be queried after the work task is done" ); assert_eq!(expected_layers, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); assert_eq!( - inlay_cache.version, edits_made, + editor.inlay_hint_cache().version, + edits_made, "Cache version should udpate once after the work task is done" ); }); @@ -1194,9 +1224,9 @@ mod tests { "Should get its first hints when opening the editor" ); assert_eq!(expected_layers, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); assert_eq!( - inlay_cache.version, 1, + editor.inlay_hint_cache().version, + 1, "Rust editor update the cache version after every cache/view change" ); }); @@ -1252,8 +1282,7 @@ mod tests { "Markdown editor should have a separate verison, repeating Rust editor rules" ); assert_eq!(expected_layers, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); - assert_eq!(inlay_cache.version, 1); + assert_eq!(editor.inlay_hint_cache().version, 1); }); rs_editor.update(cx, |editor, cx| { @@ -1269,9 +1298,9 @@ mod tests { "Rust inlay cache should change after the edit" ); assert_eq!(expected_layers, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); assert_eq!( - inlay_cache.version, 2, + editor.inlay_hint_cache().version, + 2, "Every time hint cache changes, cache version should be incremented" ); }); @@ -1283,8 +1312,7 @@ mod tests { "Markdown editor should not be affected by Rust editor changes" ); assert_eq!(expected_layers, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); - assert_eq!(inlay_cache.version, 1); + assert_eq!(editor.inlay_hint_cache().version, 1); }); md_editor.update(cx, |editor, cx| { @@ -1300,8 +1328,7 @@ mod tests { "Rust editor should not be affected by Markdown editor changes" ); assert_eq!(expected_layers, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); - assert_eq!(inlay_cache.version, 2); + assert_eq!(editor.inlay_hint_cache().version, 2); }); rs_editor.update(cx, |editor, cx| { let expected_layers = vec!["1".to_string()]; @@ -1311,8 +1338,7 @@ mod tests { "Markdown editor should also change independently" ); assert_eq!(expected_layers, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); - assert_eq!(inlay_cache.version, 2); + assert_eq!(editor.inlay_hint_cache().version, 2); }); } @@ -1433,9 +1459,9 @@ mod tests { vec!["other hint".to_string(), "type hint".to_string()], visible_hint_labels(editor, cx) ); - let inlay_cache = editor.inlay_hint_cache(); assert_eq!( - inlay_cache.version, edits_made, + editor.inlay_hint_cache().version, + edits_made, "Should not update cache version due to new loaded hints being the same" ); }); @@ -1568,9 +1594,8 @@ mod tests { ); assert!(cached_hint_labels(editor).is_empty()); assert!(visible_hint_labels(editor, cx).is_empty()); - let inlay_cache = editor.inlay_hint_cache(); assert_eq!( - inlay_cache.version, edits_made, + editor.inlay_hint_cache().version, edits_made, "The editor should not update the cache version after /refresh query without updates" ); }); @@ -1641,8 +1666,7 @@ mod tests { vec!["parameter hint".to_string()], visible_hint_labels(editor, cx), ); - let inlay_cache = editor.inlay_hint_cache(); - assert_eq!(inlay_cache.version, edits_made); + assert_eq!(editor.inlay_hint_cache().version, edits_made); }); } @@ -1720,9 +1744,8 @@ mod tests { "Should get hints from the last edit landed only" ); assert_eq!(expected_hints, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); assert_eq!( - inlay_cache.version, 1, + editor.inlay_hint_cache().version, 1, "Only one update should be registered in the cache after all cancellations" ); }); @@ -1766,9 +1789,9 @@ mod tests { "Should get hints from the last edit landed only" ); assert_eq!(expected_hints, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); assert_eq!( - inlay_cache.version, 2, + editor.inlay_hint_cache().version, + 2, "Should update the cache version once more, for the new change" ); }); @@ -1886,9 +1909,8 @@ mod tests { "Should have hints from both LSP requests made for a big file" ); assert_eq!(expected_layers, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); assert_eq!( - inlay_cache.version, 2, + editor.inlay_hint_cache().version, 2, "Both LSP queries should've bumped the cache version" ); }); @@ -1918,8 +1940,7 @@ mod tests { assert_eq!(expected_layers, cached_hint_labels(editor), "Should have hints from the new LSP response after edit"); assert_eq!(expected_layers, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); - assert_eq!(inlay_cache.version, 5, "Should update the cache for every LSP response with hints added"); + assert_eq!(editor.inlay_hint_cache().version, 5, "Should update the cache for every LSP response with hints added"); }); } @@ -2075,6 +2096,7 @@ mod tests { panic!("unexpected uri: {:?}", params.text_document.uri); }; + // one hint per excerpt let positions = [ lsp::Position::new(0, 2), lsp::Position::new(4, 2), @@ -2138,8 +2160,7 @@ mod tests { "When scroll is at the edge of a multibuffer, its visible excerpts only should be queried for inlay hints" ); assert_eq!(expected_layers, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); - assert_eq!(inlay_cache.version, 4, "Every visible excerpt hints should bump the verison"); + assert_eq!(editor.inlay_hint_cache().version, expected_layers.len(), "Every visible excerpt hints should bump the verison"); }); editor.update(cx, |editor, cx| { @@ -2169,8 +2190,8 @@ mod tests { assert_eq!(expected_layers, cached_hint_labels(editor), "With more scrolls of the multibuffer, more hints should be added into the cache and nothing invalidated without edits"); assert_eq!(expected_layers, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); - assert_eq!(inlay_cache.version, 9); + assert_eq!(editor.inlay_hint_cache().version, expected_layers.len(), + "Due to every excerpt having one hint, we update cache per new excerpt scrolled"); }); editor.update(cx, |editor, cx| { @@ -2179,7 +2200,7 @@ mod tests { }); }); cx.foreground().run_until_parked(); - editor.update(cx, |editor, cx| { + let last_scroll_update_version = editor.update(cx, |editor, cx| { let expected_layers = vec![ "main hint #0".to_string(), "main hint #1".to_string(), @@ -2197,8 +2218,8 @@ mod tests { assert_eq!(expected_layers, cached_hint_labels(editor), "After multibuffer was scrolled to the end, all hints for all excerpts should be fetched"); assert_eq!(expected_layers, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); - assert_eq!(inlay_cache.version, 12); + assert_eq!(editor.inlay_hint_cache().version, expected_layers.len()); + expected_layers.len() }); editor.update(cx, |editor, cx| { @@ -2225,12 +2246,14 @@ mod tests { assert_eq!(expected_layers, cached_hint_labels(editor), "After multibuffer was scrolled to the end, further scrolls up should not bring more hints"); assert_eq!(expected_layers, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); - assert_eq!(inlay_cache.version, 12, "No updates should happen during scrolling already scolled buffer"); + assert_eq!(editor.inlay_hint_cache().version, last_scroll_update_version, "No updates should happen during scrolling already scolled buffer"); }); editor_edited.store(true, Ordering::Release); editor.update(cx, |editor, cx| { + editor.change_selections(None, cx, |s| { + s.select_ranges([Point::new(56, 0)..Point::new(56, 0)]) + }); editor.handle_input("++++more text++++", cx); }); cx.foreground().run_until_parked(); @@ -2240,19 +2263,253 @@ mod tests { "main hint(edited) #1".to_string(), "main hint(edited) #2".to_string(), "main hint(edited) #3".to_string(), - "other hint #0".to_string(), - "other hint #1".to_string(), - "other hint #2".to_string(), - "other hint #3".to_string(), - "other hint #4".to_string(), - "other hint #5".to_string(), + "main hint(edited) #4".to_string(), + "main hint(edited) #5".to_string(), + "other hint(edited) #0".to_string(), + "other hint(edited) #1".to_string(), ]; - assert_eq!(expected_layers, cached_hint_labels(editor), - "After multibuffer was edited, hints for the edited buffer (1st) should be invalidated and requeried for all of its visible excerpts, \ -unedited (2nd) buffer should have the same hint"); + assert_eq!( + expected_layers, + cached_hint_labels(editor), + "After multibuffer edit, editor gets scolled back to the last selection; \ +all hints should be invalidated and requeried for all of its visible excerpts" + ); assert_eq!(expected_layers, visible_hint_labels(editor, cx)); - let inlay_cache = editor.inlay_hint_cache(); - assert_eq!(inlay_cache.version, 16); + assert_eq!( + editor.inlay_hint_cache().version, + last_scroll_update_version + expected_layers.len() + 1, + "Due to every excerpt having one hint, cache should update per new excerpt received + 1 for outdated hints removal" + ); + }); + } + + #[gpui::test] + async fn test_excerpts_removed( + deterministic: Arc, + cx: &mut gpui::TestAppContext, + ) { + init_test(cx, |settings| { + settings.defaults.inlay_hints = Some(InlayHintSettings { + enabled: true, + show_type_hints: false, + show_parameter_hints: false, + show_other_hints: false, + }) + }); + + let mut language = Language::new( + LanguageConfig { + name: "Rust".into(), + path_suffixes: vec!["rs".to_string()], + ..Default::default() + }, + Some(tree_sitter_rust::language()), + ); + let mut fake_servers = language + .set_fake_lsp_adapter(Arc::new(FakeLspAdapter { + capabilities: lsp::ServerCapabilities { + inlay_hint_provider: Some(lsp::OneOf::Left(true)), + ..Default::default() + }, + ..Default::default() + })) + .await; + let language = Arc::new(language); + let fs = FakeFs::new(cx.background()); + fs.insert_tree( + "/a", + json!({ + "main.rs": format!("fn main() {{\n{}\n}}", (0..501).map(|i| format!("let i = {i};\n")).collect::>().join("")), + "other.rs": format!("fn main() {{\n{}\n}}", (0..501).map(|j| format!("let j = {j};\n")).collect::>().join("")), + }), + ) + .await; + let project = Project::test(fs, ["/a".as_ref()], cx).await; + project.update(cx, |project, _| { + project.languages().add(Arc::clone(&language)) + }); + let (_, workspace) = cx.add_window(|cx| Workspace::test_new(project.clone(), cx)); + let worktree_id = workspace.update(cx, |workspace, cx| { + workspace.project().read_with(cx, |project, cx| { + project.worktrees(cx).next().unwrap().read(cx).id() + }) + }); + + let buffer_1 = project + .update(cx, |project, cx| { + project.open_buffer((worktree_id, "main.rs"), cx) + }) + .await + .unwrap(); + let buffer_2 = project + .update(cx, |project, cx| { + project.open_buffer((worktree_id, "other.rs"), cx) + }) + .await + .unwrap(); + let multibuffer = cx.add_model(|_| MultiBuffer::new(0)); + let (buffer_1_excerpts, buffer_2_excerpts) = multibuffer.update(cx, |multibuffer, cx| { + let buffer_1_excerpts = multibuffer.push_excerpts( + buffer_1.clone(), + [ExcerptRange { + context: Point::new(0, 0)..Point::new(2, 0), + primary: None, + }], + cx, + ); + let buffer_2_excerpts = multibuffer.push_excerpts( + buffer_2.clone(), + [ExcerptRange { + context: Point::new(0, 1)..Point::new(2, 1), + primary: None, + }], + cx, + ); + (buffer_1_excerpts, buffer_2_excerpts) + }); + + assert!(!buffer_1_excerpts.is_empty()); + assert!(!buffer_2_excerpts.is_empty()); + + deterministic.run_until_parked(); + cx.foreground().run_until_parked(); + let (_, editor) = + cx.add_window(|cx| Editor::for_multibuffer(multibuffer, Some(project.clone()), cx)); + let editor_edited = Arc::new(AtomicBool::new(false)); + let fake_server = fake_servers.next().await.unwrap(); + let closure_editor_edited = Arc::clone(&editor_edited); + fake_server + .handle_request::(move |params, _| { + let task_editor_edited = Arc::clone(&closure_editor_edited); + async move { + let hint_text = if params.text_document.uri + == lsp::Url::from_file_path("/a/main.rs").unwrap() + { + "main hint" + } else if params.text_document.uri + == lsp::Url::from_file_path("/a/other.rs").unwrap() + { + "other hint" + } else { + panic!("unexpected uri: {:?}", params.text_document.uri); + }; + + let positions = [ + lsp::Position::new(0, 2), + lsp::Position::new(4, 2), + lsp::Position::new(22, 2), + lsp::Position::new(44, 2), + lsp::Position::new(56, 2), + lsp::Position::new(67, 2), + ]; + let out_of_range_hint = lsp::InlayHint { + position: lsp::Position::new( + params.range.start.line + 99, + params.range.start.character + 99, + ), + label: lsp::InlayHintLabel::String( + "out of excerpt range, should be ignored".to_string(), + ), + kind: None, + text_edits: None, + tooltip: None, + padding_left: None, + padding_right: None, + data: None, + }; + + let edited = task_editor_edited.load(Ordering::Acquire); + Ok(Some( + std::iter::once(out_of_range_hint) + .chain(positions.into_iter().enumerate().map(|(i, position)| { + lsp::InlayHint { + position, + label: lsp::InlayHintLabel::String(format!( + "{hint_text}{} #{i}", + if edited { "(edited)" } else { "" }, + )), + kind: None, + text_edits: None, + tooltip: None, + padding_left: None, + padding_right: None, + data: None, + } + })) + .collect(), + )) + } + }) + .next() + .await; + cx.foreground().run_until_parked(); + + editor.update(cx, |editor, cx| { + assert_eq!( + vec!["main hint #0".to_string(), "other hint #0".to_string()], + cached_hint_labels(editor), + "Cache should update for both excerpts despite hints display was disabled" + ); + assert!( + visible_hint_labels(editor, cx).is_empty(), + "All hints are disabled and should not be shown despite being present in the cache" + ); + assert_eq!( + editor.inlay_hint_cache().version, + 2, + "Cache should update once per excerpt query" + ); + }); + + editor.update(cx, |editor, cx| { + editor.buffer().update(cx, |multibuffer, cx| { + multibuffer.remove_excerpts(buffer_2_excerpts, cx) + }) + }); + cx.foreground().run_until_parked(); + editor.update(cx, |editor, cx| { + assert_eq!( + vec!["main hint #0".to_string()], + cached_hint_labels(editor), + "For the removed excerpt, should clean corresponding cached hints" + ); + assert!( + visible_hint_labels(editor, cx).is_empty(), + "All hints are disabled and should not be shown despite being present in the cache" + ); + assert_eq!( + editor.inlay_hint_cache().version, + 3, + "Excerpt removal should trigger cache update" + ); + }); + + update_test_language_settings(cx, |settings| { + settings.defaults.inlay_hints = Some(InlayHintSettings { + enabled: true, + show_type_hints: true, + show_parameter_hints: true, + show_other_hints: true, + }) + }); + cx.foreground().run_until_parked(); + editor.update(cx, |editor, cx| { + let expected_hints = vec!["main hint #0".to_string()]; + assert_eq!( + expected_hints, + cached_hint_labels(editor), + "Hint display settings change should not change the cache" + ); + assert_eq!( + expected_hints, + visible_hint_labels(editor, cx), + "Settings change should make cached hints visible" + ); + assert_eq!( + editor.inlay_hint_cache().version, + 4, + "Settings change should trigger cache update" + ); }); }