feat: allow external formatter to cause formatting error (#710)

Author: kt3k

src/format_text.rs

@@ -93,15 +93,20 @@ pub fn format_parsed_source(source: &ParsedSource, config: &Configuration, exter
 }
 
 fn inner_format(parsed_source: &ParsedSource, config: &Configuration, external_formatter: Option<&ExternalFormatter>) -> Result<Option<String>> {
+  let mut maybe_err: Box<Option<anyhow::Error>> = Box::new(None);
   let result = dprint_core::formatting::format(
-    || {
-      #[allow(clippy::let_and_return)]
-      let print_items = generate(parsed_source, config, external_formatter);
-      // println!("{}", print_items.get_as_text());
-      print_items
+    || match generate(parsed_source, config, external_formatter) {
+      Ok(print_items) => print_items,
+      Err(e) => {
+        maybe_err.replace(e);
+        PrintItems::default()
+      }
     },
     config_to_print_options(parsed_source.text(), config),
   );
+  if let Some(e) = maybe_err.take() {
+    return Err(e);
+  }
   if result == parsed_source.text().as_ref() {
     Ok(None)
   } else {
@@ -145,4 +150,21 @@ mod test {
       assert_eq!(result, "const t = 5;\n");
     }
   }
+
+  #[test]
+  fn syntax_error_from_external_formatter() {
+    let config = crate::configuration::ConfigurationBuilder::new().build();
+    let result = format_text(FormatTextOptions {
+      path: &std::path::PathBuf::from("test.ts"),
+      extension: None,
+      text: "const content = html`<div>broken html</p>`".into(),
+      config: &config,
+      external_formatter: Some(&|media_type, _text, _config| {
+        assert!(matches!(media_type, deno_ast::MediaType::Html));
+        Err(anyhow::anyhow!("Syntax error from external formatter"))
+      }),
+    });
+    assert!(result.is_err());
+    assert_eq!(result.unwrap_err().to_string(), "Error formatting tagged template literal at line 0: Syntax error from external formatter");
+  }
 }

src/generation/context.rs

@@ -45,7 +45,11 @@ use crate::utils::Stack;
 /// cases the templates will be left as they are.
 ///
 /// Only templates with no interpolation are supported.
-pub type ExternalFormatter = dyn Fn(MediaType, String, &Configuration) -> Option<String>;
+pub type ExternalFormatter = dyn Fn(MediaType, String, &Configuration) -> anyhow::Result<Option<String>>;
+
+pub(crate) struct GenerateDiagnostic {
+  pub message: String,
+}
 
 pub struct Context<'a> {
   pub media_type: MediaType,
@@ -70,6 +74,7 @@ pub struct Context<'a> {
   /// Used for ensuring nodes are parsed in order.
   #[cfg(debug_assertions)]
   pub last_generated_node_pos: SourcePos,
+  pub diagnostics: Vec<GenerateDiagnostic>,
 }
 
 impl<'a> Context<'a> {
@@ -102,6 +107,7 @@ impl<'a> Context<'a> {
       expr_stmt_single_line_parent_brace_ref: None,
       #[cfg(debug_assertions)]
       last_generated_node_pos: deno_ast::SourceTextInfoProvider::text_info(&program).range().start.into(),
+      diagnostics: Vec::new(),
     }
   }
 

src/generation/generate.rs

@@ -25,7 +25,7 @@ use super::*;
 use crate::configuration::*;
 use crate::utils;
 
-pub fn generate(parsed_source: &ParsedSource, config: &Configuration, external_formatter: Option<&ExternalFormatter>) -> PrintItems {
+pub fn generate(parsed_source: &ParsedSource, config: &Configuration, external_formatter: Option<&ExternalFormatter>) -> anyhow::Result<PrintItems> {
   // eprintln!("Leading: {:?}", parsed_source.comments().leading_map());
   // eprintln!("Trailing: {:?}", parsed_source.comments().trailing_map());
 
@@ -49,10 +49,14 @@ pub fn generate(parsed_source: &ParsedSource, config: &Configuration, external_f
     #[cfg(debug_assertions)]
     context.assert_end_of_file_state();
 
+    if let Some(diagnostic) = context.diagnostics.pop() {
+      return Err(anyhow::anyhow!(diagnostic.message));
+    }
+
     if config.file_indent_level > 0 {
-      with_indent_times(items, config.file_indent_level)
+      Ok(with_indent_times(items, config.file_indent_level))
     } else {
-      items
+      Ok(items)
     }
   })
 }
@@ -3034,7 +3038,15 @@ fn maybe_gen_tagged_tpl_with_external_formatter<'a>(node: &TaggedTpl<'a>, contex
   .unwrap();
 
   // Then formats the text with the external formatter.
-  let formatted_tpl = external_formatter(media_type, text, context.config)?;
+  let formatted_tpl = match external_formatter(media_type, text, context.config) {
+    Ok(formatted_tpl) => formatted_tpl?,
+    Err(err) => {
+      context.diagnostics.push(context::GenerateDiagnostic {
+        message: format!("Error formatting tagged template literal at line {}: {}", node.start_line(), err),
+      });
+      return None;
+    }
+  };
 
   let mut items = PrintItems::new();
   items.push_sc(sc!("`"));

tests/spec_test.rs

@@ -1,13 +1,14 @@
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
 
+use anyhow::Result;
 use deno_ast::MediaType;
 use dprint_core::configuration::*;
 use dprint_development::*;
 use dprint_plugin_typescript::configuration::*;
 use dprint_plugin_typescript::*;
 
-fn external_formatter(media_type: MediaType, text: String, config: &Configuration) -> Option<String> {
+fn external_formatter(media_type: MediaType, text: String, config: &Configuration) -> Result<Option<String>> {
   match media_type {
     MediaType::Css => format_embedded_css(&text, config),
     MediaType::Html => format_html(&text, config),
@@ -16,7 +17,7 @@ fn external_formatter(media_type: MediaType, text: String, config: &Configuratio
   }
 }
 
-fn format_embedded_css(text: &str, config: &Configuration) -> Option<String> {
+fn format_embedded_css(text: &str, config: &Configuration) -> Result<Option<String>> {
   use malva::config;
   let options = config::FormatOptions {
     layout: config::LayoutOptions {
@@ -27,9 +28,7 @@ fn format_embedded_css(text: &str, config: &Configuration) -> Option<String> {
   };
   // Wraps the text in a css block of `a { ... }`
   // to make it valid css (scss)
-  let Ok(text) = malva::format_text(&format!("a{{\n{}\n}}", text), malva::Syntax::Scss, &options) else {
-    return None;
-  };
+  let text = malva::format_text(&format!("a{{\n{}\n}}", text), malva::Syntax::Scss, &options)?;
   let mut buf = vec![];
   for (i, l) in text.lines().enumerate() {
     // skip the first line (a {)
@@ -47,10 +46,10 @@ fn format_embedded_css(text: &str, config: &Configuration) -> Option<String> {
     }
     buf.push(chars.as_str());
   }
-  Some(buf.join("\n").to_string())
+  Ok(Some(buf.join("\n").to_string()))
 }
 
-fn format_html(text: &str, config: &Configuration) -> Option<String> {
+fn format_html(text: &str, config: &Configuration) -> Result<Option<String>> {
   use markup_fmt::config;
   let options = config::FormatOptions {
     layout: config::LayoutOptions {
@@ -59,19 +58,17 @@ fn format_html(text: &str, config: &Configuration) -> Option<String> {
     },
     ..Default::default()
   };
-  let Ok(text) = markup_fmt::format_text(text, markup_fmt::Language::Html, &options, |code, _| {
+  let text = markup_fmt::format_text(text, markup_fmt::Language::Html, &options, |code, _| {
     Ok::<_, std::convert::Infallible>(code.into())
-  }) else {
-    return None;
-  };
-  Some(text.to_string())
+  })?;
+  Ok(Some(text.to_string()))
 }
 
-fn format_sql(text: &str, config: &Configuration) -> Option<String> {
+fn format_sql(text: &str, config: &Configuration) -> Result<Option<String>> {
   let options = dprint_plugin_sql::configuration::ConfigurationBuilder::new()
     .indent_width(config.indent_width)
     .build();
-  dprint_plugin_sql::format_text(Path::new("_path.sql"), text, &options).ok().flatten()
+  dprint_plugin_sql::format_text(Path::new("_path.sql"), text, &options)
 }
 
 fn main() {