Merge #3344

3344: Added lints `into_iter_on_ref` and `into_iter_on_array`. r=phansch a=kennytm

Fixes #1565.

`into_iter_on_array` is a correctness lint against `into_iter()` on `[T; n]` and `PathBuf` (please provide a concise noun that covers both types 🙃).

`into_iter_on_ref` is a style lint against `into_iter()` on `&C` where `C` is `[T]`, `Vec<T>`, `BTreeSet<T>` etc.

Both suggests replacing the `into_iter()` with `iter()` or `iter_mut()`.

`into_iter_on_array` is a correctness lint since it is very likely the standard library would provide an `into_iter()` method for `[T; n]` (#25725) and existing type inference of `[a, b, c].into_iter()` will be broken. `PathBuf` is also placed under this lint since `PathBuf::into_iter` currently doesn't exist and it makes some sense to implement `IntoIterator` on it yielding `OsString`s.

Co-authored-by: kennytm <[email protected]>

Author: bors[bot]

Diff for: CHANGELOG.md

@@ -713,6 +713,8 @@ All notable changes to this project will be documented in this file.
 [`inline_fn_without_body`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#inline_fn_without_body
 [`int_plus_one`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#int_plus_one
 [`integer_arithmetic`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#integer_arithmetic
+[`into_iter_on_array`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#into_iter_on_array
+[`into_iter_on_ref`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#into_iter_on_ref
 [`invalid_ref`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#invalid_ref
 [`invalid_regex`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#invalid_regex
 [`invalid_upcast_comparisons`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#invalid_upcast_comparisons

Diff for: README.md

@@ -9,7 +9,7 @@ We are currently in the process of discussing Clippy 1.0 via the RFC process in
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 284 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html)
+[There are 286 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

Diff for: clippy_lints/src/lib.rs

@@ -628,6 +628,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         methods::EXPECT_FUN_CALL,
         methods::FILTER_NEXT,
         methods::GET_UNWRAP,
+        methods::INTO_ITER_ON_ARRAY,
+        methods::INTO_ITER_ON_REF,
         methods::ITER_CLONED_COLLECT,
         methods::ITER_NTH,
         methods::ITER_SKIP_NEXT,
@@ -784,6 +786,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         mem_replace::MEM_REPLACE_OPTION_WITH_NONE,
         methods::CHARS_LAST_CMP,
         methods::GET_UNWRAP,
+        methods::INTO_ITER_ON_REF,
         methods::ITER_CLONED_COLLECT,
         methods::ITER_SKIP_NEXT,
         methods::NEW_RET_NO_SELF,
@@ -935,6 +938,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         loops::WHILE_IMMUTABLE_CONDITION,
         mem_discriminant::MEM_DISCRIMINANT_NON_ENUM,
         methods::CLONE_DOUBLE_REF,
+        methods::INTO_ITER_ON_ARRAY,
         methods::TEMPORARY_CSTRING_AS_PTR,
         minmax::MIN_MAX,
         misc::CMP_NAN,

Diff for: clippy_lints/src/literal_representation.rs

@@ -210,7 +210,7 @@ impl<'a> DigitInfo<'a> {
                 .filter(|&c| c != '_')
                 .collect::<Vec<_>>()
                 .chunks(group_size)
-                .map(|chunk| chunk.into_iter().rev().collect())
+                .map(|chunk| chunk.iter().rev().collect())
                 .rev()
                 .collect::<Vec<String>>()
                 .join("_");
@@ -221,7 +221,7 @@ impl<'a> DigitInfo<'a> {
                 .filter(|&c| c != '_')
                 .collect::<Vec<_>>()
                 .chunks(group_size)
-                .map(|chunk| chunk.into_iter().collect())
+                .map(|chunk| chunk.iter().collect())
                 .collect::<Vec<String>>()
                 .join("_");
             format!(
@@ -238,7 +238,7 @@ impl<'a> DigitInfo<'a> {
                 .collect::<Vec<_>>();
             let mut hint = filtered_digits_vec
                 .chunks(group_size)
-                .map(|chunk| chunk.into_iter().rev().collect())
+                .map(|chunk| chunk.iter().rev().collect())
                 .rev()
                 .collect::<Vec<String>>()
                 .join("_");

Diff for: clippy_lints/src/methods/mod.rs

@@ -575,7 +575,7 @@ declare_clippy_lint! {
 /// temporary placeholder for dealing with the `Option` type, then this does
 /// not mitigate the need for error handling. If there is a chance that `.get()`
 /// will be `None` in your program, then it is advisable that the `None` case
-/// is handled in a future refactor instead of using `.unwrap()` or the Index 
+/// is handled in a future refactor instead of using `.unwrap()` or the Index
 /// trait.
 ///
 /// **Example:**
@@ -745,6 +745,51 @@ declare_clippy_lint! {
     "using `filter_map` when a more succinct alternative exists"
 }
 
+/// **What it does:** Checks for `into_iter` calls on types which should be replaced by `iter` or
+/// `iter_mut`.
+///
+/// **Why is this bad?** Arrays and `PathBuf` do not yet have an `into_iter` method which move out
+/// their content into an iterator. Auto-referencing resolves the `into_iter` call to its reference
+/// instead, like `<&[T; N] as IntoIterator>::into_iter`, which just iterates over item references
+/// like calling `iter` would. Furthermore, when the standard library actually
+/// [implements the `into_iter` method][25725] which moves the content out of the array, the
+/// original use of `into_iter` got inferred with the wrong type and the code will be broken.
+///
+/// **Known problems:** None
+///
+/// **Example:**
+///
+/// ```rust
+/// let _ = [1, 2, 3].into_iter().map(|x| *x).collect::<Vec<u32>>();
+/// ```
+///
+/// [25725]: https://github.com/rust-lang/rust/issues/25725
+declare_clippy_lint! {
+    pub INTO_ITER_ON_ARRAY,
+    correctness,
+    "using `.into_iter()` on an array"
+}
+
+/// **What it does:** Checks for `into_iter` calls on references which should be replaced by `iter`
+/// or `iter_mut`.
+///
+/// **Why is this bad?** Readability. Calling `into_iter` on a reference will not move out its
+/// content into the resulting iterator, which is confusing. It is better just call `iter` or
+/// `iter_mut` directly.
+///
+/// **Known problems:** None
+///
+/// **Example:**
+///
+/// ```rust
+/// let _ = (&vec![3, 4, 5]).into_iter();
+/// ```
+declare_clippy_lint! {
+    pub INTO_ITER_ON_REF,
+    style,
+    "using `.into_iter()` on a reference"
+}
+
 impl LintPass for Pass {
     fn get_lints(&self) -> LintArray {
         lint_array!(
@@ -779,7 +824,9 @@ impl LintPass for Pass {
             ITER_CLONED_COLLECT,
             USELESS_ASREF,
             UNNECESSARY_FOLD,
-            UNNECESSARY_FILTER_MAP
+            UNNECESSARY_FILTER_MAP,
+            INTO_ITER_ON_ARRAY,
+            INTO_ITER_ON_REF,
         )
     }
 }
@@ -843,6 +890,9 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
                             lint_single_char_pattern(cx, expr, &args[pos]);
                         }
                     },
+                    ty::Ref(..) if method_call.ident.name == "into_iter" => {
+                        lint_into_iter(cx, expr, self_ty, *method_span);
+                    },
                     _ => (),
                 }
             },
@@ -2084,6 +2134,71 @@ fn lint_asref(cx: &LateContext<'_, '_>, expr: &hir::Expr, call_name: &str, as_re
     }
 }
 
+fn ty_has_iter_method(cx: &LateContext<'_, '_>, self_ref_ty: ty::Ty<'_>) -> Option<(&'static Lint, &'static str, &'static str)> {
+    // FIXME: instead of this hard-coded list, we should check if `<adt>::iter`
+    // exists and has the desired signature. Unfortunately FnCtxt is not exported
+    // so we can't use its `lookup_method` method.
+    static INTO_ITER_COLLECTIONS: [(&Lint, &[&str]); 13] = [
+        (INTO_ITER_ON_REF, &paths::VEC),
+        (INTO_ITER_ON_REF, &paths::OPTION),
+        (INTO_ITER_ON_REF, &paths::RESULT),
+        (INTO_ITER_ON_REF, &paths::BTREESET),
+        (INTO_ITER_ON_REF, &paths::BTREEMAP),
+        (INTO_ITER_ON_REF, &paths::VEC_DEQUE),
+        (INTO_ITER_ON_REF, &paths::LINKED_LIST),
+        (INTO_ITER_ON_REF, &paths::BINARY_HEAP),
+        (INTO_ITER_ON_REF, &paths::HASHSET),
+        (INTO_ITER_ON_REF, &paths::HASHMAP),
+        (INTO_ITER_ON_ARRAY, &["std", "path", "PathBuf"]),
+        (INTO_ITER_ON_REF, &["std", "path", "Path"]),
+        (INTO_ITER_ON_REF, &["std", "sync", "mpsc", "Receiver"]),
+    ];
+
+    let (self_ty, mutbl) = match self_ref_ty.sty {
+        ty::TyKind::Ref(_, self_ty, mutbl) => (self_ty, mutbl),
+        _ => unreachable!(),
+    };
+    let method_name = match mutbl {
+        hir::MutImmutable => "iter",
+        hir::MutMutable => "iter_mut",
+    };
+
+    let def_id = match self_ty.sty {
+        ty::TyKind::Array(..) => return Some((INTO_ITER_ON_ARRAY, "array", method_name)),
+        ty::TyKind::Slice(..) => return Some((INTO_ITER_ON_REF, "slice", method_name)),
+        ty::Adt(adt, _) => adt.did,
+        _ => return None,
+    };
+
+    for (lint, path) in &INTO_ITER_COLLECTIONS {
+        if match_def_path(cx.tcx, def_id, path) {
+            return Some((lint, path.last().unwrap(), method_name))
+        }
+    }
+    None
+}
+
+fn lint_into_iter(cx: &LateContext<'_, '_>, expr: &hir::Expr, self_ref_ty: ty::Ty<'_>, method_span: Span) {
+    if !match_trait_method(cx, expr, &paths::INTO_ITERATOR) {
+        return;
+    }
+    if let Some((lint, kind, method_name)) = ty_has_iter_method(cx, self_ref_ty) {
+        span_lint_and_sugg(
+            cx,
+            lint,
+            method_span,
+            &format!(
+                "this .into_iter() call is equivalent to .{}() and will not move the {}",
+                method_name,
+                kind,
+            ),
+            "call directly",
+            method_name.to_owned(),
+        );
+    }
+}
+
+
 /// Given a `Result<T, E>` type, return its error type (`E`).
 fn get_error_type<'a>(cx: &LateContext<'_, '_>, ty: Ty<'a>) -> Option<Ty<'a>> {
     if let ty::Adt(_, substs) = ty.sty {

Diff for: tests/ui/for_loop.rs

@@ -29,7 +29,7 @@ impl Unrelated {
        clippy::for_kv_map)]
 #[warn(clippy::unused_collect)]
 #[allow(clippy::linkedlist, clippy::shadow_unrelated, clippy::unnecessary_mut_passed, clippy::cyclomatic_complexity, clippy::similar_names)]
-#[allow(clippy::many_single_char_names, unused_variables)]
+#[allow(clippy::many_single_char_names, unused_variables, clippy::into_iter_on_array)]
 fn main() {
     const MAX_LEN: usize = 42;
 

Diff for: tests/ui/into_iter_on_ref.rs

@@ -0,0 +1,44 @@
+#![warn(clippy::into_iter_on_ref)]
+#![deny(clippy::into_iter_on_array)]
+
+struct X;
+use std::collections::*;
+
+fn main() {
+    for _ in &[1,2,3] {}
+    for _ in vec![X, X] {}
+    for _ in &vec![X, X] {}
+    for _ in [1,2,3].into_iter() {} //~ ERROR equivalent to .iter()
+
+    let _ = [1,2,3].into_iter(); //~ ERROR equivalent to .iter()
+    let _ = vec![1,2,3].into_iter();
+    let _ = (&vec![1,2,3]).into_iter(); //~ WARN equivalent to .iter()
+    let _ = vec![1,2,3].into_boxed_slice().into_iter(); //~ WARN equivalent to .iter()
+    let _ = std::rc::Rc::from(&[X][..]).into_iter(); //~ WARN equivalent to .iter()
+    let _ = std::sync::Arc::from(&[X][..]).into_iter(); //~ WARN equivalent to .iter()
+
+    let _ = (&&&&&&&[1,2,3]).into_iter(); //~ ERROR equivalent to .iter()
+    let _ = (&&&&mut &&&[1,2,3]).into_iter(); //~ ERROR equivalent to .iter()
+    let _ = (&mut &mut &mut [1,2,3]).into_iter(); //~ ERROR equivalent to .iter_mut()
+
+    let _ = (&Some(4)).into_iter(); //~ WARN equivalent to .iter()
+    let _ = (&mut Some(5)).into_iter(); //~ WARN equivalent to .iter_mut()
+    let _ = (&Ok::<_, i32>(6)).into_iter(); //~ WARN equivalent to .iter()
+    let _ = (&mut Err::<i32, _>(7)).into_iter(); //~ WARN equivalent to .iter_mut()
+    let _ = (&Vec::<i32>::new()).into_iter(); //~ WARN equivalent to .iter()
+    let _ = (&mut Vec::<i32>::new()).into_iter(); //~ WARN equivalent to .iter_mut()
+    let _ = (&BTreeMap::<i32, u64>::new()).into_iter(); //~ WARN equivalent to .iter()
+    let _ = (&mut BTreeMap::<i32, u64>::new()).into_iter(); //~ WARN equivalent to .iter_mut()
+    let _ = (&VecDeque::<i32>::new()).into_iter(); //~ WARN equivalent to .iter()
+    let _ = (&mut VecDeque::<i32>::new()).into_iter(); //~ WARN equivalent to .iter_mut()
+    let _ = (&LinkedList::<i32>::new()).into_iter(); //~ WARN equivalent to .iter()
+    let _ = (&mut LinkedList::<i32>::new()).into_iter(); //~ WARN equivalent to .iter_mut()
+    let _ = (&HashMap::<i32, u64>::new()).into_iter(); //~ WARN equivalent to .iter()
+    let _ = (&mut HashMap::<i32, u64>::new()).into_iter(); //~ WARN equivalent to .iter_mut()
+
+    let _ = (&BTreeSet::<i32>::new()).into_iter(); //~ WARN equivalent to .iter()
+    let _ = (&BinaryHeap::<i32>::new()).into_iter(); //~ WARN equivalent to .iter()
+    let _ = (&HashSet::<i32>::new()).into_iter(); //~ WARN equivalent to .iter()
+    let _ = std::path::Path::new("12/34").into_iter(); //~ WARN equivalent to .iter()
+    let _ = std::path::PathBuf::from("12/34").into_iter(); //~ ERROR equivalent to .iter()
+}