From e707447a861739fa3e12e9c05c039c8c9c6b0544 Mon Sep 17 00:00:00 2001 From: Pavan Kumar Sunkara Date: Thu, 16 Feb 2023 11:32:12 +0000 Subject: [PATCH] Boilerplate for the new lint --- clippy_lints/src/methods/mod.rs | 52 +++++++++++++++++++ .../src/methods/unnecessary_literal_unwrap.rs | 30 +++++++++++ tests/ui/unnecessary_literal_unwrap.rs | 10 ++++ tests/ui/unwrap_literal.rs | 17 ------ 4 files changed, 92 insertions(+), 17 deletions(-) create mode 100644 clippy_lints/src/methods/unnecessary_literal_unwrap.rs create mode 100644 tests/ui/unnecessary_literal_unwrap.rs delete mode 100644 tests/ui/unwrap_literal.rs diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 88cbefbb5..f24f00ef6 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -93,6 +93,7 @@ mod unnecessary_fold; mod unnecessary_iter_cloned; mod unnecessary_join; mod unnecessary_lazy_eval; +mod unnecessary_literal_unwrap; mod unnecessary_sort_by; mod unnecessary_to_owned; mod unwrap_or_else_default; @@ -273,6 +274,56 @@ declare_clippy_lint! { "using `.unwrap()` on `Result` or `Option`, which should at least get a better message using `expect()`" } +declare_clippy_lint! { + /// ### What it does + /// Checks for `.unwrap()` or `.unwrap_err()` calls on `Result`s and `.unwrap()` call on `Option`s. + /// + /// ### Why is this bad? + /// It is better to handle the `None` or `Err` case, + /// or at least call `.expect(_)` with a more helpful message. Still, for a lot of + /// quick-and-dirty code, `unwrap` is a good choice, which is why this lint is + /// `Allow` by default. + /// + /// `result.unwrap()` will let the thread panic on `Err` values. + /// Normally, you want to implement more sophisticated error handling, + /// and propagate errors upwards with `?` operator. + /// + /// Even if you want to panic on errors, not all `Error`s implement good + /// messages on display. Therefore, it may be beneficial to look at the places + /// where they may get displayed. Activate this lint to do just that. + /// + /// ### Examples + /// ```rust + /// # let option = Some(1); + /// # let result: Result = Ok(1); + /// option.unwrap(); + /// result.unwrap(); + /// ``` + /// + /// Use instead: + /// ```rust + /// # let option = Some(1); + /// # let result: Result = Ok(1); + /// option.expect("more helpful message"); + /// result.expect("more helpful message"); + /// ``` + /// + /// If [expect_used](#expect_used) is enabled, instead: + /// ```rust,ignore + /// # let option = Some(1); + /// # let result: Result = Ok(1); + /// option?; + /// + /// // or + /// + /// result?; + /// ``` + #[clippy::version = "1.69.0"] + pub UNNECESSARY_LITERAL_UNWRAP, + complexity, + "checks for calls of `unwrap()` or `expect()` on `Some()` that cannot fail" +} + declare_clippy_lint! { /// ### What it does /// Checks for `.expect()` or `.expect_err()` calls on `Result`s and `.expect()` call on `Option`s. @@ -3814,6 +3865,7 @@ impl Methods { Some(("or", recv, [or_arg], or_span, _)) => { or_then_unwrap::check(cx, expr, recv, or_arg, or_span); }, + // unnecessary_literal_unwrap::check(cx, expr, recv); _ => {}, } unwrap_used::check(cx, expr, recv, false, self.allow_unwrap_in_tests); diff --git a/clippy_lints/src/methods/unnecessary_literal_unwrap.rs b/clippy_lints/src/methods/unnecessary_literal_unwrap.rs new file mode 100644 index 000000000..39b30b776 --- /dev/null +++ b/clippy_lints/src/methods/unnecessary_literal_unwrap.rs @@ -0,0 +1,30 @@ +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::ty::is_type_diagnostic_item; +use rustc_hir as hir; +use rustc_lint::LateContext; +use rustc_span::sym; + +use super::UNNECESSARY_LITERAL_UNWRAP; + +pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>) { + let obj_ty = cx.typeck_results().expr_ty(recv).peel_refs(); + + let mess = if is_type_diagnostic_item(cx, obj_ty, sym::Option) { + Some((UNNECESSARY_LITERAL_UNWRAP, "an `Option`", "None", "")) + } else { + None + }; + + if let Some((lint, kind, none_value, none_prefix)) = mess { + let help = format!("if this value is {none_prefix}`{none_value}`, it will panic"); + + span_lint_and_help( + cx, + lint, + expr.span, + &format!("used `unwrap()` on {kind} value"), + None, + &help, + ); + } +} diff --git a/tests/ui/unnecessary_literal_unwrap.rs b/tests/ui/unnecessary_literal_unwrap.rs new file mode 100644 index 000000000..9ae50517a --- /dev/null +++ b/tests/ui/unnecessary_literal_unwrap.rs @@ -0,0 +1,10 @@ +#![warn(clippy::unnecessary_literal_unwrap)] + +fn unwrap_option() { + let val = Some(1).unwrap(); + let val = Some(1).expect("this never happens"); +} + +fn main() { + unwrap_option(); +} diff --git a/tests/ui/unwrap_literal.rs b/tests/ui/unwrap_literal.rs deleted file mode 100644 index 917ef2126..000000000 --- a/tests/ui/unwrap_literal.rs +++ /dev/null @@ -1,17 +0,0 @@ -#![warn(clippy::unnecessary_unwrap)] - -fn unwrap_option() { - let val = Some(1).unwrap(); - let val = Some(1).expect("this never happens"); -} - -fn unwrap_result() { - let val = Ok(1).unwrap(); - let val = Err(1).unwrap_err(); - let val = Ok(1).expect("this never happens"); -} - -fn main() { - unwrap_option(); - unwrap_result(); -}