style: start phasing out .unwrap() calls (#242)

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

Author: 2bndy5

cpp-linter/clippy.toml

@@ -0,0 +1 @@
+allow-unwrap-in-tests = true

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::<String>() + 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,25 +68,31 @@ 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<Mutex<FileObj>>]) -> u64 {
+pub fn tally_format_advice(files: &[Arc<Mutex<FileObj>>]) -> Result<u64> {
     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.
 pub fn run_clang_format(
     file: &mut MutexGuard<FileObj>,
     clang_params: &ClangParams,
 ) -> Result<Vec<(log::Level, String)>> {
-    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::<Vec<_>>()

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<Vec<CompilationUnit>>,
 ) -> Result<TidyAdvice> {
-    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<Mutex<FileObj>>]) -> u64 {
+pub fn tally_tidy_advice(files: &[Arc<Mutex<FileObj>>]) -> Result<u64> {
     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,15 +264,19 @@ pub fn tally_tidy_advice(files: &[Arc<Mutex<FileObj>>]) -> u64 {
             }
         }
     }
-    total
+    Ok(total)
 }
 
 /// Run clang-tidy, then parse and return it's output.
 pub fn run_clang_tidy(
     file: &mut MutexGuard<FileObj>,
     clang_params: &ClangParams,
 ) -> Result<Vec<(log::Level, std::string::String)>> {
-    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)

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<PathBuf> {
         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<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[^\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::<Vec<String>>()
+                                    .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::<Vec<String>>()
-                            .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);

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<String> {
     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()
     }
 }