From 03d3131cb46ad8139466a58759ac312f9ebaf053 Mon Sep 17 00:00:00 2001 From: Jake Goulding Date: Wed, 18 Aug 2021 09:56:25 -0400 Subject: [PATCH] Extend unnecessary_unwrap to look for expect in addition to unwrap Closes #7581 --- clippy_lints/src/unwrap.rs | 4 +- .../ui/checked_unwrap/simple_conditionals.rs | 4 ++ .../checked_unwrap/simple_conditionals.stderr | 62 +++++++++++++++---- 3 files changed, 55 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/unwrap.rs b/clippy_lints/src/unwrap.rs index bffd9f3612b..42738bc3e1f 100644 --- a/clippy_lints/src/unwrap.rs +++ b/clippy_lints/src/unwrap.rs @@ -172,8 +172,8 @@ fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { if_chain! { if let ExprKind::MethodCall(method_name, _, args, _) = expr.kind; if let ExprKind::Path(QPath::Resolved(None, path)) = args[0].kind; - if [sym::unwrap, sym!(unwrap_err)].contains(&method_name.ident.name); - let call_to_unwrap = method_name.ident.name == sym::unwrap; + if [sym::unwrap, sym::expect, sym!(unwrap_err)].contains(&method_name.ident.name); + let call_to_unwrap = [sym::unwrap, sym::expect].contains(&method_name.ident.name); if let Some(unwrappable) = self.unwrappables.iter() .find(|u| u.ident.res == path.res); // Span contexts should not differ with the conditional branch diff --git a/tests/ui/checked_unwrap/simple_conditionals.rs b/tests/ui/checked_unwrap/simple_conditionals.rs index 8f23fb28827..ee3fdfabe9d 100644 --- a/tests/ui/checked_unwrap/simple_conditionals.rs +++ b/tests/ui/checked_unwrap/simple_conditionals.rs @@ -37,8 +37,10 @@ fn main() { let x = Some(()); if x.is_some() { x.unwrap(); // unnecessary + x.expect("an error message"); // unnecessary } else { x.unwrap(); // will panic + x.expect("an error message"); // will panic } if x.is_none() { x.unwrap(); // will panic @@ -52,9 +54,11 @@ fn main() { let mut x: Result<(), ()> = Ok(()); if x.is_ok() { x.unwrap(); // unnecessary + x.expect("an error message"); // unnecessary x.unwrap_err(); // will panic } else { x.unwrap(); // will panic + x.expect("an error message"); // will panic x.unwrap_err(); // unnecessary } if x.is_err() { diff --git a/tests/ui/checked_unwrap/simple_conditionals.stderr b/tests/ui/checked_unwrap/simple_conditionals.stderr index a4bc058fe20..7127ff89b31 100644 --- a/tests/ui/checked_unwrap/simple_conditionals.stderr +++ b/tests/ui/checked_unwrap/simple_conditionals.stderr @@ -12,8 +12,17 @@ note: the lint level is defined here LL | #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^ +error: you checked before that `expect()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` + --> $DIR/simple_conditionals.rs:40:9 + | +LL | if x.is_some() { + | ----------- the check is happening here +LL | x.unwrap(); // unnecessary +LL | x.expect("an error message"); // unnecessary + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + error: this call to `unwrap()` will always panic - --> $DIR/simple_conditionals.rs:41:9 + --> $DIR/simple_conditionals.rs:42:9 | LL | if x.is_some() { | ----------- because of this check @@ -27,8 +36,17 @@ note: the lint level is defined here LL | #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] | ^^^^^^^^^^^^^^^^^^^^^^^^ +error: this call to `expect()` will always panic + --> $DIR/simple_conditionals.rs:43:9 + | +LL | if x.is_some() { + | ----------- because of this check +... +LL | x.expect("an error message"); // will panic + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + error: this call to `unwrap()` will always panic - --> $DIR/simple_conditionals.rs:44:9 + --> $DIR/simple_conditionals.rs:46:9 | LL | if x.is_none() { | ----------- because of this check @@ -36,7 +54,7 @@ LL | 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/simple_conditionals.rs:46:9 + --> $DIR/simple_conditionals.rs:48:9 | LL | if x.is_none() { | ----------- the check is happening here @@ -58,24 +76,33 @@ LL | m!(x); = note: this error originates in the macro `m` (in Nightly builds, run with -Z macro-backtrace for more info) error: you checked before that `unwrap()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` - --> $DIR/simple_conditionals.rs:54:9 + --> $DIR/simple_conditionals.rs:56:9 | LL | if x.is_ok() { | --------- the check is happening here LL | x.unwrap(); // unnecessary | ^^^^^^^^^^ +error: you checked before that `expect()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` + --> $DIR/simple_conditionals.rs:57:9 + | +LL | if x.is_ok() { + | --------- the check is happening here +LL | x.unwrap(); // unnecessary +LL | x.expect("an error message"); // unnecessary + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + error: this call to `unwrap_err()` will always panic - --> $DIR/simple_conditionals.rs:55:9 + --> $DIR/simple_conditionals.rs:58:9 | LL | if x.is_ok() { | --------- because of this check -LL | x.unwrap(); // unnecessary +... LL | x.unwrap_err(); // will panic | ^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> $DIR/simple_conditionals.rs:57:9 + --> $DIR/simple_conditionals.rs:60:9 | LL | if x.is_ok() { | --------- because of this check @@ -83,8 +110,17 @@ LL | if x.is_ok() { LL | x.unwrap(); // will panic | ^^^^^^^^^^ +error: this call to `expect()` will always panic + --> $DIR/simple_conditionals.rs:61:9 + | +LL | if x.is_ok() { + | --------- because of this check +... +LL | x.expect("an error message"); // 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/simple_conditionals.rs:58:9 + --> $DIR/simple_conditionals.rs:62:9 | LL | if x.is_ok() { | --------- the check is happening here @@ -93,7 +129,7 @@ LL | x.unwrap_err(); // unnecessary | ^^^^^^^^^^^^^^ error: this call to `unwrap()` will always panic - --> $DIR/simple_conditionals.rs:61:9 + --> $DIR/simple_conditionals.rs:65:9 | LL | if x.is_err() { | ---------- because of this check @@ -101,7 +137,7 @@ LL | 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/simple_conditionals.rs:62:9 + --> $DIR/simple_conditionals.rs:66:9 | LL | if x.is_err() { | ---------- the check is happening here @@ -110,7 +146,7 @@ LL | 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/simple_conditionals.rs:64:9 + --> $DIR/simple_conditionals.rs:68:9 | LL | if x.is_err() { | ---------- the check is happening here @@ -119,7 +155,7 @@ LL | x.unwrap(); // unnecessary | ^^^^^^^^^^ error: this call to `unwrap_err()` will always panic - --> $DIR/simple_conditionals.rs:65:9 + --> $DIR/simple_conditionals.rs:69:9 | LL | if x.is_err() { | ---------- because of this check @@ -127,5 +163,5 @@ LL | if x.is_err() { LL | x.unwrap_err(); // will panic | ^^^^^^^^^^^^^^ -error: aborting due to 13 previous errors +error: aborting due to 17 previous errors