Auto merge of #10432 - samueltardieu:issue-10430, r=Manishearth

New lint: detect `if` expressions with simple boolean assignments to the same target

Closes #10430

changelog: [`needless_bool_assign`] new lint to detect simple boolean assignment to the same target in `if` branches

Author: bors

CHANGELOG.md

@@ -4864,6 +4864,7 @@ Released 2018-09-13
 [`needless_arbitrary_self_type`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_arbitrary_self_type
 [`needless_bitwise_bool`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_bitwise_bool
 [`needless_bool`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_bool
+[`needless_bool_assign`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_bool_assign
 [`needless_borrow`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
 [`needless_borrowed_reference`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrowed_reference
 [`needless_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_collect

clippy_lints/src/declared_lints.rs

@@ -446,6 +446,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::needless_arbitrary_self_type::NEEDLESS_ARBITRARY_SELF_TYPE_INFO,
     crate::needless_bool::BOOL_COMPARISON_INFO,
     crate::needless_bool::NEEDLESS_BOOL_INFO,
+    crate::needless_bool::NEEDLESS_BOOL_ASSIGN_INFO,
     crate::needless_borrowed_ref::NEEDLESS_BORROWED_REFERENCE_INFO,
     crate::needless_continue::NEEDLESS_CONTINUE_INFO,
     crate::needless_for_each::NEEDLESS_FOR_EACH_INFO,

clippy_lints/src/needless_bool.rs

@@ -3,10 +3,12 @@
 //! This lint is **warn** by default
 
 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::{get_parent_node, is_else_clause, is_expn_of, peel_blocks, peel_blocks_with_stmt};
+use clippy_utils::{
+    get_parent_node, is_else_clause, is_expn_of, peel_blocks, peel_blocks_with_stmt, span_extract_comment,
+};
+use clippy_utils::{higher, SpanlessEq};
 use rustc_ast::ast::LitKind;
 use rustc_errors::Applicability;
 use rustc_hir::{BinOpKind, Block, Expr, ExprKind, HirId, Node, UnOp};
@@ -77,7 +79,39 @@ declare_clippy_lint! {
     "comparing a variable to a boolean, e.g., `if x == true` or `if x != true`"
 }
 
-declare_lint_pass!(NeedlessBool => [NEEDLESS_BOOL]);
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for expressions of the form `if c { x = true } else { x = false }`
+    /// (or vice versa) and suggest assigning the variable directly from the
+    /// condition.
+    ///
+    /// ### Why is this bad?
+    /// Redundant code.
+    ///
+    /// ### Example
+    /// ```rust,ignore
+    /// # fn must_keep(x: i32, y: i32) -> bool { x == y }
+    /// # let x = 32; let y = 10;
+    /// # let mut skip: bool;
+    /// if must_keep(x, y) {
+    ///     skip = false;
+    /// } else {
+    ///     skip = true;
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust,ignore
+    /// # fn must_keep(x: i32, y: i32) -> bool { x == y }
+    /// # let x = 32; let y = 10;
+    /// # let mut skip: bool;
+    /// skip = !must_keep(x, y);
+    /// ```
+    #[clippy::version = "1.69.0"]
+    pub NEEDLESS_BOOL_ASSIGN,
+    complexity,
+    "setting the same boolean variable in both branches of an if-statement"
+}
+declare_lint_pass!(NeedlessBool => [NEEDLESS_BOOL, NEEDLESS_BOOL_ASSIGN]);
 
 fn condition_needs_parentheses(e: &Expr<'_>) -> bool {
     let mut inner = e;
@@ -173,6 +207,29 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBool {
                     _ => (),
                 }
             }
+            if let Some((lhs_a, a)) = fetch_assign(then) &&
+                let Some((lhs_b, b)) = fetch_assign(r#else) &&
+                SpanlessEq::new(cx).eq_expr(lhs_a, lhs_b) &&
+                span_extract_comment(cx.tcx.sess.source_map(), e.span).is_empty()
+            {
+                let mut applicability = Applicability::MachineApplicable;
+                let cond = Sugg::hir_with_applicability(cx, cond, "..", &mut applicability);
+                let lhs = snippet_with_applicability(cx, lhs_a.span, "..", &mut applicability);
+                let sugg = if a == b {
+                    format!("{cond}; {lhs} = {a:?};")
+                } else {
+                    format!("{lhs} = {};", if a { cond } else { !cond })
+                };
+                span_lint_and_sugg(
+                    cx,
+                    NEEDLESS_BOOL_ASSIGN,
+                    e.span,
+                    "this if-then-else expression assigns a bool literal",
+                    "you can reduce it to",
+                    sugg,
+                    applicability
+                );
+            }
         }
     }
 }
@@ -376,3 +433,11 @@ fn fetch_bool_expr(expr: &Expr<'_>) -> Option<bool> {
     }
     None
 }
+
+fn fetch_assign<'tcx>(expr: &'tcx Expr<'tcx>) -> Option<(&'tcx Expr<'tcx>, bool)> {
+    if let ExprKind::Assign(lhs, rhs, _) = peel_blocks_with_stmt(expr).kind {
+        fetch_bool_expr(rhs).map(|b| (lhs, b))
+    } else {
+        None
+    }
+}

tests/ui/needless_bool_assign.fixed

@@ -0,0 +1,33 @@
+//@run-rustfix
+
+#![allow(unused)]
+#![warn(clippy::needless_bool_assign)]
+
+fn random() -> bool {
+    true
+}
+
+fn main() {
+    struct Data {
+        field: bool,
+    };
+    let mut a = Data { field: false };
+    a.field = random() && random();
+    a.field = !(random() && random());
+    // Do not lint…
+    if random() {
+        a.field = false;
+    } else {
+        // …to avoid losing this comment
+        a.field = true
+    }
+    // This one also triggers lint `clippy::if_same_then_else`
+    // which does not suggest a rewrite.
+    random(); a.field = true;
+    let mut b = false;
+    if random() {
+        a.field = false;
+    } else {
+        b = true;
+    }
+}

tests/ui/needless_bool_assign.rs

@@ -0,0 +1,45 @@
+//@run-rustfix
+
+#![allow(unused)]
+#![warn(clippy::needless_bool_assign)]
+
+fn random() -> bool {
+    true
+}
+
+fn main() {
+    struct Data {
+        field: bool,
+    };
+    let mut a = Data { field: false };
+    if random() && random() {
+        a.field = true;
+    } else {
+        a.field = false
+    }
+    if random() && random() {
+        a.field = false;
+    } else {
+        a.field = true
+    }
+    // Do not lint…
+    if random() {
+        a.field = false;
+    } else {
+        // …to avoid losing this comment
+        a.field = true
+    }
+    // This one also triggers lint `clippy::if_same_then_else`
+    // which does not suggest a rewrite.
+    if random() {
+        a.field = true;
+    } else {
+        a.field = true;
+    }
+    let mut b = false;
+    if random() {
+        a.field = false;
+    } else {
+        b = true;
+    }
+}

tests/ui/needless_bool_assign.stderr

@@ -0,0 +1,53 @@
+error: this if-then-else expression assigns a bool literal
+  --> $DIR/needless_bool_assign.rs:15:5
+   |
+LL | /     if random() && random() {
+LL | |         a.field = true;
+LL | |     } else {
+LL | |         a.field = false
+LL | |     }
+   | |_____^ help: you can reduce it to: `a.field = random() && random();`
+   |
+   = note: `-D clippy::needless-bool-assign` implied by `-D warnings`
+
+error: this if-then-else expression assigns a bool literal
+  --> $DIR/needless_bool_assign.rs:20:5
+   |
+LL | /     if random() && random() {
+LL | |         a.field = false;
+LL | |     } else {
+LL | |         a.field = true
+LL | |     }
+   | |_____^ help: you can reduce it to: `a.field = !(random() && random());`
+
+error: this if-then-else expression assigns a bool literal
+  --> $DIR/needless_bool_assign.rs:34:5
+   |
+LL | /     if random() {
+LL | |         a.field = true;
+LL | |     } else {
+LL | |         a.field = true;
+LL | |     }
+   | |_____^ help: you can reduce it to: `random(); a.field = true;`
+
+error: this `if` has identical blocks
+  --> $DIR/needless_bool_assign.rs:34:17
+   |
+LL |       if random() {
+   |  _________________^
+LL | |         a.field = true;
+LL | |     } else {
+   | |_____^
+   |
+note: same as this
+  --> $DIR/needless_bool_assign.rs:36:12
+   |
+LL |       } else {
+   |  ____________^
+LL | |         a.field = true;
+LL | |     }
+   | |_____^
+   = note: `#[deny(clippy::if_same_then_else)]` on by default
+
+error: aborting due to 4 previous errors
+