Merge pull request #2837 from fanzier/panicking_unwrap

Implement lint checking for `unwrap`s that will always panic.
rust-lang · Jun 19, 2018 · d761ba7 · d761ba7
2 parents 4839c79 + 817da4c
commit d761ba7
Show file tree

Hide file tree

Showing 4 changed files with 308 additions and 91 deletions.
diff --git a/clippy_lints/src/lib.rs b/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,
     ]);
 }

diff --git a/clippy_lints/src/unwrap.rs b/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(
-                        self.cx,
-                        UNNECESSARY_UNWRAP,
-                        expr.span,
-                        &format!("You checked before that `{}()` cannot fail. \
-                        Instead of checking and unwrapping, it's better to use `if let` or `match`.",
-                        method_name.name),
-                        |db| { db.span_label(unwrappable.check.span, "the check is happening here"); },
-                    );
+                    if call_to_unwrap == unwrappable.safe_to_unwrap {
+                        span_lint_and_then(
+                            self.cx,
+                            UNNECESSARY_UNWRAP,
+                            expr.span,
+                            &format!("You checked before that `{}()` cannot fail. \
+                            Instead of checking and unwrapping, it's better to use `if let` or `match`.",
+                            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)
     }
 }
 

diff --git a/tests/ui/checked_unwrap.rs b/tests/ui/checked_unwrap.rs
@@ -1,75 +1,101 @@
-#![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).
     }
 }
 
 fn test_complex_conditions() {
     let x: Result<(), ()> = Ok(());
     let y: Result<(), ()> = Ok(());
     if x.is_ok() && y.is_err() {
-        x.unwrap();
-        y.unwrap_err();
+        x.unwrap(); // unnecessary
+        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();
-        y.unwrap_err();
+        x.unwrap(); // will panic
+        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();
-        y.unwrap_err();
-        z.unwrap();
+        x.unwrap(); // unnecessary
+        x.unwrap_err(); // will panic
+        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
-    } else {
-        x.unwrap_err();
+        // not statically determinable whether any of the following will always succeed or always fail:
+        x.unwrap();
         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
     }
 }
 
 fn test_nested() {
     fn nested() {
         let x = Some(());
         if x.is_some() {
-            x.unwrap();
+            x.unwrap(); // unnecessary
+        } else {
+            x.unwrap(); // will panic
         }
     }
 }