From cd9eb77aceec9a7e42538304a0a7a061e23caeaa Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Tue, 30 Sep 2025 18:13:20 -0500 Subject: [PATCH] Fix bug in code action formatter handling (#39246) Closes #39112 Release Notes: - Fixed an issue when using code actions on format where specifying multiple code actions in the same code actions block that resolved to code actions from different language servers could result in conflicting edits being applied and mangled buffer text. --- crates/editor/src/editor_tests.rs | 25 +- crates/project/src/lsp_store.rs | 600 ++++++++++++++++-------------- 2 files changed, 325 insertions(+), 300 deletions(-) diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 1dc2a1686350c087ae97c08b8425be5ed92f374f..3977f685cccd48fd39052fdc96e97e4e4b17ae62 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -11925,17 +11925,16 @@ async fn test_multiple_formatters(cx: &mut TestAppContext) { ); 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 requested_code_actions = params.context.only.expect("Expected code action request"); + assert_eq!(requested_code_actions.len(), 1); + let uri = lsp::Uri::from_file_path(path!("/file.rs")).unwrap(); - Ok(Some(vec![ - lsp::CodeActionOrCommand::CodeAction(lsp::CodeAction { + let code_action = match requested_code_actions[0].as_str() { + "code-action-1" => lsp::CodeAction { kind: Some("code-action-1".into()), edit: Some(lsp::WorkspaceEdit::new( [( - uri.clone(), + uri, 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(), @@ -11949,8 +11948,8 @@ async fn test_multiple_formatters(cx: &mut TestAppContext) { ..Default::default() }), ..Default::default() - }), - lsp::CodeActionOrCommand::CodeAction(lsp::CodeAction { + }, + "code-action-2" => lsp::CodeAction { kind: Some("code-action-2".into()), edit: Some(lsp::WorkspaceEdit::new( [( @@ -11964,8 +11963,12 @@ async fn test_multiple_formatters(cx: &mut TestAppContext) { .collect(), )), ..Default::default() - }), - ])) + }, + req => panic!("Unexpected code action request: {:?}", req), + }; + Ok(Some(vec![lsp::CodeActionOrCommand::CodeAction( + code_action, + )])) }, ); diff --git a/crates/project/src/lsp_store.rs b/crates/project/src/lsp_store.rs index 14a3f1921c04a6572fb5f4e4535ba4895c556d94..fa0e7c5860ebacdddadc14f623b9002ec804f3bb 100644 --- a/crates/project/src/lsp_store.rs +++ b/crates/project/src/lsp_store.rs @@ -1521,347 +1521,369 @@ impl LocalLspStore { 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() { + if code_actions.iter().filter(|(_, enabled)| **enabled).count() == 0 { 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() - ); + // Note: this loop exists despite `Self::get_server_code_actions_from_action_kinds` taking a `Vec` + // because code actions can resolve edits when `Self::get_server_code_actions_from_action_kinds` is called, which + // are not updated to reflect edits from previous actions or actions from other servers when `resolve_code_actions` is called later + // which can result in conflicting edits and mangled buffer state + for (code_action_name, enabled) in code_actions { + if !enabled { continue; - }; - for action in actions { - actions_and_servers.push((action, index)); } - } + let code_action_kind: CodeActionKind = code_action_name.clone().into(); + zlog::trace!(logger => "Attempting to resolve code actions {:?}", &code_action_kind); - if actions_and_servers.is_empty() { - zlog::warn!(logger => "No code actions were resolved, continuing"); - continue; - } + let mut actions_and_servers = Vec::new(); - '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(), + 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, ) - }; - - 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; - } - - 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!( + .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 => - "No changes for code action. Skipping {}", - describe_code_action(&action), + "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)); } + } - 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(), - }) - })); - } + 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 edits = Vec::with_capacity(operations.len()); + zlog::trace!(logger => "Executing {}", describe_code_action(&action)); - if operations.is_empty() { - zlog::trace!( + if let Err(err) = + Self::try_resolve_code_action(server, &mut action).await + { + zlog::error!( logger => - "No changes for code action. Skipping {}", + "Failed to resolve {}. Error: {}", describe_code_action(&action), + err ); continue; } - 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!( + + 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 convert URI '{:?}' to file path. Skipping {}", - &op.text_document.uri, + "No changes for code action. Skipping {}", describe_code_action(&action), ); - continue 'actions; - }; - if &file_path != buffer_path_abs { - zlog::warn!( + 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!( logger => - "File path '{:?}' does not match buffer path '{:?}'. Skipping {}", - file_path, - buffer_path_abs, + "No changes for code action. Skipping {}", describe_code_action(&action), ); - continue 'actions; + continue; } - - 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(_) => { + for operation in operations { + let op = match operation { + lsp::DocumentChangeOperation::Edit(op) => op, + lsp::DocumentChangeOperation::Op(_) => { zlog::warn!( logger => - "Code actions which produce snippet edits are not supported during formatting. Skipping {}", + "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 => + "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 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; - } + 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); + } - extend_formatting_transaction( - buffer, - formatting_transaction_id, - cx, - |buffer, cx| { - buffer.edit(edits, None, cx); - }, - )?; - } + if edits.is_empty() { + zlog::warn!(logger => "No edits resolved from LSP"); + continue; + } - 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, - ); + 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() + ); + }, + )?; + } - // 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) { + if let Some(command) = action.lsp_action.command() { zlog::warn!( logger => - "Cannot execute a command {} not listed in the language server capabilities of server {}", - command.command, - server.name(), + "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, ); - continue; - } - // 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 - .into_response(); + // 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 => + "Cannot execute a command {} not listed in the language server capabilities of server {}", + command.command, + server.name(), + ); + continue; + } - if execute_command_result.is_err() { - zlog::error!( - logger => - "Failed to execute command '{}' as part of {}", - &command.command, - describe_code_action(&action), - ); - continue 'actions; - } + // 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); - 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() + .remove(&server.server_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()); - } - buffer.merge_transactions( - transaction_id_project_transaction, - formatting_transaction_id, + let execute_command_result = server + .request::( + lsp::ExecuteCommandParams { + command: command.command.clone(), + arguments: command + .arguments + .clone() + .unwrap_or_default(), + ..Default::default() + }, + ) + .await + .into_response(); + + 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 !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 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, + ); + 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() { + 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 + } } } }