Auto merge of #10935 - Alexendoo:needless-if-cases, r=Manishearth

bors · bors · commit 8a1f0cd765a6 · 2023-06-13T04:34:37.000Z
Don't lint non-statement/faux empty `needless_if`s Also has a basic fall-back for `if` statements that have attributes applied to them and incorporates #10921 (review) while I was there r? `@Manishearth` changelog: none
diff --git a/clippy_lints/src/needless_if.rs b/clippy_lints/src/needless_if.rs
@@ -1,9 +1,6 @@
-use clippy_utils::{diagnostics::span_lint_and_sugg, is_from_proc_macro, source::snippet_with_applicability};
+use clippy_utils::{diagnostics::span_lint_and_sugg, higher::If, is_from_proc_macro, source::snippet_opt};
 use rustc_errors::Applicability;
-use rustc_hir::{
-    intravisit::{walk_expr, Visitor},
-    Expr, ExprKind, Node,
-};
+use rustc_hir::{ExprKind, Stmt, StmtKind};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -37,67 +34,42 @@ declare_clippy_lint! {
 declare_lint_pass!(NeedlessIf => [NEEDLESS_IF]);
 
 impl LateLintPass<'_> for NeedlessIf {
-    fn check_expr<'tcx>(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
-        if let ExprKind::If(if_expr, block, else_expr) = &expr.kind
-            && let ExprKind::Block(block, ..) = block.kind
+    fn check_stmt<'tcx>(&mut self, cx: &LateContext<'tcx>, stmt: &Stmt<'tcx>) {
+        if let StmtKind::Expr(expr) = stmt.kind
+            && let Some(If {cond, then, r#else: None }) = If::hir(expr)
+            && let ExprKind::Block(block, ..) = then.kind
             && block.stmts.is_empty()
             && block.expr.is_none()
-            && else_expr.is_none()
             && !in_external_macro(cx.sess(), expr.span)
+            && !is_from_proc_macro(cx, expr)
+            && let Some(then_snippet) = snippet_opt(cx, then.span)
+            // Ignore
+            // - empty macro expansions
+            // - empty reptitions in macro expansions
+            // - comments
+            // - #[cfg]'d out code
+            && then_snippet.chars().all(|ch| matches!(ch, '{' | '}') || ch.is_ascii_whitespace())
+            && let Some(cond_snippet) = snippet_opt(cx, cond.span)
         {
-            // Ignore `else if`
-            if let Some(parent_id) = cx.tcx.hir().opt_parent_id(expr.hir_id)
-                && let Some(Node::Expr(Expr {
-                    kind: ExprKind::If(_, _, Some(else_expr)),
-                    ..
-                })) = cx.tcx.hir().find(parent_id)
-                && else_expr.hir_id == expr.hir_id
-            {
-                return;
-            }
-
-            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,
+                stmt.span,
                 "this `if` branch is empty",
                 "you can remove it",
-                if if_expr.can_have_side_effects() {
-                    format!("{snippet};")
+                if cond.can_have_side_effects() || !cx.tcx.hir().attrs(stmt.hir_id).is_empty() {
+                    // `{ foo }` or `{ foo } && bar` placed into a statement position would be
+                    // interpreted as a block statement, force it to be an expression
+                    if cond_snippet.starts_with('{') {
+                        format!("({cond_snippet});")
+                    } else {
+                        format!("{cond_snippet};")
+                    }
                 } else {
                     String::new()
                 },
-                app,
+                Applicability::MachineApplicable,
             );
         }
     }
 }
-
-/// 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;
-        } else {
-            walk_expr(self, expr);
-        }
-    }
-}
diff --git a/tests/ui/needless_if.fixed b/tests/ui/needless_if.fixed
@@ -5,9 +5,12 @@
     clippy::blocks_in_if_conditions,
     clippy::if_same_then_else,
     clippy::ifs_same_cond,
+    clippy::let_unit_value,
     clippy::needless_else,
     clippy::no_effect,
     clippy::nonminimal_bool,
+    clippy::short_circuit_statement,
+    clippy::unnecessary_operation,
     unused
 )]
 #![warn(clippy::needless_if)]
@@ -16,45 +19,75 @@ 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;
+fn maybe_side_effect() -> bool {
     true
 }
 
 fn main() {
     // Lint
     
     // Do not remove the condition
-    no_side_effects();
-    let mut x = 0;
-    has_side_effects(&mut x);
-    assert_eq!(x, 1);
+    maybe_side_effect();
     // Do not lint
     if (true) {
     } else {
     }
-    {
+    ({
         return;
-    };
+    });
     // Do not lint if `else if` is present
     if (true) {
     } else if (true) {
     }
-    // Do not lint if any `let` is present
+    // Do not lint `if let` or let chains
     if let true = true {}
     if let true = true && true {}
     if true && let true = true {}
-    if {
+    // Can lint nested `if let`s
+    ({
         if let true = true && true { true } else { false }
-    } && true
-    {}
+    } && true);
     external! { if (true) {} }
     with_span! {
         span
         if (true) {}
     }
+
+    if true {
+        // comment
+    }
+
+    if true {
+        #[cfg(any())]
+        foo;
+    }
+
+    macro_rules! empty_expansion {
+        () => {};
+    }
+
+    if true {
+        empty_expansion!();
+    }
+
+    macro_rules! empty_repetition {
+        ($($t:tt)*) => {
+            if true {
+                $($t)*
+            }
+        }
+    }
+
+    empty_repetition!();
+
+    // Must be placed into an expression context to not be interpreted as a block
+    ({ maybe_side_effect() });
+    // Would be a block followed by `&&true` - a double reference to `true`
+    ({ maybe_side_effect() } && true);
+
+    // Don't leave trailing attributes
+    #[allow(unused)]
+    true;
+
+    let () = if maybe_side_effect() {};
 }
diff --git a/tests/ui/needless_if.rs b/tests/ui/needless_if.rs
@@ -5,9 +5,12 @@
     clippy::blocks_in_if_conditions,
     clippy::if_same_then_else,
     clippy::ifs_same_cond,
+    clippy::let_unit_value,
     clippy::needless_else,
     clippy::no_effect,
     clippy::nonminimal_bool,
+    clippy::short_circuit_statement,
+    clippy::unnecessary_operation,
     unused
 )]
 #![warn(clippy::needless_if)]
@@ -16,23 +19,15 @@ 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;
+fn maybe_side_effect() -> bool {
     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);
+    if maybe_side_effect() {}
     // Do not lint
     if (true) {
     } else {
@@ -44,10 +39,11 @@ fn main() {
     if (true) {
     } else if (true) {
     }
-    // Do not lint if any `let` is present
+    // Do not lint `if let` or let chains
     if let true = true {}
     if let true = true && true {}
     if true && let true = true {}
+    // Can lint nested `if let`s
     if {
         if let true = true && true { true } else { false }
     } && true
@@ -57,4 +53,42 @@ fn main() {
         span
         if (true) {}
     }
+
+    if true {
+        // comment
+    }
+
+    if true {
+        #[cfg(any())]
+        foo;
+    }
+
+    macro_rules! empty_expansion {
+        () => {};
+    }
+
+    if true {
+        empty_expansion!();
+    }
+
+    macro_rules! empty_repetition {
+        ($($t:tt)*) => {
+            if true {
+                $($t)*
+            }
+        }
+    }
+
+    empty_repetition!();
+
+    // Must be placed into an expression context to not be interpreted as a block
+    if { maybe_side_effect() } {}
+    // Would be a block followed by `&&true` - a double reference to `true`
+    if { maybe_side_effect() } && true {}
+
+    // Don't leave trailing attributes
+    #[allow(unused)]
+    if true {}
+
+    let () = if maybe_side_effect() {};
 }
diff --git a/tests/ui/needless_if.stderr b/tests/ui/needless_if.stderr
@@ -1,25 +1,19 @@
 error: this `if` branch is empty
-  --> $DIR/needless_if.rs:30:5
+  --> $DIR/needless_if.rs:28:5
    |
 LL |     if (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: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
+  --> $DIR/needless_if.rs:30:5
    |
-LL |     if has_side_effects(&mut x) {}
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can remove it: `has_side_effects(&mut x);`
+LL |     if maybe_side_effect() {}
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can remove it: `maybe_side_effect();`
 
 error: this `if` branch is empty
-  --> $DIR/needless_if.rs:40:5
+  --> $DIR/needless_if.rs:35:5
    |
 LL | /     if {
 LL | |         return;
@@ -28,10 +22,44 @@ LL | |     } {}
    |
 help: you can remove it
    |
-LL ~     {
+LL ~     ({
 LL +         return;
-LL +     };
+LL +     });
+   |
+
+error: this `if` branch is empty
+  --> $DIR/needless_if.rs:47:5
+   |
+LL | /     if {
+LL | |         if let true = true && true { true } else { false }
+LL | |     } && true
+LL | |     {}
+   | |______^
+   |
+help: you can remove it
+   |
+LL ~     ({
+LL +         if let true = true && true { true } else { false }
+LL +     } && true);
+   |
+
+error: this `if` branch is empty
+  --> $DIR/needless_if.rs:85:5
+   |
+LL |     if { maybe_side_effect() } {}
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can remove it: `({ maybe_side_effect() });`
+
+error: this `if` branch is empty
+  --> $DIR/needless_if.rs:87:5
+   |
+LL |     if { maybe_side_effect() } && true {}
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can remove it: `({ maybe_side_effect() } && true);`
+
+error: this `if` branch is empty
+  --> $DIR/needless_if.rs:91:5
    |
+LL |     if true {}
+   |     ^^^^^^^^^^ help: you can remove it: `true;`
 
-error: aborting due to 4 previous errors
+error: aborting due to 7 previous errors
 
