Merge pull request #1278 from d-dorazio/1112

Suggest `nth(X)` instead of `skip(X).next()`

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

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

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,

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<sugg::Sugg<'static>> {
     fn may_slice(cx: &LateContext, ty: ty::Ty) -> bool {
         match ty.sty {

tests/compile-fail/methods.rs

@@ -173,6 +173,10 @@ impl IteratorFalsePositives {
     fn nth(self, n: usize) -> Option<u32> {
         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;