Fix `formatter: "auto"` to skip language servers that can't format (#50661)

João Soares created 1 month ago

Closes #50631

Before you mark this PR as ready for review, make sure that you have:
- [x] Added a solid test coverage and/or screenshots from doing manual
testing
- [x] Done a self-review taking into account security and performance
aspects
- [ ] Aligned any UI changes with the [UI
checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)

Release Notes:

- Fixed `formatter: "auto"` silently doing nothing when the first
language server for a language doesn't support formatting (e.g., Dependi
before Tombi for
TOML).

Change summary

crates/editor/src/editor_tests.rs | 90 +++++++++++++++++++++++++++++++++
crates/project/src/lsp_store.rs   | 15 ++++-
2 files changed, 102 insertions(+), 3 deletions(-)

Detailed changes

crates/editor/src/editor_tests.rs

@@ -12915,6 +12915,96 @@ async fn test_document_format_during_save(cx: &mut TestAppContext) {
     }
 }
 
+#[gpui::test]
+async fn test_auto_formatter_skips_server_without_formatting(cx: &mut TestAppContext) {
+    init_test(cx, |_| {});
+
+    let fs = FakeFs::new(cx.executor());
+    fs.insert_file(path!("/file.rs"), Default::default()).await;
+
+    let project = Project::test(fs, [path!("/file.rs").as_ref()], cx).await;
+
+    let language_registry = project.read_with(cx, |project, _| project.languages().clone());
+    language_registry.add(rust_lang());
+
+    // First server: no formatting capability
+    let mut no_format_servers = language_registry.register_fake_lsp(
+        "Rust",
+        FakeLspAdapter {
+            name: "no-format-server",
+            capabilities: lsp::ServerCapabilities {
+                completion_provider: Some(lsp::CompletionOptions::default()),
+                ..Default::default()
+            },
+            ..Default::default()
+        },
+    );
+
+    // Second server: has formatting capability
+    let mut format_servers = language_registry.register_fake_lsp(
+        "Rust",
+        FakeLspAdapter {
+            name: "format-server",
+            capabilities: lsp::ServerCapabilities {
+                document_formatting_provider: Some(lsp::OneOf::Left(true)),
+                ..Default::default()
+            },
+            ..Default::default()
+        },
+    );
+
+    let buffer = project
+        .update(cx, |project, cx| {
+            project.open_local_buffer(path!("/file.rs"), cx)
+        })
+        .await
+        .unwrap();
+
+    let buffer = cx.new(|cx| MultiBuffer::singleton(buffer, cx));
+    let (editor, cx) = cx.add_window_view(|window, cx| {
+        build_editor_with_project(project.clone(), buffer, window, cx)
+    });
+    editor.update_in(cx, |editor, window, cx| {
+        editor.set_text("one\ntwo\nthree\n", window, cx)
+    });
+
+    let _no_format_server = no_format_servers.next().await.unwrap();
+    let format_server = format_servers.next().await.unwrap();
+
+    format_server.set_request_handler::<lsp::request::Formatting, _, _>(
+        move |params, _| async move {
+            assert_eq!(
+                params.text_document.uri,
+                lsp::Uri::from_file_path(path!("/file.rs")).unwrap()
+            );
+            Ok(Some(vec![lsp::TextEdit::new(
+                lsp::Range::new(lsp::Position::new(0, 3), lsp::Position::new(1, 0)),
+                ", ".to_string(),
+            )]))
+        },
+    );
+
+    let save = editor
+        .update_in(cx, |editor, window, cx| {
+            editor.save(
+                SaveOptions {
+                    format: true,
+                    autosave: false,
+                },
+                project.clone(),
+                window,
+                cx,
+            )
+        })
+        .unwrap();
+    save.await;
+
+    assert_eq!(
+        editor.update(cx, |editor, cx| editor.text(cx)),
+        "one, two\nthree\n"
+    );
+}
+
 #[gpui::test]
 async fn test_redo_after_noop_format(cx: &mut TestAppContext) {
     init_test(cx, |settings| {

crates/project/src/lsp_store.rs

@@ -1778,9 +1778,10 @@ impl LocalLspStore {
                                 }
                             })
                         }
-                        settings::LanguageServerFormatterSpecifier::Current => {
-                            adapters_and_servers.first().map(|e| e.1.clone())
-                        }
+                        settings::LanguageServerFormatterSpecifier::Current => adapters_and_servers
+                            .iter()
+                            .find(|(_, server)| Self::server_supports_formatting(server))
+                            .map(|(_, server)| server.clone()),
                     };
 
                     let Some(language_server) = language_server else {
@@ -2285,6 +2286,14 @@ impl LocalLspStore {
         }
     }
 
+    fn server_supports_formatting(server: &Arc<LanguageServer>) -> bool {
+        let capabilities = server.capabilities();
+        let formatting = capabilities.document_formatting_provider.as_ref();
+        let range_formatting = capabilities.document_range_formatting_provider.as_ref();
+        matches!(formatting, Some(p) if *p != OneOf::Left(false))
+            || matches!(range_formatting, Some(p) if *p != OneOf::Left(false))
+    }
+
     async fn format_via_lsp(
         this: &WeakEntity<LspStore>,
         buffer: &Entity<Buffer>,