Skip to content

Commit 5a11fef

Browse files
committed
Auto merge of #12432 - Ethiraric:fix-12411, r=y21
[`unused_enumerate_index`]: trigger on method calls The lint used to check for patterns looking like: ```rs for (_, x) in some_iter.enumerate() { // Index is ignored } ``` This commit further checks for chained method calls constructs where we can detect that the index is unused. Currently, this checks only for the following patterns: ```rs some_iter.enumerate().map_function(|(_, x)| ..) let x = some_iter.enumerate(); x.map_function(|(_, x)| ..) ``` where `map_function` is one of `all`, `any`, `filter_map`, `find_map`, `flat_map`, `for_each` or `map`. Fixes #12411. *Please write a short comment explaining your change (or "none" for internal only changes)* changelog: [`unused_enumerate_index`]: add detection for method chains such as `iter.enumerate().map(|(_, x)| x)`
2 parents b667d02 + dadcd94 commit 5a11fef

File tree

7 files changed

+380
-84
lines changed

7 files changed

+380
-84
lines changed
Lines changed: 30 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1,62 +1,41 @@
11
use super::UNUSED_ENUMERATE_INDEX;
22
use clippy_utils::diagnostics::{multispan_sugg, span_lint_and_then};
33
use clippy_utils::source::snippet;
4-
use clippy_utils::{pat_is_wild, sugg};
4+
use clippy_utils::{match_def_path, pat_is_wild, sugg};
55
use rustc_hir::def::DefKind;
66
use rustc_hir::{Expr, ExprKind, Pat, PatKind};
77
use rustc_lint::LateContext;
88
use rustc_middle::ty;
99

1010
/// Checks for the `UNUSED_ENUMERATE_INDEX` lint.
11-
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, arg: &'tcx Expr<'_>, body: &'tcx Expr<'_>) {
12-
let PatKind::Tuple([index, elem], _) = pat.kind else {
13-
return;
14-
};
15-
16-
let ExprKind::MethodCall(_method, self_arg, [], _) = arg.kind else {
17-
return;
18-
};
19-
20-
let ty = cx.typeck_results().expr_ty(arg);
21-
22-
if !pat_is_wild(cx, &index.kind, body) {
23-
return;
24-
}
25-
26-
let name = match *ty.kind() {
27-
ty::Adt(base, _substs) => cx.tcx.def_path_str(base.did()),
28-
_ => return,
29-
};
30-
31-
if name != "std::iter::Enumerate" && name != "core::iter::Enumerate" {
32-
return;
11+
///
12+
/// The lint is also partially implemented in `clippy_lints/src/methods/unused_enumerate_index.rs`.
13+
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, pat: &Pat<'tcx>, arg: &Expr<'_>, body: &'tcx Expr<'tcx>) {
14+
if let PatKind::Tuple([index, elem], _) = pat.kind
15+
&& let ExprKind::MethodCall(_method, self_arg, [], _) = arg.kind
16+
&& let ty = cx.typeck_results().expr_ty(arg)
17+
&& pat_is_wild(cx, &index.kind, body)
18+
&& let ty::Adt(base, _) = *ty.kind()
19+
&& match_def_path(cx, base.did(), &clippy_utils::paths::CORE_ITER_ENUMERATE_STRUCT)
20+
&& let Some((DefKind::AssocFn, call_id)) = cx.typeck_results().type_dependent_def(arg.hir_id)
21+
&& match_def_path(cx, call_id, &clippy_utils::paths::CORE_ITER_ENUMERATE_METHOD)
22+
{
23+
span_lint_and_then(
24+
cx,
25+
UNUSED_ENUMERATE_INDEX,
26+
arg.span,
27+
"you seem to use `.enumerate()` and immediately discard the index",
28+
|diag| {
29+
let base_iter = sugg::Sugg::hir(cx, self_arg, "base iter");
30+
multispan_sugg(
31+
diag,
32+
"remove the `.enumerate()` call",
33+
vec![
34+
(pat.span, snippet(cx, elem.span, "..").into_owned()),
35+
(arg.span, base_iter.to_string()),
36+
],
37+
);
38+
},
39+
);
3340
}
34-
35-
let Some((DefKind::AssocFn, call_id)) = cx.typeck_results().type_dependent_def(arg.hir_id) else {
36-
return;
37-
};
38-
39-
let call_name = cx.tcx.def_path_str(call_id);
40-
41-
if call_name != "std::iter::Iterator::enumerate" && call_name != "core::iter::Iterator::enumerate" {
42-
return;
43-
}
44-
45-
span_lint_and_then(
46-
cx,
47-
UNUSED_ENUMERATE_INDEX,
48-
arg.span,
49-
"you seem to use `.enumerate()` and immediately discard the index",
50-
|diag| {
51-
let base_iter = sugg::Sugg::hir(cx, self_arg, "base iter");
52-
multispan_sugg(
53-
diag,
54-
"remove the `.enumerate()` call",
55-
vec![
56-
(pat.span, snippet(cx, elem.span, "..").into_owned()),
57-
(arg.span, base_iter.to_string()),
58-
],
59-
);
60-
},
61-
);
6241
}

clippy_lints/src/methods/mod.rs

Lines changed: 40 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,7 @@ mod unnecessary_literal_unwrap;
119119
mod unnecessary_result_map_or_else;
120120
mod unnecessary_sort_by;
121121
mod unnecessary_to_owned;
122+
mod unused_enumerate_index;
122123
mod unwrap_expect_used;
123124
mod useless_asref;
124125
mod utils;
@@ -4429,6 +4430,7 @@ impl Methods {
44294430
zst_offset::check(cx, expr, recv);
44304431
},
44314432
("all", [arg]) => {
4433+
unused_enumerate_index::check(cx, expr, recv, arg);
44324434
if let Some(("cloned", recv2, [], _, _)) = method_call(recv) {
44334435
iter_overeager_cloned::check(
44344436
cx,
@@ -4447,23 +4449,26 @@ impl Methods {
44474449
unnecessary_lazy_eval::check(cx, expr, recv, arg, "and");
44484450
}
44494451
},
4450-
("any", [arg]) => match method_call(recv) {
4451-
Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(
4452-
cx,
4453-
expr,
4454-
recv,
4455-
recv2,
4456-
iter_overeager_cloned::Op::NeedlessMove(arg),
4457-
false,
4458-
),
4459-
Some(("chars", recv, _, _, _))
4460-
if let ExprKind::Closure(arg) = arg.kind
4461-
&& let body = cx.tcx.hir().body(arg.body)
4462-
&& let [param] = body.params =>
4463-
{
4464-
string_lit_chars_any::check(cx, expr, recv, param, peel_blocks(body.value), &self.msrv);
4465-
},
4466-
_ => {},
4452+
("any", [arg]) => {
4453+
unused_enumerate_index::check(cx, expr, recv, arg);
4454+
match method_call(recv) {
4455+
Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(
4456+
cx,
4457+
expr,
4458+
recv,
4459+
recv2,
4460+
iter_overeager_cloned::Op::NeedlessMove(arg),
4461+
false,
4462+
),
4463+
Some(("chars", recv, _, _, _))
4464+
if let ExprKind::Closure(arg) = arg.kind
4465+
&& let body = cx.tcx.hir().body(arg.body)
4466+
&& let [param] = body.params =>
4467+
{
4468+
string_lit_chars_any::check(cx, expr, recv, param, peel_blocks(body.value), &self.msrv);
4469+
},
4470+
_ => {},
4471+
}
44674472
},
44684473
("arg", [arg]) => {
44694474
suspicious_command_arg_space::check(cx, recv, arg, span);
@@ -4596,14 +4601,17 @@ impl Methods {
45964601
}
45974602
},
45984603
("filter_map", [arg]) => {
4604+
unused_enumerate_index::check(cx, expr, recv, arg);
45994605
unnecessary_filter_map::check(cx, expr, arg, name);
46004606
filter_map_bool_then::check(cx, expr, arg, call_span);
46014607
filter_map_identity::check(cx, expr, arg, span);
46024608
},
46034609
("find_map", [arg]) => {
4610+
unused_enumerate_index::check(cx, expr, recv, arg);
46044611
unnecessary_filter_map::check(cx, expr, arg, name);
46054612
},
46064613
("flat_map", [arg]) => {
4614+
unused_enumerate_index::check(cx, expr, recv, arg);
46074615
flat_map_identity::check(cx, expr, arg, span);
46084616
flat_map_option::check(cx, expr, arg, span);
46094617
},
@@ -4625,17 +4633,20 @@ impl Methods {
46254633
manual_try_fold::check(cx, expr, init, acc, call_span, &self.msrv);
46264634
unnecessary_fold::check(cx, expr, init, acc, span);
46274635
},
4628-
("for_each", [arg]) => match method_call(recv) {
4629-
Some(("inspect", _, [_], span2, _)) => inspect_for_each::check(cx, expr, span2),
4630-
Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(
4631-
cx,
4632-
expr,
4633-
recv,
4634-
recv2,
4635-
iter_overeager_cloned::Op::NeedlessMove(arg),
4636-
false,
4637-
),
4638-
_ => {},
4636+
("for_each", [arg]) => {
4637+
unused_enumerate_index::check(cx, expr, recv, arg);
4638+
match method_call(recv) {
4639+
Some(("inspect", _, [_], span2, _)) => inspect_for_each::check(cx, expr, span2),
4640+
Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(
4641+
cx,
4642+
expr,
4643+
recv,
4644+
recv2,
4645+
iter_overeager_cloned::Op::NeedlessMove(arg),
4646+
false,
4647+
),
4648+
_ => {},
4649+
}
46394650
},
46404651
("get", [arg]) => {
46414652
get_first::check(cx, expr, recv, arg);
@@ -4682,6 +4693,7 @@ impl Methods {
46824693
},
46834694
(name @ ("map" | "map_err"), [m_arg]) => {
46844695
if name == "map" {
4696+
unused_enumerate_index::check(cx, expr, recv, m_arg);
46854697
map_clone::check(cx, expr, recv, m_arg, &self.msrv);
46864698
match method_call(recv) {
46874699
Some((map_name @ ("iter" | "into_iter"), recv2, _, _, _)) => {
Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
use clippy_utils::diagnostics::{multispan_sugg_with_applicability, span_lint_hir_and_then};
2+
use clippy_utils::paths::{CORE_ITER_ENUMERATE_METHOD, CORE_ITER_ENUMERATE_STRUCT};
3+
use clippy_utils::source::{snippet, snippet_opt};
4+
use clippy_utils::{expr_or_init, is_trait_method, match_def_path, pat_is_wild};
5+
use rustc_errors::Applicability;
6+
use rustc_hir::{Expr, ExprKind, FnDecl, PatKind, TyKind};
7+
use rustc_lint::LateContext;
8+
use rustc_middle::ty::AdtDef;
9+
use rustc_span::{sym, Span};
10+
11+
use crate::loops::UNUSED_ENUMERATE_INDEX;
12+
13+
/// Check for the `UNUSED_ENUMERATE_INDEX` lint outside of loops.
14+
///
15+
/// The lint is declared in `clippy_lints/src/loops/mod.rs`. There, the following pattern is
16+
/// checked:
17+
/// ```ignore
18+
/// for (_, x) in some_iter.enumerate() {
19+
/// // Index is ignored
20+
/// }
21+
/// ```
22+
///
23+
/// This `check` function checks for chained method calls constructs where we can detect that the
24+
/// index is unused. Currently, this checks only for the following patterns:
25+
/// ```ignore
26+
/// some_iter.enumerate().map_function(|(_, x)| ..)
27+
/// let x = some_iter.enumerate();
28+
/// x.map_function(|(_, x)| ..)
29+
/// ```
30+
/// where `map_function` is one of `all`, `any`, `filter_map`, `find_map`, `flat_map`, `for_each` or
31+
/// `map`.
32+
///
33+
/// # Preconditions
34+
/// This function must be called not on the `enumerate` call expression itself, but on any of the
35+
/// map functions listed above. It will ensure that `recv` is a `std::iter::Enumerate` instance and
36+
/// that the method call is one of the `std::iter::Iterator` trait.
37+
///
38+
/// * `call_expr`: The map function call expression
39+
/// * `recv`: The receiver of the call
40+
/// * `closure_arg`: The argument to the map function call containing the closure/function to apply
41+
pub(super) fn check(cx: &LateContext<'_>, call_expr: &Expr<'_>, recv: &Expr<'_>, closure_arg: &Expr<'_>) {
42+
let recv_ty = cx.typeck_results().expr_ty(recv);
43+
if let Some(recv_ty_defid) = recv_ty.ty_adt_def().map(AdtDef::did)
44+
// If we call a method on a `std::iter::Enumerate` instance
45+
&& match_def_path(cx, recv_ty_defid, &CORE_ITER_ENUMERATE_STRUCT)
46+
// If we are calling a method of the `Iterator` trait
47+
&& is_trait_method(cx, call_expr, sym::Iterator)
48+
// And the map argument is a closure
49+
&& let ExprKind::Closure(closure) = closure_arg.kind
50+
&& let closure_body = cx.tcx.hir().body(closure.body)
51+
// And that closure has one argument ...
52+
&& let [closure_param] = closure_body.params
53+
// .. which is a tuple of 2 elements
54+
&& let PatKind::Tuple([index, elem], ..) = closure_param.pat.kind
55+
// And that the first element (the index) is either `_` or unused in the body
56+
&& pat_is_wild(cx, &index.kind, closure_body)
57+
// Try to find the initializer for `recv`. This is needed in case `recv` is a local_binding. In the
58+
// first example below, `expr_or_init` would return `recv`.
59+
// ```
60+
// iter.enumerate().map(|(_, x)| x)
61+
// ^^^^^^^^^^^^^^^^ `recv`, a call to `std::iter::Iterator::enumerate`
62+
//
63+
// let binding = iter.enumerate();
64+
// ^^^^^^^^^^^^^^^^ `recv_init_expr`
65+
// binding.map(|(_, x)| x)
66+
// ^^^^^^^ `recv`, not a call to `std::iter::Iterator::enumerate`
67+
// ```
68+
&& let recv_init_expr = expr_or_init(cx, recv)
69+
// Make sure the initializer is a method call. It may be that the `Enumerate` comes from something
70+
// that we cannot control.
71+
// This would for instance happen with:
72+
// ```
73+
// external_lib::some_function_returning_enumerate().map(|(_, x)| x)
74+
// ```
75+
&& let ExprKind::MethodCall(_, enumerate_recv, _, enumerate_span) = recv_init_expr.kind
76+
&& let Some(enumerate_defid) = cx.typeck_results().type_dependent_def_id(recv_init_expr.hir_id)
77+
// Make sure the method call is `std::iter::Iterator::enumerate`.
78+
&& match_def_path(cx, enumerate_defid, &CORE_ITER_ENUMERATE_METHOD)
79+
{
80+
// Check if the tuple type was explicit. It may be the type system _needs_ the type of the element
81+
// that would be explicited in the closure.
82+
let new_closure_param = match find_elem_explicit_type_span(closure.fn_decl) {
83+
// We have an explicit type. Get its snippet, that of the binding name, and do `binding: ty`.
84+
// Fallback to `..` if we fail getting either snippet.
85+
Some(ty_span) => snippet_opt(cx, elem.span)
86+
.and_then(|binding_name| snippet_opt(cx, ty_span).map(|ty_name| format!("{binding_name}: {ty_name}")))
87+
.unwrap_or_else(|| "..".to_string()),
88+
// Otherwise, we have no explicit type. We can replace with the binding name of the element.
89+
None => snippet(cx, elem.span, "..").into_owned(),
90+
};
91+
92+
// Suggest removing the tuple from the closure and the preceding call to `enumerate`, whose span we
93+
// can get from the `MethodCall`.
94+
span_lint_hir_and_then(
95+
cx,
96+
UNUSED_ENUMERATE_INDEX,
97+
recv_init_expr.hir_id,
98+
enumerate_span,
99+
"you seem to use `.enumerate()` and immediately discard the index",
100+
|diag| {
101+
multispan_sugg_with_applicability(
102+
diag,
103+
"remove the `.enumerate()` call",
104+
Applicability::MachineApplicable,
105+
vec![
106+
(closure_param.span, new_closure_param),
107+
(
108+
enumerate_span.with_lo(enumerate_recv.span.source_callsite().hi()),
109+
String::new(),
110+
),
111+
],
112+
);
113+
},
114+
);
115+
}
116+
}
117+
118+
/// Find the span of the explicit type of the element.
119+
///
120+
/// # Returns
121+
/// If the tuple argument:
122+
/// * Has no explicit type, returns `None`
123+
/// * Has an explicit tuple type with an implicit element type (`(usize, _)`), returns `None`
124+
/// * Has an explicit tuple type with an explicit element type (`(_, i32)`), returns the span for
125+
/// the element type.
126+
fn find_elem_explicit_type_span(fn_decl: &FnDecl<'_>) -> Option<Span> {
127+
if let [tuple_ty] = fn_decl.inputs
128+
&& let TyKind::Tup([_idx_ty, elem_ty]) = tuple_ty.kind
129+
&& !matches!(elem_ty.kind, TyKind::Err(..) | TyKind::Infer)
130+
{
131+
Some(elem_ty.span)
132+
} else {
133+
None
134+
}
135+
}

clippy_utils/src/paths.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ pub const BTREESET_ITER: [&str; 6] = ["alloc", "collections", "btree", "set", "B
1919
pub const CLONE_TRAIT_METHOD: [&str; 4] = ["core", "clone", "Clone", "clone"];
2020
pub const CORE_ITER_CLONED: [&str; 6] = ["core", "iter", "traits", "iterator", "Iterator", "cloned"];
2121
pub const CORE_ITER_COPIED: [&str; 6] = ["core", "iter", "traits", "iterator", "Iterator", "copied"];
22+
pub const CORE_ITER_ENUMERATE_METHOD: [&str; 6] = ["core", "iter", "traits", "iterator", "Iterator", "enumerate"];
23+
pub const CORE_ITER_ENUMERATE_STRUCT: [&str; 5] = ["core", "iter", "adapters", "enumerate", "Enumerate"];
2224
pub const CORE_ITER_FILTER: [&str; 6] = ["core", "iter", "traits", "iterator", "Iterator", "filter"];
2325
pub const CORE_RESULT_OK_METHOD: [&str; 4] = ["core", "result", "Result", "ok"];
2426
pub const CSTRING_AS_C_STR: [&str; 5] = ["alloc", "ffi", "c_str", "CString", "as_c_str"];

0 commit comments

Comments
 (0)