feat: capture and output clang tool's version number (#54)

This affects thread-comments, step-summary, and PR review summary.

I also added tests to ensure PR review summary only shows relevant information about the tools that were actually used.

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

Author: 2bndy5

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<Vec<CompilationUnit>>,
-) -> Result<Option<TidyAdvice>> {
+) -> Result<TidyAdvice> {
     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

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<String>,
+
+    /// The clang-tidy version used.
+    pub tidy_version: Option<String>,
+}
+
+/// Run `clang-tool --version`, then extract and return the version number.
+fn capture_clang_version(clang_tool: &PathBuf) -> Result<String> {
+    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<ClangVersions> {
+    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<u32>; 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(())
     }
 }

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> {}",
                         &note.line,
                         &note.cols,
                         &note.severity,
@@ -172,10 +173,10 @@ impl FileObj {
                     );
                     if !note.suggestion.is_empty() {
                         suggestion.push_str(
-                            format!("```{}\n{}```", file_ext, &note.suggestion.join("\n")).as_str(),
+                            format!("```{file_ext}\n{}```", &note.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(

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<Mutex<FileObj>>],
         feedback_inputs: FeedbackInput,
+        clang_versions: ClangVersions,
     ) -> Result<u64> {
         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();

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<Mutex<FileObj>>],
         feedback_input: &FeedbackInput,
+        clang_versions: &ClangVersions,
     ) -> Result<()> {
         let url = self
             .api_url
@@ -370,15 +371,16 @@ 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")
             },
             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![];