Skip to content

Commit

Permalink
Merge #3344
Browse files Browse the repository at this point in the history
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]` (rust-lang/rust#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 <kennytm@gmail.com>
  • Loading branch information
bors[bot] and kennytm committed Nov 2, 2018
2 parents a8d2584 + 5563bd6 commit dc1e409
Show file tree
Hide file tree
Showing 8 changed files with 350 additions and 7 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
4 changes: 4 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
6 changes: 3 additions & 3 deletions clippy_lints/src/literal_representation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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("_");
Expand All @@ -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!(
Expand All @@ -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("_");
Expand Down
119 changes: 117 additions & 2 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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:**
Expand Down Expand Up @@ -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!(
Expand Down Expand Up @@ -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,
)
}
}
Expand Down Expand Up @@ -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);
},
_ => (),
}
},
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/for_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
44 changes: 44 additions & 0 deletions tests/ui/into_iter_on_ref.rs
Original file line number Diff line number Diff line change
@@ -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()
}
Loading

0 comments on commit dc1e409

Please sign in to comment.