style: start phasing out .unwrap() calls
#242
Conversation
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
2bndy5
added
enhancement
This is the next step, but I'd rather do this in a separate PR because it involves adding a new dependency ( |
WalkthroughReplaces numerous unwraps with anyhow-based error handling, changes several tally functions to return Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @cpp-linter/src/cli/mod.rs:
- Line 1: The test currently calls cli.unwrap_err() which violates the
clippy::unwrap_used lint; change that call to cli.expect_err(...) so the failure
includes contextual message—replace the unwrap_err invocation in the failing
test (the cli variable usage around line 466) with expect_err and supply a short
explanatory message such as "expected error when providing invalid input" (or
similar) to document why an error is expected.
In @cpp-linter/src/run.rs:
- Line 5: The crate-level lint deny(clippy::unwrap_used) in run.rs should be
guarded from tests; prefix it with the same test guard used in main.rs by adding
#![cfg(not(test))] immediately above #![deny(clippy::unwrap_used)] so the
attribute becomes #![cfg(not(test))] followed by #![deny(clippy::unwrap_used)];
alternatively, if you prefer per-module control, add an inner attribute inside
the test module (e.g., #[cfg(test)] mod test { #![allow(clippy::unwrap_used)]
... }) to allow .unwrap()/.unwrap_err() in the tests.
🧹 Nitpick comments (3)
cpp-linter/src/cli/mod.rs (1)
442-454: Consider a clearer refactoring to avoid iterator state issues.The let-chain at line 442-443 avoids the unwrap, but introduces a subtle edge case: if
args.len() == 1butval.next()returnsNone(theoretically impossible), the else branch at line 453 would collect from an already-advanced iterator, returning an empty vector instead of the expected result.♻️ Cleaner alternative that avoids iterator state tracking
pub fn convert_extra_arg_val(args: &[String]) -> Vec<String> { - let mut val = args.iter(); - if args.len() == 1 - && let Some(v) = val.next() - { + if args.len() == 1 { // specified once; split and return result - v.trim_matches('\'') + args[0].trim_matches('\'') .trim_matches('"') .split(' ') .map(|i| i.to_string()) .collect() } else { // specified multiple times; just return a clone of the values - val.map(|i| i.to_string()).collect() + args.iter().map(|i| i.to_string()).collect() } }This eliminates the intermediate
valiterator and makes the logic more straightforward.cpp-linter/src/clang_tools/clang_tidy.rs (1)
355-368: Stale comment on line 365.The comment "original_content is guaranteed to be Some() value at this point" is now misleading. With the
if let Some(original_content) = &original_contentpattern on line 356,original_contentis already the inner&String, not anOption. Consider removing or updating this comment.♻️ Suggested fix
} - // original_content is guaranteed to be Some() value at this point fs::write(&file_name, original_content)cpp-linter/src/clang_tools/mod.rs (1)
460-465: Consider replacing.expect()with error propagation for consistency.Line 463 uses
.expect()which could panic ifold_lineno()returnsNone. While this is logically guarded (removed lines should have line numbers), using.ok_or()with?would be more consistent with the PR's goal of eliminating panic-inducing calls.Note:
clippy::unwrap_useddoes not cover.expect()- that requiresclippy::expect_used.♻️ Suggested fix
} else { removed.push( diff_line .old_lineno() - .expect("Removed line should have a line number"), + .ok_or(anyhow!("Removed line should have a line number"))?, ); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cpp-linter/src/clang_tools/clang_format.rscpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/clang_tools/mod.rscpp-linter/src/cli/mod.rscpp-linter/src/logger.rscpp-linter/src/main.rscpp-linter/src/rest_api/github/mod.rscpp-linter/src/run.rs
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 54
File: cpp-linter/src/rest_api/mod.rs:253-254
Timestamp: 2024-11-23T06:20:11.698Z
Learning: In `cpp-linter/src/rest_api/mod.rs`, it is safe to use `unwrap()` on `clang_versions.tidy_version` and `clang_versions.format_version` because they are guaranteed to be `Some` at that point in the code.
📚 Learning: 2025-11-04T06:50:10.870Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 208
File: cpp-linter/src/clang_tools/mod.rs:60-115
Timestamp: 2025-11-04T06:50:10.870Z
Learning: In the cpp-linter-rs project, path validation (such as checking whether a path is a file or directory) should be performed in CLI parsing (cpp-linter/src/cli/structs.rs) rather than in the tool lookup logic (cpp-linter/src/clang_tools/mod.rs). This maintains proper separation of concerns.
Applied to files:
cpp-linter/src/cli/mod.rscpp-linter/src/run.rscpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/logger.rscpp-linter/src/clang_tools/clang_format.rscpp-linter/src/clang_tools/mod.rs
📚 Learning: 2024-11-23T06:20:11.698Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 54
File: cpp-linter/src/rest_api/mod.rs:253-254
Timestamp: 2024-11-23T06:20:11.698Z
Learning: In `cpp-linter/src/rest_api/mod.rs`, it is safe to use `unwrap()` on `clang_versions.tidy_version` and `clang_versions.format_version` because they are guaranteed to be `Some` at that point in the code.
Applied to files:
cpp-linter/src/cli/mod.rscpp-linter/src/run.rscpp-linter/src/main.rscpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/logger.rscpp-linter/src/clang_tools/clang_format.rscpp-linter/src/rest_api/github/mod.rscpp-linter/src/clang_tools/mod.rs
📚 Learning: 2024-10-02T07:55:08.948Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 52
File: cpp-linter/src/clang_tools/mod.rs:146-146
Timestamp: 2024-10-02T07:55:08.948Z
Learning: In the `cpp-linter` project, the function `capture_clang_tools_output` is only called in `cpp-linter/src/run.rs`, and there are no other calls to this function elsewhere in the codebase.
Applied to files:
cpp-linter/src/run.rscpp-linter/src/main.rscpp-linter/src/clang_tools/mod.rs
📚 Learning: 2025-01-21T09:56:32.771Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 101
File: cpp-linter/src/clang_tools/clang_format.rs:155-161
Timestamp: 2025-01-21T09:56:32.771Z
Learning: In cpp-linter-rs, the XML output being parsed is generated programmatically by clang-format tool. The only failure case for XML parsing is when clang-format produces a blank XML document, in which case falling back to empty results (using unwrap_or) is the desired behavior.
Applied to files:
cpp-linter/src/run.rscpp-linter/src/main.rscpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/clang_tools/clang_format.rscpp-linter/src/clang_tools/mod.rs
📚 Learning: 2025-12-11T20:19:13.754Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 224
File: cpp-linter/src/clang_tools/clang_format.rs:103-139
Timestamp: 2025-12-11T20:19:13.754Z
Learning: In cpp-linter-rs, run_clang_format in cpp-linter/src/clang_tools/clang_format.rs must execute two clang-format commands sequentially when format_review is enabled: first to get formatted output, then to get XML replacements based on the original unformatted content. Concurrent execution would create a race condition because the commands operate on different file states. This sequential pattern is intentional and necessary given clang-format's capabilities.
Applied to files:
cpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/clang_tools/clang_format.rscpp-linter/src/clang_tools/mod.rs
📚 Learning: 2024-11-23T14:10:31.760Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 54
File: cpp-linter/src/clang_tools/mod.rs:149-159
Timestamp: 2024-11-23T14:10:31.760Z
Learning: Clang tools' `--version` output is consistent across all released versions, so adding extra error handling or unit tests for version extraction may be unnecessary. Tests are run after the clang tools are installed.
Applied to files:
cpp-linter/src/clang_tools/mod.rs
🧬 Code graph analysis (3)
cpp-linter/src/clang_tools/clang_tidy.rs (2)
cpp-linter/src/common_fs/file_filter.rs (1)
new(17-24)cpp-linter/src/common_fs/mod.rs (1)
new(44-53)
cpp-linter/src/clang_tools/clang_format.rs (1)
cpp-linter/src/common_fs/mod.rs (1)
new(44-53)
cpp-linter/src/rest_api/github/mod.rs (2)
cpp-linter/src/clang_tools/clang_tidy.rs (1)
tally_tidy_advice(252-268)cpp-linter/src/clang_tools/clang_format.rs (1)
tally_format_advice(71-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build-mkdocs
- GitHub Check: x86_64-pc-windows-msvc
- GitHub Check: aarch64-unknown-linux-musl
- GitHub Check: armv7-unknown-linux-gnueabihf
- GitHub Check: powerpc-unknown-linux-gnu
- GitHub Check: x86_64-unknown-linux-musl
- GitHub Check: powerpc64-unknown-linux-gnu
- GitHub Check: arm-unknown-linux-gnueabihf
- GitHub Check: powerpc64le-unknown-linux-gnu
- GitHub Check: s390x-unknown-linux-gnu
- GitHub Check: arm-unknown-linux-gnueabi
- GitHub Check: x86_64-unknown-linux-gnu
- GitHub Check: test (windows-latest)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: Build armv7-unknown-linux-gnueabihf
- GitHub Check: Build aarch64-unknown-linux-musl
- GitHub Check: Build x86_64-unknown-linux-gnu
- GitHub Check: Build aarch64-unknown-linux-gnu
- GitHub Check: Build x86_64-unknown-linux-musl
- GitHub Check: Build FreeBSD
🔇 Additional comments (17)
cpp-linter/src/main.rs (1)
2-2: LGTM!Adding the
deny(clippy::unwrap_used)lint enforces the PR objective at compile time. Since this module is already guarded by#![cfg(not(test))]at line 1, test code remains unaffected.cpp-linter/src/logger.rs (1)
51-61: LGTM! Clean refactoring to eliminate unwrap.The refactoring successfully replaces the pattern of checking
module.is_some()followed bymodule.unwrap()with a directlet Some(module) = modulebinding. This is more idiomatic and safer. Note that line 57'sunwrap_or_default()is acceptable as it provides a fallback value rather than panicking.cpp-linter/src/rest_api/github/mod.rs (1)
187-188: LGTM! Proper error propagation for mutex lock failures.The
?operator correctly propagates errors fromtally_tidy_adviceandtally_format_advice, which now returnResult<u64>and can fail when acquiring mutex locks on source files. This aligns with the PR's objective of replacing unwrap calls with explicit error handling.cpp-linter/src/clang_tools/clang_tidy.rs (6)
13-13: LGTM!The import of
anyhow,Context, andResultis correctly structured for the error handling pattern used throughout this file.
151-162: LGTM!Good use of
with_contextfor RegExp compilation, current directory access, and UTF-8 conversion. The error messages are clear and will help diagnose issues if they occur.
204-210: LGTM!Clean refactor using let-chains to combine the
is_absolute()check withstrip_prefix(). This ensures the path is only modified when both conditions are satisfied.
252-267: LGTM!The signature change to
Result<u64>is consistent withtally_format_advicein clang_format.rs. The mutex lock error handling provides a clear error message rather than panicking.
275-279: LGTM!Safe handling of the optional clang-tidy command path using
ok_orwith a descriptive error message.
329-334: LGTM!Good error context for command execution failures. The error message includes the file name, which helps identify which file caused the issue.
cpp-linter/src/clang_tools/clang_format.rs (4)
10-10: LGTM!Import structure matches the pattern used in clang_tidy.rs.
56-68: LGTM!Clean refactor using an iterator to capitalize the first letter. The logic correctly handles all style cases: specific named styles get capitalized first letter,
llvm/gnubecome fully uppercase, and anything else returns "Custom".
71-84: LGTM!Consistent with
tally_tidy_advice- the mutex lock error handling andResultreturn type follow the same pattern established in clang_tidy.rs.
91-95: LGTM!Safe extraction of the clang-format command path with a clear error message, consistent with the pattern in clang_tidy.rs.
cpp-linter/src/clang_tools/mod.rs (4)
1-1: Good enforcement of the PR's goal.The
#![deny(clippy::unwrap_used)]lint ensures that any newunwrap()calls will cause compilation failures, preventing regression. This aligns well with the PR's objective of phasing out.unwrap()calls.
64-69: LGTM!Good addition of context for
current_dir()failures. The error message will help users understand if the issue is with the working directory vs. the tool not being found.
124-135: LGTM!Safe handling of the regex capture group with
ok_or. This prevents a potential panic if the regex matches but the capture group is somehow missing.
467-492: LGTM!The suggestion construction logic is well-organized. The handling of removal-only hunks (empty suggestion with removed lines) vs. normal suggestions is clear. The de-duplication via
is_comment_in_suggestionscorrectly merges suggestions for the same line range.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #242 +/- ##
==========================================
- Coverage 97.00% 96.88% -0.12%
==========================================
Files 14 14
Lines 3100 3118 +18
==========================================
+ Hits 3007 3021 +14
- Misses 93 97 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.Summary by CodeRabbit
Bug Fixes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.