diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 5fd5d2ed75cb1a5c8940f8b8765304e0641aabb2..5360757ffe42c5c8d85dd1e8632c7bca62f467a8 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -14648,6 +14648,107 @@ async fn test_organize_imports_manual_trigger(cx: &mut TestAppContext) { ); } +#[gpui::test] +async fn test_formatter_failure_does_not_abort_subsequent_formatters(cx: &mut TestAppContext) { + init_test(cx, |settings| { + settings.defaults.formatter = Some(FormatterList::Vec(vec![ + Formatter::LanguageServer(settings::LanguageServerFormatterSpecifier::Current), + Formatter::CodeAction("organize-imports".into()), + ])) + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_file(path!("/file.rs"), "fn main() {}\n".into()) + .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: lsp::ServerCapabilities { + document_formatting_provider: Some(lsp::OneOf::Left(true)), + code_action_provider: Some(lsp::CodeActionProviderCapability::Simple(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) + }); + + let fake_server = fake_servers.next().await.unwrap(); + + // Formatter #1 (LanguageServer) returns an error to simulate failure + fake_server.set_request_handler::( + move |_params, _| async move { Err(anyhow::anyhow!("Simulated formatter failure")) }, + ); + + // Formatter #2 (CodeAction) returns a successful edit + fake_server.set_request_handler::( + move |_params, _| async move { + let uri = lsp::Uri::from_file_path(path!("/file.rs")).unwrap(); + Ok(Some(vec![lsp::CodeActionOrCommand::CodeAction( + lsp::CodeAction { + kind: Some("organize-imports".into()), + edit: Some(lsp::WorkspaceEdit::new( + [( + uri, + vec![lsp::TextEdit::new( + lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 0)), + "use std::io;\n".to_string(), + )], + )] + .into_iter() + .collect(), + )), + ..Default::default() + }, + )])) + }, + ); + + fake_server.set_request_handler::({ + move |params, _| async move { Ok(params) } + }); + + editor + .update_in(cx, |editor, window, cx| { + editor.perform_format( + project.clone(), + FormatTrigger::Manual, + FormatTarget::Buffers(editor.buffer().read(cx).all_buffers()), + window, + cx, + ) + }) + .unwrap() + .await; + + // Formatter #1 (LanguageServer) failed, but formatter #2 (CodeAction) should have applied + editor.update(cx, |editor, cx| { + assert_eq!(editor.text(cx), "use std::io;\nfn main() {}\n"); + }); + + // The entire format operation should undo as one transaction + editor.update_in(cx, |editor, window, cx| { + editor.undo(&Default::default(), window, cx); + assert_eq!(editor.text(cx), "fn main() {}\n"); + }); +} + #[gpui::test] async fn test_concurrent_format_requests(cx: &mut TestAppContext) { init_test(cx, |_| {}); diff --git a/crates/project/src/lsp_store.rs b/crates/project/src/lsp_store.rs index da09681e221d9e8d25365218f2e064237ddd92b1..9b0a14cbeaa83a1a7591f50fb913096a7d2af248 100644 --- a/crates/project/src/lsp_store.rs +++ b/crates/project/src/lsp_store.rs @@ -1611,28 +1611,6 @@ impl LocalLspStore { }) })?; - /// Apply edits to the buffer that will become part of the formatting transaction. - /// Fails if the buffer has been edited since the start of that transaction. - fn extend_formatting_transaction( - buffer: &FormattableBuffer, - formatting_transaction_id: text::TransactionId, - cx: &mut AsyncApp, - operation: impl FnOnce(&mut Buffer, &mut Context), - ) -> anyhow::Result<()> { - buffer.handle.update(cx, |buffer, cx| { - let last_transaction_id = buffer.peek_undo_stack().map(|t| t.transaction_id()); - if last_transaction_id != Some(formatting_transaction_id) { - anyhow::bail!("Buffer edited while formatting. Aborting") - } - buffer.start_transaction(); - operation(buffer, cx); - if let Some(transaction_id) = buffer.end_transaction(cx) { - buffer.merge_transactions(transaction_id, formatting_transaction_id); - } - Ok(()) - }) - } - // handle whitespace formatting if settings.remove_trailing_whitespace_on_save { zlog::trace!(logger => "removing trailing whitespace"); @@ -1702,508 +1680,532 @@ impl LocalLspStore { } else { formatter }; - match formatter { - Formatter::None => { - zlog::trace!(logger => "skipping formatter 'none'"); - continue; - } - Formatter::Auto => unreachable!("Auto resolved above"), - Formatter::Prettier => { - let logger = zlog::scoped!(logger => "prettier"); - zlog::trace!(logger => "formatting"); - let _timer = zlog::time!(logger => "Formatting buffer via prettier"); + if let Err(err) = Self::apply_formatter( + formatter, + &lsp_store, + buffer, + formatting_transaction_id, + &adapters_and_servers, + &settings, + request_timeout, + logger, + cx, + ) + .await + { + zlog::error!(logger => "Formatter failed, skipping: {err:#}"); + } + } - let prettier = lsp_store.read_with(cx, |lsp_store, _cx| { - lsp_store.prettier_store().unwrap().downgrade() - })?; - let diff = prettier_store::format_with_prettier(&prettier, &buffer.handle, cx) + Ok(()) + } + + async fn apply_formatter( + formatter: &Formatter, + lsp_store: &WeakEntity, + buffer: &FormattableBuffer, + formatting_transaction_id: clock::Lamport, + adapters_and_servers: &[(Arc, Arc)], + settings: &LanguageSettings, + request_timeout: Duration, + logger: zlog::Logger, + cx: &mut AsyncApp, + ) -> anyhow::Result<()> { + match formatter { + Formatter::None => { + zlog::trace!(logger => "skipping formatter 'none'"); + return Ok(()); + } + Formatter::Auto => { + debug_panic!("Auto resolved above"); + return Ok(()); + } + Formatter::Prettier => { + let logger = zlog::scoped!(logger => "prettier"); + zlog::trace!(logger => "formatting"); + let _timer = zlog::time!(logger => "Formatting buffer via prettier"); + + let prettier = lsp_store.read_with(cx, |lsp_store, _cx| { + lsp_store.prettier_store().unwrap().downgrade() + })?; + let diff = prettier_store::format_with_prettier(&prettier, &buffer.handle, cx) + .await + .transpose()?; + let Some(diff) = diff else { + zlog::trace!(logger => "No changes"); + return Ok(()); + }; + + extend_formatting_transaction( + buffer, + formatting_transaction_id, + cx, + |buffer, cx| { + buffer.apply_diff(diff, cx); + }, + )?; + } + Formatter::External { command, arguments } => { + let logger = zlog::scoped!(logger => "command"); + zlog::trace!(logger => "formatting"); + let _timer = zlog::time!(logger => "Formatting buffer via external command"); + + let diff = + Self::format_via_external_command(buffer, &command, arguments.as_deref(), cx) .await - .transpose()?; - let Some(diff) = diff else { - zlog::trace!(logger => "No changes"); - continue; - }; + .with_context(|| { + format!("Failed to format buffer via external command: {}", command) + })?; + let Some(diff) = diff else { + zlog::trace!(logger => "No changes"); + return Ok(()); + }; - extend_formatting_transaction( - buffer, - formatting_transaction_id, - cx, - |buffer, cx| { - buffer.apply_diff(diff, cx); - }, - )?; - } - Formatter::External { command, arguments } => { - let logger = zlog::scoped!(logger => "command"); - zlog::trace!(logger => "formatting"); - let _timer = zlog::time!(logger => "Formatting buffer via external command"); + extend_formatting_transaction( + buffer, + formatting_transaction_id, + cx, + |buffer, cx| { + buffer.apply_diff(diff, cx); + }, + )?; + } + Formatter::LanguageServer(specifier) => { + let logger = zlog::scoped!(logger => "language-server"); + zlog::trace!(logger => "formatting"); + let _timer = zlog::time!(logger => "Formatting buffer using language server"); - let diff = Self::format_via_external_command( - buffer, - &command, - arguments.as_deref(), + let Some(buffer_path_abs) = buffer.abs_path.as_ref() else { + zlog::warn!(logger => "Cannot format buffer that is not backed by a file on disk using language servers. Skipping"); + return Ok(()); + }; + + let language_server = match specifier { + settings::LanguageServerFormatterSpecifier::Specific { name } => { + adapters_and_servers.iter().find_map(|(adapter, server)| { + if adapter.name.0.as_ref() == name { + Some(server.clone()) + } else { + None + } + }) + } + 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 { + log::debug!( + "No language server found to format buffer '{:?}'. Skipping", + buffer_path_abs.as_path().to_string_lossy() + ); + return Ok(()); + }; + + zlog::trace!( + logger => + "Formatting buffer '{:?}' using language server '{:?}'", + buffer_path_abs.as_path().to_string_lossy(), + language_server.name() + ); + + let edits = if let Some(ranges) = buffer.ranges.as_ref() { + zlog::trace!(logger => "formatting ranges"); + Self::format_ranges_via_lsp( + &lsp_store, + &buffer.handle, + ranges, + buffer_path_abs, + &language_server, + &settings, cx, ) .await - .with_context(|| { - format!("Failed to format buffer via external command: {}", command) - })?; - let Some(diff) = diff else { - zlog::trace!(logger => "No changes"); - continue; - }; - - extend_formatting_transaction( - buffer, - formatting_transaction_id, + .context("Failed to format ranges via language server")? + } else { + zlog::trace!(logger => "formatting full"); + Self::format_via_lsp( + &lsp_store, + &buffer.handle, + buffer_path_abs, + &language_server, + &settings, cx, - |buffer, cx| { - buffer.apply_diff(diff, cx); - }, - )?; + ) + .await + .context("failed to format via language server")? + }; + + if edits.is_empty() { + zlog::trace!(logger => "No changes"); + return Ok(()); } - Formatter::LanguageServer(specifier) => { - let logger = zlog::scoped!(logger => "language-server"); - zlog::trace!(logger => "formatting"); - let _timer = zlog::time!(logger => "Formatting buffer using language server"); + extend_formatting_transaction( + buffer, + formatting_transaction_id, + cx, + |buffer, cx| { + buffer.edit(edits, None, cx); + }, + )?; + } + Formatter::CodeAction(code_action_name) => { + let logger = zlog::scoped!(logger => "code-actions"); + zlog::trace!(logger => "formatting"); + let _timer = zlog::time!(logger => "Formatting buffer using code actions"); - let Some(buffer_path_abs) = buffer.abs_path.as_ref() else { - zlog::warn!(logger => "Cannot format buffer that is not backed by a file on disk using language servers. Skipping"); - continue; - }; + let Some(buffer_path_abs) = buffer.abs_path.as_ref() else { + zlog::warn!(logger => "Cannot format buffer that is not backed by a file on disk using code actions. Skipping"); + return Ok(()); + }; - let language_server = match specifier { - settings::LanguageServerFormatterSpecifier::Specific { name } => { - adapters_and_servers.iter().find_map(|(adapter, server)| { - if adapter.name.0.as_ref() == name { - Some(server.clone()) - } else { - None - } - }) - } - settings::LanguageServerFormatterSpecifier::Current => adapters_and_servers - .iter() - .find(|(_, server)| Self::server_supports_formatting(server)) - .map(|(_, server)| server.clone()), - }; + let code_action_kind: CodeActionKind = code_action_name.clone().into(); + zlog::trace!(logger => "Attempting to resolve code actions {:?}", &code_action_kind); + + let mut actions_and_servers = Vec::new(); - let Some(language_server) = language_server else { - log::debug!( - "No language server found to format buffer '{:?}'. Skipping", - buffer_path_abs.as_path().to_string_lossy() + for (index, (_, language_server)) in adapters_and_servers.iter().enumerate() { + let actions_result = Self::get_server_code_actions_from_action_kinds( + &lsp_store, + language_server.server_id(), + vec![code_action_kind.clone()], + &buffer.handle, + cx, + ) + .await + .with_context(|| { + format!( + "Failed to resolve code action {:?} with language server {}", + code_action_kind, + language_server.name() + ) + }); + let Ok(actions) = actions_result else { + // note: it may be better to set result to the error and break formatters here + // but for now we try to execute the actions that we can resolve and skip the rest + zlog::error!( + logger => + "Failed to resolve code action {:?} with language server {}", + code_action_kind, + language_server.name() ); continue; }; + for action in actions { + actions_and_servers.push((action, index)); + } + } - zlog::trace!( - logger => - "Formatting buffer '{:?}' using language server '{:?}'", - buffer_path_abs.as_path().to_string_lossy(), - language_server.name() - ); + if actions_and_servers.is_empty() { + zlog::warn!(logger => "No code actions were resolved, continuing"); + return Ok(()); + } - let edits = if let Some(ranges) = buffer.ranges.as_ref() { - zlog::trace!(logger => "formatting ranges"); - Self::format_ranges_via_lsp( - &lsp_store, - &buffer.handle, - ranges, - buffer_path_abs, - &language_server, - &settings, - cx, - ) - .await - .context("Failed to format ranges via language server")? - } else { - zlog::trace!(logger => "formatting full"); - Self::format_via_lsp( - &lsp_store, - &buffer.handle, - buffer_path_abs, - &language_server, - &settings, - cx, + 'actions: for (mut action, server_index) in actions_and_servers { + let server = &adapters_and_servers[server_index].1; + + let describe_code_action = |action: &CodeAction| { + format!( + "code action '{}' with title \"{}\" on server {}", + action + .lsp_action + .action_kind() + .unwrap_or("unknown".into()) + .as_str(), + action.lsp_action.title(), + server.name(), ) - .await - .context("failed to format via language server")? }; - if edits.is_empty() { - zlog::trace!(logger => "No changes"); - continue; - } - extend_formatting_transaction( - buffer, - formatting_transaction_id, - cx, - |buffer, cx| { - buffer.edit(edits, None, cx); - }, - )?; - } - Formatter::CodeAction(code_action_name) => { - let logger = zlog::scoped!(logger => "code-actions"); - zlog::trace!(logger => "formatting"); - let _timer = zlog::time!(logger => "Formatting buffer using code actions"); + zlog::trace!(logger => "Executing {}", describe_code_action(&action)); - let Some(buffer_path_abs) = buffer.abs_path.as_ref() else { - zlog::warn!(logger => "Cannot format buffer that is not backed by a file on disk using code actions. Skipping"); + if let Err(err) = + Self::try_resolve_code_action(server, &mut action, request_timeout).await + { + zlog::error!( + logger => + "Failed to resolve {}. Error: {}", + describe_code_action(&action), + err + ); continue; - }; - - let code_action_kind: CodeActionKind = code_action_name.clone().into(); - zlog::trace!(logger => "Attempting to resolve code actions {:?}", &code_action_kind); - - let mut actions_and_servers = Vec::new(); + } - for (index, (_, language_server)) in adapters_and_servers.iter().enumerate() { - let actions_result = Self::get_server_code_actions_from_action_kinds( - &lsp_store, - language_server.server_id(), - vec![code_action_kind.clone()], - &buffer.handle, - cx, - ) - .await - .with_context(|| { - format!( - "Failed to resolve code action {:?} with language server {}", - code_action_kind, - language_server.name() - ) - }); - let Ok(actions) = actions_result else { - // note: it may be better to set result to the error and break formatters here - // but for now we try to execute the actions that we can resolve and skip the rest - zlog::error!( + if let Some(edit) = action.lsp_action.edit().cloned() { + // NOTE: code below duplicated from `Self::deserialize_workspace_edit` + // but filters out and logs warnings for code actions that require unreasonably + // difficult handling on our part, such as: + // - applying edits that call commands + // which can result in arbitrary workspace edits being sent from the server that + // have no way of being tied back to the command that initiated them (i.e. we + // can't know which edits are part of the format request, or if the server is done sending + // actions in response to the command) + // - actions that create/delete/modify/rename files other than the one we are formatting + // as we then would need to handle such changes correctly in the local history as well + // as the remote history through the ProjectTransaction + // - actions with snippet edits, as these simply don't make sense in the context of a format request + // Supporting these actions is not impossible, but not supported as of yet. + if edit.changes.is_none() && edit.document_changes.is_none() { + zlog::trace!( logger => - "Failed to resolve code action {:?} with language server {}", - code_action_kind, - language_server.name() + "No changes for code action. Skipping {}", + describe_code_action(&action), ); continue; - }; - for action in actions { - actions_and_servers.push((action, index)); } - } - - if actions_and_servers.is_empty() { - zlog::warn!(logger => "No code actions were resolved, continuing"); - continue; - } - 'actions: for (mut action, server_index) in actions_and_servers { - let server = &adapters_and_servers[server_index].1; - - let describe_code_action = |action: &CodeAction| { - format!( - "code action '{}' with title \"{}\" on server {}", - action - .lsp_action - .action_kind() - .unwrap_or("unknown".into()) - .as_str(), - action.lsp_action.title(), - server.name(), - ) - }; + let mut operations = Vec::new(); + if let Some(document_changes) = edit.document_changes { + match document_changes { + lsp::DocumentChanges::Edits(edits) => operations.extend( + edits.into_iter().map(lsp::DocumentChangeOperation::Edit), + ), + lsp::DocumentChanges::Operations(ops) => operations = ops, + } + } else if let Some(changes) = edit.changes { + operations.extend(changes.into_iter().map(|(uri, edits)| { + lsp::DocumentChangeOperation::Edit(lsp::TextDocumentEdit { + text_document: lsp::OptionalVersionedTextDocumentIdentifier { + uri, + version: None, + }, + edits: edits.into_iter().map(Edit::Plain).collect(), + }) + })); + } - zlog::trace!(logger => "Executing {}", describe_code_action(&action)); + let mut edits = Vec::with_capacity(operations.len()); - if let Err(err) = - Self::try_resolve_code_action(server, &mut action, request_timeout) - .await - { - zlog::error!( + if operations.is_empty() { + zlog::trace!( logger => - "Failed to resolve {}. Error: {}", + "No changes for code action. Skipping {}", describe_code_action(&action), - err ); continue; } - - if let Some(edit) = action.lsp_action.edit().cloned() { - // NOTE: code below duplicated from `Self::deserialize_workspace_edit` - // but filters out and logs warnings for code actions that require unreasonably - // difficult handling on our part, such as: - // - applying edits that call commands - // which can result in arbitrary workspace edits being sent from the server that - // have no way of being tied back to the command that initiated them (i.e. we - // can't know which edits are part of the format request, or if the server is done sending - // actions in response to the command) - // - actions that create/delete/modify/rename files other than the one we are formatting - // as we then would need to handle such changes correctly in the local history as well - // as the remote history through the ProjectTransaction - // - actions with snippet edits, as these simply don't make sense in the context of a format request - // Supporting these actions is not impossible, but not supported as of yet. - if edit.changes.is_none() && edit.document_changes.is_none() { - zlog::trace!( + for operation in operations { + let op = match operation { + lsp::DocumentChangeOperation::Edit(op) => op, + lsp::DocumentChangeOperation::Op(_) => { + zlog::warn!( + logger => + "Code actions which create, delete, or rename files are not supported on format. Skipping {}", + describe_code_action(&action), + ); + continue 'actions; + } + }; + let Ok(file_path) = op.text_document.uri.to_file_path() else { + zlog::warn!( logger => - "No changes for code action. Skipping {}", + "Failed to convert URI '{:?}' to file path. Skipping {}", + &op.text_document.uri, describe_code_action(&action), ); - continue; - } - - let mut operations = Vec::new(); - if let Some(document_changes) = edit.document_changes { - match document_changes { - lsp::DocumentChanges::Edits(edits) => operations.extend( - edits.into_iter().map(lsp::DocumentChangeOperation::Edit), - ), - lsp::DocumentChanges::Operations(ops) => operations = ops, - } - } else if let Some(changes) = edit.changes { - operations.extend(changes.into_iter().map(|(uri, edits)| { - lsp::DocumentChangeOperation::Edit(lsp::TextDocumentEdit { - text_document: - lsp::OptionalVersionedTextDocumentIdentifier { - uri, - version: None, - }, - edits: edits.into_iter().map(Edit::Plain).collect(), - }) - })); - } - - let mut edits = Vec::with_capacity(operations.len()); - - if operations.is_empty() { - zlog::trace!( + continue 'actions; + }; + if &file_path != buffer_path_abs { + zlog::warn!( logger => - "No changes for code action. Skipping {}", + "File path '{:?}' does not match buffer path '{:?}'. Skipping {}", + file_path, + buffer_path_abs, describe_code_action(&action), ); - continue; + continue 'actions; } - for operation in operations { - let op = match operation { - lsp::DocumentChangeOperation::Edit(op) => op, - lsp::DocumentChangeOperation::Op(_) => { + + let mut lsp_edits = Vec::new(); + for edit in op.edits { + match edit { + Edit::Plain(edit) => { + if !lsp_edits.contains(&edit) { + lsp_edits.push(edit); + } + } + Edit::Annotated(edit) => { + if !lsp_edits.contains(&edit.text_edit) { + lsp_edits.push(edit.text_edit); + } + } + Edit::Snippet(_) => { zlog::warn!( logger => - "Code actions which create, delete, or rename files are not supported on format. Skipping {}", + "Code actions which produce snippet edits are not supported during formatting. Skipping {}", describe_code_action(&action), ); continue 'actions; } - }; - let Ok(file_path) = op.text_document.uri.to_file_path() else { - zlog::warn!( - logger => - "Failed to convert URI '{:?}' to file path. Skipping {}", - &op.text_document.uri, - describe_code_action(&action), - ); - continue 'actions; - }; - if &file_path != buffer_path_abs { - zlog::warn!( - logger => - "File path '{:?}' does not match buffer path '{:?}'. Skipping {}", - file_path, - buffer_path_abs, - describe_code_action(&action), - ); - continue 'actions; - } - - let mut lsp_edits = Vec::new(); - for edit in op.edits { - match edit { - Edit::Plain(edit) => { - if !lsp_edits.contains(&edit) { - lsp_edits.push(edit); - } - } - Edit::Annotated(edit) => { - if !lsp_edits.contains(&edit.text_edit) { - lsp_edits.push(edit.text_edit); - } - } - Edit::Snippet(_) => { - zlog::warn!( - logger => - "Code actions which produce snippet edits are not supported during formatting. Skipping {}", - describe_code_action(&action), - ); - continue 'actions; - } - } } - let edits_result = lsp_store - .update(cx, |lsp_store, cx| { - lsp_store.as_local_mut().unwrap().edits_from_lsp( - &buffer.handle, - lsp_edits, - server.server_id(), - op.text_document.version, - cx, - ) - })? - .await; - let Ok(resolved_edits) = edits_result else { - zlog::warn!( - logger => - "Failed to resolve edits from LSP for buffer {:?} while handling {}", - buffer_path_abs.as_path(), - describe_code_action(&action), - ); - continue 'actions; - }; - edits.extend(resolved_edits); - } - - if edits.is_empty() { - zlog::warn!(logger => "No edits resolved from LSP"); - continue; } - - extend_formatting_transaction( - buffer, - formatting_transaction_id, - cx, - |buffer, cx| { - zlog::info!( - "Applying edits {edits:?}. Content: {:?}", - buffer.text() - ); - buffer.edit(edits, None, cx); - zlog::info!("Applied edits. New Content: {:?}", buffer.text()); - }, - )?; + let edits_result = lsp_store + .update(cx, |lsp_store, cx| { + lsp_store.as_local_mut().unwrap().edits_from_lsp( + &buffer.handle, + lsp_edits, + server.server_id(), + op.text_document.version, + cx, + ) + })? + .await; + let Ok(resolved_edits) = edits_result else { + zlog::warn!( + logger => + "Failed to resolve edits from LSP for buffer {:?} while handling {}", + buffer_path_abs.as_path(), + describe_code_action(&action), + ); + continue 'actions; + }; + edits.extend(resolved_edits); } - // bail early if command is invalid - let Some(command) = action.lsp_action.command() else { - continue; - }; - - zlog::warn!( - logger => - "Executing code action command '{}'. This may cause formatting to abort unnecessarily as well as splitting formatting into two entries in the undo history", - &command.command, - ); - - let server_capabilities = server.capabilities(); - let available_commands = server_capabilities - .execute_command_provider - .as_ref() - .map(|options| options.commands.as_slice()) - .unwrap_or_default(); - if !available_commands.contains(&command.command) { - zlog::warn!( - logger => - "Cannot execute a command {} not listed in the language server capabilities of server {}", - command.command, - server.name(), - ); + if edits.is_empty() { + zlog::warn!(logger => "No edits resolved from LSP"); continue; } - // noop so we just ensure buffer hasn't been edited since resolving code actions extend_formatting_transaction( buffer, formatting_transaction_id, cx, - |_, _| {}, + |buffer, cx| { + zlog::info!( + "Applying edits {edits:?}. Content: {:?}", + buffer.text() + ); + buffer.edit(edits, None, cx); + zlog::info!("Applied edits. New Content: {:?}", buffer.text()); + }, )?; - zlog::info!(logger => "Executing command {}", &command.command); + } - lsp_store.update(cx, |this, _| { - this.as_local_mut() - .unwrap() - .last_workspace_edits_by_language_server - .remove(&server.server_id()); - })?; + let Some(command) = action.lsp_action.command() else { + continue; + }; - let execute_command_result = server - .request::( - lsp::ExecuteCommandParams { - command: command.command.clone(), - arguments: command.arguments.clone().unwrap_or_default(), - ..Default::default() - }, - request_timeout, - ) - .await - .into_response(); + zlog::warn!( + logger => + "Executing code action command '{}'. This may cause formatting to abort unnecessarily as well as splitting formatting into two entries in the undo history", + &command.command, + ); - if execute_command_result.is_err() { - zlog::error!( - logger => - "Failed to execute command '{}' as part of {}", - &command.command, - describe_code_action(&action), - ); - continue 'actions; - } + let server_capabilities = server.capabilities(); + let available_commands = server_capabilities + .execute_command_provider + .as_ref() + .map(|options| options.commands.as_slice()) + .unwrap_or_default(); + if !available_commands.contains(&command.command) { + zlog::warn!( + logger => + "Cannot execute a command {} not listed in the language server capabilities of server {}", + command.command, + server.name(), + ); + continue; + } - let mut project_transaction_command = lsp_store.update(cx, |this, _| { - this.as_local_mut() - .unwrap() - .last_workspace_edits_by_language_server - .remove(&server.server_id()) - .unwrap_or_default() - })?; + extend_formatting_transaction( + buffer, + formatting_transaction_id, + cx, + |_, _| {}, + )?; + zlog::info!(logger => "Executing command {}", &command.command); - if let Some(transaction) = - project_transaction_command.0.remove(&buffer.handle) - { - zlog::trace!( - logger => - "Successfully captured {} edits that resulted from command {}", - transaction.edit_ids.len(), - &command.command, - ); - let transaction_id_project_transaction = transaction.id; - buffer.handle.update(cx, |buffer, _| { - // it may have been removed from history if push_to_history was - // false in deserialize_workspace_edit. If so push it so we - // can merge it with the format transaction - // and pop the combined transaction off the history stack - // later if push_to_history is false - if buffer.get_transaction(transaction.id).is_none() { - buffer.push_transaction(transaction, Instant::now()); - } - buffer.merge_transactions( - transaction_id_project_transaction, - formatting_transaction_id, - ); - }); - } + lsp_store.update(cx, |this, _| { + this.as_local_mut() + .unwrap() + .last_workspace_edits_by_language_server + .remove(&server.server_id()); + })?; - if project_transaction_command.0.is_empty() { - continue; - } + let execute_command_result = server + .request::( + lsp::ExecuteCommandParams { + command: command.command.clone(), + arguments: command.arguments.clone().unwrap_or_default(), + ..Default::default() + }, + request_timeout, + ) + .await + .into_response(); - let mut extra_buffers = String::new(); - for buffer in project_transaction_command.0.keys() { - buffer.read_with(cx, |b, cx| { - let Some(path) = b.project_path(cx) else { - return; - }; + if execute_command_result.is_err() { + zlog::error!( + logger => + "Failed to execute command '{}' as part of {}", + &command.command, + describe_code_action(&action), + ); + continue 'actions; + } - if !extra_buffers.is_empty() { - extra_buffers.push_str(", "); - } - extra_buffers.push_str(path.path.as_unix_str()); - }); - } - zlog::warn!( + let mut project_transaction_command = lsp_store.update(cx, |this, _| { + this.as_local_mut() + .unwrap() + .last_workspace_edits_by_language_server + .remove(&server.server_id()) + .unwrap_or_default() + })?; + + if let Some(transaction) = project_transaction_command.0.remove(&buffer.handle) + { + zlog::trace!( logger => - "Unexpected edits to buffers other than the buffer actively being formatted due to command {}. Impacted buffers: [{}].", + "Successfully captured {} edits that resulted from command {}", + transaction.edit_ids.len(), &command.command, - extra_buffers, ); - // NOTE: if this case is hit, the proper thing to do is to for each buffer, merge the extra transaction - // into the existing transaction in project_transaction if there is one, and if there isn't one in project_transaction, - // add it so it's included, and merge it into the format transaction when its created later + let transaction_id_project_transaction = transaction.id; + buffer.handle.update(cx, |buffer, _| { + // it may have been removed from history if push_to_history was + // false in deserialize_workspace_edit. If so push it so we + // can merge it with the format transaction + // and pop the combined transaction off the history stack + // later if push_to_history is false + if buffer.get_transaction(transaction.id).is_none() { + buffer.push_transaction(transaction, Instant::now()); + } + buffer.merge_transactions( + transaction_id_project_transaction, + formatting_transaction_id, + ); + }); } + + if project_transaction_command.0.is_empty() { + continue; + } + + let mut extra_buffers = String::new(); + for buffer in project_transaction_command.0.keys() { + buffer.read_with(cx, |b, cx| { + let Some(path) = b.project_path(cx) else { + return; + }; + + if !extra_buffers.is_empty() { + extra_buffers.push_str(", "); + } + extra_buffers.push_str(path.path.as_unix_str()); + }); + } + zlog::warn!( + logger => + "Unexpected edits to buffers other than the buffer actively being formatted due to command {}. Impacted buffers: [{}].", + &command.command, + extra_buffers, + ); + // NOTE: if this case is hit, the proper thing to do is to for each buffer, merge the extra transaction + // into the existing transaction in project_transaction if there is one, and if there isn't one in project_transaction, + // add it so it's included, and merge it into the format transaction when its created later } } } @@ -14542,3 +14544,25 @@ pub fn ensure_uniform_list_compatible_label(label: &mut CodeLabel) { label.text = new_text; } + +/// Apply edits to the buffer that will become part of the formatting transaction. +/// Fails if the buffer has been edited since the start of that transaction. +fn extend_formatting_transaction( + buffer: &FormattableBuffer, + formatting_transaction_id: text::TransactionId, + cx: &mut AsyncApp, + operation: impl FnOnce(&mut Buffer, &mut Context), +) -> anyhow::Result<()> { + buffer.handle.update(cx, |buffer, cx| { + let last_transaction_id = buffer.peek_undo_stack().map(|t| t.transaction_id()); + if last_transaction_id != Some(formatting_transaction_id) { + anyhow::bail!("Buffer edited while formatting. Aborting") + } + buffer.start_transaction(); + operation(buffer, cx); + if let Some(transaction_id) = buffer.end_transaction(cx) { + buffer.merge_transactions(transaction_id, formatting_transaction_id); + } + Ok(()) + }) +}