Skip to content

Commit

Permalink
rename [blocks_in_if_conditions] to [blocks_in_conditions];
Browse files Browse the repository at this point in the history
add more test cases with `match`;
minor fixes in message output regarding review feedback
  • Loading branch information
J-ZhengLi committed Nov 30, 2023
1 parent fff7aa0 commit 8a1bcaf
Show file tree
Hide file tree
Showing 22 changed files with 177 additions and 114 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4946,6 +4946,7 @@ Released 2018-09-13
[`blanket_clippy_restriction_lints`]: https://rust-lang.github.io/rust-clippy/master/index.html#blanket_clippy_restriction_lints
[`block_in_if_condition_expr`]: https://rust-lang.github.io/rust-clippy/master/index.html#block_in_if_condition_expr
[`block_in_if_condition_stmt`]: https://rust-lang.github.io/rust-clippy/master/index.html#block_in_if_condition_stmt
[`blocks_in_conditions`]: https://rust-lang.github.io/rust-clippy/master/index.html#blocks_in_conditions
[`blocks_in_if_conditions`]: https://rust-lang.github.io/rust-clippy/master/index.html#blocks_in_if_conditions
[`bool_assert_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
[`bool_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_comparison
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,31 +36,36 @@ declare_clippy_lint! {
/// if res { /* ... */ }
/// ```
#[clippy::version = "1.45.0"]
pub BLOCKS_IN_IF_CONDITIONS,
pub BLOCKS_IN_CONDITIONS,
style,
"useless or complex blocks that can be eliminated in conditions"
}

declare_lint_pass!(BlocksInIfConditions => [BLOCKS_IN_IF_CONDITIONS]);
declare_lint_pass!(BlocksInIfConditions => [BLOCKS_IN_CONDITIONS]);

const BRACED_EXPR_MESSAGE: &str = "omit braces around single expression condition";
const COMPLEX_BLOCK_MESSAGE: &str = "in an `if` condition, avoid complex blocks or closures with blocks; \
instead, move the block or closure higher and bind it with a `let`";

impl<'tcx> LateLintPass<'tcx> for BlocksInIfConditions {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if in_external_macro(cx.sess(), expr.span) {
return;
}
let Some((cond, keyword)) = higher::If::hir(expr).map(|hif| (hif.cond, "if")).or(
if let ExprKind::Match(match_ex, _, MatchSource::Normal) = expr.kind {
Some((match_ex, "match"))

let Some((cond, keyword, desc)) = higher::If::hir(expr)
.map(|hif| (hif.cond, "if", "an `if` condition"))
.or(if let ExprKind::Match(match_ex, _, MatchSource::Normal) = expr.kind {
Some((match_ex, "match", "a `match` scrutinee"))
} else {
None
},
) else {
})
else {
return;
};
let complex_block_message = &format!(
"in {desc}, avoid complex blocks or closures with blocks; \
instead, move the block or closure higher and bind it with a `let`",
);

if let ExprKind::Block(block, _) = &cond.kind {
if block.rules == BlockCheckMode::DefaultBlock {
if block.stmts.is_empty() {
Expand All @@ -73,20 +78,12 @@ impl<'tcx> LateLintPass<'tcx> for BlocksInIfConditions {
let mut applicability = Applicability::MachineApplicable;
span_lint_and_sugg(
cx,
BLOCKS_IN_IF_CONDITIONS,
BLOCKS_IN_CONDITIONS,
cond.span,
BRACED_EXPR_MESSAGE,
"try",
format!(
"{}",
snippet_block_with_applicability(
cx,
ex.span,
"..",
Some(expr.span),
&mut applicability
)
),
snippet_block_with_applicability(cx, ex.span, "..", Some(expr.span), &mut applicability)
.to_string(),
applicability,
);
}
Expand All @@ -99,9 +96,9 @@ impl<'tcx> LateLintPass<'tcx> for BlocksInIfConditions {
let mut applicability = Applicability::MachineApplicable;
span_lint_and_sugg(
cx,
BLOCKS_IN_IF_CONDITIONS,
BLOCKS_IN_CONDITIONS,
expr.span.with_hi(cond.span.hi()),
COMPLEX_BLOCK_MESSAGE,
complex_block_message,
"try",
format!(
"let res = {}; {keyword} res",
Expand All @@ -128,7 +125,7 @@ impl<'tcx> LateLintPass<'tcx> for BlocksInIfConditions {
let ex = &body.value;
if let ExprKind::Block(block, _) = ex.kind {
if !body.value.span.from_expansion() && !block.stmts.is_empty() {
span_lint(cx, BLOCKS_IN_IF_CONDITIONS, ex.span, COMPLEX_BLOCK_MESSAGE);
span_lint(cx, BLOCKS_IN_CONDITIONS, ex.span, complex_block_message);
return ControlFlow::Continue(Descend::No);
}
}
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::await_holding_invalid::AWAIT_HOLDING_INVALID_TYPE_INFO,
crate::await_holding_invalid::AWAIT_HOLDING_LOCK_INFO,
crate::await_holding_invalid::AWAIT_HOLDING_REFCELL_REF_INFO,
crate::blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS_INFO,
crate::blocks_in_conditions::BLOCKS_IN_CONDITIONS_INFO,
crate::bool_assert_comparison::BOOL_ASSERT_COMPARISON_INFO,
crate::bool_to_int_with_if::BOOL_TO_INT_WITH_IF_INFO,
crate::booleans::NONMINIMAL_BOOL_INFO,
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ mod assertions_on_result_states;
mod async_yields_async;
mod attrs;
mod await_holding_invalid;
mod blocks_in_if_conditions;
mod blocks_in_conditions;
mod bool_assert_comparison;
mod bool_to_int_with_if;
mod booleans;
Expand Down Expand Up @@ -653,7 +653,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
store.register_late_pass(|_| Box::<significant_drop_tightening::SignificantDropTightening<'_>>::default());
store.register_late_pass(|_| Box::new(len_zero::LenZero));
store.register_late_pass(|_| Box::new(attrs::Attributes));
store.register_late_pass(|_| Box::new(blocks_in_if_conditions::BlocksInIfConditions));
store.register_late_pass(|_| Box::new(blocks_in_conditions::BlocksInIfConditions));
store.register_late_pass(|_| Box::new(unicode::Unicode));
store.register_late_pass(|_| Box::new(uninit_vec::UninitVec));
store.register_late_pass(|_| Box::new(unit_return_expecting_ord::UnitReturnExpectingOrd));
Expand Down
5 changes: 3 additions & 2 deletions clippy_lints/src/renamed_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
pub static RENAMED_LINTS: &[(&str, &str)] = &[
("clippy::almost_complete_letter_range", "clippy::almost_complete_range"),
("clippy::blacklisted_name", "clippy::disallowed_names"),
("clippy::block_in_if_condition_expr", "clippy::blocks_in_if_conditions"),
("clippy::block_in_if_condition_stmt", "clippy::blocks_in_if_conditions"),
("clippy::block_in_if_condition_expr", "clippy::blocks_in_conditions"),
("clippy::block_in_if_condition_stmt", "clippy::blocks_in_conditions"),
("clippy::blocks_in_if_conditions", "clippy::blocks_in_conditions"),
("clippy::box_vec", "clippy::box_collection"),
("clippy::const_static_lifetime", "clippy::redundant_static_lifetimes"),
("clippy::cyclomatic_complexity", "clippy::cognitive_complexity"),
Expand Down
2 changes: 1 addition & 1 deletion tests/ui-toml/excessive_nesting/excessive_nesting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
#![allow(clippy::never_loop)]
#![allow(clippy::needless_if)]
#![warn(clippy::excessive_nesting)]
#![allow(clippy::collapsible_if, clippy::blocks_in_if_conditions)]
#![allow(clippy::collapsible_if, clippy::blocks_in_conditions)]

#[macro_use]
extern crate proc_macros;
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/bind_instead_of_map_multipart.fixed
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#![deny(clippy::bind_instead_of_map)]
#![allow(clippy::blocks_in_if_conditions)]
#![allow(clippy::blocks_in_conditions)]

pub fn main() {
let _ = Some("42").map(|s| if s.len() < 42 { 0 } else { s.len() });
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/bind_instead_of_map_multipart.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#![deny(clippy::bind_instead_of_map)]
#![allow(clippy::blocks_in_if_conditions)]
#![allow(clippy::blocks_in_conditions)]

pub fn main() {
let _ = Some("42").and_then(|s| if s.len() < 42 { Some(0) } else { Some(s.len()) });
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![warn(clippy::blocks_in_if_conditions)]
#![warn(clippy::blocks_in_conditions)]
#![allow(unused, clippy::let_and_return, clippy::needless_if)]
#![warn(clippy::nonminimal_bool)]

Expand All @@ -21,6 +21,7 @@ fn macro_if() {

fn condition_has_block() -> i32 {
let res = {
//~^ 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`
let x = 3;
x == 3
}; if res {
Expand All @@ -32,6 +33,7 @@ fn condition_has_block() -> i32 {

fn condition_has_block_with_single_expression() -> i32 {
if true { 6 } else { 10 }
//~^ ERROR: omit braces around single expression condition
}

fn condition_is_normal() -> i32 {
Expand Down Expand Up @@ -64,12 +66,22 @@ fn block_in_assert() {
// issue #11814
fn block_in_match_expr(num: i32) -> i32 {
let res = {
//~^ ERROR: in a `match` scrutinee, avoid complex blocks or closures with blocks; instead, move the block or closure higher and bind it with a `let`
let opt = Some(2);
opt
}; match res {
Some(0) => 1,
Some(n) => num * 2,
None => 0,
};

match unsafe {
let hearty_hearty_hearty = vec![240, 159, 146, 150];
String::from_utf8_unchecked(hearty_hearty_hearty).as_str()
} {
"💖" => 1,
"what" => 2,
_ => 3,
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![warn(clippy::blocks_in_if_conditions)]
#![warn(clippy::blocks_in_conditions)]
#![allow(unused, clippy::let_and_return, clippy::needless_if)]
#![warn(clippy::nonminimal_bool)]

Expand All @@ -21,6 +21,7 @@ fn macro_if() {

fn condition_has_block() -> i32 {
if {
//~^ 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`
let x = 3;
x == 3
} {
Expand All @@ -32,6 +33,7 @@ fn condition_has_block() -> i32 {

fn condition_has_block_with_single_expression() -> i32 {
if { true } { 6 } else { 10 }
//~^ ERROR: omit braces around single expression condition
}

fn condition_is_normal() -> i32 {
Expand Down Expand Up @@ -64,12 +66,22 @@ fn block_in_assert() {
// issue #11814
fn block_in_match_expr(num: i32) -> i32 {
match {
//~^ ERROR: in a `match` scrutinee, avoid complex blocks or closures with blocks; instead, move the block or closure higher and bind it with a `let`
let opt = Some(2);
opt
} {
Some(0) => 1,
Some(n) => num * 2,
None => 0,
};

match unsafe {
let hearty_hearty_hearty = vec![240, 159, 146, 150];
String::from_utf8_unchecked(hearty_hearty_hearty).as_str()
} {
"💖" => 1,
"what" => 2,
_ => 3,
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,41 +1,44 @@
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`
--> $DIR/blocks_in_if_conditions.rs:23:5
--> $DIR/blocks_in_conditions.rs:23:5
|
LL | / if {
LL | |
LL | | let x = 3;
LL | | x == 3
LL | | } {
| |_____^
|
= note: `-D clippy::blocks-in-if-conditions` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::blocks_in_if_conditions)]`
= note: `-D clippy::blocks-in-conditions` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::blocks_in_conditions)]`
help: try
|
LL ~ let res = {
LL +
LL + let x = 3;
LL + x == 3
LL ~ }; if res {
|

error: omit braces around single expression condition
--> $DIR/blocks_in_if_conditions.rs:34:8
--> $DIR/blocks_in_conditions.rs:35:8
|
LL | if { true } { 6 } else { 10 }
| ^^^^^^^^ help: try: `true`

error: this boolean expression can be simplified
--> $DIR/blocks_in_if_conditions.rs:39:8
--> $DIR/blocks_in_conditions.rs:41:8
|
LL | if true && x == 3 { 6 } else { 10 }
| ^^^^^^^^^^^^^^ help: try: `x == 3`
|
= note: `-D clippy::nonminimal-bool` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::nonminimal_bool)]`

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`
--> $DIR/blocks_in_if_conditions.rs:66:5
error: in a `match` scrutinee, avoid complex blocks or closures with blocks; instead, move the block or closure higher and bind it with a `let`
--> $DIR/blocks_in_conditions.rs:68:5
|
LL | / match {
LL | |
LL | | let opt = Some(2);
LL | | opt
LL | | } {
Expand All @@ -44,6 +47,7 @@ LL | | } {
help: try
|
LL ~ let res = {
LL +
LL + let opt = Some(2);
LL + opt
LL ~ }; match res {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![warn(clippy::blocks_in_if_conditions)]
#![warn(clippy::blocks_in_conditions)]
#![allow(
unused,
clippy::let_and_return,
Expand All @@ -22,7 +22,7 @@ fn pred_test() {
&& predicate(
|x| {
//~^ ERROR: in an `if` condition, avoid complex blocks or closures with blocks
//~| NOTE: `-D clippy::blocks-in-if-conditions` implied by `-D warnings`
//~| NOTE: `-D clippy::blocks-in-conditions` implied by `-D warnings`
let target = 3;
x == target
},
Expand Down Expand Up @@ -60,6 +60,23 @@ fn function_with_empty_closure() {
if closure(|| {}) {}
}

// issue #11814
fn match_with_pred() {
let v = 3;
match Some(predicate(
|x| {
//~^ ERROR: in a `match` scrutinee, avoid complex blocks or closures with blocks
let target = 3;
x == target
},
v,
)) {
Some(true) => 1,
Some(false) => 2,
None => 3,
};
}

#[rustfmt::skip]
fn main() {
let mut range = 0..10;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
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`
--> $DIR/blocks_in_if_conditions_closure.rs:23:17
--> $DIR/blocks_in_conditions_closure.rs:23:17
|
LL | |x| {
| _________________^
Expand All @@ -10,11 +10,11 @@ LL | | x == target
LL | | },
| |_____________^
|
= note: `-D clippy::blocks-in-if-conditions` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::blocks_in_if_conditions)]`
= note: `-D clippy::blocks-in-conditions` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::blocks_in_conditions)]`

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`
--> $DIR/blocks_in_if_conditions_closure.rs:34:13
--> $DIR/blocks_in_conditions_closure.rs:34:13
|
LL | |x| {
| _____________^
Expand All @@ -24,5 +24,16 @@ LL | | x == target
LL | | },
| |_________^

error: aborting due to 2 previous errors
error: in a `match` scrutinee, avoid complex blocks or closures with blocks; instead, move the block or closure higher and bind it with a `let`
--> $DIR/blocks_in_conditions_closure.rs:67:13
|
LL | |x| {
| _____________^
LL | |
LL | | let target = 3;
LL | | x == target
LL | | },
| |_________^

error: aborting due to 3 previous errors

2 changes: 1 addition & 1 deletion tests/ui/manual_filter.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ fn main() {
};
}

#[allow(clippy::blocks_in_if_conditions)]
#[allow(clippy::blocks_in_conditions)]
Some(11).filter(|&x| {
println!("foo");
x > 10 && x < 100
Expand Down

0 comments on commit 8a1bcaf

Please sign in to comment.