ruff server now supports source.fixAll source action (#10597)

## Summary

`ruff server` now has source action `source.fixAll` as an available code
action.

This also fixes https://github.com/astral-sh/ruff/issues/10593 in the
process of revising the code for quick fix code actions.

## Test Plan




f4c07425-e68a-445f-a4ed-949c9197a6be
This commit is contained in:
Jane Lewis 2024-04-03 09:22:17 -07:00 committed by GitHub
parent d467aa78c2
commit 257964a8bc
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
15 changed files with 564 additions and 191 deletions

View file

@ -16,8 +16,8 @@ use super::{client::Responder, schedule::BackgroundSchedule, Result};
/// given the parameter type used by the implementer.
macro_rules! define_document_url {
($params:ident: &$p:ty) => {
fn document_url($params: &$p) -> &lsp_types::Url {
&$params.text_document.uri
fn document_url($params: &$p) -> std::borrow::Cow<lsp_types::Url> {
std::borrow::Cow::Borrowed(&$params.text_document.uri)
}
};
}
@ -28,10 +28,13 @@ pub(super) fn request<'a>(req: server::Request) -> Task<'a> {
let id = req.id.clone();
match req.method.as_str() {
request::CodeAction::METHOD => background_request_task::<request::CodeAction>(
request::CodeActions::METHOD => background_request_task::<request::CodeActions>(
req,
BackgroundSchedule::LatencySensitive,
),
request::CodeActionResolve::METHOD => {
background_request_task::<request::CodeActionResolve>(req, BackgroundSchedule::Worker)
}
request::DocumentDiagnostic::METHOD => {
background_request_task::<request::DocumentDiagnostic>(
req,
@ -102,7 +105,7 @@ fn background_request_task<'a, R: traits::BackgroundDocumentRequestHandler>(
let (id, params) = cast_request::<R>(req)?;
Ok(Task::background(schedule, move |session: &Session| {
// TODO(jane): we should log an error if we can't take a snapshot.
let Some(snapshot) = session.take_snapshot(R::document_url(&params)) else {
let Some(snapshot) = session.take_snapshot(&R::document_url(&params)) else {
return Box::new(|_, _| {});
};
Box::new(move |notifier, responder| {
@ -131,7 +134,7 @@ fn background_notification_thread<'a, N: traits::BackgroundDocumentNotificationH
let (id, params) = cast_notification::<N>(req)?;
Ok(Task::background(schedule, move |session: &Session| {
// TODO(jane): we should log an error if we can't take a snapshot.
let Some(snapshot) = session.take_snapshot(N::document_url(&params)) else {
let Some(snapshot) = session.take_snapshot(&N::document_url(&params)) else {
return Box::new(|_, _| {});
};
Box::new(move |notifier, _| {

View file

@ -1,4 +1,5 @@
mod code_action;
mod code_action_resolve;
mod diagnostic;
mod format;
mod format_range;
@ -7,7 +8,8 @@ use super::{
define_document_url,
traits::{BackgroundDocumentRequestHandler, RequestHandler},
};
pub(super) use code_action::CodeAction;
pub(super) use code_action::CodeActions;
pub(super) use code_action_resolve::CodeActionResolve;
pub(super) use diagnostic::DocumentDiagnostic;
pub(super) use format::Format;
pub(super) use format_range::FormatRange;

View file

@ -1,81 +1,131 @@
use crate::edit::ToRangeExt;
use crate::lint::fixes_for_diagnostics;
use crate::server::api::LSPResult;
use crate::server::SupportedCodeAction;
use crate::server::{client::Notifier, Result};
use crate::session::DocumentSnapshot;
use crate::DIAGNOSTIC_NAME;
use lsp_server::ErrorCode;
use lsp_types::{self as types, request as req};
use ruff_text_size::Ranged;
use rustc_hash::FxHashSet;
use types::{CodeActionKind, CodeActionOrCommand};
pub(crate) struct CodeAction;
use super::code_action_resolve::resolve_edit_for_fix_all;
impl super::RequestHandler for CodeAction {
pub(crate) struct CodeActions;
impl super::RequestHandler for CodeActions {
type RequestType = req::CodeActionRequest;
}
impl super::BackgroundDocumentRequestHandler for CodeAction {
impl super::BackgroundDocumentRequestHandler for CodeActions {
super::define_document_url!(params: &types::CodeActionParams);
fn run_with_snapshot(
snapshot: DocumentSnapshot,
_notifier: Notifier,
params: types::CodeActionParams,
) -> Result<Option<types::CodeActionResponse>> {
let document = snapshot.document();
let url = snapshot.url();
let encoding = snapshot.encoding();
let version = document.version();
let actions: Result<Vec<_>> = params
.context
.diagnostics
.into_iter()
.map(|diagnostic| {
let Some(data) = diagnostic.data else {
return Ok(None);
};
let diagnostic_fix: crate::lint::DiagnosticFix = serde_json::from_value(data)
.map_err(|err| anyhow::anyhow!("failed to deserialize diagnostic data: {err}"))
.with_failure_code(lsp_server::ErrorCode::ParseError)?;
let edits = diagnostic_fix
.fix
.edits()
.iter()
.map(|edit| types::TextEdit {
range: edit.range().to_range(
document.contents(),
document.index(),
encoding,
),
new_text: edit.content().unwrap_or_default().to_string(),
});
let mut response: types::CodeActionResponse = types::CodeActionResponse::default();
let changes = vec![types::TextDocumentEdit {
text_document: types::OptionalVersionedTextDocumentIdentifier::new(
url.clone(),
version,
),
edits: edits.map(types::OneOf::Left).collect(),
}];
let supported_code_actions = supported_code_actions(params.context.only);
let title = diagnostic_fix
.kind
.suggestion
.unwrap_or(diagnostic_fix.kind.name);
Ok(Some(types::CodeAction {
title,
kind: Some(types::CodeActionKind::QUICKFIX),
edit: Some(types::WorkspaceEdit {
document_changes: Some(types::DocumentChanges::Edits(changes)),
..Default::default()
}),
..Default::default()
}))
})
.collect();
if supported_code_actions.contains(&SupportedCodeAction::QuickFix) {
response.extend(
quick_fix(&snapshot, params.context.diagnostics)
.with_failure_code(ErrorCode::InternalError)?,
);
}
Ok(Some(
actions?
.into_iter()
.flatten()
.map(types::CodeActionOrCommand::CodeAction)
.collect(),
))
if supported_code_actions.contains(&SupportedCodeAction::SourceFixAll) {
response.push(fix_all(&snapshot).with_failure_code(ErrorCode::InternalError)?);
}
if supported_code_actions.contains(&SupportedCodeAction::SourceOrganizeImports) {
todo!("Implement the `source.organizeImports` code action");
}
Ok(Some(response))
}
}
fn quick_fix(
snapshot: &DocumentSnapshot,
diagnostics: Vec<types::Diagnostic>,
) -> crate::Result<impl Iterator<Item = CodeActionOrCommand> + '_> {
let document = snapshot.document();
let fixes = fixes_for_diagnostics(
document,
snapshot.url(),
snapshot.encoding(),
document.version(),
diagnostics,
)?;
Ok(fixes.into_iter().map(|fix| {
types::CodeActionOrCommand::CodeAction(types::CodeAction {
title: format!("{DIAGNOSTIC_NAME} ({}): {}", fix.code, fix.title),
kind: Some(types::CodeActionKind::QUICKFIX),
edit: Some(types::WorkspaceEdit {
document_changes: Some(types::DocumentChanges::Edits(fix.document_edits.clone())),
..Default::default()
}),
diagnostics: Some(vec![fix.fixed_diagnostic.clone()]),
data: Some(serde_json::to_value(snapshot.url()).expect("document url to serialize")),
..Default::default()
})
}))
}
fn fix_all(snapshot: &DocumentSnapshot) -> crate::Result<CodeActionOrCommand> {
let document = snapshot.document();
let (edit, data) = if snapshot
.resolved_client_capabilities()
.code_action_deferred_edit_resolution
{
// The editor will request the edit in a `CodeActionsResolve` request
(
None,
Some(serde_json::to_value(snapshot.url()).expect("document url to serialize")),
)
} else {
(
Some(resolve_edit_for_fix_all(
document,
snapshot.url(),
&snapshot.configuration().linter,
snapshot.encoding(),
)?),
None,
)
};
let action = types::CodeAction {
title: format!("{DIAGNOSTIC_NAME}: Fix all auto-fixable problems"),
kind: Some(types::CodeActionKind::SOURCE_FIX_ALL),
edit,
data,
..Default::default()
};
Ok(types::CodeActionOrCommand::CodeAction(action))
}
/// If `action_filter` is `None`, this returns [`SupportedCodeActionKind::all()`]. Otherwise,
/// the list is filtered.
fn supported_code_actions(
action_filter: Option<Vec<CodeActionKind>>,
) -> FxHashSet<SupportedCodeAction> {
let Some(action_filter) = action_filter else {
return SupportedCodeAction::all().collect();
};
SupportedCodeAction::all()
.filter(move |action| {
action_filter.iter().any(|filter| {
action
.kinds()
.iter()
.any(|kind| kind.as_str().starts_with(filter.as_str()))
})
})
.collect()
}

View file

@ -0,0 +1,82 @@
use std::borrow::Cow;
use crate::server::api::LSPResult;
use crate::server::SupportedCodeAction;
use crate::server::{client::Notifier, Result};
use crate::session::DocumentSnapshot;
use crate::PositionEncoding;
use lsp_server::ErrorCode;
use lsp_types::{self as types, request as req};
use ruff_linter::settings::LinterSettings;
pub(crate) struct CodeActionResolve;
impl super::RequestHandler for CodeActionResolve {
type RequestType = req::CodeActionResolveRequest;
}
impl super::BackgroundDocumentRequestHandler for CodeActionResolve {
fn document_url(params: &types::CodeAction) -> Cow<types::Url> {
let uri: lsp_types::Url = serde_json::from_value(params.data.clone().unwrap_or_default())
.expect("code actions should have a URI in their data fields");
std::borrow::Cow::Owned(uri)
}
fn run_with_snapshot(
snapshot: DocumentSnapshot,
_notifier: Notifier,
mut action: types::CodeAction,
) -> Result<types::CodeAction> {
let document = snapshot.document();
let action_kind: SupportedCodeAction = action
.kind
.clone()
.ok_or(anyhow::anyhow!("No kind was given for code action"))
.with_failure_code(ErrorCode::InvalidParams)?
.try_into()
.map_err(|()| anyhow::anyhow!("Code action was of an invalid kind"))
.with_failure_code(ErrorCode::InvalidParams)?;
action.edit = match action_kind {
SupportedCodeAction::SourceFixAll => Some(
resolve_edit_for_fix_all(
document,
snapshot.url(),
&snapshot.configuration().linter,
snapshot.encoding(),
)
.with_failure_code(ErrorCode::InternalError)?,
),
SupportedCodeAction::SourceOrganizeImports => {
todo!("Support `source.organizeImports`")
}
SupportedCodeAction::QuickFix => {
return Err(anyhow::anyhow!(
"Got a code action that should not need additional resolution: {action_kind:?}"
))
.with_failure_code(ErrorCode::InvalidParams)
}
};
Ok(action)
}
}
pub(super) fn resolve_edit_for_fix_all(
document: &crate::edit::Document,
url: &types::Url,
linter_settings: &LinterSettings,
encoding: PositionEncoding,
) -> crate::Result<types::WorkspaceEdit> {
Ok(types::WorkspaceEdit {
changes: Some(
[(
url.clone(),
crate::fix::fix_all(document, linter_settings, encoding)?,
)]
.into_iter()
.collect(),
),
..Default::default()
})
}

View file

@ -1,10 +1,9 @@
use crate::edit::ToRangeExt;
use crate::edit::{Replacement, ToRangeExt};
use crate::server::api::LSPResult;
use crate::server::{client::Notifier, Result};
use crate::session::DocumentSnapshot;
use lsp_types::{self as types, request as req};
use ruff_source_file::LineIndex;
use ruff_text_size::{TextLen, TextRange, TextSize};
use types::TextEdit;
pub(crate) struct Format;
@ -33,8 +32,8 @@ impl super::BackgroundDocumentRequestHandler for Format {
let unformatted_index = doc.index();
let Replacement {
source_range: replace_range,
formatted_range: replacement_text_range,
source_range,
modified_range: formatted_range,
} = Replacement::between(
source,
unformatted_index.line_starts(),
@ -43,105 +42,8 @@ impl super::BackgroundDocumentRequestHandler for Format {
);
Ok(Some(vec![TextEdit {
range: replace_range.to_range(source, unformatted_index, snapshot.encoding()),
new_text: formatted[replacement_text_range].to_owned(),
range: source_range.to_range(source, unformatted_index, snapshot.encoding()),
new_text: formatted[formatted_range].to_owned(),
}]))
}
}
struct Replacement {
source_range: TextRange,
formatted_range: TextRange,
}
impl Replacement {
/// Creates a [`Replacement`] that describes the `replace_range` of `old_text` to replace
/// with `new_text` sliced by `replacement_text_range`.
fn between(
source: &str,
source_line_starts: &[TextSize],
formatted: &str,
formatted_line_starts: &[TextSize],
) -> Self {
let mut source_start = TextSize::default();
let mut formatted_start = TextSize::default();
let mut source_end = source.text_len();
let mut formatted_end = formatted.text_len();
let mut line_iter = source_line_starts
.iter()
.copied()
.zip(formatted_line_starts.iter().copied());
for (source_line_start, formatted_line_start) in line_iter.by_ref() {
if source_line_start != formatted_line_start
|| source[TextRange::new(source_start, source_line_start)]
!= formatted[TextRange::new(formatted_start, formatted_line_start)]
{
break;
}
source_start = source_line_start;
formatted_start = formatted_line_start;
}
let mut line_iter = line_iter.rev();
for (old_line_start, new_line_start) in line_iter.by_ref() {
if old_line_start <= source_start
|| new_line_start <= formatted_start
|| source[TextRange::new(old_line_start, source_end)]
!= formatted[TextRange::new(new_line_start, formatted_end)]
{
break;
}
source_end = old_line_start;
formatted_end = new_line_start;
}
Replacement {
source_range: TextRange::new(source_start, source_end),
formatted_range: TextRange::new(formatted_start, formatted_end),
}
}
}
#[cfg(test)]
mod tests {
use ruff_source_file::LineIndex;
use crate::server::api::requests::format::Replacement;
#[test]
fn find_replacement_range_works() {
let original = r#"
aaaa
bbbb
cccc
dddd
eeee
"#;
let original_index = LineIndex::from_source_text(original);
let new = r#"
bb
cccc
dd
"#;
let new_index = LineIndex::from_source_text(new);
let expected = r#"
bb
cccc
dd
"#;
let replacement = Replacement::between(
original,
original_index.line_starts(),
new,
new_index.line_starts(),
);
let mut test = original.to_string();
test.replace_range(
replacement.source_range.start().to_usize()..replacement.source_range.end().to_usize(),
&new[replacement.formatted_range],
);
assert_eq!(expected, &test);
}
}

View file

@ -31,7 +31,7 @@ pub(super) trait BackgroundDocumentRequestHandler: RequestHandler {
/// implementation.
fn document_url(
params: &<<Self as RequestHandler>::RequestType as Request>::Params,
) -> &lsp_types::Url;
) -> std::borrow::Cow<lsp_types::Url>;
fn run_with_snapshot(
snapshot: DocumentSnapshot,
@ -66,7 +66,7 @@ pub(super) trait BackgroundDocumentNotificationHandler: NotificationHandler {
/// implementation.
fn document_url(
params: &<<Self as NotificationHandler>::NotificationType as LSPNotification>::Params,
) -> &lsp_types::Url;
) -> std::borrow::Cow<lsp_types::Url>;
fn run_with_snapshot(
snapshot: DocumentSnapshot,