Add needless_if lint

clippy_lints/src/needless_if.rs

@@ -1,20 +1,23 @@
 use clippy_utils::{diagnostics::span_lint_and_sugg, is_from_proc_macro, source::snippet_with_applicability};
 use rustc_errors::Applicability;
-use rustc_hir::{Expr, ExprKind, Node};
+use rustc_hir::{
+    intravisit::{walk_expr, Visitor},
+    Expr, ExprKind, Node,
+};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Checks for empty `if` statements with no else branch.
+    /// Checks for empty `if` branches with no else branch.
     ///
     /// ### Why is this bad?
     /// It can be entirely omitted, and often the condition too.
     ///
     /// ### Known issues
-    /// This will only suggest to remove the `if` statement, not the condition. Other lints such as
-    /// `no_effect` will take care of removing the condition if it's unnecessary.
+    /// This will usually only suggest to remove the `if` statement, not the condition. Other lints
+    /// such as `no_effect` will take care of removing the condition if it's unnecessary.
     ///
     /// ### Example
     /// ```rust,ignore
@@ -42,9 +45,6 @@ impl LateLintPass<'_> for NeedlessIf {
             && else_expr.is_none()
             && !in_external_macro(cx.sess(), expr.span)
         {
-            let mut app = Applicability::MachineApplicable;
-            let snippet = snippet_with_applicability(cx, if_expr.span, "{ ... }", &mut app);
-
             // Ignore `else if`
             if let Some(parent_id) = cx.tcx.hir().opt_parent_id(expr.hir_id)
                 && let Some(Node::Expr(Expr {
@@ -56,19 +56,48 @@ impl LateLintPass<'_> for NeedlessIf {
                 return;
             }
 
-            if is_from_proc_macro(cx, expr) {
+            if is_any_if_let(if_expr) || is_from_proc_macro(cx, expr) {
                 return;
             }
 
+            let mut app = Applicability::MachineApplicable;
+            let snippet = snippet_with_applicability(cx, if_expr.span, "{ ... }", &mut app);
+
             span_lint_and_sugg(
                 cx,
                 NEEDLESS_IF,
                 expr.span,
-                "this if branch is empty",
+                "this `if` branch is empty",
                 "you can remove it",
-                format!("{snippet};"),
-                Applicability::MachineApplicable,
+                if if_expr.can_have_side_effects() {
+                    format!("{snippet};")
+                } else {
+                    String::new()
+                },
+                app,
             );
         }
     }
 }
+
+/// Returns true if any `Expr` contained within this `Expr` is a `Let`, else false.
+///
+/// Really wish `Expr` had a `walk` method...
+fn is_any_if_let(expr: &Expr<'_>) -> bool {
+    let mut v = IsAnyLetVisitor(false);
+
+    v.visit_expr(expr);
+    v.0
+}
+
+struct IsAnyLetVisitor(bool);
+
+impl Visitor<'_> for IsAnyLetVisitor {
+    fn visit_expr(&mut self, expr: &Expr<'_>) {
+        if matches!(expr.kind, ExprKind::Let(..)) {
+            self.0 = true;
+        }
+
+        walk_expr(self, expr);
+    }
+}

tests/ui/needless_if.fixed

@@ -1,11 +1,13 @@
 //@run-rustfix
 //@aux-build:proc_macros.rs
+#![feature(let_chains)]
 #![allow(
     clippy::blocks_in_if_conditions,
     clippy::if_same_then_else,
     clippy::ifs_same_cond,
     clippy::needless_else,
     clippy::no_effect,
+    clippy::nonminimal_bool,
     unused
 )]
 #![warn(clippy::needless_if)]
@@ -14,21 +16,42 @@ extern crate proc_macros;
 use proc_macros::external;
 use proc_macros::with_span;
 
+fn no_side_effects() -> bool {
+    true
+}
+
+fn has_side_effects(a: &mut u32) -> bool {
+    *a = 1;
+    true
+}
+
 fn main() {
     // Lint
-    (true);
+
+    // Do not remove the condition
+    no_side_effects();
+    let mut x = 0;
+    has_side_effects(&mut x);
+    assert_eq!(x, 1);
     // Do not lint
     if (true) {
     } else {
     }
+    {
+        return;
+    };
     // Do not lint if `else if` is present
     if (true) {
     } else if (true) {
     }
-    // Ensure clippy does not bork this up, other cases should be added
-    {
-        return;
-    };
+    // Do not lint if any `let` is present
+    if let true = true {}
+    if let true = true && true {}
+    if true && let true = true {}
+    if {
+        if let true = true && true { true } else { false }
+    } && true
+    {}
     external! { if (true) {} }
     with_span! {
         span

tests/ui/needless_if.rs

@@ -1,11 +1,13 @@
 //@run-rustfix
 //@aux-build:proc_macros.rs
+#![feature(let_chains)]
 #![allow(
     clippy::blocks_in_if_conditions,
     clippy::if_same_then_else,
     clippy::ifs_same_cond,
     clippy::needless_else,
     clippy::no_effect,
+    clippy::nonminimal_bool,
     unused
 )]
 #![warn(clippy::needless_if)]
@@ -14,21 +16,42 @@ extern crate proc_macros;
 use proc_macros::external;
 use proc_macros::with_span;
 
+fn no_side_effects() -> bool {
+    true
+}
+
+fn has_side_effects(a: &mut u32) -> bool {
+    *a = 1;
+    true
+}
+
 fn main() {
     // Lint
     if (true) {}
+    // Do not remove the condition
+    if no_side_effects() {}
+    let mut x = 0;
+    if has_side_effects(&mut x) {}
+    assert_eq!(x, 1);
     // Do not lint
     if (true) {
     } else {
     }
+    if {
+        return;
+    } {}
     // Do not lint if `else if` is present
     if (true) {
     } else if (true) {
     }
-    // Ensure clippy does not bork this up, other cases should be added
+    // Do not lint if any `let` is present
+    if let true = true {}
+    if let true = true && true {}
+    if true && let true = true {}
     if {
-        return;
-    } {}
+        if let true = true && true { true } else { false }
+    } && true
+    {}
     external! { if (true) {} }
     with_span! {
         span

tests/ui/needless_if.stderr

@@ -1,13 +1,25 @@
-error: this if branch is empty
-  --> $DIR/needless_if.rs:19:5
+error: this `if` branch is empty
+  --> $DIR/needless_if.rs:30:5
    |
 LL |     if (true) {}
-   |     ^^^^^^^^^^^^ help: you can remove it: `(true);`
+   |     ^^^^^^^^^^^^ help: you can remove it
    |
    = note: `-D clippy::needless-if` implied by `-D warnings`
 
-error: this if branch is empty
-  --> $DIR/needless_if.rs:29:5
+error: this `if` branch is empty
+  --> $DIR/needless_if.rs:32:5
+   |
+LL |     if no_side_effects() {}
+   |     ^^^^^^^^^^^^^^^^^^^^^^^ help: you can remove it: `no_side_effects();`
+
+error: this `if` branch is empty
+  --> $DIR/needless_if.rs:34:5
+   |
+LL |     if has_side_effects(&mut x) {}
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can remove it: `has_side_effects(&mut x);`
+
+error: this `if` branch is empty
+  --> $DIR/needless_if.rs:40:5
    |
 LL | /     if {
 LL | |         return;
@@ -21,5 +33,5 @@ LL +         return;
 LL +     };
    |
 
-error: aborting due to 2 previous errors
+error: aborting due to 4 previous errors