diff --git a/cpp-linter/src/clang_tools/clang_tidy/mod.rs b/cpp-linter/src/clang_tools/clang_tidy/mod.rs index 3bac4052..e90c560a 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 @@ -38,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, @@ -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,120 @@ 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 first_other = tidy_fixes.others.len(); + let fixed_lines = collect_diagnostic_fixes(diag, paths, content, &mut tidy_fixes, logs); + 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 other in other_files { + 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); + (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), + note: None, + }); + } 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; - } - // 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); - } - } - } } + fixed_lines +} +/// 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: &'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 => ( + 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) + } + }; + notes.iter_mut().find(|note| { + note.diagnostic == diag.diagnostic_name + && Some(note.line) == diag_line + && diag_path.ends_with(¬e.filename) + }) +} + +/// 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 +508,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 +632,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 +706,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/mod.rs b/cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs new file mode 100644 index 00000000..8bd3a17d --- /dev/null +++ b/cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs @@ -0,0 +1,405 @@ +//! 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, 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. +#[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 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. +#[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, + /// 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, + /// The fix overlaps a different fix that is applied to the same file. + Overlaps, +} + +impl Rejection { + fn reason(&self) -> String { + match self { + 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() + } + Self::Overlaps => "the fix overlaps another fix for the same file".to_string(), + } + } +} + +/// 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`. + /// + /// 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> { + 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(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 + 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, last), + ); + 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(Err(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(Err(Rejection::OutsideChangedLines)); + } + Ok(Ok(start_line)) + } +} + +/// 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`, 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`). +/// +/// 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_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 + .path + .strip_prefix(&working_dir) + .unwrap_or(&fix.path) + .to_string_lossy(); + let verdict = match analyzed.find(&fix.path) { + None => Err(Rejection::NotAnalyzed), + 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 => ( + 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, + Pending { + replacements, + notes, + }, + ) in pending + { + 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)?); + 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(()) +} + +/// 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. +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. + notes: Vec, +} + +impl Pending { + 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 Ok(()); + }; + 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); + } + Ok(()) + } +} + +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() + .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..287a322c --- /dev/null +++ b/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs @@ -0,0 +1,536 @@ +#![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, Pending, Replacement, TidyFixes, apply_other_file_fixes, merge_note}; +use crate::{ + clang_tools::{ + ClangTool, analyze_files, + clang_tidy::{TidyNotification, tally_tidy_advice}, + }, + 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(), + }, + 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![], + 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:#?}" + ); + + // 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] +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] +#[allow(clippy::single_range_in_vec_init)] +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 clashes with the `--lines-changed-only` constraint" + ) + ); +} + +#[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(&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, +/// 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]); + // 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] +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()); +} + +/// 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:?}" + ); +} + +/// 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::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()); + 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()); +} diff --git a/cpp-linter/src/clang_tools/clang_tidy/test.rs b/cpp-linter/src/clang_tools/clang_tidy/test.rs index a07fdfd8..d5800090 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,31 @@ 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(), + }, + // the matching note, to explain the fix once it is applied + note: Some(TidyNotification { + fixed_lines: vec![], + ..notes[0].clone() + }), + }] + ); } #[test] @@ -592,3 +609,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]); +} 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;