Skip to content

bool_comparison: no longer lint on !x != y #15498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 22, 2025
Merged

bool_comparison: no longer lint on !x != y #15498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 40 additions & 101 deletions clippy_lints/src/needless_bool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use clippy_utils::{
};
use rustc_ast::ast::LitKind;
use rustc_errors::Applicability;
use rustc_hir::{BinOpKind, Expr, ExprKind, UnOp};
use rustc_hir::{BinOpKind, Expr, ExprKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::declare_lint_pass;
use rustc_span::Span;
Expand Down Expand Up @@ -232,7 +232,12 @@ impl<'tcx> LateLintPass<'tcx> for BoolComparison {
return;
}

if let ExprKind::Binary(Spanned { node, .. }, ..) = e.kind {
if let ExprKind::Binary(Spanned { node, .. }, left_side, right_side) = e.kind
&& is_expn_of(left_side.span, sym::cfg).is_none()
&& is_expn_of(right_side.span, sym::cfg).is_none()
&& cx.typeck_results().expr_ty(left_side).is_bool()
&& cx.typeck_results().expr_ty(right_side).is_bool()
{
let ignore_case = None::<(fn(_) -> _, &str)>;
let ignore_no_literal = None::<(fn(_, _) -> _, &str)>;
match node {
Expand Down Expand Up @@ -288,30 +293,6 @@ impl<'tcx> LateLintPass<'tcx> for BoolComparison {
}
}

struct ExpressionInfoWithSpan {
one_side_is_unary_not: bool,
left_span: Span,
right_span: Span,
}

fn is_unary_not(e: &Expr<'_>) -> (bool, Span) {
if let ExprKind::Unary(UnOp::Not, operand) = e.kind {
return (true, operand.span);
}
(false, e.span)
}

fn one_side_is_unary_not<'tcx>(left_side: &'tcx Expr<'_>, right_side: &'tcx Expr<'_>) -> ExpressionInfoWithSpan {
let left = is_unary_not(left_side);
let right = is_unary_not(right_side);

ExpressionInfoWithSpan {
one_side_is_unary_not: left.0 != right.0,
left_span: left.1,
right_span: right.1,
}
}

fn check_comparison<'a, 'tcx>(
cx: &LateContext<'tcx>,
e: &'tcx Expr<'_>,
Expand All @@ -321,81 +302,39 @@ fn check_comparison<'a, 'tcx>(
right_false: Option<(impl FnOnce(Sugg<'a>) -> Sugg<'a>, &'static str)>,
no_literal: Option<(impl FnOnce(Sugg<'a>, Sugg<'a>) -> Sugg<'a>, &'static str)>,
) {
if let ExprKind::Binary(op, left_side, right_side) = e.kind {
let (l_ty, r_ty) = (
cx.typeck_results().expr_ty(left_side),
cx.typeck_results().expr_ty(right_side),
);
if is_expn_of(left_side.span, sym::cfg).is_some() || is_expn_of(right_side.span, sym::cfg).is_some() {
return;
}
if l_ty.is_bool() && r_ty.is_bool() {
let mut applicability = Applicability::MachineApplicable;
// Eliminate parentheses in `e` by using the lo pos of lhs and hi pos of rhs,
// calling `source_callsite` make sure macros are handled correctly, see issue #9907
let binop_span = left_side
.span
.source_callsite()
.with_hi(right_side.span.source_callsite().hi());
if let ExprKind::Binary(_, left_side, right_side) = e.kind {
let mut applicability = Applicability::MachineApplicable;
// Eliminate parentheses in `e` by using the lo pos of lhs and hi pos of rhs,
// calling `source_callsite` make sure macros are handled correctly, see issue #9907
let binop_span = left_side.span.source_callsite().to(right_side.span.source_callsite());

if op.node == BinOpKind::Eq {
let expression_info = one_side_is_unary_not(left_side, right_side);
if expression_info.one_side_is_unary_not {
span_lint_and_sugg(
cx,
BOOL_COMPARISON,
binop_span,
"this comparison might be written more concisely",
"try simplifying it as shown",
format!(
"{} != {}",
snippet_with_applicability(
cx,
expression_info.left_span.source_callsite(),
"..",
&mut applicability
),
snippet_with_applicability(
cx,
expression_info.right_span.source_callsite(),
"..",
&mut applicability
)
),
applicability,
);
}
}

match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) {
(Some(true), None) => left_true.map_or((), |(h, m)| {
suggest_bool_comparison(cx, binop_span, right_side, applicability, m, h);
}),
(None, Some(true)) => right_true.map_or((), |(h, m)| {
suggest_bool_comparison(cx, binop_span, left_side, applicability, m, h);
}),
(Some(false), None) => left_false.map_or((), |(h, m)| {
suggest_bool_comparison(cx, binop_span, right_side, applicability, m, h);
}),
(None, Some(false)) => right_false.map_or((), |(h, m)| {
suggest_bool_comparison(cx, binop_span, left_side, applicability, m, h);
}),
(None, None) => no_literal.map_or((), |(h, m)| {
let left_side = Sugg::hir_with_context(cx, left_side, binop_span.ctxt(), "..", &mut applicability);
let right_side =
Sugg::hir_with_context(cx, right_side, binop_span.ctxt(), "..", &mut applicability);
span_lint_and_sugg(
cx,
BOOL_COMPARISON,
binop_span,
m,
"try simplifying it as shown",
h(left_side, right_side).into_string(),
applicability,
);
}),
_ => (),
}
match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) {
(Some(true), None) => left_true.map_or((), |(h, m)| {
suggest_bool_comparison(cx, binop_span, right_side, applicability, m, h);
}),
(None, Some(true)) => right_true.map_or((), |(h, m)| {
suggest_bool_comparison(cx, binop_span, left_side, applicability, m, h);
}),
(Some(false), None) => left_false.map_or((), |(h, m)| {
suggest_bool_comparison(cx, binop_span, right_side, applicability, m, h);
}),
(None, Some(false)) => right_false.map_or((), |(h, m)| {
suggest_bool_comparison(cx, binop_span, left_side, applicability, m, h);
}),
(None, None) => no_literal.map_or((), |(h, m)| {
let left_side = Sugg::hir_with_context(cx, left_side, binop_span.ctxt(), "..", &mut applicability);
let right_side = Sugg::hir_with_context(cx, right_side, binop_span.ctxt(), "..", &mut applicability);
span_lint_and_sugg(
cx,
BOOL_COMPARISON,
binop_span,
m,
"try",
h(left_side, right_side).into_string(),
applicability,
);
}),
_ => (),
}
}
}
Expand All @@ -414,7 +353,7 @@ fn suggest_bool_comparison<'a, 'tcx>(
BOOL_COMPARISON,
span,
message,
"try simplifying it as shown",
"try",
conv_hint(hint).into_string(),
app,
);
Expand Down
148 changes: 43 additions & 105 deletions tests/ui/bool_comparison.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,94 +1,39 @@
#![allow(non_local_definitions, clippy::needless_if)]
#![warn(clippy::bool_comparison)]
#![allow(clippy::non_canonical_partial_ord_impl, clippy::nonminimal_bool)]
#![allow(clippy::non_canonical_partial_ord_impl)]

fn main() {
let x = true;
if x {
//~^ bool_comparison
"yes"
} else {
"no"
};
if !x {
//~^ bool_comparison
"yes"
} else {
"no"
};
if x {
//~^ bool_comparison
"yes"
} else {
"no"
};
if !x {
//~^ bool_comparison
"yes"
} else {
"no"
};
if !x {
//~^ bool_comparison
"yes"
} else {
"no"
};
if x {
//~^ bool_comparison
"yes"
} else {
"no"
};
if !x {
//~^ bool_comparison
"yes"
} else {
"no"
};
if x {
//~^ bool_comparison
"yes"
} else {
"no"
};
if !x {
//~^ bool_comparison
"yes"
} else {
"no"
};
if x {
//~^ bool_comparison
"yes"
} else {
"no"
};
if x {
//~^ bool_comparison
"yes"
} else {
"no"
};
if !x {
//~^ bool_comparison
"yes"
} else {
"no"
};
let _ = if x { "yes" } else { "no" };
//~^ bool_comparison
let _ = if !x { "yes" } else { "no" };
//~^ bool_comparison
let _ = if x { "yes" } else { "no" };
//~^ bool_comparison
let _ = if !x { "yes" } else { "no" };
//~^ bool_comparison
let _ = if !x { "yes" } else { "no" };
//~^ bool_comparison
let _ = if x { "yes" } else { "no" };
//~^ bool_comparison
let _ = if !x { "yes" } else { "no" };
//~^ bool_comparison
let _ = if x { "yes" } else { "no" };
//~^ bool_comparison
let _ = if !x { "yes" } else { "no" };
//~^ bool_comparison
let _ = if x { "yes" } else { "no" };
//~^ bool_comparison
let _ = if x { "yes" } else { "no" };
//~^ bool_comparison
let _ = if !x { "yes" } else { "no" };
//~^ bool_comparison

let y = true;
if !x & y {
//~^ bool_comparison
"yes"
} else {
"no"
};
if x & !y {
//~^ bool_comparison
"yes"
} else {
"no"
};
let _ = if !x & y { "yes" } else { "no" };
//~^ bool_comparison
let _ = if x & !y { "yes" } else { "no" };
//~^ bool_comparison
}

fn issue3703() {
Expand Down Expand Up @@ -126,25 +71,6 @@ fn issue3703() {
if false < Foo {}
}

fn issue4983() {
let a = true;
let b = false;

if a != b {};
//~^ bool_comparison
if a != b {};
//~^ bool_comparison
if a == b {};
if !a == !b {};

if b != a {};
//~^ bool_comparison
if b != a {};
//~^ bool_comparison
if b == a {};
if !b == !a {};
}

macro_rules! m {
($func:ident) => {
$func()
Expand Down Expand Up @@ -193,10 +119,22 @@ fn issue9907() {
//~^ bool_comparison
// This is not part of the issue, but an unexpected found when fixing the issue,
// the provided span was inside of macro rather than the macro callsite.
let _ = ((1 < 2) != m!(func)) as usize;
let _ = ((1 < 2) & !m!(func)) as usize;
//~^ bool_comparison
}

#[allow(clippy::nonminimal_bool)]
fn issue15367() {
let a = true;
let b = false;

// these cases are handled by `nonminimal_bool`, so don't double-lint
if a == !b {};
if !a == b {};
if b == !a {};
if !b == a {};
}

fn issue15497() {
fn func() -> bool {
true
Expand Down
Loading