Auto merge of #7971 - togami2864:fix/option-map-or-none, r=llogiq

fix suggestion in option_map_or_none

fix: #7960
changelog: change suggestion in the lint rule `option_map_or_none`

4 files changed, 141 insertions, 69 deletions

diff --git a/clippy_lints/src/methods/option_map_or_none.rs b/clippy_lints/src/methods/option_map_or_none.rs
index e99b6b07d15..5e5c1038e82 100644
--- a/clippy_lints/src/methods/option_map_or_none.rs
+++ b/clippy_lints/src/methods/option_map_or_none.rs
@@ -1,7 +1,7 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::is_lang_ctor;
 use clippy_utils::source::snippet;
 use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::{is_lang_ctor, single_segment_path};
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_hir::LangItem::{OptionNone, OptionSome};
@@ -11,6 +11,28 @@ use rustc_span::symbol::sym;
 use super::OPTION_MAP_OR_NONE;
 use super::RESULT_MAP_OR_INTO_OPTION;
 
+// The expression inside a closure may or may not have surrounding braces
+// which causes problems when generating a suggestion.
+fn reduce_unit_expression<'a>(
+    cx: &LateContext<'_>,
+    expr: &'a hir::Expr<'_>,
+) -> Option<(&'a hir::Expr<'a>, &'a [hir::Expr<'a>])> {
+    match expr.kind {
+        hir::ExprKind::Call(func, arg_char) => Some((func, arg_char)),
+        hir::ExprKind::Block(block, _) => {
+            match (block.stmts, block.expr) {
+                (&[], Some(inner_expr)) => {
+                    // If block only contains an expression,
+                    // reduce `|x| { x + 1 }` to `|x| x + 1`
+                    reduce_unit_expression(cx, inner_expr)
+                },
+                _ => None,
+            }
+        },
+        _ => None,
+    }
+}
+
 /// lint use of `_.map_or(None, _)` for `Option`s and `Result`s
 pub(super) fn check<'tcx>(
     cx: &LateContext<'tcx>,
@@ -31,58 +53,75 @@ pub(super) fn check<'tcx>(
         return;
     }
 
-    let (lint_name, msg, instead, hint) = {
-        let default_arg_is_none = if let hir::ExprKind::Path(ref qpath) = def_arg.kind {
-            is_lang_ctor(cx, qpath, OptionNone)
-        } else {
-            return;
-        };
+    let default_arg_is_none = if let hir::ExprKind::Path(ref qpath) = def_arg.kind {
+        is_lang_ctor(cx, qpath, OptionNone)
+    } else {
+        return;
+    };
 
-        if !default_arg_is_none {
-            // nothing to lint!
-            return;
-        }
+    if !default_arg_is_none {
+        // nothing to lint!
+        return;
+    }
 
-        let f_arg_is_some = if let hir::ExprKind::Path(ref qpath) = map_arg.kind {
-            is_lang_ctor(cx, qpath, OptionSome)
-        } else {
-            false
-        };
+    let f_arg_is_some = if let hir::ExprKind::Path(ref qpath) = map_arg.kind {
+        is_lang_ctor(cx, qpath, OptionSome)
+    } else {
+        false
+    };
+
+    if is_option {
+        let self_snippet = snippet(cx, recv.span, "..");
+        if_chain! {
+        if let hir::ExprKind::Closure(_, _, id, span, _) = map_arg.kind;
+            let arg_snippet = snippet(cx, span, "..");
+            let body = cx.tcx.hir().body(id);
+                if let Some((func, arg_char)) = reduce_unit_expression(cx, &body.value);
+                if arg_char.len() == 1;
+                if let hir::ExprKind::Path(ref qpath) = func.kind;
+                if let Some(segment) = single_segment_path(qpath);
+                if segment.ident.name == sym::Some;
+                then {
+                    let func_snippet = snippet(cx, arg_char[0].span, "..");
+                    let msg = "called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling \
+                       `map(..)` instead";
+                    return span_lint_and_sugg(
+                        cx,
+                        OPTION_MAP_OR_NONE,
+                        expr.span,
+                        msg,
+                        "try using `map` instead",
+                        format!("{0}.map({1} {2})", self_snippet, arg_snippet,func_snippet),
+                        Applicability::MachineApplicable,
+                    );
+                }
 
-        if is_option {
-            let self_snippet = snippet(cx, recv.span, "..");
-            let func_snippet = snippet(cx, map_arg.span, "..");
-            let msg = "called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling \
-                       `and_then(..)` instead";
-            (
-                OPTION_MAP_OR_NONE,
-                msg,
-                "try using `and_then` instead",
-                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, recv.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,
-    );
+        let func_snippet = snippet(cx, map_arg.span, "..");
+        let msg = "called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling \
+                       `and_then(..)` instead";
+        return span_lint_and_sugg(
+            cx,
+            OPTION_MAP_OR_NONE,
+            expr.span,
+            msg,
+            "try using `and_then` instead",
+            format!("{0}.and_then({1})", self_snippet, func_snippet),
+            Applicability::MachineApplicable,
+        );
+    } 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, recv.span, "..");
+        return span_lint_and_sugg(
+            cx,
+            RESULT_MAP_OR_INTO_OPTION,
+            expr.span,
+            msg,
+            "try using `ok` instead",
+            format!("{0}.ok()", self_snippet),
+            Applicability::MachineApplicable,
+        );
+    }
 }
diff --git a/tests/ui/option_map_or_none.fixed b/tests/ui/option_map_or_none.fixed
index d80c3c7c1b7..177054f763b 100644
--- a/tests/ui/option_map_or_none.fixed
+++ b/tests/ui/option_map_or_none.fixed
@@ -4,13 +4,19 @@
 
 fn main() {
     let opt = Some(1);
+    let bar = |_| Some(1);
 
     // Check `OPTION_MAP_OR_NONE`.
     // Single line case.
-    let _ = opt.and_then(|x| Some(x + 1));
+    let _: Option<i32> = opt.map(|x| x + 1);
     // Multi-line case.
     #[rustfmt::skip]
-    let _ = opt.and_then(|x| {
-                        Some(x + 1)
-                       });
+    let _: Option<i32> = opt.map(|x| x + 1);
+    // function returning `Option`
+    let _: Option<i32> = opt.and_then(bar);
+    let _: Option<i32> = opt.and_then(|x| {
+        let offset = 0;
+        let height = x;
+        Some(offset + height)
+    });
 }
diff --git a/tests/ui/option_map_or_none.rs b/tests/ui/option_map_or_none.rs
index 629842419e5..6908546d325 100644
--- a/tests/ui/option_map_or_none.rs
+++ b/tests/ui/option_map_or_none.rs
@@ -4,13 +4,21 @@
 
 fn main() {
     let opt = Some(1);
+    let bar = |_| Some(1);
 
     // Check `OPTION_MAP_OR_NONE`.
     // Single line case.
-    let _ = opt.map_or(None, |x| Some(x + 1));
+    let _: Option<i32> = opt.map_or(None, |x| Some(x + 1));
     // Multi-line case.
     #[rustfmt::skip]
-    let _ = opt.map_or(None, |x| {
+    let _: Option<i32> = opt.map_or(None, |x| {
                         Some(x + 1)
                        });
+    // function returning `Option`
+    let _: Option<i32> = opt.map_or(None, bar);
+    let _: Option<i32> = opt.map_or(None, |x| {
+        let offset = 0;
+        let height = x;
+        Some(offset + height)
+    });
 }
diff --git a/tests/ui/option_map_or_none.stderr b/tests/ui/option_map_or_none.stderr
index 27d68b85e6f..11bdd887b6d 100644
--- a/tests/ui/option_map_or_none.stderr
+++ b/tests/ui/option_map_or_none.stderr
@@ -1,26 +1,45 @@
-error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead
-  --> $DIR/option_map_or_none.rs:10:13
+error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `map(..)` instead
+  --> $DIR/option_map_or_none.rs:11:26
    |
-LL |     let _ = opt.map_or(None, |x| Some(x + 1));
-   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `opt.and_then(|x| Some(x + 1))`
+LL |     let _: Option<i32> = opt.map_or(None, |x| Some(x + 1));
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `map` instead: `opt.map(|x| x + 1)`
    |
    = note: `-D clippy::option-map-or-none` implied by `-D warnings`
 
-error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead
-  --> $DIR/option_map_or_none.rs:13:13
+error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `map(..)` instead
+  --> $DIR/option_map_or_none.rs:14:26
    |
-LL |       let _ = opt.map_or(None, |x| {
-   |  _____________^
+LL |       let _: Option<i32> = opt.map_or(None, |x| {
+   |  __________________________^
 LL | |                         Some(x + 1)
 LL | |                        });
-   | |_________________________^
+   | |_________________________^ help: try using `map` instead: `opt.map(|x| x + 1)`
+
+error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead
+  --> $DIR/option_map_or_none.rs:18:26
+   |
+LL |     let _: Option<i32> = opt.map_or(None, bar);
+   |                          ^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `opt.and_then(bar)`
+
+error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead
+  --> $DIR/option_map_or_none.rs:19:26
+   |
+LL |       let _: Option<i32> = opt.map_or(None, |x| {
+   |  __________________________^
+LL | |         let offset = 0;
+LL | |         let height = x;
+LL | |         Some(offset + height)
+LL | |     });
+   | |______^
    |
 help: try using `and_then` instead
    |
-LL ~     let _ = opt.and_then(|x| {
-LL +                         Some(x + 1)
-LL ~                        });
+LL ~     let _: Option<i32> = opt.and_then(|x| {
+LL +         let offset = 0;
+LL +         let height = x;
+LL +         Some(offset + height)
+LL ~     });
    |
 
-error: aborting due to 2 previous errors
+error: aborting due to 4 previous errors