From 555e4555b15b26122ad084ad5983b565200c5992 Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Thu, 16 Jun 2016 01:29:03 -0700 Subject: [PATCH 1/3] Add tests for slice_iter_nth --- tests/compile-fail/methods.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 89267462f5df..591a0c1e3df8 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -309,6 +309,15 @@ fn or_fun_call() { //~|SUGGESTION btree.entry(42).or_insert_with(String::new); } +/// Checks implementation of `SLICE_ITER_NTH` lint +fn slice_iter_nth() { + let some_vec = vec![0, 1, 2, 3]; + let bad = &some_vec[..].iter().nth(3); + //~^ERROR called `.iter().nth()` on a slice. + + let ok = some_vec.iter().nth(3); // This should be okay, since some_vec is not a slice +} + #[allow(similar_names)] fn main() { use std::io; From 7764dc5ef42754c2704217ebec4a82b964835f54 Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Thu, 16 Jun 2016 01:36:11 -0700 Subject: [PATCH 2/3] Add slice_iter_nth lint --- CHANGELOG.md | 1 + README.md | 3 ++- clippy_lints/src/lib.rs | 1 + clippy_lints/src/methods.rs | 39 ++++++++++++++++++++++++++++++++++++- 4 files changed, 42 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 81102a900e5c..30702057bdbc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -234,6 +234,7 @@ All notable changes to this project will be documented in this file. [`single_char_pattern`]: https://github.com/Manishearth/rust-clippy/wiki#single_char_pattern [`single_match`]: https://github.com/Manishearth/rust-clippy/wiki#single_match [`single_match_else`]: https://github.com/Manishearth/rust-clippy/wiki#single_match_else +[`slice_iter_nth`]: https://github.com/Manishearth/rust-clippy/wiki#slice_iter_nth [`str_to_string`]: https://github.com/Manishearth/rust-clippy/wiki#str_to_string [`string_add`]: https://github.com/Manishearth/rust-clippy/wiki#string_add [`string_add_assign`]: https://github.com/Manishearth/rust-clippy/wiki#string_add_assign diff --git a/README.md b/README.md index ffda505eeac6..5707b0ee70cf 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 ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -140,6 +140,7 @@ name [single_char_pattern](https://github.com/Manishearth/rust-clippy/wiki#single_char_pattern) | warn | using a single-character str where a char could be used, e.g. `_.split("x")` [single_match](https://github.com/Manishearth/rust-clippy/wiki#single_match) | warn | a match statement with a single nontrivial arm (i.e, where the other arm is `_ => {}`) is used; recommends `if let` instead [single_match_else](https://github.com/Manishearth/rust-clippy/wiki#single_match_else) | allow | a match statement with a two arms where the second arm's pattern is a wildcard; recommends `if let` instead +[slice_iter_nth](https://github.com/Manishearth/rust-clippy/wiki#slice_iter_nth) | warn | using `.iter().nth()` on a slice [string_add](https://github.com/Manishearth/rust-clippy/wiki#string_add) | allow | using `x + ..` where x is a `String`; suggests using `push_str()` instead [string_add_assign](https://github.com/Manishearth/rust-clippy/wiki#string_add_assign) | allow | using `x = x + ..` where x is a `String`; suggests using `push_str()` instead [string_lit_as_bytes](https://github.com/Manishearth/rust-clippy/wiki#string_lit_as_bytes) | warn | calling `as_bytes` on a string literal; suggests using a byte string literal instead diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 42a35e7009b5..072326b1a087 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -357,6 +357,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { methods::SEARCH_IS_SOME, methods::SHOULD_IMPLEMENT_TRAIT, methods::SINGLE_CHAR_PATTERN, + methods::SLICE_ITER_NTH, methods::TEMPORARY_CSTRING_AS_PTR, methods::WRONG_SELF_CONVENTION, minmax::MIN_MAX, diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 9dcc8f9d016e..b7d98b5fdd64 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -312,6 +312,28 @@ declare_lint! { "getting the inner pointer of a temporary `CString`" } +/// **What it does:** This lint checks for use of `.iter().nth()` on a slice. +/// +/// **Why is this bad?** `.get()` is more efficient and more readable. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// let some_slice = &[0, 1, 2, 3][..]; +/// let third_elem = some_slice.iter().nth(3); +/// ``` +/// The correct use would be: +/// ```rust +/// let some_slice = &[0, 1, 2, 3][..]; +/// let third_elem = some_slice.get(3); +/// ``` +declare_lint! { + pub SLICE_ITER_NTH, + Warn, + "using `.iter().nth()` on a slice" +} + impl LintPass for MethodsPass { fn get_lints(&self) -> LintArray { lint_array!(EXTEND_FROM_SLICE, @@ -330,7 +352,8 @@ impl LintPass for MethodsPass { NEW_RET_NO_SELF, SINGLE_CHAR_PATTERN, SEARCH_IS_SOME, - TEMPORARY_CSTRING_AS_PTR) + TEMPORARY_CSTRING_AS_PTR, + SLICE_ITER_NTH) } } @@ -363,6 +386,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_slice_iter_nth(cx, expr, arglists[0]); } lint_or_fun_call(cx, expr, &name.node.as_str(), args); @@ -616,6 +641,18 @@ 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_slice_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, + SLICE_ITER_NTH, + expr.span, + "called `.iter().nth()` on a slice. 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 { From 74025be59db10a5c8f5c0b5175b250730b4d4ca6 Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Thu, 16 Jun 2016 02:02:00 -0700 Subject: [PATCH 3/3] Make iter_nth work for `Vec`s too --- CHANGELOG.md | 2 +- README.md | 2 +- clippy_lints/src/lib.rs | 2 +- clippy_lints/src/methods.rs | 33 +++++++++++++++++++++------------ tests/compile-fail/methods.rs | 26 ++++++++++++++++++++++---- 5 files changed, 46 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 30702057bdbc..790bbc04691b 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 @@ -234,7 +235,6 @@ All notable changes to this project will be documented in this file. [`single_char_pattern`]: https://github.com/Manishearth/rust-clippy/wiki#single_char_pattern [`single_match`]: https://github.com/Manishearth/rust-clippy/wiki#single_match [`single_match_else`]: https://github.com/Manishearth/rust-clippy/wiki#single_match_else -[`slice_iter_nth`]: https://github.com/Manishearth/rust-clippy/wiki#slice_iter_nth [`str_to_string`]: https://github.com/Manishearth/rust-clippy/wiki#str_to_string [`string_add`]: https://github.com/Manishearth/rust-clippy/wiki#string_add [`string_add_assign`]: https://github.com/Manishearth/rust-clippy/wiki#string_add_assign diff --git a/README.md b/README.md index 5707b0ee70cf..85e9ed70bf84 100644 --- a/README.md +++ b/README.md @@ -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 @@ -140,7 +141,6 @@ name [single_char_pattern](https://github.com/Manishearth/rust-clippy/wiki#single_char_pattern) | warn | using a single-character str where a char could be used, e.g. `_.split("x")` [single_match](https://github.com/Manishearth/rust-clippy/wiki#single_match) | warn | a match statement with a single nontrivial arm (i.e, where the other arm is `_ => {}`) is used; recommends `if let` instead [single_match_else](https://github.com/Manishearth/rust-clippy/wiki#single_match_else) | allow | a match statement with a two arms where the second arm's pattern is a wildcard; recommends `if let` instead -[slice_iter_nth](https://github.com/Manishearth/rust-clippy/wiki#slice_iter_nth) | warn | using `.iter().nth()` on a slice [string_add](https://github.com/Manishearth/rust-clippy/wiki#string_add) | allow | using `x + ..` where x is a `String`; suggests using `push_str()` instead [string_add_assign](https://github.com/Manishearth/rust-clippy/wiki#string_add_assign) | allow | using `x = x + ..` where x is a `String`; suggests using `push_str()` instead [string_lit_as_bytes](https://github.com/Manishearth/rust-clippy/wiki#string_lit_as_bytes) | warn | calling `as_bytes` on a string literal; suggests using a byte string literal instead diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 072326b1a087..d64f653dd4cc 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, @@ -357,7 +358,6 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { methods::SEARCH_IS_SOME, methods::SHOULD_IMPLEMENT_TRAIT, methods::SINGLE_CHAR_PATTERN, - methods::SLICE_ITER_NTH, methods::TEMPORARY_CSTRING_AS_PTR, methods::WRONG_SELF_CONVENTION, minmax::MIN_MAX, diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index b7d98b5fdd64..da1420c3add7 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -312,7 +312,7 @@ declare_lint! { "getting the inner pointer of a temporary `CString`" } -/// **What it does:** This lint checks for use of `.iter().nth()` on a slice. +/// **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. /// @@ -320,18 +320,20 @@ declare_lint! { /// /// **Example:** /// ```rust -/// let some_slice = &[0, 1, 2, 3][..]; -/// let third_elem = some_slice.iter().nth(3); +/// 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_slice = &[0, 1, 2, 3][..]; -/// let third_elem = some_slice.get(3); +/// 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 SLICE_ITER_NTH, + pub ITER_NTH, Warn, - "using `.iter().nth()` on a slice" + "using `.iter().nth()` on a slice or Vec" } impl LintPass for MethodsPass { @@ -353,7 +355,7 @@ impl LintPass for MethodsPass { SINGLE_CHAR_PATTERN, SEARCH_IS_SOME, TEMPORARY_CSTRING_AS_PTR, - SLICE_ITER_NTH) + ITER_NTH) } } @@ -387,7 +389,7 @@ impl LateLintPass for MethodsPass { } 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_slice_iter_nth(cx, expr, arglists[0]); + lint_iter_nth(cx, expr, arglists[0]); } lint_or_fun_call(cx, expr, &name.node.as_str(), args); @@ -643,14 +645,21 @@ 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_slice_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){ + // 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, - SLICE_ITER_NTH, + 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)> { diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 591a0c1e3df8..811b91161430 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,13 +323,17 @@ fn or_fun_call() { //~|SUGGESTION btree.entry(42).or_insert_with(String::new); } -/// Checks implementation of `SLICE_ITER_NTH` lint -fn slice_iter_nth() { +/// Checks implementation of `ITER_NTH` lint +fn iter_nth() { let some_vec = vec![0, 1, 2, 3]; - let bad = &some_vec[..].iter().nth(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 ok = some_vec.iter().nth(3); // This should be okay, since some_vec is not 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)]