From 0784e1c72fd090e5337b860165f595a157ae4c26 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Thu, 4 Apr 2024 11:41:55 +0200 Subject: [PATCH] Improve handling of prettier errors on format (#10156) 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. --- crates/project/src/prettier_support.rs | 86 +++++++++++++------------- crates/project/src/project.rs | 18 +++--- 2 files changed, 53 insertions(+), 51 deletions(-) diff --git a/crates/project/src/prettier_support.rs b/crates/project/src/prettier_support.rs index 22463f7210db9ad0282142b378e689275c07fcf4..537356c39e1810258e2714d25cdad4d21cd58e0f 100644 --- a/crates/project/src/prettier_support.rs +++ b/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::{ @@ -44,51 +44,48 @@ pub(super) async fn format_with_prettier( project: &WeakModel, buffer: &Model, cx: &mut AsyncAppContext, -) -> Option { - if let Some((prettier_path, prettier_task)) = project +) -> Option> { + 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 { @@ -96,11 +93,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 { diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index 2fd39b095ed9a6a25a198da332d8c44a26ffb156..2416caf95b6999cc8d51aa1d6393b82a070ca949 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -4652,10 +4652,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( @@ -4672,10 +4673,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?); } } };