Rollup merge of #5415 - nickrtorres:master, r=flip1995

Add new lint for `Result<T, E>.map_or(None, Some(T))`

Fixes #5414

PR Checklist
---
- [x] Followed lint naming conventions (the name is a bit awkward, but it seems to conform)
- [x] Added passing UI tests (including committed .stderr file)
- [x] cargo test passes locally
- [x] Executed cargo dev update_lints
- [x] Added lint documentation
- [x] Run cargo dev fmt

`Result<T, E>` has an [`ok()`](https://doc.rust-lang.org/std/result/enum.Result.html#method.ok) method that adapts a `Result<T,E>` into an `Option<T>`.
It's possible to get around this adapter by writing `Result<T,E>.map_or(None, Some)`.

This lint is implemented as a new variant of the existing [`option_map_none` lint](https://github.com/rust-lang/rust-clippy/pull/2128)

7 files changed, 142 insertions, 16 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 894aab21fb3..b7ac3cace20 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1448,6 +1448,7 @@ Released 2018-09-13
 [`replace_consts`]: https://rust-lang.github.io/rust-clippy/master/index.html#replace_consts
 [`rest_pat_in_fully_bound_structs`]: https://rust-lang.github.io/rust-clippy/master/index.html#rest_pat_in_fully_bound_structs
 [`result_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_expect_used
+[`result_map_or_into_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_or_into_option
 [`result_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unit_fn
 [`result_map_unwrap_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unwrap_or_else
 [`result_unwrap_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_unwrap_used
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index e59cc8912d8..2dc98f96583 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -666,6 +666,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &methods::OPTION_UNWRAP_USED,
         &methods::OR_FUN_CALL,
         &methods::RESULT_EXPECT_USED,
+        &methods::RESULT_MAP_OR_INTO_OPTION,
         &methods::RESULT_MAP_UNWRAP_OR_ELSE,
         &methods::RESULT_UNWRAP_USED,
         &methods::SEARCH_IS_SOME,
@@ -1281,6 +1282,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::OPTION_AS_REF_DEREF),
         LintId::of(&methods::OPTION_MAP_OR_NONE),
         LintId::of(&methods::OR_FUN_CALL),
+        LintId::of(&methods::RESULT_MAP_OR_INTO_OPTION),
         LintId::of(&methods::SEARCH_IS_SOME),
         LintId::of(&methods::SHOULD_IMPLEMENT_TRAIT),
         LintId::of(&methods::SINGLE_CHAR_PATTERN),
@@ -1459,6 +1461,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::NEW_RET_NO_SELF),
         LintId::of(&methods::OK_EXPECT),
         LintId::of(&methods::OPTION_MAP_OR_NONE),
+        LintId::of(&methods::RESULT_MAP_OR_INTO_OPTION),
         LintId::of(&methods::SHOULD_IMPLEMENT_TRAIT),
         LintId::of(&methods::STRING_EXTEND_CHARS),
         LintId::of(&methods::UNNECESSARY_FOLD),
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
index e287d2ad484..07423111549 100644
--- a/clippy_lints/src/methods/mod.rs
+++ b/clippy_lints/src/methods/mod.rs
@@ -332,6 +332,32 @@ declare_clippy_lint! {
 }
 
 declare_clippy_lint! {
+    /// **What it does:** Checks for usage of `_.map_or(None, Some)`.
+    ///
+    /// **Why is this bad?** Readability, this can be written more concisely as
+    /// `_.ok()`.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// Bad:
+    /// ```rust
+    /// # let r: Result<u32, &str> = Ok(1);
+    /// assert_eq!(Some(1), r.map_or(None, Some));
+    /// ```
+    ///
+    /// Good:
+    /// ```rust
+    /// # let r: Result<u32, &str> = Ok(1);
+    /// assert_eq!(Some(1), r.ok());
+    /// ```
+    pub RESULT_MAP_OR_INTO_OPTION,
+    style,
+    "using `Result.map_or(None, Some)`, which is more succinctly expressed as `ok()`"
+}
+
+declare_clippy_lint! {
     /// **What it does:** Checks for usage of `_.and_then(|x| Some(y))`.
     ///
     /// **Why is this bad?** Readability, this can be written more concisely as
@@ -1249,6 +1275,7 @@ declare_lint_pass!(Methods => [
     OPTION_MAP_UNWRAP_OR,
     OPTION_MAP_UNWRAP_OR_ELSE,
     RESULT_MAP_UNWRAP_OR_ELSE,
+    RESULT_MAP_OR_INTO_OPTION,
     OPTION_MAP_OR_NONE,
     OPTION_AND_THEN_SOME,
     OR_FUN_CALL,
@@ -2524,38 +2551,78 @@ fn lint_map_unwrap_or_else<'a, 'tcx>(
     }
 }
 
-/// lint use of `_.map_or(None, _)` for `Option`s
+/// lint use of `_.map_or(None, _)` for `Option`s and `Result`s
 fn lint_map_or_none<'a, 'tcx>(
     cx: &LateContext<'a, 'tcx>,
     expr: &'tcx hir::Expr<'_>,
     map_or_args: &'tcx [hir::Expr<'_>],
 ) {
-    if match_type(cx, cx.tables.expr_ty(&map_or_args[0]), &paths::OPTION) {
-        // check if the first non-self argument to map_or() is None
-        let map_or_arg_is_none = if let hir::ExprKind::Path(ref qpath) = map_or_args[1].kind {
+    let is_option = match_type(cx, cx.tables.expr_ty(&map_or_args[0]), &paths::OPTION);
+    let is_result = match_type(cx, cx.tables.expr_ty(&map_or_args[0]), &paths::RESULT);
+
+    // There are two variants of this `map_or` lint:
+    // (1) using `map_or` as an adapter from `Result<T,E>` to `Option<T>`
+    // (2) using `map_or` as a combinator instead of `and_then`
+    //
+    // (For this lint) we don't care if any other type calls `map_or`
+    if !is_option && !is_result {
+        return;
+    }
+
+    let (lint_name, msg, instead, hint) = {
+        let default_arg_is_none = if let hir::ExprKind::Path(ref qpath) = map_or_args[1].kind {
             match_qpath(qpath, &paths::OPTION_NONE)
         } else {
+            return;
+        };
+
+        if !default_arg_is_none {
+            // nothing to lint!
+            return;
+        }
+
+        let f_arg_is_some = if let hir::ExprKind::Path(ref qpath) = map_or_args[2].kind {
+            match_qpath(qpath, &paths::OPTION_SOME)
+        } else {
             false
         };
 
-        if map_or_arg_is_none {
-            // lint message
+        if is_option {
+            let self_snippet = snippet(cx, map_or_args[0].span, "..");
+            let func_snippet = snippet(cx, map_or_args[2].span, "..");
             let msg = "called `map_or(None, f)` on an `Option` value. This can be done more directly by calling \
                        `and_then(f)` instead";
-            let map_or_self_snippet = snippet(cx, map_or_args[0].span, "..");
-            let map_or_func_snippet = snippet(cx, map_or_args[2].span, "..");
-            let hint = format!("{0}.and_then({1})", map_or_self_snippet, map_or_func_snippet);
-            span_lint_and_sugg(
-                cx,
+            (
                 OPTION_MAP_OR_NONE,
-                expr.span,
                 msg,
                 "try using `and_then` instead",
-                hint,
-                Applicability::MachineApplicable,
-            );
+                format!("{0}.and_then({1})", self_snippet, func_snippet),
+            )
+        } else if f_arg_is_some {
+            let msg = "called `map_or(None, Some)` on a `Result` value. This can be done more directly by calling \
+                       `ok()` instead";
+            let self_snippet = snippet(cx, map_or_args[0].span, "..");
+            (
+                RESULT_MAP_OR_INTO_OPTION,
+                msg,
+                "try using `ok` instead",
+                format!("{0}.ok()", self_snippet),
+            )
+        } else {
+            // nothing to lint!
+            return;
         }
-    }
+    };
+
+    span_lint_and_sugg(
+        cx,
+        lint_name,
+        expr.span,
+        msg,
+        instead,
+        hint,
+        Applicability::MachineApplicable,
+    );
 }
 
 /// Lint use of `_.and_then(|x| Some(y))` for `Option`s
diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs
index c48afc82347..6ececc8da04 100644
--- a/src/lintlist/mod.rs
+++ b/src/lintlist/mod.rs
@@ -1824,6 +1824,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         module: "methods",
     },
     Lint {
+        name: "result_map_or_into_option",
+        group: "style",
+        desc: "using `Result.map_or(None, Some)`, which is more succinctly expressed as `ok()`",
+        deprecation: None,
+        module: "methods",
+    },
+    Lint {
         name: "result_map_unit_fn",
         group: "complexity",
         desc: "using `result.map(f)`, where `f` is a function or closure that returns `()`",
diff --git a/tests/ui/result_map_or_into_option.fixed b/tests/ui/result_map_or_into_option.fixed
new file mode 100644
index 00000000000..331531b5165
--- /dev/null
+++ b/tests/ui/result_map_or_into_option.fixed
@@ -0,0 +1,19 @@
+// run-rustfix
+
+#![warn(clippy::result_map_or_into_option)]
+
+fn main() {
+    let opt: Result<u32, &str> = Ok(1);
+    let _ = opt.ok();
+
+    let rewrap = |s: u32| -> Option<u32> { Some(s) };
+
+    // A non-Some `f` arg should not emit the lint
+    let opt: Result<u32, &str> = Ok(1);
+    let _ = opt.map_or(None, rewrap);
+
+    // A non-Some `f` closure where the argument is not used as the
+    // return should not emit the lint
+    let opt: Result<u32, &str> = Ok(1);
+    opt.map_or(None, |_x| Some(1));
+}
diff --git a/tests/ui/result_map_or_into_option.rs b/tests/ui/result_map_or_into_option.rs
new file mode 100644
index 00000000000..3058480e2ad
--- /dev/null
+++ b/tests/ui/result_map_or_into_option.rs
@@ -0,0 +1,19 @@
+// run-rustfix
+
+#![warn(clippy::result_map_or_into_option)]
+
+fn main() {
+    let opt: Result<u32, &str> = Ok(1);
+    let _ = opt.map_or(None, Some);
+
+    let rewrap = |s: u32| -> Option<u32> { Some(s) };
+
+    // A non-Some `f` arg should not emit the lint
+    let opt: Result<u32, &str> = Ok(1);
+    let _ = opt.map_or(None, rewrap);
+
+    // A non-Some `f` closure where the argument is not used as the
+    // return should not emit the lint
+    let opt: Result<u32, &str> = Ok(1);
+    opt.map_or(None, |_x| Some(1));
+}
diff --git a/tests/ui/result_map_or_into_option.stderr b/tests/ui/result_map_or_into_option.stderr
new file mode 100644
index 00000000000..febf32147d1
--- /dev/null
+++ b/tests/ui/result_map_or_into_option.stderr
@@ -0,0 +1,10 @@
+error: called `map_or(None, Some)` on a `Result` value. This can be done more directly by calling `ok()` instead
+  --> $DIR/result_map_or_into_option.rs:7:13
+   |
+LL |     let _ = opt.map_or(None, Some);
+   |             ^^^^^^^^^^^^^^^^^^^^^^ help: try using `ok` instead: `opt.ok()`
+   |
+   = note: `-D clippy::result-map-or-into-option` implied by `-D warnings`
+
+error: aborting due to previous error
+