From c1e04352bd220d2b912715aa07e1d04fe97d84f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Mon, 15 Aug 2022 20:30:30 +0200 Subject: [PATCH] unwrap_used and expect_used: trigger on uses of their _err variants --- clippy_lints/src/methods/expect_used.rs | 18 ++++++++++++----- clippy_lints/src/methods/mod.rs | 10 ++++++---- clippy_lints/src/methods/unwrap_used.rs | 18 ++++++++++++----- tests/ui/unwrap_expect_used.rs | 25 ++++++++++++++++++++++++ tests/ui/unwrap_expect_used.stderr | 26 ++++++++++++++++++++----- 5 files changed, 78 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/methods/expect_used.rs b/clippy_lints/src/methods/expect_used.rs index 5ef08ca62..d59fefa1d 100644 --- a/clippy_lints/src/methods/expect_used.rs +++ b/clippy_lints/src/methods/expect_used.rs @@ -7,18 +7,26 @@ use rustc_span::sym; use super::EXPECT_USED; -/// lint use of `expect()` for `Option`s and `Result`s -pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>, allow_expect_in_tests: bool) { +/// lint use of `expect()` or `expect_err` for `Result` and `expect()` for `Option`. +pub(super) fn check( + cx: &LateContext<'_>, + expr: &hir::Expr<'_>, + recv: &hir::Expr<'_>, + is_err: bool, + allow_expect_in_tests: bool, +) { let obj_ty = cx.typeck_results().expr_ty(recv).peel_refs(); - let mess = if is_type_diagnostic_item(cx, obj_ty, sym::Option) { + let mess = if is_type_diagnostic_item(cx, obj_ty, sym::Option) && !is_err { Some((EXPECT_USED, "an Option", "None", "")) } else if is_type_diagnostic_item(cx, obj_ty, sym::Result) { - Some((EXPECT_USED, "a Result", "Err", "an ")) + Some((EXPECT_USED, "a Result", if is_err { "Ok" } else { "Err" }, "an ")) } else { None }; + let method = if is_err { "expect_err" } else { "expect" }; + if allow_expect_in_tests && is_in_test_function(cx.tcx, expr.hir_id) { return; } @@ -28,7 +36,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr cx, lint, expr.span, - &format!("used `expect()` on `{kind}` value"), + &format!("used `{method}()` on `{kind}` value"), None, &format!("if this value is {none_prefix}`{none_value}`, it will panic"), ); diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 99aae5800..b68a2651e 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -174,7 +174,7 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks for `.unwrap()` calls on `Option`s and on `Result`s. + /// 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, @@ -224,7 +224,7 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks for `.expect()` calls on `Option`s and `Result`s. + /// Checks for `.expect()` or `.expect_err()` calls on `Result`s and `.expect()` call on `Option`s. /// /// ### Why is this bad? /// Usually it is better to handle the `None` or `Err` case. @@ -2740,8 +2740,9 @@ impl Methods { ("expect", [_]) => match method_call(recv) { Some(("ok", [recv], _)) => ok_expect::check(cx, expr, recv), Some(("err", [recv], err_span)) => err_expect::check(cx, expr, recv, self.msrv, span, err_span), - _ => expect_used::check(cx, expr, recv, self.allow_expect_in_tests), + _ => expect_used::check(cx, expr, recv, false, self.allow_expect_in_tests), }, + ("expect_err", [_]) => expect_used::check(cx, expr, recv, true, self.allow_expect_in_tests), ("extend", [arg]) => { string_extend_chars::check(cx, expr, recv, arg); extend_with_drain::check(cx, expr, recv, arg); @@ -2874,8 +2875,9 @@ impl Methods { }, _ => {}, } - unwrap_used::check(cx, expr, recv, self.allow_unwrap_in_tests); + unwrap_used::check(cx, expr, recv, false, self.allow_unwrap_in_tests); }, + ("unwrap_err", []) => unwrap_used::check(cx, expr, recv, true, self.allow_unwrap_in_tests), ("unwrap_or", [u_arg]) => match method_call(recv) { Some((arith @ ("checked_add" | "checked_sub" | "checked_mul"), [lhs, rhs], _)) => { manual_saturating_arithmetic::check(cx, expr, lhs, rhs, u_arg, &arith["checked_".len()..]); diff --git a/clippy_lints/src/methods/unwrap_used.rs b/clippy_lints/src/methods/unwrap_used.rs index ce1a52e54..05915c141 100644 --- a/clippy_lints/src/methods/unwrap_used.rs +++ b/clippy_lints/src/methods/unwrap_used.rs @@ -7,18 +7,26 @@ use rustc_span::sym; use super::{EXPECT_USED, UNWRAP_USED}; -/// lint use of `unwrap()` for `Option`s and `Result`s -pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>, allow_unwrap_in_tests: bool) { +/// lint use of `unwrap()` or `unwrap_err` for `Result` and `unwrap()` for `Option`. +pub(super) fn check( + cx: &LateContext<'_>, + expr: &hir::Expr<'_>, + recv: &hir::Expr<'_>, + is_err: bool, + allow_unwrap_in_tests: bool, +) { let obj_ty = cx.typeck_results().expr_ty(recv).peel_refs(); - let mess = if is_type_diagnostic_item(cx, obj_ty, sym::Option) { + let mess = if is_type_diagnostic_item(cx, obj_ty, sym::Option) && !is_err { Some((UNWRAP_USED, "an Option", "None", "")) } else if is_type_diagnostic_item(cx, obj_ty, sym::Result) { - Some((UNWRAP_USED, "a Result", "Err", "an ")) + Some((UNWRAP_USED, "a Result", if is_err { "Ok" } else { "Err" }, "an ")) } else { None }; + let method = if is_err { "unwrap_err" } else { "unwrap" }; + if allow_unwrap_in_tests && is_in_test_function(cx.tcx, expr.hir_id) { return; } @@ -37,7 +45,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr cx, lint, expr.span, - &format!("used `unwrap()` on `{kind}` value"), + &format!("used `{method}()` on `{kind}` value"), None, &help, ); diff --git a/tests/ui/unwrap_expect_used.rs b/tests/ui/unwrap_expect_used.rs index 0d4a0504a..9f27fef82 100644 --- a/tests/ui/unwrap_expect_used.rs +++ b/tests/ui/unwrap_expect_used.rs @@ -1,10 +1,35 @@ #![warn(clippy::unwrap_used, clippy::expect_used)] +trait OptionExt { + type Item; + + fn unwrap_err(self) -> Self::Item; + + fn expect_err(self, msg: &str) -> Self::Item; +} + +impl OptionExt for Option { + type Item = T; + fn unwrap_err(self) -> T { + panic!(); + } + + fn expect_err(self, msg: &str) -> T { + panic!(); + } +} + fn main() { Some(3).unwrap(); Some(3).expect("Hello world!"); + // Don't trigger on unwrap_err on an option + Some(3).unwrap_err(); + Some(3).expect_err("Hellow none!"); + let a: Result = Ok(3); a.unwrap(); a.expect("Hello world!"); + a.unwrap_err(); + a.expect_err("Hello error!"); } diff --git a/tests/ui/unwrap_expect_used.stderr b/tests/ui/unwrap_expect_used.stderr index f54bfd617..1a19459b2 100644 --- a/tests/ui/unwrap_expect_used.stderr +++ b/tests/ui/unwrap_expect_used.stderr @@ -1,5 +1,5 @@ error: used `unwrap()` on `an Option` value - --> $DIR/unwrap_expect_used.rs:4:5 + --> $DIR/unwrap_expect_used.rs:23:5 | LL | Some(3).unwrap(); | ^^^^^^^^^^^^^^^^ @@ -8,7 +8,7 @@ LL | Some(3).unwrap(); = help: if this value is `None`, it will panic error: used `expect()` on `an Option` value - --> $DIR/unwrap_expect_used.rs:5:5 + --> $DIR/unwrap_expect_used.rs:24:5 | LL | Some(3).expect("Hello world!"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -17,7 +17,7 @@ LL | Some(3).expect("Hello world!"); = help: if this value is `None`, it will panic error: used `unwrap()` on `a Result` value - --> $DIR/unwrap_expect_used.rs:8:5 + --> $DIR/unwrap_expect_used.rs:31:5 | LL | a.unwrap(); | ^^^^^^^^^^ @@ -25,12 +25,28 @@ LL | a.unwrap(); = help: if this value is an `Err`, it will panic error: used `expect()` on `a Result` value - --> $DIR/unwrap_expect_used.rs:9:5 + --> $DIR/unwrap_expect_used.rs:32:5 | LL | a.expect("Hello world!"); | ^^^^^^^^^^^^^^^^^^^^^^^^ | = help: if this value is an `Err`, it will panic -error: aborting due to 4 previous errors +error: used `unwrap_err()` on `a Result` value + --> $DIR/unwrap_expect_used.rs:33:5 + | +LL | a.unwrap_err(); + | ^^^^^^^^^^^^^^ + | + = help: if this value is an `Ok`, it will panic + +error: used `expect_err()` on `a Result` value + --> $DIR/unwrap_expect_used.rs:34:5 + | +LL | a.expect_err("Hello error!"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: if this value is an `Ok`, it will panic + +error: aborting due to 6 previous errors