From 6644e5cea4f1038c06f0d68433b4d4565f59249b Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Wed, 7 Jan 2026 00:04:25 -0800 Subject: [PATCH 1/2] style: start phasing out `.unwrap()` calls Any `.unwrap()` will cause a panic from the call site. This replaces some calls to `.unwrap()` with a proper error message that can be better understood by users. Most of these calls sites are guarded by if conditions, so these new error messages should never be seen. But, just in case there's is some unforeseen bad behavior, these error messages should provide a better user experience. This doesn't cover the all of cpp-linter/**.rs sources. I've started migrating some of the REST API and git diff-ing behavior to [2bndy5/git-bot-feedback]. In doing so, the `.unwrap()` calls were replaced with custom exceptions. So, migrating to [2bndy5/git-bot-feedback] should take care of most other `.unwrap()` calls. [2bndy5/git-bot-feedback]: https://github.com/2bndy5/git-bot-feedback --- cpp-linter/src/clang_tools/clang_format.rs | 32 +++--- cpp-linter/src/clang_tools/clang_tidy.rs | 53 ++++++---- cpp-linter/src/clang_tools/mod.rs | 111 +++++++++++---------- cpp-linter/src/cli/mod.rs | 12 ++- cpp-linter/src/logger.rs | 9 +- cpp-linter/src/main.rs | 1 + cpp-linter/src/rest_api/github/mod.rs | 4 +- cpp-linter/src/run.rs | 2 +- 8 files changed, 127 insertions(+), 97 deletions(-) diff --git a/cpp-linter/src/clang_tools/clang_format.rs b/cpp-linter/src/clang_tools/clang_format.rs index ec0513ef..5031bcbd 100644 --- a/cpp-linter/src/clang_tools/clang_format.rs +++ b/cpp-linter/src/clang_tools/clang_format.rs @@ -7,7 +7,7 @@ use std::{ sync::{Arc, Mutex, MutexGuard}, }; -use anyhow::{Context, Result}; +use anyhow::{Context, Result, anyhow}; use log::Level; use serde::Deserialize; @@ -54,11 +54,12 @@ pub struct Replacement { /// Get a string that summarizes the given `--style` pub fn summarize_style(style: &str) -> String { - if ["google", "chromium", "microsoft", "mozilla", "webkit"].contains(&style) { + let mut char_iter = style.chars(); + if ["google", "chromium", "microsoft", "mozilla", "webkit"].contains(&style) + && let Some(first_char) = char_iter.next() + { // capitalize the first letter - let mut char_iter = style.chars(); - let first_char = char_iter.next().unwrap(); - first_char.to_uppercase().collect::() + char_iter.as_str() + first_char.to_ascii_uppercase().to_string() + char_iter.as_str() } else if style == "llvm" || style == "gnu" { style.to_ascii_uppercase() } else { @@ -67,17 +68,19 @@ pub fn summarize_style(style: &str) -> String { } /// Get a total count of clang-format advice from the given list of [FileObj]s. -pub fn tally_format_advice(files: &[Arc>]) -> u64 { +pub fn tally_format_advice(files: &[Arc>]) -> Result { let mut total = 0; for file in files { - let file = file.lock().unwrap(); + let file = file + .lock() + .map_err(|_| anyhow!("Failed to acquire lock on mutex for a source file"))?; if let Some(advice) = &file.format_advice && !advice.replacements.is_empty() { total += 1; } } - total + Ok(total) } /// Run clang-format for a specific `file`, then parse and return it's XML output. @@ -85,7 +88,11 @@ pub fn run_clang_format( file: &mut MutexGuard, clang_params: &ClangParams, ) -> Result> { - let mut cmd = Command::new(clang_params.clang_format_command.as_ref().unwrap()); + let cmd_path = clang_params + .clang_format_command + .as_ref() + .ok_or(anyhow!("clang-format path unknown"))?; + let mut cmd = Command::new(cmd_path); let mut logs = vec![]; cmd.args(["--style", &clang_params.style]); let ranges = file.get_ranges(&clang_params.lines_changed_only); @@ -101,12 +108,7 @@ pub fn run_clang_format( Level::Info, format!( "Getting format fixes with \"{} {}\"", - clang_params - .clang_format_command - .as_ref() - .unwrap() - .to_str() - .unwrap_or_default(), + cmd.get_program().to_string_lossy(), cmd.get_args() .map(|a| a.to_string_lossy()) .collect::>() diff --git a/cpp-linter/src/clang_tools/clang_tidy.rs b/cpp-linter/src/clang_tools/clang_tidy.rs index 82967997..46cfffe6 100644 --- a/cpp-linter/src/clang_tools/clang_tidy.rs +++ b/cpp-linter/src/clang_tools/clang_tidy.rs @@ -10,7 +10,7 @@ use std::{ }; // non-std crates -use anyhow::{Context, Result}; +use anyhow::{Context, Result, anyhow}; use regex::Regex; use serde::Deserialize; @@ -148,14 +148,18 @@ fn parse_tidy_output( tidy_stdout: &[u8], database_json: &Option>, ) -> Result { - let note_header = Regex::new(NOTE_HEADER).unwrap(); - let fixed_note = - Regex::new(r"^.+:(\d+):\d+:\snote: FIX-IT applied suggested code changes$").unwrap(); + let note_header = Regex::new(NOTE_HEADER) + .with_context(|| "Failed to compile RegExp pattern for note header")?; + let fixed_note = Regex::new(r"^.+:(\d+):\d+:\snote: FIX-IT applied suggested code changes$") + .with_context(|| "Failed to compile RegExp pattern for fixed note")?; let mut found_fix = false; let mut notification = None; let mut result = Vec::new(); - let cur_dir = current_dir().unwrap(); - for line in String::from_utf8(tidy_stdout.to_vec()).unwrap().lines() { + let cur_dir = current_dir().with_context(|| "Failed to access current working directory")?; + for line in String::from_utf8(tidy_stdout.to_vec()) + .with_context(|| "Failed to convert clang-tidy stdout to UTF-8 string")? + .lines() + { if let Some(captured) = note_header.captures(line) { // First check that the diagnostic name is a actual diagnostic name. // Sometimes clang-tidy uses square brackets to enclose additional context @@ -197,14 +201,12 @@ fn parse_tidy_output( filename = normalize_path(&PathBuf::from_iter([&cur_dir, &filename])); } debug_assert!(filename.is_absolute()); - if filename.is_absolute() && filename.starts_with(&cur_dir) { + if filename.is_absolute() + && let Ok(file_n) = filename.strip_prefix(&cur_dir) + { // if this filename can't be made into a relative path, then it is // likely not a member of the project's sources (ie /usr/include/stdio.h) - filename = filename - .strip_prefix(&cur_dir) - // we already checked above that filename.starts_with(current_directory) - .unwrap() - .to_path_buf(); + filename = file_n.to_path_buf(); } notification = Some(TidyNotification { @@ -247,10 +249,12 @@ fn parse_tidy_output( } /// Get a total count of clang-tidy advice from the given list of [FileObj]s. -pub fn tally_tidy_advice(files: &[Arc>]) -> u64 { +pub fn tally_tidy_advice(files: &[Arc>]) -> Result { let mut total = 0; for file in files { - let file = file.lock().unwrap(); + let file = file + .lock() + .map_err(|_| anyhow!("Failed to acquire lock on mutex for a source file"))?; if let Some(advice) = &file.tidy_advice { for tidy_note in &advice.notes { let file_path = PathBuf::from(&tidy_note.filename); @@ -260,7 +264,7 @@ pub fn tally_tidy_advice(files: &[Arc>]) -> u64 { } } } - total + Ok(total) } /// Run clang-tidy, then parse and return it's output. @@ -268,7 +272,11 @@ pub fn run_clang_tidy( file: &mut MutexGuard, clang_params: &ClangParams, ) -> Result> { - let mut cmd = Command::new(clang_params.clang_tidy_command.as_ref().unwrap()); + let cmd_path = clang_params + .clang_tidy_command + .as_ref() + .ok_or(anyhow!("clang-tidy command not located"))?; + let mut cmd = Command::new(cmd_path); let mut logs = vec![]; if !clang_params.tidy_checks.is_empty() { cmd.args(["-checks", &clang_params.tidy_checks]); @@ -318,7 +326,12 @@ pub fn run_clang_tidy( .join(" ") ), )); - let output = cmd.output().unwrap(); + let output = cmd.output().with_context(|| { + format!( + "Failed to execute clang-tidy on file: {}", + file_name.clone() + ) + })?; logs.push(( log::Level::Debug, format!( @@ -339,7 +352,9 @@ pub fn run_clang_tidy( &output.stdout, &clang_params.database_json, )?); - if clang_params.tidy_review { + if clang_params.tidy_review + && let Some(original_content) = &original_content + { if let Some(tidy_advice) = &mut file.tidy_advice { // cache file changes in a buffer and restore the original contents for further analysis tidy_advice.patched = @@ -348,7 +363,7 @@ pub fn run_clang_tidy( })?); } // original_content is guaranteed to be Some() value at this point - fs::write(&file_name, original_content.unwrap()) + fs::write(&file_name, original_content) .with_context(|| format!("Failed to restore file's original content: {file_name}"))?; } Ok(logs) diff --git a/cpp-linter/src/clang_tools/mod.rs b/cpp-linter/src/clang_tools/mod.rs index 5ade992c..e751f47e 100644 --- a/cpp-linter/src/clang_tools/mod.rs +++ b/cpp-linter/src/clang_tools/mod.rs @@ -1,3 +1,4 @@ +#![deny(clippy::unwrap_used)] //! This module holds the functionality related to running clang-format and/or //! clang-tidy. @@ -60,10 +61,12 @@ impl ClangTool { pub fn get_exe_path(&self, version: &RequestedVersion) -> Result { let name = self.as_str(); match version { - RequestedVersion::Path(path_buf) => { - which_in(name, Some(path_buf), current_dir().unwrap()) - .map_err(|_| anyhow!("Could not find {self} by path")) - } + RequestedVersion::Path(path_buf) => which_in( + name, + Some(path_buf), + current_dir().with_context(|| "Failed to access current working directory.")?, + ) + .map_err(|_| anyhow!("Could not find {self} by path")), // Thus, we should use whatever is installed and added to $PATH. RequestedVersion::SystemDefault | RequestedVersion::NoValue => { which(name).map_err(|_| anyhow!("Could not find clang tool by name")) @@ -118,12 +121,17 @@ impl ClangTool { fn capture_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[^\d]*([\d.]+)").unwrap(); + let version_pattern = Regex::new(r"(?i)version[^\d]*([\d.]+)") + .with_context(|| "Failed to allocate RegExp pattern (for clang version parsing) ")?; 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()) + Ok(captures + .get(1) + .ok_or(anyhow!("Failed to get version capture group"))? + .as_str() + .to_string()) } } @@ -434,53 +442,54 @@ pub trait MakeSuggestions { })?; hunks_in_patch += 1; let hunk_range = file_obj.is_hunk_in_diff(&hunk); - if hunk_range.is_none() { - continue; - } - let (start_line, end_line) = hunk_range.unwrap(); - let mut suggestion = String::new(); - let suggestion_help = self.get_suggestion_help(start_line, end_line); - let mut removed = vec![]; - for line_index in 0..line_count { - let diff_line = patch - .line_in_hunk(hunk_id, line_index) - .with_context(|| format!("Failed to get line {line_index} in a hunk {hunk_id} of patch for {file_name}"))?; - let line = String::from_utf8(diff_line.content().to_owned()) - .with_context(|| format!("Failed to convert line {line_index} buffer to string in hunk {hunk_id} of patch for {file_name}"))?; - if ['+', ' '].contains(&diff_line.origin()) { - suggestion.push_str(line.as_str()); - } else { - removed.push( - diff_line - .old_lineno() - .expect("Removed line should have a line number"), - ); + match hunk_range { + None => continue, + Some((start_line, end_line)) => { + let mut suggestion = String::new(); + let suggestion_help = self.get_suggestion_help(start_line, end_line); + let mut removed = vec![]; + for line_index in 0..line_count { + let diff_line = patch + .line_in_hunk(hunk_id, line_index) + .with_context(|| format!("Failed to get line {line_index} in a hunk {hunk_id} of patch for {file_name}"))?; + let line = String::from_utf8(diff_line.content().to_owned()) + .with_context(|| format!("Failed to convert line {line_index} buffer to string in hunk {hunk_id} of patch for {file_name}"))?; + if ['+', ' '].contains(&diff_line.origin()) { + suggestion.push_str(line.as_str()); + } else { + removed.push( + diff_line + .old_lineno() + .expect("Removed line should have a line number"), + ); + } + } + if suggestion.is_empty() && !removed.is_empty() { + suggestion.push_str( + format!( + "Please remove the line(s)\n- {}", + removed + .iter() + .map(|l| l.to_string()) + .collect::>() + .join("\n- ") + ) + .as_str(), + ) + } else { + suggestion = format!("```suggestion\n{suggestion}```"); + } + let comment = Suggestion { + line_start: start_line, + line_end: end_line, + suggestion: format!("{suggestion_help}\n{suggestion}"), + path: file_name.clone(), + }; + if !review_comments.is_comment_in_suggestions(&comment) { + review_comments.comments.push(comment); + } } } - if suggestion.is_empty() && !removed.is_empty() { - suggestion.push_str( - format!( - "Please remove the line(s)\n- {}", - removed - .iter() - .map(|l| l.to_string()) - .collect::>() - .join("\n- ") - ) - .as_str(), - ) - } else { - suggestion = format!("```suggestion\n{suggestion}```"); - } - let comment = Suggestion { - line_start: start_line, - line_end: end_line, - suggestion: format!("{suggestion_help}\n{suggestion}"), - path: file_name.clone(), - }; - if !review_comments.is_comment_in_suggestions(&comment) { - review_comments.comments.push(comment); - } } review_comments.tool_total[is_tidy_tool] = Some(review_comments.tool_total[is_tidy_tool].unwrap_or_default() + hunks_in_patch); diff --git a/cpp-linter/src/cli/mod.rs b/cpp-linter/src/cli/mod.rs index 9e8308a2..9b815ff8 100644 --- a/cpp-linter/src/cli/mod.rs +++ b/cpp-linter/src/cli/mod.rs @@ -1,3 +1,5 @@ +#![deny(clippy::unwrap_used)] + //! This module holds the Command Line Interface design. use std::{path::PathBuf, str::FromStr}; @@ -437,17 +439,17 @@ pub struct FeedbackOptions { /// the value will be split at spaces. pub fn convert_extra_arg_val(args: &[String]) -> Vec { let mut val = args.iter(); - if val.len() == 1 { + if args.len() == 1 + && let Some(v) = val.next() + { // specified once; split and return result - val.next() - .unwrap() - .trim_matches('\'') + v.trim_matches('\'') .trim_matches('"') .split(' ') .map(|i| i.to_string()) .collect() } else { - // specified multiple times; just return + // specified multiple times; just return a clone of the values val.map(|i| i.to_string()).collect() } } diff --git a/cpp-linter/src/logger.rs b/cpp-linter/src/logger.rs index c572c80d..767d3dbf 100644 --- a/cpp-linter/src/logger.rs +++ b/cpp-linter/src/logger.rs @@ -1,3 +1,4 @@ +#![deny(clippy::unwrap_used)] //! A module to initialize and customize the logger object used in (most) stdout. use std::{ @@ -36,7 +37,7 @@ impl Log for SimpleLogger { writeln!(stdout, "{}", record.args()).expect("Failed to write log command to stdout"); stdout .flush() - .expect("Failed to flush log command to stdout"); + .expect("Failed to flush log command in stdout"); } else if self.enabled(record.metadata()) { let module = record.module_path(); if module.is_none_or(|v| v.starts_with("cpp_linter")) { @@ -47,12 +48,12 @@ impl Log for SimpleLogger { record.args() ) .expect("Failed to write log message to stdout"); - } else { + } else if let Some(module) = module { writeln!( stdout, "[{}]{{{}:{}}}: {}", Self::level_color(&record.level()), - module.unwrap(), // safe to unwrap here because the None case is caught above + module, record.line().unwrap_or_default(), record.args() ) @@ -60,7 +61,7 @@ impl Log for SimpleLogger { } stdout .flush() - .expect("Failed to flush log message to stdout"); + .expect("Failed to flush log message in stdout"); } } diff --git a/cpp-linter/src/main.rs b/cpp-linter/src/main.rs index 27c079a3..97628ea9 100644 --- a/cpp-linter/src/main.rs +++ b/cpp-linter/src/main.rs @@ -1,4 +1,5 @@ #![cfg(not(test))] +#![deny(clippy::unwrap_used)] /// This crate is the binary executable's entrypoint. use std::env; diff --git a/cpp-linter/src/rest_api/github/mod.rs b/cpp-linter/src/rest_api/github/mod.rs index e1b20ea7..8a5c8561 100644 --- a/cpp-linter/src/rest_api/github/mod.rs +++ b/cpp-linter/src/rest_api/github/mod.rs @@ -184,8 +184,8 @@ impl RestApiClient for GithubApiClient { feedback_inputs: FeedbackInput, clang_versions: ClangVersions, ) -> Result { - let tidy_checks_failed = tally_tidy_advice(files); - let format_checks_failed = tally_format_advice(files); + let tidy_checks_failed = tally_tidy_advice(files)?; + let format_checks_failed = tally_format_advice(files)?; let mut comment = None; if feedback_inputs.file_annotations { diff --git a/cpp-linter/src/run.rs b/cpp-linter/src/run.rs index 39c0b47e..1fdb7477 100644 --- a/cpp-linter/src/run.rs +++ b/cpp-linter/src/run.rs @@ -2,7 +2,7 @@ //! //! In python, this module is exposed as `cpp_linter.run` that has 1 function exposed: //! `main()`. - +#![deny(clippy::unwrap_used)] use std::{ env, path::Path, From 05efa778f35c6a836b365d2d18287555b0eecc4b Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Wed, 7 Jan 2026 16:30:48 -0800 Subject: [PATCH 2/2] add clippy.toml --- cpp-linter/clippy.toml | 1 + 1 file changed, 1 insertion(+) create mode 100644 cpp-linter/clippy.toml diff --git a/cpp-linter/clippy.toml b/cpp-linter/clippy.toml new file mode 100644 index 00000000..154626ef --- /dev/null +++ b/cpp-linter/clippy.toml @@ -0,0 +1 @@ +allow-unwrap-in-tests = true