diff --git a/CHANGELOG.md b/CHANGELOG.md index d7102d2474e..b3a340d0cc8 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 0c66d36a1d6..4aebd0b7d01 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -446,6 +446,7 @@ 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, diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs index bd3ca7bcfac..71281a0b40b 100644 --- a/clippy_lints/src/needless_bool.rs +++ b/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 @@ "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 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { _ => (), } } + 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 { } 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 + } +} diff --git a/tests/ui/needless_bool_assign.fixed b/tests/ui/needless_bool_assign.fixed new file mode 100644 index 00000000000..3ed31d4d711 --- /dev/null +++ b/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; + } +} diff --git a/tests/ui/needless_bool_assign.rs b/tests/ui/needless_bool_assign.rs new file mode 100644 index 00000000000..efaeb67fa45 --- /dev/null +++ b/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; + } +} diff --git a/tests/ui/needless_bool_assign.stderr b/tests/ui/needless_bool_assign.stderr new file mode 100644 index 00000000000..601bbed5493 --- /dev/null +++ b/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 +