Skip to content

Commit 75a7171

Browse files
borsbors
committed
Auto merge of rust-lang#5558 - ThibsG:FixUnwrapInArgs, r=flip1995
Fix `unnecessary_unwrap` lint when checks are done in parameters Fixes a false positive in `unnecessary_unwrap` lint when checks are done in macro parameters. FIxes rust-lang#5174 changelog: Fixes a false positive in `unnecessary_unwrap` lint when checks are done in macro parameters.
2 parents 60538d6 + 72ce6d5 commit 75a7171

File tree

3 files changed

+54
-18
lines changed

3 files changed

+54
-18
lines changed

‎clippy_lints/src/unwrap.rs

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
1-
use crate::utils::{higher::if_block, is_type_diagnostic_item, span_lint_and_then, usage::is_potentially_mutated};
1+
use crate::utils::{
2+
differing_macro_contexts, higher::if_block, is_type_diagnostic_item, span_lint_and_then,
3+
usage::is_potentially_mutated,
4+
};
25
use if_chain::if_chain;
36
use rustc_hir::intravisit::{walk_expr, walk_fn, FnKind, NestedVisitorMap, Visitor};
47
use rustc_hir::{BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, Path, QPath, UnOp};
@@ -73,6 +76,8 @@ struct UnwrapInfo<'tcx> {
7376
ident: &'tcx Path<'tcx>,
7477
/// The check, like `x.is_ok()`
7578
check: &'tcx Expr<'tcx>,
79+
/// The branch where the check takes place, like `if x.is_ok() { .. }`
80+
branch: &'tcx Expr<'tcx>,
7681
/// Whether `is_some()` or `is_ok()` was called (as opposed to `is_err()` or `is_none()`).
7782
safe_to_unwrap: bool,
7883
}
@@ -82,19 +87,20 @@ struct UnwrapInfo<'tcx> {
8287
fn collect_unwrap_info<'a, 'tcx>(
8388
cx: &'a LateContext<'a, 'tcx>,
8489
expr: &'tcx Expr<'_>,
90+
branch: &'tcx Expr<'_>,
8591
invert: bool,
8692
) -> Vec<UnwrapInfo<'tcx>> {
8793
if let ExprKind::Binary(op, left, right) = &expr.kind {
8894
match (invert, op.node) {
8995
(false, BinOpKind::And) | (false, BinOpKind::BitAnd) | (true, BinOpKind::Or) | (true, BinOpKind::BitOr) => {
90-
let mut unwrap_info = collect_unwrap_info(cx, left, invert);
91-
unwrap_info.append(&mut collect_unwrap_info(cx, right, invert));
96+
let mut unwrap_info = collect_unwrap_info(cx, left, branch, invert);
97+
unwrap_info.append(&mut collect_unwrap_info(cx, right, branch, invert));
9298
return unwrap_info;
9399
},
94100
_ => (),
95101
}
96102
} else if let ExprKind::Unary(UnOp::UnNot, expr) = &expr.kind {
97-
return collect_unwrap_info(cx, expr, !invert);
103+
return collect_unwrap_info(cx, expr, branch, !invert);
98104
} else {
99105
if_chain! {
100106
if let ExprKind::MethodCall(method_name, _, args) = &expr.kind;
@@ -111,7 +117,7 @@ fn collect_unwrap_info<'a, 'tcx>(
111117
_ => unreachable!(),
112118
};
113119
let safe_to_unwrap = unwrappable != invert;
114-
return vec![UnwrapInfo { ident: path, check: expr, safe_to_unwrap }];
120+
return vec![UnwrapInfo { ident: path, check: expr, branch, safe_to_unwrap }];
115121
}
116122
}
117123
}
@@ -121,7 +127,7 @@ fn collect_unwrap_info<'a, 'tcx>(
121127
impl<'a, 'tcx> UnwrappableVariablesVisitor<'a, 'tcx> {
122128
fn visit_branch(&mut self, cond: &'tcx Expr<'_>, branch: &'tcx Expr<'_>, else_branch: bool) {
123129
let prev_len = self.unwrappables.len();
124-
for unwrap_info in collect_unwrap_info(self.cx, cond, else_branch) {
130+
for unwrap_info in collect_unwrap_info(self.cx, cond, branch, else_branch) {
125131
if is_potentially_mutated(unwrap_info.ident, cond, self.cx)
126132
|| is_potentially_mutated(unwrap_info.ident, branch, self.cx)
127133
{
@@ -158,6 +164,9 @@ impl<'a, 'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'a, 'tcx> {
158164
let call_to_unwrap = method_name.ident.name == sym!(unwrap);
159165
if let Some(unwrappable) = self.unwrappables.iter()
160166
.find(|u| u.ident.res == path.res);
167+
// Span contexts should not differ with the conditional branch
168+
if !differing_macro_contexts(unwrappable.branch.span, expr.span);
169+
if !differing_macro_contexts(unwrappable.branch.span, unwrappable.check.span);
161170
then {
162171
if call_to_unwrap == unwrappable.safe_to_unwrap {
163172
span_lint_and_then(

‎tests/ui/checked_unwrap/simple_conditionals.rs

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,30 @@ macro_rules! m {
99
};
1010
}
1111

12+
macro_rules! checks_in_param {
13+
($a:expr, $b:expr) => {
14+
if $a {
15+
$b;
16+
}
17+
};
18+
}
19+
20+
macro_rules! checks_unwrap {
21+
($a:expr, $b:expr) => {
22+
if $a.is_some() {
23+
$b;
24+
}
25+
};
26+
}
27+
28+
macro_rules! checks_some {
29+
($a:expr, $b:expr) => {
30+
if $a {
31+
$b.unwrap();
32+
}
33+
};
34+
}
35+
1236
fn main() {
1337
let x = Some(());
1438
if x.is_some() {
@@ -22,6 +46,9 @@ fn main() {
2246
x.unwrap(); // unnecessary
2347
}
2448
m!(x);
49+
checks_in_param!(x.is_some(), x.unwrap()); // ok
50+
checks_unwrap!(x, x.unwrap()); // ok
51+
checks_some!(x.is_some(), x); // ok
2552
let mut x: Result<(), ()> = Ok(());
2653
if x.is_ok() {
2754
x.unwrap(); // unnecessary

‎tests/ui/checked_unwrap/simple_conditionals.stderr

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
2-
--> $DIR/simple_conditionals.rs:15:9
2+
--> $DIR/simple_conditionals.rs:39:9
33
|
44
LL | if x.is_some() {
55
| ----------- the check is happening here
@@ -13,7 +13,7 @@ LL | #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)]
1313
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
1414

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

3030
error: This call to `unwrap()` will always panic.
31-
--> $DIR/simple_conditionals.rs:20:9
31+
--> $DIR/simple_conditionals.rs:44:9
3232
|
3333
LL | if x.is_none() {
3434
| ----------- because of this check
3535
LL | x.unwrap(); // will panic
3636
| ^^^^^^^^^^
3737

3838
error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
39-
--> $DIR/simple_conditionals.rs:22:9
39+
--> $DIR/simple_conditionals.rs:46:9
4040
|
4141
LL | if x.is_none() {
4242
| ----------- the check is happening here
@@ -58,15 +58,15 @@ LL | m!(x);
5858
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
5959

6060
error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
61-
--> $DIR/simple_conditionals.rs:27:9
61+
--> $DIR/simple_conditionals.rs:54:9
6262
|
6363
LL | if x.is_ok() {
6464
| --------- the check is happening here
6565
LL | x.unwrap(); // unnecessary
6666
| ^^^^^^^^^^
6767

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

7777
error: This call to `unwrap()` will always panic.
78-
--> $DIR/simple_conditionals.rs:30:9
78+
--> $DIR/simple_conditionals.rs:57:9
7979
|
8080
LL | if x.is_ok() {
8181
| --------- because of this check
@@ -84,7 +84,7 @@ LL | x.unwrap(); // will panic
8484
| ^^^^^^^^^^
8585

8686
error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
87-
--> $DIR/simple_conditionals.rs:31:9
87+
--> $DIR/simple_conditionals.rs:58:9
8888
|
8989
LL | if x.is_ok() {
9090
| --------- the check is happening here
@@ -93,15 +93,15 @@ LL | x.unwrap_err(); // unnecessary
9393
| ^^^^^^^^^^^^^^
9494

9595
error: This call to `unwrap()` will always panic.
96-
--> $DIR/simple_conditionals.rs:34:9
96+
--> $DIR/simple_conditionals.rs:61:9
9797
|
9898
LL | if x.is_err() {
9999
| ---------- because of this check
100100
LL | x.unwrap(); // will panic
101101
| ^^^^^^^^^^
102102

103103
error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
104-
--> $DIR/simple_conditionals.rs:35:9
104+
--> $DIR/simple_conditionals.rs:62:9
105105
|
106106
LL | if x.is_err() {
107107
| ---------- the check is happening here
@@ -110,7 +110,7 @@ LL | x.unwrap_err(); // unnecessary
110110
| ^^^^^^^^^^^^^^
111111

112112
error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
113-
--> $DIR/simple_conditionals.rs:37:9
113+
--> $DIR/simple_conditionals.rs:64:9
114114
|
115115
LL | if x.is_err() {
116116
| ---------- the check is happening here
@@ -119,7 +119,7 @@ LL | x.unwrap(); // unnecessary
119119
| ^^^^^^^^^^
120120

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

0 commit comments

Comments
 (0)