Clean up stale conflicting hints

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<Inlay> {

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<ExcerptId, (ModelHandle<Buffer>, Global, Range<usize>)>,
         invalidate: InvalidationStrategy,
         cx: &mut ViewContext<Editor>,
-    ) {
+    ) -> Option<InlaySplice> {
-        if !self.enabled || excerpts_to_query.is_empty() {
+        if !self.enabled {
-            return;
+            return None;
         }
+
         let update_tasks = &mut self.update_tasks;
+        let mut invalidated_hints = Vec::new();
         if invalidate.should_invalidate() {
-            update_tasks
+            let mut changed = false;
-                .retain(|task_excerpt_id, _| excerpts_to_query.contains_key(task_excerpt_id));
+            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!(editor.inlay_hint_cache().version, 1);
-            assert_eq!(inlay_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!(editor.inlay_hint_cache().version, 1);
-            assert_eq!(inlay_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!(editor.inlay_hint_cache().version, 2);
-            assert_eq!(inlay_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!(editor.inlay_hint_cache().version, 2);
-            assert_eq!(inlay_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!(editor.inlay_hint_cache().version, edits_made);
-            assert_eq!(inlay_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!(editor.inlay_hint_cache().version, 5, "Should update the cache for every LSP response with hints added");
-            assert_eq!(inlay_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!(editor.inlay_hint_cache().version, expected_layers.len(), "Every visible excerpt hints should bump the verison");
-            assert_eq!(inlay_cache.version, 4, "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!(editor.inlay_hint_cache().version, expected_layers.len(),
-            assert_eq!(inlay_cache.version, 9);
+                "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!(editor.inlay_hint_cache().version, expected_layers.len());
-            assert_eq!(inlay_cache.version, 12);
+            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!(editor.inlay_hint_cache().version, last_scroll_update_version, "No updates should happen during scrolling already scolled buffer");
-            assert_eq!(inlay_cache.version, 12, "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(),
+                "main hint(edited) #4".to_string(),
-                "other hint #1".to_string(),
+                "main hint(edited) #5".to_string(),
-                "other hint #2".to_string(),
+                "other hint(edited) #0".to_string(),
-                "other hint #3".to_string(),
+                "other hint(edited) #1".to_string(),
-                "other hint #4".to_string(),
-                "other hint #5".to_string(),
             ];
-            assert_eq!(expected_layers, cached_hint_labels(editor),
+            assert_eq!(
-                "After multibuffer was edited, hints for the edited buffer (1st) should be invalidated and requeried for all of its visible excerpts, \
+                expected_layers,
-unedited (2nd) buffer should have the same hint");
+                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!(
-            assert_eq!(inlay_cache.version, 16);
+                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<Deterministic>,
+        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::<Vec<_>>().join("")),
+                "other.rs": format!("fn main() {{\n{}\n}}", (0..501).map(|j| format!("let j = {j};\n")).collect::<Vec<_>>().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::<lsp::request::InlayHintRequest, _, _>(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"
+            );
         });
     }