feat: get clang-tidy fixes regardless of tidy-review value

cpp-linter/src/clang_tools/clang_tidy.rs

@@ -16,7 +16,9 @@ use serde::Deserialize;
 
 // project-specific modules/crates
 use super::MakeSuggestions;
-use crate::{cli::ClangParams, common_fs::FileObj, error::ClangCaptureError};
+use crate::{
+    clang_tools::CACHE_DIR, cli::ClangParams, common_fs::FileObj, error::ClangCaptureError,
+};
 
 /// Used to deserialize a json compilation database's translation unit.
 ///
@@ -104,7 +106,7 @@ pub struct TidyAdvice {
     pub notes: Vec<TidyNotification>,
 
     /// A buffer to hold the contents of the file after applying clang-tidy fixes.
-    pub patched: Option<Vec<u8>>,
+    pub patched: PathBuf,
 }
 
 impl MakeSuggestions for TidyAdvice {
@@ -148,7 +150,7 @@ fn parse_tidy_output(
     tidy_stdout: &[u8],
     database_json: &Option<Vec<CompilationUnit>>,
     repo_root: &Path,
-) -> Result<TidyAdvice, ClangCaptureError> {
+) -> Result<Vec<TidyNotification>, ClangCaptureError> {
     let note_header = Regex::new(NOTE_HEADER)?;
     let fixed_note = Regex::new(r"^.+:(\d+):\d+:\snote: FIX-IT applied suggested code changes$")?;
     let mut found_fix = false;
@@ -243,10 +245,7 @@ fn parse_tidy_output(
     if let Some(note) = notification {
         result.push(note);
     }
-    Ok(TidyAdvice {
-        notes: result,
-        patched: None,
-    })
+    Ok(result)
 }
 
 /// Get a total count of clang-tidy advice from the given list of [FileObj]s.
@@ -266,6 +265,26 @@ pub fn tally_tidy_advice(files: &[Arc<Mutex<FileObj>>]) -> Result<u64, String> {
     Ok(total)
 }
 
+/// RAII guard that restores a file's original content on drop.
+///
+/// This is a best-effort when an error gets propagated before
+/// we can explicitly restore a file's contents.
+struct RestoreOnDrop<'a> {
+    path: &'a Path,
+    content: String,
+    // lock:
+    armed: bool,
+}
+
+impl Drop for RestoreOnDrop<'_> {
+    fn drop(&mut self) {
+        if self.armed {
+            // We have to ignore any error here and hope for the best.
+            let _ = fs::write(self.path, &self.content);
+        }
+    }
+}
+
 /// Run clang-tidy, then parse and return it's output.
 pub fn run_clang_tidy(
     file: &mut MutexGuard<FileObj>,
@@ -301,17 +320,7 @@ pub fn run_clang_tidy(
         cmd.args(["--line-filter", filter.as_str()]);
     }
     let repo_file_path = clang_params.repo_root.join(&file.name);
-    let original_content = if !clang_params.tidy_review {
-        None
-    } else {
-        cmd.arg("--fix-errors");
-        Some(fs::read_to_string(&repo_file_path).map_err(|e| {
-            ClangCaptureError::ReadFileFailed {
-                file_name: file_name.clone(),
-                source: e,
-            }
-        })?)
-    };
+    cmd.arg("--fix-errors");
     if !clang_params.style.is_empty() {
         cmd.args(["--format-style", clang_params.style.as_str()]);
     }
@@ -327,12 +336,50 @@ pub fn run_clang_tidy(
                 .join(" ")
         ),
     ));
+    let cache_patch_path = clang_params
+        .repo_root
+        .join(CACHE_DIR)
+        .join(file.name.with_added_extension("tidy"));
+    fs::create_dir_all(
+        cache_patch_path
+            .parent()
+            .ok_or(ClangCaptureError::UnknownCacheParentPath)?,
+    )
+    .map_err(ClangCaptureError::MkDirFailed)?;
+    let mut drop_guard = RestoreOnDrop {
+        content: fs::read_to_string(&repo_file_path).map_err(|e| {
+            ClangCaptureError::ReadFileFailed {
+                file_name: file_name.clone(),
+                source: e,
+            }
+        })?,
+        armed: true,
+        path: repo_file_path.as_path(),
+    };
+    // run clang-tidy to patch the file in-place.
     let output = cmd
         .output()
         .map_err(|e| ClangCaptureError::FailedToRunCommand {
             task: format!("execute clang-tidy on file {file_name}"),
             source: e,
         })?;
+    // move the patched file content into cache
+    fs::copy(&repo_file_path, &cache_patch_path).map_err(|e| {
+        ClangCaptureError::WriteFileFailed {
+            file_name: cache_patch_path.to_string_lossy().to_string(),
+            source: e,
+        }
+    })?;
+    // restore the original file content to avoid unexpected behavior in later steps of dev workflow
+    fs::write(&repo_file_path, &drop_guard.content).map_err(|e| {
+        ClangCaptureError::WriteFileFailed {
+            file_name: file_name.clone(),
+            source: e,
+        }
+    })?;
+    // disarm the drop guard since we've already restored the original content
+    drop_guard.armed = false;
+
     logs.push((
         log::Level::Debug,
         format!(
@@ -349,32 +396,18 @@ pub fn run_clang_tidy(
             ),
         ));
     }
-    file.tidy_advice = Some(parse_tidy_output(
+
+    let notes = parse_tidy_output(
         &output.stdout,
         &clang_params.database_json,
         &clang_params.repo_root,
-    )?);
-    if clang_params.tidy_review
-        && let Some(original_content) = &original_content
-    {
-        if let Some(tidy_advice) = &mut file.tidy_advice {
-            // cache file changes in a buffer and restore the original contents for further analysis
-            tidy_advice.patched =
-                Some(
-                    fs::read(&repo_file_path).map_err(|e| ClangCaptureError::ReadFileFailed {
-                        file_name: file_name.clone(),
-                        source: e,
-                    })?,
-                );
-        }
-        // original_content is guaranteed to be Some() value at this point
-        fs::write(&repo_file_path, original_content).map_err(|e| {
-            ClangCaptureError::WriteFileFailed {
-                file_name,
-                source: e,
-            }
-        })?;
-    }
+    )?;
+
+    let tidy_advice = TidyAdvice {
+        notes,
+        patched: cache_patch_path.to_path_buf(),
+    };
+    file.tidy_advice = Some(tidy_advice);
     Ok(logs)
 }
 
@@ -383,7 +416,7 @@ mod test {
     #![allow(clippy::unwrap_used, clippy::expect_used)]
 
     use std::{
-        env,
+        env, fs,
         path::PathBuf,
         str::FromStr,
         sync::{Arc, Mutex},
@@ -398,7 +431,7 @@ mod test {
         common_fs::FileObj,
     };
 
-    use super::{NOTE_HEADER, TidyNotification, run_clang_tidy};
+    use super::{NOTE_HEADER, RestoreOnDrop, TidyNotification, run_clang_tidy};
 
     #[test]
     fn clang_diagnostic_link() {
@@ -486,7 +519,8 @@ mod test {
                 .unwrap(),
             )
             .unwrap();
-        let file = FileObj::new(PathBuf::from("tests/demo/demo.cpp"));
+        let tmp_workspace = crate::run::test::setup_tmp_workspace();
+        let file = FileObj::new(PathBuf::from("demo/demo.cpp"));
         let arc_file = Arc::new(Mutex::new(file));
         let extra_args = vec!["-std=c++17".to_string(), "-Wall".to_string()];
         let clang_params = ClangParams {
@@ -502,7 +536,7 @@ mod test {
             format_review: false,
             clang_tidy_command: Some(exe_path),
             clang_format_command: None,
-            repo_root: PathBuf::from("."),
+            repo_root: tmp_workspace.path().to_path_buf(),
         };
         let mut file_lock = arc_file.lock().unwrap();
         let logs = run_clang_tidy(&mut file_lock, &clang_params)
@@ -571,10 +605,31 @@ TrenchBroom/TrenchBroom/common/test/src/mdl/tst_ReadFreeImageTexture.cpp:44:48:
       |                                                ^
 "#;
         let advice = parse_tidy_output(tidy_out.as_bytes(), &None, &PathBuf::from(".")).unwrap();
-        assert_eq!(advice.notes.len(), 4);
-        for note in advice.notes {
+        assert_eq!(advice.len(), 4);
+        for note in advice {
             assert!(note.diagnostic.contains('-'));
             assert!(!note.diagnostic.contains(' '));
         }
     }
+
+    #[test]
+    fn restore_on_drop_fires() {
+        let tmp = tempfile::NamedTempFile::new().unwrap();
+        let original = "original content";
+        fs::write(tmp.path(), original).unwrap();
+
+        {
+            let guard = RestoreOnDrop {
+                path: tmp.path(),
+                content: original.to_string(),
+                armed: true,
+            };
+            // Simulate clang-tidy mutating the file
+            fs::write(tmp.path(), "patched content").unwrap();
+            // Explicit drop triggers restoration
+            drop(guard);
+        }
+
+        assert_eq!(fs::read_to_string(tmp.path()).unwrap(), original);
+    }
 }

cpp-linter/src/common_fs.rs

@@ -178,12 +178,9 @@ impl FileObj {
         }
 
         if let Some(advice) = &self.tidy_advice {
-            if let Some(patched) = &advice.patched {
-                let patched = String::from_utf8(patched.to_vec())
-                    .map_err(|e| FileObjError::FromUtf8Error(file_name.clone(), e))?;
-                let (diff, input) = make_patch(patched.as_str(), &original_content);
-                advice.get_suggestions(review_comments, self, &diff, &input, summary_only);
-            }
+            let patched = fs::read_to_string(&advice.patched).map_err(FileObjError::ReadFile)?;
+            let (diff, input) = make_patch(patched.as_str(), &original_content);
+            advice.get_suggestions(review_comments, self, &diff, &input, summary_only);
 
             if summary_only {
                 return Ok(());

cpp-linter/src/rest_client.rs

@@ -436,12 +436,17 @@ fn make_tidy_comment(
                 if file_path == file.name {
                     let mut tmp_note = format!("- {}\n\n", tidy_note.filename);
                     tmp_note.push_str(&format!(
-                        "   <strong>{filename}:{line}:{cols}:</strong> {severity}: [{diagnostic}]\n   > {rationale}\n{concerned_code}",
+                        "   <strong>{filename}:{line}:{cols}:</strong> {severity}: [{diagnostic}]{auto_fixable}\n   > {rationale}\n{concerned_code}",
                         filename = tidy_note.filename,
                         line = tidy_note.line,
                         cols = tidy_note.cols,
                         severity = tidy_note.severity,
                         diagnostic = tidy_note.diagnostic_link(),
+                        auto_fixable = if tidy_note.fixed_lines.contains(&tidy_note.line) {
+                            "\n   :zap: auto-fix available"
+                        } else {
+                            ""
+                        },
                         rationale = tidy_note.rationale,
                         concerned_code = if tidy_note.suggestion.is_empty() {String::from("")} else {
                             format!("\n   ```{ext}\n   {suggestion}\n   ```\n",
@@ -526,7 +531,7 @@ mod test {
                 }];
                 file.tidy_advice = Some(TidyAdvice {
                     notes,
-                    patched: None,
+                    patched: PathBuf::new(),
                 });
                 file.format_advice = Some(FormatAdvice {
                     #[allow(clippy::single_range_in_vec_init)]

cpp-linter/src/run.rs

@@ -193,11 +193,11 @@ pub async fn run_main(args: Vec<String>) -> Result<()> {
 }
 
 #[cfg(test)]
-mod test {
+pub(crate) mod test {
     #![allow(clippy::unwrap_used)]
 
     use super::run_main;
-    use std::env;
+    use std::{env, fs, path::PathBuf};
 
     /// helper to avoid writing to the same GITHUB_OUTPUT file in parallel-running tests.
     fn setup_tmp_gh_out_path() -> tempfile::NamedTempFile {
@@ -211,15 +211,32 @@ mod test {
         gh_out_path
     }
 
+    /// helper to avoid concurrent writes by executing (& processing the output of)
+    /// clang tools on the same test assets.
+    pub fn setup_tmp_workspace() -> tempfile::TempDir {
+        let tmp_workspace = tempfile::TempDir::with_prefix("cpp-linter-unit-tests_").unwrap();
+        let demo_path = tmp_workspace.path().join("demo");
+        fs::create_dir(&demo_path).unwrap();
+        for asset in ["demo.cpp", "demo.hpp"] {
+            fs::copy(
+                PathBuf::from("tests/demo").join(asset),
+                demo_path.join(asset),
+            )
+            .unwrap();
+        }
+        tmp_workspace
+    }
+
     #[tokio::test]
     async fn normal() {
         let tmp_gh_out = setup_tmp_gh_out_path();
+        let tmp_workspace = setup_tmp_workspace();
         run_main(vec![
             "cpp-linter".to_string(),
             "-l".to_string(),
             "false".to_string(),
             "--repo-root".to_string(),
-            "tests".to_string(),
+            tmp_workspace.path().to_str().unwrap().to_string(),
             "demo/demo.cpp".to_string(),
         ])
         .await
@@ -239,12 +256,15 @@ mod test {
     #[tokio::test]
     async fn force_debug_output() {
         let tmp_gh_out = setup_tmp_gh_out_path();
+        let tmp_workspace = setup_tmp_workspace();
         run_main(vec![
             "cpp-linter".to_string(),
             "-l".to_string(),
             "false".to_string(),
             "-v".to_string(),
             "-i=target|benches/libgit2".to_string(),
+            "--repo-root".to_string(),
+            tmp_workspace.path().to_str().unwrap().to_string(),
         ])
         .await
         .unwrap();
@@ -254,11 +274,15 @@ mod test {
     #[tokio::test]
     async fn no_version_input() {
         let tmp_gh_out = setup_tmp_gh_out_path();
+        let tmp_workspace = setup_tmp_workspace();
         run_main(vec![
             "cpp-linter".to_string(),
             "-l".to_string(),
             "false".to_string(),
             "-V".to_string(),
+            "-i=target|benches/libgit2".to_string(),
+            "--repo-root".to_string(),
+            tmp_workspace.path().to_str().unwrap().to_string(),
         ])
         .await
         .unwrap();
@@ -271,11 +295,15 @@ mod test {
         unsafe {
             env::set_var("PRE_COMMIT", "1");
         }
+        let tmp_workspace = setup_tmp_workspace();
         run_main(vec![
             "cpp-linter".to_string(),
             "--lines-changed-only".to_string(),
             "false".to_string(),
+            "-v".to_string(),
             "--ignore=target|benches/libgit2".to_string(),
+            "--repo-root".to_string(),
+            tmp_workspace.path().to_str().unwrap().to_string(),
         ])
         .await
         .unwrap_err();

cspell.config.yml

@@ -95,6 +95,7 @@ words:
   - pypi
   - pyproject
   - pyyaml
+  - RAII
   - ratelimit
   - reqwest
   - revparse