Merge pull request #1022 from devonhollowood/extend-iter-nth

Extend iter nth

Author: mcarton

README.md

@@ -78,7 +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
+[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
 [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

clippy_lints/src/methods.rs

@@ -312,9 +312,10 @@ 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.
+/// **What it does:** This lint checks for use of `.iter().nth()` (and the related
+/// `.iter_mut().nth()`) on standard library types with O(1) element access.
 ///
-/// **Why is this bad?** `.get()` is more efficient and more readable.
+/// **Why is this bad?** `.get()` and `.get_mut()` are more efficient and more readable.
 ///
 /// **Known problems:** None.
 ///
@@ -333,7 +334,7 @@ declare_lint! {
 declare_lint! {
     pub ITER_NTH,
     Warn,
-    "using `.iter().nth()` on a slice or Vec"
+    "using `.iter().nth()` on a standard library type with O(1) element access"
 }
 
 impl LintPass for Pass {
@@ -389,7 +390,9 @@ impl LateLintPass for Pass {
                 } 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_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);
                 }
 
                 lint_or_fun_call(cx, expr, &name.node.as_str(), args);
@@ -645,21 +648,28 @@ 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`
+fn lint_iter_nth(cx: &LateContext, expr: &hir::Expr, iter_args: &MethodArgs, is_mut: bool){
+    let caller_type;
+    let mut_str = if is_mut { "_mut" } else {""};
     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");
+        caller_type = "slice";
     }
-    // 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");
+        caller_type = "Vec";
+    }
+    else if match_type(cx, cx.tcx.expr_ty(&iter_args[0]), &paths::VEC_DEQUE) {
+        caller_type = "VecDeque";
+    }
+    else {
+        return; // caller is not a type that we want to lint
     }
+    span_lint(
+        cx,
+        ITER_NTH,
+        expr.span,
+        &format!("called `.iter{0}().nth()` on a {1}. Calling `.get{0}()` is both faster and more readable",
+                 mut_str, caller_type)
+    );
 }
 
 fn derefs_to_slice(cx: &LateContext, expr: &hir::Expr, ty: &ty::Ty) -> Option<(Span, &'static str)> {

tests/compile-fail/methods.rs

@@ -8,6 +8,7 @@
 use std::collections::BTreeMap;
 use std::collections::HashMap;
 use std::collections::HashSet;
+use std::collections::VecDeque;
 use std::ops::Mul;
 
 struct T;
@@ -136,6 +137,10 @@ impl HasIter {
     fn iter(self) -> IteratorFalsePositives {
         IteratorFalsePositives { foo: 0 }
     }
+
+    fn iter_mut(self) -> IteratorFalsePositives {
+        IteratorFalsePositives { foo: 0 }
+    }
 }
 
 /// Struct to generate false positive for Iterator-based lints
@@ -325,15 +330,37 @@ fn or_fun_call() {
 
 /// 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 mut some_vec = vec![0, 1, 2, 3];
+    let mut some_vec_deque: VecDeque<_> = some_vec.iter().cloned().collect();
+
+    {
+        // Make sure we lint `.iter()` for relevant types
+        let bad_vec = some_vec.iter().nth(3);
+        //~^ERROR called `.iter().nth()` on a Vec. Calling `.get()` is both faster and more readable
+        let bad_slice = &some_vec[..].iter().nth(3);
+        //~^ERROR called `.iter().nth()` on a slice. Calling `.get()` is both faster and more readable
+        let bad_vec_deque = some_vec_deque.iter().nth(3);
+        //~^ERROR called `.iter().nth()` on a VecDeque. Calling `.get()` is both faster and more readable
+    }
+
+    {
+        // Make sure we lint `.iter_mut()` for relevant types
+        let bad_vec = some_vec.iter_mut().nth(3);
+        //~^ERROR called `.iter_mut().nth()` on a Vec. Calling `.get_mut()` is both faster and more readable
+    }
+    {
+        let bad_slice = &some_vec[..].iter_mut().nth(3);
+        //~^ERROR called `.iter_mut().nth()` on a slice. Calling `.get_mut()` is both faster and more readable
+    }
+    {
+        let bad_vec_deque = some_vec_deque.iter_mut().nth(3);
+        //~^ERROR called `.iter_mut().nth()` on a VecDeque. Calling `.get_mut()` is both faster and more readable
+    }
 
+    // Make sure we don't lint for non-relevant types
     let false_positive = HasIter;
     let ok = false_positive.iter().nth(3);
-    // ^This should be okay, because false_positive is not a slice or Vec
+    let ok_mut = false_positive.iter_mut().nth(3);
 }
 
 #[allow(similar_names)]