Implement new lint: if_then_some_else_none

5 files changed, 175 insertions, 0 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 41c334c6816..f7916511edf 100644
--- 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
new file mode 100644
index 00000000000..0bd393f8996
--- /dev/null
+++ b/clippy_lints/src/if_then_some_else_none.rs
@@ -0,0 +1,88 @@
+use crate::utils;
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind};
+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 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`"
+}
+
+declare_lint_pass!(IfThenSomeElseNone => [IF_THEN_SOME_ELSE_NONE]);
+
+impl LateLintPass<'_> for IfThenSomeElseNone {
+    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'tcx Expr<'_>) {
+        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 mut applicability = Applicability::MachineApplicable;
+                let cond_snip = utils::snippet_with_applicability(cx, cond.span, "[condition]", &mut applicability);
+                let arg_snip = utils::snippet_with_applicability(cx, then_arg.span, "", &mut applicability);
+                let sugg = format!(
+                    "{}.then(|| {{ /* snippet */ {} }})",
+                    cond_snip,
+                    arg_snip,
+                );
+                utils::span_lint_and_sugg(
+                    cx,
+                    IF_THEN_SOME_ELSE_NONE,
+                    expr.span,
+                    "this could be simplified with `bool::then`",
+                    "try this",
+                    sugg,
+                    applicability,
+                );
+            }
+        }
+    }
+}
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 8259fd3c320..9a7dedf416e 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -230,6 +230,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;
@@ -667,6 +668,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,
@@ -1282,6 +1284,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(|| box if_then_some_else_none::IfThenSomeElseNone);
 
     store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
         LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1297,6 +1300,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
new file mode 100644
index 00000000000..b19e2a50010
--- /dev/null
+++ b/tests/ui/if_then_some_else_none.rs
@@ -0,0 +1,66 @@
+#![warn(clippy::if_then_some_else_none)]
+
+fn main() {
+    // Should issue an error.
+    let _ = if foo() {
+        println!("true!");
+        Some("foo")
+    } 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 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
new file mode 100644
index 00000000000..7ad9c4ce79d
--- /dev/null
+++ b/tests/ui/if_then_some_else_none.stderr
@@ -0,0 +1,16 @@
+error: this could be simplified with `bool::then`
+  --> $DIR/if_then_some_else_none.rs:5:13
+   |
+LL |       let _ = if foo() {
+   |  _____________^
+LL | |         println!("true!");
+LL | |         Some("foo")
+LL | |     } else {
+LL | |         None
+LL | |     };
+   | |_____^ help: try this: `foo().then(|| { /* snippet */ "foo" })`
+   |
+   = note: `-D clippy::if-then-some-else-none` implied by `-D warnings`
+
+error: aborting due to previous error
+