diff options
| -rw-r--r-- | clippy_lints/src/methods/iter_nth.rs | 49 | ||||
| -rw-r--r-- | clippy_lints/src/methods/mod.rs | 16 | ||||
| -rw-r--r-- | tests/ui/iter_nth.fixed | 60 | ||||
| -rw-r--r-- | tests/ui/iter_nth.rs | 3 | ||||
| -rw-r--r-- | tests/ui/iter_nth.stderr | 48 |
5 files changed, 139 insertions, 37 deletions
diff --git a/clippy_lints/src/methods/iter_nth.rs b/clippy_lints/src/methods/iter_nth.rs index 12104310405..5b0b70b4b96 100644 --- a/clippy_lints/src/methods/iter_nth.rs +++ b/clippy_lints/src/methods/iter_nth.rs @@ -1,10 +1,10 @@ -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; @@ -12,28 +12,33 @@ 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 } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 8a24ccea3a1..5445391c035 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1235,12 +1235,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 @@ -1256,7 +1255,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" } @@ -4723,8 +4722,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"), diff --git a/tests/ui/iter_nth.fixed b/tests/ui/iter_nth.fixed new file mode 100644 index 00000000000..aff3731a883 --- /dev/null +++ b/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() {} diff --git a/tests/ui/iter_nth.rs b/tests/ui/iter_nth.rs index 7c567bb81d8..89d68044ddd 100644 --- a/tests/ui/iter_nth.rs +++ b/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); diff --git a/tests/ui/iter_nth.stderr b/tests/ui/iter_nth.stderr index c5dd0c99727..178463f5347 100644 --- a/tests/ui/iter_nth.stderr +++ b/tests/ui/iter_nth.stderr @@ -4,9 +4,12 @@ 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 @@ -14,7 +17,10 @@ error: called `.iter().nth()` on a slice 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 @@ -22,7 +28,10 @@ error: called `.iter().nth()` on a slice 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 @@ -30,7 +39,10 @@ error: called `.iter().nth()` on a `VecDeque` 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 @@ -38,7 +50,10 @@ error: called `.iter_mut().nth()` on a `Vec` 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 @@ -46,7 +61,10 @@ error: called `.iter_mut().nth()` on a slice 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 @@ -54,7 +72,21 @@ error: called `.iter_mut().nth()` on a `VecDeque` 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 |