From dab9c4179932f40792f8e1da0a11c0a09c89d41c Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Mon, 10 Feb 2025 16:18:14 -0600 Subject: [PATCH] Fix formatters not running in order (#24584) Previously, if multiple formatters were specified for the same language, they would be run in parallel on the state of the file, and then all edits would be applied. This lead to incorrect output with many unwanted artifacts. This PR refactors the formatting code to clean it up, and ensure results from previous formatters are passed in to subsequent formatters. Closes #15544 Release Notes: - Fixed an issue where when running multiple formatters they would be ran in parallel rather than sequentially, leading to unwanted artifacts and incorrect output. --------- Co-authored-by: Conrad --- crates/project/src/lsp_store.rs | 483 ++++++++++++-------------------- 1 file changed, 184 insertions(+), 299 deletions(-) diff --git a/crates/project/src/lsp_store.rs b/crates/project/src/lsp_store.rs index df0c833ab019cd750e0d175e3af5f23600b5c1b6..7fbb781f0f8a12791c8e324c3f1667bee12368d7 100644 --- a/crates/project/src/lsp_store.rs +++ b/crates/project/src/lsp_store.rs @@ -76,7 +76,7 @@ use std::{ sync::Arc, time::{Duration, Instant}, }; -use text::{Anchor, BufferId, LineEnding, OffsetRangeExt}; +use text::{Anchor, BufferId, LineEnding, OffsetRangeExt, TransactionId}; use util::{ debug_panic, defer, maybe, merge_json_value_into, paths::SanitizedPath, post_inc, ResultExt, TryFutureExt as _, @@ -1098,7 +1098,6 @@ impl LocalLspStore { async fn format_locally( lsp_store: WeakEntity, mut buffers: Vec, - target: &LspFormatTarget, push_to_history: bool, trigger: FormatTrigger, mut cx: AsyncApp, @@ -1131,25 +1130,16 @@ impl LocalLspStore { let mut project_transaction = ProjectTransaction::default(); for buffer in &buffers { - let (primary_adapter_and_server, adapters_and_servers) = - lsp_store.update(&mut cx, |lsp_store, cx| { - let buffer = buffer.handle.read(cx); - - let adapters_and_servers = lsp_store - .as_local() - .unwrap() - .language_servers_for_buffer(buffer, cx) - .map(|(adapter, lsp)| (adapter.clone(), lsp.clone())) - .collect::>(); - - let primary_adapter = lsp_store - .as_local() - .unwrap() - .primary_language_server_for_buffer(buffer, cx) - .map(|(adapter, lsp)| (adapter.clone(), lsp.clone())); + let adapters_and_servers = lsp_store.update(&mut cx, |lsp_store, cx| { + let buffer = buffer.handle.read(cx); - (primary_adapter, adapters_and_servers) - })?; + lsp_store + .as_local() + .unwrap() + .language_servers_for_buffer(buffer, cx) + .map(|(adapter, lsp)| (adapter.clone(), lsp.clone())) + .collect::>() + })?; let settings = buffer.handle.update(&mut cx, |buffer, cx| { language_settings(buffer.language().map(|l| l.name()), buffer.file(), cx) @@ -1182,6 +1172,8 @@ impl LocalLspStore { buffer.end_transaction(cx) })?; + let initial_transaction_id = whitespace_transaction_id; + // Apply the `code_actions_on_format` before we run the formatter. let code_actions = deserialize_code_actions(&settings.code_actions_on_format); #[allow(clippy::nonminimal_bool)] @@ -1200,278 +1192,99 @@ impl LocalLspStore { .await?; } - // Apply language-specific formatting using either the primary language server - // or external command. - // 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 primary_server_and_path = primary_language_server - .as_ref() - .zip(buffer.abs_path.as_ref()); - let prettier_settings = buffer.handle.read_with(&cx, |buffer, cx| { language_settings(buffer.language().map(|l| l.name()), buffer.file(), cx) .prettier .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 = vec![]; - { - match trigger { - FormatTrigger::Save => { - match &settings.format_on_save { - FormatOnSave::Off => { - // nothing - } - FormatOnSave::On => { - match &settings.formatter { - SelectedFormatter::Auto => { - // do the auto-format: prefer prettier, fallback to primary language server - let diff = { - if prettier_settings.allowed { - Self::perform_format( - &Formatter::Prettier, - buffer, - ranges, - primary_server_and_path, - lsp_store.clone(), - &settings, - &adapters_and_servers, - push_to_history, - &mut project_transaction, - &mut cx, - ) - .await - } else { - Self::perform_format( - &Formatter::LanguageServer { name: None }, - buffer, - ranges, - primary_server_and_path, - lsp_store.clone(), - &settings, - &adapters_and_servers, - push_to_history, - &mut project_transaction, - &mut cx, - ) - .await - } - }?; - - if let Some(op) = diff { - format_operations.push(op); - } - } - SelectedFormatter::List(formatters) => { - for formatter in formatters.as_ref() { - let diff = Self::perform_format( - formatter, - buffer, - ranges, - primary_server_and_path, - lsp_store.clone(), - &settings, - &adapters_and_servers, - push_to_history, - &mut project_transaction, - &mut cx, - ) - .await?; - if let Some(op) = diff { - format_operations.push(op); - } - - // format with formatter - } - } - } - } - FormatOnSave::List(formatters) => { - for formatter in formatters.as_ref() { - let diff = Self::perform_format( - formatter, - buffer, - ranges, - primary_server_and_path, - lsp_store.clone(), - &settings, - &adapters_and_servers, - push_to_history, - &mut project_transaction, - &mut cx, - ) - .await?; - if let Some(op) = diff { - format_operations.push(op); - } - } - } - } - } - FormatTrigger::Manual => { - match &settings.formatter { - SelectedFormatter::Auto => { - // do the auto-format: prefer prettier, fallback to primary language server - let diff = { - if prettier_settings.allowed { - Self::perform_format( - &Formatter::Prettier, - buffer, - ranges, - primary_server_and_path, - lsp_store.clone(), - &settings, - &adapters_and_servers, - push_to_history, - &mut project_transaction, - &mut cx, - ) - .await - } else { - let formatter = Formatter::LanguageServer { - name: primary_language_server - .as_ref() - .map(|server| server.name().to_string()), - }; - Self::perform_format( - &formatter, - buffer, - ranges, - primary_server_and_path, - lsp_store.clone(), - &settings, - &adapters_and_servers, - push_to_history, - &mut project_transaction, - &mut cx, - ) - .await - } - }?; - - if let Some(op) = diff { - format_operations.push(op) - } - } - SelectedFormatter::List(formatters) => { - for formatter in formatters.as_ref() { - // format with formatter - let diff = Self::perform_format( - formatter, - buffer, - ranges, - primary_server_and_path, - lsp_store.clone(), - &settings, - &adapters_and_servers, - push_to_history, - &mut project_transaction, - &mut cx, - ) - .await?; - if let Some(op) = diff { - format_operations.push(op); - } - } + let formatters = match (trigger, &settings.format_on_save) { + (FormatTrigger::Save, FormatOnSave::Off) => &[], + (FormatTrigger::Save, FormatOnSave::List(formatters)) => formatters.as_ref(), + (FormatTrigger::Manual, _) | (FormatTrigger::Save, FormatOnSave::On) => { + match &settings.formatter { + SelectedFormatter::Auto => { + if prettier_settings.allowed { + std::slice::from_ref(&Formatter::Prettier) + } else { + std::slice::from_ref(&Formatter::LanguageServer { name: None }) } } + SelectedFormatter::List(formatter_list) => formatter_list.as_ref(), } } - } - - buffer.handle.update(&mut cx, |b, cx| { - // If the buffer had its whitespace formatted and was edited while the language-specific - // formatting was being computed, avoid applying the language-specific formatting, because - // it can't be grouped with the whitespace formatting in the undo history. - if let Some(transaction_id) = whitespace_transaction_id { - if b.peek_undo_stack() - .map_or(true, |e| e.transaction_id() != transaction_id) - { - format_operations.clear(); - } - } - - // Apply any language-specific formatting, and group the two formatting operations - // in the buffer's undo history. - for operation in format_operations { - match operation { - FormatOperation::Lsp(edits) => { - b.edit(edits, None, cx); - } - FormatOperation::External(diff) => { - b.apply_diff(diff, cx); - } - FormatOperation::Prettier(diff) => { - b.apply_diff(diff, cx); - } - } - - if let Some(transaction_id) = whitespace_transaction_id { - b.group_until_transaction(transaction_id); - } else if let Some(transaction) = project_transaction.0.get(&buffer.handle) { - b.group_until_transaction(transaction.id) - } - } - - if let Some(transaction) = b.finalize_last_transaction().cloned() { - if !push_to_history { - b.forget_transaction(transaction.id); - } - project_transaction - .0 - .insert(buffer.handle.clone(), transaction); - } - })?; + }; + Self::execute_formatters( + lsp_store.clone(), + formatters, + buffer, + &settings, + &adapters_and_servers, + push_to_history, + initial_transaction_id, + &mut project_transaction, + &mut cx, + ) + .await?; } Ok(project_transaction) } #[allow(clippy::too_many_arguments)] - async fn perform_format( - formatter: &Formatter, - buffer: &FormattableBuffer, - ranges: Option<&Vec>>, - primary_server_and_path: Option<(&Arc, &PathBuf)>, + async fn execute_formatters( lsp_store: WeakEntity, + formatters: &[Formatter], + buffer: &FormattableBuffer, settings: &LanguageSettings, adapters_and_servers: &[(Arc, Arc)], push_to_history: bool, - transaction: &mut ProjectTransaction, + mut initial_transaction_id: Option, + project_transaction: &mut ProjectTransaction, cx: &mut AsyncApp, - ) -> Result, anyhow::Error> { - let result = match formatter { - Formatter::LanguageServer { name } => { - if let Some((language_server, buffer_abs_path)) = primary_server_and_path { + ) -> anyhow::Result<()> { + let mut prev_transaction_id = initial_transaction_id; + + for formatter in formatters { + let operation = match formatter { + Formatter::LanguageServer { name } => { + let Some(language_server) = lsp_store.update(cx, |lsp_store, cx| { + lsp_store + .as_local() + .unwrap() + .primary_language_server_for_buffer(buffer.handle.read(cx), cx) + .map(|(_, lsp)| lsp.clone()) + })? + else { + continue; + }; + let Some(buffer_abs_path) = buffer.abs_path.as_ref() else { + continue; + }; + let language_server = if let Some(name) = name { adapters_and_servers .iter() .find_map(|(adapter, server)| { - adapter.name.0.as_ref().eq(name.as_str()).then_some(server) + adapter + .name + .0 + .as_ref() + .eq(name.as_str()) + .then_some(server.clone()) }) .unwrap_or(language_server) } else { language_server }; - let result = if let Some(ranges) = ranges { + let result = if let Some(ranges) = &buffer.ranges { Self::format_ranges_via_lsp( &lsp_store, - &buffer, + &buffer.handle, ranges, buffer_abs_path, - language_server, + &language_server, settings, cx, ) @@ -1482,7 +1295,7 @@ impl LocalLspStore { &lsp_store, &buffer.handle, buffer_abs_path, - language_server, + &language_server, settings, cx, ) @@ -1491,51 +1304,114 @@ impl LocalLspStore { }; Some(FormatOperation::Lsp(result)) - } else { + } + Formatter::Prettier => { + let prettier = lsp_store.update(cx, |lsp_store, _cx| { + lsp_store.prettier_store().unwrap().downgrade() + })?; + prettier_store::format_with_prettier(&prettier, &buffer.handle, cx) + .await + .transpose()? + } + Formatter::External { command, arguments } => { + Self::format_via_external_command(buffer, command, arguments.as_deref(), cx) + .await + .context(format!( + "failed to format via external command {:?}", + command + ))? + .map(FormatOperation::External) + } + Formatter::CodeActions(code_actions) => { + let code_actions = deserialize_code_actions(code_actions); + if !code_actions.is_empty() { + Self::execute_code_actions_on_servers( + &lsp_store, + adapters_and_servers, + code_actions, + &buffer.handle, + push_to_history, + project_transaction, + cx, + ) + .await?; + let buf_transaction_id = + project_transaction.0.get(&buffer.handle).map(|t| t.id); + // NOTE: same logic as in buffer.handle.update below + if initial_transaction_id.is_none() { + initial_transaction_id = buf_transaction_id; + } + if buf_transaction_id.is_some() { + prev_transaction_id = buf_transaction_id; + } + } None } + }; + let Some(operation) = operation else { + continue; + }; + + let should_continue_formatting = buffer.handle.update(cx, |b, cx| { + // If a previous format succeeded and the buffer was edited while the language-specific + // formatting information for this format was being computed, avoid applying the + // language-specific formatting, because it can't be grouped with the previous formatting + // in the undo history. + let should_continue_formatting = match (prev_transaction_id, b.peek_undo_stack()) { + (Some(prev_transaction_id), Some(last_history_entry)) => { + let last_history_transaction_id = last_history_entry.transaction_id(); + let is_same_as_prev = last_history_transaction_id == prev_transaction_id; + is_same_as_prev + } + (Some(_), None) => false, + (_, _) => true, + }; + + if should_continue_formatting { + // Apply any language-specific formatting, and group the two formatting operations + // in the buffer's undo history. + let this_transaction_id = match operation { + FormatOperation::Lsp(edits) => b.edit(edits, None, cx), + FormatOperation::External(diff) => b.apply_diff(diff, cx), + FormatOperation::Prettier(diff) => b.apply_diff(diff, cx), + }; + if initial_transaction_id.is_none() { + initial_transaction_id = this_transaction_id; + } + if this_transaction_id.is_some() { + prev_transaction_id = this_transaction_id; + } + } + + if let Some(transaction_id) = initial_transaction_id { + b.group_until_transaction(transaction_id); + } else if let Some(transaction) = project_transaction.0.get(&buffer.handle) { + b.group_until_transaction(transaction.id) + } + return should_continue_formatting; + })?; + if !should_continue_formatting { + break; } - Formatter::Prettier => { - let prettier = lsp_store.update(cx, |lsp_store, _cx| { - lsp_store.prettier_store().unwrap().downgrade() - })?; - prettier_store::format_with_prettier(&prettier, &buffer.handle, cx) - .await - .transpose()? - } - Formatter::External { command, arguments } => { - Self::format_via_external_command(buffer, command, arguments.as_deref(), cx) - .await - .context(format!( - "failed to format via external command {:?}", - command - ))? - .map(FormatOperation::External) - } - Formatter::CodeActions(code_actions) => { - let code_actions = deserialize_code_actions(code_actions); - if !code_actions.is_empty() { - Self::execute_code_actions_on_servers( - &lsp_store, - adapters_and_servers, - code_actions, - &buffer.handle, - push_to_history, - transaction, - cx, - ) - .await?; + } + + buffer.handle.update(cx, |b, _cx| { + if let Some(transaction) = b.finalize_last_transaction().cloned() { + if !push_to_history { + b.forget_transaction(transaction.id); + project_transaction + .0 + .insert(buffer.handle.clone(), transaction); } - None } - }; - anyhow::Ok(result) + })?; + return Ok(()); } pub async fn format_ranges_via_lsp( this: &WeakEntity, - buffer: &FormattableBuffer, - ranges: &Vec>, + buffer_handle: &Entity, + ranges: &[Range], abs_path: &Path, language_server: &Arc, settings: &LanguageSettings, @@ -1563,7 +1439,7 @@ impl LocalLspStore { // // TODO: Instead of using current snapshot, should use the latest snapshot sent to // LSP. - let snapshot = buffer.handle.read(cx).snapshot(); + let snapshot = buffer_handle.read(cx).snapshot(); for range in ranges { lsp_ranges.push(range_to_lsp(range.to_point_utf16(&snapshot))?); } @@ -1590,7 +1466,7 @@ impl LocalLspStore { if let Some(lsp_edits) = lsp_edits { this.update(cx, |this, cx| { this.as_local_mut().unwrap().edits_from_lsp( - &buffer.handle, + &buffer_handle, lsp_edits, language_server.server_id(), None, @@ -2779,10 +2655,10 @@ impl LocalLspStore { #[derive(Debug)] pub struct FormattableBuffer { - id: BufferId, handle: Entity, abs_path: Option, env: Option>, + ranges: Option>>, } pub struct RemoteLspStore { @@ -7041,18 +6917,27 @@ impl LspStore { })? .await; + let ranges = match &target { + LspFormatTarget::Buffers => None, + LspFormatTarget::Ranges(ranges) => { + let Some(ranges) = ranges.get(&id) else { + return Err(anyhow!("No format ranges provided for buffer")); + }; + Some(ranges.clone()) + } + }; + formattable_buffers.push(FormattableBuffer { - id, handle, abs_path, env, + ranges, }); } let result = LocalLspStore::format_locally( lsp_store.clone(), formattable_buffers, - &target, push_to_history, trigger, cx.clone(),