Skip to content

Commit

Permalink
Move iter_nth to style, add machine applicable suggestion
Browse files Browse the repository at this point in the history
  • Loading branch information
Alexendoo committed Mar 4, 2024
1 parent 5214ab5 commit 301d293
Show file tree
Hide file tree
Showing 5 changed files with 139 additions and 37 deletions.
49 changes: 27 additions & 22 deletions clippy_lints/src/methods/iter_nth.rs
Original file line number Diff line number Diff line change
@@ -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
}
16 changes: 9 additions & 7 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"
}

Expand Down Expand Up @@ -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"),
Expand Down
60 changes: 60 additions & 0 deletions tests/ui/iter_nth.fixed
Original file line number Diff line number Diff line change
@@ -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() {}
3 changes: 3 additions & 0 deletions tests/ui/iter_nth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
48 changes: 40 additions & 8 deletions tests/ui/iter_nth.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 301d293

Please sign in to comment.