diff --git a/cpp-linter/src/clang_tools/clang_tidy/mod.rs b/cpp-linter/src/clang_tools/clang_tidy/mod.rs index e90c560a..a245e6e4 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/mod.rs +++ b/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, + + /// 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, +} + +/// 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 ¬e.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 { 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 8bd3a17d..da194c6a 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 @@ -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>, + 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, ¬e)) { 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 287a322c..d25c86b7 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 @@ -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] diff --git a/cpp-linter/src/clang_tools/clang_tidy/test.rs b/cpp-linter/src/clang_tools/clang_tidy/test.rs index d5800090..ebcaf0ac 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/test.rs +++ b/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) -> 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() }), }] diff --git a/cpp-linter/src/clang_tools/mod.rs b/cpp-linter/src/clang_tools/mod.rs index 305f99df..c092e3e3 100644 --- a/cpp-linter/src/clang_tools/mod.rs +++ b/cpp-linter/src/clang_tools/mod.rs @@ -273,8 +273,15 @@ pub struct ReviewComments { /// This includes changes from both clang-tidy and clang-format /// (assembled in that order). pub full_patch: String, + /// Markdown list items about the parts of clang-tidy fixes that were not applied + /// to other files (see [`clang_tidy::TidyNotification::skipped_fixes`]). + pub skipped_fixes: Vec, } +/// The notice added to a review summary instead of a patch that does not fit. +const PATCH_TOO_LARGE: &str = + "\nThe full patch of fixes is too large to include in this summary.\n"; + impl ReviewComments { /// Get a markdown-formatted string that summarizes the given [`ReviewComment`]s. /// @@ -335,6 +342,7 @@ impl ReviewComments { format!("\n{dupes} suggestions were duplicates of previous reviews.\n").as_str(), ); } + self.summarize_skipped_fixes(&mut body); // The `full_patch` includes all suggestions that didn't fit in the diff. // It can also contain suggestions that were duplicates of previous reviews. if !self.full_patch.is_empty() { @@ -354,16 +362,15 @@ impl ReviewComments { log::warn!( "The full patch of fixes is too large to include in the review summary." ); - body.push_str( - "\nThe full patch of fixes is too large to include in this summary.\n", - ); + body.push_str(PATCH_TOO_LARGE); } else { body.push_str(&patch_prefix); body.push_str(self.full_patch.as_str()); body.push_str(patch_suffix); } - } else if total_review_comments == 0 { + } else if total_review_comments == 0 && self.skipped_fixes.is_empty() { // Only congratulate if there was no reused comments + // and no fix parts left for the user to apply log::info!("No concerns to report: LGTM"); body.push_str("\nNo concerns to report. Great job! :tada:\n"); } @@ -371,6 +378,44 @@ impl ReviewComments { body } + /// Append the list of [`Self::skipped_fixes`] to the review summary `body`. + /// + /// Only the list items that fit within the maximum length of a GitHub comment + /// (leaving room for [`USER_OUTREACH`]) are added. + /// The number of items left out is mentioned instead. + fn summarize_skipped_fixes(&self, body: &mut String) { + if self.skipped_fixes.is_empty() { + return; + } + const HEADING: &str = "\nSome clang-tidy fixes also change files that cpp-linter did not change. \ + Please apply these parts yourself:\n\n"; + // Room kept for the line about omitted items. + const OMITTED_RESERVE: usize = 100; + // Also keep room for the notice that replaces a patch that does not fit. + let max_len = (u16::MAX as usize) + .saturating_sub(USER_OUTREACH.len() + OMITTED_RESERVE + PATCH_TOO_LARGE.len()); + if body.len() + HEADING.len() > max_len { + log::warn!("The list of skipped fixes is too large to include in the review summary."); + return; + } + body.push_str(HEADING); + let mut omitted = 0; + for item in &self.skipped_fixes { + if omitted == 0 && body.len() + item.len() < max_len { + body.push_str(item); + body.push('\n'); + } else { + omitted += 1; + } + } + if omitted > 0 { + log::warn!("{omitted} skipped fixes did not fit in the review summary."); + body.push_str(&format!( + "\n{omitted} more were not listed because this summary is too long.\n" + )); + } + } + /// Check if a given comment's [`Suggestion`] is already contained within the existing [`Self::comments`]. pub fn is_comment_in_suggestions(&mut self, comment: &Suggestion) -> bool { for s in &mut self.comments { @@ -474,6 +519,53 @@ mod tests { assert!(review_summary.contains("suggestions were duplicates of previous reviews")); } + #[test] + fn summarize_skipped_fixes() { + let clang_versions = ClangVersions { + format_version: None, + tidy_version: Some(PSEUDO_VERSION), + }; + let review_summary = ReviewComments { + skipped_fixes: vec!["- first".to_string(), "- second".to_string()], + ..Default::default() + } + .summarize(&clang_versions, &[], 0, true); + assert!( + review_summary.contains("Please apply these parts yourself:\n\n- first\n- second\n") + ); + assert!(!review_summary.contains("No concerns to report")); + } + + #[test] + fn summarize_skipped_fixes_truncated() { + let clang_versions = ClangVersions { + format_version: None, + tidy_version: Some(PSEUDO_VERSION), + }; + let item = format!("- {}", "a".repeat(1000)); + let review_summary = ReviewComments { + skipped_fixes: vec![item; 100], + full_patch: "a".repeat(1000), + ..Default::default() + } + .summarize(&clang_versions, &[], 0, true); + assert!(review_summary.len() <= u16::MAX as usize); + assert!(review_summary.contains(PATCH_TOO_LARGE)); + assert!(review_summary.contains("more were not listed because this summary is too long.")); + assert!(review_summary.ends_with(USER_OUTREACH)); + } + + #[test] + fn summarize_skipped_fixes_no_room() { + let mut body = "a".repeat(u16::MAX as usize); + ReviewComments { + skipped_fixes: vec!["- first".to_string()], + ..Default::default() + } + .summarize_skipped_fixes(&mut body); + assert_eq!(body.len(), u16::MAX as usize); + } + #[test] fn summary_len_truncated() { let comments = vec![ReviewComment { diff --git a/cpp-linter/src/common_fs.rs b/cpp-linter/src/common_fs.rs index f8d82347..d58e409b 100644 --- a/cpp-linter/src/common_fs.rs +++ b/cpp-linter/src/common_fs.rs @@ -250,6 +250,16 @@ impl FileObj { // Count of clang-tidy diagnostics that had no fixes applied let mut total = 0; for note in &advice.notes { + if note.filename == file_name { + for skipped in ¬e.skipped_fixes { + review_comments.skipped_fixes.push(format!( + "- **{file_name}:{}** [{}]: {}", + note.line, + note.diagnostic_link(), + skipped.describe() + )); + } + } // Skip diagnostics about other files (eg. included headers); // their line numbers do not refer to this file. if note.filename == file_name @@ -275,6 +285,9 @@ impl FileObj { .as_str(), ); } + for skipped in ¬e.skipped_fixes { + suggestion.push_str(&format!("\n:warning: {}\n", skipped.describe())); + } let mut is_merged = false; for s in &mut review_comments.comments { if s.path == file_name @@ -506,7 +519,7 @@ mod test { #[test] fn unfixed_tidy_diagnostics() { - use crate::clang_tools::clang_tidy::{TidyAdvice, TidyNotification}; + use crate::clang_tools::clang_tidy::{SkippedFix, TidyAdvice, TidyNotification}; let repo_root = TempDir::new().unwrap(); let file_name = PathBuf::from("test_file.cpp"); @@ -527,6 +540,7 @@ mod test { diagnostic: "readability-identifier-length".to_string(), suggestion, fixed_lines: vec![], + skipped_fixes: vec![], }; let mut file_obj = FileObj::from(file_name, vec![1, 3], vec![1..=1, 3..=4]); file_obj.patched_path = Some(patched_file.path().to_path_buf()); @@ -535,7 +549,13 @@ mod test { // merged into the existing suggestion for line 1 note(1, vec![]), // not in any existing suggestion; creates a new suggestion - note(3, vec!["line3".to_string()]), + TidyNotification { + skipped_fixes: vec![SkippedFix { + file: "test_file.h".to_string(), + reason: "it is not one of the analyzed files".to_string(), + }], + ..note(3, vec!["line3".to_string()]) + }, // a diagnostic about an included header is not about this file's line 4 TidyNotification { filename: "test_file.h".to_string(), @@ -559,6 +579,18 @@ mod test { assert_eq!(new_suggestion.line_end, 3); assert!(new_suggestion.suggestion.contains("rationale for line 3")); assert!(new_suggestion.suggestion.contains("```cpp\nline3\n```")); + let skipped = "Part of this fix was not applied to `test_file.h` \ + because it is not one of the analyzed files."; + assert!( + new_suggestion + .suggestion + .contains(&format!("\n:warning: {skipped}\n")) + ); + assert_eq!(review_comments.skipped_fixes.len(), 1); + assert!( + review_comments.skipped_fixes[0].starts_with("- **test_file.cpp:3** [[readability") + ); + assert!(review_comments.skipped_fixes[0].ends_with(skipped)); // Processing the same file again merges duplicate suggestions file_obj diff --git a/cpp-linter/src/rest_client.rs b/cpp-linter/src/rest_client.rs index 551a0ebc..c3399ba0 100644 --- a/cpp-linter/src/rest_client.rs +++ b/cpp-linter/src/rest_client.rs @@ -236,7 +236,8 @@ impl RestClient { let total_review_comments = options.comments.len() as u32; self.client.cull_pr_reviews(&mut options).await?; - let has_changes = review_comments.tool_total > 0; + let has_changes = + review_comments.tool_total > 0 || !review_comments.skipped_fixes.is_empty(); let is_lgtm = options.comments.is_empty() && !has_changes; options.action = if feedback_inputs.passive_reviews { ReviewAction::Comment @@ -554,7 +555,7 @@ fn make_tidy_comment( let uses_auto_fix = !tidy_note.fixed_lines.is_empty(); let mut tmp_note = format!("- {}\n\n", tidy_note.filename); tmp_note.push_str(&format!( - " {filename}:{line}:{cols}: {severity}: [{diagnostic}]{auto_fixable}\n > {rationale}\n{concerned_code}", + " {filename}:{line}:{cols}: {severity}: [{diagnostic}]{auto_fixable}{skipped_fixes}\n > {rationale}\n{concerned_code}", filename = tidy_note.filename, line = tidy_note.line, cols = tidy_note.cols, @@ -565,6 +566,11 @@ fn make_tidy_comment( } else { "" }, + skipped_fixes = tidy_note + .skipped_fixes + .iter() + .map(|skipped| format!("\n :warning: {}", skipped.describe())) + .collect::(), rationale = tidy_note.rationale, concerned_code = if tidy_note.suggestion.is_empty() {String::from("")} else { format!("\n ```{ext}\n {suggestion}\n ```\n", @@ -625,7 +631,7 @@ mod test { clang_tools::{ ClangVersions, clang_format::FormatAdvice, - clang_tidy::{TidyAdvice, TidyNotification}, + clang_tidy::{SkippedFix, TidyAdvice, TidyNotification}, }, cli::{FeedbackInput, ThreadComments}, common_fs::FileObj, @@ -664,6 +670,7 @@ mod test { diagnostic: String::from("clang-diagnostic-warning"), suggestion: vec![], fixed_lines: vec![], + skipped_fixes: vec![], }]; file.tidy_advice = Some(TidyAdvice { notes }); file.format_advice = Some(FormatAdvice { @@ -755,6 +762,7 @@ mod test { diagnostic: String::from("clang-diagnostic-error"), suggestion: vec![String::from("int main() {}")], fixed_lines: vec![1], + skipped_fixes: vec![], }], }); file.format_advice = Some(FormatAdvice { @@ -764,6 +772,33 @@ mod test { vec![Arc::new(Mutex::new(file))] } + #[test] + fn tidy_comment_with_skipped_fixes() { + let files = make_dummy_files("warning"); + { + let mut file = files[0].lock().unwrap(); + let note = &mut file.tidy_advice.as_mut().unwrap().notes[0]; + note.skipped_fixes.push(SkippedFix { + file: "tests/demo/demo.hpp".to_string(), + reason: "it is not one of the analyzed files".to_string(), + }); + } + let mut comment = String::new(); + let mut remaining_length = u64::from(u16::MAX); + super::make_tidy_comment( + &files, + &mut comment, + 1, + &"1.2.3".to_string(), + &mut remaining_length, + ) + .unwrap(); + assert!(comment.contains( + "auto-fix included in generated patch[^1]\n :warning: Part of this fix was not \ + applied to `tests/demo/demo.hpp` because it is not one of the analyzed files.\n > " + )); + } + fn dummy_versions() -> ClangVersions { ClangVersions { format_version: Some(Version::new(1, 2, 3)), diff --git a/cpp-linter/src/sarif.rs b/cpp-linter/src/sarif.rs index 8ded7444..05367d3d 100644 --- a/cpp-linter/src/sarif.rs +++ b/cpp-linter/src/sarif.rs @@ -356,6 +356,7 @@ mod test { diagnostic: diagnostic.to_string(), suggestion: vec![], fixed_lines: vec![], + skipped_fixes: vec![], } } diff --git a/docs/docs/pr-review-caveats.md b/docs/docs/pr-review-caveats.md index ddc1c123..84dbee49 100644 --- a/docs/docs/pr-review-caveats.md +++ b/docs/docs/pr-review-caveats.md @@ -84,3 +84,9 @@ Request diff (the summation of file differences in a Pull Request). - Some fixes provided might depend on what compiler is used. We have made it so clang-tidy takes advantage of any fixes provided by the compiler. Compilation errors may still prevent clang-tidy from reporting all concerns. +- A clang-tidy fix can also change another file (for example, the declaration in a header). + That part of the fix is only applied if the other file is also analyzed, is not ignored by + `ignore-tidy`, and the change is within the file's changed lines (per + [`lines-changed-only`][lines-changed-only]). + Otherwise, the review summary (and the thread comment) lists the parts that were not applied, + so they can be applied by hand.