Skip to content

Commit

Permalink
Fix unwrap lint when checks are done in parameters
Browse files Browse the repository at this point in the history
  • Loading branch information
@ThibsG
ThibsG committed May 2, 2020
1 parent 991efa6 commit 72ce6d5
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 18 deletions.
21 changes: 15 additions & 6 deletions clippy_lints/src/unwrap.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
use crate::utils::{higher::if_block, is_type_diagnostic_item, span_lint_and_then, usage::is_potentially_mutated};
use crate::utils::{
differing_macro_contexts, higher::if_block, is_type_diagnostic_item, span_lint_and_then,
usage::is_potentially_mutated,
};
use if_chain::if_chain;
use rustc_hir::intravisit::{walk_expr, walk_fn, FnKind, NestedVisitorMap, Visitor};
use rustc_hir::{BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, Path, QPath, UnOp};
Expand Down Expand Up @@ -73,6 +76,8 @@ struct UnwrapInfo<'tcx> {
ident: &'tcx Path<'tcx>,
/// The check, like `x.is_ok()`
check: &'tcx Expr<'tcx>,
/// The branch where the check takes place, like `if x.is_ok() { .. }`
branch: &'tcx Expr<'tcx>,
/// Whether `is_some()` or `is_ok()` was called (as opposed to `is_err()` or `is_none()`).
safe_to_unwrap: bool,
}
Expand All @@ -82,19 +87,20 @@ struct UnwrapInfo<'tcx> {
fn collect_unwrap_info<'a, 'tcx>(
cx: &'a LateContext<'a, 'tcx>,
expr: &'tcx Expr<'_>,
branch: &'tcx Expr<'_>,
invert: bool,
) -> Vec<UnwrapInfo<'tcx>> {
if let ExprKind::Binary(op, left, right) = &expr.kind {
match (invert, op.node) {
(false, BinOpKind::And) | (false, BinOpKind::BitAnd) | (true, BinOpKind::Or) | (true, BinOpKind::BitOr) => {
let mut unwrap_info = collect_unwrap_info(cx, left, invert);
unwrap_info.append(&mut collect_unwrap_info(cx, right, invert));
let mut unwrap_info = collect_unwrap_info(cx, left, branch, invert);
unwrap_info.append(&mut collect_unwrap_info(cx, right, branch, invert));
return unwrap_info;
},
_ => (),
}
} else if let ExprKind::Unary(UnOp::UnNot, expr) = &expr.kind {
return collect_unwrap_info(cx, expr, !invert);
return collect_unwrap_info(cx, expr, branch, !invert);
} else {
if_chain! {
if let ExprKind::MethodCall(method_name, _, args) = &expr.kind;
Expand All @@ -111,7 +117,7 @@ fn collect_unwrap_info<'a, 'tcx>(
_ => unreachable!(),
};
let safe_to_unwrap = unwrappable != invert;
return vec![UnwrapInfo { ident: path, check: expr, safe_to_unwrap }];
return vec![UnwrapInfo { ident: path, check: expr, branch, safe_to_unwrap }];
}
}
}
Expand All @@ -121,7 +127,7 @@ fn collect_unwrap_info<'a, 'tcx>(
impl<'a, 'tcx> UnwrappableVariablesVisitor<'a, 'tcx> {
fn visit_branch(&mut self, cond: &'tcx Expr<'_>, branch: &'tcx Expr<'_>, else_branch: bool) {
let prev_len = self.unwrappables.len();
for unwrap_info in collect_unwrap_info(self.cx, cond, else_branch) {
for unwrap_info in collect_unwrap_info(self.cx, cond, branch, else_branch) {
if is_potentially_mutated(unwrap_info.ident, cond, self.cx)
|| is_potentially_mutated(unwrap_info.ident, branch, self.cx)
{
Expand Down Expand Up @@ -158,6 +164,9 @@ impl<'a, 'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'a, 'tcx> {
let call_to_unwrap = method_name.ident.name == sym!(unwrap);
if let Some(unwrappable) = self.unwrappables.iter()
.find(|u| u.ident.res == path.res);
// Span contexts should not differ with the conditional branch
if !differing_macro_contexts(unwrappable.branch.span, expr.span);
if !differing_macro_contexts(unwrappable.branch.span, unwrappable.check.span);
then {
if call_to_unwrap == unwrappable.safe_to_unwrap {
span_lint_and_then(
Expand Down
27 changes: 27 additions & 0 deletions tests/ui/checked_unwrap/simple_conditionals.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,30 @@ macro_rules! m {
};
}

macro_rules! checks_in_param {
($a:expr, $b:expr) => {
if $a {
$b;
}
};
}

macro_rules! checks_unwrap {
($a:expr, $b:expr) => {
if $a.is_some() {
$b;
}
};
}

macro_rules! checks_some {
($a:expr, $b:expr) => {
if $a {
$b.unwrap();
}
};
}

fn main() {
let x = Some(());
if x.is_some() {
Expand All @@ -22,6 +46,9 @@ fn main() {
x.unwrap(); // unnecessary
}
m!(x);
checks_in_param!(x.is_some(), x.unwrap()); // ok
checks_unwrap!(x, x.unwrap()); // ok
checks_some!(x.is_some(), x); // ok
let mut x: Result<(), ()> = Ok(());
if x.is_ok() {
x.unwrap(); // unnecessary
Expand Down
24 changes: 12 additions & 12 deletions tests/ui/checked_unwrap/simple_conditionals.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
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:15:9
--> $DIR/simple_conditionals.rs:39:9
|
LL | if x.is_some() {
| ----------- the check is happening here
Expand All @@ -13,7 +13,7 @@ LL | #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^

error: This call to `unwrap()` will always panic.
--> $DIR/simple_conditionals.rs:17:9
--> $DIR/simple_conditionals.rs:41:9
|
LL | if x.is_some() {
| ----------- because of this check
Expand All @@ -28,15 +28,15 @@ LL | #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)]
| ^^^^^^^^^^^^^^^^^^^^^^^^

error: This call to `unwrap()` will always panic.
--> $DIR/simple_conditionals.rs:20:9
--> $DIR/simple_conditionals.rs:44:9
|
LL | if x.is_none() {
| ----------- because of this check
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:22:9
--> $DIR/simple_conditionals.rs:46:9
|
LL | if x.is_none() {
| ----------- the check is happening here
Expand All @@ -58,15 +58,15 @@ LL | m!(x);
= note: this error originates in a macro (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:27:9
--> $DIR/simple_conditionals.rs:54:9
|
LL | if x.is_ok() {
| --------- the check is happening here
LL | x.unwrap(); // unnecessary
| ^^^^^^^^^^

error: This call to `unwrap_err()` will always panic.
--> $DIR/simple_conditionals.rs:28:9
--> $DIR/simple_conditionals.rs:55:9
|
LL | if x.is_ok() {
| --------- because of this check
Expand All @@ -75,7 +75,7 @@ LL | x.unwrap_err(); // will panic
| ^^^^^^^^^^^^^^

error: This call to `unwrap()` will always panic.
--> $DIR/simple_conditionals.rs:30:9
--> $DIR/simple_conditionals.rs:57:9
|
LL | if x.is_ok() {
| --------- because of this check
Expand All @@ -84,7 +84,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:31:9
--> $DIR/simple_conditionals.rs:58:9
|
LL | if x.is_ok() {
| --------- the check is happening here
Expand All @@ -93,15 +93,15 @@ LL | x.unwrap_err(); // unnecessary
| ^^^^^^^^^^^^^^

error: This call to `unwrap()` will always panic.
--> $DIR/simple_conditionals.rs:34:9
--> $DIR/simple_conditionals.rs:61:9
|
LL | if x.is_err() {
| ---------- because of this check
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:35:9
--> $DIR/simple_conditionals.rs:62:9
|
LL | if x.is_err() {
| ---------- the check is happening here
Expand All @@ -110,7 +110,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:37:9
--> $DIR/simple_conditionals.rs:64:9
|
LL | if x.is_err() {
| ---------- the check is happening here
Expand All @@ -119,7 +119,7 @@ LL | x.unwrap(); // unnecessary
| ^^^^^^^^^^

error: This call to `unwrap_err()` will always panic.
--> $DIR/simple_conditionals.rs:38:9
--> $DIR/simple_conditionals.rs:65:9
|
LL | if x.is_err() {
| ---------- because of this check
Expand Down

0 comments on commit 72ce6d5

Please sign in to comment.