Skip to content

Commit

Permalink
ruff server: Formatting a document with syntax problems no longer s…
Browse files Browse the repository at this point in the history
…pams a visible error popup (#11745)

## Summary

Fixes astral-sh/ruff-vscode#482.

I've made adjustments to `format` and `format_range` that handle parsing
errors before they become server errors. We'll still log this as a
problem, but there will no longer be a visible popup.

## Test Plan

Instead of seeing a visible error when formatting a document with syntax
issues, you should see this warning in the LSP logs:

<img width="991" alt="Screenshot 2024-06-04 at 3 38 23 PM"
src="https://github.com/astral-sh/ruff/assets/19577865/9d68947d-6462-4ca6-ab5a-65e573c91db6">

Similarly, if you try to format a range with syntax issues, you should
see this warning in the LSP logs instead of a visible error popup:

<img width="1010" alt="Screenshot 2024-06-04 at 3 39 10 PM"
src="https://github.com/astral-sh/ruff/assets/19577865/99fff098-798d-406a-976e-81ead0da0352">

---------

Co-authored-by: Zanie Blue <contact@zanie.dev>
  • Loading branch information
snowsignal and zanieb committed Jun 5, 2024
1 parent d056d09 commit 8338db6
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 22 deletions.
46 changes: 36 additions & 10 deletions crates/ruff_server/src/format.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use ruff_formatter::PrintedRange;
use ruff_python_ast::PySourceType;
use ruff_python_formatter::format_module_source;
use ruff_python_formatter::{format_module_source, FormatModuleError};
use ruff_text_size::TextRange;
use ruff_workspace::FormatterSettings;

Expand All @@ -10,23 +10,49 @@ pub(crate) fn format(
document: &TextDocument,
source_type: PySourceType,
formatter_settings: &FormatterSettings,
) -> crate::Result<String> {
) -> crate::Result<Option<String>> {
let format_options = formatter_settings.to_format_options(source_type, document.contents());
let formatted = format_module_source(document.contents(), format_options)?;
Ok(formatted.into_code())
match format_module_source(document.contents(), format_options) {
Ok(formatted) => {
let formatted = formatted.into_code();
if formatted == document.contents() {
Ok(None)
} else {
Ok(Some(formatted))
}
}
// Special case - syntax/parse errors are handled here instead of
// being propagated as visible server errors.
Err(FormatModuleError::ParseError(error)) => {
tracing::warn!("Unable to format document: {error}");
Ok(None)
}
Err(err) => Err(err.into()),
}
}

pub(crate) fn format_range(
document: &TextDocument,
source_type: PySourceType,
formatter_settings: &FormatterSettings,
range: TextRange,
) -> crate::Result<PrintedRange> {
) -> crate::Result<Option<PrintedRange>> {
let format_options = formatter_settings.to_format_options(source_type, document.contents());

Ok(ruff_python_formatter::format_range(
document.contents(),
range,
format_options,
)?)
match ruff_python_formatter::format_range(document.contents(), range, format_options) {
Ok(formatted) => {
if formatted.as_code() == document.contents() {
Ok(None)
} else {
Ok(Some(formatted))
}
}
// Special case - syntax/parse errors are handled here instead of
// being propagated as visible server errors.
Err(FormatModuleError::ParseError(error)) => {
tracing::warn!("Unable to format document range: {error}");
Ok(None)
}
Err(err) => Err(err.into()),
}
}
10 changes: 4 additions & 6 deletions crates/ruff_server/src/server/api/requests/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,11 @@ fn format_text_document(
}

let source = text_document.contents();
let mut formatted =
crate::format::format(text_document, query.source_type(), formatter_settings)
.with_failure_code(lsp_server::ErrorCode::InternalError)?;
// fast path - if the code is the same, return early
if formatted == source {
let formatted = crate::format::format(text_document, query.source_type(), formatter_settings)
.with_failure_code(lsp_server::ErrorCode::InternalError)?;
let Some(mut formatted) = formatted else {
return Ok(None);
}
};

// special case - avoid adding a newline to a notebook cell if it didn't already exist
if is_notebook {
Expand Down
14 changes: 8 additions & 6 deletions crates/ruff_server/src/server/api/requests/format_range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,12 @@ fn format_text_document_range(
)
.with_failure_code(lsp_server::ErrorCode::InternalError)?;

Ok(Some(vec![types::TextEdit {
range: formatted_range
.source_range()
.to_range(text, index, encoding),
new_text: formatted_range.into_code(),
}]))
Ok(formatted_range.map(|formatted_range| {
vec![types::TextEdit {
range: formatted_range
.source_range()
.to_range(text, index, encoding),
new_text: formatted_range.into_code(),
}]
}))
}

0 comments on commit 8338db6

Please sign in to comment.