diff --git a/CHANGELOG.md b/CHANGELOG.md index 81102a900e5..790bbc04691 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -172,6 +172,7 @@ All notable changes to this project will be documented in this file. [`invalid_upcast_comparisons`]: https://github.com/Manishearth/rust-clippy/wiki#invalid_upcast_comparisons [`items_after_statements`]: https://github.com/Manishearth/rust-clippy/wiki#items_after_statements [`iter_next_loop`]: https://github.com/Manishearth/rust-clippy/wiki#iter_next_loop +[`iter_nth`]: https://github.com/Manishearth/rust-clippy/wiki#iter_nth [`len_without_is_empty`]: https://github.com/Manishearth/rust-clippy/wiki#len_without_is_empty [`len_zero`]: https://github.com/Manishearth/rust-clippy/wiki#len_zero [`let_and_return`]: https://github.com/Manishearth/rust-clippy/wiki#let_and_return diff --git a/README.md b/README.md index ffda505eeac..85e9ed70bf8 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ Table of contents: ## Lints -There are 152 lints included in this crate: +There are 153 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -78,6 +78,7 @@ name [invalid_upcast_comparisons](https://github.com/Manishearth/rust-clippy/wiki#invalid_upcast_comparisons) | allow | a comparison involving an upcast which is always true or false [items_after_statements](https://github.com/Manishearth/rust-clippy/wiki#items_after_statements) | allow | finds blocks where an item comes after a statement [iter_next_loop](https://github.com/Manishearth/rust-clippy/wiki#iter_next_loop) | warn | for-looping over `_.next()` which is probably not intended +[iter_nth](https://github.com/Manishearth/rust-clippy/wiki#iter_nth) | warn | using `.iter().nth()` on a slice or Vec [len_without_is_empty](https://github.com/Manishearth/rust-clippy/wiki#len_without_is_empty) | warn | traits and impls that have `.len()` but not `.is_empty()` [len_zero](https://github.com/Manishearth/rust-clippy/wiki#len_zero) | warn | checking `.len() == 0` or `.len() > 0` (or similar) when `.is_empty()` could be used instead [let_and_return](https://github.com/Manishearth/rust-clippy/wiki#let_and_return) | warn | creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a block diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 42a35e7009b..d64f653dd4c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -349,6 +349,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { methods::CLONE_ON_COPY, methods::EXTEND_FROM_SLICE, methods::FILTER_NEXT, + methods::ITER_NTH, methods::NEW_RET_NO_SELF, methods::OK_EXPECT, methods::OPTION_MAP_UNWRAP_OR, diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 9dcc8f9d016..da1420c3add 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -312,6 +312,30 @@ declare_lint! { "getting the inner pointer of a temporary `CString`" } +/// **What it does:** This lint checks for use of `.iter().nth()` on a slice or Vec. +/// +/// **Why is this bad?** `.get()` is more efficient and more readable. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// let some_vec = vec![0, 1, 2, 3]; +/// let bad_vec = some_vec.iter().nth(3); +/// let bad_slice = &some_vec[..].iter().nth(3); +/// ``` +/// The correct use would be: +/// ```rust +/// let some_vec = vec![0, 1, 2, 3]; +/// let bad_vec = some_vec.get(3); +/// let bad_slice = &some_vec[..].get(3); +/// ``` +declare_lint! { + pub ITER_NTH, + Warn, + "using `.iter().nth()` on a slice or Vec" +} + impl LintPass for MethodsPass { fn get_lints(&self) -> LintArray { lint_array!(EXTEND_FROM_SLICE, @@ -330,7 +354,8 @@ impl LintPass for MethodsPass { NEW_RET_NO_SELF, SINGLE_CHAR_PATTERN, SEARCH_IS_SOME, - TEMPORARY_CSTRING_AS_PTR) + TEMPORARY_CSTRING_AS_PTR, + ITER_NTH) } } @@ -363,6 +388,8 @@ impl LateLintPass for MethodsPass { lint_extend(cx, expr, arglists[0]); } else if let Some(arglists) = method_chain_args(expr, &["unwrap", "as_ptr"]) { lint_cstring_as_ptr(cx, expr, &arglists[0][0], &arglists[1][0]); + } else if let Some(arglists) = method_chain_args(expr, &["iter", "nth"]) { + lint_iter_nth(cx, expr, arglists[0]); } lint_or_fun_call(cx, expr, &name.node.as_str(), args); @@ -616,6 +643,25 @@ fn lint_cstring_as_ptr(cx: &LateContext, expr: &hir::Expr, new: &hir::Expr, unwr }} } +#[allow(ptr_arg)] +// Type of MethodArgs is potentially a Vec +fn lint_iter_nth(cx: &LateContext, expr: &hir::Expr, iter_args: &MethodArgs){ + // lint if the caller of `.iter().nth()` is a `slice` + if let Some(_) = derefs_to_slice(cx, &iter_args[0], &cx.tcx.expr_ty(&iter_args[0])) { + span_lint(cx, + ITER_NTH, + expr.span, + "called `.iter().nth()` on a slice. Calling `.get()` is both faster and more readable"); + } + // lint if the caller of `.iter().nth()` is a `Vec` + else if match_type(cx, cx.tcx.expr_ty(&iter_args[0]), &paths::VEC) { + span_lint(cx, + ITER_NTH, + expr.span, + "called `.iter().nth()` on a Vec. Calling `.get()` is both faster and more readable"); + } +} + fn derefs_to_slice(cx: &LateContext, expr: &hir::Expr, ty: &ty::Ty) -> Option<(Span, &'static str)> { fn may_slice(cx: &LateContext, ty: &ty::Ty) -> bool { match ty.sty { diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 89267462f5d..811b9116143 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -128,6 +128,16 @@ fn option_methods() { } +/// Struct to generate false positives for things with .iter() +#[derive(Copy, Clone)] +struct HasIter; + +impl HasIter { + fn iter(self) -> IteratorFalsePositives { + IteratorFalsePositives { foo: 0 } + } +} + /// Struct to generate false positive for Iterator-based lints #[derive(Copy, Clone)] struct IteratorFalsePositives { @@ -154,6 +164,10 @@ impl IteratorFalsePositives { fn rposition(self) -> Option { Some(self.foo) } + + fn nth(self, n: usize) -> Option { + Some(self.foo) + } } /// Checks implementation of `FILTER_NEXT` lint @@ -309,6 +323,19 @@ fn or_fun_call() { //~|SUGGESTION btree.entry(42).or_insert_with(String::new); } +/// Checks implementation of `ITER_NTH` lint +fn iter_nth() { + let some_vec = vec![0, 1, 2, 3]; + let bad_vec = some_vec.iter().nth(3); + //~^ERROR called `.iter().nth()` on a Vec. + let bad_slice = &some_vec[..].iter().nth(3); + //~^ERROR called `.iter().nth()` on a slice. + + let false_positive = HasIter; + let ok = false_positive.iter().nth(3); + // ^This should be okay, because false_positive is not a slice or Vec +} + #[allow(similar_names)] fn main() { use std::io;