From 23404287fcb0fd63093d7ef98e6e2bf844b37287 Mon Sep 17 00:00:00 2001 From: Fabian Zaiser Date: Fri, 8 Jun 2018 18:12:01 +0200 Subject: [PATCH] Implement lint checking for `unwrap`s that will always panic. --- clippy_lints/src/unwrap.rs | 52 ++++- tests/ui/checked_unwrap.rs | 72 ++++--- tests/ui/checked_unwrap.stderr | 346 ++++++++++++++++++++++++--------- 3 files changed, 340 insertions(+), 130 deletions(-) diff --git a/clippy_lints/src/unwrap.rs b/clippy_lints/src/unwrap.rs index db840c79a29..8c11f027db6 100644 --- 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:** None. +/// +/// **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, + UNNECESSARY_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); diff --git a/tests/ui/checked_unwrap.rs b/tests/ui/checked_unwrap.rs index fec52940614..893e2db0433 100644 --- a/tests/ui/checked_unwrap.rs +++ b/tests/ui/checked_unwrap.rs @@ -1,32 +1,41 @@ #![deny(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(); - 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 } } @@ -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 } } } diff --git a/tests/ui/checked_unwrap.stderr b/tests/ui/checked_unwrap.stderr index bfa5ec08f2e..28e0df8920a 100644 --- a/tests/ui/checked_unwrap.stderr +++ b/tests/ui/checked_unwrap.stderr @@ -1,9 +1,9 @@ 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 @@ -12,144 +12,296 @@ note: lint level defined here 1 | #![deny(unnecessary_unwrap)] | ^^^^^^^^^^^^^^^^^^ -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 +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 + | ^^^^^^^^^^ + +error: This call to `unwrap()` will always panic. + --> $DIR/checked_unwrap.rs:12:9 | -8 | if x.is_none() { +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:14:9 + | +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`. - --> $DIR/checked_unwrap.rs:17:9 +error: This call to `unwrap_err()` will always panic. + --> $DIR/checked_unwrap.rs:19:9 | -14 | if x.is_ok() { - | --------- the check is happening here +17 | if x.is_ok() { + | --------- 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`. - --> $DIR/checked_unwrap.rs:37:9 +error: This call to `unwrap_err()` will always panic. + --> $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`. - --> $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`. +error: This call to `unwrap()` will always panic. --> $DIR/checked_unwrap.rs:63:9 | -59 | if x.is_ok() || !(y.is_ok() && z.is_err()) { - | --------- the check is happening here +58 | if x.is_ok() || y.is_ok() { + | --------- 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()) { - | ---------- the check is happening here +58 | if x.is_ok() || y.is_ok() { + | --------- 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