From 62831c6e295b8d6f23dca5bbf17022153ab11a9d Mon Sep 17 00:00:00 2001 From: d-dorazio Date: Fri, 14 Oct 2016 13:35:25 +0200 Subject: [PATCH] Suggest `nth(X)` instead of `skip(X).next()` --- CHANGELOG.md | 1 + README.md | 3 ++- clippy_lints/src/lib.rs | 1 + clippy_lints/src/methods.rs | 42 ++++++++++++++++++++++++++++++++++- tests/compile-fail/methods.rs | 26 ++++++++++++++++++++++ 5 files changed, 71 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f3c51a8bb94..4f7176f5bb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -266,6 +266,7 @@ All notable changes to this project will be documented in this file. [`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 +[`iter_skip_next`]: https://github.com/Manishearth/rust-clippy/wiki#iter_skip_next [`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 45f236aba4e..fee1c5427f2 100644 --- a/README.md +++ b/README.md @@ -174,7 +174,7 @@ You can check out this great service at [clippy.bashy.io](https://clippy.bashy.i ## Lints -There are 173 lints included in this crate: +There are 174 lints included in this crate: name | default | triggers on ---------------------------------------------------------------------------------------------------------------------|---------|---------------------------------------------------------------------------------------------------------------------------------- @@ -244,6 +244,7 @@ name [items_after_statements](https://github.com/Manishearth/rust-clippy/wiki#items_after_statements) | allow | 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 standard library type with O(1) element access +[iter_skip_next](https://github.com/Manishearth/rust-clippy/wiki#iter_skip_next) | warn | using `.skip(x).next()` on an iterator [len_without_is_empty](https://github.com/Manishearth/rust-clippy/wiki#len_without_is_empty) | warn | traits or impls with a public `len` method but no corresponding `is_empty` method [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 1d262622d12..8aa84082bce 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -372,6 +372,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { methods::EXTEND_FROM_SLICE, methods::FILTER_NEXT, methods::ITER_NTH, + methods::ITER_SKIP_NEXT, methods::NEW_RET_NO_SELF, methods::OK_EXPECT, methods::OR_FUN_CALL, diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 13664e7d912..4bf8e806b7b 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -440,6 +440,31 @@ declare_lint! { "using `.iter().nth()` on a standard library type with O(1) element access" } +/// **What it does:** Checks for use of `.skip(x).next()` on iterators. +/// +/// **Why is this bad?** `.nth(x)` is cleaner +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// let some_vec = vec![0, 1, 2, 3]; +/// let bad_vec = some_vec.iter().skip(3).next(); +/// let bad_slice = &some_vec[..].iter().skip(3).next(); +/// ``` +/// The correct use would be: +/// ```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); +/// ``` +declare_lint! { + pub ITER_SKIP_NEXT, + Warn, + "using `.skip(x).next()` on an iterator" +} + + impl LintPass for Pass { fn get_lints(&self) -> LintArray { lint_array!(EXTEND_FROM_SLICE, @@ -461,7 +486,8 @@ impl LintPass for Pass { TEMPORARY_CSTRING_AS_PTR, FILTER_NEXT, FILTER_MAP, - ITER_NTH) + ITER_NTH, + ITER_SKIP_NEXT) } } @@ -506,6 +532,8 @@ impl LateLintPass for Pass { lint_iter_nth(cx, expr, arglists[0], false); } else if let Some(arglists) = method_chain_args(expr, &["iter_mut", "nth"]) { lint_iter_nth(cx, expr, arglists[0], true); + } else if let Some(_) = method_chain_args(expr, &["skip", "next"]) { + lint_iter_skip_next(cx, expr); } lint_or_fun_call(cx, expr, &name.node.as_str(), args); @@ -790,6 +818,18 @@ fn lint_iter_nth(cx: &LateContext, expr: &hir::Expr, iter_args: &MethodArgs, is_ ); } +fn lint_iter_skip_next(cx: &LateContext, expr: &hir::Expr){ + // lint if caller of skip is an Iterator + if match_trait_method(cx, expr, &paths::ITERATOR) { + span_lint( + cx, + ITER_SKIP_NEXT, + expr.span, + "called `skip(x).next()` on an iterator. This is more succinctly expressed by calling `nth(x)`" + ); + } +} + fn derefs_to_slice(cx: &LateContext, expr: &hir::Expr, ty: ty::Ty) -> Option> { 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 0412dfecac1..a26a396526e 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -173,6 +173,10 @@ impl IteratorFalsePositives { fn nth(self, n: usize) -> Option { Some(self.foo) } + + fn skip(self, _: usize) -> IteratorFalsePositives { + self + } } /// Checks implementation of `FILTER_NEXT` lint @@ -363,6 +367,28 @@ fn iter_nth() { let ok_mut = false_positive.iter_mut().nth(3); } +/// Checks implementation of `ITER_SKIP_NEXT` lint +fn iter_skip_next() { + let mut some_vec = vec![0, 1, 2, 3]; + + let _ = some_vec.iter().skip(42).next(); + //~^ERROR called `skip(x).next()` on an iterator. This is more succinctly expressed by calling `nth(x)` + + let _ = some_vec.iter().cycle().skip(42).next(); + //~^ERROR called `skip(x).next()` on an iterator. This is more succinctly expressed by calling `nth(x)` + + let _ = (1..10).skip(10).next(); + //~^ERROR called `skip(x).next()` on an iterator. This is more succinctly expressed by calling `nth(x)` + + let _ = &some_vec[..].iter().skip(3).next(); + //~^ERROR called `skip(x).next()` on an iterator. This is more succinctly expressed by calling `nth(x)` + + let foo = IteratorFalsePositives { foo : 0 }; + let _ = foo.skip(42).next(); + let _ = foo.filter().skip(42).next(); +} + + #[allow(similar_names)] fn main() { use std::io;