diff --git a/cpp-linter/src/clang_tools/clang_tidy.rs b/cpp-linter/src/clang_tools/clang_tidy.rs index 9132090..6d87adc 100644 --- a/cpp-linter/src/clang_tools/clang_tidy.rs +++ b/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. /// @@ -103,8 +105,8 @@ pub struct TidyAdvice { /// A list of notifications parsed from clang-tidy stdout. pub notes: Vec, - /// A buffer to hold the contents of the file after applying clang-tidy fixes. - pub patched: Option>, + /// A path to the cached contents of the file after applying clang-tidy fixes. + pub patched: PathBuf, } impl MakeSuggestions for TidyAdvice { @@ -148,7 +150,7 @@ fn parse_tidy_output( tidy_stdout: &[u8], database_json: &Option>, repo_root: &Path, -) -> Result { +) -> Result, 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>]) -> Result { 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, @@ -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::test_common::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,32 @@ 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::tempdir().unwrap(); + let file_path = tmp.path().join("test_file.txt"); + let original = "original content"; + fs::write(&file_path, original).unwrap(); + + { + let guard = RestoreOnDrop { + path: &file_path, + content: original.to_string(), + armed: true, + }; + // Simulate clang-tidy mutating the file + fs::write(&file_path, "patched content").unwrap(); + // Explicit drop triggers restoration + drop(guard); + } + + assert_eq!(fs::read_to_string(&file_path).unwrap(), original); + } } diff --git a/cpp-linter/src/common_fs.rs b/cpp-linter/src/common_fs.rs index f6bc50b..419b787 100644 --- a/cpp-linter/src/common_fs.rs +++ b/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(()); diff --git a/cpp-linter/src/lib.rs b/cpp-linter/src/lib.rs index eb6fa86..9241a98 100644 --- a/cpp-linter/src/lib.rs +++ b/cpp-linter/src/lib.rs @@ -23,3 +23,26 @@ mod git; pub mod logger; pub mod rest_client; pub mod run; + +#[cfg(test)] +pub(crate) mod test_common { + #![allow(clippy::unwrap_used)] + + use std::{fs, path::PathBuf}; + + /// 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 + } +} diff --git a/cpp-linter/src/rest_client.rs b/cpp-linter/src/rest_client.rs index 1426ab6..ae3c398 100644 --- a/cpp-linter/src/rest_client.rs +++ b/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!( - " {filename}:{line}:{cols}: {severity}: [{diagnostic}]\n > {rationale}\n{concerned_code}", + " {filename}:{line}:{cols}: {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.is_empty() { + "" + } else { + "\n :zap: auto-fix available" + }, 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)] diff --git a/cpp-linter/src/run.rs b/cpp-linter/src/run.rs index 0ec141e..59063aa 100644 --- a/cpp-linter/src/run.rs +++ b/cpp-linter/src/run.rs @@ -193,10 +193,11 @@ pub async fn run_main(args: Vec) -> Result<()> { } #[cfg(test)] -mod test { +pub(crate) mod test { #![allow(clippy::unwrap_used)] use super::run_main; + use crate::test_common::setup_tmp_workspace; use std::env; /// helper to avoid writing to the same GITHUB_OUTPUT file in parallel-running tests. @@ -214,12 +215,13 @@ mod test { #[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 +241,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 +259,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 +280,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(); diff --git a/cspell.config.yml b/cspell.config.yml index 444984c..5bccf46 100644 --- a/cspell.config.yml +++ b/cspell.config.yml @@ -95,6 +95,7 @@ words: - pypi - pyproject - pyyaml + - RAII - ratelimit - reqwest - revparse