needless_else: new lint to check for empty else clauses

samueltardieu · samueltardieu · commit e6646eb5fd18 · 2023-05-22T11:52:26.000+02:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4874,6 +4874,7 @@ Released 2018-09-13
 [`needless_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_collect
 [`needless_continue`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_continue
 [`needless_doctest_main`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_doctest_main
+[`needless_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_else
 [`needless_for_each`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_for_each
 [`needless_late_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_late_init
 [`needless_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
@@ -449,6 +449,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::needless_bool::NEEDLESS_BOOL_ASSIGN_INFO,
     crate::needless_borrowed_ref::NEEDLESS_BORROWED_REFERENCE_INFO,
     crate::needless_continue::NEEDLESS_CONTINUE_INFO,
+    crate::needless_else::NEEDLESS_ELSE_INFO,
     crate::needless_for_each::NEEDLESS_FOR_EACH_INFO,
     crate::needless_late_init::NEEDLESS_LATE_INIT_INFO,
     crate::needless_parens_on_range_literals::NEEDLESS_PARENS_ON_RANGE_LITERALS_INFO,
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -218,6 +218,7 @@ mod needless_arbitrary_self_type;
 mod needless_bool;
 mod needless_borrowed_ref;
 mod needless_continue;
+mod needless_else;
 mod needless_for_each;
 mod needless_late_init;
 mod needless_parens_on_range_literals;
@@ -992,6 +993,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|_| Box::new(items_after_test_module::ItemsAfterTestModule));
     store.register_early_pass(|| Box::new(ref_patterns::RefPatterns));
     store.register_late_pass(|_| Box::new(default_constructed_unit_structs::DefaultConstructedUnitStructs));
+    store.register_early_pass(|| Box::new(needless_else::NeedlessElse));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 
diff --git a/clippy_lints/src/needless_else.rs b/clippy_lints/src/needless_else.rs
@@ -0,0 +1,57 @@
+use clippy_utils::{diagnostics::span_lint_and_sugg, source::trim_span, span_extract_comment};
+use rustc_ast::ast::{Expr, ExprKind};
+use rustc_errors::Applicability;
+use rustc_lint::{EarlyContext, EarlyLintPass, LintContext};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for empty `else` branches.
+    ///
+    /// ### Why is this bad?
+    /// An empty else branch does nothing and can be removed.
+    ///
+    /// ### Example
+    /// ```rust
+    ///# fn check() -> bool { true }
+    /// if check() {
+    ///     println!("Check successful!");
+    /// } else {
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    ///# fn check() -> bool { true }
+    /// if check() {
+    ///     println!("Check successful!");
+    /// }
+    /// ```
+    #[clippy::version = "1.71.0"]
+    pub NEEDLESS_ELSE,
+    style,
+    "empty else branch"
+}
+declare_lint_pass!(NeedlessElse => [NEEDLESS_ELSE]);
+
+impl EarlyLintPass for NeedlessElse {
+    fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
+        if let ExprKind::If(_, then_block, Some(else_clause)) = &expr.kind &&
+            let ExprKind::Block(block, _) = &else_clause.kind &&
+            !expr.span.from_expansion() &&
+            !else_clause.span.from_expansion() &&
+            block.stmts.is_empty() {
+                let span = trim_span(cx.sess().source_map(), expr.span.trim_start(then_block.span).unwrap());
+                if span_extract_comment(cx.sess().source_map(), span).is_empty() {
+                    span_lint_and_sugg(
+                        cx,
+                        NEEDLESS_ELSE,
+                        span,
+                        "this else branch is empty",
+                        "you can remove it",
+                        String::new(),
+                        Applicability::MachineApplicable,
+                    );
+                }
+            }
+    }
+}
diff --git a/tests/ui-toml/ifs_same_cond/ifs_same_cond.rs b/tests/ui-toml/ifs_same_cond/ifs_same_cond.rs
@@ -1,5 +1,5 @@
 #![warn(clippy::ifs_same_cond)]
-#![allow(clippy::if_same_then_else, clippy::comparison_chain)]
+#![allow(clippy::if_same_then_else, clippy::comparison_chain, clippy::needless_else)]
 
 fn main() {}
 
diff --git a/tests/ui/branches_sharing_code/valid_if_blocks.rs b/tests/ui/branches_sharing_code/valid_if_blocks.rs
@@ -1,6 +1,10 @@
 #![deny(clippy::branches_sharing_code, clippy::if_same_then_else)]
 #![allow(dead_code)]
-#![allow(clippy::mixed_read_write_in_expression, clippy::uninlined_format_args)]
+#![allow(
+    clippy::mixed_read_write_in_expression,
+    clippy::uninlined_format_args,
+    clippy::needless_else
+)]
 
 // This tests valid if blocks that shouldn't trigger the lint
 
diff --git a/tests/ui/branches_sharing_code/valid_if_blocks.stderr b/tests/ui/branches_sharing_code/valid_if_blocks.stderr
@@ -1,13 +1,13 @@
 error: this `if` has identical blocks
-  --> $DIR/valid_if_blocks.rs:105:14
+  --> $DIR/valid_if_blocks.rs:109:14
    |
 LL |       if false {
    |  ______________^
 LL | |     } else {
    | |_____^
    |
 note: same as this
-  --> $DIR/valid_if_blocks.rs:106:12
+  --> $DIR/valid_if_blocks.rs:110:12
    |
 LL |       } else {
    |  ____________^
@@ -20,7 +20,7 @@ LL | #![deny(clippy::branches_sharing_code, clippy::if_same_then_else)]
    |                                        ^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: this `if` has identical blocks
-  --> $DIR/valid_if_blocks.rs:116:15
+  --> $DIR/valid_if_blocks.rs:120:15
    |
 LL |       if x == 0 {
    |  _______________^
@@ -31,7 +31,7 @@ LL | |     } else {
    | |_____^
    |
 note: same as this
-  --> $DIR/valid_if_blocks.rs:120:12
+  --> $DIR/valid_if_blocks.rs:124:12
    |
 LL |       } else {
    |  ____________^
@@ -42,19 +42,19 @@ LL | |     }
    | |_____^
 
 error: this `if` has identical blocks
-  --> $DIR/valid_if_blocks.rs:127:23
+  --> $DIR/valid_if_blocks.rs:131:23
    |
 LL |     let _ = if x == 6 { 7 } else { 7 };
    |                       ^^^^^
    |
 note: same as this
-  --> $DIR/valid_if_blocks.rs:127:34
+  --> $DIR/valid_if_blocks.rs:131:34
    |
 LL |     let _ = if x == 6 { 7 } else { 7 };
    |                                  ^^^^^
 
 error: this `if` has identical blocks
-  --> $DIR/valid_if_blocks.rs:133:23
+  --> $DIR/valid_if_blocks.rs:137:23
    |
 LL |       } else if x == 68 {
    |  _______________________^
@@ -66,7 +66,7 @@ LL | |     } else {
    | |_____^
    |
 note: same as this
-  --> $DIR/valid_if_blocks.rs:138:12
+  --> $DIR/valid_if_blocks.rs:142:12
    |
 LL |       } else {
    |  ____________^
@@ -78,7 +78,7 @@ LL | |     };
    | |_____^
 
 error: this `if` has identical blocks
-  --> $DIR/valid_if_blocks.rs:147:23
+  --> $DIR/valid_if_blocks.rs:151:23
    |
 LL |       } else if x == 68 {
    |  _______________________^
@@ -88,7 +88,7 @@ LL | |     } else {
    | |_____^
    |
 note: same as this
-  --> $DIR/valid_if_blocks.rs:150:12
+  --> $DIR/valid_if_blocks.rs:154:12
    |
 LL |       } else {
    |  ____________^
diff --git a/tests/ui/crashes/ice-7410.rs b/tests/ui/crashes/ice-7410.rs
@@ -6,6 +6,7 @@
 #![no_std]
 #![allow(clippy::if_same_then_else)]
 #![allow(clippy::redundant_pattern_matching)]
+#![allow(clippy::needless_else)]
 
 use core::panic::PanicInfo;
 
diff --git a/tests/ui/ifs_same_cond.rs b/tests/ui/ifs_same_cond.rs
@@ -1,5 +1,5 @@
 #![warn(clippy::ifs_same_cond)]
-#![allow(clippy::if_same_then_else, clippy::comparison_chain)] // all empty blocks
+#![allow(clippy::if_same_then_else, clippy::comparison_chain, clippy::needless_else)] // all empty blocks
 
 fn ifs_same_cond() {
     let a = 0;
diff --git a/tests/ui/needless_else.fixed b/tests/ui/needless_else.fixed
@@ -0,0 +1,40 @@
+//@run-rustfix
+#![allow(unused)]
+#![warn(clippy::needless_else)]
+#![allow(clippy::suspicious_else_formatting)]
+
+macro_rules! mac {
+    ($test:expr) => {
+        if $test {
+            println!("Test successful!");
+        } else {
+        }
+    };
+}
+
+fn main() {
+    let b = std::hint::black_box(true);
+
+    if b {
+        println!("Foobar");
+    } 
+
+    if b {
+        println!("Foobar");
+    } else {
+        // Do not lint because this comment might be important
+    }
+
+    if b {
+        println!("Foobar");
+    } else
+    /* Do not lint because this comment might be important */
+    {
+    }
+
+    // Do not lint because of the expression
+    let _ = if b { 1 } else { 2 };
+
+    // Do not lint because inside a macro
+    mac!(b);
+}
diff --git a/tests/ui/needless_else.rs b/tests/ui/needless_else.rs
@@ -0,0 +1,41 @@
+//@run-rustfix
+#![allow(unused)]
+#![warn(clippy::needless_else)]
+#![allow(clippy::suspicious_else_formatting)]
+
+macro_rules! mac {
+    ($test:expr) => {
+        if $test {
+            println!("Test successful!");
+        } else {
+        }
+    };
+}
+
+fn main() {
+    let b = std::hint::black_box(true);
+
+    if b {
+        println!("Foobar");
+    } else {
+    }
+
+    if b {
+        println!("Foobar");
+    } else {
+        // Do not lint because this comment might be important
+    }
+
+    if b {
+        println!("Foobar");
+    } else
+    /* Do not lint because this comment might be important */
+    {
+    }
+
+    // Do not lint because of the expression
+    let _ = if b { 1 } else { 2 };
+
+    // Do not lint because inside a macro
+    mac!(b);
+}
diff --git a/tests/ui/needless_else.stderr b/tests/ui/needless_else.stderr
@@ -0,0 +1,12 @@
+error: this else branch is empty
+  --> $DIR/needless_else.rs:20:7
+   |
+LL |       } else {
+   |  _______^
+LL | |     }
+   | |_____^ help: you can remove it
+   |
+   = note: `-D clippy::needless-else` implied by `-D warnings`
+
+error: aborting due to previous error
+
diff --git a/tests/ui/needless_return.fixed b/tests/ui/needless_return.fixed
@@ -7,7 +7,8 @@
     clippy::if_same_then_else,
     clippy::single_match,
     clippy::needless_bool,
-    clippy::equatable_if_let
+    clippy::equatable_if_let,
+    clippy::needless_else
 )]
 #![warn(clippy::needless_return)]
 
diff --git a/tests/ui/needless_return.rs b/tests/ui/needless_return.rs
@@ -7,7 +7,8 @@
     clippy::if_same_then_else,
     clippy::single_match,
     clippy::needless_bool,
-    clippy::equatable_if_let
+    clippy::equatable_if_let,
+    clippy::needless_else
 )]
 #![warn(clippy::needless_return)]
 
diff --git a/tests/ui/needless_return.stderr b/tests/ui/needless_return.stderr
diff --git a/tests/ui/redundant_pattern_matching_drop_order.fixed b/tests/ui/redundant_pattern_matching_drop_order.fixed
diff --git a/tests/ui/redundant_pattern_matching_drop_order.rs b/tests/ui/redundant_pattern_matching_drop_order.rs
diff --git a/tests/ui/suspicious_else_formatting.rs b/tests/ui/suspicious_else_formatting.rs

Original file line number	Diff line number	Diff line change
`@@ -1,13 +1,13 @@`
`1`	`1`	error: this `if` has identical blocks
`2`		`- --> $DIR/valid_if_blocks.rs:105:14`
	`2`	`+ --> $DIR/valid_if_blocks.rs:109:14`
`3`	`3`	`\|`
`4`	`4`	`LL \| if false {`
`5`	`5`	`\| ______________^`
`6`	`6`	`LL \| \| } else {`
`7`	`7`	`\| \|_____^`
`8`	`8`	`\|`
`9`	`9`	`note: same as this`
`10`		`- --> $DIR/valid_if_blocks.rs:106:12`
	`10`	`+ --> $DIR/valid_if_blocks.rs:110:12`
`11`	`11`	`\|`
`12`	`12`	`LL \| } else {`
`13`	`13`	`\| ____________^`
`@@ -20,7 +20,7 @@ LL \| #![deny(clippy::branches_sharing_code, clippy::if_same_then_else)]`
`20`	`20`	`\| ^^^^^^^^^^^^^^^^^^^^^^^^^`
`21`	`21`
`22`	`22`	error: this `if` has identical blocks
`23`		`- --> $DIR/valid_if_blocks.rs:116:15`
	`23`	`+ --> $DIR/valid_if_blocks.rs:120:15`
`24`	`24`	`\|`
`25`	`25`	`LL \| if x == 0 {`
`26`	`26`	`\| _______________^`
`@@ -31,7 +31,7 @@ LL \| \| } else {`
`31`	`31`	`\| \|_____^`
`32`	`32`	`\|`
`33`	`33`	`note: same as this`
`34`		`- --> $DIR/valid_if_blocks.rs:120:12`
	`34`	`+ --> $DIR/valid_if_blocks.rs:124:12`
`35`	`35`	`\|`
`36`	`36`	`LL \| } else {`
`37`	`37`	`\| ____________^`
`@@ -42,19 +42,19 @@ LL \| \| }`
`42`	`42`	`\| \|_____^`
`43`	`43`
`44`	`44`	error: this `if` has identical blocks
`45`		`- --> $DIR/valid_if_blocks.rs:127:23`
	`45`	`+ --> $DIR/valid_if_blocks.rs:131:23`
`46`	`46`	`\|`
`47`	`47`	`LL \| let _ = if x == 6 { 7 } else { 7 };`
`48`	`48`	`\| ^^^^^`
`49`	`49`	`\|`
`50`	`50`	`note: same as this`
`51`		`- --> $DIR/valid_if_blocks.rs:127:34`
	`51`	`+ --> $DIR/valid_if_blocks.rs:131:34`
`52`	`52`	`\|`
`53`	`53`	`LL \| let _ = if x == 6 { 7 } else { 7 };`
`54`	`54`	`\| ^^^^^`
`55`	`55`
`56`	`56`	error: this `if` has identical blocks
`57`		`- --> $DIR/valid_if_blocks.rs:133:23`
	`57`	`+ --> $DIR/valid_if_blocks.rs:137:23`
`58`	`58`	`\|`
`59`	`59`	`LL \| } else if x == 68 {`
`60`	`60`	`\| _______________________^`
`@@ -66,7 +66,7 @@ LL \| \| } else {`
`66`	`66`	`\| \|_____^`
`67`	`67`	`\|`
`68`	`68`	`note: same as this`
`69`		`- --> $DIR/valid_if_blocks.rs:138:12`
	`69`	`+ --> $DIR/valid_if_blocks.rs:142:12`
`70`	`70`	`\|`
`71`	`71`	`LL \| } else {`
`72`	`72`	`\| ____________^`
`@@ -78,7 +78,7 @@ LL \| \| };`
`78`	`78`	`\| \|_____^`
`79`	`79`
`80`	`80`	error: this `if` has identical blocks
`81`		`- --> $DIR/valid_if_blocks.rs:147:23`
	`81`	`+ --> $DIR/valid_if_blocks.rs:151:23`
`82`	`82`	`\|`
`83`	`83`	`LL \| } else if x == 68 {`
`84`	`84`	`\| _______________________^`
`@@ -88,7 +88,7 @@ LL \| \| } else {`
`88`	`88`	`\| \|_____^`
`89`	`89`	`\|`
`90`	`90`	`note: same as this`
`91`		`- --> $DIR/valid_if_blocks.rs:150:12`
	`91`	`+ --> $DIR/valid_if_blocks.rs:154:12`
`92`	`92`	`\|`
`93`	`93`	`LL \| } else {`
`94`	`94`	`\| ____________^`