diff --git a/cpp-linter/src/clang_tools/clang_tidy.rs b/cpp-linter/src/clang_tools/clang_tidy.rs index 52d5640..cd82709 100644 --- a/cpp-linter/src/clang_tools/clang_tidy.rs +++ b/cpp-linter/src/clang_tools/clang_tidy.rs @@ -133,7 +133,7 @@ impl MakeSuggestions for TidyAdvice { fn parse_tidy_output( tidy_stdout: &[u8], database_json: &Option>, -) -> Result> { +) -> Result { let note_header = Regex::new(r"^(.+):(\d+):(\d+):\s(\w+):(.*)\[([a-zA-Z\d\-\.]+)\]$").unwrap(); let fixed_note = Regex::new(r"^.+:(\d+):\d+:\snote: FIX-IT applied suggested code changes$").unwrap(); @@ -218,14 +218,10 @@ fn parse_tidy_output( if let Some(note) = notification { result.push(note); } - if result.is_empty() { - Ok(None) - } else { - Ok(Some(TidyAdvice { - notes: result, - patched: None, - })) - } + Ok(TidyAdvice { + notes: result, + patched: None, + }) } /// Get a total count of clang-tidy advice from the given list of [FileObj]s. @@ -316,7 +312,10 @@ pub fn run_clang_tidy( ), )); } - file.tidy_advice = parse_tidy_output(&output.stdout, &clang_params.database_json)?; + file.tidy_advice = Some(parse_tidy_output( + &output.stdout, + &clang_params.database_json, + )?); if clang_params.tidy_review { if let Some(tidy_advice) = &mut file.tidy_advice { // cache file changes in a buffer and restore the original contents for further analysis by clang-format diff --git a/cpp-linter/src/clang_tools/mod.rs b/cpp-linter/src/clang_tools/mod.rs index 1b1bdd6..817db84 100644 --- a/cpp-linter/src/clang_tools/mod.rs +++ b/cpp-linter/src/clang_tools/mod.rs @@ -13,6 +13,7 @@ use anyhow::{anyhow, Context, Result}; use git2::{DiffOptions, Patch}; // non-std crates use lenient_semver; +use regex::Regex; use semver::Version; use tokio::task::JoinSet; use which::{which, which_in}; @@ -135,6 +136,28 @@ fn analyze_single_file( Ok((file.name.clone(), logs)) } +/// A struct to contain the version numbers of the clang-tools used +#[derive(Default)] +pub struct ClangVersions { + /// The clang-format version used. + pub format_version: Option, + + /// The clang-tidy version used. + pub tidy_version: Option, +} + +/// Run `clang-tool --version`, then extract and return the version number. +fn capture_clang_version(clang_tool: &PathBuf) -> Result { + let output = Command::new(clang_tool).arg("--version").output()?; + let stdout = String::from_utf8_lossy(&output.stdout); + let version_pattern = Regex::new(r"(?i)version\s*([\d.]+)").unwrap(); + let captures = version_pattern.captures(&stdout).ok_or(anyhow!( + "Failed to find version number in `{} --version` output", + clang_tool.to_string_lossy() + ))?; + Ok(captures.get(1).unwrap().as_str().to_string()) +} + /// Runs clang-tidy and/or clang-format and returns the parsed output from each. /// /// If `tidy_checks` is `"-*"` then clang-tidy is not executed. @@ -144,30 +167,29 @@ pub async fn capture_clang_tools_output( version: &str, clang_params: &mut ClangParams, rest_api_client: &impl RestApiClient, -) -> Result<()> { +) -> Result { + let mut clang_versions = ClangVersions::default(); // find the executable paths for clang-tidy and/or clang-format and show version // info as debugging output. if clang_params.tidy_checks != "-*" { - clang_params.clang_tidy_command = { - let cmd = get_clang_tool_exe("clang-tidy", version)?; - log::debug!( - "{} --version\n{}", - &cmd.to_string_lossy(), - String::from_utf8_lossy(&Command::new(&cmd).arg("--version").output()?.stdout) - ); - Some(cmd) - } + let exe_path = get_clang_tool_exe("clang-tidy", version)?; + let version_found = capture_clang_version(&exe_path)?; + log::debug!( + "{} --version: v{version_found}", + &exe_path.to_string_lossy() + ); + clang_versions.tidy_version = Some(version_found); + clang_params.clang_tidy_command = Some(exe_path); }; if !clang_params.style.is_empty() { - clang_params.clang_format_command = { - let cmd = get_clang_tool_exe("clang-format", version)?; - log::debug!( - "{} --version\n{}", - &cmd.to_string_lossy(), - String::from_utf8_lossy(&Command::new(&cmd).arg("--version").output()?.stdout) - ); - Some(cmd) - } + let exe_path = get_clang_tool_exe("clang-format", version)?; + let version_found = capture_clang_version(&exe_path)?; + log::debug!( + "{} --version: v{version_found}", + &exe_path.to_string_lossy() + ); + clang_versions.format_version = Some(version_found); + clang_params.clang_format_command = Some(exe_path); }; // parse database (if provided) to match filenames when parsing clang-tidy's stdout @@ -199,7 +221,7 @@ pub async fn capture_clang_tools_output( rest_api_client.end_log_group(); } } - Ok(()) + Ok(clang_versions) } /// A struct to describe a single suggestion in a pull_request review. @@ -221,7 +243,7 @@ pub struct ReviewComments { /// /// This differs from `comments.len()` because some suggestions may /// not fit within the file's diff. - pub tool_total: [u32; 2], + pub tool_total: [Option; 2], /// A list of comment suggestions to be posted. /// /// These suggestions are guaranteed to fit in the file's diff. @@ -234,11 +256,28 @@ pub struct ReviewComments { } impl ReviewComments { - pub fn summarize(&self) -> String { + pub fn summarize(&self, clang_versions: &ClangVersions) -> String { let mut body = format!("{COMMENT_MARKER}## Cpp-linter Review\n"); for t in 0u8..=1 { let mut total = 0; - let tool_name = if t == 0 { "clang-format" } else { "clang-tidy" }; + let (tool_name, tool_version) = if t == 0 { + ("clang-format", clang_versions.format_version.as_ref()) + } else { + ("clang-tidy", clang_versions.tidy_version.as_ref()) + }; + + let tool_total = if let Some(total) = self.tool_total[t as usize] { + total + } else { + // review was not requested from this tool or the tool was not used at all + continue; + }; + + // If the tool's version is unknown, then we don't need to output this line. + // NOTE: If the tool was invoked at all, then the tool's version shall be known. + if let Some(ver_str) = tool_version { + body.push_str(format!("### Used {tool_name} {ver_str}\n").as_str()); + } for comment in &self.comments { if comment .suggestion @@ -248,11 +287,10 @@ impl ReviewComments { } } - if total != self.tool_total[t as usize] { + if total != tool_total { body.push_str( format!( - "\nOnly {} out of {} {tool_name} concerns fit within this pull request's diff.\n", - self.tool_total[t as usize], total + "\nOnly {total} out of {tool_total} {tool_name} concerns fit within this pull request's diff.\n", ) .as_str(), ); @@ -351,6 +389,7 @@ pub trait MakeSuggestions { .with_context(|| format!("Failed to convert patch to string: {file_name}"))? .as_str(), ); + review_comments.tool_total[is_tidy_tool as usize].get_or_insert(0); if summary_only { return Ok(()); } @@ -408,7 +447,9 @@ pub trait MakeSuggestions { review_comments.comments.push(comment); } } - review_comments.tool_total[is_tidy_tool as usize] += hunks_in_patch; + review_comments.tool_total[is_tidy_tool as usize] = Some( + review_comments.tool_total[is_tidy_tool as usize].unwrap_or_default() + hunks_in_patch, + ); Ok(()) } } diff --git a/cpp-linter/src/common_fs/mod.rs b/cpp-linter/src/common_fs/mod.rs index b35dcb2..447e69d 100644 --- a/cpp-linter/src/common_fs/mod.rs +++ b/cpp-linter/src/common_fs/mod.rs @@ -158,12 +158,13 @@ impl FileObj { .unwrap_or_default() .to_str() .unwrap_or_default(); + // Count of clang-tidy diagnostics that had no fixes applied + let mut total = 0; for note in &advice.notes { if note.fixed_lines.is_empty() { // notification had no suggestion applied in `patched` let mut suggestion = format!( - "### clang-tidy diagnostic\n**{}:{}:{}** {}: [{}]\n> {}", - file_name, + "### clang-tidy diagnostic\n**{file_name}:{}:{}** {}: [{}]\n> {}", ¬e.line, ¬e.cols, ¬e.severity, @@ -172,10 +173,10 @@ impl FileObj { ); if !note.suggestion.is_empty() { suggestion.push_str( - format!("```{}\n{}```", file_ext, ¬e.suggestion.join("\n")).as_str(), + format!("```{file_ext}\n{}```", ¬e.suggestion.join("\n")).as_str(), ); } - review_comments.tool_total[1] += 1; + total += 1; let mut is_merged = false; for s in &mut review_comments.comments { if s.path == file_name @@ -197,6 +198,8 @@ impl FileObj { } } } + review_comments.tool_total[1] = + Some(review_comments.tool_total[1].unwrap_or_default() + total); } Ok(()) } @@ -308,14 +311,14 @@ mod test { // *********************** tests for FileObj::get_ranges() #[test] - fn get_ranges_0() { + fn get_ranges_none() { let file_obj = FileObj::new(PathBuf::from("tests/demo/demo.cpp")); let ranges = file_obj.get_ranges(&LinesChangedOnly::Off); assert!(ranges.is_empty()); } #[test] - fn get_ranges_2() { + fn get_ranges_diff() { let diff_chunks = vec![1..=10]; let added_lines = vec![4, 5, 9]; let file_obj = FileObj::from( @@ -328,7 +331,7 @@ mod test { } #[test] - fn get_ranges_1() { + fn get_ranges_added() { let diff_chunks = vec![1..=10]; let added_lines = vec![4, 5, 9]; let file_obj = FileObj::from( diff --git a/cpp-linter/src/rest_api/github/mod.rs b/cpp-linter/src/rest_api/github/mod.rs index 1d7c067..500d7f6 100644 --- a/cpp-linter/src/rest_api/github/mod.rs +++ b/cpp-linter/src/rest_api/github/mod.rs @@ -20,6 +20,7 @@ use serde_json; use super::{RestApiClient, RestApiRateLimitHeaders}; use crate::clang_tools::clang_format::tally_format_advice; use crate::clang_tools::clang_tidy::tally_tidy_advice; +use crate::clang_tools::ClangVersions; use crate::cli::{FeedbackInput, ThreadComments}; use crate::common_fs::{FileFilter, FileObj}; use crate::git::{get_diff, open_repo, parse_diff, parse_diff_from_buf}; @@ -230,6 +231,7 @@ impl RestApiClient for GithubApiClient { &self, files: &[Arc>], feedback_inputs: FeedbackInput, + clang_versions: ClangVersions, ) -> Result { let tidy_checks_failed = tally_tidy_advice(files); let format_checks_failed = tally_format_advice(files); @@ -239,8 +241,13 @@ impl RestApiClient for GithubApiClient { self.post_annotations(files, feedback_inputs.style.as_str()); } if feedback_inputs.step_summary { - comment = - Some(self.make_comment(files, format_checks_failed, tidy_checks_failed, None)); + comment = Some(self.make_comment( + files, + format_checks_failed, + tidy_checks_failed, + &clang_versions, + None, + )); self.post_step_summary(comment.as_ref().unwrap()); } self.set_exit_code( @@ -256,6 +263,7 @@ impl RestApiClient for GithubApiClient { files, format_checks_failed, tidy_checks_failed, + &clang_versions, Some(65535), )); } @@ -284,7 +292,8 @@ impl RestApiClient for GithubApiClient { if self.event_name == "pull_request" && (feedback_inputs.tidy_review || feedback_inputs.format_review) { - self.post_review(files, &feedback_inputs).await?; + self.post_review(files, &feedback_inputs, &clang_versions) + .await?; } Ok(format_checks_failed + tidy_checks_failed) } @@ -308,6 +317,7 @@ mod test { clang_tools::{ clang_format::{FormatAdvice, Replacement}, clang_tidy::{TidyAdvice, TidyNotification}, + ClangVersions, }, cli::FeedbackInput, common_fs::{FileFilter, FileObj}, @@ -389,8 +399,12 @@ mod test { gh_out_path.path() }, ); + let clang_versions = ClangVersions { + format_version: Some("x.y.z".to_string()), + tidy_version: Some("x.y.z".to_string()), + }; rest_api_client - .post_feedback(&files, feedback_inputs) + .post_feedback(&files, feedback_inputs, clang_versions) .await .unwrap(); let mut step_summary_content = String::new(); diff --git a/cpp-linter/src/rest_api/github/specific_api.rs b/cpp-linter/src/rest_api/github/specific_api.rs index 0ecb25a..be367f3 100644 --- a/cpp-linter/src/rest_api/github/specific_api.rs +++ b/cpp-linter/src/rest_api/github/specific_api.rs @@ -12,7 +12,7 @@ use anyhow::{anyhow, Context, Result}; use reqwest::{Client, Method, Url}; use crate::{ - clang_tools::{clang_format::summarize_style, ReviewComments}, + clang_tools::{clang_format::summarize_style, ClangVersions, ReviewComments}, cli::FeedbackInput, common_fs::FileObj, rest_api::{RestApiRateLimitHeaders, COMMENT_MARKER, USER_AGENT}, @@ -305,6 +305,7 @@ impl GithubApiClient { &self, files: &[Arc>], feedback_input: &FeedbackInput, + clang_versions: &ClangVersions, ) -> Result<()> { let url = self .api_url @@ -370,7 +371,8 @@ impl GithubApiClient { let mut payload = FullReview { event: if feedback_input.passive_reviews { String::from("COMMENT") - } else if has_no_changes { + } else if has_no_changes && review_comments.comments.is_empty() { + // if patches have no changes AND there are no comments about clang-tidy diagnostics String::from("APPROVE") } else { String::from("REQUEST_CHANGES") @@ -378,7 +380,7 @@ impl GithubApiClient { body: String::new(), comments: vec![], }; - payload.body = review_comments.summarize(); + payload.body = review_comments.summarize(clang_versions); if !summary_only { payload.comments = { let mut comments = vec![]; diff --git a/cpp-linter/src/rest_api/mod.rs b/cpp-linter/src/rest_api/mod.rs index 589b52c..198c5c6 100644 --- a/cpp-linter/src/rest_api/mod.rs +++ b/cpp-linter/src/rest_api/mod.rs @@ -17,6 +17,7 @@ use reqwest::{Client, IntoUrl, Method, Request, Response, Url}; // project specific modules pub mod github; +use crate::clang_tools::ClangVersions; use crate::cli::FeedbackInput; use crate::common_fs::{FileFilter, FileObj}; @@ -233,6 +234,7 @@ pub trait RestApiClient { files: &[Arc>], format_checks_failed: u64, tidy_checks_failed: u64, + clang_versions: &ClangVersions, max_len: Option, ) -> String { let mut comment = format!("{COMMENT_MARKER}# Cpp-Linter Report "); @@ -248,6 +250,8 @@ pub trait RestApiClient { files, &mut comment, format_checks_failed, + // tidy_version should be `Some()` value at this point. + clang_versions.tidy_version.as_ref().unwrap(), &mut remaining_length, ); } @@ -256,6 +260,8 @@ pub trait RestApiClient { files, &mut comment, tidy_checks_failed, + // format_version should be `Some()` value at this point. + clang_versions.format_version.as_ref().unwrap(), &mut remaining_length, ); } @@ -280,6 +286,7 @@ pub trait RestApiClient { &self, files: &[Arc>], user_inputs: FeedbackInput, + clang_versions: ClangVersions, ) -> impl Future>; /// Gets the URL for the next page in a paginated response. @@ -324,9 +331,12 @@ fn make_format_comment( files: &[Arc>], comment: &mut String, format_checks_failed: u64, + version_used: &String, remaining_length: &mut u64, ) { - let opener = format!("\n
clang-format reports: {} file(s) not formatted\n\n", format_checks_failed); + let opener = format!( + "\n
clang-format (v{version_used}) reports: {format_checks_failed} file(s) not formatted\n\n", + ); let closer = String::from("\n
"); let mut format_comment = String::new(); *remaining_length -= opener.len() as u64 + closer.len() as u64; @@ -351,11 +361,11 @@ fn make_tidy_comment( files: &[Arc>], comment: &mut String, tidy_checks_failed: u64, + version_used: &String, remaining_length: &mut u64, ) { let opener = format!( - "\n
clang-tidy reports: {} concern(s)\n\n", - tidy_checks_failed + "\n
clang-tidy (v{version_used}) reports: {tidy_checks_failed} concern(s)\n\n" ); let closer = String::from("\n
"); let mut tidy_comment = String::new(); @@ -412,6 +422,7 @@ mod test { use reqwest::{Method, Url}; use crate::{ + clang_tools::ClangVersions, cli::FeedbackInput, common_fs::{FileFilter, FileObj}, logger, @@ -456,6 +467,7 @@ mod test { &self, _files: &[Arc>], _user_inputs: FeedbackInput, + _clang_versions: ClangVersions, ) -> Result { Err(anyhow!("Not implemented")) } @@ -487,7 +499,7 @@ mod test { .await .is_err()); assert!(dummy - .post_feedback(&[], FeedbackInput::default()) + .post_feedback(&[], FeedbackInput::default(), ClangVersions::default()) .await .is_err()); dummy.end_log_group(); diff --git a/cpp-linter/src/run.rs b/cpp-linter/src/run.rs index a59b2af..f35a8d8 100644 --- a/cpp-linter/src/run.rs +++ b/cpp-linter/src/run.rs @@ -128,7 +128,7 @@ pub async fn run_main(args: Vec) -> Result<()> { let mut clang_params = ClangParams::from(&cli); let user_inputs = FeedbackInput::from(&cli); - capture_clang_tools_output( + let clang_versions = capture_clang_tools_output( &mut arc_files, cli.version.as_str(), &mut clang_params, @@ -137,7 +137,7 @@ pub async fn run_main(args: Vec) -> Result<()> { .await?; rest_api_client.start_log_group(String::from("Posting feedback")); let checks_failed = rest_api_client - .post_feedback(&arc_files, user_inputs) + .post_feedback(&arc_files, user_inputs, clang_versions) .await?; rest_api_client.end_log_group(); if env::var("PRE_COMMIT").is_ok_and(|v| v == "1") { diff --git a/cpp-linter/tests/reviews.rs b/cpp-linter/tests/reviews.rs index a949f6c..6e1fa81 100644 --- a/cpp-linter/tests/reviews.rs +++ b/cpp-linter/tests/reviews.rs @@ -60,6 +60,17 @@ impl Default for TestParams { } } +fn generate_tool_summary(review_enabled: bool, force_lgtm: bool, tool_name: &str) -> String { + if !review_enabled { + return String::new(); + } + if force_lgtm { + format!("No concerns reported by {}. Great job! :tada:", tool_name) + } else { + format!("Click here for the full {} patch", tool_name) + } +} + async fn setup(lib_root: &Path, test_params: &TestParams) { env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests env::set_var("GITHUB_EVENT_NAME", "pull_request"); @@ -161,16 +172,16 @@ async fn setup(lib_root: &Path, test_params: &TestParams) { } else { "REQUEST_CHANGES" }; - let tidy_summary = if test_params.force_lgtm { - "No concerns reported by clang-tidy. Great job! :tada:" - } else { - "Click here for the full clang-tidy patch" - }; - let format_summary = if test_params.force_lgtm { - "No concerns reported by clang-format. Great job! :tada:" - } else { - "Click here for the full clang-format patch" - }; + let tidy_summary = generate_tool_summary( + test_params.tidy_review, + test_params.force_lgtm, + "clang-tidy", + ); + let format_summary = generate_tool_summary( + test_params.format_review, + test_params.force_lgtm, + "clang-format", + ); let review_summary = format!( "{}## Cpp-linter Review.*{format_summary}.*{tidy_summary}.*{}", regex::escape(format!("{}", COMMENT_MARKER.escape_default()).as_str()), @@ -198,9 +209,7 @@ async fn setup(lib_root: &Path, test_params: &TestParams) { let mut tool_ignore = "**/*.c".to_string(); if test_params.force_lgtm { - // force a LGTM condition by skipping analysis on all files - tool_ignore.push('|'); - tool_ignore.push_str("src"); + tool_ignore.push_str("|**/*.cpp|**/*.h"); } let mut args = vec![ "cpp-linter".to_string(), @@ -209,8 +218,6 @@ async fn setup(lib_root: &Path, test_params: &TestParams) { format!("-l={}", test_params.lines_changed_only), format!("--ignore-tidy={}", tool_ignore), format!("--ignore-format={}", tool_ignore), - // "--tidy-checks=".to_string(), // use .clang-tidy file - "--style=file".to_string(), // use .clang-format file format!("--tidy-review={}", test_params.tidy_review), format!("--format-review={}", test_params.format_review), format!("--passive-reviews={}", test_params.passive_reviews), @@ -219,7 +226,16 @@ async fn setup(lib_root: &Path, test_params: &TestParams) { "-i=build".to_string(), ]; if test_params.force_lgtm { - args.push("-e=c".to_string()); + if test_params.tidy_review { + // only use a check that doesn't trigger concern on test assets + args.push("--tidy-checks=-*,bugprone-infinite-loop".to_string()); + } + if test_params.format_review { + // explicitly disable formatting using `DisableFormat: true` + args.push("--style={DisableFormat: true}".to_string()); + } + } else { + args.push("--style=file".to_string()); // use .clang-format file } let result = run_main(args).await; assert!(result.is_ok()); @@ -246,6 +262,26 @@ async fn all_lines() { .await; } +#[tokio::test] +async fn all_lines_tidy_only() { + test_review(&TestParams { + lines_changed_only: LinesChangedOnly::Off, + format_review: false, + ..Default::default() + }) + .await; +} + +#[tokio::test] +async fn all_lines_format_only() { + test_review(&TestParams { + lines_changed_only: LinesChangedOnly::Off, + tidy_review: false, + ..Default::default() + }) + .await; +} + #[tokio::test] async fn changed_lines() { test_review(&TestParams::default()).await; diff --git a/cspell.config.yml b/cspell.config.yml index a69f735..72c0cd0 100644 --- a/cspell.config.yml +++ b/cspell.config.yml @@ -49,6 +49,7 @@ words: - revparse - serde - superfences + - tada - tasklist - tempdir - tempfile