Allow all completions with preresolved additional text edits (#2711)

Kirill Bulatov created 2 years ago

Deals with https://github.com/zed-industries/community/issues/752
Deals with https://github.com/zed-industries/community/issues/566

Currently, when converting from LSP to Zed objects, completions with
non-empty `additional_text_edits` are filtered out.
Later, all other completions form a list and the selected one gets the
`Editor::confirm_completion` call, which always queries an LSP
completion resolve request to get the `additional_text_edits` field.

Otherwise, `additional_text_edits` field is ignored entirely for the
rest of the completion lifetime — and we always pass the selected
completion through the resolve request.

The PR changes the logic, removing the `additional_text_edits` filtering
and instead of resolving every completion, now we check for
`additional_text_edits` in the completion before resolving: resolve
happens only if the data is absent.

Generally, feels like resolve has to happen before the completion
selection: LSP servers may send us markdown for completion documentation
preview pop ups and similar extra info.
Also, the server may lack resolve capabilities entirely, always sending
the request seems dangerous.
For now, the PR does not attempt to change either.

Release Notes:

- Brings rust-analyzer's postfix completions and others completions with
preresolved additional text edits

Change summary

crates/editor/src/editor_tests.rs | 91 +++++++++++++++++++++++++++++++++
crates/project/src/lsp_command.rs | 10 ---
crates/project/src/project.rs     |  8 +-
3 files changed, 95 insertions(+), 14 deletions(-)

Detailed changes

crates/editor/src/editor_tests.rs

@@ -7223,6 +7223,97 @@ async fn test_language_server_restart_due_to_settings_change(cx: &mut gpui::Test
     );
 }
 
+#[gpui::test]
+async fn test_completions_with_additional_edits(cx: &mut gpui::TestAppContext) {
+    init_test(cx, |_| {});
+
+    let mut cx = EditorLspTestContext::new_rust(
+        lsp::ServerCapabilities {
+            completion_provider: Some(lsp::CompletionOptions {
+                trigger_characters: Some(vec![".".to_string()]),
+                ..Default::default()
+            }),
+            ..Default::default()
+        },
+        cx,
+    )
+    .await;
+
+    cx.set_state(indoc! {"fn main() { let a = 2ˇ; }"});
+    cx.simulate_keystroke(".");
+    let completion_item = lsp::CompletionItem {
+        label: "some".into(),
+        kind: Some(lsp::CompletionItemKind::SNIPPET),
+        detail: Some("Wrap the expression in an `Option::Some`".to_string()),
+        documentation: Some(lsp::Documentation::MarkupContent(lsp::MarkupContent {
+            kind: lsp::MarkupKind::Markdown,
+            value: "```rust\nSome(2)\n```".to_string(),
+        })),
+        deprecated: Some(false),
+        sort_text: Some("fffffff2".to_string()),
+        filter_text: Some("some".to_string()),
+        insert_text_format: Some(lsp::InsertTextFormat::SNIPPET),
+        text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit {
+            range: lsp::Range {
+                start: lsp::Position {
+                    line: 0,
+                    character: 22,
+                },
+                end: lsp::Position {
+                    line: 0,
+                    character: 22,
+                },
+            },
+            new_text: "Some(2)".to_string(),
+        })),
+        additional_text_edits: Some(vec![lsp::TextEdit {
+            range: lsp::Range {
+                start: lsp::Position {
+                    line: 0,
+                    character: 20,
+                },
+                end: lsp::Position {
+                    line: 0,
+                    character: 22,
+                },
+            },
+            new_text: "".to_string(),
+        }]),
+        ..Default::default()
+    };
+
+    let closure_completion_item = completion_item.clone();
+    let mut request = cx.handle_request::<lsp::request::Completion, _, _>(move |_, _, _| {
+        let task_completion_item = closure_completion_item.clone();
+        async move {
+            Ok(Some(lsp::CompletionResponse::Array(vec![
+                task_completion_item,
+            ])))
+        }
+    });
+
+    request.next().await;
+
+    cx.condition(|editor, _| editor.context_menu_visible())
+        .await;
+    let apply_additional_edits = cx.update_editor(|editor, cx| {
+        editor
+            .confirm_completion(&ConfirmCompletion::default(), cx)
+            .unwrap()
+    });
+    cx.assert_editor_state(indoc! {"fn main() { let a = 2.Some(2)ˇ; }"});
+
+    cx.handle_request::<lsp::request::ResolveCompletionItem, _, _>(move |_, _, _| {
+        let task_completion_item = completion_item.clone();
+        async move { Ok(task_completion_item) }
+    })
+    .next()
+    .await
+    .unwrap();
+    apply_additional_edits.await.unwrap();
+    cx.assert_editor_state(indoc! {"fn main() { let a = Some(2)ˇ; }"});
+}
+
 fn empty_range(row: usize, column: usize) -> Range<DisplayPoint> {
     let point = DisplayPoint::new(row as u32, column as u32);
     point..point

crates/project/src/lsp_command.rs

@@ -1358,16 +1358,6 @@ impl LspCommand for GetCompletions {
             completions
                 .into_iter()
                 .filter_map(move |mut lsp_completion| {
-                    // For now, we can only handle additional edits if they are returned
-                    // when resolving the completion, not if they are present initially.
-                    if lsp_completion
-                        .additional_text_edits
-                        .as_ref()
-                        .map_or(false, |edits| !edits.is_empty())
-                    {
-                        return None;
-                    }
-
                     let (old_range, mut new_text) = match lsp_completion.text_edit.as_ref() {
                         // If the language server provides a range to overwrite, then
                         // check that the range is valid.

crates/project/src/project.rs

@@ -4447,11 +4447,11 @@ impl Project {
             };
 
             cx.spawn(|this, mut cx| async move {
-                let resolved_completion = lang_server
+                let additional_text_edits = lang_server
                     .request::<lsp::request::ResolveCompletionItem>(completion.lsp_completion)
-                    .await?;
-
-                if let Some(edits) = resolved_completion.additional_text_edits {
+                    .await?
+                    .additional_text_edits;
+                if let Some(edits) = additional_text_edits {
                     let edits = this
                         .update(&mut cx, |this, cx| {
                             this.edits_from_lsp(