Merge pull request #2837 from fanzier/panicking_unwrap

Implement lint checking for `unwrap`s that will always panic.

clippy_lints/src/lib.rs

@@ -941,6 +941,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         mutex_atomic::MUTEX_INTEGER,
         needless_borrow::NEEDLESS_BORROW,
         ranges::RANGE_PLUS_ONE,
+        unwrap::PANICKING_UNWRAP,
         unwrap::UNNECESSARY_UNWRAP,
     ]);
 }

clippy_lints/src/unwrap.rs

@@ -32,6 +32,27 @@ declare_clippy_lint! {
     "checks for calls of unwrap[_err]() that cannot fail"
 }
 
+/// **What it does:** Checks for calls of `unwrap[_err]()` that will always fail.
+///
+/// **Why is this bad?** If panicking is desired, an explicit `panic!()` should be used.
+///
+/// **Known problems:** This lint only checks `if` conditions not assignments.
+/// So something like `let x: Option<()> = None; x.unwrap();` will not be recognized.
+///
+/// **Example:**
+/// ```rust
+/// if option.is_none() {
+///     do_something_with(option.unwrap())
+/// }
+/// ```
+///
+/// This code will always panic. The if condition should probably be inverted.
+declare_clippy_lint! {
+    pub PANICKING_UNWRAP,
+    nursery,
+    "checks for calls of unwrap[_err]() that will always fail"
+}
+
 pub struct Pass;
 
 /// Visitor that keeps track of which variables are unwrappable.
@@ -124,17 +145,28 @@ impl<'a, 'tcx: 'a> Visitor<'tcx> for UnwrappableVariablesVisitor<'a, 'tcx> {
                 if ["unwrap", "unwrap_err"].contains(&&*method_name.name.as_str());
                 let call_to_unwrap = method_name.name == "unwrap";
                 if let Some(unwrappable) = self.unwrappables.iter()
-                    .find(|u| u.ident.def == path.def && call_to_unwrap == u.safe_to_unwrap);
+                    .find(|u| u.ident.def == path.def);
                 then {
-                    span_lint_and_then(
+                    if call_to_unwrap == unwrappable.safe_to_unwrap {
-                        self.cx,
+                        span_lint_and_then(
-                        UNNECESSARY_UNWRAP,
+                            self.cx,
-                        expr.span,
+                            UNNECESSARY_UNWRAP,
-                        &format!("You checked before that `{}()` cannot fail. \
+                            expr.span,
-                        Instead of checking and unwrapping, it's better to use `if let` or `match`.",
+                            &format!("You checked before that `{}()` cannot fail. \
-                        method_name.name),
+                            Instead of checking and unwrapping, it's better to use `if let` or `match`.",
-                        |db| { db.span_label(unwrappable.check.span, "the check is happening here"); },
+                            method_name.name),
-                    );
+                            |db| { db.span_label(unwrappable.check.span, "the check is happening here"); },
+                        );
+                    } else {
+                        span_lint_and_then(
+                            self.cx,
+                            PANICKING_UNWRAP,
+                            expr.span,
+                            &format!("This call to `{}()` will always panic.",
+                            method_name.name),
+                            |db| { db.span_label(unwrappable.check.span, "because of this check"); },
+                        );
+                    }
                 }
             }
             walk_expr(self, expr);
@@ -148,7 +180,7 @@ impl<'a, 'tcx: 'a> Visitor<'tcx> for UnwrappableVariablesVisitor<'a, 'tcx> {
 
 impl<'a> LintPass for Pass {
     fn get_lints(&self) -> LintArray {
-        lint_array!(UNNECESSARY_UNWRAP)
+        lint_array!(PANICKING_UNWRAP, UNNECESSARY_UNWRAP)
     }
 }
 

tests/ui/checked_unwrap.rs

@@ -1,32 +1,41 @@
-#![deny(unnecessary_unwrap)]
+#![deny(panicking_unwrap, unnecessary_unwrap)]
+#![allow(if_same_then_else)]
 
 fn main() {
     let x = Some(());
     if x.is_some() {
-        x.unwrap();
+        x.unwrap(); // unnecessary
+    } else {
+        x.unwrap(); // will panic
     }
     if x.is_none() {
-        // nothing to do here
+        x.unwrap(); // will panic
     } else {
-        x.unwrap();
+        x.unwrap(); // unnecessary
     }
     let mut x: Result<(), ()> = Ok(());
     if x.is_ok() {
-        x.unwrap();
+        x.unwrap(); // unnecessary
+        x.unwrap_err(); // will panic
     } else {
-        x.unwrap_err();
+        x.unwrap(); // will panic
+        x.unwrap_err(); // unnecessary
     }
     if x.is_err() {
-        x.unwrap_err();
+        x.unwrap(); // will panic
+        x.unwrap_err(); // unnecessary
     } else {
-        x.unwrap();
+        x.unwrap(); // unnecessary
+        x.unwrap_err(); // will panic
     }
     if x.is_ok() {
         x = Err(());
-        x.unwrap();
+        x.unwrap(); // not unnecessary because of mutation of x
+        // it will always panic but the lint is not smart enoguh to see this (it only checks if conditions).
     } else {
         x = Ok(());
-        x.unwrap_err();
+        x.unwrap_err(); // not unnecessary because of mutation of x
+        // it will always panic but the lint is not smart enoguh to see this (it only checks if conditions).
     }
 }
 
@@ -34,34 +43,49 @@ fn test_complex_conditions() {
     let x: Result<(), ()> = Ok(());
     let y: Result<(), ()> = Ok(());
     if x.is_ok() && y.is_err() {
-        x.unwrap();
+        x.unwrap(); // unnecessary
-        y.unwrap_err();
+        x.unwrap_err(); // will panic
+        y.unwrap(); // will panic
+        y.unwrap_err(); // unnecessary
     } else {
-        // not clear whether unwrappable:
+        // not statically determinable whether any of the following will always succeed or always fail:
+        x.unwrap();
         x.unwrap_err();
         y.unwrap();
+        y.unwrap_err();
     }
 
     if x.is_ok() || y.is_ok() {
-        // not clear whether unwrappable:
+        // not statically determinable whether any of the following will always succeed or always fail:
         x.unwrap();
         y.unwrap();
     } else {
-        x.unwrap_err();
+        x.unwrap(); // will panic
-        y.unwrap_err();
+        x.unwrap_err(); // unnecessary
+        y.unwrap(); // will panic
+        y.unwrap_err(); // unnecessary
     }
     let z: Result<(), ()> = Ok(());
     if x.is_ok() && !(y.is_ok() || z.is_err()) {
-        x.unwrap();
+        x.unwrap(); // unnecessary
-        y.unwrap_err();
+        x.unwrap_err(); // will panic
-        z.unwrap();
+        y.unwrap(); // will panic
+        y.unwrap_err(); // unnecessary
+        z.unwrap(); // unnecessary
+        z.unwrap_err(); // will panic
     }
     if x.is_ok() || !(y.is_ok() && z.is_err()) {
-        // not clear what's unwrappable
+        // not statically determinable whether any of the following will always succeed or always fail:
-    } else {
+        x.unwrap();
-        x.unwrap_err();
         y.unwrap();
-        z.unwrap_err();
+        z.unwrap();
+    } else {
+        x.unwrap(); // will panic
+        x.unwrap_err(); // unnecessary
+        y.unwrap(); // unnecessary
+        y.unwrap_err(); // will panic
+        z.unwrap(); // will panic
+        z.unwrap_err(); // unnecessary
     }
 }
 
@@ -69,7 +93,9 @@ fn test_nested() {
     fn nested() {
         let x = Some(());
         if x.is_some() {
-            x.unwrap();
+            x.unwrap(); // unnecessary
+        } else {
+            x.unwrap(); // will panic
         }
     }
 }

tests/ui/checked_unwrap.stderr

@@ -1,155 +1,313 @@
 error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
- --> $DIR/checked_unwrap.rs:6:9
+ --> $DIR/checked_unwrap.rs:7:9
   |
-5 |     if x.is_some() {
+6 |     if x.is_some() {
   |        ----------- the check is happening here
-6 |         x.unwrap();
+7 |         x.unwrap(); // unnecessary
+  |         ^^^^^^^^^^
+  |
+note: lint level defined here
+ --> $DIR/checked_unwrap.rs:1:27
+  |
+1 | #![deny(panicking_unwrap, unnecessary_unwrap)]
+  |                           ^^^^^^^^^^^^^^^^^^
+
+error: This call to `unwrap()` will always panic.
+ --> $DIR/checked_unwrap.rs:9:9
+  |
+6 |     if x.is_some() {
+  |        ----------- because of this check
+...
+9 |         x.unwrap(); // will panic
   |         ^^^^^^^^^^
   |
 note: lint level defined here
  --> $DIR/checked_unwrap.rs:1:9
   |
-1 | #![deny(unnecessary_unwrap)]
+1 | #![deny(panicking_unwrap, unnecessary_unwrap)]
-  |         ^^^^^^^^^^^^^^^^^^
+  |         ^^^^^^^^^^^^^^^^
+
+error: This call to `unwrap()` will always panic.
+  --> $DIR/checked_unwrap.rs:12:9
+   |
+11 |     if x.is_none() {
+   |        ----------- because of this check
+12 |         x.unwrap(); // will panic
+   |         ^^^^^^^^^^
 
 error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
-  --> $DIR/checked_unwrap.rs:11:9
+  --> $DIR/checked_unwrap.rs:14:9
    |
-8  |     if x.is_none() {
+11 |     if x.is_none() {
    |        ----------- the check is happening here
 ...
-11 |         x.unwrap();
+14 |         x.unwrap(); // unnecessary
    |         ^^^^^^^^^^
 
 error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
-  --> $DIR/checked_unwrap.rs:15:9
+  --> $DIR/checked_unwrap.rs:18:9
    |
-14 |     if x.is_ok() {
+17 |     if x.is_ok() {
    |        --------- the check is happening here
-15 |         x.unwrap();
+18 |         x.unwrap(); // unnecessary
    |         ^^^^^^^^^^
 
-error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
+error: This call to `unwrap_err()` will always panic.
-  --> $DIR/checked_unwrap.rs:17:9
+  --> $DIR/checked_unwrap.rs:19:9
    |
-14 |     if x.is_ok() {
+17 |     if x.is_ok() {
-   |        --------- the check is happening here
+   |        --------- because of this check
+18 |         x.unwrap(); // unnecessary
+19 |         x.unwrap_err(); // will panic
+   |         ^^^^^^^^^^^^^^
+
+error: This call to `unwrap()` will always panic.
+  --> $DIR/checked_unwrap.rs:21:9
+   |
+17 |     if x.is_ok() {
+   |        --------- because of this check
 ...
-17 |         x.unwrap_err();
+21 |         x.unwrap(); // will panic
-   |         ^^^^^^^^^^^^^^
+   |         ^^^^^^^^^^
 
 error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
-  --> $DIR/checked_unwrap.rs:20:9
-   |
-19 |     if x.is_err() {
-   |        ---------- the check is happening here
-20 |         x.unwrap_err();
-   |         ^^^^^^^^^^^^^^
-
-error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
   --> $DIR/checked_unwrap.rs:22:9
    |
-19 |     if x.is_err() {
+17 |     if x.is_ok() {
+   |        --------- the check is happening here
+...
+22 |         x.unwrap_err(); // unnecessary
+   |         ^^^^^^^^^^^^^^
+
+error: This call to `unwrap()` will always panic.
+  --> $DIR/checked_unwrap.rs:25:9
+   |
+24 |     if x.is_err() {
+   |        ---------- because of this check
+25 |         x.unwrap(); // will panic
+   |         ^^^^^^^^^^
+
+error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
+  --> $DIR/checked_unwrap.rs:26:9
+   |
+24 |     if x.is_err() {
+   |        ---------- the check is happening here
+25 |         x.unwrap(); // will panic
+26 |         x.unwrap_err(); // unnecessary
+   |         ^^^^^^^^^^^^^^
+
+error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
+  --> $DIR/checked_unwrap.rs:28:9
+   |
+24 |     if x.is_err() {
    |        ---------- the check is happening here
 ...
-22 |         x.unwrap();
+28 |         x.unwrap(); // unnecessary
    |         ^^^^^^^^^^
 
-error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
+error: This call to `unwrap_err()` will always panic.
-  --> $DIR/checked_unwrap.rs:37:9
+  --> $DIR/checked_unwrap.rs:29:9
    |
-36 |     if x.is_ok() && y.is_err() {
+24 |     if x.is_err() {
+   |        ---------- because of this check
+...
+29 |         x.unwrap_err(); // will panic
+   |         ^^^^^^^^^^^^^^
+
+error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
+  --> $DIR/checked_unwrap.rs:46:9
+   |
+45 |     if x.is_ok() && y.is_err() {
    |        --------- the check is happening here
-37 |         x.unwrap();
+46 |         x.unwrap(); // unnecessary
+   |         ^^^^^^^^^^
+
+error: This call to `unwrap_err()` will always panic.
+  --> $DIR/checked_unwrap.rs:47:9
+   |
+45 |     if x.is_ok() && y.is_err() {
+   |        --------- because of this check
+46 |         x.unwrap(); // unnecessary
+47 |         x.unwrap_err(); // will panic
+   |         ^^^^^^^^^^^^^^
+
+error: This call to `unwrap()` will always panic.
+  --> $DIR/checked_unwrap.rs:48:9
+   |
+45 |     if x.is_ok() && y.is_err() {
+   |                     ---------- because of this check
+...
+48 |         y.unwrap(); // will panic
    |         ^^^^^^^^^^
 
 error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
-  --> $DIR/checked_unwrap.rs:38:9
+  --> $DIR/checked_unwrap.rs:49:9
    |
-36 |     if x.is_ok() && y.is_err() {
+45 |     if x.is_ok() && y.is_err() {
    |                     ---------- the check is happening here
-37 |         x.unwrap();
-38 |         y.unwrap_err();
-   |         ^^^^^^^^^^^^^^
-
-error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
-  --> $DIR/checked_unwrap.rs:50:9
-   |
-45 |     if x.is_ok() || y.is_ok() {
-   |        --------- the check is happening here
 ...
-50 |         x.unwrap_err();
+49 |         y.unwrap_err(); // unnecessary
    |         ^^^^^^^^^^^^^^
 
-error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
+error: This call to `unwrap()` will always panic.
-  --> $DIR/checked_unwrap.rs:51:9
-   |
-45 |     if x.is_ok() || y.is_ok() {
-   |                     --------- the check is happening here
-...
-51 |         y.unwrap_err();
-   |         ^^^^^^^^^^^^^^
-
-error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
-  --> $DIR/checked_unwrap.rs:55:9
-   |
-54 |     if x.is_ok() && !(y.is_ok() || z.is_err()) {
-   |        --------- the check is happening here
-55 |         x.unwrap();
-   |         ^^^^^^^^^^
-
-error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
-  --> $DIR/checked_unwrap.rs:56:9
-   |
-54 |     if x.is_ok() && !(y.is_ok() || z.is_err()) {
-   |                       --------- the check is happening here
-55 |         x.unwrap();
-56 |         y.unwrap_err();
-   |         ^^^^^^^^^^^^^^
-
-error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
-  --> $DIR/checked_unwrap.rs:57:9
-   |
-54 |     if x.is_ok() && !(y.is_ok() || z.is_err()) {
-   |                                    ---------- the check is happening here
-...
-57 |         z.unwrap();
-   |         ^^^^^^^^^^
-
-error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
-  --> $DIR/checked_unwrap.rs:62:9
-   |
-59 |     if x.is_ok() || !(y.is_ok() && z.is_err()) {
-   |        --------- the check is happening here
-...
-62 |         x.unwrap_err();
-   |         ^^^^^^^^^^^^^^
-
-error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
   --> $DIR/checked_unwrap.rs:63:9
    |
-59 |     if x.is_ok() || !(y.is_ok() && z.is_err()) {
+58 |     if x.is_ok() || y.is_ok() {
-   |                       --------- the check is happening here
+   |        --------- because of this check
 ...
-63 |         y.unwrap();
+63 |         x.unwrap(); // will panic
    |         ^^^^^^^^^^
 
 error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
   --> $DIR/checked_unwrap.rs:64:9
    |
-59 |     if x.is_ok() || !(y.is_ok() && z.is_err()) {
+58 |     if x.is_ok() || y.is_ok() {
-   |                                    ---------- the check is happening here
+   |        --------- the check is happening here
 ...
-64 |         z.unwrap_err();
+64 |         x.unwrap_err(); // unnecessary
+   |         ^^^^^^^^^^^^^^
+
+error: This call to `unwrap()` will always panic.
+  --> $DIR/checked_unwrap.rs:65:9
+   |
+58 |     if x.is_ok() || y.is_ok() {
+   |                     --------- because of this check
+...
+65 |         y.unwrap(); // will panic
+   |         ^^^^^^^^^^
+
+error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
+  --> $DIR/checked_unwrap.rs:66:9
+   |
+58 |     if x.is_ok() || y.is_ok() {
+   |                     --------- the check is happening here
+...
+66 |         y.unwrap_err(); // unnecessary
    |         ^^^^^^^^^^^^^^
 
 error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
-  --> $DIR/checked_unwrap.rs:72:13
+  --> $DIR/checked_unwrap.rs:70:9
    |
-71 |         if x.is_some() {
+69 |     if x.is_ok() && !(y.is_ok() || z.is_err()) {
+   |        --------- the check is happening here
+70 |         x.unwrap(); // unnecessary
+   |         ^^^^^^^^^^
+
+error: This call to `unwrap_err()` will always panic.
+  --> $DIR/checked_unwrap.rs:71:9
+   |
+69 |     if x.is_ok() && !(y.is_ok() || z.is_err()) {
+   |        --------- because of this check
+70 |         x.unwrap(); // unnecessary
+71 |         x.unwrap_err(); // will panic
+   |         ^^^^^^^^^^^^^^
+
+error: This call to `unwrap()` will always panic.
+  --> $DIR/checked_unwrap.rs:72:9
+   |
+69 |     if x.is_ok() && !(y.is_ok() || z.is_err()) {
+   |                       --------- because of this check
+...
+72 |         y.unwrap(); // will panic
+   |         ^^^^^^^^^^
+
+error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
+  --> $DIR/checked_unwrap.rs:73:9
+   |
+69 |     if x.is_ok() && !(y.is_ok() || z.is_err()) {
+   |                       --------- the check is happening here
+...
+73 |         y.unwrap_err(); // unnecessary
+   |         ^^^^^^^^^^^^^^
+
+error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
+  --> $DIR/checked_unwrap.rs:74:9
+   |
+69 |     if x.is_ok() && !(y.is_ok() || z.is_err()) {
+   |                                    ---------- the check is happening here
+...
+74 |         z.unwrap(); // unnecessary
+   |         ^^^^^^^^^^
+
+error: This call to `unwrap_err()` will always panic.
+  --> $DIR/checked_unwrap.rs:75:9
+   |
+69 |     if x.is_ok() && !(y.is_ok() || z.is_err()) {
+   |                                    ---------- because of this check
+...
+75 |         z.unwrap_err(); // will panic
+   |         ^^^^^^^^^^^^^^
+
+error: This call to `unwrap()` will always panic.
+  --> $DIR/checked_unwrap.rs:83:9
+   |
+77 |     if x.is_ok() || !(y.is_ok() && z.is_err()) {
+   |        --------- because of this check
+...
+83 |         x.unwrap(); // will panic
+   |         ^^^^^^^^^^
+
+error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
+  --> $DIR/checked_unwrap.rs:84:9
+   |
+77 |     if x.is_ok() || !(y.is_ok() && z.is_err()) {
+   |        --------- the check is happening here
+...
+84 |         x.unwrap_err(); // unnecessary
+   |         ^^^^^^^^^^^^^^
+
+error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
+  --> $DIR/checked_unwrap.rs:85:9
+   |
+77 |     if x.is_ok() || !(y.is_ok() && z.is_err()) {
+   |                       --------- the check is happening here
+...
+85 |         y.unwrap(); // unnecessary
+   |         ^^^^^^^^^^
+
+error: This call to `unwrap_err()` will always panic.
+  --> $DIR/checked_unwrap.rs:86:9
+   |
+77 |     if x.is_ok() || !(y.is_ok() && z.is_err()) {
+   |                       --------- because of this check
+...
+86 |         y.unwrap_err(); // will panic
+   |         ^^^^^^^^^^^^^^
+
+error: This call to `unwrap()` will always panic.
+  --> $DIR/checked_unwrap.rs:87:9
+   |
+77 |     if x.is_ok() || !(y.is_ok() && z.is_err()) {
+   |                                    ---------- because of this check
+...
+87 |         z.unwrap(); // will panic
+   |         ^^^^^^^^^^
+
+error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
+  --> $DIR/checked_unwrap.rs:88:9
+   |
+77 |     if x.is_ok() || !(y.is_ok() && z.is_err()) {
+   |                                    ---------- the check is happening here
+...
+88 |         z.unwrap_err(); // unnecessary
+   |         ^^^^^^^^^^^^^^
+
+error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
+  --> $DIR/checked_unwrap.rs:96:13
+   |
+95 |         if x.is_some() {
    |            ----------- the check is happening here
-72 |             x.unwrap();
+96 |             x.unwrap(); // unnecessary
    |             ^^^^^^^^^^
 
-error: aborting due to 17 previous errors
+error: This call to `unwrap()` will always panic.
+  --> $DIR/checked_unwrap.rs:98:13
+   |
+95 |         if x.is_some() {
+   |            ----------- because of this check
+...
+98 |             x.unwrap(); // will panic
+   |             ^^^^^^^^^^
+
+error: aborting due to 34 previous errors