result_map_or_into_option: add lint to catch manually adpating Result -> Option

Result<T, E> has an `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](rust-lang#2128)

Author: nickrtorres

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

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),

clippy_lints/src/methods/mod.rs

@@ -331,6 +331,24 @@ declare_clippy_lint! {
     "using `Option.map_or(None, f)`, which is more succinctly expressed as `and_then(f)`"
 }
 
+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:**
+    ///
+    /// **Example:**
+    /// ```rust
+    /// # let opt = Some(1);
+    /// # let r = opt.map_or(None, Some);
+    /// ```
+    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))`.
     ///
@@ -1249,6 +1267,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,37 +2543,73 @@ 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 {
-            match_qpath(qpath, &paths::OPTION_NONE)
-        } else {
-            false
-        };
+    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;
+    }
 
-        if map_or_arg_is_none {
-            // lint message
-            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,
-            );
-        }
+    let default_arg_is_none = if let hir::ExprKind::Path(ref qpath) = map_or_args[1].kind {
+        match_qpath(qpath, &paths::OPTION_NONE)
+    } else {
+        false
+    };
+
+    // This is really only needed if `is_result` holds. Computing it here
+    // makes `mess`'s assignment a bit easier, so just compute it here.
+    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
+    };
+
+    let mess = if is_option && default_arg_is_none {
+        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";
+        Some((
+            OPTION_MAP_OR_NONE,
+            msg,
+            "try using `and_then` instead",
+            format!("{0}.and_then({1})", self_snippet, func_snippet),
+        ))
+    } else if is_result && 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, "..");
+        Some((
+            RESULT_MAP_OR_INTO_OPTION,
+            msg,
+            "try using `ok` instead",
+            format!("{0}.ok()", self_snippet),
+        ))
+    } else {
+        None
+    };
+
+    if let Some((lint, msg, instead, hint)) = mess {
+        span_lint_and_sugg(
+            cx,
+            lint,
+            expr.span,
+            msg,
+            instead,
+            hint,
+            Applicability::MachineApplicable,
+        );
     }
 }
 

src/lintlist/mod.rs

@@ -1823,6 +1823,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         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",

tests/ui/result_map_or_into_option.fixed

@@ -0,0 +1,16 @@
+// 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);
+}

tests/ui/result_map_or_into_option.rs

@@ -0,0 +1,14 @@
+// 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);
+}

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
+