From 53cde329dad6bf8ac555f660552b9f0af2529a05 Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Thu, 10 Apr 2025 11:32:43 -0400 Subject: [PATCH] Clean up formatting code and add testing for formatting with multiple formatters (including code actions!) (#28457) Release Notes: - N/A --------- Co-authored-by: Max Brunsfeld --- crates/editor/src/editor_tests.rs | 444 ++++++++-- crates/project/src/lsp_store.rs | 1356 +++++++++++++---------------- 2 files changed, 942 insertions(+), 858 deletions(-) diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index d7bb0eb33e..b0034d9df1 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -7756,77 +7756,81 @@ async fn test_document_format_during_save(cx: &mut TestAppContext) { cx.executor().start_waiting(); let fake_server = fake_servers.next().await.unwrap(); - let save = editor - .update_in(cx, |editor, window, cx| { - editor.save(true, project.clone(), window, cx) - }) - .unwrap(); - fake_server - .set_request_handler::(move |params, _| async move { - assert_eq!( - params.text_document.uri, - lsp::Url::from_file_path(path!("/file.rs")).unwrap() - ); - assert_eq!(params.options.tab_size, 4); - Ok(Some(vec![lsp::TextEdit::new( - lsp::Range::new(lsp::Position::new(0, 3), lsp::Position::new(1, 0)), - ", ".to_string(), - )])) - }) - .next() - .await; - cx.executor().start_waiting(); - save.await; + { + fake_server.set_request_handler::( + move |params, _| async move { + assert_eq!( + params.text_document.uri, + lsp::Url::from_file_path(path!("/file.rs")).unwrap() + ); + assert_eq!(params.options.tab_size, 4); + 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(true, project.clone(), window, cx) + }) + .unwrap(); + cx.executor().start_waiting(); + save.await; - assert_eq!( - editor.update(cx, |editor, cx| editor.text(cx)), - "one, two\nthree\n" - ); - assert!(!cx.read(|cx| editor.is_dirty(cx))); + assert_eq!( + editor.update(cx, |editor, cx| editor.text(cx)), + "one, two\nthree\n" + ); + assert!(!cx.read(|cx| editor.is_dirty(cx))); + } - editor.update_in(cx, |editor, window, cx| { - editor.set_text("one\ntwo\nthree\n", window, cx) - }); - assert!(cx.read(|cx| editor.is_dirty(cx))); + { + editor.update_in(cx, |editor, window, cx| { + editor.set_text("one\ntwo\nthree\n", window, cx) + }); + assert!(cx.read(|cx| editor.is_dirty(cx))); - // Ensure we can still save even if formatting hangs. - fake_server.set_request_handler::( - move |params, _| async move { - assert_eq!( - params.text_document.uri, - lsp::Url::from_file_path(path!("/file.rs")).unwrap() - ); - futures::future::pending::<()>().await; - unreachable!() - }, - ); - let save = editor - .update_in(cx, |editor, window, cx| { - editor.save(true, project.clone(), window, cx) - }) - .unwrap(); - cx.executor().advance_clock(super::FORMAT_TIMEOUT); - cx.executor().start_waiting(); - save.await; - assert_eq!( - editor.update(cx, |editor, cx| editor.text(cx)), - "one\ntwo\nthree\n" - ); - assert!(!cx.read(|cx| editor.is_dirty(cx))); + // Ensure we can still save even if formatting hangs. + fake_server.set_request_handler::( + move |params, _| async move { + assert_eq!( + params.text_document.uri, + lsp::Url::from_file_path(path!("/file.rs")).unwrap() + ); + futures::future::pending::<()>().await; + unreachable!() + }, + ); + let save = editor + .update_in(cx, |editor, window, cx| { + editor.save(true, project.clone(), window, cx) + }) + .unwrap(); + cx.executor().advance_clock(super::FORMAT_TIMEOUT); + cx.executor().start_waiting(); + save.await; + assert_eq!( + editor.update(cx, |editor, cx| editor.text(cx)), + "one\ntwo\nthree\n" + ); + } // For non-dirty buffer, no formatting request should be sent - let save = editor - .update_in(cx, |editor, window, cx| { - editor.save(true, project.clone(), window, cx) - }) - .unwrap(); - let _pending_format_request = fake_server - .set_request_handler::(move |_, _| async move { + { + assert!(!cx.read(|cx| editor.is_dirty(cx))); + + fake_server.set_request_handler::(move |_, _| async move { panic!("Should not be invoked on non-dirty buffer"); - }) - .next(); - cx.executor().start_waiting(); - save.await; + }); + let save = editor + .update_in(cx, |editor, window, cx| { + editor.save(true, project.clone(), window, cx) + }) + .unwrap(); + cx.executor().start_waiting(); + save.await; + } // Set rust language override and assert overridden tabsize is sent to language server update_test_language_settings(cx, |settings| { @@ -7839,28 +7843,28 @@ async fn test_document_format_during_save(cx: &mut TestAppContext) { ); }); - editor.update_in(cx, |editor, window, cx| { - editor.set_text("somehting_new\n", window, cx) - }); - assert!(cx.read(|cx| editor.is_dirty(cx))); - let save = editor - .update_in(cx, |editor, window, cx| { - editor.save(true, project.clone(), window, cx) - }) - .unwrap(); - fake_server - .set_request_handler::(move |params, _| async move { - assert_eq!( - params.text_document.uri, - lsp::Url::from_file_path(path!("/file.rs")).unwrap() - ); - assert_eq!(params.options.tab_size, 8); - Ok(Some(vec![])) - }) - .next() - .await; - cx.executor().start_waiting(); - save.await; + { + editor.update_in(cx, |editor, window, cx| { + editor.set_text("somehting_new\n", window, cx) + }); + assert!(cx.read(|cx| editor.is_dirty(cx))); + let _formatting_request_signal = fake_server + .set_request_handler::(move |params, _| async move { + assert_eq!( + params.text_document.uri, + lsp::Url::from_file_path(path!("/file.rs")).unwrap() + ); + assert_eq!(params.options.tab_size, 8); + Ok(Some(vec![])) + }); + let save = editor + .update_in(cx, |editor, window, cx| { + editor.save(true, project.clone(), window, cx) + }) + .unwrap(); + cx.executor().start_waiting(); + save.await; + } } #[gpui::test] @@ -8342,6 +8346,272 @@ async fn test_document_format_manual_trigger(cx: &mut TestAppContext) { ); } +#[gpui::test] +async fn test_multiple_formatters(cx: &mut TestAppContext) { + init_test(cx, |settings| { + settings.defaults.remove_trailing_whitespace_on_save = Some(true); + settings.defaults.formatter = + Some(language_settings::SelectedFormatter::List(FormatterList( + vec![ + Formatter::LanguageServer { name: None }, + Formatter::CodeActions( + [ + ("code-action-1".into(), true), + ("code-action-2".into(), true), + ] + .into_iter() + .collect(), + ), + ] + .into(), + ))) + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_file(path!("/file.rs"), "one \ntwo \nthree".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)), + execute_command_provider: Some(lsp::ExecuteCommandOptions { + commands: vec!["the-command-for-code-action-1".into()], + ..Default::default() + }), + 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) + }); + + cx.executor().start_waiting(); + + let fake_server = fake_servers.next().await.unwrap(); + fake_server.set_request_handler::( + move |_params, _| async move { + Ok(Some(vec![lsp::TextEdit::new( + lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 0)), + "applied-formatting\n".to_string(), + )])) + }, + ); + fake_server.set_request_handler::( + move |params, _| async move { + assert_eq!( + params.context.only, + Some(vec!["code-action-1".into(), "code-action-2".into()]) + ); + let uri = lsp::Url::from_file_path(path!("/file.rs")).unwrap(); + Ok(Some(vec![ + lsp::CodeActionOrCommand::CodeAction(lsp::CodeAction { + kind: Some("code-action-1".into()), + edit: Some(lsp::WorkspaceEdit::new( + [( + uri.clone(), + vec![lsp::TextEdit::new( + lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 0)), + "applied-code-action-1-edit\n".to_string(), + )], + )] + .into_iter() + .collect(), + )), + command: Some(lsp::Command { + command: "the-command-for-code-action-1".into(), + ..Default::default() + }), + ..Default::default() + }), + lsp::CodeActionOrCommand::CodeAction(lsp::CodeAction { + kind: Some("code-action-2".into()), + edit: Some(lsp::WorkspaceEdit::new( + [( + uri.clone(), + vec![lsp::TextEdit::new( + lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 0)), + "applied-code-action-2-edit\n".to_string(), + )], + )] + .into_iter() + .collect(), + )), + ..Default::default() + }), + ])) + }, + ); + + fake_server.set_request_handler::({ + move |params, _| async move { Ok(params) } + }); + + let command_lock = Arc::new(futures::lock::Mutex::new(())); + fake_server.set_request_handler::({ + let fake = fake_server.clone(); + let lock = command_lock.clone(); + move |params, _| { + assert_eq!(params.command, "the-command-for-code-action-1"); + let fake = fake.clone(); + let lock = lock.clone(); + async move { + lock.lock().await; + fake.server + .request::(lsp::ApplyWorkspaceEditParams { + label: None, + edit: lsp::WorkspaceEdit { + changes: Some( + [( + lsp::Url::from_file_path(path!("/file.rs")).unwrap(), + vec![lsp::TextEdit { + range: lsp::Range::new( + lsp::Position::new(0, 0), + lsp::Position::new(0, 0), + ), + new_text: "applied-code-action-1-command\n".into(), + }], + )] + .into_iter() + .collect(), + ), + ..Default::default() + }, + }) + .await + .unwrap(); + Ok(Some(json!(null))) + } + } + }); + + cx.executor().start_waiting(); + editor + .update_in(cx, |editor, window, cx| { + editor.perform_format( + project.clone(), + FormatTrigger::Manual, + FormatTarget::Buffers, + window, + cx, + ) + }) + .unwrap() + .await; + editor.update(cx, |editor, cx| { + assert_eq!( + editor.text(cx), + r#" + applied-code-action-2-edit + applied-code-action-1-command + applied-code-action-1-edit + applied-formatting + one + two + three + "# + .unindent() + ); + }); + + editor.update_in(cx, |editor, window, cx| { + editor.undo(&Default::default(), window, cx); + assert_eq!(editor.text(cx), "one \ntwo \nthree"); + }); + + // Perform a manual edit while waiting for an LSP command + // that's being run as part of a formatting code action. + let lock_guard = command_lock.lock().await; + let format = editor + .update_in(cx, |editor, window, cx| { + editor.perform_format( + project.clone(), + FormatTrigger::Manual, + FormatTarget::Buffers, + window, + cx, + ) + }) + .unwrap(); + cx.run_until_parked(); + editor.update(cx, |editor, cx| { + assert_eq!( + editor.text(cx), + r#" + applied-code-action-1-edit + applied-formatting + one + two + three + "# + .unindent() + ); + + editor.buffer.update(cx, |buffer, cx| { + let ix = buffer.len(cx); + buffer.edit([(ix..ix, "edited\n")], None, cx); + }); + }); + + // Allow the LSP command to proceed. Because the buffer was edited, + // the second code action will not be run. + drop(lock_guard); + format.await; + editor.update_in(cx, |editor, window, cx| { + assert_eq!( + editor.text(cx), + r#" + applied-code-action-1-command + applied-code-action-1-edit + applied-formatting + one + two + three + edited + "# + .unindent() + ); + + // The manual edit is undone first, because it is the last thing the user did + // (even though the command completed afterwards). + editor.undo(&Default::default(), window, cx); + assert_eq!( + editor.text(cx), + r#" + applied-code-action-1-command + applied-code-action-1-edit + applied-formatting + one + two + three + "# + .unindent() + ); + + // All the formatting (including the command, which completed after the manual edit) + // is undone together. + editor.undo(&Default::default(), window, cx); + assert_eq!(editor.text(cx), "one \ntwo \nthree"); + }); +} + #[gpui::test] async fn test_organize_imports_manual_trigger(cx: &mut TestAppContext) { init_test(cx, |settings| { diff --git a/crates/project/src/lsp_store.rs b/crates/project/src/lsp_store.rs index 07a56e5d89..4b39b9496d 100644 --- a/crates/project/src/lsp_store.rs +++ b/crates/project/src/lsp_store.rs @@ -1082,17 +1082,6 @@ impl LocalLspStore { }) } - fn primary_language_server_for_buffer<'a>( - &'a self, - buffer: &'a Buffer, - cx: &'a mut App, - ) -> Option<(&'a Arc, &'a Arc)> { - // The list of language servers is ordered based on the `language_servers` setting - // for each language, thus we can consider the first one in the list to be the - // primary one. - self.language_servers_for_buffer(buffer, cx).next() - } - async fn execute_code_action_kind_locally( lsp_store: WeakEntity, mut buffers: Vec>, @@ -1198,816 +1187,656 @@ impl LocalLspStore { let mut project_transaction = ProjectTransaction::default(); for buffer in &buffers { - let adapters_and_servers = lsp_store.update(cx, |lsp_store, cx| { - buffer.handle.update(cx, |buffer, cx| { - lsp_store - .as_local() - .unwrap() - .language_servers_for_buffer(buffer, cx) - .map(|(adapter, lsp)| (adapter.clone(), lsp.clone())) - .collect::>() - }) - })?; - - let settings = buffer.handle.read_with(cx, |buffer, cx| { - language_settings(buffer.language().map(|l| l.name()), buffer.file(), cx) - .into_owned() - })?; - - let mut transaction_id_format = None; - - // ensure no transactions created while formatting are - // grouped with the previous transaction in the history - // based on the transaction group interval - buffer.handle.update(cx, |buffer, _| { + // Create an empty transaction to hold all of the formatting edits. + let formatting_transaction_id = buffer.handle.update(cx, |buffer, cx| { + // ensure no transactions created while formatting are + // grouped with the previous transaction in the history + // based on the transaction group interval buffer.finalize_last_transaction(); + let transaction_id = buffer + .start_transaction() + .ok_or_else(|| anyhow!("transaction already open"))?; + let transaction = buffer + .get_transaction(transaction_id) + .expect("transaction started") + .clone(); + buffer.end_transaction(cx); + buffer.push_transaction(transaction, cx.background_executor().now()); + buffer.finalize_last_transaction(); + anyhow::Ok(transaction_id) + })??; + + let result = Self::format_buffer_locally( + lsp_store.clone(), + buffer, + formatting_transaction_id, + trigger, + logger, + cx, + ) + .await; + + buffer.handle.update(cx, |buffer, cx| { + let Some(formatting_transaction) = + buffer.get_transaction(formatting_transaction_id).cloned() + else { + zlog::warn!(logger => "no formatting transaction"); + return; + }; + if formatting_transaction.edit_ids.is_empty() { + buffer.forget_transaction(formatting_transaction_id); + return; + } + if !push_to_history { + zlog::trace!(logger => "forgetting format transaction"); + buffer.forget_transaction(formatting_transaction.id); + } + project_transaction + .0 + .insert(cx.entity(), formatting_transaction); })?; - // helper function to avoid duplicate logic between formatter handlers below - // We want to avoid continuing to format the buffer if it has been edited since the start - // so we check that the last transaction id on the undo stack matches the one we expect - // This check should be done after each "gather" step where we generate a diff or edits to apply, - // and before applying them to the buffer to avoid messing up the user's buffer - fn err_if_buffer_edited_since_start( - buffer: &FormattableBuffer, - transaction_id_format: Option, - cx: &AsyncApp, - ) -> Option { - let transaction_id_last = buffer - .handle - .read_with(cx, |buffer, _| { - buffer.peek_undo_stack().map(|t| t.transaction_id()) - }) - .ok() - .flatten(); - let should_continue_formatting = match (transaction_id_format, transaction_id_last) - { - (Some(format), Some(last)) => format == last, - (Some(_), None) => false, - (_, _) => true, - }; - if !should_continue_formatting { - return Some(anyhow::anyhow!("Buffer edited while formatting. Aborting")); + result?; + } + + Ok(project_transaction) + } + + async fn format_buffer_locally( + lsp_store: WeakEntity, + buffer: &FormattableBuffer, + formatting_transaction_id: clock::Lamport, + trigger: FormatTrigger, + logger: zlog::Logger, + cx: &mut AsyncApp, + ) -> Result<()> { + let (adapters_and_servers, settings) = lsp_store.update(cx, |lsp_store, cx| { + buffer.handle.update(cx, |buffer, 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 settings = + language_settings(buffer.language().map(|l| l.name()), buffer.file(), cx) + .into_owned(); + (adapters_and_servers, settings) + }) + })?; + + // 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") } - return None; + 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"); + let diff = buffer + .handle + .read_with(cx, |buffer, cx| buffer.remove_trailing_whitespace(cx))? + .await; + extend_formatting_transaction(buffer, formatting_transaction_id, cx, |buffer, cx| { + buffer.apply_diff(diff, cx); + })?; + } + + if settings.ensure_final_newline_on_save { + zlog::trace!(logger => "ensuring final newline"); + extend_formatting_transaction(buffer, formatting_transaction_id, cx, |buffer, cx| { + buffer.ensure_final_newline(cx); + })?; + } + + // Formatter for `code_actions_on_format` that runs before + // the rest of the formatters + let mut code_actions_on_format_formatter = None; + let should_run_code_actions_on_format = !matches!( + (trigger, &settings.format_on_save), + (FormatTrigger::Save, &FormatOnSave::Off) + ); + if should_run_code_actions_on_format { + let have_code_actions_to_run_on_format = settings + .code_actions_on_format + .values() + .any(|enabled| *enabled); + if have_code_actions_to_run_on_format { + zlog::trace!(logger => "going to run code actions on format"); + code_actions_on_format_formatter = Some(Formatter::CodeActions( + settings.code_actions_on_format.clone(), + )); } + } - // variable used to track errors that occur during the formatting process below, - // but that need to not be returned right away (with `?` for example) because we - // still need to clean up the transaction history and update the project transaction - // at the very end - let mut result = anyhow::Ok(()); - - // see handling of code action formatting for why there might already be transactions - // in project_transaction for this buffer - if let Some(transaction_existing) = project_transaction.0.remove(&buffer.handle) { - transaction_id_format = Some(transaction_existing.id); - buffer.handle.update(cx, |buffer, _| { - // ensure the transaction is in the history so we can group with it - if buffer.get_transaction(transaction_existing.id).is_none() { - buffer.push_transaction(transaction_existing, Instant::now()); - } - })?; - } - - if let Some(err) = err_if_buffer_edited_since_start(buffer, transaction_id_format, &cx) - { - zlog::warn!(logger => "Buffer edited while formatting. Aborting"); - result = Err(err); - } - - // handle whitespace formatting - if result.is_ok() { - if settings.remove_trailing_whitespace_on_save { - zlog::trace!(logger => "removing trailing whitespace"); - let diff = buffer - .handle - .read_with(cx, |buffer, cx| buffer.remove_trailing_whitespace(cx))? - .await; - buffer.handle.update(cx, |buffer, cx| { - buffer.start_transaction(); - buffer.apply_diff(diff, cx); - transaction_id_format = - transaction_id_format.or(buffer.end_transaction(cx)); - if let Some(transaction_id) = transaction_id_format { - buffer.group_until_transaction(transaction_id); + 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 settings.prettier.allowed { + zlog::trace!(logger => "Formatter set to auto: defaulting to prettier"); + std::slice::from_ref(&Formatter::Prettier) + } else { + zlog::trace!(logger => "Formatter set to auto: defaulting to primary language server"); + std::slice::from_ref(&Formatter::LanguageServer { name: None }) } + } + SelectedFormatter::List(formatter_list) => formatter_list.as_ref(), + } + } + }; + + let formatters = code_actions_on_format_formatter.iter().chain(formatters); + + for formatter in formatters { + match formatter { + 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"); + continue; + }; - if settings.ensure_final_newline_on_save { - zlog::trace!(logger => "ensuring final newline"); - buffer.handle.update(cx, |buffer, cx| { - buffer.start_transaction(); - buffer.ensure_final_newline(cx); - transaction_id_format = - transaction_id_format.or(buffer.end_transaction(cx)); - if let Some(transaction_id) = transaction_id_format { - buffer.group_until_transaction(transaction_id); - } - })?; - } - } - - // Formatter for `code_actions_on_format` that runs before - // the rest of the formatters - let code_actions_on_format_formatter = 'ca_formatter: { - let should_run_code_actions_on_format = !matches!( - (trigger, &settings.format_on_save), - (FormatTrigger::Save, &FormatOnSave::Off) - ); - let have_code_actions_to_run_on_format = settings - .code_actions_on_format - .values() - .any(|enabled| *enabled); - - if should_run_code_actions_on_format { - if have_code_actions_to_run_on_format { - zlog::trace!(logger => "going to run code actions on format"); - break 'ca_formatter Some(Formatter::CodeActions( - settings.code_actions_on_format.clone(), - )); - } - } - break 'ca_formatter None; - }; - - 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 settings.prettier.allowed { - zlog::trace!(logger => "Formatter set to auto: defaulting to prettier"); - std::slice::from_ref(&Formatter::Prettier) - } else { - zlog::trace!(logger => "Formatter set to auto: defaulting to primary language server"); - std::slice::from_ref(&Formatter::LanguageServer { name: None }) - } - } - SelectedFormatter::List(formatter_list) => formatter_list.as_ref(), - } - } - }; - - let formatters = code_actions_on_format_formatter.iter().chain(formatters); - - 'formatters: for formatter in formatters { - if result.is_err() { - // may have been set above, instead of indenting this whole block with an if, just don't do anything - // destructive if we're aborting - continue; - } - match formatter { - 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_result = - prettier_store::format_with_prettier(&prettier, &buffer.handle, cx) - .await - .transpose(); - let Ok(diff) = diff_result else { - result = Err(diff_result.unwrap_err()); - zlog::error!(logger => "failed, reason: {:?}", result.as_ref()); - break 'formatters; - }; - let Some(diff) = diff else { - zlog::trace!(logger => "No changes"); - continue 'formatters; - }; - if let Some(err) = - err_if_buffer_edited_since_start(buffer, transaction_id_format, &cx) - { - zlog::warn!(logger => "Buffer edited while formatting. Aborting"); - result = Err(err); - break 'formatters; - } - zlog::trace!(logger => "Applying changes"); - buffer.handle.update(cx, |buffer, cx| { - buffer.start_transaction(); + extend_formatting_transaction( + buffer, + formatting_transaction_id, + cx, + |buffer, cx| { buffer.apply_diff(diff, cx); - transaction_id_format = - transaction_id_format.or(buffer.end_transaction(cx)); - if let Some(transaction_id) = transaction_id_format { - buffer.group_until_transaction(transaction_id); - } - })?; - } - Formatter::External { command, arguments } => { - let logger = zlog::scoped!(logger => "command"); - zlog::trace!(logger => "formatting"); - let _timer = - zlog::time!(logger => "Formatting buffer via external command"); + }, + )?; + } + 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_result = Self::format_via_external_command( - buffer, - command.as_ref(), - arguments.as_deref(), + let diff = Self::format_via_external_command( + buffer, + command.as_ref(), + arguments.as_deref(), + 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, + cx, + |buffer, cx| { + buffer.apply_diff(diff, cx); + }, + )?; + } + Formatter::LanguageServer { name } => { + let logger = zlog::scoped!(logger => "language-server"); + zlog::trace!(logger => "formatting"); + let _timer = zlog::time!(logger => "Formatting buffer using language server"); + + 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 language_server = if let Some(name) = name.as_deref() { + adapters_and_servers.iter().find_map(|(adapter, server)| { + if adapter.name.0.as_ref() == name { + Some(server.clone()) + } else { + None + } + }) + } else { + adapters_and_servers.first().map(|e| e.1.clone()) + }; + + let Some(language_server) = language_server else { + log::warn!( + "No language server found to format buffer '{:?}'. Skipping", + buffer_path_abs.as_path().to_string_lossy() + ); + continue; + }; + + 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 Ok(diff) = diff_result else { - result = Err(diff_result.unwrap_err()); - zlog::error!(logger => "failed, reason: {:?}", result.as_ref()); - break 'formatters; - }; - let Some(diff) = diff else { - zlog::trace!(logger => "No changes"); - continue 'formatters; - }; - if let Some(err) = - err_if_buffer_edited_since_start(buffer, transaction_id_format, &cx) - { - zlog::warn!(logger => "Buffer edited while formatting. Aborting"); - result = Err(err); - break 'formatters; - } - zlog::trace!(logger => "Applying changes"); - buffer.handle.update(cx, |buffer, cx| { - buffer.start_transaction(); - buffer.apply_diff(diff, cx); - transaction_id_format = - transaction_id_format.or(buffer.end_transaction(cx)); - if let Some(transaction_id) = transaction_id_format { - buffer.group_until_transaction(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, + ) + .await + .context("failed to format via language server")? + }; + + if edits.is_empty() { + zlog::trace!(logger => "No changes"); + continue; } - Formatter::LanguageServer { name } => { - let logger = zlog::scoped!(logger => "language-server"); - zlog::trace!(logger => "formatting"); - let _timer = - zlog::time!(logger => "Formatting buffer using language server"); - - 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 'formatters; - }; - - let language_server = 'language_server: { - // if a name was provided, try to find the server with that name - if let Some(name) = name.as_deref() { - for (adapter, server) in &adapters_and_servers { - if adapter.name.0.as_ref() == name { - break 'language_server server.clone(); - } - } - } - - // otherwise, fall back to the primary language server for the buffer if one exists - let default_lsp = lsp_store.update(cx, |lsp_store, cx| { - buffer.handle.update(cx, |buffer, cx| { - lsp_store - .as_local() - .unwrap() - .primary_language_server_for_buffer(buffer, cx) - .map(|(_, lsp)| lsp.clone()) - }) - })?; - - if let Some(default_lsp) = default_lsp { - break 'language_server default_lsp; - } else { - log::warn!( - "No language server found to format buffer '{:?}'. Skipping", - buffer_path_abs.as_path().to_string_lossy() - ); - continue 'formatters; - } - }; - - zlog::trace!( - logger => - "Formatting buffer '{:?}' using language server '{:?}'", - buffer_path_abs.as_path().to_string_lossy(), - language_server.name() - ); - - let edits_result = 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, - ) - .await - .context("failed to format via language server") - }; - - let Ok(edits) = edits_result else { - result = Err(edits_result.unwrap_err()); - zlog::error!(logger => "Failed, reason: {:?}", result.as_ref()); - break 'formatters; - }; - - if edits.is_empty() { - zlog::trace!(logger => "No changes"); - continue 'formatters; - } - if let Some(err) = - err_if_buffer_edited_since_start(buffer, transaction_id_format, &cx) - { - zlog::warn!(logger => "Buffer edited while formatting. Aborting"); - result = Err(err); - break 'formatters; - } - zlog::trace!(logger => "Applying changes"); - buffer.handle.update(cx, |buffer, cx| { - buffer.start_transaction(); + extend_formatting_transaction( + buffer, + formatting_transaction_id, + cx, + |buffer, cx| { buffer.edit(edits, None, cx); - transaction_id_format = - transaction_id_format.or(buffer.end_transaction(cx)); - if let Some(transaction_id) = transaction_id_format { - buffer.group_until_transaction(transaction_id); - } - })?; - } - Formatter::CodeActions(code_actions) => { - let logger = zlog::scoped!(logger => "code-actions"); - zlog::trace!(logger => "formatting"); - let _timer = zlog::time!(logger => "Formatting buffer using code actions"); + }, + )?; + } + Formatter::CodeActions(code_actions) => { + 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 code actions. Skipping"); - continue 'formatters; + 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"); + continue; + }; + let code_action_kinds = code_actions + .iter() + .filter_map(|(action_kind, enabled)| { + enabled.then_some(action_kind.clone().into()) + }) + .collect::>(); + if code_action_kinds.is_empty() { + zlog::trace!(logger => "No code action kinds enabled, skipping"); + continue; + } + zlog::trace!(logger => "Attempting to resolve code actions {:?}", &code_action_kinds); + + 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(), + code_action_kinds.clone(), + &buffer.handle, + cx, + ) + .await + .with_context( + || format!("Failed to resolve code actions with kinds {:?} for language server {}", + code_action_kinds.iter().map(|kind| kind.as_str()).join(", "), + 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 actions with kinds {:?} with language server {}", + code_action_kinds.iter().map(|kind| kind.as_str()).join(", "), + language_server.name() + ); + continue; }; - let code_action_kinds = code_actions - .iter() - .filter_map(|(action_kind, enabled)| { - enabled.then_some(action_kind.clone().into()) - }) - .collect::>(); - if code_action_kinds.is_empty() { - zlog::trace!(logger => "No code action kinds enabled, skipping"); - continue 'formatters; + 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(), + ) + }; + + zlog::trace!(logger => "Executing {}", describe_code_action(&action)); + + if let Err(err) = Self::try_resolve_code_action(server, &mut action).await { + zlog::error!( + logger => + "Failed to resolve {}. Error: {}", + describe_code_action(&action), + err + ); + continue; } - 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(), - code_action_kinds.clone(), - &buffer.handle, - cx, - ) - .await - .with_context( - || format!("Failed to resolve code actions with kinds {:?} for language server {}", - code_action_kinds.iter().map(|kind| kind.as_str()).join(", "), - 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 cause 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 actions with kinds {:?} with language server {}", - code_action_kinds.iter().map(|kind| kind.as_str()).join(", "), - 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::trace!(logger => "No code actions were resolved, continuing"); - continue 'formatters; - } + 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(), + }) + })); + } - 'actions: for (mut action, server_index) in actions_and_servers { - let server = &adapters_and_servers[server_index].1; + let mut edits = Vec::with_capacity(operations.len()); - 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(), - ) - }; - - zlog::trace!(logger => "Executing {}", describe_code_action(&action)); - - if let Err(err) = - Self::try_resolve_code_action(server, &mut action).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 'actions; + 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 cause 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 => - "No changes for code action. Skipping {}", - describe_code_action(&action), - ); - continue 'actions; - } - - 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, + 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; } - } 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!( + }; + 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 '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; } - 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"); + 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; - } - - if let Some(err) = err_if_buffer_edited_since_start( - buffer, - transaction_id_format, - &cx, - ) { - zlog::warn!(logger => "Buffer edited while formatting. Aborting"); - result = Err(err); - break 'formatters; - } - zlog::info!(logger => "Applying changes"); - buffer.handle.update(cx, |buffer, cx| { - buffer.start_transaction(); - buffer.edit(edits, None, cx); - transaction_id_format = - transaction_id_format.or(buffer.end_transaction(cx)); - if let Some(transaction_id) = transaction_id_format { - buffer.group_until_transaction(transaction_id); - } - })?; + }; + edits.extend(resolved_edits); } - if let Some(command) = action.lsp_action.command() { + + if edits.is_empty() { + zlog::warn!(logger => "No edits resolved from LSP"); + continue; + } + + extend_formatting_transaction( + buffer, + formatting_transaction_id, + cx, + |buffer, cx| { + buffer.edit(edits, None, cx); + }, + )?; + } + + if let Some(command) = action.lsp_action.command() { + 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, + ); + + // bail early if command is invalid + 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 => - "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, + "Cannot execute a command {} not listed in the language server capabilities of server {}", + command.command, + server.name(), ); - // bail early and command is invalid - { - 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 'actions; - } - } + continue; + } - if let Some(err) = err_if_buffer_edited_since_start( - buffer, - transaction_id_format, - &cx, - ) { - zlog::warn!(logger => "Buffer edited while formatting. Aborting"); - result = Err(err); - break 'formatters; - } - zlog::info!(logger => "Executing command {}", &command.command); + // noop so we just ensure buffer hasn't been edited since resolving code actions + extend_formatting_transaction( + buffer, + formatting_transaction_id, + cx, + |_, _| {}, + )?; + 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 execute_command_result = server + .request::( + lsp::ExecuteCommandParams { + command: command.command.clone(), + arguments: command.arguments.clone().unwrap_or_default(), + ..Default::default() + }, + ) + .await; + + 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 mut project_transaction_command = lsp_store.update(cx, |this, _| { this.as_local_mut() .unwrap() .last_workspace_edits_by_language_server - .remove(&server.server_id()); + .remove(&server.server_id()) + .unwrap_or_default() })?; - let execute_command_result = server - .request::( - lsp::ExecuteCommandParams { - command: command.command.clone(), - arguments: command - .arguments - .clone() - .unwrap_or_default(), - ..Default::default() - }, - ) - .await; - - 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 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 => - "Successfully captured {} edits that resulted from command {}", - transaction.edit_ids.len(), - &command.command, - ); - if let Some(transaction_id_format) = transaction_id_format { - 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, - transaction_id_format, - ); - })?; - } else { - transaction_id_format = Some(transaction.id); + 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()); } - } - if !project_transaction_command.0.is_empty() { - let extra_buffers = project_transaction_command - .0 - .keys() - .filter_map(|buffer_handle| { - buffer_handle - .read_with(cx, |b, cx| b.project_path(cx)) - .ok() - .flatten() - }) - .map(|p| p.path.to_sanitized_string()) - .join(", "); - zlog::warn!( - logger => - "Unexpected edits to buffers other than the buffer actively being formatted due to command {}. Impacted buffers: [{}].", - &command.command, - extra_buffers, + buffer.merge_transactions( + transaction_id_project_transaction, + formatting_transaction_id, ); - for (buffer_handle, transaction) in - project_transaction_command.0 - { - let entry = project_transaction.0.entry(buffer_handle); - use std::collections::hash_map::Entry; - match entry { - // if project_transaction already contains a transaction for this buffer, then - // we already formatted it, and we need to merge the new transaction into the one - // in project_transaction that we created while formatting - Entry::Occupied(mut occupied_entry) => { - let buffer_handle = occupied_entry.key(); - buffer_handle.update(cx, |buffer, _| { - // if push_to_history is true, then we need to make sure it is merged in the - // buffer history as well - if push_to_history { - let transaction_id_project_transaction = - transaction.id; - // ensure transaction from command project transaction - // is in history so we can merge - if buffer - .get_transaction(transaction.id) - .is_none() - { - buffer.push_transaction( - transaction.clone(), - Instant::now(), - ); - } - let transaction_id_existing = - occupied_entry.get().id; - buffer.merge_transactions( - transaction_id_project_transaction, - transaction_id_existing, - ); - } - })?; - let transaction_existing = occupied_entry.get_mut(); - transaction_existing.merge_in(transaction); - } - // if there is no transaction in project_transaction, we either haven't formatted the buffer yet, - // or we won't format this buffer - // later iterations over the formattable_buffers list will use this as the starting transaction - // for the formatting on that buffer - Entry::Vacant(vacant_entry) => { - vacant_entry.insert(transaction); - } - } - } - } + })?; + } + + if !project_transaction_command.0.is_empty() { + let extra_buffers = project_transaction_command + .0 + .keys() + .filter_map(|buffer_handle| { + buffer_handle + .read_with(cx, |b, cx| b.project_path(cx)) + .ok() + .flatten() + }) + .map(|p| p.path.to_sanitized_string()) + .join(", "); + 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 } } } } } - - let buffer_handle = buffer.handle.clone(); - buffer.handle.update(cx, |buffer, _| { - let Some(transaction_id) = transaction_id_format else { - zlog::trace!(logger => "No formatting transaction id"); - return result; - }; - let Some(transaction_id_last) = - buffer.peek_undo_stack().map(|t| t.transaction_id()) - else { - // unwrapping should work here, how would we get a transaction id - // with no transaction on the undo stack? - // *but* it occasionally panics. Avoiding panics for now... - zlog::warn!(logger => "No transaction present on undo stack, despite having a formatting transaction id?"); - return result; - }; - if transaction_id_last != transaction_id { - zlog::trace!(logger => "Last transaction on undo stack is not the formatting transaction, skipping finalization & update of project transaction"); - return result; - } - let transaction = buffer - .finalize_last_transaction() - .cloned() - .expect("There is a transaction on the undo stack if we were able to peek it"); - // debug_assert_eq!(transaction.id, transaction_id); - if !push_to_history { - zlog::trace!(logger => "forgetting format transaction"); - buffer.forget_transaction(transaction.id); - } - project_transaction - .0 - .insert(buffer_handle.clone(), transaction); - return result; - })??; } - return Ok(project_transaction); + Ok(()) } pub async fn format_ranges_via_lsp( @@ -3088,19 +2917,7 @@ impl LocalLspStore { .await?; let transaction = buffer_to_edit.update(cx, |buffer, cx| { - let is_formatting = this.read_with(cx, |lsp_store, _| { - lsp_store - .as_local() - .map_or(false, |local| !local.buffers_being_formatted.is_empty()) - }); - // finalizing last transaction breaks workspace edits received due to - // code actions that are run as part of formatting because formatting - // groups transactions from format steps together to allow undoing all formatting with - // a single undo, and to bail when the user modifies the buffer during formatting - // finalizing the transaction prevents the necessary grouping from happening - if !is_formatting { - buffer.finalize_last_transaction(); - } + buffer.finalize_last_transaction(); buffer.start_transaction(); for (range, text) in edits { buffer.edit([(range, text)], None, cx); @@ -3108,10 +2925,7 @@ impl LocalLspStore { let transaction = buffer.end_transaction(cx).and_then(|transaction_id| { if push_to_history { - if !is_formatting { - // see comment above about finalizing during formatting - buffer.finalize_last_transaction(); - } + buffer.finalize_last_transaction(); buffer.get_transaction(transaction_id).cloned() } else { buffer.forget_transaction(transaction_id)