Auto merge of #6101 - pitiK3U:from_iter_instead_of_collect, r=flip1995

Add lint: from_iter_instead_of_collect

Fixes #5679

This implements lint for `::from_iter()` from #5679 not the general issue (`std::ops::Add::add`, etc.).
This lint checks if expression is function call with `from_iter` name and if it's implementation of the `std::iter::FromIterator` trait.

changelog: Introduce  from_iter_instead_of_collect lint

Author: bors

CHANGELOG.md

@@ -1732,6 +1732,7 @@ Released 2018-09-13
 [`for_loops_over_fallibles`]: https://rust-lang.github.io/rust-clippy/master/index.html#for_loops_over_fallibles
 [`forget_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_copy
 [`forget_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_ref
+[`from_iter_instead_of_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_iter_instead_of_collect
 [`future_not_send`]: https://rust-lang.github.io/rust-clippy/master/index.html#future_not_send
 [`get_last_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_last_with_len
 [`get_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_unwrap

clippy_lints/src/lib.rs

@@ -693,6 +693,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &methods::FILTER_NEXT,
         &methods::FIND_MAP,
         &methods::FLAT_MAP_IDENTITY,
+        &methods::FROM_ITER_INSTEAD_OF_COLLECT,
         &methods::GET_UNWRAP,
         &methods::INEFFICIENT_TO_STRING,
         &methods::INTO_ITER_ON_REF,
@@ -1422,6 +1423,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::EXPECT_FUN_CALL),
         LintId::of(&methods::FILTER_NEXT),
         LintId::of(&methods::FLAT_MAP_IDENTITY),
+        LintId::of(&methods::FROM_ITER_INSTEAD_OF_COLLECT),
         LintId::of(&methods::INTO_ITER_ON_REF),
         LintId::of(&methods::ITERATOR_STEP_BY_ZERO),
         LintId::of(&methods::ITER_CLONED_COLLECT),
@@ -1620,6 +1622,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&mem_replace::MEM_REPLACE_WITH_DEFAULT),
         LintId::of(&methods::CHARS_LAST_CMP),
         LintId::of(&methods::CHARS_NEXT_CMP),
+        LintId::of(&methods::FROM_ITER_INSTEAD_OF_COLLECT),
         LintId::of(&methods::INTO_ITER_ON_REF),
         LintId::of(&methods::ITER_CLONED_COLLECT),
         LintId::of(&methods::ITER_NEXT_SLICE),

clippy_lints/src/lifetimes.rs

@@ -16,7 +16,6 @@ use rustc_middle::hir::map::Map;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::source_map::Span;
 use rustc_span::symbol::{kw, Symbol};
-use std::iter::FromIterator;
 
 declare_clippy_lint! {
     /// **What it does:** Checks for lifetime annotations which can be removed by
@@ -214,14 +213,15 @@ fn could_use_elision<'tcx>(
     }
 
     if allowed_lts
-        .intersection(&FxHashSet::from_iter(
-            input_visitor
+        .intersection(
+            &input_visitor
                 .nested_elision_site_lts
                 .iter()
                 .chain(output_visitor.nested_elision_site_lts.iter())
                 .cloned()
-                .filter(|v| matches!(v, RefLt::Named(_))),
-        ))
+                .filter(|v| matches!(v, RefLt::Named(_)))
+                .collect(),
+        )
         .next()
         .is_some()
     {

clippy_lints/src/methods/mod.rs

@@ -1371,6 +1371,39 @@ declare_clippy_lint! {
     "using `.map(_).collect::<Result<(),_>()`, which can be replaced with `try_for_each`"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for `from_iter()` function calls on types that implement the `FromIterator`
+    /// trait.
+    ///
+    /// **Why is this bad?** It is recommended style to use collect. See
+    /// [FromIterator documentation](https://doc.rust-lang.org/std/iter/trait.FromIterator.html)
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// use std::iter::FromIterator;
+    ///
+    /// let five_fives = std::iter::repeat(5).take(5);
+    ///
+    /// let v = Vec::from_iter(five_fives);
+    ///
+    /// assert_eq!(v, vec![5, 5, 5, 5, 5]);
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let five_fives = std::iter::repeat(5).take(5);
+    ///
+    /// let v: Vec<i32> = five_fives.collect();
+    ///
+    /// assert_eq!(v, vec![5, 5, 5, 5, 5]);
+    /// ```
+    pub FROM_ITER_INSTEAD_OF_COLLECT,
+    style,
+    "use `.collect()` instead of `::from_iter()`"
+}
+
 declare_lint_pass!(Methods => [
     UNWRAP_USED,
     EXPECT_USED,
@@ -1421,6 +1454,7 @@ declare_lint_pass!(Methods => [
     OPTION_AS_REF_DEREF,
     UNNECESSARY_LAZY_EVALUATIONS,
     MAP_COLLECT_RESULT_UNIT,
+    FROM_ITER_INSTEAD_OF_COLLECT,
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for Methods {
@@ -1507,6 +1541,13 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
         }
 
         match expr.kind {
+            hir::ExprKind::Call(ref func, ref args) => {
+                if let hir::ExprKind::Path(path) = &func.kind {
+                    if match_qpath(path, &["from_iter"]) {
+                        lint_from_iter(cx, expr, args);
+                    }
+                }
+            },
             hir::ExprKind::MethodCall(ref method_call, ref method_span, ref args, _) => {
                 lint_or_fun_call(cx, expr, *method_span, &method_call.ident.as_str(), args);
                 lint_expect_fun_call(cx, expr, *method_span, &method_call.ident.as_str(), args);
@@ -3856,6 +3897,28 @@ fn lint_filetype_is_file(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir
     span_lint_and_help(cx, FILETYPE_IS_FILE, span, &lint_msg, None, &help_msg);
 }
 
+fn lint_from_iter(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_>]) {
+    let ty = cx.typeck_results().expr_ty(expr);
+    let arg_ty = cx.typeck_results().expr_ty(&args[0]);
+
+    let from_iter_id = get_trait_def_id(cx, &paths::FROM_ITERATOR).unwrap();
+    let iter_id = get_trait_def_id(cx, &paths::ITERATOR).unwrap();
+
+    if implements_trait(cx, ty, from_iter_id, &[]) && implements_trait(cx, arg_ty, iter_id, &[]) {
+        // `expr` implements `FromIterator` trait
+        let iter_expr = snippet(cx, args[0].span, "..");
+        span_lint_and_sugg(
+            cx,
+            FROM_ITER_INSTEAD_OF_COLLECT,
+            expr.span,
+            "usage of `FromIterator::from_iter`",
+            "use `.collect()` instead of `::from_iter()`",
+            format!("{}.collect()", iter_expr),
+            Applicability::MaybeIncorrect,
+        );
+    }
+}
+
 fn fn_header_equals(expected: hir::FnHeader, actual: hir::FnHeader) -> bool {
     expected.constness == actual.constness
         && expected.unsafety == actual.unsafety

clippy_lints/src/utils/paths.rs

@@ -44,6 +44,7 @@ pub const FN: [&str; 3] = ["core", "ops", "Fn"];
 pub const FN_MUT: [&str; 3] = ["core", "ops", "FnMut"];
 pub const FN_ONCE: [&str; 3] = ["core", "ops", "FnOnce"];
 pub const FROM_FROM: [&str; 4] = ["core", "convert", "From", "from"];
+pub const FROM_ITERATOR: [&str; 5] = ["core", "iter", "traits", "collect", "FromIterator"];
 pub const FROM_TRAIT: [&str; 3] = ["core", "convert", "From"];
 pub const FUTURE_FROM_GENERATOR: [&str; 3] = ["core", "future", "from_generator"];
 pub const HASH: [&str; 3] = ["core", "hash", "Hash"];

src/lintlist/mod.rs

@@ -753,6 +753,13 @@ vec![
         deprecation: None,
         module: "drop_forget_ref",
     },
+    Lint {
+        name: "from_iter_instead_of_collect",
+        group: "style",
+        desc: "use `.collect()` instead of `::from_iter()`",
+        deprecation: None,
+        module: "methods",
+    },
     Lint {
         name: "future_not_send",
         group: "nursery",

tests/ui/from_iter_instead_of_collect.rs

@@ -0,0 +1,13 @@
+#![warn(clippy::from_iter_instead_of_collect)]
+
+use std::collections::HashMap;
+use std::iter::FromIterator;
+
+fn main() {
+    let iter_expr = std::iter::repeat(5).take(5);
+    Vec::from_iter(iter_expr);
+
+    HashMap::<usize, &i8>::from_iter(vec![5, 5, 5, 5].iter().enumerate());
+
+    Vec::from_iter(vec![42u32]);
+}

tests/ui/from_iter_instead_of_collect.stderr

@@ -0,0 +1,16 @@
+error: usage of `FromIterator::from_iter`
+  --> $DIR/from_iter_instead_of_collect.rs:8:5
+   |
+LL |     Vec::from_iter(iter_expr);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `.collect()` instead of `::from_iter()`: `iter_expr.collect()`
+   |
+   = note: `-D clippy::from-iter-instead-of-collect` implied by `-D warnings`
+
+error: usage of `FromIterator::from_iter`
+  --> $DIR/from_iter_instead_of_collect.rs:10:5
+   |
+LL |     HashMap::<usize, &i8>::from_iter(vec![5, 5, 5, 5].iter().enumerate());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `.collect()` instead of `::from_iter()`: `vec![5, 5, 5, 5].iter().enumerate().collect()`
+
+error: aborting due to 2 previous errors
+

tests/ui/get_unwrap.fixed

@@ -1,5 +1,5 @@
 // run-rustfix
-#![allow(unused_mut)]
+#![allow(unused_mut, clippy::from_iter_instead_of_collect)]
 #![deny(clippy::get_unwrap)]
 
 use std::collections::BTreeMap;

tests/ui/get_unwrap.rs

@@ -1,5 +1,5 @@
 // run-rustfix
-#![allow(unused_mut)]
+#![allow(unused_mut, clippy::from_iter_instead_of_collect)]
 #![deny(clippy::get_unwrap)]
 
 use std::collections::BTreeMap;