Merge pull request #2112 from topecongiro/issue-2109

Add a suggestion to replace `map(f).unwrap_or(None)` with `and_then(f)`.
rust-lang · Oct 8, 2017 · a54baad · a54baad
2 parents 45e5f28 + 7f4b583
commit a54baad
Show file tree

Hide file tree

Showing 3 changed files with 291 additions and 244 deletions.
diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs
@@ -1150,29 +1150,37 @@ fn lint_ok_expect(cx: &LateContext, expr: &hir::Expr, ok_args: &[hir::Expr]) {
 fn lint_map_unwrap_or(cx: &LateContext, expr: &hir::Expr, map_args: &[hir::Expr], unwrap_args: &[hir::Expr]) {
     // lint if the caller of `map()` is an `Option`
     if match_type(cx, cx.tables.expr_ty(&map_args[0]), &paths::OPTION) {
-        // lint message
-        let msg = "called `map(f).unwrap_or(a)` on an Option value. This can be done more directly by calling \
-                   `map_or(a, f)` instead";
         // get snippets for args to map() and unwrap_or()
         let map_snippet = snippet(cx, map_args[1].span, "..");
         let unwrap_snippet = snippet(cx, unwrap_args[1].span, "..");
+        // lint message
+        // comparing the snippet from source to raw text ("None") below is safe
+        // because we already have checked the type.
+        let arg = if unwrap_snippet == "None" { "None" } else { "a" };
+        let suggest = if unwrap_snippet == "None" { "and_then(f)" } else { "map_or(a, f)" };
+        let msg = &format!(
+            "called `map(f).unwrap_or({})` on an Option value. \
+             This can be done more directly by calling `{}` instead",
+            arg,
+            suggest
+        );
         // lint, with note if neither arg is > 1 line and both map() and
         // unwrap_or() have the same span
         let multiline = map_snippet.lines().count() > 1 || unwrap_snippet.lines().count() > 1;
         let same_span = map_args[1].span.ctxt() == unwrap_args[1].span.ctxt();
         if same_span && !multiline {
-            span_note_and_lint(
-                cx,
-                OPTION_MAP_UNWRAP_OR,
-                expr.span,
-                msg,
-                expr.span,
-                &format!(
-                    "replace `map({0}).unwrap_or({1})` with `map_or({1}, {0})`",
-                    map_snippet,
-                    unwrap_snippet
-                ),
+            let suggest = if unwrap_snippet == "None" {
+                format!("and_then({})", map_snippet)
+            } else {
+                format!("map_or({}, {})", unwrap_snippet, map_snippet)
+            };
+            let note = format!(
+                "replace `map({}).unwrap_or({})` with `{}`",
+                map_snippet,
+                unwrap_snippet,
+                suggest
             );
+            span_note_and_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, expr.span, &note);
         } else if same_span && multiline {
             span_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg);
         };

diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs
@@ -108,6 +108,16 @@ fn option_methods() {
                .unwrap_or({
                     0
                 });
+    // single line `map(f).unwrap_or(None)` case
+    let _ = opt.map(|x| Some(x + 1)).unwrap_or(None);
+    // multiline `map(f).unwrap_or(None)` cases
+    let _ = opt.map(|x| {
+        Some(x + 1)
+    }
+    ).unwrap_or(None);
+    let _ = opt
+        .map(|x| Some(x + 1))
+        .unwrap_or(None);
     // macro case
     let _ = opt_map!(opt, |x| x + 1).unwrap_or(0); // should not lint