refactor(cli/fmt_errors): Improve source line formatting (#4832)

1 files changed, 65 insertions, 136 deletions

diff --git a/cli/fmt_errors.rs b/cli/fmt_errors.rs
index 19e6616ca..d2c69819f 100644
--- a/cli/fmt_errors.rs
+++ b/cli/fmt_errors.rs
@@ -10,63 +10,49 @@ use std::ops::Deref;
 
 const SOURCE_ABBREV_THRESHOLD: usize = 150;
 
-/// A trait which specifies parts of a diagnostic like item needs to be able to
-/// generate to conform its display to other diagnostic like items
-pub trait DisplayFormatter {
-  fn format_category_and_code(&self) -> String;
-  fn format_message(&self, level: usize) -> String;
-  fn format_related_info(&self) -> String;
-  fn format_source_line(&self, level: usize) -> String;
-  fn format_source_name(&self) -> String;
-}
-
-fn format_source_name(
-  file_name: String,
-  line_number: i64,
-  column_number: i64,
-) -> String {
-  let line_number = line_number;
-  let column_number = column_number;
-  let file_name_c = colors::cyan(file_name);
-  let line_c = colors::yellow(line_number.to_string());
-  let column_c = colors::yellow(column_number.to_string());
-  format!("{}:{}:{}", file_name_c, line_c, column_c)
-}
-
-/// Formats optional source, line and column numbers into a single string.
-pub fn format_maybe_source_name(
-  file_name: Option<String>,
-  line_number: Option<i64>,
-  column_number: Option<i64>,
+pub fn format_stack(
+  is_error: bool,
+  message_line: String,
+  source_line: Option<String>,
+  start_column: Option<i64>,
+  end_column: Option<i64>,
+  formatted_frames: &[String],
+  level: usize,
 ) -> String {
-  if file_name.is_none() {
-    return "".to_string();
-  }
-
-  assert!(line_number.is_some());
-  assert!(column_number.is_some());
-  format_source_name(
-    file_name.unwrap(),
-    line_number.unwrap(),
-    column_number.unwrap(),
-  )
+  let mut s = String::new();
+  s.push_str(&format!("{:indent$}{}", "", message_line, indent = level));
+  s.push_str(&format_maybe_source_line(
+    source_line,
+    start_column,
+    end_column,
+    is_error,
+    level,
+  ));
+  for formatted_frame in formatted_frames {
+    s.push_str(&format!(
+      "\n{:indent$}    at {}",
+      "",
+      formatted_frame,
+      indent = level
+    ));
+  }
+  s
 }
 
 /// Take an optional source line and associated information to format it into
 /// a pretty printed version of that line.
-pub fn format_maybe_source_line(
+fn format_maybe_source_line(
   source_line: Option<String>,
-  line_number: Option<i64>,
   start_column: Option<i64>,
   end_column: Option<i64>,
   is_error: bool,
   level: usize,
 ) -> String {
-  if source_line.is_none() || line_number.is_none() {
+  if source_line.is_none() || start_column.is_none() || end_column.is_none() {
     return "".to_string();
   }
 
-  let source_line = source_line.as_ref().unwrap();
+  let source_line = source_line.unwrap();
   // sometimes source_line gets set with an empty string, which then outputs
   // an empty source line when displayed, so need just short circuit here.
   // Also short-circuit on error line too long.
@@ -76,12 +62,6 @@ pub fn format_maybe_source_line(
 
   assert!(start_column.is_some());
   assert!(end_column.is_some());
-  let line_number = line_number.unwrap().to_string();
-  let line_color = colors::black_on_white(line_number.to_string());
-  let line_number_len = line_number.len();
-  let line_padding =
-    colors::black_on_white(format!("{:indent$}", "", indent = line_number_len))
-      .to_string();
   let mut s = String::new();
   let start_column = start_column.unwrap();
   let end_column = end_column.unwrap();
@@ -107,16 +87,7 @@ pub fn format_maybe_source_line(
 
   let indent = format!("{:indent$}", "", indent = level);
 
-  format!(
-    "\n\n{}{} {}\n{}{} {}\n",
-    indent, line_color, source_line, indent, line_padding, color_underline
-  )
-}
-
-/// Format a message to preface with `error: ` with ansi codes for red.
-pub fn format_error_message(msg: String) -> String {
-  let preamble = colors::red("error:".to_string());
-  format!("{} {}", preamble, msg)
+  format!("\n{}{}\n{}{}", indent, source_line, indent, color_underline)
 }
 
 /// Wrapper around deno_core::JSError which provides color to_string.
@@ -141,63 +112,44 @@ impl Deref for JSError {
   }
 }
 
-impl DisplayFormatter for JSError {
-  fn format_category_and_code(&self) -> String {
-    "".to_string()
-  }
-
-  fn format_message(&self, _level: usize) -> String {
-    format!(
-      "{}{}",
-      colors::red_bold("error: ".to_string()),
-      self.0.message.clone()
-    )
-  }
-
-  fn format_related_info(&self) -> String {
-    "".to_string()
-  }
-
-  fn format_source_line(&self, level: usize) -> String {
-    format_maybe_source_line(
-      self.0.source_line.clone(),
-      self.0.line_number,
-      self.0.start_column,
-      self.0.end_column,
-      true,
-      level,
-    )
-  }
-
-  fn format_source_name(&self) -> String {
-    let e = &self.0;
-    if e.script_resource_name.is_none() {
-      return "".to_string();
-    }
-
-    format!(
-      "\n► {}",
-      format_maybe_source_name(
-        e.script_resource_name.clone(),
-        e.line_number,
-        e.start_column.map(|n| n + 1),
-      )
-    )
-  }
-}
-
 impl fmt::Display for JSError {
   fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+    // When the stack frame array is empty, but the source location given by
+    // (script_resource_name, line_number, start_column + 1) exists, this is
+    // likely a syntax error. For the sake of formatting we treat it like it was
+    // given as a single stack frame.
+    let formatted_frames = if self.0.formatted_frames.is_empty()
+      && self.0.script_resource_name.is_some()
+      && self.0.line_number.is_some()
+      && self.0.start_column.is_some()
+    {
+      vec![format!(
+        "{}:{}:{}",
+        colors::cyan(self.0.script_resource_name.clone().unwrap()),
+        colors::yellow(self.0.line_number.unwrap().to_string()),
+        colors::yellow((self.0.start_column.unwrap() + 1).to_string())
+      )]
+    } else {
+      self.0.formatted_frames.clone()
+    };
+
     write!(
       f,
-      "{}{}{}",
-      self.format_message(0),
-      self.format_source_name(),
-      self.format_source_line(0),
+      "{}",
+      &format_stack(
+        true,
+        format!(
+          "{}: {}",
+          colors::red_bold("error".to_string()),
+          self.0.message.clone()
+        ),
+        self.0.source_line.clone(),
+        self.0.start_column,
+        self.0.end_column,
+        &formatted_frames,
+        0
+      )
     )?;
-    for formatted_frame in &self.0.formatted_frames {
-      write!(f, "\n    at {}", formatted_frame)?;
-    }
     Ok(())
   }
 }
@@ -210,24 +162,8 @@ mod tests {
   use crate::colors::strip_ansi_codes;
 
   #[test]
-  fn test_format_none_source_name() {
-    let actual = format_maybe_source_name(None, None, None);
-    assert_eq!(actual, "");
-  }
-
-  #[test]
-  fn test_format_some_source_name() {
-    let actual = format_maybe_source_name(
-      Some("file://foo/bar.ts".to_string()),
-      Some(1),
-      Some(2),
-    );
-    assert_eq!(strip_ansi_codes(&actual), "file://foo/bar.ts:1:2");
-  }
-
-  #[test]
   fn test_format_none_source_line() {
-    let actual = format_maybe_source_line(None, None, None, None, false, 0);
+    let actual = format_maybe_source_line(None, None, None, false, 0);
     assert_eq!(actual, "");
   }
 
@@ -236,20 +172,13 @@ mod tests {
     let actual = format_maybe_source_line(
       Some("console.log('foo');".to_string()),
       Some(8),
-      Some(8),
       Some(11),
       true,
       0,
     );
     assert_eq!(
       strip_ansi_codes(&actual),
-      "\n\n8 console.log(\'foo\');\n          ~~~\n"
+      "\nconsole.log(\'foo\');\n        ~~~"
     );
   }
-
-  #[test]
-  fn test_format_error_message() {
-    let actual = format_error_message("foo".to_string());
-    assert_eq!(strip_ansi_codes(&actual), "error: foo");
-  }
 }