refactor: use stricter linting rules (#353)

- 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`)

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
  * Added REST client helpers for debug checks and log-group start/end.

* **Bug Fixes / Improvements**
  * Safer comment generation with fallible error propagation.
* Filtered out invalid zero line numbers when computing added-line
ranges.
  * Tests adjusted to allow certain test-only unwrap/expect usage.

* **Documentation**
* Expanded Rustdoc comments across CLI, error types, and tooling
modules.

* **Refactor**
  * Hardened crate linting and made the git-related module internal.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Author: 2bndy5

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<RangeInclusive<u32>>,
 
+    /// A path to a cached file containing the full contents of the file after applying clang-format fixes.
     pub patched: PathBuf,
 }
 

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<TidyNotification>,
+
+    /// A buffer to hold the contents of the file after applying clang-tidy fixes.
     pub patched: Option<Vec<u8>>,
 }
 
@@ -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,

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<Version>,
 }
 
-/// 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,

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<Vec<String>>,
 
+    /// 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<CliCommand>,
 }

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<PathBuf>,
+
+    /// Extra arguments to pass to clang-tidy.
+    ///
+    /// Format of these strings follows the `-extra-arg` argument of clang-tidy.
     pub extra_args: Vec<String>,
+
+    /// 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<Vec<CompilationUnit>>,
+
+    /// 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<PathBuf>,
+
+    /// 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<PathBuf>,
+
+    /// An optional [`FileFilter`] to exclude files only from clang-tidy analysis.
     pub tidy_filter: Option<FileFilter>,
+
+    /// An optional [`FileFilter`] to exclude files only from clang-format analysis.
     pub format_filter: Option<FileFilter>,
+
+    /// 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};
 

cpp-linter/src/common_fs/mod.rs cpp-linter/src/common_fs.rscpp-linter/src/common_fs/mod.rs renamed to 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<u32>,
         diff_chunks: Vec<RangeInclusive<u32>>,
     ) -> Self {
+        // filter out any line numbers that are 0 since line numbers are always 1-indexed in diffs
+        let added_lines: Vec<NonZeroU32> = 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,38 @@ impl FileObj {
     /// A helper function to consolidate a [Vec<u32>] of line numbers into a
     /// [Vec<RangeInclusive<u32>>] in which each range describes the beginning and
     /// ending of a group of consecutive line numbers.
-    fn consolidate_numbers_to_ranges(lines: &[u32]) -> Vec<RangeInclusive<u32>> {
-        let mut range_start = None;
+    fn consolidate_numbers_to_ranges(lines: &[NonZeroU32]) -> Vec<RangeInclusive<u32>> {
         let mut ranges: Vec<RangeInclusive<u32>> = 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;
+        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.
+            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<RangeInclusive<u32>> {
         match lines_changed_only {
             LinesChangedOnly::Diff => self.diff_chunks.to_vec(),
@@ -265,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"));