From 0739bc41786acd928124de397047e25590b4379a Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Thu, 11 Jun 2026 18:58:40 -0700 Subject: [PATCH 1/3] feat: always get clang-tidy fixes This is another step toward presenting auto-fixed patches for consumers to commit/push. This basically does what `--tidy-review` conditionally did before. But now it is done every time clang-tidy is invoked. Even if not using the PR review feature, this allows us to boast if a clang-tidy note/warning can be auto-fixed. For example, the new thread comment may contain the following for something that can be auto-fixed: - **src/demo/demo.cpp:10:3** note: [readability] :zap: auto-fix available > {concerned code snippet if any} --- cpp-linter/src/clang_tools/clang_tidy.rs | 88 +++++++++++++----------- cpp-linter/src/common_fs.rs | 9 +-- cpp-linter/src/rest_client.rs | 9 ++- 3 files changed, 57 insertions(+), 49 deletions(-) diff --git a/cpp-linter/src/clang_tools/clang_tidy.rs b/cpp-linter/src/clang_tools/clang_tidy.rs index 9132090..4d534bc 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. /// @@ -104,7 +106,7 @@ pub struct TidyAdvice { pub notes: Vec, /// A buffer to hold the contents of the file after applying clang-tidy fixes. - pub patched: Option>, + 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. @@ -301,17 +300,12 @@ 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"); + let original_content = + fs::read_to_string(&repo_file_path).map_err(|e| ClangCaptureError::ReadFileFailed { + file_name: file_name.clone(), + source: e, + })?; if !clang_params.style.is_empty() { cmd.args(["--format-style", clang_params.style.as_str()]); } @@ -349,32 +343,44 @@ 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, - } + )?; + + // move the patched file content into cache + let patched_content = + fs::read_to_string(&repo_file_path).map_err(|e| ClangCaptureError::ReadFileFailed { + file_name: file_name.clone(), + source: e, })?; + let cache_patch_path = clang_params + .repo_root + .join(CACHE_DIR) + .join(file.name.with_added_extension("tidy")); + if let Some(parent) = cache_patch_path.parent() { + fs::create_dir_all(parent).map_err(ClangCaptureError::MkDirFailed)?; } + fs::write(&cache_patch_path, &patched_content).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, &original_content).map_err(|e| { + ClangCaptureError::WriteFileFailed { + file_name: file_name.clone(), + source: e, + } + })?; + + let tidy_advice = TidyAdvice { + notes, + patched: cache_patch_path.to_path_buf(), + }; + file.tidy_advice = Some(tidy_advice); Ok(logs) } @@ -571,8 +577,8 @@ 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(' ')); } 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/rest_client.rs b/cpp-linter/src/rest_client.rs index 1426ab6..7cbda84 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.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)] From d7902c30179dccf35336dbf8c1cb22ab76dccefa Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Fri, 12 Jun 2026 00:36:00 -0700 Subject: [PATCH 2/3] implement drop guard and isolate test assets try to restore scanned file's contents upon error propagation. add more test isolation to prevent race conditions with shared test asserts --- cpp-linter/src/clang_tools/clang_tidy.rs | 121 ++++++++++++++++------- cpp-linter/src/run.rs | 34 ++++++- cspell.config.yml | 1 + 3 files changed, 117 insertions(+), 39 deletions(-) diff --git a/cpp-linter/src/clang_tools/clang_tidy.rs b/cpp-linter/src/clang_tools/clang_tidy.rs index 4d534bc..d3ccdba 100644 --- a/cpp-linter/src/clang_tools/clang_tidy.rs +++ b/cpp-linter/src/clang_tools/clang_tidy.rs @@ -265,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,11 +321,6 @@ pub fn run_clang_tidy( } let repo_file_path = clang_params.repo_root.join(&file.name); cmd.arg("--fix-errors"); - let original_content = - fs::read_to_string(&repo_file_path).map_err(|e| ClangCaptureError::ReadFileFailed { - file_name: file_name.clone(), - source: e, - })?; if !clang_params.style.is_empty() { cmd.args(["--format-style", clang_params.style.as_str()]); } @@ -321,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!( @@ -343,39 +396,13 @@ pub fn run_clang_tidy( ), )); } + let notes = parse_tidy_output( &output.stdout, &clang_params.database_json, &clang_params.repo_root, )?; - // move the patched file content into cache - let patched_content = - fs::read_to_string(&repo_file_path).map_err(|e| ClangCaptureError::ReadFileFailed { - file_name: file_name.clone(), - source: e, - })?; - let cache_patch_path = clang_params - .repo_root - .join(CACHE_DIR) - .join(file.name.with_added_extension("tidy")); - if let Some(parent) = cache_patch_path.parent() { - fs::create_dir_all(parent).map_err(ClangCaptureError::MkDirFailed)?; - } - fs::write(&cache_patch_path, &patched_content).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, &original_content).map_err(|e| { - ClangCaptureError::WriteFileFailed { - file_name: file_name.clone(), - source: e, - } - })?; - let tidy_advice = TidyAdvice { notes, patched: cache_patch_path.to_path_buf(), @@ -389,7 +416,7 @@ mod test { #![allow(clippy::unwrap_used, clippy::expect_used)] use std::{ - env, + env, fs, path::PathBuf, str::FromStr, sync::{Arc, Mutex}, @@ -404,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() { @@ -492,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 { @@ -508,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) @@ -583,4 +611,25 @@ TrenchBroom/TrenchBroom/common/test/src/mdl/tst_ReadFreeImageTexture.cpp:44:48: 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); + } } diff --git a/cpp-linter/src/run.rs b/cpp-linter/src/run.rs index 0ec141e..c94e28a 100644 --- a/cpp-linter/src/run.rs +++ b/cpp-linter/src/run.rs @@ -193,11 +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 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(); 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 From 707938496b7fff96a0d0d5cdd8dab77d285fd392 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Fri, 12 Jun 2026 00:59:51 -0700 Subject: [PATCH 3/3] nit picky changes --- cpp-linter/src/clang_tools/clang_tidy.rs | 15 ++++++++------- cpp-linter/src/lib.rs | 23 +++++++++++++++++++++++ cpp-linter/src/rest_client.rs | 6 +++--- cpp-linter/src/run.rs | 19 ++----------------- 4 files changed, 36 insertions(+), 27 deletions(-) diff --git a/cpp-linter/src/clang_tools/clang_tidy.rs b/cpp-linter/src/clang_tools/clang_tidy.rs index d3ccdba..6d87adc 100644 --- a/cpp-linter/src/clang_tools/clang_tidy.rs +++ b/cpp-linter/src/clang_tools/clang_tidy.rs @@ -105,7 +105,7 @@ 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. + /// A path to the cached contents of the file after applying clang-tidy fixes. pub patched: PathBuf, } @@ -519,7 +519,7 @@ mod test { .unwrap(), ) .unwrap(); - let tmp_workspace = crate::run::test::setup_tmp_workspace(); + 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()]; @@ -614,22 +614,23 @@ TrenchBroom/TrenchBroom/common/test/src/mdl/tst_ReadFreeImageTexture.cpp:44:48: #[test] fn restore_on_drop_fires() { - let tmp = tempfile::NamedTempFile::new().unwrap(); + let tmp = tempfile::tempdir().unwrap(); + let file_path = tmp.path().join("test_file.txt"); let original = "original content"; - fs::write(tmp.path(), original).unwrap(); + fs::write(&file_path, original).unwrap(); { let guard = RestoreOnDrop { - path: tmp.path(), + path: &file_path, content: original.to_string(), armed: true, }; // Simulate clang-tidy mutating the file - fs::write(tmp.path(), "patched content").unwrap(); + fs::write(&file_path, "patched content").unwrap(); // Explicit drop triggers restoration drop(guard); } - assert_eq!(fs::read_to_string(tmp.path()).unwrap(), original); + assert_eq!(fs::read_to_string(&file_path).unwrap(), original); } } 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 7cbda84..ae3c398 100644 --- a/cpp-linter/src/rest_client.rs +++ b/cpp-linter/src/rest_client.rs @@ -442,10 +442,10 @@ fn make_tidy_comment( 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 { + 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 { diff --git a/cpp-linter/src/run.rs b/cpp-linter/src/run.rs index c94e28a..59063aa 100644 --- a/cpp-linter/src/run.rs +++ b/cpp-linter/src/run.rs @@ -197,7 +197,8 @@ pub(crate) mod test { #![allow(clippy::unwrap_used)] use super::run_main; - use std::{env, fs, path::PathBuf}; + use crate::test_common::setup_tmp_workspace; + use std::env; /// helper to avoid writing to the same GITHUB_OUTPUT file in parallel-running tests. fn setup_tmp_gh_out_path() -> tempfile::NamedTempFile { @@ -211,22 +212,6 @@ pub(crate) 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();