Skip to content

Commit 4bf19dc

Browse files
couchandcouchand
committed
Prefer assert_eq!(.., false) to assert!(!..)
1 parent 94ca94f commit 4bf19dc

10 files changed

+161
-3
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -2772,6 +2772,7 @@ Released 2018-09-13
27722772
[`blanket_clippy_restriction_lints`]: https://rust-lang.github.io/rust-clippy/master/index.html#blanket_clippy_restriction_lints
27732773
[`blocks_in_if_conditions`]: https://rust-lang.github.io/rust-clippy/master/index.html#blocks_in_if_conditions
27742774
[`bool_assert_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
2775+
[`bool_assert_inverted`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_inverted
27752776
[`bool_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_comparison
27762777
[`borrow_interior_mutable_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrow_interior_mutable_const
27772778
[`borrowed_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrowed_box

‎clippy_lints/src/bool_assert_comparison.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ declare_clippy_lint! {
2525
/// ```
2626
#[clippy::version = "1.53.0"]
2727
pub BOOL_ASSERT_COMPARISON,
28-
style,
28+
pedantic,
2929
"Using a boolean as comparison value in an assert_* macro when there is no need"
3030
}
3131

‎clippy_lints/src/bool_assert_inverted.rs

+70
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
use clippy_utils::{diagnostics::span_lint_and_sugg, higher, is_direct_expn_of};
2+
use rustc_errors::Applicability;
3+
use rustc_hir::{Expr, ExprKind, UnOp};
4+
use rustc_lint::{LateContext, LateLintPass};
5+
use rustc_session::{declare_lint_pass, declare_tool_lint};
6+
7+
declare_clippy_lint! {
8+
/// ### What it does
9+
/// This lint warns about the use of inverted conditions in assert-like macros.
10+
///
11+
/// ### Why is this bad?
12+
/// It is all too easy to misread the semantics of an assertion when the
13+
/// logic of the condition is reversed. Explicitly comparing to a boolean
14+
/// value is preferable.
15+
///
16+
/// ### Example
17+
/// ```rust
18+
/// // Bad
19+
/// assert!(!"a".is_empty());
20+
///
21+
/// // Good
22+
/// assert_eq!("a".is_empty(), false);
23+
///
24+
/// // Okay
25+
/// assert_ne!("a".is_empty(), true);
26+
/// ```
27+
#[clippy::version = "1.XX.0"]
28+
pub BOOL_ASSERT_INVERTED,
29+
style,
30+
"Asserting on an inverted condition"
31+
}
32+
33+
declare_lint_pass!(BoolAssertInverted => [BOOL_ASSERT_INVERTED]);
34+
35+
fn is_inverted(e: &Expr<'_>) -> bool {
36+
matches!(
37+
e.kind,
38+
ExprKind::Unary(UnOp::Not, _),
39+
) && !e.span.from_expansion()
40+
}
41+
42+
impl<'tcx> LateLintPass<'tcx> for BoolAssertInverted {
43+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
44+
let macros = ["assert", "debug_assert"];
45+
46+
for mac in macros.iter() {
47+
if let Some(span) = is_direct_expn_of(expr.span, mac) {
48+
if let Some(args) = higher::extract_assert_macro_args(expr) {
49+
if let [a, ..] = args[..] {
50+
if !is_inverted(a) {
51+
return;
52+
}
53+
54+
let eq_mac = format!("{}_eq", mac);
55+
span_lint_and_sugg(
56+
cx,
57+
BOOL_ASSERT_INVERTED,
58+
span,
59+
&format!("used `{}!` with an inverted condition", mac),
60+
"replace it with",
61+
format!("{}!(.., false, ..)", eq_mac),
62+
Applicability::MaybeIncorrect,
63+
);
64+
return;
65+
}
66+
}
67+
}
68+
}
69+
}
70+
}

‎clippy_lints/src/lib.register_all.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
1818
LintId::of(bit_mask::INEFFECTIVE_BIT_MASK),
1919
LintId::of(blacklisted_name::BLACKLISTED_NAME),
2020
LintId::of(blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS),
21-
LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON),
21+
LintId::of(bool_assert_inverted::BOOL_ASSERT_INVERTED),
2222
LintId::of(booleans::LOGIC_BUG),
2323
LintId::of(booleans::NONMINIMAL_BOOL),
2424
LintId::of(casts::CAST_REF_TO_MUT),

‎clippy_lints/src/lib.register_lints.rs

+1
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ store.register_lints(&[
5757
blacklisted_name::BLACKLISTED_NAME,
5858
blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS,
5959
bool_assert_comparison::BOOL_ASSERT_COMPARISON,
60+
bool_assert_inverted::BOOL_ASSERT_INVERTED,
6061
booleans::LOGIC_BUG,
6162
booleans::NONMINIMAL_BOOL,
6263
bytecount::NAIVE_BYTECOUNT,

‎clippy_lints/src/lib.register_pedantic.rs

+1
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![
77
LintId::of(await_holding_invalid::AWAIT_HOLDING_LOCK),
88
LintId::of(await_holding_invalid::AWAIT_HOLDING_REFCELL_REF),
99
LintId::of(bit_mask::VERBOSE_BIT_MASK),
10+
LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON),
1011
LintId::of(bytecount::NAIVE_BYTECOUNT),
1112
LintId::of(case_sensitive_file_extension_comparisons::CASE_SENSITIVE_FILE_EXTENSION_COMPARISONS),
1213
LintId::of(casts::CAST_LOSSLESS),

‎clippy_lints/src/lib.register_style.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![
77
LintId::of(assign_ops::ASSIGN_OP_PATTERN),
88
LintId::of(blacklisted_name::BLACKLISTED_NAME),
99
LintId::of(blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS),
10-
LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON),
10+
LintId::of(bool_assert_inverted::BOOL_ASSERT_INVERTED),
1111
LintId::of(casts::FN_TO_NUMERIC_CAST),
1212
LintId::of(casts::FN_TO_NUMERIC_CAST_WITH_TRUNCATION),
1313
LintId::of(collapsible_if::COLLAPSIBLE_ELSE_IF),

‎clippy_lints/src/lib.rs

+2
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,7 @@ mod bit_mask;
176176
mod blacklisted_name;
177177
mod blocks_in_if_conditions;
178178
mod bool_assert_comparison;
179+
mod bool_assert_inverted;
179180
mod booleans;
180181
mod bytecount;
181182
mod cargo_common_metadata;
@@ -825,6 +826,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
825826
store.register_late_pass(|| Box::new(manual_map::ManualMap));
826827
store.register_late_pass(move || Box::new(if_then_some_else_none::IfThenSomeElseNone::new(msrv)));
827828
store.register_late_pass(|| Box::new(bool_assert_comparison::BoolAssertComparison));
829+
store.register_late_pass(|| Box::new(bool_assert_inverted::BoolAssertInverted));
828830
store.register_early_pass(move || Box::new(module_style::ModStyle));
829831
store.register_late_pass(|| Box::new(unused_async::UnusedAsync));
830832
let disallowed_types = conf.disallowed_types.clone();

‎tests/ui/bool_assert_inverted.rs

+55
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
#![warn(clippy::bool_assert_inverted)]
2+
3+
use std::ops::Not;
4+
5+
macro_rules! a {
6+
() => {
7+
true
8+
};
9+
}
10+
macro_rules! b {
11+
() => {
12+
true
13+
};
14+
}
15+
16+
#[derive(Debug, Clone, Copy)]
17+
struct ImplNotTraitWithBool;
18+
19+
impl PartialEq<bool> for ImplNotTraitWithBool {
20+
fn eq(&self, other: &bool) -> bool {
21+
false
22+
}
23+
}
24+
25+
impl Not for ImplNotTraitWithBool {
26+
type Output = bool;
27+
28+
fn not(self) -> Self::Output {
29+
true
30+
}
31+
}
32+
33+
fn main() {
34+
let a = ImplNotTraitWithBool;
35+
36+
assert!(!"a".is_empty());
37+
assert!("".is_empty());
38+
assert!(!a);
39+
assert!(a);
40+
41+
debug_assert!(!"a".is_empty());
42+
debug_assert!("".is_empty());
43+
debug_assert!(!a);
44+
debug_assert!(a);
45+
46+
assert!(!"a".is_empty(), "tadam {}", false);
47+
assert!("".is_empty(), "tadam {}", false);
48+
assert!(!a, "tadam {}", false);
49+
assert!(a, "tadam {}", false);
50+
51+
debug_assert!(!"a".is_empty(), "tadam {}", false);
52+
debug_assert!("".is_empty(), "tadam {}", false);
53+
debug_assert!(!a, "tadam {}", false);
54+
debug_assert!(a, "tadam {}", false);
55+
}

‎tests/ui/bool_assert_inverted.stderr

+28
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
error: used `debug_assert!` with an inverted condition
2+
--> $DIR/bool_assert_inverted.rs:41:5
3+
|
4+
LL | debug_assert!(!"a".is_empty());
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert_eq!(.., false, ..)`
6+
|
7+
= note: `-D clippy::bool-assert-inverted` implied by `-D warnings`
8+
9+
error: used `debug_assert!` with an inverted condition
10+
--> $DIR/bool_assert_inverted.rs:43:5
11+
|
12+
LL | debug_assert!(!a);
13+
| ^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert_eq!(.., false, ..)`
14+
15+
error: used `debug_assert!` with an inverted condition
16+
--> $DIR/bool_assert_inverted.rs:51:5
17+
|
18+
LL | debug_assert!(!"a".is_empty(), "tadam {}", false);
19+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert_eq!(.., false, ..)`
20+
21+
error: used `debug_assert!` with an inverted condition
22+
--> $DIR/bool_assert_inverted.rs:53:5
23+
|
24+
LL | debug_assert!(!a, "tadam {}", false);
25+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert_eq!(.., false, ..)`
26+
27+
error: aborting due to 4 previous errors
28+

0 commit comments

Comments
 (0)