From 8ef53bf1967dfef9d20204552b0725673356ca14 Mon Sep 17 00:00:00 2001 From: Brad Sherman Date: Sat, 28 Dec 2019 15:31:41 -0700 Subject: [PATCH 1/2] Fix existing iter-nth-zero violations --- clippy_lints/src/inherent_impl.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/inherent_impl.rs b/clippy_lints/src/inherent_impl.rs index 445891ff58a0..ff35b81042b7 100644 --- a/clippy_lints/src/inherent_impl.rs +++ b/clippy_lints/src/inherent_impl.rs @@ -61,7 +61,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MultipleInherentImpl { } fn check_crate_post(&mut self, cx: &LateContext<'a, 'tcx>, krate: &'tcx Crate<'_>) { - if let Some(item) = krate.items.values().nth(0) { + if let Some(item) = krate.items.values().next() { // Retrieve all inherent implementations from the crate, grouped by type for impls in cx .tcx @@ -71,7 +71,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MultipleInherentImpl { { // Filter out implementations that have generic params (type or lifetime) let mut impl_spans = impls.iter().filter_map(|impl_def| self.impls.get(impl_def)); - if let Some(initial_span) = impl_spans.nth(0) { + if let Some(initial_span) = impl_spans.next() { impl_spans.for_each(|additional_span| { span_lint_and_then( cx, From ab5ff0352e9c6d9a46b5557f68abba9c004e8b93 Mon Sep 17 00:00:00 2001 From: Brad Sherman Date: Sat, 28 Dec 2019 15:37:23 -0700 Subject: [PATCH 2/2] Add lint for iter.nth(0) - Encourage iter.next() rather than iter.nth(0), which is less readable --- CHANGELOG.md | 1 + README.md | 2 +- clippy_lints/src/lib.rs | 3 ++ clippy_lints/src/methods/mod.rs | 59 ++++++++++++++++++++++++++++++--- src/lintlist/mod.rs | 9 ++++- tests/ui/iter_nth_zero.fixed | 31 +++++++++++++++++ tests/ui/iter_nth_zero.rs | 31 +++++++++++++++++ tests/ui/iter_nth_zero.stderr | 22 ++++++++++++ 8 files changed, 152 insertions(+), 6 deletions(-) create mode 100644 tests/ui/iter_nth_zero.fixed create mode 100644 tests/ui/iter_nth_zero.rs create mode 100644 tests/ui/iter_nth_zero.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 05d437d3598c..874cb056bc9e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1141,6 +1141,7 @@ Released 2018-09-13 [`iter_cloned_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_cloned_collect [`iter_next_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_next_loop [`iter_nth`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth +[`iter_nth_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth_zero [`iter_skip_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_skip_next [`iterator_step_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iterator_step_by_zero [`just_underscores_and_digits`]: https://rust-lang.github.io/rust-clippy/master/index.html#just_underscores_and_digits diff --git a/README.md b/README.md index 215ad6f56301..3b4e22c3a392 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 343 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 344 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3aace11716e7..4a0976fe8007 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -618,6 +618,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf &methods::ITERATOR_STEP_BY_ZERO, &methods::ITER_CLONED_COLLECT, &methods::ITER_NTH, + &methods::ITER_NTH_ZERO, &methods::ITER_SKIP_NEXT, &methods::MANUAL_SATURATING_ARITHMETIC, &methods::MAP_FLATTEN, @@ -1197,6 +1198,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf LintId::of(&methods::ITERATOR_STEP_BY_ZERO), LintId::of(&methods::ITER_CLONED_COLLECT), LintId::of(&methods::ITER_NTH), + LintId::of(&methods::ITER_NTH_ZERO), LintId::of(&methods::ITER_SKIP_NEXT), LintId::of(&methods::MANUAL_SATURATING_ARITHMETIC), LintId::of(&methods::NEW_RET_NO_SELF), @@ -1375,6 +1377,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf LintId::of(&methods::CHARS_LAST_CMP), LintId::of(&methods::INTO_ITER_ON_REF), LintId::of(&methods::ITER_CLONED_COLLECT), + LintId::of(&methods::ITER_NTH_ZERO), LintId::of(&methods::ITER_SKIP_NEXT), LintId::of(&methods::MANUAL_SATURATING_ARITHMETIC), LintId::of(&methods::NEW_RET_NO_SELF), diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index d0021d129675..27a8b61fdbae 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -20,6 +20,7 @@ use rustc_span::source_map::Span; use rustc_span::symbol::{sym, Symbol, SymbolStr}; use syntax::ast; +use crate::consts::{constant, Constant}; use crate::utils::usage::mutated_variables; use crate::utils::{ get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, implements_trait, in_macro, is_copy, @@ -756,6 +757,33 @@ declare_clippy_lint! { "using `Iterator::step_by(0)`, which will panic at runtime" } +declare_clippy_lint! { + /// **What it does:** Checks for the use of `iter.nth(0)`. + /// + /// **Why is this bad?** `iter.nth(0)` is unnecessary, and `iter.next()` + /// is more readable. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// # use std::collections::HashSet; + /// // Bad + /// # let mut s = HashSet::new(); + /// # s.insert(1); + /// let x = s.iter().nth(0); + /// + /// // Good + /// # let mut s = HashSet::new(); + /// # s.insert(1); + /// let x = s.iter().next(); + /// ``` + pub ITER_NTH_ZERO, + style, + "replace `iter.nth(0)` with `iter.next()`" +} + declare_clippy_lint! { /// **What it does:** Checks for use of `.iter().nth()` (and the related /// `.iter_mut().nth()`) on standard library types with O(1) element access. @@ -1136,6 +1164,7 @@ declare_lint_pass!(Methods => [ MAP_FLATTEN, ITERATOR_STEP_BY_ZERO, ITER_NTH, + ITER_NTH_ZERO, ITER_SKIP_NEXT, GET_UNWRAP, STRING_EXTEND_CHARS, @@ -1191,8 +1220,9 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods { ["as_ptr", "unwrap"] | ["as_ptr", "expect"] => { lint_cstring_as_ptr(cx, expr, &arg_lists[1][0], &arg_lists[0][0]) }, - ["nth", "iter"] => lint_iter_nth(cx, expr, arg_lists[1], false), - ["nth", "iter_mut"] => lint_iter_nth(cx, expr, arg_lists[1], true), + ["nth", "iter"] => lint_iter_nth(cx, expr, &arg_lists, false), + ["nth", "iter_mut"] => lint_iter_nth(cx, expr, &arg_lists, true), + ["nth", ..] => lint_iter_nth_zero(cx, expr, arg_lists[0]), ["step_by", ..] => lint_step_by(cx, expr, arg_lists[0]), ["next", "skip"] => lint_iter_skip_next(cx, expr), ["collect", "cloned"] => lint_iter_cloned_collect(cx, expr, arg_lists[1]), @@ -1983,7 +2013,6 @@ fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr<'_>, fold_ar fn lint_step_by<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr<'_>, args: &'tcx [hir::Expr<'_>]) { if match_trait_method(cx, expr, &paths::ITERATOR) { - use crate::consts::{constant, Constant}; if let Some((Constant::Int(0), _)) = constant(cx, cx.tables, &args[1]) { span_lint( cx, @@ -1998,9 +2027,10 @@ fn lint_step_by<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr<'_>, args fn lint_iter_nth<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, expr: &hir::Expr<'_>, - iter_args: &'tcx [hir::Expr<'_>], + nth_and_iter_args: &[&'tcx [hir::Expr<'tcx>]], is_mut: bool, ) { + let iter_args = nth_and_iter_args[1]; let mut_str = if is_mut { "_mut" } else { "" }; let caller_type = if derefs_to_slice(cx, &iter_args[0], cx.tables.expr_ty(&iter_args[0])).is_some() { "slice" @@ -2009,6 +2039,8 @@ fn lint_iter_nth<'a, 'tcx>( } else if match_type(cx, cx.tables.expr_ty(&iter_args[0]), &paths::VEC_DEQUE) { "VecDeque" } else { + let nth_args = nth_and_iter_args[0]; + lint_iter_nth_zero(cx, expr, &nth_args); return; // caller is not a type that we want to lint }; @@ -2023,6 +2055,25 @@ fn lint_iter_nth<'a, 'tcx>( ); } +fn lint_iter_nth_zero<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr<'_>, nth_args: &'tcx [hir::Expr<'_>]) { + if_chain! { + if match_trait_method(cx, expr, &paths::ITERATOR); + if let Some((Constant::Int(0), _)) = constant(cx, cx.tables, &nth_args[1]); + then { + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + ITER_NTH_ZERO, + expr.span, + "called `.nth(0)` on a `std::iter::Iterator`", + "try calling", + format!("{}.next()", snippet_with_applicability(cx, nth_args[0].span, "..", &mut applicability)), + applicability, + ); + } + } +} + fn lint_get_unwrap<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, expr: &hir::Expr<'_>, diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index cd3336bdb378..5bc49f7b6002 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::Lint; pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 343] = [ +pub const ALL_LINTS: [Lint; 344] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -875,6 +875,13 @@ pub const ALL_LINTS: [Lint; 343] = [ deprecation: None, module: "methods", }, + Lint { + name: "iter_nth_zero", + group: "style", + desc: "replace `iter.nth(0)` with `iter.next()`", + deprecation: None, + module: "methods", + }, Lint { name: "iter_skip_next", group: "style", diff --git a/tests/ui/iter_nth_zero.fixed b/tests/ui/iter_nth_zero.fixed new file mode 100644 index 000000000000..b54147c94d19 --- /dev/null +++ b/tests/ui/iter_nth_zero.fixed @@ -0,0 +1,31 @@ +// run-rustfix + +#![warn(clippy::iter_nth_zero)] +use std::collections::HashSet; + +struct Foo {} + +impl Foo { + fn nth(&self, index: usize) -> usize { + index + 1 + } +} + +fn main() { + let f = Foo {}; + f.nth(0); // lint does not apply here + + let mut s = HashSet::new(); + s.insert(1); + let _x = s.iter().next(); + + let mut s2 = HashSet::new(); + s2.insert(2); + let mut iter = s2.iter(); + let _y = iter.next(); + + let mut s3 = HashSet::new(); + s3.insert(3); + let mut iter2 = s3.iter(); + let _unwrapped = iter2.next().unwrap(); +} diff --git a/tests/ui/iter_nth_zero.rs b/tests/ui/iter_nth_zero.rs new file mode 100644 index 000000000000..b92c7d18adb4 --- /dev/null +++ b/tests/ui/iter_nth_zero.rs @@ -0,0 +1,31 @@ +// run-rustfix + +#![warn(clippy::iter_nth_zero)] +use std::collections::HashSet; + +struct Foo {} + +impl Foo { + fn nth(&self, index: usize) -> usize { + index + 1 + } +} + +fn main() { + let f = Foo {}; + f.nth(0); // lint does not apply here + + let mut s = HashSet::new(); + s.insert(1); + let _x = s.iter().nth(0); + + let mut s2 = HashSet::new(); + s2.insert(2); + let mut iter = s2.iter(); + let _y = iter.nth(0); + + let mut s3 = HashSet::new(); + s3.insert(3); + let mut iter2 = s3.iter(); + let _unwrapped = iter2.nth(0).unwrap(); +} diff --git a/tests/ui/iter_nth_zero.stderr b/tests/ui/iter_nth_zero.stderr new file mode 100644 index 000000000000..2b20a4ceb4ab --- /dev/null +++ b/tests/ui/iter_nth_zero.stderr @@ -0,0 +1,22 @@ +error: called `.nth(0)` on a `std::iter::Iterator` + --> $DIR/iter_nth_zero.rs:20:14 + | +LL | let _x = s.iter().nth(0); + | ^^^^^^^^^^^^^^^ help: try calling: `s.iter().next()` + | + = note: `-D clippy::iter-nth-zero` implied by `-D warnings` + +error: called `.nth(0)` on a `std::iter::Iterator` + --> $DIR/iter_nth_zero.rs:25:14 + | +LL | let _y = iter.nth(0); + | ^^^^^^^^^^^ help: try calling: `iter.next()` + +error: called `.nth(0)` on a `std::iter::Iterator` + --> $DIR/iter_nth_zero.rs:30:22 + | +LL | let _unwrapped = iter2.nth(0).unwrap(); + | ^^^^^^^^^^^^ help: try calling: `iter2.next()` + +error: aborting due to 3 previous errors +