diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 46b7ee6a6522579d5872868fcb173bfe3f0297ba..bd3cb0ef8e06bbef9709f4cd4225633f1c70b75e 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -11934,17 +11934,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(), @@ -11958,8 +11957,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( [( @@ -11973,8 +11972,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 6a5d118951ed332097ec318ae2005f7cc3349586..9d328f04763d36904490e91c158709aef5beae2b 100644 --- a/crates/project/src/lsp_store.rs +++ b/crates/project/src/lsp_store.rs @@ -1521,349 +1521,371 @@ 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 mut extra_buffers = String::new(); - for buffer in project_transaction_command.0.keys() { - buffer - .read_with(cx, |b, cx| { - if let Some(path) = b.project_path(cx) { - if !extra_buffers.is_empty() { - extra_buffers.push_str(", "); + 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 mut extra_buffers = String::new(); + for buffer in project_transaction_command.0.keys() { + buffer + .read_with(cx, |b, cx| { + if let Some(path) = b.project_path(cx) { + if !extra_buffers.is_empty() { + extra_buffers.push_str(", "); + } + extra_buffers.push_str(path.path.as_unix_str()); } - extra_buffers.push_str(path.path.as_unix_str()); - } - }) - .ok(); + }) + .ok(); + } + 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 } - 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 } } }