From 2d1c9313b0e901229c0ed69f030302917cf8fd18 Mon Sep 17 00:00:00 2001 From: kennytm Date: Sun, 21 Oct 2018 04:03:38 +0800 Subject: [PATCH] Added lints `into_iter_on_ref` and `into_iter_on_array`. Fix #1565. --- CHANGELOG.md | 2 + README.md | 2 +- clippy_lints/src/lib.rs | 4 + clippy_lints/src/methods/mod.rs | 117 +++++++++++++++++++- tests/ui/for_loop.rs | 2 +- tests/ui/into_iter_on_ref.rs | 44 ++++++++ tests/ui/into_iter_on_ref.stderr | 178 +++++++++++++++++++++++++++++++ 7 files changed, 346 insertions(+), 3 deletions(-) create mode 100644 tests/ui/into_iter_on_ref.rs create mode 100644 tests/ui/into_iter_on_ref.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d5d10c7a..72183f68e 100644 --- a/CHANGELOG.md +++ b/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 --git a/README.md b/README.md index 3c056171f..8af7a20f6 100644 --- a/README.md +++ b/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 --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 56cd2ca3e..863c06234 100644 --- a/clippy_lints/src/lib.rs +++ b/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 --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 319827064..4b83c6e6b 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -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. Calling `into_iter` instead just forwards to `iter` or +/// `iter_mut` due to auto-referencing, of which only yield references. 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::>(); +/// ``` +/// +/// [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)> { + 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, + }; + + // FIXME: instead of this hard-coded list, we should check if `::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"]), + ]; + + 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` type, return its error type (`E`). fn get_error_type<'a>(cx: &LateContext<'_, '_>, ty: Ty<'a>) -> Option> { if let ty::Adt(_, substs) = ty.sty { diff --git a/tests/ui/for_loop.rs b/tests/ui/for_loop.rs index 2a70149f2..513a3c0ee 100644 --- a/tests/ui/for_loop.rs +++ b/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 --git a/tests/ui/into_iter_on_ref.rs b/tests/ui/into_iter_on_ref.rs new file mode 100644 index 000000000..72aa6341a --- /dev/null +++ b/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::(7)).into_iter(); //~ WARN equivalent to .iter_mut() + let _ = (&Vec::::new()).into_iter(); //~ WARN equivalent to .iter() + let _ = (&mut Vec::::new()).into_iter(); //~ WARN equivalent to .iter_mut() + let _ = (&BTreeMap::::new()).into_iter(); //~ WARN equivalent to .iter() + let _ = (&mut BTreeMap::::new()).into_iter(); //~ WARN equivalent to .iter_mut() + let _ = (&VecDeque::::new()).into_iter(); //~ WARN equivalent to .iter() + let _ = (&mut VecDeque::::new()).into_iter(); //~ WARN equivalent to .iter_mut() + let _ = (&LinkedList::::new()).into_iter(); //~ WARN equivalent to .iter() + let _ = (&mut LinkedList::::new()).into_iter(); //~ WARN equivalent to .iter_mut() + let _ = (&HashMap::::new()).into_iter(); //~ WARN equivalent to .iter() + let _ = (&mut HashMap::::new()).into_iter(); //~ WARN equivalent to .iter_mut() + + let _ = (&BTreeSet::::new()).into_iter(); //~ WARN equivalent to .iter() + let _ = (&BinaryHeap::::new()).into_iter(); //~ WARN equivalent to .iter() + let _ = (&HashSet::::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() +} diff --git a/tests/ui/into_iter_on_ref.stderr b/tests/ui/into_iter_on_ref.stderr new file mode 100644 index 000000000..390554230 --- /dev/null +++ b/tests/ui/into_iter_on_ref.stderr @@ -0,0 +1,178 @@ +error: this .into_iter() call is equivalent to .iter() and will not move the array + --> $DIR/into_iter_on_ref.rs:11:22 + | +11 | for _ in [1,2,3].into_iter() {} //~ ERROR equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + | +note: lint level defined here + --> $DIR/into_iter_on_ref.rs:2:9 + | +2 | #![deny(clippy::into_iter_on_array)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this .into_iter() call is equivalent to .iter() and will not move the array + --> $DIR/into_iter_on_ref.rs:13:21 + | +13 | let _ = [1,2,3].into_iter(); //~ ERROR equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: this .into_iter() call is equivalent to .iter() and will not move the Vec + --> $DIR/into_iter_on_ref.rs:15:28 + | +15 | let _ = (&vec![1,2,3]).into_iter(); //~ WARN equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + | + = note: `-D clippy::into-iter-on-ref` implied by `-D warnings` + +error: this .into_iter() call is equivalent to .iter() and will not move the slice + --> $DIR/into_iter_on_ref.rs:16:44 + | +16 | let _ = vec![1,2,3].into_boxed_slice().into_iter(); //~ WARN equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: this .into_iter() call is equivalent to .iter() and will not move the slice + --> $DIR/into_iter_on_ref.rs:17:41 + | +17 | let _ = std::rc::Rc::from(&[X][..]).into_iter(); //~ WARN equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: this .into_iter() call is equivalent to .iter() and will not move the slice + --> $DIR/into_iter_on_ref.rs:18:44 + | +18 | let _ = std::sync::Arc::from(&[X][..]).into_iter(); //~ WARN equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: this .into_iter() call is equivalent to .iter() and will not move the array + --> $DIR/into_iter_on_ref.rs:20:30 + | +20 | let _ = (&&&&&&&[1,2,3]).into_iter(); //~ ERROR equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: this .into_iter() call is equivalent to .iter() and will not move the array + --> $DIR/into_iter_on_ref.rs:21:34 + | +21 | let _ = (&&&&mut &&&[1,2,3]).into_iter(); //~ ERROR equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: this .into_iter() call is equivalent to .iter_mut() and will not move the array + --> $DIR/into_iter_on_ref.rs:22:38 + | +22 | let _ = (&mut &mut &mut [1,2,3]).into_iter(); //~ ERROR equivalent to .iter_mut() + | ^^^^^^^^^ help: call directly: `iter_mut` + +error: this .into_iter() call is equivalent to .iter() and will not move the Option + --> $DIR/into_iter_on_ref.rs:24:24 + | +24 | let _ = (&Some(4)).into_iter(); //~ WARN equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: this .into_iter() call is equivalent to .iter_mut() and will not move the Option + --> $DIR/into_iter_on_ref.rs:25:28 + | +25 | let _ = (&mut Some(5)).into_iter(); //~ WARN equivalent to .iter_mut() + | ^^^^^^^^^ help: call directly: `iter_mut` + +error: this .into_iter() call is equivalent to .iter() and will not move the Result + --> $DIR/into_iter_on_ref.rs:26:32 + | +26 | let _ = (&Ok::<_, i32>(6)).into_iter(); //~ WARN equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: this .into_iter() call is equivalent to .iter_mut() and will not move the Result + --> $DIR/into_iter_on_ref.rs:27:37 + | +27 | let _ = (&mut Err::(7)).into_iter(); //~ WARN equivalent to .iter_mut() + | ^^^^^^^^^ help: call directly: `iter_mut` + +error: this .into_iter() call is equivalent to .iter() and will not move the Vec + --> $DIR/into_iter_on_ref.rs:28:34 + | +28 | let _ = (&Vec::::new()).into_iter(); //~ WARN equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: this .into_iter() call is equivalent to .iter_mut() and will not move the Vec + --> $DIR/into_iter_on_ref.rs:29:38 + | +29 | let _ = (&mut Vec::::new()).into_iter(); //~ WARN equivalent to .iter_mut() + | ^^^^^^^^^ help: call directly: `iter_mut` + +error: this .into_iter() call is equivalent to .iter() and will not move the BTreeMap + --> $DIR/into_iter_on_ref.rs:30:44 + | +30 | let _ = (&BTreeMap::::new()).into_iter(); //~ WARN equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: this .into_iter() call is equivalent to .iter_mut() and will not move the BTreeMap + --> $DIR/into_iter_on_ref.rs:31:48 + | +31 | let _ = (&mut BTreeMap::::new()).into_iter(); //~ WARN equivalent to .iter_mut() + | ^^^^^^^^^ help: call directly: `iter_mut` + +error: this .into_iter() call is equivalent to .iter() and will not move the VecDeque + --> $DIR/into_iter_on_ref.rs:32:39 + | +32 | let _ = (&VecDeque::::new()).into_iter(); //~ WARN equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: this .into_iter() call is equivalent to .iter_mut() and will not move the VecDeque + --> $DIR/into_iter_on_ref.rs:33:43 + | +33 | let _ = (&mut VecDeque::::new()).into_iter(); //~ WARN equivalent to .iter_mut() + | ^^^^^^^^^ help: call directly: `iter_mut` + +error: this .into_iter() call is equivalent to .iter() and will not move the LinkedList + --> $DIR/into_iter_on_ref.rs:34:41 + | +34 | let _ = (&LinkedList::::new()).into_iter(); //~ WARN equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: this .into_iter() call is equivalent to .iter_mut() and will not move the LinkedList + --> $DIR/into_iter_on_ref.rs:35:45 + | +35 | let _ = (&mut LinkedList::::new()).into_iter(); //~ WARN equivalent to .iter_mut() + | ^^^^^^^^^ help: call directly: `iter_mut` + +error: this .into_iter() call is equivalent to .iter() and will not move the HashMap + --> $DIR/into_iter_on_ref.rs:36:43 + | +36 | let _ = (&HashMap::::new()).into_iter(); //~ WARN equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: this .into_iter() call is equivalent to .iter_mut() and will not move the HashMap + --> $DIR/into_iter_on_ref.rs:37:47 + | +37 | let _ = (&mut HashMap::::new()).into_iter(); //~ WARN equivalent to .iter_mut() + | ^^^^^^^^^ help: call directly: `iter_mut` + +error: this .into_iter() call is equivalent to .iter() and will not move the BTreeSet + --> $DIR/into_iter_on_ref.rs:39:39 + | +39 | let _ = (&BTreeSet::::new()).into_iter(); //~ WARN equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: this .into_iter() call is equivalent to .iter() and will not move the BinaryHeap + --> $DIR/into_iter_on_ref.rs:40:41 + | +40 | let _ = (&BinaryHeap::::new()).into_iter(); //~ WARN equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: this .into_iter() call is equivalent to .iter() and will not move the HashSet + --> $DIR/into_iter_on_ref.rs:41:38 + | +41 | let _ = (&HashSet::::new()).into_iter(); //~ WARN equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: this .into_iter() call is equivalent to .iter() and will not move the Path + --> $DIR/into_iter_on_ref.rs:42:43 + | +42 | let _ = std::path::Path::new("12/34").into_iter(); //~ WARN equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: this .into_iter() call is equivalent to .iter() and will not move the PathBuf + --> $DIR/into_iter_on_ref.rs:43:47 + | +43 | let _ = std::path::PathBuf::from("12/34").into_iter(); //~ ERROR equivalent to .iter() + | ^^^^^^^^^ help: call directly: `iter` + +error: aborting due to 28 previous errors +