feat: report clang-tidy fixes that could not be applied to other files

cpp-linter/src/clang_tools/clang_tidy/mod.rs

@@ -70,6 +70,32 @@ pub struct TidyNotification {
 
     /// The list of line numbers (in the analyzed file) that clang-tidy suggested fixes for.
     pub fixed_lines: Vec<u32>,
+
+    /// The parts of the diagnostic's fix that were not applied to other files
+    /// (eg. a header that is not one of the analyzed files).
+    ///
+    /// Users have to apply these parts themselves, so they are mentioned in the
+    /// thread comment and PR review.
+    pub skipped_fixes: Vec<SkippedFix>,
+}
+
+/// A part of a clang-tidy fix that was not applied to another file.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct SkippedFix {
+    /// The other file's path (relative to the repository root, if possible).
+    pub file: String,
+    /// Why the fix was not applied to the other file.
+    pub reason: String,
+}
+
+impl SkippedFix {
+    /// Describe the skipped fix in a sentence (in markdown).
+    pub fn describe(&self) -> String {
+        format!(
+            "Part of this fix was not applied to `{}` because {}.",
+            self.file, self.reason
+        )
+    }
 }
 
 /// The URL to clang-tidy's documentation.
@@ -135,6 +161,9 @@ impl TidyAdvice {
                         note.rationale,
                         note.diagnostic_link()
                     ));
+                    for skipped in &note.skipped_fixes {
+                        diagnostics.push(format!("  - :warning: {}\n", skipped.describe()));
+                    }
                 }
             }
         }
@@ -232,6 +261,7 @@ fn parse_tidy_output(
                     diagnostic: String::from(&captured[6]),
                     suggestion: Vec::new(),
                     fixed_lines: Vec::new(),
+                    skipped_fixes: vec![],
                 });
             }
         } else if let Some(note) = &mut notification {

cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs

@@ -16,7 +16,8 @@ use std::{
 use clang_tools_manager::utils::normalize_path;
 
 use super::{
-    ExportedReplacement, TidyAdvice, TidyNotification, offset_to_line, patch_content, write_cache,
+    ExportedReplacement, SkippedFix, TidyAdvice, TidyNotification, offset_to_line, patch_content,
+    write_cache,
 };
 use crate::{cli::ClangParams, common_fs::FileObj, error::ClangCaptureError};
 
@@ -262,23 +263,25 @@ pub fn apply_other_file_fixes(
                         .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,
-                    ),
+            let message = match verdict {
+                Ok(()) => 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(),
-                    ),
-                ),
-            });
+                Err(rejection) => {
+                    let skipped = SkippedFix {
+                        file: other.replace('\\', "/"),
+                        reason: rejection.reason(),
+                    };
+                    let message = format!(
+                        "Not applying the fix for {} (reported in {source_name}) to {} because {}",
+                        fix.diagnostic, skipped.file, skipped.reason,
+                    );
+                    record_skipped_fix(&files[*source_index], fix, skipped)?;
+                    message
+                }
+            };
+            logs[*source_index].push((log::Level::Info, message));
         }
     }
     for (
@@ -324,6 +327,33 @@ fn overlaps(a: &Replacement, b: &Replacement) -> bool {
     first != second && second.offset < first.offset + first.length
 }
 
+/// Record a `skipped` part of a `fix` on the note about the fix's diagnostic in the
+/// analyzed `source` file, so the thread comment and PR review can mention it.
+///
+/// Nothing is recorded if no note about the diagnostic was found.
+fn record_skipped_fix(
+    source: &Arc<Mutex<FileObj>>,
+    fix: &OtherFileFix,
+    skipped: SkippedFix,
+) -> Result<(), ClangCaptureError> {
+    let Some(fix_note) = &fix.note else {
+        return Ok(());
+    };
+    let mut source = source
+        .lock()
+        .map_err(|_| ClangCaptureError::MutexPoisoned)?;
+    let note = source
+        .tidy_advice
+        .as_mut()
+        .and_then(|advice| advice.notes.iter_mut().find(|n| is_same_note(n, fix_note)));
+    if let Some(note) = note
+        && !note.skipped_fixes.contains(&skipped)
+    {
+        note.skipped_fixes.push(skipped);
+    }
+    Ok(())
+}
+
 /// The accepted fixes for one analyzed file.
 struct Pending {
     /// The file's own fixes, followed by the accepted fixes from other files.
@@ -357,6 +387,7 @@ impl Pending {
         };
         let note = TidyNotification {
             fixed_lines: vec![line],
+            skipped_fixes: vec![],
             ..note.clone()
         };
         if let Some(existing) = self.notes.iter_mut().find(|n| is_same_note(n, &note)) {

cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs

@@ -14,7 +14,7 @@ use super::{OtherFileFix, Pending, Replacement, TidyFixes, apply_other_file_fixe
 use crate::{
     clang_tools::{
         ClangTool, analyze_files,
-        clang_tidy::{TidyNotification, tally_tidy_advice},
+        clang_tidy::{SkippedFix, TidyAdvice, TidyNotification, tally_tidy_advice},
     },
     cli::{ClangParams, LinesChangedOnly},
     common_fs::FileObj,
@@ -74,6 +74,7 @@ fn source_note() -> TidyNotification {
         diagnostic: "readability-non-const-parameter".to_string(),
         suggestion: vec![],
         fixed_lines: vec![],
+        skipped_fixes: vec![],
     }
 }
 
@@ -171,20 +172,39 @@ fn not_analyzed() {
         repo_root: tmp.path().to_path_buf(),
         ..Default::default()
     };
-    let files = as_files(vec![FileObj::new(PathBuf::from("fix.cpp"))]);
+    let mut source = FileObj::new(PathBuf::from("fix.cpp"));
+    source.tidy_advice = Some(TidyAdvice {
+        notes: vec![source_note()],
+    });
+    let files = as_files(vec![source]);
+    let mut without_note = header_fix(tmp.path());
+    without_note.note = None;
+    let fixes = TidyFixes {
+        own: vec![],
+        // the same fix twice, and a fix whose diagnostic has no note
+        others: vec![header_fix(tmp.path()), header_fix(tmp.path()), without_note],
+    };
     let mut logs = vec![vec![]];
-    apply_other_file_fixes(
-        &files,
-        &[(0, header_fixes(tmp.path()))],
-        &clang_params,
-        &mut logs,
-    )
-    .unwrap();
+    apply_other_file_fixes(&files, &[(0, fixes)], &clang_params, &mut logs).unwrap();
     assert!(patched(&files[0]).is_none());
     let messages = messages(&logs[0]);
-    assert_eq!(messages.len(), 1);
+    assert_eq!(messages.len(), 3);
     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"));
+    // the skipped fix is recorded once on the diagnostic's note
+    let file = files[0].lock().unwrap();
+    let note = &file.tidy_advice.as_ref().unwrap().notes[0];
+    assert_eq!(
+        note.skipped_fixes,
+        vec![SkippedFix {
+            file: "fix.h".to_string(),
+            reason: "it is not one of the analyzed files".to_string(),
+        }]
+    );
+    assert_eq!(
+        note.skipped_fixes[0].describe(),
+        "Part of this fix was not applied to `fix.h` because it is not one of the analyzed files."
+    );
 }
 
 #[test]

cpp-linter/src/clang_tools/clang_tidy/test.rs

@@ -32,6 +32,7 @@ fn clang_diagnostic_link() {
         diagnostic: String::from("clang-diagnostic-error"),
         suggestion: vec![],
         fixed_lines: vec![],
+        skipped_fixes: vec![],
     };
     assert_eq!(note.diagnostic_link(), note.diagnostic);
 }
@@ -47,6 +48,7 @@ fn clang_diagnostic_warning_link() {
         diagnostic: String::from("clang-diagnostic-unused-variable"),
         suggestion: vec![],
         fixed_lines: vec![],
+        skipped_fixes: vec![],
     };
     let expected = format!(
         "[{}]({CLANG_DIAGNOSTICS_URL}#w{})",
@@ -66,6 +68,7 @@ fn clang_analyzer_link() {
         diagnostic: String::from("clang-analyzer-core.NullDereference"),
         suggestion: vec![],
         fixed_lines: vec![],
+        skipped_fixes: vec![],
     };
     let expected = format!(
         "[{}](https://clang.llvm.org/extra/clang-tidy/checks/{}/{}.html)",
@@ -86,6 +89,7 @@ fn invalid_diagnostic_link() {
         diagnostic: expected.clone(),
         suggestion: vec![],
         fixed_lines: vec![],
+        skipped_fixes: vec![],
     };
     assert_eq!(note.diagnostic_link(), expected);
 }
@@ -228,6 +232,7 @@ fn dummy_note(filename: &str, fixed_lines: Vec<u32>) -> TidyNotification {
         diagnostic: String::from("modernize-use-auto"),
         suggestion: vec![],
         fixed_lines,
+        skipped_fixes: vec![],
     }
 }
 
@@ -264,6 +269,23 @@ fn suggestion_help_with_and_without_fixes() {
     assert!(help.contains("clang-tidy suggestion"));
 }
 
+#[test]
+fn suggestion_help_with_skipped_fixes() {
+    use super::{SkippedFix, TidyAdvice};
+
+    let mut note = dummy_note("demo.cpp", vec![5]);
+    note.skipped_fixes.push(SkippedFix {
+        file: "demo.h".to_string(),
+        reason: "it is not one of the analyzed files".to_string(),
+    });
+    let advice = TidyAdvice { notes: vec![note] };
+    let help = advice.get_suggestion_help(5, 5);
+    assert!(help.contains(
+        "\n  - :warning: Part of this fix was not applied to `demo.h` \
+        because it is not one of the analyzed files.\n"
+    ));
+}
+
 #[test]
 fn parse_non_utf8_output() {
     let err = parse_tidy_output(&[0xFF, 0xFE], &None, &PathBuf::from(".")).unwrap_err();
@@ -528,6 +550,7 @@ fn apply_exported_fixes() {
             // the matching note, to explain the fix once it is applied
             note: Some(TidyNotification {
                 fixed_lines: vec![],
+                skipped_fixes: vec![],
                 ..notes[0].clone()
             }),
         }]