Merge option_map_unwrap_or, option_map_unwrap_or_else and result_map_unwrap_or_else lints into map_unwrap lint

Author: ThibsG

CHANGELOG.md

@@ -1430,6 +1430,7 @@ Released 2018-09-13
 [`map_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_clone
 [`map_entry`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_entry
 [`map_flatten`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten
+[`map_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_unwrap
 [`match_as_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_as_ref
 [`match_bool`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_bool
 [`match_on_vec_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_on_vec_items
@@ -1499,8 +1500,6 @@ Released 2018-09-13
 [`option_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_expect_used
 [`option_map_or_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_or_none
 [`option_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unit_fn
-[`option_map_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unwrap_or
-[`option_map_unwrap_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unwrap_or_else
 [`option_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_option
 [`option_unwrap_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_unwrap_used
 [`or_fun_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call
@@ -1542,7 +1541,6 @@ Released 2018-09-13
 [`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
 [`reversed_empty_ranges`]: https://rust-lang.github.io/rust-clippy/master/index.html#reversed_empty_ranges
 [`same_functions_in_if_condition`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_functions_in_if_condition

clippy_lints/src/lib.rs

@@ -673,19 +673,17 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &methods::ITER_SKIP_NEXT,
         &methods::MANUAL_SATURATING_ARITHMETIC,
         &methods::MAP_FLATTEN,
+        &methods::MAP_UNWRAP,
         &methods::NEW_RET_NO_SELF,
         &methods::OK_EXPECT,
         &methods::OPTION_AND_THEN_SOME,
         &methods::OPTION_AS_REF_DEREF,
         &methods::OPTION_EXPECT_USED,
         &methods::OPTION_MAP_OR_NONE,
-        &methods::OPTION_MAP_UNWRAP_OR,
-        &methods::OPTION_MAP_UNWRAP_OR_ELSE,
         &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,
         &methods::SHOULD_IMPLEMENT_TRAIT,
@@ -1152,9 +1150,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::FIND_MAP),
         LintId::of(&methods::INEFFICIENT_TO_STRING),
         LintId::of(&methods::MAP_FLATTEN),
-        LintId::of(&methods::OPTION_MAP_UNWRAP_OR),
-        LintId::of(&methods::OPTION_MAP_UNWRAP_OR_ELSE),
-        LintId::of(&methods::RESULT_MAP_UNWRAP_OR_ELSE),
+        LintId::of(&methods::MAP_UNWRAP),
         LintId::of(&misc::USED_UNDERSCORE_BINDING),
         LintId::of(&misc_early::UNSEPARATED_LITERAL_SUFFIX),
         LintId::of(&mut_mut::MUT_MUT),

clippy_lints/src/methods/mod.rs

@@ -257,59 +257,40 @@ declare_clippy_lint! {
 }
 
 declare_clippy_lint! {
-    /// **What it does:** Checks for usage of `_.map(_).unwrap_or(_)`.
+    /// **What it does:** Checks for usage of `option.map(_).unwrap_or(_)` or `option.map(_).unwrap_or_else(_)` or
+    /// `result.map(_).unwrap_or_else(_)`.
     ///
-    /// **Why is this bad?** Readability, this can be written more concisely as
-    /// `_.map_or(_, _)`.
+    /// **Why is this bad?** Readability, these can be written more concisely (resp.) as
+    /// `option.map_or(_, _)`, `option.map_or_else(_, _)` and `result.map_or_else(_, _)`.
     ///
     /// **Known problems:** The order of the arguments is not in execution order
     ///
-    /// **Example:**
+    /// **Examples:**
     /// ```rust
     /// # let x = Some(1);
-    /// x.map(|a| a + 1).unwrap_or(0);
-    /// ```
-    pub OPTION_MAP_UNWRAP_OR,
-    pedantic,
-    "using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as `map_or(a, f)`"
-}
-
-declare_clippy_lint! {
-    /// **What it does:** Checks for usage of `_.map(_).unwrap_or_else(_)`.
-    ///
-    /// **Why is this bad?** Readability, this can be written more concisely as
-    /// `_.map_or_else(_, _)`.
     ///
-    /// **Known problems:** The order of the arguments is not in execution order.
+    /// // Bad
+    /// x.map(|a| a + 1).unwrap_or(0);
     ///
-    /// **Example:**
-    /// ```rust
-    /// # let x = Some(1);
-    /// # fn some_function() -> usize { 1 }
-    /// x.map(|a| a + 1).unwrap_or_else(some_function);
+    /// // Good
+    /// x.map_or(0, |a| a + 1);
     /// ```
-    pub OPTION_MAP_UNWRAP_OR_ELSE,
-    pedantic,
-    "using `Option.map(f).unwrap_or_else(g)`, which is more succinctly expressed as `map_or_else(g, f)`"
-}
-
-declare_clippy_lint! {
-    /// **What it does:** Checks for usage of `result.map(_).unwrap_or_else(_)`.
     ///
-    /// **Why is this bad?** Readability, this can be written more concisely as
-    /// `result.map_or_else(_, _)`.
-    ///
-    /// **Known problems:** None.
+    /// // or
     ///
-    /// **Example:**
     /// ```rust
     /// # let x: Result<usize, ()> = Ok(1);
     /// # fn some_function(foo: ()) -> usize { 1 }
+    ///
+    /// // Bad
     /// x.map(|a| a + 1).unwrap_or_else(some_function);
+    ///
+    /// // Good
+    /// x.map_or_else(some_function, |a| a + 1);
     /// ```
-    pub RESULT_MAP_UNWRAP_OR_ELSE,
+    pub MAP_UNWRAP,
     pedantic,
-    "using `Result.map(f).unwrap_or_else(g)`, which is more succinctly expressed as `.map_or_else(g, f)`"
+    "using `.map(f).unwrap_or(a)` or `.map(f).unwrap_or_else(func)`, which are more succinctly expressed as `map_or(a, f)` or `map_or_else(a, f)`"
 }
 
 declare_clippy_lint! {
@@ -1294,9 +1275,7 @@ declare_lint_pass!(Methods => [
     WRONG_SELF_CONVENTION,
     WRONG_PUB_SELF_CONVENTION,
     OK_EXPECT,
-    OPTION_MAP_UNWRAP_OR,
-    OPTION_MAP_UNWRAP_OR_ELSE,
-    RESULT_MAP_UNWRAP_OR_ELSE,
+    MAP_UNWRAP,
     RESULT_MAP_OR_INTO_OPTION,
     OPTION_MAP_OR_NONE,
     OPTION_AND_THEN_SOME,
@@ -1503,9 +1482,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
                             cx,
                             lint,
                             first_arg.pat.span,
-                            &format!(
-                               "methods called `{}` usually take {}; consider choosing a less \
-                                 ambiguous name",
+                            &format!("methods called `{}` usually take {}; consider choosing a less ambiguous name",
                                 conv,
                                 &self_kinds
                                     .iter()
@@ -1678,7 +1655,7 @@ fn lint_or_fun_call<'a, 'tcx>(
             let self_ty = cx.tables.expr_ty(self_expr);
 
             if let Some(&(_, fn_has_arguments, poss, suffix)) =
-                   know_types.iter().find(|&&i| match_type(cx, self_ty, i.0));
+                know_types.iter().find(|&&i| match_type(cx, self_ty, i.0));
 
             if poss.contains(&name);
 
@@ -1931,7 +1908,7 @@ fn lint_clone_on_copy(cx: &LateContext<'_, '_>, expr: &hir::Expr<'_>, arg: &hir:
                 CLONE_DOUBLE_REF,
                 expr.span,
                 "using `clone` on a double-reference; \
-                 this will copy the reference instead of cloning the inner type",
+                this will copy the reference instead of cloning the inner type",
                 |diag| {
                     if let Some(snip) = sugg::Sugg::hir_opt(cx, arg) {
                         let mut ty = innermost;
@@ -2121,7 +2098,7 @@ fn lint_iter_cloned_collect<'a, 'tcx>(
                 ITER_CLONED_COLLECT,
                 to_replace,
                 "called `iter().cloned().collect()` on a slice to create a `Vec`. Calling `to_vec()` is both faster and \
-                 more readable",
+                more readable",
                 "try",
                 ".to_vec()".to_string(),
                 Applicability::MachineApplicable,
@@ -2436,7 +2413,7 @@ fn lint_unwrap(cx: &LateContext<'_, '_>, expr: &hir::Expr<'_>, unwrap_args: &[hi
             None,
             &format!(
                 "if you don't want to handle the `{}` case gracefully, consider \
-                 using `expect()` to provide a better panic message",
+                using `expect()` to provide a better panic message",
                 none_value,
             ),
         );
@@ -2494,7 +2471,7 @@ fn lint_map_flatten<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr<
     // lint if caller of `.map().flatten()` is an Iterator
     if match_trait_method(cx, expr, &paths::ITERATOR) {
         let msg = "called `map(..).flatten()` on an `Iterator`. \
-                   This is more succinctly expressed by calling `.flat_map(..)`";
+                    This is more succinctly expressed by calling `.flat_map(..)`";
         let self_snippet = snippet(cx, map_args[0].span, "..");
         let func_snippet = snippet(cx, map_args[1].span, "..");
         let hint = format!("{0}.flat_map({1})", self_snippet, func_snippet);
@@ -2555,10 +2532,10 @@ fn lint_map_unwrap_or_else<'a, 'tcx>(
         // lint message
         let msg = if is_option {
             "called `map(f).unwrap_or_else(g)` on an `Option` value. This can be done more directly by calling \
-             `map_or_else(g, f)` instead"
+            `map_or_else(g, f)` instead"
         } else {
             "called `map(f).unwrap_or_else(g)` on a `Result` value. This can be done more directly by calling \
-             `.map_or_else(g, f)` instead"
+            `.map_or_else(g, f)` instead"
         };
         // get snippets for args to map() and unwrap_or_else()
         let map_snippet = snippet(cx, map_args[1].span, "..");
@@ -2570,11 +2547,7 @@ fn lint_map_unwrap_or_else<'a, 'tcx>(
         if same_span && !multiline {
             span_lint_and_note(
                 cx,
-                if is_option {
-                    OPTION_MAP_UNWRAP_OR_ELSE
-                } else {
-                    RESULT_MAP_UNWRAP_OR_ELSE
-                },
+                MAP_UNWRAP,
                 expr.span,
                 msg,
                 None,
@@ -2584,16 +2557,7 @@ fn lint_map_unwrap_or_else<'a, 'tcx>(
                 ),
             );
         } else if same_span && multiline {
-            span_lint(
-                cx,
-                if is_option {
-                    OPTION_MAP_UNWRAP_OR_ELSE
-                } else {
-                    RESULT_MAP_UNWRAP_OR_ELSE
-                },
-                expr.span,
-                msg,
-            );
+            span_lint(cx, MAP_UNWRAP, expr.span, msg);
         };
     }
 }

clippy_lints/src/methods/option_map_unwrap_or.rs

@@ -9,7 +9,7 @@ use rustc_middle::hir::map::Map;
 use rustc_span::source_map::Span;
 use rustc_span::symbol::Symbol;
 
-use super::OPTION_MAP_UNWRAP_OR;
+use super::MAP_UNWRAP;
 
 /// lint use of `map().unwrap_or()` for `Option`s
 pub(super) fn lint<'a, 'tcx>(
@@ -62,11 +62,11 @@ pub(super) fn lint<'a, 'tcx>(
         };
         let msg = &format!(
             "called `map(f).unwrap_or({})` on an `Option` value. \
-             This can be done more directly by calling `{}` instead",
+            This can be done more directly by calling `{}` instead",
             arg, suggest
         );
 
-        span_lint_and_then(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, |diag| {
+        span_lint_and_then(cx, MAP_UNWRAP, expr.span, msg, |diag| {
             let map_arg_span = map_args[1].span;
 
             let mut suggestion = vec![

src/lintlist/mod.rs

@@ -1137,6 +1137,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "methods",
     },
+    Lint {
+        name: "map_unwrap",
+        group: "pedantic",
+        desc: "using `.map(f).unwrap_or(a)` or `.map(f).unwrap_or_else(func)`, which are more succinctly expressed as `map_or(a, f)` or `map_or_else(a, f)`",
+        deprecation: None,
+        module: "methods",
+    },
     Lint {
         name: "match_as_ref",
         group: "complexity",
@@ -1613,20 +1620,6 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "map_unit_fn",
     },
-    Lint {
-        name: "option_map_unwrap_or",
-        group: "pedantic",
-        desc: "using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as `map_or(a, f)`",
-        deprecation: None,
-        module: "methods",
-    },
-    Lint {
-        name: "option_map_unwrap_or_else",
-        group: "pedantic",
-        desc: "using `Option.map(f).unwrap_or_else(g)`, which is more succinctly expressed as `map_or_else(g, f)`",
-        deprecation: None,
-        module: "methods",
-    },
     Lint {
         name: "option_option",
         group: "pedantic",
@@ -1900,13 +1893,6 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "map_unit_fn",
     },
-    Lint {
-        name: "result_map_unwrap_or_else",
-        group: "pedantic",
-        desc: "using `Result.map(f).unwrap_or_else(g)`, which is more succinctly expressed as `.map_or_else(g, f)`",
-        deprecation: None,
-        module: "methods",
-    },
     Lint {
         name: "result_unwrap_used",
         group: "restriction",

tests/ui/option_map_unwrap_or.rs renamed to tests/ui/map_unwrap.rs

@@ -1,21 +1,18 @@
 // FIXME: Add "run-rustfix" once it's supported for multipart suggestions
 // aux-build:option_helpers.rs
 
-#![warn(clippy::option_map_unwrap_or, clippy::option_map_unwrap_or_else)]
+#![warn(clippy::map_unwrap)]
 
 #[macro_use]
 extern crate option_helpers;
 
 use std::collections::HashMap;
 
-/// Checks implementation of the following lints:
-/// * `OPTION_MAP_UNWRAP_OR`
-/// * `OPTION_MAP_UNWRAP_OR_ELSE`
 #[rustfmt::skip]
 fn option_methods() {
     let opt = Some(1);
 
-    // Check `OPTION_MAP_UNWRAP_OR`.
+    // Check for `option.map(_).unwrap_or(_)` use.
     // Single line case.
     let _ = opt.map(|x| x + 1)
         // Should lint even though this call is on a separate line.
@@ -49,7 +46,7 @@ fn option_methods() {
     let id: String = "identifier".to_string();
     let _ = Some("prefix").map(|p| format!("{}.", p)).unwrap_or(id);
 
-    // Check OPTION_MAP_UNWRAP_OR_ELSE
+    // Check for `option.map(_).unwrap_or_else(_)` use.
     // single line case
     let _ = opt.map(|x| x + 1)
         // Should lint even though this call is on a separate line.
@@ -83,6 +80,20 @@ fn option_methods() {
     }
 }
 
+fn result_methods() {
+    let res: Result<i32, ()> = Ok(1);
+
+    // Check for `result.map(_).unwrap_or_else(_)` use.
+    // single line case
+    let _ = res.map(|x| x + 1).unwrap_or_else(|e| 0); // should lint even though this call is on a separate line
+                                                      // multi line cases
+    let _ = res.map(|x| x + 1).unwrap_or_else(|e| 0);
+    let _ = res.map(|x| x + 1).unwrap_or_else(|e| 0);
+    // macro case
+    let _ = opt_map!(res, |x| x + 1).unwrap_or_else(|e| 0); // should not lint
+}
+
 fn main() {
     option_methods();
+    result_methods();
 }