ruff server: Formatting a document with syntax problems no longer spams a visible error popup (#11745)

snowsignal · zanieb · web-flow · commit 8338db6c1247 · 2024-06-04T17:18:21.000-07:00
## 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>
diff --git a/crates/ruff_server/src/format.rs b/crates/ruff_server/src/format.rs
@@ -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;
 
@@ -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()),
+    }
 }
diff --git a/crates/ruff_server/src/server/api/requests/format.rs b/crates/ruff_server/src/server/api/requests/format.rs
@@ -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 {
diff --git a/crates/ruff_server/src/server/api/requests/format_range.rs b/crates/ruff_server/src/server/api/requests/format_range.rs
@@ -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(),
+        }]
+    }))
 }
