perf(fmt/lint): incremental formatting and linting (#14314)

cli/tools/fmt.rs

@@ -11,6 +11,7 @@ use crate::colors;
 use crate::config_file::FmtConfig;
 use crate::config_file::FmtOptionsConfig;
 use crate::config_file::ProseWrap;
+use crate::deno_dir::DenoDir;
 use crate::diff::diff;
 use crate::file_watcher;
 use crate::file_watcher::ResolutionResult;
@@ -40,11 +41,14 @@ use std::sync::atomic::AtomicUsize;
 use std::sync::atomic::Ordering;
 use std::sync::Arc;
 
+use super::incremental_cache::IncrementalCache;
+
 /// Format JavaScript/TypeScript files.
 pub async fn format(
   flags: &Flags,
   fmt_flags: FmtFlags,
   maybe_fmt_config: Option<FmtConfig>,
+  deno_dir: &DenoDir,
 ) -> Result<(), AnyError> {
   let FmtFlags {
     files,
@@ -132,11 +136,18 @@ pub async fn format(
     }
   };
   let operation = |(paths, fmt_options): (Vec<PathBuf>, FmtOptionsConfig)| async move {
+    let incremental_cache = Arc::new(IncrementalCache::new(
+      &deno_dir.fmt_incremental_cache_db_file_path(),
+      &fmt_options,
+      &paths,
+    ));
     if check {
-      check_source_files(paths, fmt_options).await?;
+      check_source_files(paths, fmt_options, incremental_cache.clone()).await?;
     } else {
-      format_source_files(paths, fmt_options).await?;
+      format_source_files(paths, fmt_options, incremental_cache.clone())
+        .await?;
     }
+    incremental_cache.wait_completion().await;
     Ok(())
   };
 
@@ -234,18 +245,18 @@ pub fn format_json(
 pub fn format_file(
   file_path: &Path,
   file_text: &str,
-  fmt_options: FmtOptionsConfig,
+  fmt_options: &FmtOptionsConfig,
 ) -> Result<Option<String>, AnyError> {
   let ext = get_extension(file_path).unwrap_or_default();
   if matches!(
     ext.as_str(),
     "md" | "mkd" | "mkdn" | "mdwn" | "mdown" | "markdown"
   ) {
-    format_markdown(file_text, &fmt_options)
+    format_markdown(file_text, fmt_options)
   } else if matches!(ext.as_str(), "json" | "jsonc") {
-    format_json(file_text, &fmt_options)
+    format_json(file_text, fmt_options)
   } else {
-    let config = get_resolved_typescript_config(&fmt_options);
+    let config = get_resolved_typescript_config(fmt_options);
     dprint_plugin_typescript::format_text(file_path, file_text, &config)
   }
 }
@@ -263,6 +274,7 @@ pub fn format_parsed_source(
 async fn check_source_files(
   paths: Vec<PathBuf>,
   fmt_options: FmtOptionsConfig,
+  incremental_cache: Arc<IncrementalCache>,
 ) -> Result<(), AnyError> {
   let not_formatted_files_count = Arc::new(AtomicUsize::new(0));
   let checked_files_count = Arc::new(AtomicUsize::new(0));
@@ -277,7 +289,12 @@ async fn check_source_files(
       checked_files_count.fetch_add(1, Ordering::Relaxed);
       let file_text = read_file_contents(&file_path)?.text;
 
-      match format_file(&file_path, &file_text, fmt_options.clone()) {
+      // skip checking the file if we know it's formatted
+      if incremental_cache.is_file_same(&file_path, &file_text) {
+        return Ok(());
+      }
+
+      match format_file(&file_path, &file_text, &fmt_options) {
         Ok(Some(formatted_text)) => {
           not_formatted_files_count.fetch_add(1, Ordering::Relaxed);
           let _g = output_lock.lock();
@@ -286,7 +303,14 @@ async fn check_source_files(
           info!("{} {}:", colors::bold("from"), file_path.display());
           info!("{}", diff);
         }
-        Ok(None) => {}
+        Ok(None) => {
+          // When checking formatting, only update the incremental cache when
+          // the file is the same since we don't bother checking for stable
+          // formatting here. Additionally, ensure this is done during check
+          // so that CIs that cache the DENO_DIR will get the benefit of
+          // incremental formatting
+          incremental_cache.update_file(&file_path, &file_text);
+        }
         Err(e) => {
           let _g = output_lock.lock();
           eprintln!("Error checking: {}", file_path.to_string_lossy());
@@ -318,6 +342,7 @@ async fn check_source_files(
 async fn format_source_files(
   paths: Vec<PathBuf>,
   fmt_options: FmtOptionsConfig,
+  incremental_cache: Arc<IncrementalCache>,
 ) -> Result<(), AnyError> {
   let formatted_files_count = Arc::new(AtomicUsize::new(0));
   let checked_files_count = Arc::new(AtomicUsize::new(0));
@@ -330,8 +355,19 @@ async fn format_source_files(
       checked_files_count.fetch_add(1, Ordering::Relaxed);
       let file_contents = read_file_contents(&file_path)?;
 
-      match format_file(&file_path, &file_contents.text, fmt_options.clone()) {
+      // skip formatting the file if we know it's formatted
+      if incremental_cache.is_file_same(&file_path, &file_contents.text) {
+        return Ok(());
+      }
+
+      match format_ensure_stable(
+        &file_path,
+        &file_contents.text,
+        &fmt_options,
+        format_file,
+      ) {
         Ok(Some(formatted_text)) => {
+          incremental_cache.update_file(&file_path, &formatted_text);
           write_file_contents(
             &file_path,
             FileContents {
@@ -343,7 +379,9 @@ async fn format_source_files(
           let _g = output_lock.lock();
           info!("{}", file_path.to_string_lossy());
         }
-        Ok(None) => {}
+        Ok(None) => {
+          incremental_cache.update_file(&file_path, &file_contents.text);
+        }
         Err(e) => {
           let _g = output_lock.lock();
           eprintln!("Error formatting: {}", file_path.to_string_lossy());
@@ -372,6 +410,66 @@ async fn format_source_files(
   Ok(())
 }
 
+/// When storing any formatted text in the incremental cache, we want
+/// to ensure that anything stored when formatted will have itself as
+/// the output as well. This is to prevent "double format" issues where
+/// a user formats their code locally and it fails on the CI afterwards.
+fn format_ensure_stable(
+  file_path: &Path,
+  file_text: &str,
+  fmt_options: &FmtOptionsConfig,
+  fmt_func: impl Fn(
+    &Path,
+    &str,
+    &FmtOptionsConfig,
+  ) -> Result<Option<String>, AnyError>,
+) -> Result<Option<String>, AnyError> {
+  let formatted_text = fmt_func(file_path, file_text, fmt_options)?;
+
+  match formatted_text {
+    Some(mut current_text) => {
+      let mut count = 0;
+      loop {
+        match fmt_func(file_path, &current_text, fmt_options) {
+          Ok(Some(next_pass_text)) => {
+            // just in case
+            if next_pass_text == current_text {
+              return Ok(Some(next_pass_text));
+            }
+            current_text = next_pass_text;
+          }
+          Ok(None) => {
+            return Ok(Some(current_text));
+          }
+          Err(err) => {
+            panic!(
+              concat!(
+                "Formatting succeeded initially, but failed when ensuring a ",
+                "stable format. This indicates a bug in the formatter where ",
+                "the text it produces is not syntatically correct. As a temporary ",
+                "workfaround you can ignore this file.\n\n{:#}"
+              ),
+              err,
+            )
+          }
+        }
+        count += 1;
+        if count == 5 {
+          panic!(
+            concat!(
+              "Formatting not stable. Bailed after {} tries. This indicates a bug ",
+              "in the formatter where it formats the file differently each time. As a ",
+              "temporary workaround you can ignore this file."
+            ),
+            count
+          )
+        }
+      }
+    }
+    None => Ok(None),
+  }
+}
+
 /// Format stdin and write result to stdout.
 /// Treats input as TypeScript or as set by `--ext` flag.
 /// Compatible with `--check` flag.
@@ -386,7 +484,7 @@ pub fn format_stdin(
   let file_path = PathBuf::from(format!("_stdin.{}", fmt_flags.ext));
   let fmt_options = resolve_fmt_options(&fmt_flags, fmt_options);
 
-  let formatted_text = format_file(&file_path, &source, fmt_options)?;
+  let formatted_text = format_file(&file_path, &source, &fmt_options)?;
   if fmt_flags.check {
     if formatted_text.is_some() {
       println!("Not formatted stdin");
@@ -628,37 +726,106 @@ fn is_contain_git(path: &Path) -> bool {
   path.components().any(|c| c.as_os_str() == ".git")
 }
 
-#[test]
-fn test_is_supported_ext_fmt() {
-  assert!(!is_supported_ext_fmt(Path::new("tests/subdir/redirects")));
-  assert!(is_supported_ext_fmt(Path::new("README.md")));
-  assert!(is_supported_ext_fmt(Path::new("readme.MD")));
-  assert!(is_supported_ext_fmt(Path::new("readme.mkd")));
-  assert!(is_supported_ext_fmt(Path::new("readme.mkdn")));
-  assert!(is_supported_ext_fmt(Path::new("readme.mdwn")));
-  assert!(is_supported_ext_fmt(Path::new("readme.mdown")));
-  assert!(is_supported_ext_fmt(Path::new("readme.markdown")));
-  assert!(is_supported_ext_fmt(Path::new("lib/typescript.d.ts")));
-  assert!(is_supported_ext_fmt(Path::new("testdata/001_hello.js")));
-  assert!(is_supported_ext_fmt(Path::new("testdata/002_hello.ts")));
-  assert!(is_supported_ext_fmt(Path::new("foo.jsx")));
-  assert!(is_supported_ext_fmt(Path::new("foo.tsx")));
-  assert!(is_supported_ext_fmt(Path::new("foo.TS")));
-  assert!(is_supported_ext_fmt(Path::new("foo.TSX")));
-  assert!(is_supported_ext_fmt(Path::new("foo.JS")));
-  assert!(is_supported_ext_fmt(Path::new("foo.JSX")));
-  assert!(is_supported_ext_fmt(Path::new("foo.mjs")));
-  assert!(!is_supported_ext_fmt(Path::new("foo.mjsx")));
-  assert!(is_supported_ext_fmt(Path::new("foo.jsonc")));
-  assert!(is_supported_ext_fmt(Path::new("foo.JSONC")));
-  assert!(is_supported_ext_fmt(Path::new("foo.json")));
-  assert!(is_supported_ext_fmt(Path::new("foo.JsON")));
-}
+#[cfg(test)]
+mod test {
+  use super::*;
 
-#[test]
-fn test_is_located_in_git() {
-  assert!(is_contain_git(Path::new("test/.git")));
-  assert!(is_contain_git(Path::new(".git/bad.json")));
-  assert!(is_contain_git(Path::new("test/.git/bad.json")));
-  assert!(!is_contain_git(Path::new("test/bad.git/bad.json")));
+  #[test]
+  fn test_is_supported_ext_fmt() {
+    assert!(!is_supported_ext_fmt(Path::new("tests/subdir/redirects")));
+    assert!(is_supported_ext_fmt(Path::new("README.md")));
+    assert!(is_supported_ext_fmt(Path::new("readme.MD")));
+    assert!(is_supported_ext_fmt(Path::new("readme.mkd")));
+    assert!(is_supported_ext_fmt(Path::new("readme.mkdn")));
+    assert!(is_supported_ext_fmt(Path::new("readme.mdwn")));
+    assert!(is_supported_ext_fmt(Path::new("readme.mdown")));
+    assert!(is_supported_ext_fmt(Path::new("readme.markdown")));
+    assert!(is_supported_ext_fmt(Path::new("lib/typescript.d.ts")));
+    assert!(is_supported_ext_fmt(Path::new("testdata/001_hello.js")));
+    assert!(is_supported_ext_fmt(Path::new("testdata/002_hello.ts")));
+    assert!(is_supported_ext_fmt(Path::new("foo.jsx")));
+    assert!(is_supported_ext_fmt(Path::new("foo.tsx")));
+    assert!(is_supported_ext_fmt(Path::new("foo.TS")));
+    assert!(is_supported_ext_fmt(Path::new("foo.TSX")));
+    assert!(is_supported_ext_fmt(Path::new("foo.JS")));
+    assert!(is_supported_ext_fmt(Path::new("foo.JSX")));
+    assert!(is_supported_ext_fmt(Path::new("foo.mjs")));
+    assert!(!is_supported_ext_fmt(Path::new("foo.mjsx")));
+    assert!(is_supported_ext_fmt(Path::new("foo.jsonc")));
+    assert!(is_supported_ext_fmt(Path::new("foo.JSONC")));
+    assert!(is_supported_ext_fmt(Path::new("foo.json")));
+    assert!(is_supported_ext_fmt(Path::new("foo.JsON")));
+  }
+
+  #[test]
+  fn test_is_located_in_git() {
+    assert!(is_contain_git(Path::new("test/.git")));
+    assert!(is_contain_git(Path::new(".git/bad.json")));
+    assert!(is_contain_git(Path::new("test/.git/bad.json")));
+    assert!(!is_contain_git(Path::new("test/bad.git/bad.json")));
+  }
+
+  #[test]
+  #[should_panic(expected = "Formatting not stable. Bailed after 5 tries.")]
+  fn test_format_ensure_stable_unstable_format() {
+    format_ensure_stable(
+      &PathBuf::from("mod.ts"),
+      "1",
+      &Default::default(),
+      |_, file_text, _| Ok(Some(format!("1{}", file_text))),
+    )
+    .unwrap();
+  }
+
+  #[test]
+  fn test_format_ensure_stable_error_first() {
+    let err = format_ensure_stable(
+      &PathBuf::from("mod.ts"),
+      "1",
+      &Default::default(),
+      |_, _, _| bail!("Error formatting."),
+    )
+    .unwrap_err();
+
+    assert_eq!(err.to_string(), "Error formatting.");
+  }
+
+  #[test]
+  #[should_panic(expected = "Formatting succeeded initially, but failed when")]
+  fn test_format_ensure_stable_error_second() {
+    format_ensure_stable(
+      &PathBuf::from("mod.ts"),
+      "1",
+      &Default::default(),
+      |_, file_text, _| {
+        if file_text == "1" {
+          Ok(Some("11".to_string()))
+        } else {
+          bail!("Error formatting.")
+        }
+      },
+    )
+    .unwrap();
+  }
+
+  #[test]
+  fn test_format_stable_after_two() {
+    let result = format_ensure_stable(
+      &PathBuf::from("mod.ts"),
+      "1",
+      &Default::default(),
+      |_, file_text, _| {
+        if file_text == "1" {
+          Ok(Some("11".to_string()))
+        } else if file_text == "11" {
+          Ok(None)
+        } else {
+          unreachable!();
+        }
+      },
+    )
+    .unwrap();
+
+    assert_eq!(result, Some("11".to_string()));
+  }
 }