Skip to content

Commit

Permalink
match_bool: omit suggestion if guard is present
Browse files Browse the repository at this point in the history 
Without this check, the lint would suggest that

```rust
    match test {
        true if option == 5 => 10,
        _ => 1,
    };
```

is replaced by `if test { 10 } else { 1 }`.
  • Loading branch information
@samueltardieu
samueltardieu committed Jan 20, 2025
1 parent 2280b8a commit 1763d49
Show file tree
Hide file tree
Showing 4 changed files with 195 additions and 54 deletions.
51 changes: 27 additions & 24 deletions clippy_lints/src/matches/match_bool.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::is_unit_expr;
use clippy_utils::source::{expr_block, snippet};
use clippy_utils::source::expr_block;
use clippy_utils::sugg::Sugg;
use rustc_ast::LitKind;
use rustc_errors::Applicability;
Expand All @@ -20,54 +20,57 @@ pub(crate) fn check(cx: &LateContext<'_>, scrutinee: &Expr<'_>, arms: &[Arm<'_>]
"you seem to be trying to match on a boolean expression",
move |diag| {
if arms.len() == 2 {
// no guards
let exprs = if let PatKind::Lit(arm_bool) = arms[0].pat.kind {
let mut app = Applicability::MachineApplicable;
let test_sugg = if let PatKind::Lit(arm_bool) = arms[0].pat.kind {
let guard = arms[0]
.guard
.map(|g| Sugg::hir_with_applicability(cx, g, "_", &mut app));
let test = Sugg::hir_with_applicability(cx, scrutinee, "_", &mut app);
if let ExprKind::Lit(lit) = arm_bool.kind {
match lit.node {
LitKind::Bool(true) => Some((arms[0].body, arms[1].body)),
LitKind::Bool(false) => Some((arms[1].body, arms[0].body)),
match &lit.node {
LitKind::Bool(true) => Some(test),
LitKind::Bool(false) => Some(!test),
_ => None,
}
.map(|test| {
if let Some(guard) = &guard {
test.and(guard)
} else {
test
}
})
} else {
None
}
} else {
None
};

if let Some((true_expr, false_expr)) = exprs {
let mut app = Applicability::HasPlaceholders;
if let Some(test_sugg) = test_sugg {
let ctxt = expr.span.ctxt();
let (true_expr, false_expr) = (arms[0].body, arms[1].body);
let sugg = match (is_unit_expr(true_expr), is_unit_expr(false_expr)) {
(false, false) => Some(format!(
"if {} {} else {}",
snippet(cx, scrutinee.span, "b"),
test_sugg,
expr_block(cx, true_expr, ctxt, "..", Some(expr.span), &mut app),
expr_block(cx, false_expr, ctxt, "..", Some(expr.span), &mut app)
)),
(false, true) => Some(format!(
"if {} {}",
snippet(cx, scrutinee.span, "b"),
test_sugg,
expr_block(cx, true_expr, ctxt, "..", Some(expr.span), &mut app)
)),
(true, false) => {
let test = Sugg::hir(cx, scrutinee, "..");
Some(format!(
"if {} {}",
!test,
expr_block(cx, false_expr, ctxt, "..", Some(expr.span), &mut app)
))
},
(true, false) => Some(format!(
"if {} {}",
!test_sugg,
expr_block(cx, false_expr, ctxt, "..", Some(expr.span), &mut app)
)),
(true, true) => None,
};

if let Some(sugg) = sugg {
diag.span_suggestion(
expr.span,
"consider using an `if`/`else` expression",
sugg,
Applicability::HasPlaceholders,
);
diag.span_suggestion(expr.span, "consider using an `if`/`else` expression", sugg, app);
}
}
}
Expand Down
58 changes: 58 additions & 0 deletions tests/ui/match_bool.fixed
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
#![deny(clippy::match_bool)]
#![allow(clippy::nonminimal_bool, clippy::eq_op)]

fn match_bool() {
let test: bool = true;

if test { 0 } else { 42 };

let option = 1;
if option == 1 { 1 } else { 0 };

if !test {
println!("Noooo!");
};

if !test {
println!("Noooo!");
};

if !(test && test) {
println!("Noooo!");
};

if !test {
println!("Noooo!");
} else {
println!("Yes!");
};

// Not linted
match option {
1..=10 => 1,
11..=20 => 2,
_ => 3,
};

// Don't lint
let _ = match test {
#[cfg(feature = "foo")]
true if option == 5 => 10,
true => 0,
false => 1,
};

let _ = if test && option == 5 { 10 } else { 1 };

let _ = if !test && option == 5 { 10 } else { 1 };

if test && option == 5 { println!("Hello") };

if !(test && option == 5) { println!("Hello") };

if !test && option == 5 { println!("Hello") };

if !(!test && option == 5) { println!("Hello") };
}

fn main() {}
44 changes: 38 additions & 6 deletions tests/ui/match_bool.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//@no-rustfix: overlapping suggestions
#![deny(clippy::match_bool)]
#![allow(clippy::nonminimal_bool, clippy::eq_op)]

fn match_bool() {
let test: bool = true;
Expand Down Expand Up @@ -34,11 +34,7 @@ fn match_bool() {
};

match test && test {
//~^ ERROR: this boolean expression can be simplified
//~| NOTE: `-D clippy::nonminimal-bool` implied by `-D warnings`
//~| ERROR: you seem to be trying to match on a boolean expression
//~| ERROR: equal expressions as operands to `&&`
//~| NOTE: `#[deny(clippy::eq_op)]` on by default
//~^ ERROR: you seem to be trying to match on a boolean expression
false => {
println!("Noooo!");
},
Expand Down Expand Up @@ -69,6 +65,42 @@ fn match_bool() {
true => 0,
false => 1,
};

let _ = match test {
//~^ ERROR: you seem to be trying to match on a boolean expression
true if option == 5 => 10,
_ => 1,
};

let _ = match test {
//~^ ERROR: you seem to be trying to match on a boolean expression
false if option == 5 => 10,
_ => 1,
};

match test {
//~^ ERROR: you seem to be trying to match on a boolean expression
true if option == 5 => println!("Hello"),
_ => (),
};

match test {
//~^ ERROR: you seem to be trying to match on a boolean expression
true if option == 5 => (),
_ => println!("Hello"),
};

match test {
//~^ ERROR: you seem to be trying to match on a boolean expression
false if option == 5 => println!("Hello"),
_ => (),
};

match test {
//~^ ERROR: you seem to be trying to match on a boolean expression
false if option == 5 => (),
_ => println!("Hello"),
};
}

fn main() {}
96 changes: 72 additions & 24 deletions tests/ui/match_bool.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,3 @@
error: this boolean expression can be simplified
--> tests/ui/match_bool.rs:36:11
|
LL | match test && test {
| ^^^^^^^^^^^^ help: try: `test`
|
= note: `-D clippy::nonminimal-bool` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::nonminimal_bool)]`

error: you seem to be trying to match on a boolean expression
--> tests/ui/match_bool.rs:7:5
|
Expand All @@ -18,7 +9,7 @@ LL | | };
| |_____^ help: consider using an `if`/`else` expression: `if test { 0 } else { 42 }`
|
note: the lint level is defined here
--> tests/ui/match_bool.rs:2:9
--> tests/ui/match_bool.rs:1:9
|
LL | #![deny(clippy::match_bool)]
| ^^^^^^^^^^^^^^^^^^
Expand Down Expand Up @@ -75,7 +66,10 @@ error: you seem to be trying to match on a boolean expression
--> tests/ui/match_bool.rs:36:5
|
LL | / match test && test {
... |
LL | |
LL | | false => {
LL | | println!("Noooo!");
LL | | },
LL | | _ => (),
LL | | };
| |_____^
Expand All @@ -87,16 +81,8 @@ LL + println!("Noooo!");
LL ~ };
|

error: equal expressions as operands to `&&`
--> tests/ui/match_bool.rs:36:11
|
LL | match test && test {
| ^^^^^^^^^^^^
|
= note: `#[deny(clippy::eq_op)]` on by default

error: you seem to be trying to match on a boolean expression
--> tests/ui/match_bool.rs:48:5
--> tests/ui/match_bool.rs:44:5
|
LL | / match test {
LL | |
Expand All @@ -109,12 +95,74 @@ LL | | };
|
help: consider using an `if`/`else` expression
|
LL ~ if test {
LL + println!("Yes!");
LL + } else {
LL ~ if !test {
LL + println!("Noooo!");
LL + } else {
LL + println!("Yes!");
LL ~ };
|

error: aborting due to 8 previous errors
error: you seem to be trying to match on a boolean expression
--> tests/ui/match_bool.rs:69:13
|
LL | let _ = match test {
| _____________^
LL | |
LL | | true if option == 5 => 10,
LL | | _ => 1,
LL | | };
| |_____^ help: consider using an `if`/`else` expression: `if test && option == 5 { 10 } else { 1 }`

error: you seem to be trying to match on a boolean expression
--> tests/ui/match_bool.rs:75:13
|
LL | let _ = match test {
| _____________^
LL | |
LL | | false if option == 5 => 10,
LL | | _ => 1,
LL | | };
| |_____^ help: consider using an `if`/`else` expression: `if !test && option == 5 { 10 } else { 1 }`

error: you seem to be trying to match on a boolean expression
--> tests/ui/match_bool.rs:81:5
|
LL | / match test {
LL | |
LL | | true if option == 5 => println!("Hello"),
LL | | _ => (),
LL | | };
| |_____^ help: consider using an `if`/`else` expression: `if test && option == 5 { println!("Hello") }`

error: you seem to be trying to match on a boolean expression
--> tests/ui/match_bool.rs:87:5
|
LL | / match test {
LL | |
LL | | true if option == 5 => (),
LL | | _ => println!("Hello"),
LL | | };
| |_____^ help: consider using an `if`/`else` expression: `if !(test && option == 5) { println!("Hello") }`

error: you seem to be trying to match on a boolean expression
--> tests/ui/match_bool.rs:93:5
|
LL | / match test {
LL | |
LL | | false if option == 5 => println!("Hello"),
LL | | _ => (),
LL | | };
| |_____^ help: consider using an `if`/`else` expression: `if !test && option == 5 { println!("Hello") }`

error: you seem to be trying to match on a boolean expression
--> tests/ui/match_bool.rs:99:5
|
LL | / match test {
LL | |
LL | | false if option == 5 => (),
LL | | _ => println!("Hello"),
LL | | };
| |_____^ help: consider using an `if`/`else` expression: `if !(!test && option == 5) { println!("Hello") }`

error: aborting due to 12 previous errors

0 comments on commit 1763d49

Please sign in to comment.