Skip to content

Note that using enumerate() will swap the arguments #14969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 87 additions & 9 deletions clippy_lints/src/methods/range_zip_with_len.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::snippet;
use clippy_utils::{SpanlessEq, higher, is_integer_const, is_trait_method};
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::source::{SpanRangeExt as _, snippet_with_applicability};
use clippy_utils::{SpanlessEq, get_parent_expr, higher, is_integer_const, is_trait_method, sym};
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind, QPath};
use rustc_hir::{Expr, ExprKind, Node, Pat, PatKind, QPath};
use rustc_lint::LateContext;
use rustc_span::sym;

use super::RANGE_ZIP_WITH_LEN;

Expand All @@ -21,14 +20,93 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, recv: &'
&& let ExprKind::Path(QPath::Resolved(_, len_path)) = len_recv.kind
&& SpanlessEq::new(cx).eq_path_segments(iter_path.segments, len_path.segments)
{
span_lint_and_sugg(
span_lint_and_then(
cx,
RANGE_ZIP_WITH_LEN,
expr.span,
"using `.zip()` with a range and `.len()`",
"try",
format!("{}.iter().enumerate()", snippet(cx, recv.span, "_")),
Applicability::MachineApplicable,
|diag| {
// If the iterator content is consumed by a pattern with exactly two elements, swap
// the order of those elements. Otherwise, the suggestion will be marked as
// `Applicability::MaybeIncorrect` (because it will be), and a note will be added
// to the diagnostic to underline the swapping of the index and the content.
let pat = methods_pattern(cx, expr).or_else(|| for_loop_pattern(cx, expr));
let invert_bindings = if let Some(pat) = pat
&& pat.span.eq_ctxt(expr.span)
&& let PatKind::Tuple([first, second], _) = pat.kind
{
Some((first.span, second.span))
} else {
None
};
let mut app = Applicability::MachineApplicable;
let mut suggestions = vec![(
expr.span,
format!(
"{}.iter().enumerate()",
snippet_with_applicability(cx, recv.span, "_", &mut app)
),
)];
if let Some((left, right)) = invert_bindings
&& let Some(snip_left) = left.get_source_text(cx)
&& let Some(snip_right) = right.get_source_text(cx)
{
suggestions.extend([(left, snip_right.to_string()), (right, snip_left.to_string())]);
} else {
app = Applicability::MaybeIncorrect;
}
diag.multipart_suggestion("use", suggestions, app);
if app != Applicability::MachineApplicable {
diag.note("the order of the element and the index will be swapped");
}
},
);
}
}

/// If `expr` is the argument of a `for` loop, return the loop pattern.
fn for_loop_pattern<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx Pat<'tcx>> {
cx.tcx.hir_parent_iter(expr.hir_id).find_map(|(_, node)| {
if let Node::Expr(ancestor_expr) = node
&& let Some(for_loop) = higher::ForLoop::hir(ancestor_expr)
&& for_loop.arg.hir_id == expr.hir_id
{
Some(for_loop.pat)
} else {
None
}
})
}

/// If `expr` is the receiver of an `Iterator` method which consumes the iterator elements and feed
/// them to a closure, return the pattern of the closure.
fn methods_pattern<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx Pat<'tcx>> {
if let Some(parent_expr) = get_parent_expr(cx, expr)
&& is_trait_method(cx, expr, sym::Iterator)
&& let ExprKind::MethodCall(method, recv, [arg], _) = parent_expr.kind
&& recv.hir_id == expr.hir_id
&& matches!(
method.ident.name,
sym::all
| sym::any
| sym::filter_map
| sym::find_map
| sym::flat_map
| sym::for_each
| sym::is_partitioned
| sym::is_sorted_by_key
| sym::map
| sym::map_while
| sym::position
| sym::rposition
| sym::try_for_each
)
&& let ExprKind::Closure(closure) = arg.kind
&& let body = cx.tcx.hir_body(closure.body)
&& let [param] = body.params
{
Some(param.pat)
} else {
None
}
}
4 changes: 4 additions & 0 deletions clippy_utils/src/sym.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,10 @@ generate! {
is_none,
is_none_or,
is_ok,
is_partitioned,
is_some,
is_some_and,
is_sorted_by_key,
isqrt,
itertools,
join,
Expand All @@ -210,6 +212,7 @@ generate! {
map_continue,
map_or,
map_or_else,
map_while,
match_indices,
matches,
max,
Expand Down Expand Up @@ -349,6 +352,7 @@ generate! {
trim_start,
trim_start_matches,
truncate,
try_for_each,
unreachable_pub,
unsafe_removed_from_name,
unused,
Expand Down
16 changes: 14 additions & 2 deletions tests/ui/range.fixed
Original file line number Diff line number Diff line change
@@ -1,11 +1,23 @@
#![allow(clippy::useless_vec)]
#[warn(clippy::range_zip_with_len)]
fn main() {
let v1 = vec![1, 2, 3];
let v2 = vec![4, 5];
let v1: Vec<u64> = vec![1, 2, 3];
let v2: Vec<u64> = vec![4, 5];
let _x = v1.iter().enumerate();
//~^ range_zip_with_len

//~v range_zip_with_len
for (i, e) in v1.iter().enumerate() {
let _: &u64 = e;
let _: usize = i;
}

//~v range_zip_with_len
v1.iter().enumerate().for_each(|(i, e)| {
let _: &u64 = e;
let _: usize = i;
});

let _y = v1.iter().zip(0..v2.len()); // No error
}

Expand Down
16 changes: 14 additions & 2 deletions tests/ui/range.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,23 @@
#![allow(clippy::useless_vec)]
#[warn(clippy::range_zip_with_len)]
fn main() {
let v1 = vec![1, 2, 3];
let v2 = vec![4, 5];
let v1: Vec<u64> = vec![1, 2, 3];
let v2: Vec<u64> = vec![4, 5];
let _x = v1.iter().zip(0..v1.len());
//~^ range_zip_with_len

//~v range_zip_with_len
for (e, i) in v1.iter().zip(0..v1.len()) {
let _: &u64 = e;
let _: usize = i;
}

//~v range_zip_with_len
v1.iter().zip(0..v1.len()).for_each(|(e, i)| {
let _: &u64 = e;
let _: usize = i;
});

let _y = v1.iter().zip(0..v2.len()); // No error
}

Expand Down
29 changes: 27 additions & 2 deletions tests/ui/range.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,35 @@ error: using `.zip()` with a range and `.len()`
--> tests/ui/range.rs:6:14
|
LL | let _x = v1.iter().zip(0..v1.len());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `v1.iter().enumerate()`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `v1.iter().enumerate()`
|
= note: the order of the element and the index will be swapped
= note: `-D clippy::range-zip-with-len` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::range_zip_with_len)]`

error: aborting due to 1 previous error
error: using `.zip()` with a range and `.len()`
--> tests/ui/range.rs:10:19
|
LL | for (e, i) in v1.iter().zip(0..v1.len()) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: use
|
LL - for (e, i) in v1.iter().zip(0..v1.len()) {
LL + for (i, e) in v1.iter().enumerate() {
|

error: using `.zip()` with a range and `.len()`
--> tests/ui/range.rs:16:5
|
LL | v1.iter().zip(0..v1.len()).for_each(|(e, i)| {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: use
|
LL - v1.iter().zip(0..v1.len()).for_each(|(e, i)| {
LL + v1.iter().enumerate().for_each(|(i, e)| {
|

error: aborting due to 3 previous errors

14 changes: 14 additions & 0 deletions tests/ui/range_unfixable.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
//@no-rustfix
#![allow(clippy::useless_vec)]
#[warn(clippy::range_zip_with_len)]
fn main() {
let v1: Vec<u64> = vec![1, 2, 3];
let v2: Vec<u64> = vec![4, 5];

// Do not autofix, `filter()` would not consume the iterator.
//~v range_zip_with_len
v1.iter().zip(0..v1.len()).filter(|(_, i)| *i < 2).for_each(|(e, i)| {
let _: &u64 = e;
let _: usize = i;
});
}
12 changes: 12 additions & 0 deletions tests/ui/range_unfixable.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error: using `.zip()` with a range and `.len()`
--> tests/ui/range_unfixable.rs:10:5
|
LL | v1.iter().zip(0..v1.len()).filter(|(_, i)| *i < 2).for_each(|(e, i)| {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `v1.iter().enumerate()`
|
= note: the order of the element and the index will be swapped
= note: `-D clippy::range-zip-with-len` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::range_zip_with_len)]`

error: aborting due to 1 previous error