Parenthesize blocks in needless_bool suggestion

llogiq · llogiq · commit 4a3970779586 · 2021-12-06T13:15:31.000+01:00
Because the `if .. {}` statement already puts the condition in expression scope, contained blocks would be parsed as complete statements, so any `&` binary expression whose left operand ended in a block would lead to a non-compiling suggestion. This adds a visitor to identify such expressions and add parentheses. This fixes #8052.
diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs
@@ -6,10 +6,10 @@ use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg};
 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_clippy_lint! {
 
 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 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBool {
                     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
@@ -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
@@ -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
@@ -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,33 +71,33 @@ 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`
    |
    = 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
 
