Auto merge of rust-lang#9338 - sgued:9331-unwrap-err-used, r=giraffate

unwrap_used and expect_used: trigger on uses of their _err variants

changelog: [`unwrap_used`]: lint uses of `unwrap_err`
changelog: [`expect_used`]: lint uses of `expect_err`

fixes rust-lang#9331

Author: bors

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"),
         );

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()..]);

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_suffix = if is_err { "_err" } else { "" };
+
     if allow_unwrap_in_tests && is_in_test_function(cx.tcx, expr.hir_id) {
         return;
     }
@@ -27,7 +35,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr
         let help = if is_lint_allowed(cx, EXPECT_USED, expr.hir_id) {
             format!(
                 "if you don't want to handle the `{none_value}` case gracefully, consider \
-                using `expect()` to provide a better panic message"
+                using `expect{method_suffix}()` to provide a better panic message"
             )
         } else {
             format!("if this value is {none_prefix}`{none_value}`, it will panic")
@@ -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 `unwrap{method_suffix}()` on `{kind}` value"),
             None,
             &help,
         );

tests/ui/expect.rs

@@ -6,8 +6,9 @@ fn expect_option() {
 }
 
 fn expect_result() {
-    let res: Result<u8, ()> = Ok(0);
+    let res: Result<u8, u8> = Ok(0);
     let _ = res.expect("");
+    let _ = res.expect_err("");
 }
 
 fn main() {

tests/ui/expect.stderr

@@ -15,5 +15,13 @@ LL |     let _ = res.expect("");
    |
    = help: if this value is an `Err`, it will panic
 
-error: aborting due to 2 previous errors
+error: used `expect_err()` on `a Result` value
+  --> $DIR/expect.rs:11:13
+   |
+LL |     let _ = res.expect_err("");
+   |             ^^^^^^^^^^^^^^^^^^
+   |
+   = help: if this value is an `Ok`, it will panic
+
+error: aborting due to 3 previous errors
 

tests/ui/unwrap.rs

@@ -6,8 +6,9 @@ fn unwrap_option() {
 }
 
 fn unwrap_result() {
-    let res: Result<u8, ()> = Ok(0);
+    let res: Result<u8, u8> = Ok(0);
     let _ = res.unwrap();
+    let _ = res.unwrap_err();
 }
 
 fn main() {

tests/ui/unwrap.stderr

@@ -15,5 +15,13 @@ LL |     let _ = res.unwrap();
    |
    = help: if you don't want to handle the `Err` case gracefully, consider using `expect()` to provide a better panic message
 
-error: aborting due to 2 previous errors
+error: used `unwrap_err()` on `a Result` value
+  --> $DIR/unwrap.rs:11:13
+   |
+LL |     let _ = res.unwrap_err();
+   |             ^^^^^^^^^^^^^^^^
+   |
+   = help: if you don't want to handle the `Ok` case gracefully, consider using `expect_err()` to provide a better panic message
+
+error: aborting due to 3 previous errors
 

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<T> OptionExt for Option<T> {
+    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<i32, i32> = Ok(3);
     a.unwrap();
     a.expect("Hello world!");
+    a.unwrap_err();
+    a.expect_err("Hello error!");
 }

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,20 +17,36 @@ 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();
    |     ^^^^^^^^^^
    |
    = 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