Add lint for replacing `.map().collect()` with `.try_for_each()`

7 files changed, 118 insertions, 0 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 25f3b5da198..287cf6bb725 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1802,6 +1802,7 @@ Released 2018-09-13
 [`manual_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_unwrap_or
 [`many_single_char_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#many_single_char_names
 [`map_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_clone
+[`map_collect_result_unit`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_collect_result_unit
 [`map_entry`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_entry
 [`map_err_ignore`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_err_ignore
 [`map_flatten`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 3be8bc0e36d..138d1ab9b50 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -697,6 +697,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &methods::ITER_NTH_ZERO,
         &methods::ITER_SKIP_NEXT,
         &methods::MANUAL_SATURATING_ARITHMETIC,
+        &methods::MAP_COLLECT_RESULT_UNIT,
         &methods::MAP_FLATTEN,
         &methods::MAP_UNWRAP_OR,
         &methods::NEW_RET_NO_SELF,
@@ -1420,6 +1421,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::ITER_NTH_ZERO),
         LintId::of(&methods::ITER_SKIP_NEXT),
         LintId::of(&methods::MANUAL_SATURATING_ARITHMETIC),
+        LintId::of(&methods::MAP_COLLECT_RESULT_UNIT),
         LintId::of(&methods::NEW_RET_NO_SELF),
         LintId::of(&methods::OK_EXPECT),
         LintId::of(&methods::OPTION_AS_REF_DEREF),
@@ -1615,6 +1617,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::ITER_NTH_ZERO),
         LintId::of(&methods::ITER_SKIP_NEXT),
         LintId::of(&methods::MANUAL_SATURATING_ARITHMETIC),
+        LintId::of(&methods::MAP_COLLECT_RESULT_UNIT),
         LintId::of(&methods::NEW_RET_NO_SELF),
         LintId::of(&methods::OK_EXPECT),
         LintId::of(&methods::OPTION_MAP_OR_NONE),
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
index c0824bacbc7..5f9bed90845 100644
--- a/clippy_lints/src/methods/mod.rs
+++ b/clippy_lints/src/methods/mod.rs
@@ -1383,6 +1383,27 @@ declare_clippy_lint! {
     "using unnecessary lazy evaluation, which can be replaced with simpler eager evaluation"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for usage of `_.map(_).collect::<Result<(),_>()`.
+    ///
+    /// **Why is this bad?** Using `try_for_each` instead is more readable and idiomatic.
+    ///
+    /// **Known problems:** None
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// (0..3).map(|t| Err(t)).collect::<Result<(), _>>();
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// (0..3).try_for_each(|t| Err(t));
+    /// ```
+    pub MAP_COLLECT_RESULT_UNIT,
+    style,
+    "using `.map(_).collect::<Result<(),_>()`, which can be replaced with `try_for_each`"
+}
+
 declare_lint_pass!(Methods => [
     UNWRAP_USED,
     EXPECT_USED,
@@ -1433,6 +1454,7 @@ declare_lint_pass!(Methods => [
     FILETYPE_IS_FILE,
     OPTION_AS_REF_DEREF,
     UNNECESSARY_LAZY_EVALUATIONS,
+    MAP_COLLECT_RESULT_UNIT,
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for Methods {
@@ -1515,6 +1537,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
             ["unwrap_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "unwrap_or"),
             ["get_or_insert_with", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "get_or_insert"),
             ["ok_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "ok_or"),
+            ["collect", "map"] => lint_map_collect(cx, expr, arg_lists[1], arg_lists[0]),
             _ => {},
         }
 
@@ -3501,6 +3524,42 @@ fn lint_option_as_ref_deref<'tcx>(
     }
 }
 
+fn lint_map_collect(
+    cx: &LateContext<'_>,
+    expr: &hir::Expr<'_>,
+    map_args: &[hir::Expr<'_>],
+    collect_args: &[hir::Expr<'_>],
+) {
+    if_chain! {
+        // called on Iterator
+        if let [map_expr] = collect_args;
+        if match_trait_method(cx, map_expr, &paths::ITERATOR);
+        // return of collect `Result<(),_>`
+        let collect_ret_ty = cx.typeck_results().expr_ty(expr);
+        if is_type_diagnostic_item(cx, collect_ret_ty, sym!(result_type));
+        if let ty::Adt(_, substs) = collect_ret_ty.kind();
+        if let Some(result_t) = substs.types().next();
+        if result_t.is_unit();
+        // get parts for snippet
+        if let [iter, map_fn] = map_args;
+        then {
+            span_lint_and_sugg(
+                cx,
+                MAP_COLLECT_RESULT_UNIT,
+                expr.span,
+                "`.map().collect()` can be replaced with `.try_for_each()`",
+                "try this",
+                format!(
+                    "{}.try_for_each({})",
+                    snippet(cx, iter.span, ".."),
+                    snippet(cx, map_fn.span, "..")
+                ),
+                Applicability::MachineApplicable,
+            );
+        }
+    }
+}
+
 /// Given a `Result<T, E>` type, return its error type (`E`).
 fn get_error_type<'a>(cx: &LateContext<'_>, ty: Ty<'a>) -> Option<Ty<'a>> {
     match ty.kind() {
diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs
index 6272ce45efb..d9b3e5c17f4 100644
--- a/src/lintlist/mod.rs
+++ b/src/lintlist/mod.rs
@@ -1223,6 +1223,13 @@ vec![
         module: "map_clone",
     },
     Lint {
+        name: "map_collect_result_unit",
+        group: "style",
+        desc: "using `.map(_).collect::<Result<(),_>()`, which can be replaced with `try_for_each`",
+        deprecation: None,
+        module: "methods",
+    },
+    Lint {
         name: "map_entry",
         group: "perf",
         desc: "use of `contains_key` followed by `insert` on a `HashMap` or `BTreeMap`",
diff --git a/tests/ui/map_collect_result_unit.fixed b/tests/ui/map_collect_result_unit.fixed
new file mode 100644
index 00000000000..e66c9cc2420
--- /dev/null
+++ b/tests/ui/map_collect_result_unit.fixed
@@ -0,0 +1,16 @@
+// run-rustfix
+#![warn(clippy::map_collect_result_unit)]
+
+fn main() {
+    {
+        let _ = (0..3).try_for_each(|t| Err(t + 1));
+        let _: Result<(), _> = (0..3).try_for_each(|t| Err(t + 1));
+
+        let _ = (0..3).try_for_each(|t| Err(t + 1));
+    }
+}
+
+fn _ignore() {
+    let _ = (0..3).map(|t| Err(t + 1)).collect::<Result<Vec<i32>, _>>();
+    let _ = (0..3).map(|t| Err(t + 1)).collect::<Vec<Result<(), _>>>();
+}
diff --git a/tests/ui/map_collect_result_unit.rs b/tests/ui/map_collect_result_unit.rs
new file mode 100644
index 00000000000..6f08f4c3c53
--- /dev/null
+++ b/tests/ui/map_collect_result_unit.rs
@@ -0,0 +1,16 @@
+// run-rustfix
+#![warn(clippy::map_collect_result_unit)]
+
+fn main() {
+    {
+        let _ = (0..3).map(|t| Err(t + 1)).collect::<Result<(), _>>();
+        let _: Result<(), _> = (0..3).map(|t| Err(t + 1)).collect();
+
+        let _ = (0..3).try_for_each(|t| Err(t + 1));
+    }
+}
+
+fn _ignore() {
+    let _ = (0..3).map(|t| Err(t + 1)).collect::<Result<Vec<i32>, _>>();
+    let _ = (0..3).map(|t| Err(t + 1)).collect::<Vec<Result<(), _>>>();
+}
diff --git a/tests/ui/map_collect_result_unit.stderr b/tests/ui/map_collect_result_unit.stderr
new file mode 100644
index 00000000000..8b06e13baa6
--- /dev/null
+++ b/tests/ui/map_collect_result_unit.stderr
@@ -0,0 +1,16 @@
+error: `.map().collect()` can be replaced with `.try_for_each()`
+  --> $DIR/map_collect_result_unit.rs:6:17
+   |
+LL |         let _ = (0..3).map(|t| Err(t + 1)).collect::<Result<(), _>>();
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(0..3).try_for_each(|t| Err(t + 1))`
+   |
+   = note: `-D clippy::map-collect-result-unit` implied by `-D warnings`
+
+error: `.map().collect()` can be replaced with `.try_for_each()`
+  --> $DIR/map_collect_result_unit.rs:7:32
+   |
+LL |         let _: Result<(), _> = (0..3).map(|t| Err(t + 1)).collect();
+   |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(0..3).try_for_each(|t| Err(t + 1))`
+
+error: aborting due to 2 previous errors
+