Skip to content

Commit fff7aa0

Browse files
J-ZhengLiJ-ZhengLi
committed
expending lint [blocks_in_if_conditions] to check match expr as well
1 parent 8b0bf64 commit fff7aa0

File tree

5 files changed

+106
-64
lines changed

5 files changed

+106
-64
lines changed

‎clippy_lints/src/blocks_in_if_conditions.rs

Lines changed: 63 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use clippy_utils::visitors::{for_each_expr, Descend};
55
use clippy_utils::{get_parent_expr, higher};
66
use core::ops::ControlFlow;
77
use rustc_errors::Applicability;
8-
use rustc_hir::{BlockCheckMode, Expr, ExprKind};
8+
use rustc_hir::{BlockCheckMode, Expr, ExprKind, MatchSource};
99
use rustc_lint::{LateContext, LateLintPass, LintContext};
1010
use rustc_middle::lint::in_external_macro;
1111
use rustc_session::declare_lint_pass;
@@ -52,88 +52,89 @@ impl<'tcx> LateLintPass<'tcx> for BlocksInIfConditions {
5252
if in_external_macro(cx.sess(), expr.span) {
5353
return;
5454
}
55-
if let Some(higher::If { cond, .. }) = higher::If::hir(expr) {
56-
if let ExprKind::Block(block, _) = &cond.kind {
57-
if block.rules == BlockCheckMode::DefaultBlock {
58-
if block.stmts.is_empty() {
59-
if let Some(ex) = &block.expr {
60-
// don't dig into the expression here, just suggest that they remove
61-
// the block
62-
if expr.span.from_expansion() || ex.span.from_expansion() {
63-
return;
64-
}
65-
let mut applicability = Applicability::MachineApplicable;
66-
span_lint_and_sugg(
67-
cx,
68-
BLOCKS_IN_IF_CONDITIONS,
69-
cond.span,
70-
BRACED_EXPR_MESSAGE,
71-
"try",
72-
format!(
73-
"{}",
74-
snippet_block_with_applicability(
75-
cx,
76-
ex.span,
77-
"..",
78-
Some(expr.span),
79-
&mut applicability
80-
)
81-
),
82-
applicability,
83-
);
84-
}
85-
} else {
86-
let span = block.expr.as_ref().map_or_else(|| block.stmts[0].span, |e| e.span);
87-
if span.from_expansion() || expr.span.from_expansion() {
55+
let Some((cond, keyword)) = higher::If::hir(expr).map(|hif| (hif.cond, "if")).or(
56+
if let ExprKind::Match(match_ex, _, MatchSource::Normal) = expr.kind {
57+
Some((match_ex, "match"))
58+
} else {
59+
None
60+
},
61+
) else {
62+
return;
63+
};
64+
if let ExprKind::Block(block, _) = &cond.kind {
65+
if block.rules == BlockCheckMode::DefaultBlock {
66+
if block.stmts.is_empty() {
67+
if let Some(ex) = &block.expr {
68+
// don't dig into the expression here, just suggest that they remove
69+
// the block
70+
if expr.span.from_expansion() || ex.span.from_expansion() {
8871
return;
8972
}
90-
// move block higher
9173
let mut applicability = Applicability::MachineApplicable;
9274
span_lint_and_sugg(
9375
cx,
9476
BLOCKS_IN_IF_CONDITIONS,
95-
expr.span.with_hi(cond.span.hi()),
96-
COMPLEX_BLOCK_MESSAGE,
77+
cond.span,
78+
BRACED_EXPR_MESSAGE,
9779
"try",
9880
format!(
99-
"let res = {}; if res",
81+
"{}",
10082
snippet_block_with_applicability(
10183
cx,
102-
block.span,
84+
ex.span,
10385
"..",
10486
Some(expr.span),
10587
&mut applicability
106-
),
88+
)
10789
),
10890
applicability,
10991
);
11092
}
93+
} else {
94+
let span = block.expr.as_ref().map_or_else(|| block.stmts[0].span, |e| e.span);
95+
if span.from_expansion() || expr.span.from_expansion() {
96+
return;
97+
}
98+
// move block higher
99+
let mut applicability = Applicability::MachineApplicable;
100+
span_lint_and_sugg(
101+
cx,
102+
BLOCKS_IN_IF_CONDITIONS,
103+
expr.span.with_hi(cond.span.hi()),
104+
COMPLEX_BLOCK_MESSAGE,
105+
"try",
106+
format!(
107+
"let res = {}; {keyword} res",
108+
snippet_block_with_applicability(cx, block.span, "..", Some(expr.span), &mut applicability),
109+
),
110+
applicability,
111+
);
111112
}
112-
} else {
113-
let _: Option<!> = for_each_expr(cond, |e| {
114-
if let ExprKind::Closure(closure) = e.kind {
115-
// do not lint if the closure is called using an iterator (see #1141)
116-
if let Some(parent) = get_parent_expr(cx, e)
117-
&& let ExprKind::MethodCall(_, self_arg, _, _) = &parent.kind
118-
&& let caller = cx.typeck_results().expr_ty(self_arg)
119-
&& let Some(iter_id) = cx.tcx.get_diagnostic_item(sym::Iterator)
120-
&& implements_trait(cx, caller, iter_id, &[])
121-
{
122-
return ControlFlow::Continue(Descend::No);
123-
}
113+
}
114+
} else {
115+
let _: Option<!> = for_each_expr(cond, |e| {
116+
if let ExprKind::Closure(closure) = e.kind {
117+
// do not lint if the closure is called using an iterator (see #1141)
118+
if let Some(parent) = get_parent_expr(cx, e)
119+
&& let ExprKind::MethodCall(_, self_arg, _, _) = &parent.kind
120+
&& let caller = cx.typeck_results().expr_ty(self_arg)
121+
&& let Some(iter_id) = cx.tcx.get_diagnostic_item(sym::Iterator)
122+
&& implements_trait(cx, caller, iter_id, &[])
123+
{
124+
return ControlFlow::Continue(Descend::No);
125+
}
124126

125-
let body = cx.tcx.hir().body(closure.body);
126-
let ex = &body.value;
127-
if let ExprKind::Block(block, _) = ex.kind {
128-
if !body.value.span.from_expansion() && !block.stmts.is_empty() {
129-
span_lint(cx, BLOCKS_IN_IF_CONDITIONS, ex.span, COMPLEX_BLOCK_MESSAGE);
130-
return ControlFlow::Continue(Descend::No);
131-
}
127+
let body = cx.tcx.hir().body(closure.body);
128+
let ex = &body.value;
129+
if let ExprKind::Block(block, _) = ex.kind {
130+
if !body.value.span.from_expansion() && !block.stmts.is_empty() {
131+
span_lint(cx, BLOCKS_IN_IF_CONDITIONS, ex.span, COMPLEX_BLOCK_MESSAGE);
132+
return ControlFlow::Continue(Descend::No);
132133
}
133134
}
134-
ControlFlow::Continue(Descend::Yes)
135-
});
136-
}
135+
}
136+
ControlFlow::Continue(Descend::Yes)
137+
});
137138
}
138139
}
139140
}

‎tests/ui-toml/excessive_nesting/excessive_nesting.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
#![allow(clippy::never_loop)]
1010
#![allow(clippy::needless_if)]
1111
#![warn(clippy::excessive_nesting)]
12-
#![allow(clippy::collapsible_if)]
12+
#![allow(clippy::collapsible_if, clippy::blocks_in_if_conditions)]
1313

1414
#[macro_use]
1515
extern crate proc_macros;

‎tests/ui/blocks_in_if_conditions.fixed

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,4 +61,16 @@ fn block_in_assert() {
6161
);
6262
}
6363

64+
// issue #11814
65+
fn block_in_match_expr(num: i32) -> i32 {
66+
let res = {
67+
let opt = Some(2);
68+
opt
69+
}; match res {
70+
Some(0) => 1,
71+
Some(n) => num * 2,
72+
None => 0,
73+
}
74+
}
75+
6476
fn main() {}

‎tests/ui/blocks_in_if_conditions.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,4 +61,16 @@ fn block_in_assert() {
6161
);
6262
}
6363

64+
// issue #11814
65+
fn block_in_match_expr(num: i32) -> i32 {
66+
match {
67+
let opt = Some(2);
68+
opt
69+
} {
70+
Some(0) => 1,
71+
Some(n) => num * 2,
72+
None => 0,
73+
}
74+
}
75+
6476
fn main() {}

‎tests/ui/blocks_in_if_conditions.stderr

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,5 +32,22 @@ LL | if true && x == 3 { 6 } else { 10 }
3232
= note: `-D clippy::nonminimal-bool` implied by `-D warnings`
3333
= help: to override `-D warnings` add `#[allow(clippy::nonminimal_bool)]`
3434

35-
error: aborting due to 3 previous errors
35+
error: in an `if` condition, avoid complex blocks or closures with blocks; instead, move the block or closure higher and bind it with a `let`
36+
--> $DIR/blocks_in_if_conditions.rs:66:5
37+
|
38+
LL | / match {
39+
LL | | let opt = Some(2);
40+
LL | | opt
41+
LL | | } {
42+
| |_____^
43+
|
44+
help: try
45+
|
46+
LL ~ let res = {
47+
LL + let opt = Some(2);
48+
LL + opt
49+
LL ~ }; match res {
50+
|
51+
52+
error: aborting due to 4 previous errors
3653

0 commit comments

Comments
 (0)