diff --git a/cpp-linter/clippy.toml b/cpp-linter/clippy.toml new file mode 100644 index 0000000..154626e --- /dev/null +++ b/cpp-linter/clippy.toml @@ -0,0 +1 @@ +allow-unwrap-in-tests = true diff --git a/cpp-linter/src/clang_tools/clang_format.rs b/cpp-linter/src/clang_tools/clang_format.rs index ec0513e..5031bcb 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 8296799..46cfffe 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 5ade992..e751f47 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 9e8308a..9b815ff 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 c572c80..767d3db 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 27c079a..97628ea 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 e1b20ea..8a5c856 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 39c0b47..1fdb747 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,