Skip to content

Commit 87c375f

Browse files
borsbors
committed
Auto merge of rust-lang#7605 - xordi:issue-7548-fix, r=giraffate
Issue 7548 fix Close rust-lang#7548 changelog: [`bool_assert_comparison`] fixes should be emitted only in case they are comparing a value of a type that implements the `Not` trait with an output of type `bool` against a boolean literal.
2 parents fd30241 + 83f1454 commit 87c375f

File tree

4 files changed

+169
-48
lines changed

4 files changed

+169
-48
lines changed

‎clippy_lints/src/bool_assert_comparison.rs‎

Lines changed: 62 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
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};
1+
use clippy_utils::{diagnostics::span_lint_and_sugg, higher, is_direct_expn_of, ty::implements_trait};
2+
use rustc_ast::ast::LitKind;
43
use rustc_errors::Applicability;
5-
use rustc_lint::{EarlyContext, EarlyLintPass};
4+
use rustc_hir::{Expr, ExprKind, Lit};
5+
use rustc_lint::{LateContext, LateLintPass};
6+
use rustc_middle::ty;
67
use rustc_session::{declare_lint_pass, declare_tool_lint};
8+
use rustc_span::symbol::Ident;
79

810
declare_clippy_lint! {
911
/// ### What it does
@@ -28,45 +30,77 @@ declare_clippy_lint! {
2830

2931
declare_lint_pass!(BoolAssertComparison => [BOOL_ASSERT_COMPARISON]);
3032

31-
fn is_bool_lit(e: &Expr) -> bool {
33+
fn is_bool_lit(e: &Expr<'_>) -> bool {
3234
matches!(
3335
e.kind,
3436
ExprKind::Lit(Lit {
35-
kind: LitKind::Bool(_),
37+
node: LitKind::Bool(_),
3638
..
3739
})
3840
) && !e.span.from_expansion()
3941
}
4042

41-
impl EarlyLintPass for BoolAssertComparison {
42-
fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &Expr) {
43+
fn is_impl_not_trait_with_bool_out(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool {
44+
let ty = cx.typeck_results().expr_ty(e);
45+
46+
cx.tcx
47+
.lang_items()
48+
.not_trait()
49+
.filter(|trait_id| implements_trait(cx, ty, *trait_id, &[]))
50+
.and_then(|trait_id| {
51+
cx.tcx.associated_items(trait_id).find_by_name_and_kind(
52+
cx.tcx,
53+
Ident::from_str("Output"),
54+
ty::AssocKind::Type,
55+
trait_id,
56+
)
57+
})
58+
.map_or(false, |assoc_item| {
59+
let proj = cx.tcx.mk_projection(assoc_item.def_id, cx.tcx.mk_substs_trait(ty, &[]));
60+
let nty = cx.tcx.normalize_erasing_regions(cx.param_env, proj);
61+
62+
nty.is_bool()
63+
})
64+
}
65+
66+
impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
67+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
4368
let macros = ["assert_eq", "debug_assert_eq"];
4469
let inverted_macros = ["assert_ne", "debug_assert_ne"];
4570

4671
for mac in macros.iter().chain(inverted_macros.iter()) {
47-
if let Some(span) = is_direct_expn_of(e.span, mac) {
48-
if let Some([a, b]) = ast_utils::extract_assert_macro_args(e) {
49-
let nb_bool_args = is_bool_lit(a) as usize + is_bool_lit(b) as usize;
72+
if let Some(span) = is_direct_expn_of(expr.span, mac) {
73+
if let Some(args) = higher::extract_assert_macro_args(expr) {
74+
if let [a, b, ..] = args[..] {
75+
let nb_bool_args = is_bool_lit(a) as usize + is_bool_lit(b) as usize;
76+
77+
if nb_bool_args != 1 {
78+
// If there are two boolean arguments, we definitely don't understand
79+
// what's going on, so better leave things as is...
80+
//
81+
// Or there is simply no boolean and then we can leave things as is!
82+
return;
83+
}
5084

51-
if nb_bool_args != 1 {
52-
// If there are two boolean arguments, we definitely don't understand
53-
// what's going on, so better leave things as is...
54-
//
55-
// Or there is simply no boolean and then we can leave things as is!
85+
if !is_impl_not_trait_with_bool_out(cx, a) || !is_impl_not_trait_with_bool_out(cx, b) {
86+
// At this point the expression which is not a boolean
87+
// literal does not implement Not trait with a bool output,
88+
// so we cannot suggest to rewrite our code
89+
return;
90+
}
91+
92+
let non_eq_mac = &mac[..mac.len() - 3];
93+
span_lint_and_sugg(
94+
cx,
95+
BOOL_ASSERT_COMPARISON,
96+
span,
97+
&format!("used `{}!` with a literal bool", mac),
98+
"replace it with",
99+
format!("{}!(..)", non_eq_mac),
100+
Applicability::MaybeIncorrect,
101+
);
56102
return;
57103
}
58-
59-
let non_eq_mac = &mac[..mac.len() - 3];
60-
span_lint_and_sugg(
61-
cx,
62-
BOOL_ASSERT_COMPARISON,
63-
span,
64-
&format!("used `{}!` with a literal bool", mac),
65-
"replace it with",
66-
format!("{}!(..)", non_eq_mac),
67-
Applicability::MaybeIncorrect,
68-
);
69-
return;
70104
}
71105
}
72106
}

‎clippy_lints/src/lib.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2115,7 +2115,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
21152115
store.register_late_pass(|| box from_str_radix_10::FromStrRadix10);
21162116
store.register_late_pass(|| box manual_map::ManualMap);
21172117
store.register_late_pass(move || box if_then_some_else_none::IfThenSomeElseNone::new(msrv));
2118-
store.register_early_pass(|| box bool_assert_comparison::BoolAssertComparison);
2118+
store.register_late_pass(|| box bool_assert_comparison::BoolAssertComparison);
21192119
store.register_early_pass(move || box module_style::ModStyle);
21202120
store.register_late_pass(|| box unused_async::UnusedAsync);
21212121
let disallowed_types = conf.disallowed_types.iter().cloned().collect::<FxHashSet<_>>();

‎tests/ui/bool_assert_comparison.rs‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
#![warn(clippy::bool_assert_comparison)]
22

3+
use std::ops::Not;
4+
35
macro_rules! a {
46
() => {
57
true
@@ -11,14 +13,67 @@ macro_rules! b {
1113
};
1214
}
1315

16+
// Implements the Not trait but with an output type
17+
// that's not bool. Should not suggest a rewrite
18+
#[derive(Debug)]
19+
enum ImplNotTraitWithoutBool {
20+
VariantX(bool),
21+
VariantY(u32),
22+
}
23+
24+
impl PartialEq<bool> for ImplNotTraitWithoutBool {
25+
fn eq(&self, other: &bool) -> bool {
26+
match *self {
27+
ImplNotTraitWithoutBool::VariantX(b) => b == *other,
28+
_ => false,
29+
}
30+
}
31+
}
32+
33+
impl Not for ImplNotTraitWithoutBool {
34+
type Output = Self;
35+
36+
fn not(self) -> Self::Output {
37+
match self {
38+
ImplNotTraitWithoutBool::VariantX(b) => ImplNotTraitWithoutBool::VariantX(!b),
39+
ImplNotTraitWithoutBool::VariantY(0) => ImplNotTraitWithoutBool::VariantY(1),
40+
ImplNotTraitWithoutBool::VariantY(_) => ImplNotTraitWithoutBool::VariantY(0),
41+
}
42+
}
43+
}
44+
45+
// This type implements the Not trait with an Output of
46+
// type bool. Using assert!(..) must be suggested
47+
#[derive(Debug)]
48+
struct ImplNotTraitWithBool;
49+
50+
impl PartialEq<bool> for ImplNotTraitWithBool {
51+
fn eq(&self, other: &bool) -> bool {
52+
false
53+
}
54+
}
55+
56+
impl Not for ImplNotTraitWithBool {
57+
type Output = bool;
58+
59+
fn not(self) -> Self::Output {
60+
true
61+
}
62+
}
63+
1464
fn main() {
65+
let a = ImplNotTraitWithoutBool::VariantX(true);
66+
let b = ImplNotTraitWithBool;
67+
1568
assert_eq!("a".len(), 1);
1669
assert_eq!("a".is_empty(), false);
1770
assert_eq!("".is_empty(), true);
1871
assert_eq!(true, "".is_empty());
1972
assert_eq!(a!(), b!());
2073
assert_eq!(a!(), "".is_empty());
2174
assert_eq!("".is_empty(), b!());
75+
assert_eq!(a, true);
76+
assert_eq!(b, true);
2277

2378
assert_ne!("a".len(), 1);
2479
assert_ne!("a".is_empty(), false);
@@ -27,6 +82,8 @@ fn main() {
2782
assert_ne!(a!(), b!());
2883
assert_ne!(a!(), "".is_empty());
2984
assert_ne!("".is_empty(), b!());
85+
assert_ne!(a, true);
86+
assert_ne!(b, true);
3087

3188
debug_assert_eq!("a".len(), 1);
3289
debug_assert_eq!("a".is_empty(), false);
@@ -35,6 +92,8 @@ fn main() {
3592
debug_assert_eq!(a!(), b!());
3693
debug_assert_eq!(a!(), "".is_empty());
3794
debug_assert_eq!("".is_empty(), b!());
95+
debug_assert_eq!(a, true);
96+
debug_assert_eq!(b, true);
3897

3998
debug_assert_ne!("a".len(), 1);
4099
debug_assert_ne!("a".is_empty(), false);
@@ -43,17 +102,21 @@ fn main() {
43102
debug_assert_ne!(a!(), b!());
44103
debug_assert_ne!(a!(), "".is_empty());
45104
debug_assert_ne!("".is_empty(), b!());
105+
debug_assert_ne!(a, true);
106+
debug_assert_ne!(b, true);
46107

47108
// assert with error messages
48109
assert_eq!("a".len(), 1, "tadam {}", 1);
49110
assert_eq!("a".len(), 1, "tadam {}", true);
50111
assert_eq!("a".is_empty(), false, "tadam {}", 1);
51112
assert_eq!("a".is_empty(), false, "tadam {}", true);
52113
assert_eq!(false, "a".is_empty(), "tadam {}", true);
114+
assert_eq!(a, true, "tadam {}", false);
53115

54116
debug_assert_eq!("a".len(), 1, "tadam {}", 1);
55117
debug_assert_eq!("a".len(), 1, "tadam {}", true);
56118
debug_assert_eq!("a".is_empty(), false, "tadam {}", 1);
57119
debug_assert_eq!("a".is_empty(), false, "tadam {}", true);
58120
debug_assert_eq!(false, "a".is_empty(), "tadam {}", true);
121+
debug_assert_eq!(a, true, "tadam {}", false);
59122
}

‎tests/ui/bool_assert_comparison.stderr‎

Lines changed: 43 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,112 +1,136 @@
11
error: used `assert_eq!` with a literal bool
2-
--> $DIR/bool_assert_comparison.rs:16:5
2+
--> $DIR/bool_assert_comparison.rs:69:5
33
|
44
LL | assert_eq!("a".is_empty(), false);
55
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
66
|
77
= note: `-D clippy::bool-assert-comparison` implied by `-D warnings`
88

99
error: used `assert_eq!` with a literal bool
10-
--> $DIR/bool_assert_comparison.rs:17:5
10+
--> $DIR/bool_assert_comparison.rs:70:5
1111
|
1212
LL | assert_eq!("".is_empty(), true);
1313
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
1414

1515
error: used `assert_eq!` with a literal bool
16-
--> $DIR/bool_assert_comparison.rs:18:5
16+
--> $DIR/bool_assert_comparison.rs:71:5
1717
|
1818
LL | assert_eq!(true, "".is_empty());
1919
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
2020

21+
error: used `assert_eq!` with a literal bool
22+
--> $DIR/bool_assert_comparison.rs:76:5
23+
|
24+
LL | assert_eq!(b, true);
25+
| ^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
26+
2127
error: used `assert_ne!` with a literal bool
22-
--> $DIR/bool_assert_comparison.rs:24:5
28+
--> $DIR/bool_assert_comparison.rs:79:5
2329
|
2430
LL | assert_ne!("a".is_empty(), false);
2531
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
2632

2733
error: used `assert_ne!` with a literal bool
28-
--> $DIR/bool_assert_comparison.rs:25:5
34+
--> $DIR/bool_assert_comparison.rs:80:5
2935
|
3036
LL | assert_ne!("".is_empty(), true);
3137
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
3238

3339
error: used `assert_ne!` with a literal bool
34-
--> $DIR/bool_assert_comparison.rs:26:5
40+
--> $DIR/bool_assert_comparison.rs:81:5
3541
|
3642
LL | assert_ne!(true, "".is_empty());
3743
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
3844

45+
error: used `assert_ne!` with a literal bool
46+
--> $DIR/bool_assert_comparison.rs:86:5
47+
|
48+
LL | assert_ne!(b, true);
49+
| ^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
50+
3951
error: used `debug_assert_eq!` with a literal bool
40-
--> $DIR/bool_assert_comparison.rs:32:5
52+
--> $DIR/bool_assert_comparison.rs:89:5
4153
|
4254
LL | debug_assert_eq!("a".is_empty(), false);
4355
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
4456

4557
error: used `debug_assert_eq!` with a literal bool
46-
--> $DIR/bool_assert_comparison.rs:33:5
58+
--> $DIR/bool_assert_comparison.rs:90:5
4759
|
4860
LL | debug_assert_eq!("".is_empty(), true);
4961
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
5062

5163
error: used `debug_assert_eq!` with a literal bool
52-
--> $DIR/bool_assert_comparison.rs:34:5
64+
--> $DIR/bool_assert_comparison.rs:91:5
5365
|
5466
LL | debug_assert_eq!(true, "".is_empty());
5567
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
5668

69+
error: used `debug_assert_eq!` with a literal bool
70+
--> $DIR/bool_assert_comparison.rs:96:5
71+
|
72+
LL | debug_assert_eq!(b, true);
73+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
74+
5775
error: used `debug_assert_ne!` with a literal bool
58-
--> $DIR/bool_assert_comparison.rs:40:5
76+
--> $DIR/bool_assert_comparison.rs:99:5
5977
|
6078
LL | debug_assert_ne!("a".is_empty(), false);
6179
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
6280

6381
error: used `debug_assert_ne!` with a literal bool
64-
--> $DIR/bool_assert_comparison.rs:41:5
82+
--> $DIR/bool_assert_comparison.rs:100:5
6583
|
6684
LL | debug_assert_ne!("".is_empty(), true);
6785
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
6886

6987
error: used `debug_assert_ne!` with a literal bool
70-
--> $DIR/bool_assert_comparison.rs:42:5
88+
--> $DIR/bool_assert_comparison.rs:101:5
7189
|
7290
LL | debug_assert_ne!(true, "".is_empty());
7391
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
7492

93+
error: used `debug_assert_ne!` with a literal bool
94+
--> $DIR/bool_assert_comparison.rs:106:5
95+
|
96+
LL | debug_assert_ne!(b, true);
97+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
98+
7599
error: used `assert_eq!` with a literal bool
76-
--> $DIR/bool_assert_comparison.rs:50:5
100+
--> $DIR/bool_assert_comparison.rs:111:5
77101
|
78102
LL | assert_eq!("a".is_empty(), false, "tadam {}", 1);
79103
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
80104

81105
error: used `assert_eq!` with a literal bool
82-
--> $DIR/bool_assert_comparison.rs:51:5
106+
--> $DIR/bool_assert_comparison.rs:112:5
83107
|
84108
LL | assert_eq!("a".is_empty(), false, "tadam {}", true);
85109
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
86110

87111
error: used `assert_eq!` with a literal bool
88-
--> $DIR/bool_assert_comparison.rs:52:5
112+
--> $DIR/bool_assert_comparison.rs:113:5
89113
|
90114
LL | assert_eq!(false, "a".is_empty(), "tadam {}", true);
91115
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)`
92116

93117
error: used `debug_assert_eq!` with a literal bool
94-
--> $DIR/bool_assert_comparison.rs:56:5
118+
--> $DIR/bool_assert_comparison.rs:118:5
95119
|
96120
LL | debug_assert_eq!("a".is_empty(), false, "tadam {}", 1);
97121
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
98122

99123
error: used `debug_assert_eq!` with a literal bool
100-
--> $DIR/bool_assert_comparison.rs:57:5
124+
--> $DIR/bool_assert_comparison.rs:119:5
101125
|
102126
LL | debug_assert_eq!("a".is_empty(), false, "tadam {}", true);
103127
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
104128

105129
error: used `debug_assert_eq!` with a literal bool
106-
--> $DIR/bool_assert_comparison.rs:58:5
130+
--> $DIR/bool_assert_comparison.rs:120:5
107131
|
108132
LL | debug_assert_eq!(false, "a".is_empty(), "tadam {}", true);
109133
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)`
110134

111-
error: aborting due to 18 previous errors
135+
error: aborting due to 22 previous errors
112136

0 commit comments

Comments
 (0)