Auto merge of #6859 - magurotuna:if_then_some_else_none, r=giraffate

bors · bors · commit 781de34222a7 · 2021-03-13T15:54:54.000Z
Implement new lint: if_then_some_else_none Resolves #6760 changelog: Added a new lint: `if_then_some_else_none`
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2103,6 +2103,7 @@ Released 2018-09-13
 [`if_let_some_result`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_let_some_result
 [`if_not_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_not_else
 [`if_same_then_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else
+[`if_then_some_else_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_then_some_else_none
 [`ifs_same_cond`]: https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond
 [`implicit_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_clone
 [`implicit_hasher`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_hasher
diff --git a/clippy_lints/src/if_then_some_else_none.rs b/clippy_lints/src/if_then_some_else_none.rs
@@ -0,0 +1,115 @@
+use crate::utils;
+use if_chain::if_chain;
+use rustc_hir::{Expr, ExprKind};
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_middle::lint::in_external_macro;
+use rustc_semver::RustcVersion;
+use rustc_session::{declare_tool_lint, impl_lint_pass};
+
+const IF_THEN_SOME_ELSE_NONE_MSRV: RustcVersion = RustcVersion::new(1, 50, 0);
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for if-else that could be written to `bool::then`.
+    ///
+    /// **Why is this bad?** Looks a little redundant. Using `bool::then` helps it have less lines of code.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// # let v = vec![0];
+    /// let a = if v.is_empty() {
+    ///     println!("true!");
+    ///     Some(42)
+    /// } else {
+    ///     None
+    /// };
+    /// ```
+    ///
+    /// Could be written:
+    ///
+    /// ```rust
+    /// # let v = vec![0];
+    /// let a = v.is_empty().then(|| {
+    ///     println!("true!");
+    ///     42
+    /// });
+    /// ```
+    pub IF_THEN_SOME_ELSE_NONE,
+    restriction,
+    "Finds if-else that could be written using `bool::then`"
+}
+
+pub struct IfThenSomeElseNone {
+    msrv: Option<RustcVersion>,
+}
+
+impl IfThenSomeElseNone {
+    #[must_use]
+    pub fn new(msrv: Option<RustcVersion>) -> Self {
+        Self { msrv }
+    }
+}
+
+impl_lint_pass!(IfThenSomeElseNone => [IF_THEN_SOME_ELSE_NONE]);
+
+impl LateLintPass<'_> for IfThenSomeElseNone {
+    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'tcx Expr<'_>) {
+        if !utils::meets_msrv(self.msrv.as_ref(), &IF_THEN_SOME_ELSE_NONE_MSRV) {
+            return;
+        }
+
+        if in_external_macro(cx.sess(), expr.span) {
+            return;
+        }
+
+        // We only care about the top-most `if` in the chain
+        if utils::parent_node_is_if_expr(expr, cx) {
+            return;
+        }
+
+        if_chain! {
+            if let ExprKind::If(ref cond, ref then, Some(ref els)) = expr.kind;
+            if let ExprKind::Block(ref then_block, _) = then.kind;
+            if let Some(ref then_expr) = then_block.expr;
+            if let ExprKind::Call(ref then_call, [then_arg]) = then_expr.kind;
+            if let ExprKind::Path(ref then_call_qpath) = then_call.kind;
+            if utils::match_qpath(then_call_qpath, &utils::paths::OPTION_SOME);
+            if let ExprKind::Block(ref els_block, _) = els.kind;
+            if els_block.stmts.is_empty();
+            if let Some(ref els_expr) = els_block.expr;
+            if let ExprKind::Path(ref els_call_qpath) = els_expr.kind;
+            if utils::match_qpath(els_call_qpath, &utils::paths::OPTION_NONE);
+            then {
+                let cond_snip = utils::snippet_with_macro_callsite(cx, cond.span, "[condition]");
+                let cond_snip = if matches!(cond.kind, ExprKind::Unary(_, _) | ExprKind::Binary(_, _, _)) {
+                    format!("({})", cond_snip)
+                } else {
+                    cond_snip.into_owned()
+                };
+                let arg_snip = utils::snippet_with_macro_callsite(cx, then_arg.span, "");
+                let closure_body = if then_block.stmts.is_empty() {
+                    arg_snip.into_owned()
+                } else {
+                    format!("{{ /* snippet */ {} }}", arg_snip)
+                };
+                let help = format!(
+                    "consider using `bool::then` like: `{}.then(|| {})`",
+                    cond_snip,
+                    closure_body,
+                );
+                utils::span_lint_and_help(
+                    cx,
+                    IF_THEN_SOME_ELSE_NONE,
+                    expr.span,
+                    "this could be simplified with `bool::then`",
+                    None,
+                    &help,
+                );
+            }
+        }
+    }
+
+    extract_msrv_attr!(LateContext);
+}
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -231,6 +231,7 @@ mod identity_op;
 mod if_let_mutex;
 mod if_let_some_result;
 mod if_not_else;
+mod if_then_some_else_none;
 mod implicit_return;
 mod implicit_saturating_sub;
 mod inconsistent_struct_constructor;
@@ -680,6 +681,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &if_let_mutex::IF_LET_MUTEX,
         &if_let_some_result::IF_LET_SOME_RESULT,
         &if_not_else::IF_NOT_ELSE,
+        &if_then_some_else_none::IF_THEN_SOME_ELSE_NONE,
         &implicit_return::IMPLICIT_RETURN,
         &implicit_saturating_sub::IMPLICIT_SATURATING_SUB,
         &inconsistent_struct_constructor::INCONSISTENT_STRUCT_CONSTRUCTOR,
@@ -1280,6 +1282,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box redundant_slicing::RedundantSlicing);
     store.register_late_pass(|| box from_str_radix_10::FromStrRadix10);
     store.register_late_pass(|| box manual_map::ManualMap);
+    store.register_late_pass(move || box if_then_some_else_none::IfThenSomeElseNone::new(msrv));
 
     store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
         LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1295,6 +1298,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&exhaustive_items::EXHAUSTIVE_STRUCTS),
         LintId::of(&exit::EXIT),
         LintId::of(&float_literal::LOSSY_FLOAT_LITERAL),
+        LintId::of(&if_then_some_else_none::IF_THEN_SOME_ELSE_NONE),
         LintId::of(&implicit_return::IMPLICIT_RETURN),
         LintId::of(&indexing_slicing::INDEXING_SLICING),
         LintId::of(&inherent_impl::MULTIPLE_INHERENT_IMPL),
diff --git a/tests/ui/if_then_some_else_none.rs b/tests/ui/if_then_some_else_none.rs
@@ -0,0 +1,104 @@
+#![warn(clippy::if_then_some_else_none)]
+#![feature(custom_inner_attributes)]
+
+fn main() {
+    // Should issue an error.
+    let _ = if foo() {
+        println!("true!");
+        Some("foo")
+    } else {
+        None
+    };
+
+    // Should issue an error when macros are used.
+    let _ = if matches!(true, true) {
+        println!("true!");
+        Some(matches!(true, false))
+    } else {
+        None
+    };
+
+    // Should issue an error. Binary expression `o < 32` should be parenthesized.
+    let x = Some(5);
+    let _ = x.and_then(|o| if o < 32 { Some(o) } else { None });
+
+    // Should issue an error. Unary expression `!x` should be parenthesized.
+    let x = true;
+    let _ = if !x { Some(0) } else { None };
+
+    // Should not issue an error since the `else` block has a statement besides `None`.
+    let _ = if foo() {
+        println!("true!");
+        Some("foo")
+    } else {
+        eprintln!("false...");
+        None
+    };
+
+    // Should not issue an error since there are more than 2 blocks in the if-else chain.
+    let _ = if foo() {
+        println!("foo true!");
+        Some("foo")
+    } else if bar() {
+        println!("bar true!");
+        Some("bar")
+    } else {
+        None
+    };
+
+    let _ = if foo() {
+        println!("foo true!");
+        Some("foo")
+    } else {
+        bar().then(|| {
+            println!("bar true!");
+            "bar"
+        })
+    };
+
+    // Should not issue an error since the `then` block has `None`, not `Some`.
+    let _ = if foo() { None } else { Some("foo is false") };
+
+    // Should not issue an error since the `else` block doesn't use `None` directly.
+    let _ = if foo() { Some("foo is true") } else { into_none() };
+
+    // Should not issue an error since the `then` block doesn't use `Some` directly.
+    let _ = if foo() { into_some("foo") } else { None };
+}
+
+fn _msrv_1_49() {
+    #![clippy::msrv = "1.49"]
+    // `bool::then` was stabilized in 1.50. Do not lint this
+    let _ = if foo() {
+        println!("true!");
+        Some(149)
+    } else {
+        None
+    };
+}
+
+fn _msrv_1_50() {
+    #![clippy::msrv = "1.50"]
+    let _ = if foo() {
+        println!("true!");
+        Some(150)
+    } else {
+        None
+    };
+}
+
+fn foo() -> bool {
+    unimplemented!()
+}
+
+fn bar() -> bool {
+    unimplemented!()
+}
+
+fn into_some<T>(v: T) -> Option<T> {
+    Some(v)
+}
+
+fn into_none<T>() -> Option<T> {
+    None
+}
diff --git a/tests/ui/if_then_some_else_none.stderr b/tests/ui/if_then_some_else_none.stderr
@@ -0,0 +1,61 @@
+error: this could be simplified with `bool::then`
+  --> $DIR/if_then_some_else_none.rs:6:13
+   |
+LL |       let _ = if foo() {
+   |  _____________^
+LL | |         println!("true!");
+LL | |         Some("foo")
+LL | |     } else {
+LL | |         None
+LL | |     };
+   | |_____^
+   |
+   = note: `-D clippy::if-then-some-else-none` implied by `-D warnings`
+   = help: consider using `bool::then` like: `foo().then(|| { /* snippet */ "foo" })`
+
+error: this could be simplified with `bool::then`
+  --> $DIR/if_then_some_else_none.rs:14:13
+   |
+LL |       let _ = if matches!(true, true) {
+   |  _____________^
+LL | |         println!("true!");
+LL | |         Some(matches!(true, false))
+LL | |     } else {
+LL | |         None
+LL | |     };
+   | |_____^
+   |
+   = help: consider using `bool::then` like: `matches!(true, true).then(|| { /* snippet */ matches!(true, false) })`
+
+error: this could be simplified with `bool::then`
+  --> $DIR/if_then_some_else_none.rs:23:28
+   |
+LL |     let _ = x.and_then(|o| if o < 32 { Some(o) } else { None });
+   |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using `bool::then` like: `(o < 32).then(|| o)`
+
+error: this could be simplified with `bool::then`
+  --> $DIR/if_then_some_else_none.rs:27:13
+   |
+LL |     let _ = if !x { Some(0) } else { None };
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using `bool::then` like: `(!x).then(|| 0)`
+
+error: this could be simplified with `bool::then`
+  --> $DIR/if_then_some_else_none.rs:82:13
+   |
+LL |       let _ = if foo() {
+   |  _____________^
+LL | |         println!("true!");
+LL | |         Some(150)
+LL | |     } else {
+LL | |         None
+LL | |     };
+   | |_____^
+   |
+   = help: consider using `bool::then` like: `foo().then(|| { /* snippet */ 150 })`
+
+error: aborting due to 5 previous errors
+
