From 8338db6c1247a22e0e42ab1b24acc45ae4837149 Mon Sep 17 00:00:00 2001
From: Jane Lewis <jane@astral.sh>
Date: Tue, 4 Jun 2024 17:18:21 -0700
Subject: [PATCH] `ruff server`: Formatting a document with syntax problems no
 longer spams a visible error popup (#11745)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

## Summary

Fixes https://github.com/astral-sh/ruff-vscode/issues/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>
---
 crates/ruff_server/src/format.rs              | 46 +++++++++++++++----
 .../src/server/api/requests/format.rs         | 10 ++--
 .../src/server/api/requests/format_range.rs   | 14 +++---
 3 files changed, 48 insertions(+), 22 deletions(-)

diff --git a/crates/ruff_server/src/format.rs b/crates/ruff_server/src/format.rs
index 05708326be..a2257ccedf 100644
--- 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,10 +10,25 @@ 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(
@@ -21,12 +36,23 @@ pub(crate) fn format_range(
     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
index 917b6bf45a..b8d4d27c1f 100644
--- 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
index 7b678a3e16..54ea1699b8 100644
--- 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(),
+        }]
+    }))
 }