Yehowshua/ZIm
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix handling of selection ranges for format selections in multibuffer (#22929)

Browse source 
Before this change it was using the same multibuffer point ranges in
every buffer, which only worked correctly for singleton buffers.

Release Notes:

- Fixed handling of selection ranges when formatting selections within a
multibuffer.

---------

Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
This commit is contained in:
Michael Sloan 2025-01-09 17:17:04 -07:00 • committed by GitHub
parent 29aa291d28
commit 685dd77d97
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 170 additions and 119 deletions
Download patch file Download diff file Expand all files Collapse all files

14
crates/collab/src/tests/integration_tests.rs
View file

@ -27,10 +27,10 @@ use language::{
};
use lsp::LanguageServerId;
use parking_lot::Mutex;
use project::lsp_store::FormatTarget;
use project::{
lsp_store::FormatTrigger, search::SearchQuery, search::SearchResult, DiagnosticSummary,
HoverBlockKind, Project, ProjectPath,
lsp_store::{FormatTrigger, LspFormatTarget},
search::{SearchQuery, SearchResult},
DiagnosticSummary, HoverBlockKind, Project, ProjectPath,
};
use rand::prelude::*;
use serde_json::json;
@ -4400,9 +4400,9 @@ async fn test_formatting_buffer(
.update(cx_b, |project, cx| {
project.format(
HashSet::from_iter([buffer_b.clone()]),
LspFormatTarget::Buffers,
true,
FormatTrigger::Save,
FormatTarget::Buffer,
cx,
)
})
@ -4436,9 +4436,9 @@ async fn test_formatting_buffer(
.update(cx_b, |project, cx| {
project.format(
HashSet::from_iter([buffer_b.clone()]),
LspFormatTarget::Buffers,
true,
FormatTrigger::Save,
FormatTarget::Buffer,
cx,
)
})
@ -4546,9 +4546,9 @@ async fn test_prettier_formatting_buffer(
.update(cx_b, |project, cx| {
project.format(
HashSet::from_iter([buffer_b.clone()]),
LspFormatTarget::Buffers,
true,
FormatTrigger::Save,
FormatTarget::Buffer,
cx,
)
})
@ -4566,9 +4566,9 @@ async fn test_prettier_formatting_buffer(
.update(cx_a, |project, cx| {
project.format(
HashSet::from_iter([buffer_a.clone()]),
LspFormatTarget::Buffers,
true,
FormatTrigger::Manual,
FormatTarget::Buffer,
cx,
)
})

6
crates/collab/src/tests/remote_editing_collaboration_tests.rs
View file

@ -16,7 +16,7 @@ use language::{
};
use node_runtime::NodeRuntime;
use project::{
lsp_store::{FormatTarget, FormatTrigger},
lsp_store::{FormatTrigger, LspFormatTarget},
ProjectPath,
};
use remote::SshRemoteClient;
@ -472,9 +472,9 @@ async fn test_ssh_collaboration_formatting_with_prettier(
.update(cx_b, |project, cx| {
project.format(
HashSet::from_iter([buffer_b.clone()]),
LspFormatTarget::Buffers,
true,
FormatTrigger::Save,
FormatTarget::Buffer,
cx,
)
})
@ -509,9 +509,9 @@ async fn test_ssh_collaboration_formatting_with_prettier(
.update(cx_a, |project, cx| {
project.format(
HashSet::from_iter([buffer_a.clone()]),
LspFormatTarget::Buffers,
true,
FormatTrigger::Manual,
FormatTarget::Buffer,
cx,
)
})

44
crates/editor/src/editor.rs
View file

@ -129,7 +129,7 @@ use multi_buffer::{
};
use project::{
buffer_store::BufferChangeSet,
lsp_store::{FormatTarget, FormatTrigger, OpenLspBufferHandle},
lsp_store::{FormatTrigger, LspFormatTarget, OpenLspBufferHandle},
project_settings::{GitGutterSetting, ProjectSettings},
CodeAction, Completion, CompletionIntent, DocumentHighlight, InlayHint, Location, LocationLink,
LspStore, Project, ProjectItem, ProjectTransaction, TaskSourceKind,
@ -986,6 +986,11 @@ impl InlayHintRefreshReason {
}
}
pub enum FormatTarget {
Buffers,
Ranges(Vec<Range<MultiBufferPoint>>),
}
pub(crate) struct FocusedBlock {
id: BlockId,
focus_handle: WeakFocusHandle,
@ -10237,7 +10242,7 @@ impl Editor {
None => return None,
};
Some(self.perform_format(project, FormatTrigger::Manual, FormatTarget::Buffer, cx))
Some(self.perform_format(project, FormatTrigger::Manual, FormatTarget::Buffers, cx))
}
fn format_selections(
@ -10250,17 +10255,18 @@ impl Editor {
None => return None,
};
let selections = self
let ranges = self
.selections
.all_adjusted(cx)
.into_iter()
.map(|selection| selection.range())
.filter(|s| !s.is_empty())
.collect_vec();
Some(self.perform_format(
project,
FormatTrigger::Manual,
FormatTarget::Ranges(selections),
FormatTarget::Ranges(ranges),
cx,
))
}
@ -10272,15 +10278,41 @@ impl Editor {
target: FormatTarget,
cx: &mut ViewContext<Self>,
) -> Task<Result<()>> {
let buffer = self.buffer().clone();
let buffer = self.buffer.clone();
let (buffers, target) = match target {
FormatTarget::Buffers => {
let mut buffers = buffer.read(cx).all_buffers();
if trigger == FormatTrigger::Save {
buffers.retain(|buffer| buffer.read(cx).is_dirty());
}
(buffers, LspFormatTarget::Buffers)
}
FormatTarget::Ranges(selection_ranges) => {
let multi_buffer = buffer.read(cx);
let snapshot = multi_buffer.read(cx);
let mut buffers = HashSet::default();
let mut buffer_id_to_ranges: BTreeMap<BufferId, Vec<Range<text::Anchor>>> =
BTreeMap::new();
for selection_range in selection_ranges {
for (excerpt, buffer_range) in snapshot.range_to_buffer_ranges(selection_range)
{
let buffer_id = excerpt.buffer_id();
let start = excerpt.buffer().anchor_before(buffer_range.start);
let end = excerpt.buffer().anchor_after(buffer_range.end);
buffers.insert(multi_buffer.buffer(buffer_id).unwrap());
buffer_id_to_ranges
.entry(buffer_id)
.and_modify(|buffer_ranges| buffer_ranges.push(start..end))
.or_insert_with(|| vec![start..end]);
}
}
(buffers, LspFormatTarget::Ranges(buffer_id_to_ranges))
}
};
let mut timeout = cx.background_executor().timer(FORMAT_TIMEOUT).fuse();
let format = project.update(cx, |project, cx| {
project.format(buffers, true, trigger, target, cx)
project.format(buffers, target, true, trigger, cx)
});
cx.spawn(|_, mut cx| async move {

8
crates/editor/src/editor_tests.rs
View file

@ -7380,7 +7380,7 @@ async fn test_document_format_manual_trigger(cx: &mut gpui::TestAppContext) {
editor.perform_format(
project.clone(),
FormatTrigger::Manual,
FormatTarget::Buffer,
FormatTarget::Buffers,
cx,
)
})
@ -7418,7 +7418,7 @@ async fn test_document_format_manual_trigger(cx: &mut gpui::TestAppContext) {
});
let format = editor
.update(cx, |editor, cx| {
editor.perform_format(project, FormatTrigger::Manual, FormatTarget::Buffer, cx)
editor.perform_format(project, FormatTrigger::Manual, FormatTarget::Buffers, cx)
})
.unwrap();
cx.executor().advance_clock(super::FORMAT_TIMEOUT);
@ -11293,7 +11293,7 @@ async fn test_document_format_with_prettier(cx: &mut gpui::TestAppContext) {
editor.perform_format(
project.clone(),
FormatTrigger::Manual,
FormatTarget::Buffer,
FormatTarget::Buffers,
cx,
)
})
@ -11312,7 +11312,7 @@ async fn test_document_format_with_prettier(cx: &mut gpui::TestAppContext) {
editor.perform_format(
project.clone(),
FormatTrigger::Manual,
FormatTarget::Buffer,
FormatTarget::Buffers,
cx,
)
});

7
crates/editor/src/items.rs
View file

@ -2,8 +2,8 @@ use crate::{
editor_settings::SeedQuerySetting,
persistence::{SerializedEditor, DB},
scroll::ScrollAnchor,
Anchor, Autoscroll, Editor, EditorEvent, EditorSettings, ExcerptId, ExcerptRange, MultiBuffer,
MultiBufferSnapshot, NavigationData, SearchWithinRange, ToPoint as _,
Anchor, Autoscroll, Editor, EditorEvent, EditorSettings, ExcerptId, ExcerptRange, FormatTarget,
MultiBuffer, MultiBufferSnapshot, NavigationData, SearchWithinRange, ToPoint as _,
};
use anyhow::{anyhow, Context as _, Result};
use collections::HashSet;
@ -29,7 +29,6 @@ use rpc::proto::{self, update_view, PeerId};
use settings::Settings;
use workspace::item::{Dedup, ItemSettings, SerializableItem, TabContentParams};
use project::lsp_store::FormatTarget;
use std::{
any::TypeId,
borrow::Cow,
@ -756,7 +755,7 @@ impl Item for Editor {
editor.perform_format(
project.clone(),
FormatTrigger::Save,
FormatTarget::Buffer,
FormatTarget::Buffers,
cx,
)
})?

179
crates/project/src/lsp_store.rs
View file

@ -36,11 +36,11 @@ use language::{
},
markdown, point_to_lsp, prepare_completion_documentation,
proto::{deserialize_anchor, deserialize_version, serialize_anchor, serialize_version},
range_from_lsp, Bias, Buffer, BufferSnapshot, CachedLspAdapter, CodeLabel, Diagnostic,
DiagnosticEntry, DiagnosticSet, Diff, Documentation, File as _, Language, LanguageName,
LanguageRegistry, LanguageServerBinaryStatus, LanguageToolchainStore, LocalFile, LspAdapter,
LspAdapterDelegate, Patch, PointUtf16, TextBufferSnapshot, ToOffset, ToPointUtf16, Transaction,
Unclipped,
range_from_lsp, range_to_lsp, Bias, Buffer, BufferSnapshot, CachedLspAdapter, CodeLabel,
Diagnostic, DiagnosticEntry, DiagnosticSet, Diff, Documentation, File as _, Language,
LanguageName, LanguageRegistry, LanguageServerBinaryStatus, LanguageToolchainStore, LocalFile,
LspAdapter, LspAdapterDelegate, Patch, PointUtf16, TextBufferSnapshot, ToOffset, ToPointUtf16,
Transaction, Unclipped,
};
use lsp::{
notification::DidRenameFiles, CodeActionKind, CompletionContext, DiagnosticSeverity,
@ -77,7 +77,7 @@ use std::{
sync::Arc,
time::{Duration, Instant},
};
use text::{Anchor, BufferId, LineEnding, Point, Selection};
use text::{Anchor, BufferId, LineEnding, OffsetRangeExt};
use util::{
debug_panic, defer, maybe, merge_json_value_into, post_inc, ResultExt, TryFutureExt as _,
};
@ -100,18 +100,9 @@ pub enum FormatTrigger {
Manual,
}
pub enum FormatTarget {
Buffer,
Ranges(Vec<Selection<Point>>),
}
impl FormatTarget {
pub fn as_selections(&self) -> Option<&[Selection<Point>]> {
match self {
FormatTarget::Buffer => None,
FormatTarget::Ranges(selections) => Some(selections.as_slice()),
}
}
pub enum LspFormatTarget {
Buffers,
Ranges(BTreeMap<BufferId, Vec<Range<Anchor>>>),
}
// proto::RegisterBufferWithLanguageServer {}
@ -1075,9 +1066,9 @@ impl LocalLspStore {
async fn format_locally(
lsp_store: WeakModel<LspStore>,
mut buffers: Vec<FormattableBuffer>,
target: &LspFormatTarget,
push_to_history: bool,
trigger: FormatTrigger,
target: FormatTarget,
mut cx: AsyncAppContext,
) -> anyhow::Result<ProjectTransaction> {
// Do not allow multiple concurrent formatting requests for the
@ -1182,7 +1173,7 @@ impl LocalLspStore {
// Except for code actions, which are applied with all connected language servers.
let primary_language_server =
primary_adapter_and_server.map(|(_adapter, server)| server.clone());
let server_and_buffer = primary_language_server
let primary_server_and_path = primary_language_server
.as_ref()
.zip(buffer.abs_path.as_ref());
@ -1192,6 +1183,16 @@ impl LocalLspStore {
.clone()
})?;
let ranges = match target {
LspFormatTarget::Buffers => None,
LspFormatTarget::Ranges(ranges) => {
let Some(ranges) = ranges.get(&buffer.id) else {
return Err(anyhow!("No format ranges provided for buffer"));
};
Some(ranges)
}
};
let mut format_operations: Vec<FormatOperation> = vec![];
{
match trigger {
@ -1208,10 +1209,10 @@ impl LocalLspStore {
if prettier_settings.allowed {
Self::perform_format(
&Formatter::Prettier,
&target,
server_and_buffer,
lsp_store.clone(),
buffer,
ranges,
primary_server_and_path,
lsp_store.clone(),
&settings,
&adapters_and_servers,
push_to_history,
@ -1222,10 +1223,10 @@ impl LocalLspStore {
} else {
Self::perform_format(
&Formatter::LanguageServer { name: None },
&target,
server_and_buffer,
lsp_store.clone(),
buffer,
ranges,
primary_server_and_path,
lsp_store.clone(),
&settings,
&adapters_and_servers,
push_to_history,
@ -1244,10 +1245,10 @@ impl LocalLspStore {
for formatter in formatters.as_ref() {
let diff = Self::perform_format(
formatter,
&target,
server_and_buffer,
lsp_store.clone(),
buffer,
ranges,
primary_server_and_path,
lsp_store.clone(),
&settings,
&adapters_and_servers,
push_to_history,
@ -1268,10 +1269,10 @@ impl LocalLspStore {
for formatter in formatters.as_ref() {
let diff = Self::perform_format(
formatter,
&target,
server_and_buffer,
lsp_store.clone(),
buffer,
ranges,
primary_server_and_path,
lsp_store.clone(),
&settings,
&adapters_and_servers,
push_to_history,
@ -1294,10 +1295,10 @@ impl LocalLspStore {
if prettier_settings.allowed {
Self::perform_format(
&Formatter::Prettier,
&target,
server_and_buffer,
lsp_store.clone(),
buffer,
ranges,
primary_server_and_path,
lsp_store.clone(),
&settings,
&adapters_and_servers,
push_to_history,
@ -1313,10 +1314,10 @@ impl LocalLspStore {
};
Self::perform_format(
&formatter,
&target,
server_and_buffer,
lsp_store.clone(),
buffer,
ranges,
primary_server_and_path,
lsp_store.clone(),
&settings,
&adapters_and_servers,
push_to_history,
@ -1336,10 +1337,10 @@ impl LocalLspStore {
// format with formatter
let diff = Self::perform_format(
formatter,
&target,
server_and_buffer,
lsp_store.clone(),
buffer,
ranges,
primary_server_and_path,
lsp_store.clone(),
&settings,
&adapters_and_servers,
push_to_history,
@ -1408,10 +1409,10 @@ impl LocalLspStore {
#[allow(clippy::too_many_arguments)]
async fn perform_format(
formatter: &Formatter,
format_target: &FormatTarget,
primary_server_and_buffer: Option<(&Arc<LanguageServer>, &PathBuf)>,
lsp_store: WeakModel<LspStore>,
buffer: &FormattableBuffer,
ranges: Option<&Vec<Range<Anchor>>>,
primary_server_and_path: Option<(&Arc<LanguageServer>, &PathBuf)>,
lsp_store: WeakModel<LspStore>,
settings: &LanguageSettings,
adapters_and_servers: &[(Arc<CachedLspAdapter>, Arc<LanguageServer>)],
push_to_history: bool,
@ -1420,7 +1421,7 @@ impl LocalLspStore {
) -> Result<Option<FormatOperation>, anyhow::Error> {
let result = match formatter {
Formatter::LanguageServer { name } => {
if let Some((language_server, buffer_abs_path)) = primary_server_and_buffer {
if let Some((language_server, buffer_abs_path)) = primary_server_and_path {
let language_server = if let Some(name) = name {
adapters_and_servers
.iter()
@ -1432,8 +1433,19 @@ impl LocalLspStore {
language_server
};
match format_target {
FormatTarget::Buffer => Some(FormatOperation::Lsp(
let result = if let Some(ranges) = ranges {
Self::format_ranges_via_lsp(
&lsp_store,
&buffer,
ranges,
buffer_abs_path,
language_server,
settings,
cx,
)
.await
.context("failed to format ranges via language server")?
} else {
Self::format_via_lsp(
&lsp_store,
&buffer.handle,
@ -1443,22 +1455,10 @@ impl LocalLspStore {
cx,
)
.await
.context("failed to format via language server")?,
)),
FormatTarget::Ranges(selections) => Some(FormatOperation::Lsp(
Self::format_range_via_lsp(
&lsp_store,
&buffer.handle,
selections.as_slice(),
buffer_abs_path,
language_server,
settings,
cx,
)
.await
.context("failed to format ranges via language server")?,
)),
}
.context("failed to format via language server")?
};
Some(FormatOperation::Lsp(result))
} else {
None
}
@ -1500,10 +1500,10 @@ impl LocalLspStore {
anyhow::Ok(result)
}
pub async fn format_range_via_lsp(
pub async fn format_ranges_via_lsp(
this: &WeakModel<LspStore>,
buffer: &Model<Buffer>,
selections: &[Selection<Point>],
buffer: &FormattableBuffer,
ranges: &Vec<Range<Anchor>>,
abs_path: &Path,
language_server: &Arc<LanguageServer>,
settings: &LanguageSettings,
@ -1523,14 +1523,23 @@ impl LocalLspStore {
let text_document = lsp::TextDocumentIdentifier::new(uri);
let lsp_edits = {
let ranges = selections.into_iter().map(|s| {
let start = lsp::Position::new(s.start.row, s.start.column);
let end = lsp::Position::new(s.end.row, s.end.column);
lsp::Range::new(start, end)
});
let mut lsp_ranges = Vec::new();
this.update(cx, |_this, cx| {
// TODO(#22930): In the case of formatting multibuffer selections, this buffer may
// not have been sent to the language server. This seems like a fairly systemic
// issue, though, the resolution probably is not specific to formatting.
//
// TODO: Instead of using current snapshot, should use the latest snapshot sent to
// LSP.
let snapshot = buffer.handle.read(cx).snapshot();
for range in ranges {
lsp_ranges.push(range_to_lsp(range.to_point_utf16(&snapshot))?);
}
anyhow::Ok(())
})??;
let mut edits = None;
for range in ranges {
for range in lsp_ranges {
if let Some(mut edit) = language_server
.request::<lsp::request::RangeFormatting>(lsp::DocumentRangeFormattingParams {
text_document: text_document.clone(),
@ -1549,7 +1558,7 @@ impl LocalLspStore {
if let Some(lsp_edits) = lsp_edits {
this.update(cx, |this, cx| {
this.as_local_mut().unwrap().edits_from_lsp(
buffer,
&buffer.handle,
lsp_edits,
language_server.server_id(),
None,
@ -2734,6 +2743,7 @@ impl LocalLspStore {
#[derive(Debug)]
pub struct FormattableBuffer {
id: BufferId,
handle: Model<Buffer>,
abs_path: Option<PathBuf>,
env: Option<HashMap<String, String>>,
@ -6906,27 +6916,27 @@ impl LspStore {
pub fn format(
&mut self,
buffers: HashSet<Model<Buffer>>,
target: LspFormatTarget,
push_to_history: bool,
trigger: FormatTrigger,
target: FormatTarget,
cx: &mut ModelContext<Self>,
) -> Task<anyhow::Result<ProjectTransaction>> {
if let Some(_) = self.as_local() {
let buffers_with_paths = buffers
let buffers = buffers
.into_iter()
.map(|buffer_handle| {
let buffer = buffer_handle.read(cx);
let buffer_abs_path = File::from_dyn(buffer.file())
.and_then(|file| file.as_local().map(|f| f.abs_path(cx)));
(buffer_handle, buffer_abs_path)
(buffer_handle, buffer_abs_path, buffer.remote_id())
})
.collect::<Vec<_>>();
cx.spawn(move |lsp_store, mut cx| async move {
let mut formattable_buffers = Vec::with_capacity(buffers_with_paths.len());
let mut formattable_buffers = Vec::with_capacity(buffers.len());
for (handle, abs_path) in buffers_with_paths {
for (handle, abs_path, id) in buffers {
let env = lsp_store
.update(&mut cx, |lsp_store, cx| {
lsp_store.environment_for_buffer(&handle, cx)
@ -6934,6 +6944,7 @@ impl LspStore {
.await;
formattable_buffers.push(FormattableBuffer {
id,
handle,
abs_path,
env,
@ -6943,9 +6954,9 @@ impl LspStore {
let result = LocalLspStore::format_locally(
lsp_store.clone(),
formattable_buffers,
&target,
push_to_history,
trigger,
target,
cx.clone(),
)
.await;
@ -6956,6 +6967,14 @@ impl LspStore {
result
})
} else if let Some((client, project_id)) = self.upstream_client() {
// Don't support formatting ranges via remote
match target {
LspFormatTarget::Buffers => {}
LspFormatTarget::Ranges(_) => {
return Task::ready(Ok(ProjectTransaction::default()));
}
}
let buffer_store = self.buffer_store();
cx.spawn(move |lsp_store, mut cx| async move {
let result = client
@ -7005,7 +7024,7 @@ impl LspStore {
buffers.insert(this.buffer_store.read(cx).get_existing(buffer_id)?);
}
let trigger = FormatTrigger::from_proto(envelope.payload.trigger);
anyhow::Ok(this.format(buffers, false, trigger, FormatTarget::Buffer, cx))
anyhow::Ok(this.format(buffers, LspFormatTarget::Buffers, false, trigger, cx))
})??;
let project_transaction = format.await?;

5
crates/project/src/project.rs
View file

@ -59,6 +59,7 @@ use lsp::{
LanguageServerId, LanguageServerName, MessageActionItem,
};
use lsp_command::*;
use lsp_store::LspFormatTarget;
use node_runtime::NodeRuntime;
use parking_lot::Mutex;
pub use prettier_store::PrettierStore;
@ -2632,13 +2633,13 @@ impl Project {
pub fn format(
&mut self,
buffers: HashSet<Model<Buffer>>,
target: LspFormatTarget,
push_to_history: bool,
trigger: lsp_store::FormatTrigger,
target: lsp_store::FormatTarget,
cx: &mut ModelContext<Project>,
) -> Task<anyhow::Result<ProjectTransaction>> {
self.lsp_store.update(cx, |lsp_store, cx| {
lsp_store.format(buffers, push_to_history, trigger, target, cx)
lsp_store.format(buffers, target, push_to_history, trigger, cx)
})
}