Auto merge of #12417 - Alexendoo:iter-nth, r=flip1995

Move `iter_nth` to `style`, add machine applicable suggestion

There's no `O(n)` involved with `.iter().nth()` on the linted types since the iterator implementations provide `nth` and/or `advance_by` that operate in `O(1)`

For slice iterators the codegen is equivalent, `VecDeque`'s iterator seems to codegen differently but that doesn't seem significant enough to keep it as a perf lint

changelog: [`iter_nth`] Move to `style`

r? `@flip1995`

Author: bors

clippy_lints/src/methods/iter_nth.rs

@@ -1,39 +1,44 @@
-use super::utils::derefs_to_slice;
-use crate::methods::iter_nth_zero;
-use clippy_utils::diagnostics::span_lint_and_help;
-use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::ty::get_type_diagnostic_name;
+use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_lint::LateContext;
 use rustc_span::symbol::sym;
+use rustc_span::Span;
 
 use super::ITER_NTH;
 
 pub(super) fn check<'tcx>(
     cx: &LateContext<'tcx>,
     expr: &hir::Expr<'_>,
     iter_recv: &'tcx hir::Expr<'tcx>,
-    nth_recv: &hir::Expr<'_>,
-    nth_arg: &hir::Expr<'_>,
-    is_mut: bool,
-) {
-    let mut_str = if is_mut { "_mut" } else { "" };
-    let caller_type = if derefs_to_slice(cx, iter_recv, cx.typeck_results().expr_ty(iter_recv)).is_some() {
-        "slice"
-    } else if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(iter_recv), sym::Vec) {
-        "`Vec`"
-    } else if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(iter_recv), sym::VecDeque) {
-        "`VecDeque`"
-    } else {
-        iter_nth_zero::check(cx, expr, nth_recv, nth_arg);
-        return; // caller is not a type that we want to lint
+    iter_method: &str,
+    iter_span: Span,
+    nth_span: Span,
+) -> bool {
+    let caller_type = match get_type_diagnostic_name(cx, cx.typeck_results().expr_ty(iter_recv).peel_refs()) {
+        Some(sym::Vec) => "`Vec`",
+        Some(sym::VecDeque) => "`VecDeque`",
+        _ if cx.typeck_results().expr_ty_adjusted(iter_recv).peel_refs().is_slice() => "slice",
+        // caller is not a type that we want to lint
+        _ => return false,
     };
 
-    span_lint_and_help(
+    span_lint_and_then(
         cx,
         ITER_NTH,
         expr.span,
-        &format!("called `.iter{mut_str}().nth()` on a {caller_type}"),
-        None,
-        &format!("calling `.get{mut_str}()` is both faster and more readable"),
+        &format!("called `.{iter_method}().nth()` on a {caller_type}"),
+        |diag| {
+            let get_method = if iter_method == "iter_mut" { "get_mut" } else { "get" };
+            diag.span_suggestion_verbose(
+                iter_span.to(nth_span),
+                format!("`{get_method}` is equivalent but more concise"),
+                get_method,
+                Applicability::MachineApplicable,
+            );
+        },
     );
+
+    true
 }

clippy_lints/src/methods/mod.rs

@@ -1236,12 +1236,11 @@ declare_clippy_lint! {
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Checks for usage of `.iter().nth()` (and the related
-    /// `.iter_mut().nth()`) on standard library types with *O*(1) element access.
+    /// Checks for usage of `.iter().nth()`/`.iter_mut().nth()` on standard library types that have
+    /// equivalent `.get()`/`.get_mut()` methods.
     ///
     /// ### Why is this bad?
-    /// `.get()` and `.get_mut()` are more efficient and more
-    /// readable.
+    /// `.get()` and `.get_mut()` are equivalent but more concise.
     ///
     /// ### Example
     /// ```no_run
@@ -1257,7 +1256,7 @@ declare_clippy_lint! {
     /// ```
     #[clippy::version = "pre 1.29.0"]
     pub ITER_NTH,
-    perf,
+    style,
     "using `.iter().nth()` on a standard library type with O(1) element access"
 }
 
@@ -4756,8 +4755,11 @@ impl Methods {
                         iter_overeager_cloned::Op::LaterCloned,
                         false,
                     ),
-                    Some(("iter", recv2, [], _, _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, false),
-                    Some(("iter_mut", recv2, [], _, _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, true),
+                    Some((iter_method @ ("iter" | "iter_mut"), iter_recv, [], iter_span, _)) => {
+                        if !iter_nth::check(cx, expr, iter_recv, iter_method, iter_span, span) {
+                            iter_nth_zero::check(cx, expr, recv, n_arg);
+                        }
+                    },
                     _ => iter_nth_zero::check(cx, expr, recv, n_arg),
                 },
                 ("ok_or_else", [arg]) => unnecessary_lazy_eval::check(cx, expr, recv, arg, "ok_or"),

tests/ui/iter_nth.fixed

@@ -0,0 +1,60 @@
+//@aux-build:option_helpers.rs
+
+#![warn(clippy::iter_nth)]
+#![allow(clippy::useless_vec)]
+
+#[macro_use]
+extern crate option_helpers;
+
+use option_helpers::IteratorFalsePositives;
+use std::collections::VecDeque;
+
+/// Struct to generate false positives for things with `.iter()`.
+#[derive(Copy, Clone)]
+struct HasIter;
+
+impl HasIter {
+    fn iter(self) -> IteratorFalsePositives {
+        IteratorFalsePositives { foo: 0 }
+    }
+
+    fn iter_mut(self) -> IteratorFalsePositives {
+        IteratorFalsePositives { foo: 0 }
+    }
+}
+
+/// Checks implementation of `ITER_NTH` lint.
+fn iter_nth() {
+    let mut some_vec = vec![0, 1, 2, 3];
+    let mut boxed_slice: Box<[u8]> = Box::new([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.get(3);
+        let bad_slice = &some_vec[..].get(3);
+        let bad_boxed_slice = boxed_slice.get(3);
+        let bad_vec_deque = some_vec_deque.get(3);
+    }
+
+    {
+        // Make sure we lint `.iter_mut()` for relevant types.
+        let bad_vec = some_vec.get_mut(3);
+    }
+    {
+        let bad_slice = &some_vec[..].get_mut(3);
+    }
+    {
+        let bad_vec_deque = some_vec_deque.get_mut(3);
+    }
+
+    let vec_ref = &Vec::<String>::new();
+    vec_ref.get(3);
+
+    // Make sure we don't lint for non-relevant types.
+    let false_positive = HasIter;
+    let ok = false_positive.iter().nth(3);
+    let ok_mut = false_positive.iter_mut().nth(3);
+}
+
+fn main() {}

tests/ui/iter_nth.rs

@@ -48,6 +48,9 @@ fn iter_nth() {
         let bad_vec_deque = some_vec_deque.iter_mut().nth(3);
     }
 
+    let vec_ref = &Vec::<String>::new();
+    vec_ref.iter().nth(3);
+
     // Make sure we don't lint for non-relevant types.
     let false_positive = HasIter;
     let ok = false_positive.iter().nth(3);

tests/ui/iter_nth.stderr

@@ -4,57 +4,89 @@ error: called `.iter().nth()` on a `Vec`
 LL |         let bad_vec = some_vec.iter().nth(3);
    |                       ^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: calling `.get()` is both faster and more readable
    = note: `-D clippy::iter-nth` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::iter_nth)]`
+help: `get` is equivalent but more concise
+   |
+LL |         let bad_vec = some_vec.get(3);
+   |                                ~~~
 
 error: called `.iter().nth()` on a slice
   --> tests/ui/iter_nth.rs:35:26
    |
 LL |         let bad_slice = &some_vec[..].iter().nth(3);
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: calling `.get()` is both faster and more readable
+help: `get` is equivalent but more concise
+   |
+LL |         let bad_slice = &some_vec[..].get(3);
+   |                                       ~~~
 
 error: called `.iter().nth()` on a slice
   --> tests/ui/iter_nth.rs:36:31
    |
 LL |         let bad_boxed_slice = boxed_slice.iter().nth(3);
    |                               ^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: calling `.get()` is both faster and more readable
+help: `get` is equivalent but more concise
+   |
+LL |         let bad_boxed_slice = boxed_slice.get(3);
+   |                                           ~~~
 
 error: called `.iter().nth()` on a `VecDeque`
   --> tests/ui/iter_nth.rs:37:29
    |
 LL |         let bad_vec_deque = some_vec_deque.iter().nth(3);
    |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: calling `.get()` is both faster and more readable
+help: `get` is equivalent but more concise
+   |
+LL |         let bad_vec_deque = some_vec_deque.get(3);
+   |                                            ~~~
 
 error: called `.iter_mut().nth()` on a `Vec`
   --> tests/ui/iter_nth.rs:42:23
    |
 LL |         let bad_vec = some_vec.iter_mut().nth(3);
    |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: calling `.get_mut()` is both faster and more readable
+help: `get_mut` is equivalent but more concise
+   |
+LL |         let bad_vec = some_vec.get_mut(3);
+   |                                ~~~~~~~
 
 error: called `.iter_mut().nth()` on a slice
   --> tests/ui/iter_nth.rs:45:26
    |
 LL |         let bad_slice = &some_vec[..].iter_mut().nth(3);
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: calling `.get_mut()` is both faster and more readable
+help: `get_mut` is equivalent but more concise
+   |
+LL |         let bad_slice = &some_vec[..].get_mut(3);
+   |                                       ~~~~~~~
 
 error: called `.iter_mut().nth()` on a `VecDeque`
   --> tests/ui/iter_nth.rs:48:29
    |
 LL |         let bad_vec_deque = some_vec_deque.iter_mut().nth(3);
    |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: calling `.get_mut()` is both faster and more readable
+help: `get_mut` is equivalent but more concise
+   |
+LL |         let bad_vec_deque = some_vec_deque.get_mut(3);
+   |                                            ~~~~~~~
+
+error: called `.iter().nth()` on a `Vec`
+  --> tests/ui/iter_nth.rs:52:5
+   |
+LL |     vec_ref.iter().nth(3);
+   |     ^^^^^^^^^^^^^^^^^^^^^
+   |
+help: `get` is equivalent but more concise
+   |
+LL |     vec_ref.get(3);
+   |             ~~~
 
-error: aborting due to 7 previous errors
+error: aborting due to 8 previous errors