Auto merge of #4966 - bradsherman:iter-nth-zero, r=flip1995

New Lint: Iter nth zero Check for the use of `iter.nth(0)` and encourage `iter.next()` instead as it is more readable changelog: add new lint when `iter.nth(0)` is used Fixes #4957
rust-lang · Jan 4, 2020 · d9d2013 · d9d2013
2 parents 05b4603 + ab5ff03
commit d9d2013
Show file tree

Hide file tree

Showing 9 changed files with 154 additions and 8 deletions.
diff --git 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
@@ -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/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,

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
+