Skip to content

Commit e2e104b

Browse files
GuillaumeGomezGuillaumeGomez
committed
Add lint to check for boolean comparison in assert macro calls
1 parent b1c675f commit e2e104b

File tree

6 files changed

+284
-0
lines changed

6 files changed

+284
-0
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2124,6 +2124,7 @@ Released 2018-09-13
21242124
[`blacklisted_name`]: https://rust-lang.github.io/rust-clippy/master/index.html#blacklisted_name
21252125
[`blanket_clippy_restriction_lints`]: https://rust-lang.github.io/rust-clippy/master/index.html#blanket_clippy_restriction_lints
21262126
[`blocks_in_if_conditions`]: https://rust-lang.github.io/rust-clippy/master/index.html#blocks_in_if_conditions
2127+
[`bool_assert_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
21272128
[`bool_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_comparison
21282129
[`borrow_interior_mutable_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrow_interior_mutable_const
21292130
[`borrowed_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrowed_box

‎clippy_lints/src/bool_assert_comparison.rs‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use clippy_utils::{ast_utils, is_direct_expn_of};
3+
use rustc_ast::ast::{Expr, ExprKind, Lit, LitKind};
4+
use rustc_errors::Applicability;
5+
use rustc_lint::{EarlyContext, EarlyLintPass};
6+
use rustc_session::{declare_lint_pass, declare_tool_lint};
7+
8+
declare_clippy_lint! {
9+
/// **What it does:** This lint warns about boolean comparisons in assert-like macros.
10+
///
11+
/// **Why is this bad?** It is shorter to use the equivalent.
12+
///
13+
/// **Known problems:** None.
14+
///
15+
/// **Example:**
16+
///
17+
/// ```rust
18+
/// // Bad
19+
/// assert_eq!("a".is_empty(), false);
20+
/// assert_ne!("a".is_empty(), true);
21+
///
22+
/// // Good
23+
/// assert!(!"a".is_empty());
24+
/// ```
25+
pub BOOL_ASSERT_COMPARISON,
26+
style,
27+
"Using a boolean as comparison value in an assert_* macro when there is no need"
28+
}
29+
30+
declare_lint_pass!(BoolAssertComparison => [BOOL_ASSERT_COMPARISON]);
31+
32+
fn is_bool_lit(e: &Expr) -> bool {
33+
matches!(
34+
e.kind,
35+
ExprKind::Lit(Lit {
36+
kind: LitKind::Bool(_),
37+
..
38+
})
39+
) && !e.span.from_expansion()
40+
}
41+
42+
impl EarlyLintPass for BoolAssertComparison {
43+
fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &Expr) {
44+
let macros = ["assert_eq", "debug_assert_eq"];
45+
let inverted_macros = ["assert_ne", "debug_assert_ne"];
46+
47+
for mac in macros.iter().chain(inverted_macros.iter()) {
48+
if let Some(span) = is_direct_expn_of(e.span, mac) {
49+
if let Some([a, b]) = ast_utils::extract_assert_macro_args(e) {
50+
let nb_bool_args = is_bool_lit(a) as usize + is_bool_lit(b) as usize;
51+
52+
if nb_bool_args != 1 {
53+
// If there are two boolean arguments, we definitely don't understand
54+
// what's going on, so better leave things as is...
55+
//
56+
// Or there is simply no boolean and then we can leave things as is!
57+
return;
58+
}
59+
60+
let non_eq_mac = &mac[..mac.len() - 3];
61+
span_lint_and_sugg(
62+
cx,
63+
BOOL_ASSERT_COMPARISON,
64+
span,
65+
&format!("used `{}!` with a literal bool", mac),
66+
"replace it with",
67+
format!("{}!(..)", non_eq_mac),
68+
Applicability::MaybeIncorrect,
69+
);
70+
return;
71+
}
72+
}
73+
}
74+
}
75+
}

‎clippy_lints/src/lib.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,7 @@ mod await_holding_invalid;
178178
mod bit_mask;
179179
mod blacklisted_name;
180180
mod blocks_in_if_conditions;
181+
mod bool_assert_comparison;
181182
mod booleans;
182183
mod bytecount;
183184
mod cargo_common_metadata;
@@ -567,6 +568,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
567568
bit_mask::VERBOSE_BIT_MASK,
568569
blacklisted_name::BLACKLISTED_NAME,
569570
blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS,
571+
bool_assert_comparison::BOOL_ASSERT_COMPARISON,
570572
booleans::LOGIC_BUG,
571573
booleans::NONMINIMAL_BOOL,
572574
bytecount::NAIVE_BYTECOUNT,
@@ -1270,6 +1272,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
12701272
store.register_late_pass(|| box from_str_radix_10::FromStrRadix10);
12711273
store.register_late_pass(|| box manual_map::ManualMap);
12721274
store.register_late_pass(move || box if_then_some_else_none::IfThenSomeElseNone::new(msrv));
1275+
store.register_early_pass(|| box bool_assert_comparison::BoolAssertComparison);
12731276

12741277
store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
12751278
LintId::of(arithmetic::FLOAT_ARITHMETIC),
@@ -1448,6 +1451,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
14481451
LintId::of(bit_mask::INEFFECTIVE_BIT_MASK),
14491452
LintId::of(blacklisted_name::BLACKLISTED_NAME),
14501453
LintId::of(blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS),
1454+
LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON),
14511455
LintId::of(booleans::LOGIC_BUG),
14521456
LintId::of(booleans::NONMINIMAL_BOOL),
14531457
LintId::of(casts::CAST_REF_TO_MUT),
@@ -1734,6 +1738,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
17341738
LintId::of(attrs::BLANKET_CLIPPY_RESTRICTION_LINTS),
17351739
LintId::of(blacklisted_name::BLACKLISTED_NAME),
17361740
LintId::of(blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS),
1741+
LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON),
17371742
LintId::of(casts::FN_TO_NUMERIC_CAST),
17381743
LintId::of(casts::FN_TO_NUMERIC_CAST_WITH_TRUNCATION),
17391744
LintId::of(collapsible_if::COLLAPSIBLE_ELSE_IF),

‎clippy_utils/src/ast_utils.rs‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#![allow(clippy::similar_names, clippy::wildcard_imports, clippy::enum_glob_use)]
66

77
use crate::{both, over};
8+
use if_chain::if_chain;
89
use rustc_ast::ptr::P;
910
use rustc_ast::{self as ast, *};
1011
use rustc_span::symbol::Ident;
@@ -571,3 +572,34 @@ pub fn eq_mac_args(l: &MacArgs, r: &MacArgs) -> bool {
571572
_ => false,
572573
}
573574
}
575+
576+
/// Extract args from an assert-like macro.
577+
///
578+
/// Currently working with:
579+
/// - `assert_eq!` and `assert_ne!`
580+
/// - `debug_assert_eq!` and `debug_assert_ne!`
581+
///
582+
/// For example:
583+
///
584+
/// `debug_assert_eq!(a, b)` will return Some([a, b])
585+
pub fn extract_assert_macro_args(mut expr: &Expr) -> Option<[&Expr; 2]> {
586+
if_chain! {
587+
if let ExprKind::If(_, ref block, _) = expr.kind;
588+
if let StmtKind::Semi(ref e) = block.stmts.get(0)?.kind;
589+
then {
590+
expr = e;
591+
}
592+
}
593+
if_chain! {
594+
if let ExprKind::Block(ref block, _) = expr.kind;
595+
if let StmtKind::Expr(ref expr) = block.stmts.get(0)?.kind;
596+
if let ExprKind::Match(ref match_expr, _) = expr.kind;
597+
if let ExprKind::Tup(ref tup) = match_expr.kind;
598+
if let [a, b, ..] = tup.as_slice();
599+
if let (&ExprKind::AddrOf(_, _, ref a), &ExprKind::AddrOf(_, _, ref b)) = (&a.kind, &b.kind);
600+
then {
601+
return Some([&*a, &*b]);
602+
}
603+
}
604+
None
605+
}

‎tests/ui/bool_assert_comparison.rs‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
#![warn(clippy::bool_assert_comparison)]
2+
3+
macro_rules! a {
4+
() => {
5+
true
6+
};
7+
}
8+
macro_rules! b {
9+
() => {
10+
true
11+
};
12+
}
13+
14+
fn main() {
15+
assert_eq!("a".len(), 1);
16+
assert_eq!("a".is_empty(), false);
17+
assert_eq!("".is_empty(), true);
18+
assert_eq!(true, "".is_empty());
19+
assert_eq!(a!(), b!());
20+
assert_eq!(a!(), "".is_empty());
21+
assert_eq!("".is_empty(), b!());
22+
23+
assert_ne!("a".len(), 1);
24+
assert_ne!("a".is_empty(), false);
25+
assert_ne!("".is_empty(), true);
26+
assert_ne!(true, "".is_empty());
27+
assert_ne!(a!(), b!());
28+
assert_ne!(a!(), "".is_empty());
29+
assert_ne!("".is_empty(), b!());
30+
31+
debug_assert_eq!("a".len(), 1);
32+
debug_assert_eq!("a".is_empty(), false);
33+
debug_assert_eq!("".is_empty(), true);
34+
debug_assert_eq!(true, "".is_empty());
35+
debug_assert_eq!(a!(), b!());
36+
debug_assert_eq!(a!(), "".is_empty());
37+
debug_assert_eq!("".is_empty(), b!());
38+
39+
debug_assert_ne!("a".len(), 1);
40+
debug_assert_ne!("a".is_empty(), false);
41+
debug_assert_ne!("".is_empty(), true);
42+
debug_assert_ne!(true, "".is_empty());
43+
debug_assert_ne!(a!(), b!());
44+
debug_assert_ne!(a!(), "".is_empty());
45+
debug_assert_ne!("".is_empty(), b!());
46+
47+
// assert with error messages
48+
assert_eq!("a".len(), 1, "tadam {}", 1);
49+
assert_eq!("a".len(), 1, "tadam {}", true);
50+
assert_eq!("a".is_empty(), false, "tadam {}", 1);
51+
assert_eq!("a".is_empty(), false, "tadam {}", true);
52+
assert_eq!(false, "a".is_empty(), "tadam {}", true);
53+
54+
debug_assert_eq!("a".len(), 1, "tadam {}", 1);
55+
debug_assert_eq!("a".len(), 1, "tadam {}", true);
56+
debug_assert_eq!("a".is_empty(), false, "tadam {}", 1);
57+
debug_assert_eq!("a".is_empty(), false, "tadam {}", true);
58+
debug_assert_eq!(false, "a".is_empty(), "tadam {}", true);
59+
}

‎tests/ui/bool_assert_comparison.stderr‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
error: used `assert_eq!` with a literal bool
2+
--> $DIR/bool_assert_comparison.rs:16:5
3+
|
4+
LL | assert_eq!("a".is_empty(), false);
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
6+
|
7+
= note: `-D clippy::bool-assert-comparison` implied by `-D warnings`
8+
9+
error: used `assert_eq!` with a literal bool
10+
--> $DIR/bool_assert_comparison.rs:17:5
11+
|
12+
LL | assert_eq!("".is_empty(), true);
13+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
14+
15+
error: used `assert_eq!` with a literal bool
16+
--> $DIR/bool_assert_comparison.rs:18:5
17+
|
18+
LL | assert_eq!(true, "".is_empty());
19+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
20+
21+
error: used `assert_ne!` with a literal bool
22+
--> $DIR/bool_assert_comparison.rs:24:5
23+
|
24+
LL | assert_ne!("a".is_empty(), false);
25+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
26+
27+
error: used `assert_ne!` with a literal bool
28+
--> $DIR/bool_assert_comparison.rs:25:5
29+
|
30+
LL | assert_ne!("".is_empty(), true);
31+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
32+
33+
error: used `assert_ne!` with a literal bool
34+
--> $DIR/bool_assert_comparison.rs:26:5
35+
|
36+
LL | assert_ne!(true, "".is_empty());
37+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
38+
39+
error: used `debug_assert_eq!` with a literal bool
40+
--> $DIR/bool_assert_comparison.rs:32:5
41+
|
42+
LL | debug_assert_eq!("a".is_empty(), false);
43+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
44+
45+
error: used `debug_assert_eq!` with a literal bool
46+
--> $DIR/bool_assert_comparison.rs:33:5
47+
|
48+
LL | debug_assert_eq!("".is_empty(), true);
49+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
50+
51+
error: used `debug_assert_eq!` with a literal bool
52+
--> $DIR/bool_assert_comparison.rs:34:5
53+
|
54+
LL | debug_assert_eq!(true, "".is_empty());
55+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
56+
57+
error: used `debug_assert_ne!` with a literal bool
58+
--> $DIR/bool_assert_comparison.rs:40:5
59+
|
60+
LL | debug_assert_ne!("a".is_empty(), false);
61+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
62+
63+
error: used `debug_assert_ne!` with a literal bool
64+
--> $DIR/bool_assert_comparison.rs:41:5
65+
|
66+
LL | debug_assert_ne!("".is_empty(), true);
67+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
68+
69+
error: used `debug_assert_ne!` with a literal bool
70+
--> $DIR/bool_assert_comparison.rs:42:5
71+
|
72+
LL | debug_assert_ne!(true, "".is_empty());
73+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
74+
75+
error: used `assert_eq!` with a literal bool
76+
--> $DIR/bool_assert_comparison.rs:50:5
77+
|
78+
LL | assert_eq!("a".is_empty(), false, "tadam {}", 1);
79+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
80+
81+
error: used `assert_eq!` with a literal bool
82+
--> $DIR/bool_assert_comparison.rs:51:5
83+
|
84+
LL | assert_eq!("a".is_empty(), false, "tadam {}", true);
85+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
86+
87+
error: used `assert_eq!` with a literal bool
88+
--> $DIR/bool_assert_comparison.rs:52:5
89+
|
90+
LL | assert_eq!(false, "a".is_empty(), "tadam {}", true);
91+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
92+
93+
error: used `debug_assert_eq!` with a literal bool
94+
--> $DIR/bool_assert_comparison.rs:56:5
95+
|
96+
LL | debug_assert_eq!("a".is_empty(), false, "tadam {}", 1);
97+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
98+
99+
error: used `debug_assert_eq!` with a literal bool
100+
--> $DIR/bool_assert_comparison.rs:57:5
101+
|
102+
LL | debug_assert_eq!("a".is_empty(), false, "tadam {}", true);
103+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
104+
105+
error: used `debug_assert_eq!` with a literal bool
106+
--> $DIR/bool_assert_comparison.rs:58:5
107+
|
108+
LL | debug_assert_eq!(false, "a".is_empty(), "tadam {}", true);
109+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
110+
111+
error: aborting due to 18 previous errors
112+

0 commit comments

Comments
 (0)