From fed8b40ecf6498b95eb761e54be2179a24b55029 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Sun, 18 Oct 2026 09:32:09 +0000 Subject: [PATCH 1/6] feat: apply clang-tidy fixes to other analyzed files A clang-tidy fix for a diagnostic in one file can also change another file (eg. a header declaring a function defined in the analyzed source). These fixes are now collected while files are analyzed in parallel and applied once all files are analyzed, before clang-format runs. A fix for another file is applied to that file's cached copy only if the file is also analyzed, is not ignored by `--ignore-tidy`, and the fix is within the file's changed lines. Each decision is logged in the group of the file whose diagnostic suggested the fix. Resolves #489 --- cpp-linter/src/clang_tools/clang_tidy/mod.rs | 197 +++++++----- .../src/clang_tools/clang_tidy/other_files.rs | 278 +++++++++++++++++ .../clang_tidy/other_files/test.rs | 286 ++++++++++++++++++ cpp-linter/src/clang_tools/clang_tidy/test.rs | 38 ++- cpp-linter/src/clang_tools/mod.rs | 168 ++++++---- 5 files changed, 820 insertions(+), 147 deletions(-) create mode 100644 cpp-linter/src/clang_tools/clang_tidy/other_files.rs create mode 100644 cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs diff --git a/cpp-linter/src/clang_tools/clang_tidy/mod.rs b/cpp-linter/src/clang_tools/clang_tidy/mod.rs index 3bac4052..82e39001 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/mod.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/mod.rs @@ -2,7 +2,7 @@ //! output. use std::{ - collections::{HashMap, HashSet}, + collections::HashMap, env::consts::OS, fs, path::{Path, PathBuf}, @@ -18,6 +18,9 @@ use serde::Deserialize; // project-specific modules/crates use crate::{cli::ClangParams, common_fs::FileObj, error::ClangCaptureError}; +pub mod other_files; +use other_files::{OtherFileFix, Replacement, TidyFixes}; + /// Used to deserialize a json compilation database's translation unit. /// /// The only purpose this serves is to normalize relative paths for build systems that @@ -370,8 +373,9 @@ fn offset_to_line(content: &[u8], offset: usize) -> u32 { /// Apply the clang-tidy `fixes` that pertain to the analyzed file onto its `content`. /// /// Fixes for any other file (eg. a header declaring a function defined in the analyzed -/// file) are not applied, so nothing outside the analyzed file is ever changed. -/// Such fixes are noted in the returned logs instead. +/// file) are not applied here, so nothing outside the analyzed file is ever changed. +/// They are returned in [`TidyFixes::others`] instead, to be applied (if allowed) +/// after all files are analyzed; see [`other_files::apply_other_file_fixes()`]. /// /// Each diagnostic's `fixed_lines` (in the original `content`) are recorded in the /// matching `notes`. @@ -381,80 +385,110 @@ fn apply_fixes<'a>( content: &[u8], notes: &mut [TidyNotification], logs: &mut Vec<(log::Level, String)>, -) -> Vec { - let mut replacements = vec![]; +) -> (Vec, TidyFixes) { + let mut tidy_fixes = TidyFixes::default(); for diag in &fixes.diagnostics { - let msg = &diag.diagnostic_message; - let build_dir = diag.build_directory.as_str(); - let mut fixed_lines = vec![]; - let mut other_files = HashSet::new(); - for replacement in &msg.replacements { - if let Some(other) = paths.other_file(build_dir, &replacement.file_path) { - other_files.insert(other); - } else if replacement - .offset - .checked_add(replacement.length) - .is_some_and(|end| end <= content.len()) - { - let line = offset_to_line(content, replacement.offset); - if !fixed_lines.contains(&line) { - fixed_lines.push(line); - } - replacements.push(replacement); - } else { - logs.push(( - log::Level::Warn, - format!( - "Skipping a fix for {} that is out of bounds of {}: {replacement:?}", - diag.diagnostic_name, - paths.target.to_string_lossy(), - ), - )); - } + let fixed_lines = collect_diagnostic_fixes(diag, paths, content, &mut tidy_fixes, logs); + if !fixed_lines.is_empty() { + record_fixed_lines(diag, paths, content, notes, fixed_lines); } - for other in other_files { + } + let patched = patch_content(content, &tidy_fixes.own, paths.target, logs); + (patched, tidy_fixes) +} + +/// Sort the replacements of one `diag` into [`TidyFixes::own`] or [`TidyFixes::others`]. +/// +/// Returns the lines (in the analyzed file's `content`) that this diagnostic fixes. +fn collect_diagnostic_fixes<'a>( + diag: &'a ExportedDiagnostic, + paths: &mut FixPaths<'a>, + content: &[u8], + tidy_fixes: &mut TidyFixes, + logs: &mut Vec<(log::Level, String)>, +) -> Vec { + let build_dir = diag.build_directory.as_str(); + let mut fixed_lines = vec![]; + for replacement in &diag.diagnostic_message.replacements { + if let Some(other) = paths.other_file(build_dir, &replacement.file_path) { + tidy_fixes.others.push(OtherFileFix { + path: other, + diagnostic: diag.diagnostic_name.clone(), + replacement: Replacement::from(replacement), + }); + } else if replacement + .offset + .checked_add(replacement.length) + .is_some_and(|end| end <= content.len()) + { + let line = offset_to_line(content, replacement.offset); + if !fixed_lines.contains(&line) { + fixed_lines.push(line); + } + tidy_fixes.own.push(Replacement::from(replacement)); + } else { logs.push(( - log::Level::Info, + log::Level::Warn, format!( - "Not applying the fix for {} (reported in {}) to {} because cpp-linter only applies fixes to the analyzed file", + "Skipping a fix for {} that is out of bounds of {}: {replacement:?}", diag.diagnostic_name, paths.target.to_string_lossy(), - other.to_string_lossy(), ), )); } - if fixed_lines.is_empty() { - continue; + } + fixed_lines +} + +/// Record the `fixed_lines` of a `diag` in the notification (parsed from stdout) +/// that matches it. +fn record_fixed_lines<'a>( + diag: &'a ExportedDiagnostic, + paths: &mut FixPaths<'a>, + content: &[u8], + notes: &mut [TidyNotification], + fixed_lines: Vec, +) { + let msg = &diag.diagnostic_message; + let (diag_path, diag_line) = match paths.other_file(&diag.build_directory, &msg.file_path) { + None => ( + paths.target.to_path_buf(), + Some(offset_to_line(content, msg.file_offset)), + ), + Some(other) => { + let line = fs::read(&other) + .ok() + .map(|c| offset_to_line(&c, msg.file_offset)); + (other, line) } - // Find the notification (parsed from stdout) for this diagnostic. - let (diag_path, diag_line) = match paths.other_file(build_dir, &msg.file_path) { - None => ( - paths.target.to_path_buf(), - Some(offset_to_line(content, msg.file_offset)), - ), - Some(other) => { - let line = fs::read(&other) - .ok() - .map(|c| offset_to_line(&c, msg.file_offset)); - (other, line) - } - }; - if let Some(note) = notes.iter_mut().find(|note| { - note.diagnostic == diag.diagnostic_name - && Some(note.line) == diag_line - && diag_path.ends_with(¬e.filename) - }) { - for line in fixed_lines { - if !note.fixed_lines.contains(&line) { - note.fixed_lines.push(line); - } + }; + if let Some(note) = notes.iter_mut().find(|note| { + note.diagnostic == diag.diagnostic_name + && Some(note.line) == diag_line + && diag_path.ends_with(¬e.filename) + }) { + for line in fixed_lines { + if !note.fixed_lines.contains(&line) { + note.fixed_lines.push(line); } } } +} +/// Apply the given `replacements` onto a file's original `content`. +/// +/// The `file` is only used in log messages. +pub(super) fn patch_content( + content: &[u8], + replacements: &[Replacement], + file: &Path, + logs: &mut Vec<(log::Level, String)>, +) -> Vec { // clang-tidy already discards conflicting fixes before exporting them, // but be defensive about duplicates and overlaps anyway. - replacements.sort_by_key(|r| (r.offset, r.length)); + // Duplicates are expected when several translation units fix the same header. + let mut replacements = replacements.iter().collect::>(); + replacements.sort(); replacements.dedup(); let mut patched = Vec::with_capacity(content.len()); let mut cursor = 0; @@ -464,19 +498,41 @@ fn apply_fixes<'a>( log::Level::Warn, format!( "Skipping a fix that overlaps another fix in {}: {replacement:?}", - paths.target.to_string_lossy(), + file.to_string_lossy(), ), )); continue; } patched.extend_from_slice(&content[cursor..replacement.offset]); - patched.extend_from_slice(replacement.replacement_text.as_bytes()); + patched.extend_from_slice(replacement.text.as_bytes()); cursor = replacement.offset + replacement.length; } patched.extend_from_slice(&content[cursor..]); patched } +/// Write the `patched` content of a `file` to the cache. +/// +/// Returns the path to the cached file. +pub(super) fn write_cache( + clang_params: &ClangParams, + file: &Path, + patched: &[u8], +) -> Result { + let cache_patch_path = clang_params.get_cache_path().join(file); + fs::create_dir_all( + cache_patch_path + .parent() + .ok_or(ClangCaptureError::UnknownCacheParentPath)?, + ) + .map_err(ClangCaptureError::MkDirFailed)?; + fs::write(&cache_patch_path, patched).map_err(|e| ClangCaptureError::WriteFileFailed { + file_name: cache_patch_path.to_string_lossy().to_string(), + source: e, + })?; + Ok(cache_patch_path) +} + /// Build the clang-tidy command that analyzes the given `file`. /// /// clang-tidy exports its fixes to `fixes_path` instead of applying them. @@ -566,10 +622,11 @@ fn read_exported_fixes(fixes_path: &Path) -> Result, clang_params: &ClangParams, -) -> Result, ClangCaptureError> { +) -> Result<(Vec<(log::Level, String)>, TidyFixes), ClangCaptureError> { let file_name = file.name.to_string_lossy().to_string(); let repo_file_path = clang_params.repo_root.join(&file.name); let original_content = @@ -639,15 +696,11 @@ pub fn run_clang_tidy( .map_err(ClangCaptureError::UnknownWorkingDirectory)?; let target = normalize_path(&working_dir.join(&file.name)); let mut paths = FixPaths::new(&target, &working_dir); - let patched = apply_fixes(&fixes, &mut paths, &original_content, &mut notes, &mut logs); - fs::write(&cache_patch_path, patched).map_err(|e| ClangCaptureError::WriteFileFailed { - file_name: cache_patch_path.to_string_lossy().to_string(), - source: e, - })?; - - file.patched_path = Some(cache_patch_path); + let (patched, tidy_fixes) = + apply_fixes(&fixes, &mut paths, &original_content, &mut notes, &mut logs); + file.patched_path = Some(write_cache(clang_params, &file.name, &patched)?); file.tidy_advice = Some(TidyAdvice { notes }); - Ok(logs) + Ok((logs, tidy_fixes)) } #[cfg(test)] diff --git a/cpp-linter/src/clang_tools/clang_tidy/other_files.rs b/cpp-linter/src/clang_tools/clang_tidy/other_files.rs new file mode 100644 index 00000000..fa1707b4 --- /dev/null +++ b/cpp-linter/src/clang_tools/clang_tidy/other_files.rs @@ -0,0 +1,278 @@ +//! Applying clang-tidy fixes to files other than the analyzed one. +//! +//! A fix for a diagnostic in one file can also change another file +//! (eg. `readability-non-const-parameter` adds `const` to the definition in a source +//! file and to the declaration in a header). clang-tidy analyzes one file at a time, +//! so these fixes are collected while files are analyzed in parallel, and applied +//! once all files are analyzed. This way, no two threads write the same cached file. + +use std::{ + collections::{BTreeMap, HashMap}, + fs, + path::{Path, PathBuf}, + sync::{Arc, Mutex}, +}; + +use clang_tools_manager::utils::normalize_path; + +use super::{ExportedReplacement, offset_to_line, patch_content, write_cache}; +use crate::{cli::ClangParams, common_fs::FileObj, error::ClangCaptureError}; + +/// A text replacement in a file's original content. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] +pub struct Replacement { + /// The byte offset where the replaced text starts. + pub offset: usize, + /// The length (in bytes) of the replaced text. + pub length: usize, + /// The replacement text. + pub text: String, +} + +impl From<&ExportedReplacement> for Replacement { + fn from(value: &ExportedReplacement) -> Self { + Self { + offset: value.offset, + length: value.length, + text: value.replacement_text.clone(), + } + } +} + +/// A clang-tidy fix for a file other than the analyzed one. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct OtherFileFix { + /// The absolute, normalized path to the other file. + pub path: PathBuf, + /// The name of the diagnostic that suggested the fix. + pub diagnostic: String, + /// The replacement in the other file's original content. + pub replacement: Replacement, +} + +/// The fixes that clang-tidy suggested while analyzing a file. +#[derive(Debug, Default, Clone, PartialEq, Eq)] +pub struct TidyFixes { + /// The replacements in the analyzed file. + pub own: Vec, + /// The replacements in other files. + pub others: Vec, +} + +/// Why a [`OtherFileFix`] was not applied. +enum Rejection { + NotAnalyzed, + IgnoredByTidy, + OutOfBounds, + OutsideChangedLines, +} + +impl Rejection { + fn reason(&self) -> &'static str { + match self { + Self::NotAnalyzed => "it is not one of the analyzed files", + Self::IgnoredByTidy => "it is ignored by `--ignore-tidy`", + Self::OutOfBounds => "the fix is out of the file's bounds", + Self::OutsideChangedLines => "the fix is outside the file's changed lines", + } + } +} + +/// The analyzed files, looked up by their absolute path. +struct AnalyzedFiles<'a> { + files: &'a [Arc>], + by_path: HashMap, + contents: HashMap>, +} + +impl<'a> AnalyzedFiles<'a> { + fn new( + files: &'a [Arc>], + working_dir: &Path, + ) -> Result { + let mut by_path = HashMap::new(); + for (index, file) in files.iter().enumerate() { + let file = file.lock().map_err(|_| ClangCaptureError::MutexPoisoned)?; + let path = normalize_path(&working_dir.join(&file.name)); + // also match paths that clang-tidy reports through symbolic links + if let Ok(canonical) = fs::canonicalize(&path) { + by_path.insert(canonical, index); + } + by_path.insert(path, index); + } + Ok(Self { + files, + by_path, + contents: HashMap::new(), + }) + } + + /// Get the index of the analyzed file at `path`. + fn find(&self, path: &Path) -> Option { + self.by_path.get(path).copied().or_else(|| { + fs::canonicalize(path) + .ok() + .and_then(|canonical| self.by_path.get(&canonical).copied()) + }) + } + + /// Get (and cache) the original content of the analyzed file at `index`. + fn content( + &mut self, + index: usize, + clang_params: &ClangParams, + ) -> Result<&[u8], ClangCaptureError> { + if !self.contents.contains_key(&index) { + let name = self.files[index] + .lock() + .map_err(|_| ClangCaptureError::MutexPoisoned)? + .name + .clone(); + let content = fs::read(clang_params.repo_root.join(&name)).map_err(|e| { + ClangCaptureError::ReadFileFailed { + file_name: name.to_string_lossy().to_string(), + source: e, + } + })?; + self.contents.insert(index, content); + } + Ok(self.contents[&index].as_slice()) + } + + /// Decide whether the `fix` may be applied to the analyzed file at `index`. + fn check( + &mut self, + index: usize, + fix: &OtherFileFix, + clang_params: &ClangParams, + ) -> Result, ClangCaptureError> { + let content_len = self.content(index, clang_params)?.len(); + let replacement = &fix.replacement; + let Some(end) = replacement + .offset + .checked_add(replacement.length) + .filter(|end| *end <= content_len) + else { + return Ok(Some(Rejection::OutOfBounds)); + }; + let content = self.content(index, clang_params)?; + let (start_line, end_line) = ( + offset_to_line(content, replacement.offset), + offset_to_line(content, end), + ); + let file = self.files[index] + .lock() + .map_err(|_| ClangCaptureError::MutexPoisoned)?; + if clang_params + .tidy_filter + .as_ref() + .is_some_and(|f| !f.is_qualified(&file.name)) + { + return Ok(Some(Rejection::IgnoredByTidy)); + } + let ranges = file.get_ranges(&clang_params.lines_changed_only); + if !ranges.is_empty() + && !ranges + .iter() + .any(|r| r.contains(&start_line) && r.contains(&end_line)) + { + return Ok(Some(Rejection::OutsideChangedLines)); + } + Ok(None) + } +} + +/// Apply the clang-tidy fixes for other files (see [`TidyFixes::others`]) to the +/// cached copies of those files. +/// +/// `tidy_fixes` holds the fixes found while analyzing the file at the paired index of +/// `files`. A fix for another file is only applied if that file is also analyzed, +/// is not ignored by `--ignore-tidy`, and the fix is within the file's changed lines +/// (according to `--lines-changed-only`). +/// +/// Messages about each fix are added to the `logs` of the analyzed file whose +/// diagnostic suggested it (at the paired index of `files`). +/// +/// This must be called after all files are analyzed by clang-tidy and before any +/// file is analyzed by clang-format, because clang-format formats the fixes in the +/// cached copy. +pub fn apply_other_file_fixes( + files: &[Arc>], + tidy_fixes: &[(usize, TidyFixes)], + clang_params: &ClangParams, + logs: &mut [Vec<(log::Level, String)>], +) -> Result<(), ClangCaptureError> { + if tidy_fixes.iter().all(|(_, fixes)| fixes.others.is_empty()) { + return Ok(()); + } + let working_dir = std::path::absolute(&clang_params.repo_root) + .map_err(ClangCaptureError::UnknownWorkingDirectory)?; + let mut analyzed = AnalyzedFiles::new(files, &working_dir)?; + // the replacements to apply, keyed by the index of the file they apply to + let mut pending: BTreeMap> = BTreeMap::new(); + for (source, fixes) in tidy_fixes { + let source_name = file_name(&files[*source])?; + for fix in &fixes.others { + // show the path relative to the repository root, if possible + let other = fix + .path + .strip_prefix(&working_dir) + .unwrap_or(&fix.path) + .to_string_lossy(); + let rejection = match analyzed.find(&fix.path) { + None => Some(Rejection::NotAnalyzed), + Some(index) => { + let rejection = analyzed.check(index, fix, clang_params)?; + if rejection.is_none() { + pending + .entry(index) + .or_default() + .push(fix.replacement.clone()); + } + rejection + } + }; + logs[*source].push(match rejection { + None => ( + log::Level::Info, + format!( + "Applying the fix for {} (reported in {source_name}) to {other}", + fix.diagnostic, + ), + ), + Some(rejection) => ( + log::Level::Info, + format!( + "Not applying the fix for {} (reported in {source_name}) to {other} because {}", + fix.diagnostic, + rejection.reason(), + ), + ), + }); + } + } + for (index, mut replacements) in pending { + if let Some((_, fixes)) = tidy_fixes.iter().find(|(i, _)| *i == index) { + replacements.extend(fixes.own.iter().cloned()); + } + let content = analyzed.content(index, clang_params)?; + let mut file = files[index] + .lock() + .map_err(|_| ClangCaptureError::MutexPoisoned)?; + let patched = patch_content(content, &replacements, &file.name, &mut logs[index]); + file.patched_path = Some(write_cache(clang_params, &file.name, &patched)?); + } + Ok(()) +} + +fn file_name(file: &Arc>) -> Result { + Ok(file + .lock() + .map_err(|_| ClangCaptureError::MutexPoisoned)? + .name + .to_string_lossy() + .to_string()) +} + +#[cfg(test)] +mod test; diff --git a/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs b/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs new file mode 100644 index 00000000..13c96eb0 --- /dev/null +++ b/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs @@ -0,0 +1,286 @@ +#![allow(clippy::unwrap_used)] + +use std::{ + env, fs, + path::{Path, PathBuf}, + str::FromStr, + sync::{Arc, Mutex}, +}; + +use clang_tools_manager::RequestedVersion; +use git_bot_feedback::FileFilter; + +use super::{OtherFileFix, Replacement, TidyFixes, apply_other_file_fixes}; +use crate::{ + clang_tools::{ClangTool, analyze_files}, + cli::{ClangParams, LinesChangedOnly}, + common_fs::FileObj, +}; + +const HEADER: &str = "#pragma once\nvoid set(int *p);\n"; +const SOURCE: &str = "#include \"fix.h\"\nvoid set(int *p) {\n int x = *p;\n (void)x;\n}\n"; + +/// The offset of `int *p` in [`HEADER`]. +const HEADER_FIX_OFFSET: usize = 22; + +fn get_tidy_exe() -> PathBuf { + ClangTool::ClangTidy + .get_exe_path( + &RequestedVersion::from_str( + env::var("CLANG_VERSION").unwrap_or("".to_string()).as_str(), + ) + .unwrap(), + ) + .unwrap() +} + +/// Write [`HEADER`] and [`SOURCE`] into a new temporary workspace. +fn workspace() -> tempfile::TempDir { + let tmp = tempfile::TempDir::with_prefix("cpp-linter-unit-tests_").unwrap(); + fs::write(tmp.path().join("fix.h"), HEADER).unwrap(); + fs::write(tmp.path().join("fix.cpp"), SOURCE).unwrap(); + tmp +} + +fn as_files(files: Vec) -> Vec>> { + files.into_iter().map(|f| Arc::new(Mutex::new(f))).collect() +} + +/// A fix that adds `const` to the declaration in the header. +fn header_fix(workspace: &Path) -> OtherFileFix { + OtherFileFix { + path: workspace.join("fix.h"), + diagnostic: "readability-non-const-parameter".to_string(), + replacement: Replacement { + offset: HEADER_FIX_OFFSET, + length: 0, + text: "const ".to_string(), + }, + } +} + +fn header_fixes(workspace: &Path) -> TidyFixes { + TidyFixes { + own: vec![], + others: vec![header_fix(workspace)], + } +} + +fn patched(file: &Arc>) -> Option { + let file = file.lock().unwrap(); + file.patched_path + .as_ref() + .map(|p| fs::read_to_string(p).unwrap()) +} + +fn messages(logs: &[(log::Level, String)]) -> Vec<&str> { + logs.iter().map(|(_, msg)| msg.as_str()).collect() +} + +/// clang-tidy suggests a fix for the header while analyzing the source. +/// The header is also analyzed, so the fix is applied to the header's cached copy. +#[tokio::test] +async fn fix_analyzed_header() { + let tmp = workspace(); + let clang_params = Arc::new(ClangParams { + tidy_checks: "-*,readability-non-const-parameter".to_string(), + clang_tidy_command: Some(get_tidy_exe()), + repo_root: tmp.path().to_path_buf(), + ..Default::default() + }); + let files = as_files(vec![ + FileObj::new(PathBuf::from("fix.cpp")), + FileObj::new(PathBuf::from("fix.h")), + ]); + let logs = analyze_files(&files, clang_params).await.unwrap(); + + // worktree is untouched + assert_eq!( + fs::read_to_string(tmp.path().join("fix.h")).unwrap(), + HEADER + ); + assert_eq!( + fs::read_to_string(tmp.path().join("fix.cpp")).unwrap(), + SOURCE + ); + + // both cached copies are fixed + let fixed = |content: &str| content.replace("set(int *p)", "set(const int *p)"); + assert_eq!(patched(&files[0]).unwrap(), fixed(SOURCE)); + assert_eq!(patched(&files[1]).unwrap(), fixed(HEADER)); + + // the fix for the header is logged in the source's group + assert!( + messages(&logs[0]).iter().any(|msg| { + *msg == "Applying the fix for readability-non-const-parameter (reported in fix.cpp) to fix.h" + }), + "{logs:#?}" + ); +} + +#[test] +fn not_analyzed() { + let tmp = workspace(); + let clang_params = ClangParams { + repo_root: tmp.path().to_path_buf(), + ..Default::default() + }; + let files = as_files(vec![FileObj::new(PathBuf::from("fix.cpp"))]); + let mut logs = vec![vec![]]; + apply_other_file_fixes( + &files, + &[(0, header_fixes(tmp.path()))], + &clang_params, + &mut logs, + ) + .unwrap(); + assert!(patched(&files[0]).is_none()); + let messages = messages(&logs[0]); + assert_eq!(messages.len(), 1); + assert!(messages[0].starts_with("Not applying the fix for readability-non-const-parameter")); + assert!(messages[0].ends_with("to fix.h because it is not one of the analyzed files")); +} + +#[test] +fn ignored_by_tidy() { + let tmp = workspace(); + let clang_params = ClangParams { + repo_root: tmp.path().to_path_buf(), + tidy_filter: Some(FileFilter::new(&["fix.h"], &["cpp", "h"], None)), + ..Default::default() + }; + let files = as_files(vec![ + FileObj::new(PathBuf::from("fix.cpp")), + FileObj::new(PathBuf::from("fix.h")), + ]); + let mut logs = vec![vec![], vec![]]; + apply_other_file_fixes( + &files, + &[(0, header_fixes(tmp.path()))], + &clang_params, + &mut logs, + ) + .unwrap(); + assert!(patched(&files[1]).is_none()); + let messages = messages(&logs[0]); + assert_eq!(messages.len(), 1); + assert!(messages[0].ends_with("to fix.h because it is ignored by `--ignore-tidy`")); + assert!(logs[1].is_empty()); +} + +#[test] +fn outside_changed_lines() { + let tmp = workspace(); + let clang_params = ClangParams { + repo_root: tmp.path().to_path_buf(), + lines_changed_only: LinesChangedOnly::On, + ..Default::default() + }; + let files = as_files(vec![ + FileObj::from(PathBuf::from("fix.cpp"), vec![2], vec![2..=2]), + // only the first line of the header was changed + FileObj::from(PathBuf::from("fix.h"), vec![1], vec![1..=1]), + ]); + let mut logs = vec![vec![], vec![]]; + apply_other_file_fixes( + &files, + &[(0, header_fixes(tmp.path()))], + &clang_params, + &mut logs, + ) + .unwrap(); + assert!(patched(&files[1]).is_none()); + let messages = messages(&logs[0]); + assert_eq!(messages.len(), 1); + assert!(messages[0].ends_with("to fix.h because the fix is outside the file's changed lines")); +} + +#[test] +fn out_of_bounds() { + let tmp = workspace(); + let clang_params = ClangParams { + repo_root: tmp.path().to_path_buf(), + ..Default::default() + }; + let files = as_files(vec![ + FileObj::new(PathBuf::from("fix.cpp")), + FileObj::new(PathBuf::from("fix.h")), + ]); + let mut fix = header_fix(tmp.path()); + fix.replacement.offset = HEADER.len(); + fix.replacement.length = 1; + let fixes = TidyFixes { + own: vec![], + others: vec![fix], + }; + let mut logs = vec![vec![], vec![]]; + apply_other_file_fixes(&files, &[(0, fixes)], &clang_params, &mut logs).unwrap(); + assert!(patched(&files[1]).is_none()); + let messages = messages(&logs[0]); + assert_eq!(messages.len(), 1); + assert!(messages[0].ends_with("to fix.h because the fix is out of the file's bounds")); +} + +/// The same fix for the header is suggested by two analyzed sources, +/// and the header has a fix of its own. +#[test] +fn merge_with_own_fixes() { + let tmp = workspace(); + fs::write(tmp.path().join("other.cpp"), SOURCE).unwrap(); + let clang_params = ClangParams { + repo_root: tmp.path().to_path_buf(), + ..Default::default() + }; + let files = as_files(vec![ + FileObj::new(PathBuf::from("fix.cpp")), + FileObj::new(PathBuf::from("fix.h")), + FileObj::new(PathBuf::from("other.cpp")), + ]); + let own = TidyFixes { + own: vec![Replacement { + offset: 0, + length: 0, + text: "// fixed\n".to_string(), + }], + others: vec![], + }; + let tidy_fixes = [ + (0, header_fixes(tmp.path())), + (1, own), + (2, header_fixes(tmp.path())), + ]; + let mut logs = vec![vec![], vec![], vec![]]; + apply_other_file_fixes(&files, &tidy_fixes, &clang_params, &mut logs).unwrap(); + assert_eq!( + patched(&files[1]).unwrap(), + "// fixed\n#pragma once\nvoid set(const int *p);\n" + ); + for (source, name) in [(0, "fix.cpp"), (2, "other.cpp")] { + let messages = messages(&logs[source]); + assert_eq!(messages.len(), 1); + assert!(messages[0].contains(&format!("(reported in {name}) to "))); + assert!(messages[0].starts_with("Applying the fix")); + } + // duplicate fixes do not cause warnings + assert!(logs[1].is_empty(), "{:#?}", logs[1]); +} + +#[test] +fn no_other_fixes() { + let clang_params = ClangParams { + // an unknown directory is never read + repo_root: PathBuf::from("does-not-exist"), + ..Default::default() + }; + let files = as_files(vec![FileObj::new(PathBuf::from("fix.cpp"))]); + let mut logs = vec![vec![]]; + apply_other_file_fixes( + &files, + &[(0, TidyFixes::default())], + &clang_params, + &mut logs, + ) + .unwrap(); + assert!(logs[0].is_empty()); + assert!(patched(&files[0]).is_none()); +} diff --git a/cpp-linter/src/clang_tools/clang_tidy/test.rs b/cpp-linter/src/clang_tools/clang_tidy/test.rs index a07fdfd8..e4c0a6d2 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/test.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/test.rs @@ -18,7 +18,7 @@ use crate::{ use super::{ CLANG_DIAGNOSTICS_URL, ClangCaptureError, CompilationUnit, ExportedFixes, FixPaths, - NOTE_HEADER, TidyNotification, apply_fixes, run_clang_tidy, + NOTE_HEADER, OtherFileFix, Replacement, TidyNotification, apply_fixes, run_clang_tidy, }; #[test] @@ -145,6 +145,7 @@ fn use_extra_args() { fs::create_dir_all(clang_params.get_cache_path()).unwrap(); let logs = run_clang_tidy(&mut file_lock, &clang_params) .unwrap() + .0 .into_iter() .filter_map(|(_lvl, msg)| { if msg.contains("Running ") { @@ -410,7 +411,7 @@ fn fixes_do_not_modify_worktree() { }; let file = Mutex::new(FileObj::new(PathBuf::from("fix.cpp"))); let mut file = file.lock().unwrap(); - let logs = run_clang_tidy(&mut file, &clang_params).unwrap(); + let (_logs, fixes) = run_clang_tidy(&mut file, &clang_params).unwrap(); // worktree is untouched assert_eq!( @@ -442,13 +443,12 @@ fn fixes_do_not_modify_worktree() { .unwrap(); assert_eq!(note.fixed_lines, vec![2]); - // the fix that was not applied to the header is noted - assert!(logs.iter().any(|(lvl, msg)| { - *lvl == log::Level::Info - && msg.starts_with("Not applying the fix for readability-non-const-parameter") - && msg.ends_with("because cpp-linter only applies fixes to the analyzed file") - && msg.contains("fix.h") - })); + // the fix for the header is returned (to be applied later, if allowed) + assert_eq!(fixes.own.len(), 1); + assert_eq!(fixes.others.len(), 1); + let header_fix = &fixes.others[0]; + assert_eq!(header_fix.diagnostic, "readability-non-const-parameter"); + assert!(header_fix.path.ends_with("fix.h")); } fn dummy_fixes(target: &Path) -> String { @@ -507,14 +507,26 @@ fn apply_exported_fixes() { notes[0].line = 2; let mut logs = vec![]; let mut paths = FixPaths::new(&target, tmp.path()); - let patched = apply_fixes(&fixes, &mut paths, content, &mut notes, &mut logs); + let (patched, tidy_fixes) = apply_fixes(&fixes, &mut paths, content, &mut notes, &mut logs); assert_eq!(String::from_utf8(patched).unwrap(), "line\n auto\nx y;\n"); assert_eq!(notes[0].fixed_lines, vec![2]); let messages = logs.into_iter().map(|(_, msg)| msg).collect::>(); - assert_eq!(messages.len(), 3, "{messages:#?}"); + assert_eq!(messages.len(), 2, "{messages:#?}"); assert!(messages[0].contains("out of bounds")); - assert!(messages[1].starts_with("Not applying") && messages[1].contains("fix.h")); - assert!(messages[2].contains("overlaps")); + assert!(messages[1].contains("overlaps")); + // the fix for another file is not applied, but returned + assert_eq!( + tidy_fixes.others, + vec![OtherFileFix { + path: tmp.path().join("fix.h"), + diagnostic: "modernize-use-auto".to_string(), + replacement: Replacement { + offset: 0, + length: 4, + text: "nope".to_string(), + }, + }] + ); } #[test] diff --git a/cpp-linter/src/clang_tools/mod.rs b/cpp-linter/src/clang_tools/mod.rs index 93527b13..305f99df 100644 --- a/cpp-linter/src/clang_tools/mod.rs +++ b/cpp-linter/src/clang_tools/mod.rs @@ -3,7 +3,6 @@ use std::{ fs, - path::PathBuf, sync::{Arc, Mutex}, }; @@ -25,61 +24,110 @@ use crate::{ pub mod clang_format; use clang_format::run_clang_format; pub mod clang_tidy; -use clang_tidy::run_clang_tidy; +use clang_tidy::{ + other_files::{TidyFixes, apply_other_file_fixes}, + run_clang_tidy, +}; + +/// A collection of cached logs: a [`Vec`] of tuples that hold +/// - log level +/// - messages +type Logs = Vec<(log::Level, String)>; -/// This creates a task to run clang-tidy and clang-format on a single file. +/// Run clang-tidy on a single file. /// -/// Returns a Future that infallibly resolves to a 2-tuple that contains +/// Returns the cached logs and the fixes that clang-tidy suggested (if it was run). +fn analyze_tidy( + file: Arc>, + clang_params: Arc, +) -> Result<(Logs, Option), ClangCaptureError> { + let mut file = file.lock().map_err(|_| ClangCaptureError::MutexPoisoned)?; + if clang_params.clang_tidy_command.is_none() { + return Ok((vec![], None)); + } + if clang_params + .tidy_filter + .as_ref() + .is_some_and(|f| !f.is_qualified(file.name.as_path())) + { + let msg = format!( + "{} not scanned by clang-tidy due to `--ignore-tidy`", + file.name.as_os_str().to_string_lossy() + ); + return Ok((vec![(log::Level::Info, msg)], None)); + } + let (logs, fixes) = run_clang_tidy(&mut file, &clang_params)?; + Ok((logs, Some(fixes))) +} + +/// Run clang-format on a single file. /// -/// 1. The file's path. -/// 2. A collections of cached logs. A [`Vec`] of tuples that hold -/// - log level -/// - messages -fn analyze_single_file( +/// This must happen after all clang-tidy fixes for the `file` are cached +/// (see [`apply_other_file_fixes()`]), because clang-format also formats those fixes. +fn analyze_format( file: Arc>, clang_params: Arc, -) -> Result<(PathBuf, Vec<(log::Level, String)>), ClangCaptureError> { +) -> Result { let mut file = file.lock().map_err(|_| ClangCaptureError::MutexPoisoned)?; - let mut logs = vec![]; - if clang_params.clang_tidy_command.is_some() { - if clang_params - .tidy_filter - .as_ref() - .is_some_and(|f| f.is_qualified(file.name.as_path())) - || clang_params.tidy_filter.is_none() - { - let tidy_result = run_clang_tidy(&mut file, &clang_params)?; - logs.extend(tidy_result); - } else { - logs.push(( - log::Level::Info, - format!( - "{} not scanned by clang-tidy due to `--ignore-tidy`", - file.name.as_os_str().to_string_lossy() - ), - )); - } + if clang_params.clang_format_command.is_none() { + return Ok(vec![]); } - if clang_params.clang_format_command.is_some() { - if clang_params - .format_filter - .as_ref() - .is_some_and(|f| f.is_qualified(file.name.as_path())) - || clang_params.format_filter.is_none() - { - let format_result = run_clang_format(&mut file, &clang_params)?; - logs.extend(format_result); - } else { - logs.push(( - log::Level::Info, - format!( - "{} not scanned by clang-format due to `--ignore-format`", - file.name.as_os_str().to_string_lossy() - ), - )); + if clang_params + .format_filter + .as_ref() + .is_some_and(|f| !f.is_qualified(file.name.as_path())) + { + let msg = format!( + "{} not scanned by clang-format due to `--ignore-format`", + file.name.as_os_str().to_string_lossy() + ); + return Ok(vec![(log::Level::Info, msg)]); + } + run_clang_format(&mut file, &clang_params) +} + +/// Run clang-tidy and then clang-format on all `files`. +/// +/// Files are analyzed in parallel. clang-format only runs after clang-tidy analyzed +/// all files, so that clang-tidy fixes for other analyzed files (eg. a header) are +/// cached first; see [`apply_other_file_fixes()`]. +/// +/// Returns the cached logs for each file (in the same order as `files`). +async fn analyze_files( + files: &[Arc>], + clang_params: Arc, +) -> Result, ClangTaskError> { + let mut logs = vec![Logs::new(); files.len()]; + + let mut executors = JoinSet::new(); + for (index, file) in files.iter().enumerate() { + let (file, params) = (file.clone(), clang_params.clone()); + executors.spawn(async move { (index, analyze_tidy(file, params)) }); + } + let mut tidy_fixes = vec![]; + while let Some(output) = executors.join_next().await { + // output? acts as a fast-fail for any `spawn()` error, + // and result? does the same for any `analyze_tidy()` error. + // Any unresolved tasks are aborted and dropped when an error is returned here. + let (index, result) = output?; + let (file_logs, fixes) = result?; + logs[index].extend(file_logs); + if let Some(fixes) = fixes { + tidy_fixes.push((index, fixes)); } } - Ok((file.name.clone(), logs)) + apply_other_file_fixes(files, &tidy_fixes, &clang_params, &mut logs)?; + + let mut executors = JoinSet::new(); + for (index, file) in files.iter().enumerate() { + let (file, params) = (file.clone(), clang_params.clone()); + executors.spawn(async move { (index, analyze_format(file, params)) }); + } + while let Some(output) = executors.join_next().await { + let (index, result) = output?; + logs[index].extend(result?); + } + Ok(logs) } /// A struct to contain the version numbers of the clang-tools used @@ -164,20 +212,13 @@ pub async fn capture_clang_tools_output( } }; - let mut executors = JoinSet::new(); - let arc_params = Arc::new(clang_params); - // iterate over the discovered files and run the clang tools - for file in files { - let arc_file = file.clone(); - let arc_params = arc_params.clone(); - executors.spawn(async move { analyze_single_file(arc_file, arc_params) }); - } - - while let Some(output) = executors.join_next().await { - // output?? acts as a fast-fail for any error encountered. - // This includes any `spawn()` error and any `analyze_single_file()` error. - // Any unresolved tasks are aborted and dropped when an error is returned here. - let (file_name, logs) = output??; + let logs = analyze_files(files, Arc::new(clang_params)).await?; + for (file, logs) in files.iter().zip(logs) { + let file_name = file + .lock() + .map_err(|_| ClangCaptureError::MutexPoisoned)? + .name + .clone(); let log_group_name = format!("Analyzing {}", file_name.to_string_lossy()); rest_api_client.start_log_group(&log_group_name); for (level, msg) in logs { @@ -362,7 +403,10 @@ pub fn make_patch<'buffer>( mod tests { #![allow(clippy::unwrap_used)] - use std::{env, fs, path::Path}; + use std::{ + env, fs, + path::{Path, PathBuf}, + }; use clang_tools_manager::logger::try_init_logger; use git_bot_feedback::ReviewComment; From 50414df9f562fa1241436283634004fba4faac63 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Sun, 18 Oct 2026 09:34:17 +0000 Subject: [PATCH 2/6] test: allow single range in vec init --- cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs b/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs index 13c96eb0..61b56d2a 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs @@ -169,6 +169,7 @@ fn ignored_by_tidy() { } #[test] +#[allow(clippy::single_range_in_vec_init)] fn outside_changed_lines() { let tmp = workspace(); let clang_params = ClangParams { From 00d3e696af13417040b6886743018c3d157d55eb Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Sun, 18 Oct 2026 10:14:21 +0000 Subject: [PATCH 3/6] test: cover diagnostics in other files and unreadable analyzed files --- .../clang_tidy/other_files/test.rs | 28 +++++++++++ cpp-linter/src/clang_tools/clang_tidy/test.rs | 47 +++++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs b/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs index 61b56d2a..c0da5cc1 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs @@ -285,3 +285,31 @@ fn no_other_fixes() { assert!(logs[0].is_empty()); assert!(patched(&files[0]).is_none()); } + +/// An analyzed file that was removed after it was analyzed cannot be fixed. +#[test] +fn analyzed_file_missing() { + let tmp = workspace(); + let clang_params = ClangParams { + repo_root: tmp.path().to_path_buf(), + ..Default::default() + }; + let files = as_files(vec![ + FileObj::new(PathBuf::from("fix.cpp")), + FileObj::new(PathBuf::from("fix.h")), + ]); + // keep the header's path known to `AnalyzedFiles`, but make it unreadable + let fix = header_fix(tmp.path()); + fs::remove_file(tmp.path().join("fix.h")).unwrap(); + fs::create_dir(tmp.path().join("fix.h")).unwrap(); + let fixes = TidyFixes { + own: vec![], + others: vec![fix], + }; + let mut logs = vec![vec![], vec![]]; + let err = apply_other_file_fixes(&files, &[(0, fixes)], &clang_params, &mut logs).unwrap_err(); + assert!( + matches!(err, crate::error::ClangCaptureError::ReadFileFailed { ref file_name, .. } if file_name == "fix.h"), + "{err:?}" + ); +} diff --git a/cpp-linter/src/clang_tools/clang_tidy/test.rs b/cpp-linter/src/clang_tools/clang_tidy/test.rs index e4c0a6d2..ee76bbeb 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/test.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/test.rs @@ -604,3 +604,50 @@ fn tidy_stale_fixes_not_removable() { let err = run_clang_tidy(&mut file.lock().unwrap(), &clang_params).unwrap_err(); assert!(matches!(err, ClangCaptureError::RemoveFileFailed { .. })); } + +/// A diagnostic reported in another file (eg. a header) can suggest a fix +/// for the analyzed file. The fixed lines are recorded in the header's note. +#[test] +fn fixed_lines_of_diagnostic_in_other_file() { + let tmp = tempfile::tempdir().unwrap(); + let header = tmp.path().join("fix.h"); + fs::write(&header, FIX_HEADER).unwrap(); + let target = tmp.path().join("demo.cpp"); + let (target_str, header_str) = (target.to_string_lossy(), header.to_string_lossy()); + let yml = format!( + r#"--- +MainSourceFile: '{target_str}' +Diagnostics: + - DiagnosticName: modernize-use-auto + DiagnosticMessage: + Message: 'a rationale' + FilePath: '{header_str}' + FileOffset: 22 + Replacements: + - FilePath: '{target_str}' + Offset: 7 + Length: 3 + ReplacementText: 'auto' + Level: Warning + BuildDirectory: '{build_dir}' +... +"#, + build_dir = tmp.path().to_string_lossy(), + ); + let fixes: ExportedFixes = serde_saphyr::from_str(&yml).unwrap(); + let mut notes = vec![dummy_note("fix.h", vec![])]; + notes[0].line = 2; + let mut logs = vec![]; + let mut paths = FixPaths::new(&target, tmp.path()); + let (patched, tidy_fixes) = apply_fixes( + &fixes, + &mut paths, + b"line\n int y;\n", + &mut notes, + &mut logs, + ); + assert_eq!(String::from_utf8(patched).unwrap(), "line\n auto y;\n"); + assert!(logs.is_empty(), "{logs:#?}"); + assert!(tidy_fixes.others.is_empty()); + assert_eq!(notes[0].fixed_lines, vec![2]); +} From f49e5bc164e6b2696171b763bbff93b0bf370270 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Sun, 18 Oct 2026 11:52:44 +0000 Subject: [PATCH 4/6] fix: explain cross-file fixes and check the last replaced line A fix applied to another analyzed file now adds a copy of the source diagnostic's note to that file's tidy advice, so the PR review suggestion for it names the diagnostic and rationale. Notes about the file itself are only merged into its own note, so nothing is counted twice. The changed-lines check now uses the last replaced byte, so a fix that removes a whole line (with its line break) is not rejected because of the next line. --- cpp-linter/src/clang_tools/clang_tidy/mod.rs | 44 +++--- .../src/clang_tools/clang_tidy/other_files.rs | 113 +++++++++++++--- .../clang_tidy/other_files/test.rs | 126 +++++++++++++++++- cpp-linter/src/clang_tools/clang_tidy/test.rs | 5 + 4 files changed, 250 insertions(+), 38 deletions(-) diff --git a/cpp-linter/src/clang_tools/clang_tidy/mod.rs b/cpp-linter/src/clang_tools/clang_tidy/mod.rs index 82e39001..e90c560a 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/mod.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/mod.rs @@ -41,7 +41,7 @@ pub struct CompilationUnit { } /// A structure that represents a single notification parsed from clang-tidy's stdout. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct TidyNotification { /// The file's path and name (supposedly relative to the repository root folder). pub filename: String, @@ -388,9 +388,26 @@ fn apply_fixes<'a>( ) -> (Vec, TidyFixes) { let mut tidy_fixes = TidyFixes::default(); for diag in &fixes.diagnostics { + let first_other = tidy_fixes.others.len(); let fixed_lines = collect_diagnostic_fixes(diag, paths, content, &mut tidy_fixes, logs); - if !fixed_lines.is_empty() { - record_fixed_lines(diag, paths, content, notes, fixed_lines); + let other_fixes = &mut tidy_fixes.others[first_other..]; + if fixed_lines.is_empty() && other_fixes.is_empty() { + continue; + } + let Some(note) = find_note(diag, paths, content, notes) else { + continue; + }; + for line in fixed_lines { + if !note.fixed_lines.contains(&line) { + note.fixed_lines.push(line); + } + } + for fix in other_fixes { + // the fixed lines in the other file are recorded once the fix is applied + fix.note = Some(TidyNotification { + fixed_lines: vec![], + ..note.clone() + }); } } let patched = patch_content(content, &tidy_fixes.own, paths.target, logs); @@ -415,6 +432,7 @@ fn collect_diagnostic_fixes<'a>( path: other, diagnostic: diag.diagnostic_name.clone(), replacement: Replacement::from(replacement), + note: None, }); } else if replacement .offset @@ -440,15 +458,13 @@ fn collect_diagnostic_fixes<'a>( fixed_lines } -/// Record the `fixed_lines` of a `diag` in the notification (parsed from stdout) -/// that matches it. -fn record_fixed_lines<'a>( +/// Find the notification (parsed from stdout) that matches a `diag`. +fn find_note<'a, 'n>( diag: &'a ExportedDiagnostic, paths: &mut FixPaths<'a>, content: &[u8], - notes: &mut [TidyNotification], - fixed_lines: Vec, -) { + notes: &'n mut [TidyNotification], +) -> Option<&'n mut TidyNotification> { let msg = &diag.diagnostic_message; let (diag_path, diag_line) = match paths.other_file(&diag.build_directory, &msg.file_path) { None => ( @@ -462,17 +478,11 @@ fn record_fixed_lines<'a>( (other, line) } }; - if let Some(note) = notes.iter_mut().find(|note| { + notes.iter_mut().find(|note| { note.diagnostic == diag.diagnostic_name && Some(note.line) == diag_line && diag_path.ends_with(¬e.filename) - }) { - for line in fixed_lines { - if !note.fixed_lines.contains(&line) { - note.fixed_lines.push(line); - } - } - } + }) } /// Apply the given `replacements` onto a file's original `content`. diff --git a/cpp-linter/src/clang_tools/clang_tidy/other_files.rs b/cpp-linter/src/clang_tools/clang_tidy/other_files.rs index fa1707b4..ddd589d0 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/other_files.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/other_files.rs @@ -15,7 +15,9 @@ use std::{ use clang_tools_manager::utils::normalize_path; -use super::{ExportedReplacement, offset_to_line, patch_content, write_cache}; +use super::{ + ExportedReplacement, TidyAdvice, TidyNotification, offset_to_line, patch_content, write_cache, +}; use crate::{cli::ClangParams, common_fs::FileObj, error::ClangCaptureError}; /// A text replacement in a file's original content. @@ -48,6 +50,13 @@ pub struct OtherFileFix { pub diagnostic: String, /// The replacement in the other file's original content. pub replacement: Replacement, + /// The notification (parsed from clang-tidy's stdout) about the diagnostic, + /// if one was found. + /// + /// Once the fix is applied, a copy (with the fixed line in the other file) is + /// added to the other file's [`TidyAdvice`], so suggestions in a PR review can + /// explain the fix. + pub note: Option, } /// The fixes that clang-tidy suggested while analyzing a file. @@ -140,12 +149,14 @@ impl<'a> AnalyzedFiles<'a> { } /// Decide whether the `fix` may be applied to the analyzed file at `index`. + /// + /// Returns the line where the fix starts if it may be applied. fn check( &mut self, index: usize, fix: &OtherFileFix, clang_params: &ClangParams, - ) -> Result, ClangCaptureError> { + ) -> Result, ClangCaptureError> { let content_len = self.content(index, clang_params)?.len(); let replacement = &fix.replacement; let Some(end) = replacement @@ -153,12 +164,14 @@ impl<'a> AnalyzedFiles<'a> { .checked_add(replacement.length) .filter(|end| *end <= content_len) else { - return Ok(Some(Rejection::OutOfBounds)); + return Ok(Err(Rejection::OutOfBounds)); }; let content = self.content(index, clang_params)?; + // `end` is exclusive, so use the last replaced byte (if any) for the end line + let last = if replacement.length > 0 { end - 1 } else { end }; let (start_line, end_line) = ( offset_to_line(content, replacement.offset), - offset_to_line(content, end), + offset_to_line(content, last), ); let file = self.files[index] .lock() @@ -168,7 +181,7 @@ impl<'a> AnalyzedFiles<'a> { .as_ref() .is_some_and(|f| !f.is_qualified(&file.name)) { - return Ok(Some(Rejection::IgnoredByTidy)); + return Ok(Err(Rejection::IgnoredByTidy)); } let ranges = file.get_ranges(&clang_params.lines_changed_only); if !ranges.is_empty() @@ -176,9 +189,9 @@ impl<'a> AnalyzedFiles<'a> { .iter() .any(|r| r.contains(&start_line) && r.contains(&end_line)) { - return Ok(Some(Rejection::OutsideChangedLines)); + return Ok(Err(Rejection::OutsideChangedLines)); } - Ok(None) + Ok(Ok(start_line)) } } @@ -209,7 +222,7 @@ pub fn apply_other_file_fixes( .map_err(ClangCaptureError::UnknownWorkingDirectory)?; let mut analyzed = AnalyzedFiles::new(files, &working_dir)?; // the replacements to apply, keyed by the index of the file they apply to - let mut pending: BTreeMap> = BTreeMap::new(); + let mut pending: BTreeMap = BTreeMap::new(); for (source, fixes) in tidy_fixes { let source_name = file_name(&files[*source])?; for fix in &fixes.others { @@ -219,20 +232,17 @@ pub fn apply_other_file_fixes( .strip_prefix(&working_dir) .unwrap_or(&fix.path) .to_string_lossy(); - let rejection = match analyzed.find(&fix.path) { - None => Some(Rejection::NotAnalyzed), + let verdict = match analyzed.find(&fix.path) { + None => Err(Rejection::NotAnalyzed), Some(index) => { - let rejection = analyzed.check(index, fix, clang_params)?; - if rejection.is_none() { - pending - .entry(index) - .or_default() - .push(fix.replacement.clone()); + let verdict = analyzed.check(index, fix, clang_params)?; + if let Ok(line) = verdict { + pending.entry(index).or_default().add(fix, line); } - rejection + verdict } }; - logs[*source].push(match rejection { + logs[*source].push(match verdict.err() { None => ( log::Level::Info, format!( @@ -251,7 +261,14 @@ pub fn apply_other_file_fixes( }); } } - for (index, mut replacements) in pending { + for ( + index, + Pending { + mut replacements, + notes, + }, + ) in pending + { if let Some((_, fixes)) = tidy_fixes.iter().find(|(i, _)| *i == index) { replacements.extend(fixes.own.iter().cloned()); } @@ -261,10 +278,68 @@ pub fn apply_other_file_fixes( .map_err(|_| ClangCaptureError::MutexPoisoned)?; let patched = patch_content(content, &replacements, &file.name, &mut logs[index]); file.patched_path = Some(write_cache(clang_params, &file.name, &patched)?); + let name = file.name.to_string_lossy().replace('\\', "/"); + let advice = file + .tidy_advice + .get_or_insert_with(|| TidyAdvice { notes: vec![] }); + for note in notes { + merge_note(&mut advice.notes, note, &name); + } } Ok(()) } +/// The accepted fixes for one analyzed file. +#[derive(Default)] +struct Pending { + replacements: Vec, + /// Copies of the notes about the diagnostics that suggested the fixes, + /// with the fixed lines in the analyzed file. + notes: Vec, +} + +impl Pending { + fn add(&mut self, fix: &OtherFileFix, line: u32) { + self.replacements.push(fix.replacement.clone()); + let Some(note) = &fix.note else { + return; + }; + let note = TidyNotification { + fixed_lines: vec![line], + ..note.clone() + }; + if let Some(existing) = self.notes.iter_mut().find(|n| is_same_note(n, ¬e)) { + if !existing.fixed_lines.contains(&line) { + existing.fixed_lines.push(line); + } + } else { + self.notes.push(note); + } + } +} + +fn is_same_note(a: &TidyNotification, b: &TidyNotification) -> bool { + a.filename == b.filename && a.line == b.line && a.diagnostic == b.diagnostic +} + +/// Add the fixed lines of a `note` to the `notes` of the analyzed file named `file_name`. +/// +/// A note about another file (eg. the source file that suggested the fix) is added, +/// so suggestions in a PR review can explain the fix; notes about other files are not +/// counted as this file's diagnostics. A note about this file is only merged into the +/// same note found by analyzing this file, so it is not counted twice. +fn merge_note(notes: &mut Vec, note: TidyNotification, file_name: &str) { + if let Some(existing) = notes.iter_mut().find(|n| is_same_note(n, ¬e)) { + for line in note.fixed_lines { + if !existing.fixed_lines.contains(&line) { + existing.fixed_lines.push(line); + } + } + } else if note.filename != file_name { + notes.push(note); + } +} + fn file_name(file: &Arc>) -> Result { Ok(file .lock() diff --git a/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs b/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs index c0da5cc1..535c1fd3 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs @@ -10,9 +10,12 @@ use std::{ use clang_tools_manager::RequestedVersion; use git_bot_feedback::FileFilter; -use super::{OtherFileFix, Replacement, TidyFixes, apply_other_file_fixes}; +use super::{OtherFileFix, Pending, Replacement, TidyFixes, apply_other_file_fixes, merge_note}; use crate::{ - clang_tools::{ClangTool, analyze_files}, + clang_tools::{ + ClangTool, analyze_files, + clang_tidy::{TidyNotification, tally_tidy_advice}, + }, cli::{ClangParams, LinesChangedOnly}, common_fs::FileObj, }; @@ -56,9 +59,33 @@ fn header_fix(workspace: &Path) -> OtherFileFix { length: 0, text: "const ".to_string(), }, + note: Some(source_note()), } } +/// The note about the diagnostic (in [`SOURCE`]) that suggests [`header_fix()`]. +fn source_note() -> TidyNotification { + TidyNotification { + filename: "fix.cpp".to_string(), + line: 2, + cols: 10, + severity: "warning".to_string(), + rationale: "pointer parameter 'p' can be pointer to const".to_string(), + diagnostic: "readability-non-const-parameter".to_string(), + suggestion: vec![], + fixed_lines: vec![], + } +} + +/// The notes (about other files) in the header's tidy advice. +fn header_notes(file: &Arc>) -> Vec { + let file = file.lock().unwrap(); + file.tidy_advice + .as_ref() + .map(|advice| advice.notes.clone()) + .unwrap_or_default() +} + fn header_fixes(workspace: &Path) -> TidyFixes { TidyFixes { own: vec![], @@ -116,6 +143,25 @@ async fn fix_analyzed_header() { }), "{logs:#?}" ); + + // the header's advice explains the fix, without counting it as the header's diagnostic + let notes = header_notes(&files[1]); + let note = notes + .iter() + .find(|n| n.diagnostic == "readability-non-const-parameter") + .unwrap(); + assert_eq!(note.filename, "fix.cpp"); + assert_eq!(note.fixed_lines, vec![2]); + let help = files[1] + .lock() + .unwrap() + .tidy_advice + .as_ref() + .unwrap() + .get_suggestion_help(2, 2); + assert!(help.starts_with("### clang-tidy diagnostic(s)"), "{help}"); + assert!(help.contains(¬e.rationale), "{help}"); + assert_eq!(tally_tidy_advice(&files).unwrap(), 1); } #[test] @@ -264,6 +310,63 @@ fn merge_with_own_fixes() { } // duplicate fixes do not cause warnings assert!(logs[1].is_empty(), "{:#?}", logs[1]); + // the same note is recorded once, with the fixed line in the header + let notes = header_notes(&files[1]); + assert_eq!(notes.len(), 1, "{notes:#?}"); + assert_eq!(notes[0].fixed_lines, vec![2]); +} + +/// A fix that removes a whole changed line (including its line break) +/// does not reach into the next line. +#[test] +#[allow(clippy::single_range_in_vec_init)] +fn remove_changed_line() { + let tmp = workspace(); + let clang_params = ClangParams { + repo_root: tmp.path().to_path_buf(), + lines_changed_only: LinesChangedOnly::On, + ..Default::default() + }; + let files = as_files(vec![ + FileObj::from(PathBuf::from("fix.cpp"), vec![2], vec![2..=2]), + // only the first line of the header was changed + FileObj::from(PathBuf::from("fix.h"), vec![1], vec![1..=1]), + ]); + let mut fix = header_fix(tmp.path()); + fix.replacement = Replacement { + offset: 0, + length: "#pragma once\n".len(), + text: String::new(), + }; + let fixes = TidyFixes { + own: vec![], + others: vec![fix], + }; + let mut logs = vec![vec![], vec![]]; + apply_other_file_fixes(&files, &[(0, fixes)], &clang_params, &mut logs).unwrap(); + assert_eq!(patched(&files[1]).unwrap(), "void set(int *p);\n"); + assert_eq!(header_notes(&files[1])[0].fixed_lines, vec![1]); +} + +/// A note about the analyzed file itself is only merged into the file's own note, +/// so it is never counted twice. +#[test] +fn merge_note_about_same_file() { + let mut note = source_note(); + note.fixed_lines = vec![3]; + let mut notes = vec![]; + merge_note(&mut notes, note.clone(), "fix.cpp"); + assert!(notes.is_empty()); + + let own = TidyNotification { + fixed_lines: vec![2], + ..source_note() + }; + let mut notes = vec![own]; + merge_note(&mut notes, note.clone(), "fix.cpp"); + merge_note(&mut notes, note, "fix.cpp"); + assert_eq!(notes.len(), 1); + assert_eq!(notes[0].fixed_lines, vec![2, 3]); } #[test] @@ -313,3 +416,22 @@ fn analyzed_file_missing() { "{err:?}" ); } + +/// A fix whose diagnostic has no matching note is applied without a note. +#[test] +fn pending_fix_without_note() { + let tmp = workspace(); + let mut fix = header_fix(tmp.path()); + fix.note = None; + let mut pending = Pending::default(); + pending.add(&fix, 2); + assert_eq!(pending.replacements, vec![fix.replacement]); + assert!(pending.notes.is_empty()); + + // fixes for the same diagnostic on different lines share a note + let fix = header_fix(tmp.path()); + pending.add(&fix, 2); + pending.add(&fix, 3); + assert_eq!(pending.notes.len(), 1); + assert_eq!(pending.notes[0].fixed_lines, vec![2, 3]); +} diff --git a/cpp-linter/src/clang_tools/clang_tidy/test.rs b/cpp-linter/src/clang_tools/clang_tidy/test.rs index ee76bbeb..d5800090 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/test.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/test.rs @@ -525,6 +525,11 @@ fn apply_exported_fixes() { length: 4, text: "nope".to_string(), }, + // the matching note, to explain the fix once it is applied + note: Some(TidyNotification { + fixed_lines: vec![], + ..notes[0].clone() + }), }] ); } From aa6606e2b1b71e20dd7d8cdc250114d221bc734f Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Sun, 18 Oct 2026 20:34:56 +0000 Subject: [PATCH 5/6] refactor: address review on cross-file fixes - move other_files.rs to other_files/mod.rs - rename the source file's index variable to source_index - explain out-of-bounds fixes with their byte offsets - name --lines-changed-only in the rejection reason --- .../{other_files.rs => other_files/mod.rs} | 37 ++++++++++++++----- .../clang_tidy/other_files/test.rs | 14 ++++++- 2 files changed, 39 insertions(+), 12 deletions(-) rename cpp-linter/src/clang_tools/clang_tidy/{other_files.rs => other_files/mod.rs} (91%) diff --git a/cpp-linter/src/clang_tools/clang_tidy/other_files.rs b/cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs similarity index 91% rename from cpp-linter/src/clang_tools/clang_tidy/other_files.rs rename to cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs index ddd589d0..c4e8ec56 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/other_files.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs @@ -72,17 +72,30 @@ pub struct TidyFixes { enum Rejection { NotAnalyzed, IgnoredByTidy, - OutOfBounds, + /// The fix's byte offsets do not fit in the file. + OutOfBounds { + /// The byte offset where the fix starts. + start: usize, + /// The byte offset where the fix ends (exclusive). + end: usize, + /// The file's size (in bytes). + size: usize, + }, OutsideChangedLines, } impl Rejection { - fn reason(&self) -> &'static str { + fn reason(&self) -> String { match self { - Self::NotAnalyzed => "it is not one of the analyzed files", - Self::IgnoredByTidy => "it is ignored by `--ignore-tidy`", - Self::OutOfBounds => "the fix is out of the file's bounds", - Self::OutsideChangedLines => "the fix is outside the file's changed lines", + Self::NotAnalyzed => "it is not one of the analyzed files".to_string(), + Self::IgnoredByTidy => "it is ignored by `--ignore-tidy`".to_string(), + Self::OutOfBounds { start, end, size } => format!( + "the fix seems malformed and is out of the file's bounds \ + (bytes {start} to {end} of a {size} byte file)" + ), + Self::OutsideChangedLines => { + "the fix clashes with the `--lines-changed-only` constraint".to_string() + } } } } @@ -164,7 +177,11 @@ impl<'a> AnalyzedFiles<'a> { .checked_add(replacement.length) .filter(|end| *end <= content_len) else { - return Ok(Err(Rejection::OutOfBounds)); + return Ok(Err(Rejection::OutOfBounds { + start: replacement.offset, + end: replacement.offset.saturating_add(replacement.length), + size: content_len, + })); }; let content = self.content(index, clang_params)?; // `end` is exclusive, so use the last replaced byte (if any) for the end line @@ -223,8 +240,8 @@ pub fn apply_other_file_fixes( let mut analyzed = AnalyzedFiles::new(files, &working_dir)?; // the replacements to apply, keyed by the index of the file they apply to let mut pending: BTreeMap = BTreeMap::new(); - for (source, fixes) in tidy_fixes { - let source_name = file_name(&files[*source])?; + for (source_index, fixes) in tidy_fixes { + let source_name = file_name(&files[*source_index])?; for fix in &fixes.others { // show the path relative to the repository root, if possible let other = fix @@ -242,7 +259,7 @@ pub fn apply_other_file_fixes( verdict } }; - logs[*source].push(match verdict.err() { + logs[*source_index].push(match verdict.err() { None => ( log::Level::Info, format!( diff --git a/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs b/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs index 535c1fd3..0d4740c7 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs @@ -239,7 +239,11 @@ fn outside_changed_lines() { assert!(patched(&files[1]).is_none()); let messages = messages(&logs[0]); assert_eq!(messages.len(), 1); - assert!(messages[0].ends_with("to fix.h because the fix is outside the file's changed lines")); + assert!( + messages[0].ends_with( + "to fix.h because the fix clashes with the `--lines-changed-only` constraint" + ) + ); } #[test] @@ -265,7 +269,13 @@ fn out_of_bounds() { assert!(patched(&files[1]).is_none()); let messages = messages(&logs[0]); assert_eq!(messages.len(), 1); - assert!(messages[0].ends_with("to fix.h because the fix is out of the file's bounds")); + assert!(messages[0].ends_with(&format!( + "to fix.h because the fix seems malformed and is out of the file's bounds \ + (bytes {} to {} of a {} byte file)", + HEADER.len(), + HEADER.len() + 1, + HEADER.len(), + ))); } /// The same fix for the header is suggested by two analyzed sources, From c79344027280f5aed5610ab1e087b4efcc468e6b Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Sun, 18 Oct 2026 20:41:33 +0000 Subject: [PATCH 6/6] fix: reject cross-file fixes that overlap other fixes A fix that overlapped the target's own fix or another accepted fix was logged as applied and given a fixed line, but then skipped when patching. Such fixes are now rejected (and logged as such) before patching. --- .../clang_tools/clang_tidy/other_files/mod.rs | 67 ++++++++++--- .../clang_tidy/other_files/test.rs | 97 ++++++++++++++++++- 2 files changed, 144 insertions(+), 20 deletions(-) diff --git a/cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs b/cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs index c4e8ec56..8bd3a17d 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs @@ -82,6 +82,8 @@ enum Rejection { size: usize, }, OutsideChangedLines, + /// The fix overlaps a different fix that is applied to the same file. + Overlaps, } impl Rejection { @@ -96,6 +98,7 @@ impl Rejection { Self::OutsideChangedLines => { "the fix clashes with the `--lines-changed-only` constraint".to_string() } + Self::Overlaps => "the fix overlaps another fix for the same file".to_string(), } } } @@ -217,8 +220,9 @@ impl<'a> AnalyzedFiles<'a> { /// /// `tidy_fixes` holds the fixes found while analyzing the file at the paired index of /// `files`. A fix for another file is only applied if that file is also analyzed, -/// is not ignored by `--ignore-tidy`, and the fix is within the file's changed lines -/// (according to `--lines-changed-only`). +/// is not ignored by `--ignore-tidy`, the fix is within the file's changed lines +/// (according to `--lines-changed-only`), and it does not overlap the file's own fixes +/// or a different fix accepted before. /// /// Messages about each fix are added to the `logs` of the analyzed file whose /// diagnostic suggested it (at the paired index of `files`). @@ -251,13 +255,12 @@ pub fn apply_other_file_fixes( .to_string_lossy(); let verdict = match analyzed.find(&fix.path) { None => Err(Rejection::NotAnalyzed), - Some(index) => { - let verdict = analyzed.check(index, fix, clang_params)?; - if let Ok(line) = verdict { - pending.entry(index).or_default().add(fix, line); - } - verdict - } + Some(index) => analyzed.check(index, fix, clang_params)?.and_then(|line| { + pending + .entry(index) + .or_insert_with(|| Pending::new(own_fixes(tidy_fixes, index))) + .add(fix, line) + }), }; logs[*source_index].push(match verdict.err() { None => ( @@ -281,14 +284,11 @@ pub fn apply_other_file_fixes( for ( index, Pending { - mut replacements, + replacements, notes, }, ) in pending { - if let Some((_, fixes)) = tidy_fixes.iter().find(|(i, _)| *i == index) { - replacements.extend(fixes.own.iter().cloned()); - } let content = analyzed.content(index, clang_params)?; let mut file = files[index] .lock() @@ -306,9 +306,27 @@ pub fn apply_other_file_fixes( Ok(()) } +/// Get the fixes that clang-tidy suggested for the analyzed file at `index` +/// while analyzing that file. +fn own_fixes(tidy_fixes: &[(usize, TidyFixes)], index: usize) -> &[Replacement] { + tidy_fixes + .iter() + .find(|(i, _)| *i == index) + .map(|(_, fixes)| fixes.own.as_slice()) + .unwrap_or_default() +} + +/// Do two replacements overlap, so only one of them can be applied? +/// +/// Identical replacements do not overlap, because they are applied once. +fn overlaps(a: &Replacement, b: &Replacement) -> bool { + let (first, second) = if a <= b { (a, b) } else { (b, a) }; + first != second && second.offset < first.offset + first.length +} + /// The accepted fixes for one analyzed file. -#[derive(Default)] struct Pending { + /// The file's own fixes, followed by the accepted fixes from other files. replacements: Vec, /// Copies of the notes about the diagnostics that suggested the fixes, /// with the fixed lines in the analyzed file. @@ -316,10 +334,26 @@ struct Pending { } impl Pending { - fn add(&mut self, fix: &OtherFileFix, line: u32) { + fn new(own: &[Replacement]) -> Self { + Self { + replacements: own.to_vec(), + notes: vec![], + } + } + + /// Accept a `fix` that starts at `line`, unless it overlaps a fix accepted before + /// (or the file's own fixes, which take precedence). + fn add(&mut self, fix: &OtherFileFix, line: u32) -> Result<(), Rejection> { + if self + .replacements + .iter() + .any(|r| overlaps(r, &fix.replacement)) + { + return Err(Rejection::Overlaps); + } self.replacements.push(fix.replacement.clone()); let Some(note) = &fix.note else { - return; + return Ok(()); }; let note = TidyNotification { fixed_lines: vec![line], @@ -332,6 +366,7 @@ impl Pending { } else { self.notes.push(note); } + Ok(()) } } diff --git a/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs b/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs index 0d4740c7..287a322c 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs @@ -433,15 +433,104 @@ fn pending_fix_without_note() { let tmp = workspace(); let mut fix = header_fix(tmp.path()); fix.note = None; - let mut pending = Pending::default(); - pending.add(&fix, 2); + let mut pending = Pending::new(&[]); + assert!(pending.add(&fix, 2).is_ok()); assert_eq!(pending.replacements, vec![fix.replacement]); assert!(pending.notes.is_empty()); // fixes for the same diagnostic on different lines share a note let fix = header_fix(tmp.path()); - pending.add(&fix, 2); - pending.add(&fix, 3); + assert!(pending.add(&fix, 2).is_ok()); + assert!(pending.add(&fix, 3).is_ok()); assert_eq!(pending.notes.len(), 1); assert_eq!(pending.notes[0].fixed_lines, vec![2, 3]); } + +/// A fix that replaces `int` in the header's declaration. +fn replace_int(workspace: &Path, text: &str) -> OtherFileFix { + OtherFileFix { + replacement: Replacement { + offset: HEADER_FIX_OFFSET, + length: 3, + text: text.to_string(), + }, + ..header_fix(workspace) + } +} + +/// Of two different fixes for the same text in another file, only the first is applied. +#[test] +fn overlapping_fixes() { + let tmp = workspace(); + fs::write(tmp.path().join("other.cpp"), SOURCE).unwrap(); + let clang_params = ClangParams { + repo_root: tmp.path().to_path_buf(), + ..Default::default() + }; + let files = as_files(vec![ + FileObj::new(PathBuf::from("fix.cpp")), + FileObj::new(PathBuf::from("fix.h")), + FileObj::new(PathBuf::from("other.cpp")), + ]); + let fixes_of = |text: &str| TidyFixes { + own: vec![], + others: vec![replace_int(tmp.path(), text)], + }; + let tidy_fixes = [(0, fixes_of("long")), (2, fixes_of("char"))]; + let mut logs = vec![vec![], vec![], vec![]]; + apply_other_file_fixes(&files, &tidy_fixes, &clang_params, &mut logs).unwrap(); + assert_eq!( + patched(&files[1]).unwrap(), + "#pragma once\nvoid set(long *p);\n" + ); + assert!(messages(&logs[0])[0].starts_with("Applying the fix")); + let messages = messages(&logs[2]); + assert_eq!(messages.len(), 1); + assert!( + messages[0].ends_with("to fix.h because the fix overlaps another fix for the same file") + ); + // the overlapping fix does not touch the patch, so it gets no fixed line + let notes = header_notes(&files[1]); + assert_eq!(notes.len(), 1, "{notes:#?}"); + assert_eq!(notes[0].fixed_lines, vec![2]); + // the patch is not changed behind the scenes either + assert!(logs[1].is_empty(), "{:#?}", logs[1]); +} + +/// A fix from another file that overlaps the file's own fix is not applied. +#[test] +fn overlaps_own_fix() { + let tmp = workspace(); + let clang_params = ClangParams { + repo_root: tmp.path().to_path_buf(), + ..Default::default() + }; + let files = as_files(vec![ + FileObj::new(PathBuf::from("fix.cpp")), + FileObj::new(PathBuf::from("fix.h")), + ]); + let own = TidyFixes { + own: vec![replace_int(tmp.path(), "long").replacement], + others: vec![], + }; + let mut fix = header_fix(tmp.path()); + // insert inside the replaced `int` + fix.replacement.offset += 1; + let others = TidyFixes { + own: vec![], + others: vec![fix], + }; + let mut logs = vec![vec![], vec![]]; + apply_other_file_fixes(&files, &[(0, others), (1, own)], &clang_params, &mut logs).unwrap(); + // only the header's own fix is applied + assert_eq!( + patched(&files[1]).unwrap(), + "#pragma once\nvoid set(long *p);\n" + ); + let messages = messages(&logs[0]); + assert_eq!(messages.len(), 1); + assert!( + messages[0].ends_with("to fix.h because the fix overlaps another fix for the same file") + ); + assert!(header_notes(&files[1]).is_empty()); +}