From 12bc90d457594cca77edd430478e88c4a13ff201 Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Thu, 16 Jun 2016 14:30:05 -0700 Subject: [PATCH 1/3] Add tests for extend-iter-nth --- tests/compile-fail/methods.rs | 39 +++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 811b91161430..647cca5a39cc 100644 --- a/tests/compile-fail/methods.rs +++ b/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)] From cfa0c5782eabd49b896e287f6a67dc7f3a529d98 Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Thu, 16 Jun 2016 14:46:29 -0700 Subject: [PATCH 2/3] Extend iter_nth lint to work with iter_mut() and VecDeque --- README.md | 2 +- clippy_lints/src/methods.rs | 39 ++++++++++++++++++++++++------------- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index a8e8cbb3a5ad..f410ff990d9c 100644 --- a/README.md +++ b/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 diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 45d2e259dfad..2256c8a23fd0 100644 --- a/clippy_lints/src/methods.rs +++ b/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,31 @@ 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){ +fn lint_iter_nth(cx: &LateContext, expr: &hir::Expr, iter_args: &MethodArgs, is_mut: bool){ // lint if the caller of `.iter().nth()` is a `slice` + 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"; } + // lint if the caller of `.iter().nth()` is a `VecDeque` + 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)> { From 0e04153a7051135dafd8c86b5eab3f591639a96b Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Thu, 16 Jun 2016 14:51:16 -0700 Subject: [PATCH 3/3] Remove uneccessary, leftover comments in lint_iter_mut() --- clippy_lints/src/methods.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 2256c8a23fd0..795e4e18d878 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -649,17 +649,14 @@ 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, is_mut: bool){ - // lint if the caller of `.iter().nth()` is a `slice` 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])) { 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) { caller_type = "Vec"; } - // lint if the caller of `.iter().nth()` is a `VecDeque` else if match_type(cx, cx.tcx.expr_ty(&iter_args[0]), &paths::VEC_DEQUE) { caller_type = "VecDeque"; }