From b4993eb89c1372fed23c38158b0a5a5697077af9 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Thu, 20 Aug 2026 12:15:06 +0000 Subject: [PATCH] Properly deduplicate overlapping range formatting results (#62935) Fixes https://github.com/zed-industries/zed/issues/62780 When formatting changed-only regions, we have received formatting changes that are larger than the regions submitted. Previous code merged only adjacent hunks in a single response, without merging anything else, e.g. multiple responses' ranges that overlapped. Release Notes: - Fixed overlapping range format results duplicating the text --- crates/editor/src/editor_tests.rs | 516 +++++++++++++++++++++--------- crates/project/src/lsp_store.rs | 74 ++++- 2 files changed, 432 insertions(+), 158 deletions(-) diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 062c869d78..2246b2bddd 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -16228,160 +16228,6 @@ async fn test_autosave_with_dirty_buffers(cx: &mut TestAppContext) { ); } -async fn setup_range_format_test_with_capabilities( - cx: &mut TestAppContext, - capabilities: lsp::ServerCapabilities, -) -> ( - Entity, - Entity, - &mut gpui::VisualTestContext, - lsp::FakeLanguageServer, -) { - init_test(cx, |_| {}); - - let fs = FakeFs::new(cx.executor()); - fs.insert_file(path!("/file.rs"), Default::default()).await; - - let project = Project::test(fs, [path!("/").as_ref()], cx).await; - - let language_registry = project.read_with(cx, |project, _| project.languages().clone()); - language_registry.add(rust_lang()); - let mut fake_servers = language_registry.register_fake_lsp( - "Rust", - FakeLspAdapter { - capabilities, - ..FakeLspAdapter::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| { - window.focus(&editor.focus_handle(cx), cx); - }); - - let fake_server = fake_servers.next().await.unwrap(); - - (project, editor, cx, fake_server) -} - -async fn setup_range_format_test( - cx: &mut TestAppContext, -) -> ( - Entity, - Entity, - &mut gpui::VisualTestContext, - lsp::FakeLanguageServer, -) { - setup_range_format_test_with_capabilities( - cx, - lsp::ServerCapabilities { - document_range_formatting_provider: Some(lsp::OneOf::Left(true)), - ..lsp::ServerCapabilities::default() - }, - ) - .await -} - -/// Like `setup_range_format_test`, but backs the buffer with a FakeFs git -/// repository so that `GitStore::get_unstaged_diff` returns a real diff. -/// `head_content` sets the HEAD base, `index_content` sets the staged base. -/// The buffer starts empty; the caller must `editor.set_text(...)` to set the -/// working-tree content (the diff recomputes from buffer changes). -async fn setup_range_format_test_with_git<'a>( - cx: &'a mut TestAppContext, - head_content: &str, - index_content: &str, -) -> ( - Entity, - Entity, - &'a mut gpui::VisualTestContext, - lsp::FakeLanguageServer, -) { - init_test(cx, |_| {}); - - let fs = FakeFs::new(cx.executor()); - fs.insert_tree( - path!("/project"), - json!({ - ".git": {}, - "file.rs": "", - }), - ) - .await; - - fs.set_head_for_repo( - std::path::Path::new(path!("/project/.git")), - &[("file.rs", head_content.to_string())], - "deadbeef", - ); - fs.set_index_for_repo( - std::path::Path::new(path!("/project/.git")), - &[("file.rs", index_content.to_string())], - ); - - let project = Project::test(fs, [path!("/project").as_ref()], cx).await; - - let language_registry = project.read_with(cx, |project, _| project.languages().clone()); - language_registry.add(rust_lang()); - let mut fake_servers = language_registry.register_fake_lsp( - "Rust", - FakeLspAdapter { - capabilities: lsp::ServerCapabilities { - document_range_formatting_provider: Some(lsp::OneOf::Left(true)), - document_formatting_provider: Some(lsp::OneOf::Left(true)), - ..lsp::ServerCapabilities::default() - }, - ..FakeLspAdapter::default() - }, - ); - - let buffer = project - .update(cx, |project, cx| { - project.open_local_buffer(path!("/project/file.rs"), cx) - }) - .await - .unwrap(); - - // Open the unstaged diff so GitStore tracks this buffer. Without this, - // `get_unstaged_diff` returns None and compute_format_target cannot - // produce range-based FormatTarget. - project - .update(cx, |project, cx| { - project.open_unstaged_diff(buffer.clone(), 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| { - window.focus(&editor.focus_handle(cx), cx); - }); - - let fake_server = fake_servers.next().await.unwrap(); - - (project, editor, cx, fake_server) -} - -fn refresh_editor_actions(cx: &mut VisualTestContext) { - cx.executor().run_until_parked(); - cx.update(|window, cx| { - let _ = window.draw(cx); - }); -} - #[gpui::test] async fn test_format_selections_action_available_when_range_formatting_is_supported( cx: &mut TestAppContext, @@ -16901,6 +16747,129 @@ async fn test_modifications_format_multiple_hunks(cx: &mut TestAppContext) { assert!(!cx.read(|cx| editor.is_dirty(cx))); } +#[gpui::test] +async fn test_modifications_format_overlapping_range_responses(cx: &mut TestAppContext) { + assert_range_format_merge( + cx, + "line0\nline1\nline2\nline3\nline4\nline5\n", + "line0\nLINE1\nline2\nline3\nLINE4\nline5\n", + vec![vec![lsp_line_edit(0, 6, "fmt0\nfmt1\nfmt2\nfmt3\nfmt4\nfmt5\n")]], + "fmt0\nfmt1\nfmt2\nfmt3\nfmt4\nfmt5\n", + "a response covering the second hunk must suppress its request and must not duplicate the formatted block", + ) + .await; +} + +#[gpui::test] +async fn test_modifications_format_divergent_overlapping_responses(cx: &mut TestAppContext) { + assert_range_format_merge( + cx, + "line0\nline1\nline2\nline3\nline4\nline5\n", + "line0\nLINE1\nline2\nline3\nLINE4\nline5\n", + vec![ + vec![lsp_line_edit(1, 3, "one\ntwo\n")], + vec![lsp_line_edit(2, 5, "TWO\nthree\nfour\n")], + ], + "line0\none\ntwo\nline3\nLINE4\nline5\n", + "the first edit must win and the divergent overlapping edit must be dropped without corrupting the buffer", + ) + .await; +} + +#[gpui::test] +async fn test_modifications_format_partially_overlapping_response_dropped_atomically( + cx: &mut TestAppContext, +) { + assert_range_format_merge( + cx, + "line0\nline1\nline2\nline3\nline4\nline5\n", + "line0\nLINE1\nline2\nline3\nLINE4\nline5\n", + vec![ + vec![lsp_line_edit(1, 2, "one\n")], + vec![ + lsp_line_edit(4, 5, "four\n"), + lsp_line_edit(1, 2, "ONE\n"), + ], + ], + "line0\none\nline2\nline3\nLINE4\nline5\n", + "a response with any divergent overlapping edit must be dropped whole, including its non-overlapping edits", + ) + .await; +} + +#[gpui::test] +async fn test_modifications_format_overlapping_edits_within_one_response(cx: &mut TestAppContext) { + assert_range_format_merge( + cx, + "line0\nline1\nline2\nline3\nline4\nline5\n", + "line0\nLINE1\nline2\nline3\nline4\nline5\n", + vec![vec![ + lsp_line_edit(1, 2, "one\n"), + lsp_line_edit(1, 2, "uno\n"), + ]], + "line0\none\nline2\nline3\nline4\nline5\n", + "the later overlapping edit within one response must be dropped", + ) + .await; +} + +#[gpui::test] +async fn test_modifications_format_same_position_inserts_within_one_response( + cx: &mut TestAppContext, +) { + assert_range_format_merge( + cx, + "line0\nline1\nline2\n", + "line0\nLINE1\nline2\n", + vec![vec![ + lsp_line_edit(1, 1, "alpha\n"), + lsp_line_edit(1, 1, "beta\n"), + ]], + "line0\nalpha\nbeta\nLINE1\nline2\n", + "multiple inserts at one position within one response are valid per the LSP spec and must all apply in array order", + ) + .await; +} + +#[gpui::test] +async fn test_modifications_format_divergent_same_position_inserts_across_responses( + cx: &mut TestAppContext, +) { + assert_range_format_merge( + cx, + "line0\nline1\nline2\nline3\nline4\nline5\n", + "line0\nLINE1\nline2\nline3\nLINE4\nline5\n", + vec![ + vec![lsp_line_edit(4, 4, "alpha\n")], + vec![lsp_line_edit(4, 4, "beta\n")], + ], + "line0\nLINE1\nline2\nline3\nalpha\nLINE4\nline5\n", + "a later response inserting different text at the same position diverges and must be dropped", + ) + .await; +} + +#[gpui::test] +async fn test_modifications_format_insert_touching_kept_edit_across_responses( + cx: &mut TestAppContext, +) { + assert_range_format_merge( + cx, + "line0\nline1\nline2\nline3\nline4\nline5\n", + "line0\nLINE1\nline2\nline3\nLINE4\nline5\n", + vec![ + vec![ + lsp_line_edit(1, 2, "one\n"), + lsp_line_edit(4, 4, "alpha\n"), + ], + vec![lsp_line_edit(4, 5, "alpha\nfour\n")], + ], + "line0\none\nline2\nline3\nalpha\nLINE4\nline5\n", + "an edit touching a kept insert at its boundary can duplicate the inserted text and must be dropped as divergent", + ) + .await; +} + #[gpui::test] async fn test_modifications_format_excludes_staged_changes(cx: &mut TestAppContext) { let head_content = "line0\nline1\nline2\nline3\nline4\n"; @@ -43435,3 +43404,242 @@ async fn test_in_preview_key_context(cx: &mut TestAppContext) { ); }); } + +async fn setup_range_format_test_with_capabilities( + cx: &mut TestAppContext, + capabilities: lsp::ServerCapabilities, +) -> ( + Entity, + Entity, + &mut gpui::VisualTestContext, + lsp::FakeLanguageServer, +) { + init_test(cx, |_| {}); + + let fs = FakeFs::new(cx.executor()); + fs.insert_file(path!("/file.rs"), Default::default()).await; + + let project = Project::test(fs, [path!("/").as_ref()], cx).await; + + let language_registry = project.read_with(cx, |project, _| project.languages().clone()); + language_registry.add(rust_lang()); + let mut fake_servers = language_registry.register_fake_lsp( + "Rust", + FakeLspAdapter { + capabilities, + ..FakeLspAdapter::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| { + window.focus(&editor.focus_handle(cx), cx); + }); + + let fake_server = fake_servers.next().await.unwrap(); + + (project, editor, cx, fake_server) +} + +async fn setup_range_format_test( + cx: &mut TestAppContext, +) -> ( + Entity, + Entity, + &mut gpui::VisualTestContext, + lsp::FakeLanguageServer, +) { + setup_range_format_test_with_capabilities( + cx, + lsp::ServerCapabilities { + document_range_formatting_provider: Some(lsp::OneOf::Left(true)), + ..lsp::ServerCapabilities::default() + }, + ) + .await +} + +/// Like `setup_range_format_test`, but backs the buffer with a FakeFs git +/// repository so that `GitStore::get_unstaged_diff` returns a real diff. +/// `head_content` sets the HEAD base, `index_content` sets the staged base. +/// The buffer starts empty; the caller must `editor.set_text(...)` to set the +/// working-tree content (the diff recomputes from buffer changes). +async fn setup_range_format_test_with_git<'a>( + cx: &'a mut TestAppContext, + head_content: &str, + index_content: &str, +) -> ( + Entity, + Entity, + &'a mut gpui::VisualTestContext, + lsp::FakeLanguageServer, +) { + init_test(cx, |_| {}); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + path!("/project"), + json!({ + ".git": {}, + "file.rs": "", + }), + ) + .await; + + fs.set_head_for_repo( + std::path::Path::new(path!("/project/.git")), + &[("file.rs", head_content.to_string())], + "deadbeef", + ); + fs.set_index_for_repo( + std::path::Path::new(path!("/project/.git")), + &[("file.rs", index_content.to_string())], + ); + + let project = Project::test(fs, [path!("/project").as_ref()], cx).await; + + let language_registry = project.read_with(cx, |project, _| project.languages().clone()); + language_registry.add(rust_lang()); + let mut fake_servers = language_registry.register_fake_lsp( + "Rust", + FakeLspAdapter { + capabilities: lsp::ServerCapabilities { + document_range_formatting_provider: Some(lsp::OneOf::Left(true)), + document_formatting_provider: Some(lsp::OneOf::Left(true)), + ..lsp::ServerCapabilities::default() + }, + ..FakeLspAdapter::default() + }, + ); + + let buffer = project + .update(cx, |project, cx| { + project.open_local_buffer(path!("/project/file.rs"), cx) + }) + .await + .unwrap(); + + // Open the unstaged diff so GitStore tracks this buffer. Without this, + // `get_unstaged_diff` returns None and compute_format_target cannot + // produce range-based FormatTarget. + project + .update(cx, |project, cx| { + project.open_unstaged_diff(buffer.clone(), 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| { + window.focus(&editor.focus_handle(cx), cx); + }); + + let fake_server = fake_servers.next().await.unwrap(); + + (project, editor, cx, fake_server) +} + +fn refresh_editor_actions(cx: &mut VisualTestContext) { + cx.executor().run_until_parked(); + cx.update(|window, cx| { + let _ = window.draw(cx); + }); +} + +fn lsp_line_edit(start_line: u32, end_line: u32, text: &str) -> lsp::TextEdit { + lsp::TextEdit::new( + lsp::Range::new( + lsp::Position::new(start_line, 0), + lsp::Position::new(end_line, 0), + ), + text.to_string(), + ) +} + +async fn assert_range_format_merge( + cx: &mut TestAppContext, + head_content: &str, + buffer_text: &str, + responses: Vec>, + expected_text: &str, + description: &str, +) { + let expected_requests = responses.len(); + let (project, editor, cx, fake_server) = + setup_range_format_test_with_git(cx, head_content, head_content).await; + + update_test_language_settings(cx, &|settings| { + settings.defaults.format_on_save = Some(FormatOnSave::Modifications); + }); + + editor.update_in(cx, |editor, window, cx| { + editor.set_text(buffer_text, window, cx); + }); + cx.run_until_parked(); + assert!(cx.read(|cx| editor.is_dirty(cx))); + + let request_count = Arc::new(AtomicUsize::new(0)); + let responses = Arc::new(responses); + let mut responded_rx = + fake_server.set_request_handler::({ + let request_count = request_count.clone(); + move |params, _| { + let count = request_count.fetch_add(1, atomic::Ordering::SeqCst); + let responses = responses.clone(); + async move { + assert_eq!( + params.text_document.uri, + lsp::Uri::from_file_path(path!("/project/file.rs")).unwrap() + ); + match responses.get(count) { + Some(edits) => Ok(Some(edits.clone())), + None => panic!("unexpected range formatting request #{}", count + 1), + } + } + } + }); + + let save = editor + .update_in(cx, |editor, window, cx| { + editor.save( + SaveOptions { + format: true, + autosave: false, + force_format: false, + }, + project.clone(), + window, + cx, + ) + }) + .unwrap(); + for _ in 0..expected_requests { + responded_rx.next().await; + } + save.await; + + assert_eq!( + request_count.load(atomic::Ordering::SeqCst), + expected_requests, + "{description}" + ); + assert_eq!( + editor.update(cx, |editor, cx| editor.text(cx)), + expected_text, + "{description}" + ); + assert!(!cx.read(|cx| editor.is_dirty(cx))); +} diff --git a/crates/project/src/lsp_store.rs b/crates/project/src/lsp_store.rs index 0c06cbf08d..c152a8832e 100644 --- a/crates/project/src/lsp_store.rs +++ b/crates/project/src/lsp_store.rs @@ -2424,9 +2424,20 @@ impl LocalLspStore { anyhow::Ok(()) })??; - let mut edits = None; + let mut edits = None::>; for range in lsp_ranges { - if let Some(mut edit) = language_server + if edits.as_ref().is_some_and(|edits| { + edits + .iter() + .any(|kept| kept.range.start <= range.start && range.end <= kept.range.end) + }) { + log::debug!( + "language server {}: skipping range formatting request for {range:?}: an earlier response already covers it", + language_server.name() + ); + continue; + } + if let Some(range_edits) = language_server .request::( lsp::DocumentRangeFormattingParams { text_document: text_document.clone(), @@ -2439,7 +2450,50 @@ impl LocalLspStore { .await .into_response()? { - edits.get_or_insert_with(Vec::new).append(&mut edit); + let kept_edits = edits.get_or_insert_with(Vec::new); + let mut new_edits = Vec::new(); + let mut overlaps_earlier_response = false; + for edit in range_edits { + let is_duplicate = kept_edits + .iter() + .any(|kept| kept.range == edit.range && kept.new_text == edit.new_text); + if is_duplicate { + continue; + } + if kept_edits + .iter() + .any(|kept| lsp_ranges_conflict(&edit.range, &kept.range)) + { + overlaps_earlier_response = true; + break; + } + new_edits.push(edit); + } + if overlaps_earlier_response { + log::debug!( + "language server {} returned range formatting edits for {range:?} that overlap an earlier response and differ; dropping the whole response", + language_server.name() + ); + continue; + } + let response_start = kept_edits.len(); + let mut dropped_within_response = 0_usize; + for edit in new_edits { + if kept_edits[response_start..] + .iter() + .any(|kept| lsp_ranges_overlap(&edit.range, &kept.range)) + { + dropped_within_response += 1; + continue; + } + kept_edits.push(edit); + } + if dropped_within_response > 0 { + log::debug!( + "language server {} returned overlapping edits within one range formatting response for {range:?}; dropped {dropped_within_response} later edit(s)", + language_server.name() + ); + } } } edits @@ -3381,7 +3435,7 @@ impl LocalLspStore { }) .collect::>(); - lsp_edits.sort_unstable_by_key(|(range, _)| (range.start, range.end)); + lsp_edits.sort_by_key(|(range, _)| (range.start, range.end)); let mut lsp_edits = lsp_edits.into_iter().peekable(); let mut edits = Vec::new(); @@ -13680,6 +13734,18 @@ fn server_capabilities_support_range_formatting(capabilities: &lsp::ServerCapabi ) } +fn lsp_ranges_conflict(a: &lsp::Range, b: &lsp::Range) -> bool { + if a.start == a.end || b.start == b.end { + a.start <= b.end && b.start <= a.end + } else { + lsp_ranges_overlap(a, b) + } +} + +fn lsp_ranges_overlap(a: &lsp::Range, b: &lsp::Range) -> bool { + a.start < b.end && b.start < a.end +} + fn subscribe_to_binary_statuses( languages: &Arc, cx: &mut Context<'_, LspStore>,