Skip to content

Commit c2c3e09

Browse files
ada4aada4a
committed
fix: don't check for !x = y/x = !y
misc: rm "as shown" from help message - clippy guidelines recommend against this misc: pull conditions into let-chain misc: use `Span::to` misc: inline `{l,r}_ty` misc: move the type checks out of `check_comparison` misc: make test cases much less verbose
1 parent df0499a commit c2c3e09

File tree

7 files changed

+212
-435
lines changed

7 files changed

+212
-435
lines changed

‎clippy_lints/src/needless_bool.rs

Lines changed: 40 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use clippy_utils::{
77
};
88
use rustc_ast::ast::LitKind;
99
use rustc_errors::Applicability;
10-
use rustc_hir::{BinOpKind, Expr, ExprKind, UnOp};
10+
use rustc_hir::{BinOpKind, Expr, ExprKind};
1111
use rustc_lint::{LateContext, LateLintPass};
1212
use rustc_session::declare_lint_pass;
1313
use rustc_span::Span;
@@ -232,7 +232,12 @@ impl<'tcx> LateLintPass<'tcx> for BoolComparison {
232232
return;
233233
}
234234

235-
if let ExprKind::Binary(Spanned { node, .. }, ..) = e.kind {
235+
if let ExprKind::Binary(Spanned { node, .. }, left_side, right_side) = e.kind
236+
&& is_expn_of(left_side.span, sym::cfg).is_none()
237+
&& is_expn_of(right_side.span, sym::cfg).is_none()
238+
&& cx.typeck_results().expr_ty(left_side).is_bool()
239+
&& cx.typeck_results().expr_ty(right_side).is_bool()
240+
{
236241
let ignore_case = None::<(fn(_) -> _, &str)>;
237242
let ignore_no_literal = None::<(fn(_, _) -> _, &str)>;
238243
match node {
@@ -288,30 +293,6 @@ impl<'tcx> LateLintPass<'tcx> for BoolComparison {
288293
}
289294
}
290295

291-
struct ExpressionInfoWithSpan {
292-
one_side_is_unary_not: bool,
293-
left_span: Span,
294-
right_span: Span,
295-
}
296-
297-
fn is_unary_not(e: &Expr<'_>) -> (bool, Span) {
298-
if let ExprKind::Unary(UnOp::Not, operand) = e.kind {
299-
return (true, operand.span);
300-
}
301-
(false, e.span)
302-
}
303-
304-
fn one_side_is_unary_not<'tcx>(left_side: &'tcx Expr<'_>, right_side: &'tcx Expr<'_>) -> ExpressionInfoWithSpan {
305-
let left = is_unary_not(left_side);
306-
let right = is_unary_not(right_side);
307-
308-
ExpressionInfoWithSpan {
309-
one_side_is_unary_not: left.0 != right.0,
310-
left_span: left.1,
311-
right_span: right.1,
312-
}
313-
}
314-
315296
fn check_comparison<'a, 'tcx>(
316297
cx: &LateContext<'tcx>,
317298
e: &'tcx Expr<'_>,
@@ -321,81 +302,39 @@ fn check_comparison<'a, 'tcx>(
321302
right_false: Option<(impl FnOnce(Sugg<'a>) -> Sugg<'a>, &'static str)>,
322303
no_literal: Option<(impl FnOnce(Sugg<'a>, Sugg<'a>) -> Sugg<'a>, &'static str)>,
323304
) {
324-
if let ExprKind::Binary(op, left_side, right_side) = e.kind {
325-
let (l_ty, r_ty) = (
326-
cx.typeck_results().expr_ty(left_side),
327-
cx.typeck_results().expr_ty(right_side),
328-
);
329-
if is_expn_of(left_side.span, sym::cfg).is_some() || is_expn_of(right_side.span, sym::cfg).is_some() {
330-
return;
331-
}
332-
if l_ty.is_bool() && r_ty.is_bool() {
333-
let mut applicability = Applicability::MachineApplicable;
334-
// Eliminate parentheses in `e` by using the lo pos of lhs and hi pos of rhs,
335-
// calling `source_callsite` make sure macros are handled correctly, see issue #9907
336-
let binop_span = left_side
337-
.span
338-
.source_callsite()
339-
.with_hi(right_side.span.source_callsite().hi());
305+
if let ExprKind::Binary(_, left_side, right_side) = e.kind {
306+
let mut applicability = Applicability::MachineApplicable;
307+
// Eliminate parentheses in `e` by using the lo pos of lhs and hi pos of rhs,
308+
// calling `source_callsite` make sure macros are handled correctly, see issue #9907
309+
let binop_span = left_side.span.source_callsite().to(right_side.span.source_callsite());
340310

341-
if op.node == BinOpKind::Eq {
342-
let expression_info = one_side_is_unary_not(left_side, right_side);
343-
if expression_info.one_side_is_unary_not {
344-
span_lint_and_sugg(
345-
cx,
346-
BOOL_COMPARISON,
347-
binop_span,
348-
"this comparison might be written more concisely",
349-
"try simplifying it as shown",
350-
format!(
351-
"{} != {}",
352-
snippet_with_applicability(
353-
cx,
354-
expression_info.left_span.source_callsite(),
355-
"..",
356-
&mut applicability
357-
),
358-
snippet_with_applicability(
359-
cx,
360-
expression_info.right_span.source_callsite(),
361-
"..",
362-
&mut applicability
363-
)
364-
),
365-
applicability,
366-
);
367-
}
368-
}
369-
370-
match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) {
371-
(Some(true), None) => left_true.map_or((), |(h, m)| {
372-
suggest_bool_comparison(cx, binop_span, right_side, applicability, m, h);
373-
}),
374-
(None, Some(true)) => right_true.map_or((), |(h, m)| {
375-
suggest_bool_comparison(cx, binop_span, left_side, applicability, m, h);
376-
}),
377-
(Some(false), None) => left_false.map_or((), |(h, m)| {
378-
suggest_bool_comparison(cx, binop_span, right_side, applicability, m, h);
379-
}),
380-
(None, Some(false)) => right_false.map_or((), |(h, m)| {
381-
suggest_bool_comparison(cx, binop_span, left_side, applicability, m, h);
382-
}),
383-
(None, None) => no_literal.map_or((), |(h, m)| {
384-
let left_side = Sugg::hir_with_context(cx, left_side, binop_span.ctxt(), "..", &mut applicability);
385-
let right_side =
386-
Sugg::hir_with_context(cx, right_side, binop_span.ctxt(), "..", &mut applicability);
387-
span_lint_and_sugg(
388-
cx,
389-
BOOL_COMPARISON,
390-
binop_span,
391-
m,
392-
"try simplifying it as shown",
393-
h(left_side, right_side).into_string(),
394-
applicability,
395-
);
396-
}),
397-
_ => (),
398-
}
311+
match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) {
312+
(Some(true), None) => left_true.map_or((), |(h, m)| {
313+
suggest_bool_comparison(cx, binop_span, right_side, applicability, m, h);
314+
}),
315+
(None, Some(true)) => right_true.map_or((), |(h, m)| {
316+
suggest_bool_comparison(cx, binop_span, left_side, applicability, m, h);
317+
}),
318+
(Some(false), None) => left_false.map_or((), |(h, m)| {
319+
suggest_bool_comparison(cx, binop_span, right_side, applicability, m, h);
320+
}),
321+
(None, Some(false)) => right_false.map_or((), |(h, m)| {
322+
suggest_bool_comparison(cx, binop_span, left_side, applicability, m, h);
323+
}),
324+
(None, None) => no_literal.map_or((), |(h, m)| {
325+
let left_side = Sugg::hir_with_context(cx, left_side, binop_span.ctxt(), "..", &mut applicability);
326+
let right_side = Sugg::hir_with_context(cx, right_side, binop_span.ctxt(), "..", &mut applicability);
327+
span_lint_and_sugg(
328+
cx,
329+
BOOL_COMPARISON,
330+
binop_span,
331+
m,
332+
"try",
333+
h(left_side, right_side).into_string(),
334+
applicability,
335+
);
336+
}),
337+
_ => (),
399338
}
400339
}
401340
}
@@ -414,7 +353,7 @@ fn suggest_bool_comparison<'a, 'tcx>(
414353
BOOL_COMPARISON,
415354
span,
416355
message,
417-
"try simplifying it as shown",
356+
"try",
418357
conv_hint(hint).into_string(),
419358
app,
420359
);

‎tests/ui/bool_comparison.fixed

Lines changed: 43 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -1,94 +1,39 @@
11
#![allow(non_local_definitions, clippy::needless_if)]
22
#![warn(clippy::bool_comparison)]
3-
#![allow(clippy::non_canonical_partial_ord_impl, clippy::nonminimal_bool)]
3+
#![allow(clippy::non_canonical_partial_ord_impl)]
44

55
fn main() {
66
let x = true;
7-
if x {
8-
//~^ bool_comparison
9-
"yes"
10-
} else {
11-
"no"
12-
};
13-
if !x {
14-
//~^ bool_comparison
15-
"yes"
16-
} else {
17-
"no"
18-
};
19-
if x {
20-
//~^ bool_comparison
21-
"yes"
22-
} else {
23-
"no"
24-
};
25-
if !x {
26-
//~^ bool_comparison
27-
"yes"
28-
} else {
29-
"no"
30-
};
31-
if !x {
32-
//~^ bool_comparison
33-
"yes"
34-
} else {
35-
"no"
36-
};
37-
if x {
38-
//~^ bool_comparison
39-
"yes"
40-
} else {
41-
"no"
42-
};
43-
if !x {
44-
//~^ bool_comparison
45-
"yes"
46-
} else {
47-
"no"
48-
};
49-
if x {
50-
//~^ bool_comparison
51-
"yes"
52-
} else {
53-
"no"
54-
};
55-
if !x {
56-
//~^ bool_comparison
57-
"yes"
58-
} else {
59-
"no"
60-
};
61-
if x {
62-
//~^ bool_comparison
63-
"yes"
64-
} else {
65-
"no"
66-
};
67-
if x {
68-
//~^ bool_comparison
69-
"yes"
70-
} else {
71-
"no"
72-
};
73-
if !x {
74-
//~^ bool_comparison
75-
"yes"
76-
} else {
77-
"no"
78-
};
7+
let _ = if x { "yes" } else { "no" };
8+
//~^ bool_comparison
9+
let _ = if !x { "yes" } else { "no" };
10+
//~^ bool_comparison
11+
let _ = if x { "yes" } else { "no" };
12+
//~^ bool_comparison
13+
let _ = if !x { "yes" } else { "no" };
14+
//~^ bool_comparison
15+
let _ = if !x { "yes" } else { "no" };
16+
//~^ bool_comparison
17+
let _ = if x { "yes" } else { "no" };
18+
//~^ bool_comparison
19+
let _ = if !x { "yes" } else { "no" };
20+
//~^ bool_comparison
21+
let _ = if x { "yes" } else { "no" };
22+
//~^ bool_comparison
23+
let _ = if !x { "yes" } else { "no" };
24+
//~^ bool_comparison
25+
let _ = if x { "yes" } else { "no" };
26+
//~^ bool_comparison
27+
let _ = if x { "yes" } else { "no" };
28+
//~^ bool_comparison
29+
let _ = if !x { "yes" } else { "no" };
30+
//~^ bool_comparison
31+
7932
let y = true;
80-
if !x & y {
81-
//~^ bool_comparison
82-
"yes"
83-
} else {
84-
"no"
85-
};
86-
if x & !y {
87-
//~^ bool_comparison
88-
"yes"
89-
} else {
90-
"no"
91-
};
33+
let _ = if !x & y { "yes" } else { "no" };
34+
//~^ bool_comparison
35+
let _ = if x & !y { "yes" } else { "no" };
36+
//~^ bool_comparison
9237
}
9338

9439
fn issue3703() {
@@ -126,25 +71,6 @@ fn issue3703() {
12671
if false < Foo {}
12772
}
12873

129-
fn issue4983() {
130-
let a = true;
131-
let b = false;
132-
133-
if a != b {};
134-
//~^ bool_comparison
135-
if a != b {};
136-
//~^ bool_comparison
137-
if a == b {};
138-
if !a == !b {};
139-
140-
if b != a {};
141-
//~^ bool_comparison
142-
if b != a {};
143-
//~^ bool_comparison
144-
if b == a {};
145-
if !b == !a {};
146-
}
147-
14874
macro_rules! m {
14975
($func:ident) => {
15076
$func()
@@ -193,10 +119,22 @@ fn issue9907() {
193119
//~^ bool_comparison
194120
// This is not part of the issue, but an unexpected found when fixing the issue,
195121
// the provided span was inside of macro rather than the macro callsite.
196-
let _ = ((1 < 2) != m!(func)) as usize;
122+
let _ = ((1 < 2) & !m!(func)) as usize;
197123
//~^ bool_comparison
198124
}
199125

126+
#[allow(clippy::nonminimal_bool)]
127+
fn issue15367() {
128+
let a = true;
129+
let b = false;
130+
131+
// these cases are handled by `nonminimal_bool`, so don't double-lint
132+
if a == !b {};
133+
if !a == b {};
134+
if b == !a {};
135+
if !b == a {};
136+
}
137+
200138
fn issue15497() {
201139
fn func() -> bool {
202140
true

0 commit comments

Comments
 (0)