Improve handling of prettier errors on format (#10156)

Thorsten Ball created 

When no formatter for a language is specified, Zed has the default
behaviour:

1. Attempt to format the buffer with `prettier`
2. If that doesn't work, use the language server.

The problem was that if `prettier` failed to format a buffer due to
legitimate syntax errors, we simply did a fallback to the language
server, which would then format over the syntax errors.

With JavaScript/React/TypeScript projects this could lead to a situation
where

1. Syntax error was introduced
2. Prettier fails
3. Zed ignores the error
4. typescript-language-server formats the buffer despite syntax errors

This would lead to some very weird formatting issues.

What this PR does is to fix the issue by handling `prettier` errors and
results in two user facing changes:

1. When no formatter is set (or set to `auto`) and if we attempted to
start a prettier instance to format, we will now display that error and
*not* fall back to language server formatting.
2. If the formatter is explicitly set to `prettier`, we will now show
errors if we failed to spawn prettier or failed to format with it.

This means that we now might show *more* errors than previously, but I
think that's better than not showing anything to the user at all.

And, of course, it also fixes the issue of invalid syntax being
formatted by the language server even though `prettier` failed with an
error.

Release Notes:

- Improved error handling when formatting buffers with `prettier`.
Previously `prettier` errors would be logged but ignored. Now `prettier`
errors are shown in the UI, just like language server errors when
formatting. And if no formatter is specified (or set to `"auto"`) and
Zed attempts to use `prettier` for formatting, then `prettier` errors
are no longer skipped. That fixes the issue of `prettier` not formatting
invalid syntax, but its error being skipped, leading to
`typescript-language-server` or another language server formatting
invalid syntax.

Change summary

crates/project/src/prettier_support.rs | 86 ++++++++++++++--------------
crates/project/src/project.rs          | 18 +++--
2 files changed, 53 insertions(+), 51 deletions(-)

Detailed changes

crates/project/src/prettier_support.rs 🔗 

@@ -4,7 +4,7 @@ use std::{
     sync::Arc,
 };
 
-use anyhow::Context;
+use anyhow::{anyhow, Context, Result};
 use collections::HashSet;
 use fs::Fs;
 use futures::{
@@ -46,51 +46,48 @@ pub(super) async fn format_with_prettier(
     project: &WeakModel<Project>,
     buffer: &Model<Buffer>,
     cx: &mut AsyncAppContext,
-) -> Option<FormatOperation> {
-    if let Some((prettier_path, prettier_task)) = project
+) -> Option<Result<FormatOperation>> {
+    let prettier_instance = project
         .update(cx, |project, cx| {
             project.prettier_instance_for_buffer(buffer, cx)
         })
         .ok()?
-        .await
-    {
-        match prettier_task.await {
-            Ok(prettier) => {
-                let buffer_path = buffer
-                    .update(cx, |buffer, cx| {
-                        File::from_dyn(buffer.file()).map(|file| file.abs_path(cx))
-                    })
-                    .ok()?;
-                match prettier.format(buffer, buffer_path, cx).await {
-                    Ok(new_diff) => return Some(FormatOperation::Prettier(new_diff)),
-                    Err(e) => {
-                        match prettier_path {
-                            Some(prettier_path) => log::error!(
-                                "Prettier instance from path {prettier_path:?} failed to format a buffer: {e:#}"
-                            ),
-                            None => log::error!(
-                                "Default prettier instance failed to format a buffer: {e:#}"
-                            ),
-                        }
-                    }
-                }
-            }
-            Err(e) => project
+        .await;
+
+    let Some((prettier_path, prettier_task)) = prettier_instance else {
+        return None;
+    };
+
+    let prettier_description = match prettier_path.as_ref() {
+        Some(path) => format!("prettier at {path:?}"),
+        None => "default prettier instance".to_string(),
+    };
+
+    match prettier_task.await {
+        Ok(prettier) => {
+            let buffer_path = buffer
+                .update(cx, |buffer, cx| {
+                    File::from_dyn(buffer.file()).map(|file| file.abs_path(cx))
+                })
+                .ok()?;
+
+            let format_result = prettier
+                .format(buffer, buffer_path, cx)
+                .await
+                .map(FormatOperation::Prettier)
+                .with_context(|| format!("{} failed to format buffer", prettier_description));
+
+            Some(format_result)
+        }
+        Err(error) => {
+            project
                 .update(cx, |project, _| {
                     let instance_to_update = match prettier_path {
-                        Some(prettier_path) => {
-                            log::error!(
-                            "Prettier instance from path {prettier_path:?} failed to spawn: {e:#}"
-                        );
-                            project.prettier_instances.get_mut(&prettier_path)
-                        }
-                        None => {
-                            log::error!("Default prettier instance failed to spawn: {e:#}");
-                            match &mut project.default_prettier.prettier {
-                                PrettierInstallation::NotInstalled { .. } => None,
-                                PrettierInstallation::Installed(instance) => Some(instance),
-                            }
-                        }
+                        Some(prettier_path) => project.prettier_instances.get_mut(&prettier_path),
+                        None => match &mut project.default_prettier.prettier {
+                            PrettierInstallation::NotInstalled { .. } => None,
+                            PrettierInstallation::Installed(instance) => Some(instance),
+                        },
                     };
 
                     if let Some(instance) = instance_to_update {
@@ -98,11 +95,14 @@ pub(super) async fn format_with_prettier(
                         instance.prettier = None;
                     }
                 })
-                .ok()?,
+                .log_err();
+
+            Some(Err(anyhow!(
+                "{} failed to spawn: {error:#}",
+                prettier_description
+            )))
         }
     }
-
-    None
 }
 
 pub struct DefaultPrettier {

crates/project/src/project.rs 🔗 

@@ -4628,10 +4628,11 @@ impl Project {
                     }
                 }
                 (Formatter::Auto, FormatOnSave::On | FormatOnSave::Off) => {
-                    if let Some(new_operation) =
-                        prettier_support::format_with_prettier(&project, buffer, &mut cx).await
-                    {
-                        format_operation = Some(new_operation);
+                    let prettier =
+                        prettier_support::format_with_prettier(&project, buffer, &mut cx).await;
+
+                    if let Some(operation) = prettier {
+                        format_operation = Some(operation?);
                     } else if let Some((language_server, buffer_abs_path)) = server_and_buffer {
                         format_operation = Some(FormatOperation::Lsp(
                             Self::format_via_lsp(
@@ -4648,10 +4649,11 @@ impl Project {
                     }
                 }
                 (Formatter::Prettier, FormatOnSave::On | FormatOnSave::Off) => {
-                    if let Some(new_operation) =
-                        prettier_support::format_with_prettier(&project, buffer, &mut cx).await
-                    {
-                        format_operation = Some(new_operation);
+                    let prettier =
+                        prettier_support::format_with_prettier(&project, buffer, &mut cx).await;
+
+                    if let Some(operation) = prettier {
+                        format_operation = Some(operation?);
                     }
                 }
             };