diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs index 3709f3948c2..e7809436252 100644 --- a/clippy_lints/src/needless_bool.rs +++ b/clippy_lints/src/needless_bool.rs @@ -6,10 +6,10 @@ use clippy_utils::higher; use clippy_utils::source::snippet_with_applicability; use clippy_utils::sugg::Sugg; -use clippy_utils::{is_else_clause, is_expn_of}; +use clippy_utils::{get_parent_node, is_else_clause, is_expn_of}; use rustc_ast::ast::LitKind; use rustc_errors::Applicability; -use rustc_hir::{BinOpKind, Block, Expr, ExprKind, StmtKind, UnOp}; +use rustc_hir::{BinOpKind, Block, Expr, ExprKind, HirId, Node, StmtKind, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Spanned; @@ -74,6 +74,36 @@ declare_lint_pass!(NeedlessBool => [NEEDLESS_BOOL]); +fn condition_needs_parentheses(e: &Expr<'_>) -> bool { + let mut inner = e; + while let ExprKind::Binary(_, i, _) + | ExprKind::Call(i, _) + | ExprKind::Cast(i, _) + | ExprKind::Type(i, _) + | ExprKind::Index(i, _) = inner.kind + { + if matches!( + i.kind, + ExprKind::Block(..) + | ExprKind::ConstBlock(..) + | ExprKind::If(..) + | ExprKind::Loop(..) + | ExprKind::Match(..) + ) { + return true; + } + inner = i; + } + false +} + +fn is_parent_stmt(cx: &LateContext<'_>, id: HirId) -> bool { + matches!( + get_parent_node(cx.tcx, id), + Some(Node::Stmt(..) | Node::Block(Block { stmts: &[], .. })) + ) +} + impl<'tcx> LateLintPass<'tcx> for NeedlessBool { fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { use self::Expression::{Bool, RetBool}; @@ -99,6 +129,10 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { snip = snip.blockify(); } + if condition_needs_parentheses(cond) && is_parent_stmt(cx, e.hir_id) { + snip = snip.maybe_par(); + } + span_lint_and_sugg( cx, NEEDLESS_BOOL, diff --git a/tests/ui/needless_bool/fixable.fixed b/tests/ui/needless_bool/fixable.fixed index a2e3988daff..85da1f4e104 100644 --- a/tests/ui/needless_bool/fixable.fixed +++ b/tests/ui/needless_bool/fixable.fixed @@ -53,6 +53,7 @@ fn main() { needless_bool(x); needless_bool2(x); needless_bool3(x); + needless_bool_condition(); } fn bool_ret3(x: bool) -> bool { @@ -98,3 +99,19 @@ fn needless_bool_in_the_suggestion_wraps_the_predicate_of_if_else_statement_in_b true } else { !returns_bool() }; } + +unsafe fn no(v: u8) -> u8 { + v +} + +#[allow(clippy::unnecessary_operation)] +fn needless_bool_condition() -> bool { + (unsafe { no(4) } & 1 != 0); + let _brackets_unneeded = unsafe { no(4) } & 1 != 0; + fn foo() -> bool { + // parentheses are needed here + (unsafe { no(4) } & 1 != 0) + } + + foo() +} diff --git a/tests/ui/needless_bool/fixable.rs b/tests/ui/needless_bool/fixable.rs index 75805e85789..add60630251 100644 --- a/tests/ui/needless_bool/fixable.rs +++ b/tests/ui/needless_bool/fixable.rs @@ -65,6 +65,7 @@ fn main() { needless_bool(x); needless_bool2(x); needless_bool3(x); + needless_bool_condition(); } fn bool_ret3(x: bool) -> bool { @@ -130,3 +131,23 @@ fn needless_bool_in_the_suggestion_wraps_the_predicate_of_if_else_statement_in_b true }; } + +unsafe fn no(v: u8) -> u8 { + v +} + +#[allow(clippy::unnecessary_operation)] +fn needless_bool_condition() -> bool { + if unsafe { no(4) } & 1 != 0 { + true + } else { + false + }; + let _brackets_unneeded = if unsafe { no(4) } & 1 != 0 { true } else { false }; + fn foo() -> bool { + // parentheses are needed here + if unsafe { no(4) } & 1 != 0 { true } else { false } + } + + foo() +} diff --git a/tests/ui/needless_bool/fixable.stderr b/tests/ui/needless_bool/fixable.stderr index 1fa12add167..22c0a7bb491 100644 --- a/tests/ui/needless_bool/fixable.stderr +++ b/tests/ui/needless_bool/fixable.stderr @@ -31,7 +31,7 @@ LL | | }; | |_____^ help: you can reduce it to: `!(x && y)` error: this if-then-else expression returns a bool literal - --> $DIR/fixable.rs:71:5 + --> $DIR/fixable.rs:72:5 | LL | / if x { LL | | return true; @@ -41,7 +41,7 @@ LL | | }; | |_____^ help: you can reduce it to: `return x` error: this if-then-else expression returns a bool literal - --> $DIR/fixable.rs:79:5 + --> $DIR/fixable.rs:80:5 | LL | / if x { LL | | return false; @@ -51,7 +51,7 @@ LL | | }; | |_____^ help: you can reduce it to: `return !x` error: this if-then-else expression returns a bool literal - --> $DIR/fixable.rs:87:5 + --> $DIR/fixable.rs:88:5 | LL | / if x && y { LL | | return true; @@ -61,7 +61,7 @@ LL | | }; | |_____^ help: you can reduce it to: `return x && y` error: this if-then-else expression returns a bool literal - --> $DIR/fixable.rs:95:5 + --> $DIR/fixable.rs:96:5 | LL | / if x && y { LL | | return false; @@ -71,7 +71,7 @@ LL | | }; | |_____^ help: you can reduce it to: `return !(x && y)` error: equality checks against true are unnecessary - --> $DIR/fixable.rs:103:8 + --> $DIR/fixable.rs:104:8 | LL | if x == true {}; | ^^^^^^^^^ help: try simplifying it as shown: `x` @@ -79,25 +79,25 @@ LL | if x == true {}; = note: `-D clippy::bool-comparison` implied by `-D warnings` error: equality checks against false can be replaced by a negation - --> $DIR/fixable.rs:107:8 + --> $DIR/fixable.rs:108:8 | LL | if x == false {}; | ^^^^^^^^^^ help: try simplifying it as shown: `!x` error: equality checks against true are unnecessary - --> $DIR/fixable.rs:117:8 + --> $DIR/fixable.rs:118:8 | LL | if x == true {}; | ^^^^^^^^^ help: try simplifying it as shown: `x` error: equality checks against false can be replaced by a negation - --> $DIR/fixable.rs:118:8 + --> $DIR/fixable.rs:119:8 | LL | if x == false {}; | ^^^^^^^^^^ help: try simplifying it as shown: `!x` error: this if-then-else expression returns a bool literal - --> $DIR/fixable.rs:127:12 + --> $DIR/fixable.rs:128:12 | LL | } else if returns_bool() { | ____________^ @@ -107,5 +107,27 @@ LL | | true LL | | }; | |_____^ help: you can reduce it to: `{ !returns_bool() }` -error: aborting due to 12 previous errors +error: this if-then-else expression returns a bool literal + --> $DIR/fixable.rs:141:5 + | +LL | / if unsafe { no(4) } & 1 != 0 { +LL | | true +LL | | } else { +LL | | false +LL | | }; + | |_____^ help: you can reduce it to: `(unsafe { no(4) } & 1 != 0)` + +error: this if-then-else expression returns a bool literal + --> $DIR/fixable.rs:146:30 + | +LL | let _brackets_unneeded = if unsafe { no(4) } & 1 != 0 { true } else { false }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `unsafe { no(4) } & 1 != 0` + +error: this if-then-else expression returns a bool literal + --> $DIR/fixable.rs:149:9 + | +LL | if unsafe { no(4) } & 1 != 0 { true } else { false } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `(unsafe { no(4) } & 1 != 0)` + +error: aborting due to 15 previous errors