From a3dbd01574a1294d26a1d20c53d5df3771da33ad Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Thu, 11 Jun 2026 07:03:29 -0700 Subject: [PATCH 1/2] refactor: use stricter linting rules - mandates no panicking code (eg. `unwrap()`, `expect()`, `panic!()`, etc) used in production; tests are allowed to panic. - ensures all public API has a doc comment. Any public API missing a doc comment now has a one. - collapsed single-file submodules into a normal submodule (eg. `common_fs/mod.rs` becomes just `common_fs.rs`) --- cpp-linter/src/clang_tools/clang_format.rs | 2 + cpp-linter/src/clang_tools/clang_tidy.rs | 5 +- cpp-linter/src/clang_tools/mod.rs | 8 ++- cpp-linter/src/cli/mod.rs | 13 +++- cpp-linter/src/cli/structs.rs | 70 ++++++++++++++++++- .../src/{common_fs/mod.rs => common_fs.rs} | 38 +++++++--- cpp-linter/src/error.rs | 62 ++++++++++++++++ cpp-linter/src/git.rs | 8 ++- cpp-linter/src/lib.rs | 10 ++- cpp-linter/src/logger.rs | 2 +- cpp-linter/src/main.rs | 1 - .../{rest_client/mod.rs => rest_client.rs} | 69 +++++++++++++----- cpp-linter/src/run.rs | 2 +- 13 files changed, 248 insertions(+), 42 deletions(-) rename cpp-linter/src/{common_fs/mod.rs => common_fs.rs} (86%) rename cpp-linter/src/{rest_client/mod.rs => rest_client.rs} (91%) diff --git a/cpp-linter/src/clang_tools/clang_format.rs b/cpp-linter/src/clang_tools/clang_format.rs index 35d51fdf..aac933a0 100644 --- a/cpp-linter/src/clang_tools/clang_format.rs +++ b/cpp-linter/src/clang_tools/clang_format.rs @@ -16,11 +16,13 @@ use log::Level; use super::{CACHE_DIR, MakeSuggestions}; use crate::{cli::ClangParams, common_fs::FileObj, error::ClangCaptureError}; +/// A struct to hold clang-format advice for a single file. #[derive(Debug, Clone, PartialEq, Eq, Default)] pub struct FormatAdvice { /// A list of line ranges that clang-format wants to replace. pub replacements: Vec>, + /// A path to a cached file containing the full contents of the file after applying clang-format fixes. pub patched: PathBuf, } diff --git a/cpp-linter/src/clang_tools/clang_tidy.rs b/cpp-linter/src/clang_tools/clang_tidy.rs index bc7201b4..91320904 100644 --- a/cpp-linter/src/clang_tools/clang_tidy.rs +++ b/cpp-linter/src/clang_tools/clang_tidy.rs @@ -70,6 +70,7 @@ pub struct TidyNotification { } impl TidyNotification { + /// Get a markdown-formatted link to the clang-tidy documentation page for [`Self::diagnostic`]. pub fn diagnostic_link(&self) -> String { if self.diagnostic.starts_with("clang-diagnostic-") { // clang-diagnostic-* diagnostics are compiler diagnostics and don't have @@ -101,6 +102,8 @@ impl TidyNotification { pub struct TidyAdvice { /// A list of notifications parsed from clang-tidy stdout. pub notes: Vec, + + /// A buffer to hold the contents of the file after applying clang-tidy fixes. pub patched: Option>, } @@ -377,7 +380,7 @@ pub fn run_clang_tidy( #[cfg(test)] mod test { - #![allow(clippy::unwrap_used)] + #![allow(clippy::unwrap_used, clippy::expect_used)] use std::{ env, diff --git a/cpp-linter/src/clang_tools/mod.rs b/cpp-linter/src/clang_tools/mod.rs index e5312ccb..e4f25b2a 100644 --- a/cpp-linter/src/clang_tools/mod.rs +++ b/cpp-linter/src/clang_tools/mod.rs @@ -1,4 +1,3 @@ -#![deny(clippy::unwrap_used)] //! This module holds the functionality related to running clang-format and/or //! clang-tidy. @@ -28,6 +27,7 @@ use clang_format::run_clang_format; pub mod clang_tidy; use clang_tidy::run_clang_tidy; +/// The directory name to use for caching clang-tidy and clang-format results. pub const CACHE_DIR: &str = ".cpp-linter-cache"; /// This creates a task to run clang-tidy and clang-format on a single file. @@ -95,7 +95,7 @@ pub struct ClangVersions { pub tidy_version: Option, } -/// Runs clang-tidy and/or clang-format and returns the parsed output from each. +/// Runs clang-tidy and/or clang-format and returns the version used for each. /// /// If `tidy_checks` is `"-*"` then clang-tidy is not executed. /// If `style` is a blank string (`""`), then clang-format is not executed. @@ -228,6 +228,7 @@ pub struct ReviewComments { } impl ReviewComments { + /// Get a markdown-formatted string that summarizes the given [`ReviewComment`]s. pub fn summarize( &self, clang_versions: &ClangVersions, @@ -290,6 +291,7 @@ impl ReviewComments { body } + /// Check if a given comment's [`Suggestion`] is already contained within the existing [`Self::comments`]. pub fn is_comment_in_suggestions(&mut self, comment: &Suggestion) -> bool { for s in &mut self.comments { if s.path == comment.path @@ -305,6 +307,8 @@ impl ReviewComments { } } +/// A helper function to create a [`Diff`] and its associated [`InternedInput`] from +/// a `patched` buffer and the `original_content`` of the file. pub fn make_patch<'buffer>( patched: &'buffer str, original_content: &'buffer str, diff --git a/cpp-linter/src/cli/mod.rs b/cpp-linter/src/cli/mod.rs index 7a4a68bc..bf53ba6b 100644 --- a/cpp-linter/src/cli/mod.rs +++ b/cpp-linter/src/cli/mod.rs @@ -1,6 +1,5 @@ -#![deny(clippy::unwrap_used)] - //! This module holds the Command Line Interface design. + use std::path::PathBuf; #[cfg(feature = "bin")] use std::str::FromStr; @@ -22,7 +21,9 @@ pub use structs::{ClangParams, FeedbackInput, LinesChangedOnly, ThreadComments}; #[cfg(feature = "bin")] #[derive(Debug, Clone, PartialEq, Eq, ValueEnum)] pub enum Verbosity { + /// Enables the [`log::Level::Info`] log level and above. Info, + /// Enables the [`log::Level::Debug`] log level and above. Debug, } @@ -39,18 +40,23 @@ impl Verbosity { #[derive(Debug, Clone, Parser)] #[command(author, about)] pub struct Cli { + /// The CLI's general options, such as `--version` and `--verbosity`. #[command(flatten)] pub general_options: GeneralOptions, + /// The CLI's source options, such as `--extensions` and `--ignore`. #[command(flatten)] pub source_options: SourceOptions, + /// The CLI's clang-format options, such as `--style` and `--ignore-format`. #[command(flatten)] pub format_options: FormatOptions, + /// The CLI's clang-tidy options, such as `--tidy-checks` and `--database`. #[command(flatten)] pub tidy_options: TidyOptions, + /// The CLI's feedback options, such as `--thread-comments` and `--no-lgtm`. #[command(flatten)] pub feedback_options: FeedbackOptions, @@ -67,6 +73,9 @@ pub struct Cli { )] pub not_ignored: Option>, + /// A subcommand to run instead of the default action of cpp-linter. + /// + /// This is currently only used for the `version` subcommand, which prints the version of cpp-linter and exits. #[command(subcommand)] pub commands: Option, } diff --git a/cpp-linter/src/cli/structs.rs b/cpp-linter/src/cli/structs.rs index 572234be..37254b76 100644 --- a/cpp-linter/src/cli/structs.rs +++ b/cpp-linter/src/cli/structs.rs @@ -78,6 +78,7 @@ impl ValueEnum for LinesChangedOnly { } impl LinesChangedOnly { + /// Is the instance valid for under the given conditions/flags? pub fn is_change_valid(&self, added_lines: bool, diff_chunks: bool) -> bool { match self { LinesChangedOnly::Off => true, @@ -164,20 +165,68 @@ impl Display for ThreadComments { /// A data structure to contain CLI options that relate to /// clang-tidy or clang-format arguments. +/// +/// This struct is designed to be a thread-safe vehicle for common clang arguments and configurations. #[derive(Debug, Clone, Default)] pub struct ClangParams { + /// The clang-tidy checks to run. + /// + /// Format of this string follows the `-checks` argument of clang-tidy. pub tidy_checks: String, + + /// Focus on changed lines or entire files. pub lines_changed_only: LinesChangedOnly, + + /// An optional path to a compilation database, used for clang-tidy. pub database: Option, + + /// Extra arguments to pass to clang-tidy. + /// + /// Format of these strings follows the `-extra-arg` argument of clang-tidy. pub extra_args: Vec, + + /// An optional list of compilation units, used for clang-tidy. + /// + /// This can be set to None initially, but it will be populated by + /// [`capture_clang_tools_output()`](crate::clang_tools::capture_clang_tools_output), + /// if the [`Self::database`] is given [`Some`] valid value (and the + /// compile_commands.json file is parsed successfully). pub database_json: Option>, + + /// The clang-format style to use. + /// + /// Format of this string follows the `-style` argument of clang-format. pub style: String, + + /// An optional path to the clang-tidy executable. + /// + /// If [`Self::tidy_checks`] is not `-*`, then this will be populated by + /// [`capture_clang_tools_output()`](crate::clang_tools::capture_clang_tools_output), + /// regardless if this is given [`Some`] value. pub clang_tidy_command: Option, + + /// An optional path to the clang-format executable. + /// + /// If [`Self::style`] is not an empty string, then this will be populated by + /// [`capture_clang_tools_output()`](crate::clang_tools::capture_clang_tools_output), + /// regardless if this is given [`Some`] value. pub clang_format_command: Option, + + /// An optional [`FileFilter`] to exclude files only from clang-tidy analysis. pub tidy_filter: Option, + + /// An optional [`FileFilter`] to exclude files only from clang-format analysis. pub format_filter: Option, + + /// Assert this if preparing a PR review with clang-tidy advice. pub tidy_review: bool, + + /// Assert this if preparing a PR review with clang-format advice. pub format_review: bool, + + /// The root of the repository, used to locate relative file paths in processing. + /// + /// A project-specific cache folder is created in this path. pub repo_root: PathBuf, } @@ -237,14 +286,33 @@ impl From<&Cli> for ClangParams { /// A struct to contain CLI options that relate to /// [`RestClient.post_feedback()`](fn@crate::rest_api::RestClient.post_feedback()). pub struct FeedbackInput { + /// How thread comments are created or updated. pub thread_comments: ThreadComments, + + /// Whether to omit a "LGTM" type message. pub no_lgtm: bool, + + /// Whether to post a step summary comment. pub step_summary: bool, + + /// Whether to post file annotations. pub file_annotations: bool, + + /// The clang-format style to show in file annotations. pub style: String, + + /// Whether to post a PR review with clang-tidy suggestions/notes. pub tidy_review: bool, + + /// Whether to post a PR review with clang-format suggestions. pub format_review: bool, + + /// Should PR reviews be commentary? + /// + /// If false, reviews will approve or request changes. pub passive_reviews: bool, + + /// The root of the repository, used to locate relative file paths in processing. pub repo_root: PathBuf, } @@ -285,7 +353,7 @@ impl Default for FeedbackInput { #[cfg(all(test, feature = "bin"))] mod test { - #![allow(clippy::unwrap_used)] + #![allow(clippy::unwrap_used, clippy::expect_used)] use clap::{Parser, ValueEnum}; diff --git a/cpp-linter/src/common_fs/mod.rs b/cpp-linter/src/common_fs.rs similarity index 86% rename from cpp-linter/src/common_fs/mod.rs rename to cpp-linter/src/common_fs.rs index cd78b340..cebffa57 100644 --- a/cpp-linter/src/common_fs/mod.rs +++ b/cpp-linter/src/common_fs.rs @@ -3,6 +3,7 @@ use std::{ fmt::Debug, fs, + num::NonZeroU32, ops::RangeInclusive, path::{Path, PathBuf}, }; @@ -61,10 +62,15 @@ impl FileObj { added_lines: Vec, diff_chunks: Vec>, ) -> Self { + // filter out any line numbers that are 0 since line numbers are always 1-indexed in diffs + let added_lines: Vec = added_lines + .into_iter() + .filter_map(NonZeroU32::new) + .collect(); let added_ranges = FileObj::consolidate_numbers_to_ranges(&added_lines); FileObj { name, - added_lines, + added_lines: added_lines.into_iter().map(|v| v.get()).collect(), added_ranges, diff_chunks, format_advice: None, @@ -75,23 +81,33 @@ impl FileObj { /// A helper function to consolidate a [Vec] of line numbers into a /// [Vec>] in which each range describes the beginning and /// ending of a group of consecutive line numbers. - fn consolidate_numbers_to_ranges(lines: &[u32]) -> Vec> { - let mut range_start = None; + fn consolidate_numbers_to_ranges(lines: &[NonZeroU32]) -> Vec> { let mut ranges: Vec> = Vec::new(); - for (index, number) in lines.iter().enumerate() { - if index == 0 { - range_start = Some(*number); - } else if number - 1 != lines[index - 1] { - ranges.push(RangeInclusive::new(range_start.unwrap(), lines[index - 1])); - range_start = Some(*number); + let mut line_iter = lines.iter().enumerate(); + let mut range_start = match line_iter.next() { + Some((_, number)) => number.get(), + None => return ranges, // return empty vector if no lines + }; + // lines.len() cannot be 0 at this point + let last_index = lines.len() - 1; + for (index, number) in line_iter { + // use let chain to avoid repeated lookup of lines[index - 1]. + // should always yield some value since we entered the for loop at index 1. + if let Some(prev_line) = lines.get(index - 1) + && number.get() - 1 != prev_line.get() + { + ranges.push(RangeInclusive::new(range_start, prev_line.get())); + range_start = number.get(); } - if index == lines.len() - 1 { - ranges.push(RangeInclusive::new(range_start.unwrap(), *number)); + if index == last_index { + ranges.push(RangeInclusive::new(range_start, number.get())); } } ranges } + /// Get the list of line ranges to consider based on the given + /// [`LinesChangedOnly`] configuration. pub fn get_ranges(&self, lines_changed_only: &LinesChangedOnly) -> Vec> { match lines_changed_only { LinesChangedOnly::Diff => self.diff_chunks.to_vec(), diff --git a/cpp-linter/src/error.rs b/cpp-linter/src/error.rs index e92fcfa6..32bd094f 100644 --- a/cpp-linter/src/error.rs +++ b/cpp-linter/src/error.rs @@ -1,78 +1,140 @@ +//! This module defines error types for the cpp-linter crate. + use clang_tools_manager::GetToolError; use git_bot_feedback::RestClientError; +/// Errors related to [`FileObj`](crate::common_fs::FileObj) methods' results. #[derive(Debug, thiserror::Error)] pub enum FileObjError { + /// Error when failing to read a file's contents. #[error("Failed to read file contents")] ReadFile(std::io::Error), + + /// Error when failing to convert a file's contents to a UTF-8 string. #[error("Failed to convert patch buffer to UTF-8 string for file {0}: {1}")] FromUtf8Error(String, #[source] std::string::FromUtf8Error), } +/// Errors related to the REST client used for posting feedback and special logging. #[derive(Debug, thiserror::Error)] pub enum ClientError { + /// Error to propagate client failures. #[error(transparent)] RestClientError(#[from] RestClientError), + + /// Error when the client cannot detect a supported Git server or CI platform. #[error("Unsupported Git server or CI platform")] GitServerUnsupported, + + /// Error when the client encounters a poisoned mutex during file processing. #[error("Mutex lock poisoned for a source file: {0}")] MutexPoisoned(String), + + /// Error to propagate a [`FileObjError`] encountered during file processing. #[error(transparent)] FileObjError(#[from] FileObjError), } +/// Errors related to invoking clang tools and processing their output. #[derive(Debug, thiserror::Error)] pub enum ClangCaptureError { + /// Error when failing to acquire a lock on a file's mutex. #[error("Failed to acquire a lock on a file's mutex")] MutexPoisoned, + + /// Error when invoking a clang tool with no known path to the binary executable. #[error("Unknown path to {0} tool; required to invoke it.")] ToolPathUnknown(&'static str), + + /// Error when a clang tool fails to be invoked. #[error("Failed to {task}: {source}")] FailedToRunCommand { + /// The purpose of running the clang tool. + /// + /// May include context about the arguments passed to the clang-tool. task: String, + + /// The underlying error from trying to run the clang tool. #[source] source: std::io::Error, }, + + /// Error when the output of a clang tool cannot be parsed as a UTF-8 string. #[error("{task} output was not valid UTF-8: {source}")] NonUtf8Output { + /// The clang tool that produced the output. task: String, + + /// The underlying error from trying to convert the clang tool's output to a UTF-8 string. #[source] source: std::string::FromUtf8Error, }, + + /// Error when failing to read a file's contents. #[error("Failed to read contents of file '{file_name}': {source}")] ReadFileFailed { + /// The name of the file that failed to be read. file_name: String, + + /// The underlying error from trying to read the file's contents. #[source] source: std::io::Error, }, + + /// Error when failing to write a file. #[error("Failed to write file '{file_name}': {source}")] WriteFileFailed { + /// The name of the file that failed to be written. file_name: String, + + /// The underlying error from trying to write the file. #[source] source: std::io::Error, }, + + /// Error when failing to compile a regular expression pattern. #[error("Failed to compile regular expression: {0}")] RegexError(#[from] regex::Error), + + /// Error when failing to determine the current working directory. #[error("Failed to determine the current working directory: {0}")] UnknownWorkingDirectory(#[source] std::io::Error), + + /// Error when failing to parse an integer from a string. #[error("Failed to parse integer from string: {0}")] ParseIntError(#[from] std::num::ParseIntError), + + /// Error when failing to determine the parent directory for caching purposes. #[error("Failed to determine the parent directory for caching purposes")] UnknownCacheParentPath, + + /// Error when failing to create a directory for caching purposes. #[error("Failed to create directory for caching patches: {0}")] MkDirFailed(#[source] std::io::Error), } +/// Errors related to orchestrating clang tools in parallel. #[derive(Debug, thiserror::Error)] pub enum ClangTaskError { + /// Error to propagate failures from downloading/installing/finding a clang tool. #[error(transparent)] GetToolError(#[from] GetToolError), + + /// Error when the tool manager cannot find a suitable version of a clang tool. #[error("Failed to find tool {0} or install a suitable version")] FindToolError(&'static str), + + /// Error when failing to parse the compilation database. + /// + /// This can occur regardless of invoking clang-tidy. #[error("Failed to parse compilation database: {0}")] ParseJsonError(#[from] serde_json::Error), + + /// Error to propagate task joining failures (from the tokio runtime). #[error("Failed to execute task in parallel: {0}")] JoinError(#[from] tokio::task::JoinError), + + /// Error to propagate failures from capturing clang tools' output. #[error(transparent)] ClangCaptureError(#[from] ClangCaptureError), } diff --git a/cpp-linter/src/git.rs b/cpp-linter/src/git.rs index 03c7cb44..393d54c2 100644 --- a/cpp-linter/src/git.rs +++ b/cpp-linter/src/git.rs @@ -1,11 +1,13 @@ //! This module was primarily used to parse diff blobs. //! //! Since migrating to git-bot-feedback crate, this module is now purely for regression testing. -//! Any logic that parses diffs from 2 text blobs has been moved to git-bot-feedback. -//! Some diff creation/parsing logic remains in clang_tools/mod.rs module using libgit2 API instead. +//! Any logic that parses diffs has been moved to git-bot-feedback. +//! Some diff creation/parsing logic remains in clang_tools/mod.rs module using gix-imara-diff API instead. +#![cfg(test)] -#[cfg(test)] mod test { + #![allow(clippy::unwrap_used, clippy::panic, clippy::expect_used)] + use std::{ env::{self, current_dir, set_current_dir}, fs, diff --git a/cpp-linter/src/lib.rs b/cpp-linter/src/lib.rs index f7250a6c..eb6fa860 100644 --- a/cpp-linter/src/lib.rs +++ b/cpp-linter/src/lib.rs @@ -5,13 +5,21 @@ html_favicon_url = "https://github.com/cpp-linter/cpp-linter-rs/raw/main/docs/docs/images/favicon.ico" )] #![doc = include_str!("../README.md")] +#![deny( + clippy::unwrap_used, + clippy::expect_used, + clippy::panic, + clippy::unimplemented, + clippy::todo, + missing_docs +)] // project specific modules/crates pub mod clang_tools; pub mod cli; pub mod common_fs; pub mod error; -pub mod git; +mod git; pub mod logger; pub mod rest_client; pub mod run; diff --git a/cpp-linter/src/logger.rs b/cpp-linter/src/logger.rs index 6dc9bc34..436f04ea 100644 --- a/cpp-linter/src/logger.rs +++ b/cpp-linter/src/logger.rs @@ -1,4 +1,3 @@ -#![deny(clippy::unwrap_used)] #![cfg(feature = "bin")] //! A module to initialize and customize the logger object used in (most) stdout. @@ -26,6 +25,7 @@ impl SimpleLogger { } } +#[allow(clippy::expect_used)] impl Log for SimpleLogger { fn enabled(&self, metadata: &Metadata) -> bool { metadata.level() <= log::max_level() diff --git a/cpp-linter/src/main.rs b/cpp-linter/src/main.rs index 97628ea9..27c079a3 100644 --- a/cpp-linter/src/main.rs +++ b/cpp-linter/src/main.rs @@ -1,5 +1,4 @@ #![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_client/mod.rs b/cpp-linter/src/rest_client.rs similarity index 91% rename from cpp-linter/src/rest_client/mod.rs rename to cpp-linter/src/rest_client.rs index e330af87..1426ab65 100644 --- a/cpp-linter/src/rest_client/mod.rs +++ b/cpp-linter/src/rest_client.rs @@ -1,3 +1,5 @@ +//! This module defines the struct that wraps employs a REST API client. + use std::{ env, path::{Path, PathBuf}, @@ -33,21 +35,34 @@ pub const USER_OUTREACH: &str = concat!( "(https://github.com/cpp-linter/cpp-linter-action/issues)" ); +/// A wrapper struct around a REST API client. +/// +/// Underneath, the client may support various Git servers and CI platforms. +/// Currently, the client supports GitHub and GitHub Actions. pub struct RestClient { client: Box, } impl RestClient { + /// Initializes the REST API client and sets the UserAgent header. + /// + /// See [git_bot_feedback::client::init_client] for details on + /// possible errors during initialization. pub fn new() -> Result { let mut client = init_client()?; client.set_user_agent(USER_AGENT)?; Ok(Self { client }) } + /// Is this a pull request event? pub fn is_pr(&self) -> bool { self.client.is_pr_event() } + /// Gets a list of changed files. + /// + /// Use the [`FileFilter`] and [`LinesChangedOnly`] to filter out files + /// that might be negligible. pub async fn get_list_of_changed_files( &self, file_filter: &FileFilter, @@ -86,18 +101,25 @@ impl RestClient { .collect()) } + /// Should debug logs be enabled? pub fn is_debug_enabled(&self) -> bool { self.client.is_debug_enabled() } + /// Start a log group with the given `name`. pub fn start_log_group(&self, name: &str) { self.client.start_log_group(name) } + /// End a log group with the given `name`. pub fn end_log_group(&self, name: &str) { self.client.end_log_group(name) } + /// Post various forms of feedback. + /// + /// Use [`FeedbackInput`] to configure feedback behavior. + /// The given [`ClangVersions`] is used in thread comments, step summaries, and PR reviews' summary comment. pub async fn post_feedback( &mut self, files: &[Arc>], @@ -114,14 +136,15 @@ impl RestClient { self.client.write_file_annotations(&annotations)?; } if feedback_inputs.step_summary { - comment = Some(Self::make_comment( + let summary = Self::make_comment( files, format_checks_failed, tidy_checks_failed, &clang_versions, None, - )); - self.client.append_step_summary(comment.as_ref().unwrap())?; + )?; + self.client.append_step_summary(&summary)?; + comment = Some(summary); } let output_vars = [ OutputVariable { @@ -148,7 +171,7 @@ impl RestClient { tidy_checks_failed, &clang_versions, Some(65535), - )); + )?); } let options = ThreadCommentOptions { policy: if feedback_inputs.thread_comments == ThreadComments::Update { @@ -308,7 +331,7 @@ impl RestClient { tidy_checks_failed: u64, clang_versions: &ClangVersions, max_len: Option, - ) -> String { + ) -> Result { let mut comment = format!("{COMMENT_MARKER}# Cpp-Linter Report "); let mut remaining_length = max_len.unwrap_or(u64::MAX) - comment.len() as u64 - USER_OUTREACH.len() as u64; @@ -317,31 +340,33 @@ impl RestClient { let prompt = ":warning:\nSome files did not pass the configured checks!\n"; remaining_length -= prompt.len() as u64; comment.push_str(prompt); - if format_checks_failed > 0 { + if format_checks_failed > 0 + && let Some(format_version) = &clang_versions.format_version + { make_format_comment( files, &mut comment, format_checks_failed, - // format_version should be `Some()` value at this point. - &clang_versions.format_version.as_ref().unwrap().to_string(), + &format_version.to_string(), &mut remaining_length, - ); + )?; } - if tidy_checks_failed > 0 { + if tidy_checks_failed > 0 + && let Some(tidy_version) = &clang_versions.tidy_version + { make_tidy_comment( files, &mut comment, tidy_checks_failed, - // tidy_version should be `Some()` value at this point. - &clang_versions.tidy_version.as_ref().unwrap().to_string(), + &tidy_version.to_string(), &mut remaining_length, - ); + )?; } } else { comment.push_str(":heavy_check_mark:\nNo problems need attention."); } comment.push_str(USER_OUTREACH); - comment + Ok(comment) } } @@ -354,14 +379,16 @@ fn make_format_comment( format_checks_failed: u64, version_used: &String, remaining_length: &mut u64, -) { +) -> Result<(), ClientError> { let opener = format!( "\n
clang-format (v{version_used}) reports: {format_checks_failed} file(s) not formatted\n\n", ); let mut format_comment = String::new(); *remaining_length = remaining_length.saturating_sub(opener.len() as u64 + CLOSER.len() as u64); for file in files { - let file = file.lock().unwrap(); + let file = file + .lock() + .map_err(|e| ClientError::MutexPoisoned(e.to_string()))?; if let Some(format_advice) = &file.format_advice && !format_advice.replacements.is_empty() && *remaining_length > 0 @@ -384,6 +411,7 @@ fn make_format_comment( comment.push_str(&opener); comment.push_str(&format_comment); comment.push_str(CLOSER); + Ok(()) } fn make_tidy_comment( @@ -392,14 +420,16 @@ fn make_tidy_comment( tidy_checks_failed: u64, version_used: &String, remaining_length: &mut u64, -) { +) -> Result<(), ClientError> { let opener = format!( "\n
clang-tidy (v{version_used}) reports: {tidy_checks_failed} concern(s)\n\n" ); let mut tidy_comment = String::new(); *remaining_length = remaining_length.saturating_sub(opener.len() as u64 + CLOSER.len() as u64); for file in files { - let file = file.lock().unwrap(); + let file = file + .lock() + .map_err(|e| ClientError::MutexPoisoned(e.to_string()))?; if let Some(tidy_advice) = &file.tidy_advice { for tidy_note in &tidy_advice.notes { let file_path = PathBuf::from(&tidy_note.filename); @@ -432,10 +462,13 @@ fn make_tidy_comment( comment.push_str(&opener); comment.push_str(&tidy_comment); comment.push_str(CLOSER); + Ok(()) } #[cfg(all(test, feature = "bin"))] mod test { + #![allow(clippy::expect_used, clippy::unwrap_used)] + use std::{ default::Default, env, diff --git a/cpp-linter/src/run.rs b/cpp-linter/src/run.rs index 2aa5e6d0..0ec141e7 100644 --- a/cpp-linter/src/run.rs +++ b/cpp-linter/src/run.rs @@ -2,8 +2,8 @@ //! //! In python, this module is exposed as `cpp_linter.run` that has 1 function exposed: //! `main()`. -#![deny(clippy::unwrap_used)] #![cfg(feature = "bin")] + use std::{ env, path::PathBuf, From 9376992598ded1c7cf001742910ee5b6b6cf18e8 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Thu, 11 Jun 2026 13:48:11 -0700 Subject: [PATCH 2/2] consolidate a single added line into ranges correctly --- cpp-linter/src/common_fs.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/cpp-linter/src/common_fs.rs b/cpp-linter/src/common_fs.rs index cebffa57..f6bc50b8 100644 --- a/cpp-linter/src/common_fs.rs +++ b/cpp-linter/src/common_fs.rs @@ -90,6 +90,11 @@ impl FileObj { }; // lines.len() cannot be 0 at this point let last_index = lines.len() - 1; + if last_index == 0 { + // Single element case: push range and return + ranges.push(RangeInclusive::new(range_start, range_start)); + return ranges; + } for (index, number) in line_iter { // use let chain to avoid repeated lookup of lines[index - 1]. // should always yield some value since we entered the for loop at index 1. @@ -281,6 +286,14 @@ mod test { assert_eq!(ranges, vec![4..=5, 9..=9]); } + #[test] + fn get_ranges_single_added_line() { + let added_lines = vec![5]; + let file_obj = FileObj::from(PathBuf::from("tests/demo/demo.cpp"), added_lines, vec![]); + let ranges = file_obj.get_ranges(&LinesChangedOnly::On); + assert_eq!(ranges, vec![5..=5]); + } + #[test] fn line_not_in_diff() { let file_obj = FileObj::new(PathBuf::from("tests/demo/demo.cpp"));