Also lint Result.map for unit returns

CHANGELOG.md

@@ -705,7 +705,7 @@ All notable changes to this project will be documented in this file.
 [`nonsensical_open_options`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#nonsensical_open_options
 [`not_unsafe_ptr_arg_deref`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref
 [`ok_expect`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#ok_expect
-[`option_map_unit_fn`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#option_map_unit_fn
+[`map_unit_fn`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#map_unit_fn
 [`op_ref`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#op_ref
 [`option_map_or_none`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#option_map_or_none
 [`option_map_unwrap_or`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#option_map_unwrap_or

clippy_lints/src/lib.rs

@@ -444,6 +444,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         infinite_iter::MAYBE_INFINITE_ITER,
         items_after_statements::ITEMS_AFTER_STATEMENTS,
         map_unit_fn::OPTION_MAP_UNIT_FN,
+        map_unit_fn::RESULT_MAP_UNIT_FN,
         matches::SINGLE_MATCH_ELSE,
         methods::FILTER_MAP,
         methods::OPTION_MAP_UNWRAP_OR,

clippy_lints/src/map_unit_fn.rs

@@ -41,10 +41,43 @@ declare_clippy_lint! {
     "using `Option.map(f)`, where f is a function or closure that returns ()"
 }
 
+/// **What it does:** Checks for usage of `Result.map(f)` where f is a function
+/// or closure that returns the unit type.
+///
+/// **Why is this bad?** Readability, this can be written more clearly with
+/// an if statement
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+///
+/// ```rust
+/// let x: Result<&str, &str> = do_stuff();
+/// x.map(log_err_msg);
+/// x.map(|msg| log_err_msg(format_msg(msg)))
+/// ```
+///
+/// The correct use would be:
+///
+/// ```rust
+/// let x: Result<&str, &str> = do_stuff();
+/// if let Ok(msg) = x {
+///     log_err_msg(msg)
+/// }
+/// if let Ok(msg) = x {
+///     log_err_msg(format_msg(msg))
+/// }
+/// ```
+declare_clippy_lint! {
+    pub RESULT_MAP_UNIT_FN,
+    complexity,
+    "using `Result.map(f)`, where f is a function or closure that returns ()"
+}
+
 
 impl LintPass for Pass {
     fn get_lints(&self) -> LintArray {
-        lint_array!(OPTION_MAP_UNIT_FN)
+        lint_array!(OPTION_MAP_UNIT_FN, RESULT_MAP_UNIT_FN)
     }
 }
 
@@ -147,28 +180,40 @@ fn let_binding_name(cx: &LateContext, var_arg: &hir::Expr) -> String {
     }
 }
 
+fn suggestion_msg(function_type: &str, map_type: &str) -> String {
+    format!(
+        "called `map(f)` on an {0} value where `f` is a unit {1}",
+        map_type,
+        function_type
+    )
+}
+
 fn lint_map_unit_fn(cx: &LateContext, stmt: &hir::Stmt, expr: &hir::Expr, map_args: &[hir::Expr]) {
     let var_arg = &map_args[0];
     let fn_arg = &map_args[1];
 
-    if !match_type(cx, cx.tables.expr_ty(var_arg), &paths::OPTION) {
+    let (map_type, variant, lint) =
-        return;
+        if match_type(cx, cx.tables.expr_ty(var_arg), &paths::OPTION) {
-    }
+            ("Option", "Some", OPTION_MAP_UNIT_FN)
+        } else if match_type(cx, cx.tables.expr_ty(var_arg), &paths::RESULT) {
+            ("Result", "Ok", RESULT_MAP_UNIT_FN)
+        } else {
+            return
+        };
 
     if is_unit_function(cx, fn_arg) {
-        let msg = "called `map(f)` on an Option value where `f` is a unit function";
+        let msg = suggestion_msg("function", map_type);
-        let suggestion = format!("if let Some({0}) = {1} {{ {2}(...) }}",
+        let suggestion = format!("if let {0}({1}) = {2} {{ {3}(...) }}",
+                                 variant,
                                  let_binding_name(cx, var_arg),
                                  snippet(cx, var_arg.span, "_"),
                                  snippet(cx, fn_arg.span, "_"));
 
-        span_lint_and_then(cx,
+        span_lint_and_then(cx, lint, expr.span, &msg, |db| {
-                           OPTION_MAP_UNIT_FN,
+            db.span_approximate_suggestion(stmt.span, "try this", suggestion);
-                           expr.span,
+        });
-                           msg,
-                           |db| { db.span_approximate_suggestion(stmt.span, "try this", suggestion); });
     } else if let Some((binding, closure_expr)) = unit_closure(cx, fn_arg) {
-        let msg = "called `map(f)` on an Option value where `f` is a unit closure";
+        let msg = suggestion_msg("closure", map_type);
 
         enum Suggestion {
             Full(String),
@@ -177,20 +222,22 @@ fn lint_map_unit_fn(cx: &LateContext, stmt: &hir::Stmt, expr: &hir::Expr, map_ar
 
         let suggestion = if let Some(expr_span) = reduce_unit_expression(cx, closure_expr) {
             Suggestion::Full(
-                format!("if let Some({0}) = {1} {{ {2} }}",
+                format!("if let {0}({1}) = {2} {{ {3} }}",
+                        variant,
                         snippet(cx, binding.pat.span, "_"),
                         snippet(cx, var_arg.span, "_"),
                         snippet(cx, expr_span, "_"))
             )
         } else {
             Suggestion::Approx(
-                format!("if let Some({0}) = {1} {{ ... }}",
+                format!("if let {0}({1}) = {2} {{ ... }}",
+                        variant,
                         snippet(cx, binding.pat.span, "_"),
                         snippet(cx, var_arg.span, "_"))
             )
         };
 
-        span_lint_and_then(cx, OPTION_MAP_UNIT_FN, expr.span, msg, |db| {
+        span_lint_and_then(cx, lint, expr.span, &msg, |db| {
             match suggestion {
                 Suggestion::Full(sugg) => db.span_suggestion(stmt.span, "try this", sugg),
                 Suggestion::Approx(sugg) => db.span_approximate_suggestion(stmt.span, "try this", sugg),

tests/ui/result_map_unit_fn.rs

@@ -0,0 +1,102 @@
+#![warn(result_map_unit_fn)]
+#![allow(unused)]
+
+fn do_nothing<T>(_: T) {}
+
+fn diverge<T>(_: T) -> ! {
+    panic!()
+}
+
+fn plus_one(value: usize) -> usize {
+    value + 1
+}
+
+struct HasResult {
+    field: Result<usize, usize>,
+}
+
+impl HasResult {
+    fn do_result_nothing(self: &Self, value: usize) {}
+
+    fn do_result_plus_one(self: &Self, value: usize) -> usize {
+        value + 1
+    }
+}
+
+fn result_map_unit_fn() {
+    let x = HasResult { field: Ok(10) };
+
+    x.field.map(plus_one);
+    let _ : Result<(), usize> = x.field.map(do_nothing);
+
+    x.field.map(do_nothing);
+
+    x.field.map(do_nothing);
+
+    x.field.map(diverge);
+
+    let captured = 10;
+    if let Ok(value) = x.field { do_nothing(value + captured) };
+    let _ : Result<(), usize> = x.field.map(|value| do_nothing(value + captured));
+
+    x.field.map(|value| x.do_result_nothing(value + captured));
+
+    x.field.map(|value| { x.do_result_plus_one(value + captured); });
+
+
+    x.field.map(|value| do_nothing(value + captured));
+
+    x.field.map(|value| { do_nothing(value + captured) });
+
+    x.field.map(|value| { do_nothing(value + captured); });
+
+    x.field.map(|value| { { do_nothing(value + captured); } });
+
+
+    x.field.map(|value| diverge(value + captured));
+
+    x.field.map(|value| { diverge(value + captured) });
+
+    x.field.map(|value| { diverge(value + captured); });
+
+    x.field.map(|value| { { diverge(value + captured); } });
+
+
+    x.field.map(|value| plus_one(value + captured));
+    x.field.map(|value| { plus_one(value + captured) });
+    x.field.map(|value| { let y = plus_one(value + captured); });
+
+    x.field.map(|value| { plus_one(value + captured); });
+
+    x.field.map(|value| { { plus_one(value + captured); } });
+
+
+    x.field.map(|ref value| { do_nothing(value + captured) });
+
+
+    x.field.map(|value| { do_nothing(value); do_nothing(value) });
+
+    x.field.map(|value| if value > 0 { do_nothing(value); do_nothing(value) });
+
+    // Suggestion for the let block should be `{ ... }` as it's too difficult to build a
+    // proper suggestion for these cases
+    x.field.map(|value| {
+        do_nothing(value);
+        do_nothing(value)
+    });
+    x.field.map(|value| { do_nothing(value); do_nothing(value); });
+
+    // The following should suggest `if let Ok(_X) ...` as it's difficult to generate a proper let variable name for them
+    let res: Result<!, usize> = Ok(42).map(diverge);
+    "12".parse::<i32>().map(diverge);
+
+    let res: Result<(), usize> = Ok(plus_one(1)).map(do_nothing);
+
+    // Should suggest `if let Ok(_y) ...` to not override the existing foo variable
+    let y: Result<usize, usize> = Ok(42);
+    y.map(do_nothing);
+}
+
+fn main() {
+}
+

tests/ui/result_map_unit_fn.stderr

@@ -0,0 +1,194 @@
+error: called `map(f)` on an Result value where `f` is a unit function
+  --> $DIR/result_map_unit_fn.rs:32:5
+   |
+32 |     x.field.map(do_nothing);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(x_field) = x.field { do_nothing(...) }`
+   |
+   = note: `-D result-map-unit-fn` implied by `-D warnings`
+
+error: called `map(f)` on an Result value where `f` is a unit function
+  --> $DIR/result_map_unit_fn.rs:34:5
+   |
+34 |     x.field.map(do_nothing);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(x_field) = x.field { do_nothing(...) }`
+
+error: called `map(f)` on an Result value where `f` is a unit function
+  --> $DIR/result_map_unit_fn.rs:36:5
+   |
+36 |     x.field.map(diverge);
+   |     ^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(x_field) = x.field { diverge(...) }`
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:42:5
+   |
+42 |     x.field.map(|value| x.do_result_nothing(value + captured));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(value) = x.field { x.do_result_nothing(value + captured) }`
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:44:5
+   |
+44 |     x.field.map(|value| { x.do_result_plus_one(value + captured); });
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(value) = x.field { x.do_result_plus_one(value + captured); }`
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:47:5
+   |
+47 |     x.field.map(|value| do_nothing(value + captured));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(value) = x.field { do_nothing(value + captured) }`
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:49:5
+   |
+49 |     x.field.map(|value| { do_nothing(value + captured) });
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(value) = x.field { do_nothing(value + captured) }`
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:51:5
+   |
+51 |     x.field.map(|value| { do_nothing(value + captured); });
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(value) = x.field { do_nothing(value + captured); }`
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:53:5
+   |
+53 |     x.field.map(|value| { { do_nothing(value + captured); } });
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(value) = x.field { do_nothing(value + captured); }`
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:56:5
+   |
+56 |     x.field.map(|value| diverge(value + captured));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(value) = x.field { diverge(value + captured) }`
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:58:5
+   |
+58 |     x.field.map(|value| { diverge(value + captured) });
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(value) = x.field { diverge(value + captured) }`
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:60:5
+   |
+60 |     x.field.map(|value| { diverge(value + captured); });
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(value) = x.field { diverge(value + captured); }`
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:62:5
+   |
+62 |     x.field.map(|value| { { diverge(value + captured); } });
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(value) = x.field { diverge(value + captured); }`
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:67:5
+   |
+67 |     x.field.map(|value| { let y = plus_one(value + captured); });
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(value) = x.field { let y = plus_one(value + captured); }`
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:69:5
+   |
+69 |     x.field.map(|value| { plus_one(value + captured); });
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(value) = x.field { plus_one(value + captured); }`
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:71:5
+   |
+71 |     x.field.map(|value| { { plus_one(value + captured); } });
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(value) = x.field { plus_one(value + captured); }`
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:74:5
+   |
+74 |     x.field.map(|ref value| { do_nothing(value + captured) });
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(ref value) = x.field { do_nothing(value + captured) }`
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:77:5
+   |
+77 |     x.field.map(|value| { do_nothing(value); do_nothing(value) });
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(value) = x.field { ... }`
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:79:5
+   |
+79 |     x.field.map(|value| if value > 0 { do_nothing(value); do_nothing(value) });
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(value) = x.field { ... }`
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:83:5
+   |
+83 |        x.field.map(|value| {
+   |   _____^
+   |  |_____|
+   | ||
+84 | ||         do_nothing(value);
+85 | ||         do_nothing(value)
+86 | ||     });
+   | ||______^- help: try this: `if let Ok(value) = x.field { ... }`
+   | |_______|
+   | 
+
+error: called `map(f)` on an Result value where `f` is a unit closure
+  --> $DIR/result_map_unit_fn.rs:87:5
+   |
+87 |     x.field.map(|value| { do_nothing(value); do_nothing(value); });
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(value) = x.field { ... }`
+
+error: called `map(f)` on an Result value where `f` is a unit function
+  --> $DIR/result_map_unit_fn.rs:91:5
+   |
+91 |     "12".parse::<i32>().map(diverge);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(_) = "12".parse::<i32>() { diverge(...) }`
+
+error: called `map(f)` on an Result value where `f` is a unit function
+  --> $DIR/result_map_unit_fn.rs:97:5
+   |
+97 |     y.map(do_nothing);
+   |     ^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Ok(_y) = y { do_nothing(...) }`
+
+error: aborting due to 23 previous errors
+