Skip to content

Commit bbc22e2

Browse files
borsbors
committed
Auto merge of #7083 - GuillaumeGomez:bool-assert-eq, r=camsteffen
Add lint to check for boolean comparison in assert macro calls This PR adds a lint to check if an assert macro is using a boolean as "comparison value". For example: ```rust assert_eq!("a".is_empty(), false); ``` Could be rewritten as: ```rust assert!(!"a".is_empty()); ``` PS: The dev guidelines are amazing. Thanks a lot for writing them! changelog: Add `bool_assert_comparison` lint
2 parents 926286a + e2e104b commit bbc22e2

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;
@@ -568,6 +569,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
568569
bit_mask::VERBOSE_BIT_MASK,
569570
blacklisted_name::BLACKLISTED_NAME,
570571
blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS,
572+
bool_assert_comparison::BOOL_ASSERT_COMPARISON,
571573
booleans::LOGIC_BUG,
572574
booleans::NONMINIMAL_BOOL,
573575
bytecount::NAIVE_BYTECOUNT,
@@ -1273,6 +1275,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
12731275
store.register_late_pass(|| box from_str_radix_10::FromStrRadix10);
12741276
store.register_late_pass(|| box manual_map::ManualMap);
12751277
store.register_late_pass(move || box if_then_some_else_none::IfThenSomeElseNone::new(msrv));
1278+
store.register_early_pass(|| box bool_assert_comparison::BoolAssertComparison);
12761279

12771280
store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
12781281
LintId::of(arithmetic::FLOAT_ARITHMETIC),
@@ -1453,6 +1456,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
14531456
LintId::of(bit_mask::INEFFECTIVE_BIT_MASK),
14541457
LintId::of(blacklisted_name::BLACKLISTED_NAME),
14551458
LintId::of(blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS),
1459+
LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON),
14561460
LintId::of(booleans::LOGIC_BUG),
14571461
LintId::of(booleans::NONMINIMAL_BOOL),
14581462
LintId::of(casts::CAST_REF_TO_MUT),
@@ -1739,6 +1743,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
17391743
LintId::of(attrs::BLANKET_CLIPPY_RESTRICTION_LINTS),
17401744
LintId::of(blacklisted_name::BLACKLISTED_NAME),
17411745
LintId::of(blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS),
1746+
LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON),
17421747
LintId::of(casts::FN_TO_NUMERIC_CAST),
17431748
LintId::of(casts::FN_TO_NUMERIC_CAST_WITH_TRUNCATION),
17441749
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)