From 7cdeac5773ac7664731d8cb850334559b4b9dab8 Mon Sep 17 00:00:00 2001 From: Ethiraric Date: Thu, 7 Mar 2024 18:50:47 +0100 Subject: [PATCH] [`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. --- .../src/loops/unused_enumerate_index.rs | 2 + clippy_lints/src/methods/mod.rs | 68 +++++++----- .../src/methods/unused_enumerate_index.rs | 102 ++++++++++++++++++ clippy_utils/src/paths.rs | 2 + tests/ui/unused_enumerate_index.fixed | 34 ++++++ tests/ui/unused_enumerate_index.rs | 34 ++++++ tests/ui/unused_enumerate_index.stderr | 42 +++++++- 7 files changed, 253 insertions(+), 31 deletions(-) create mode 100644 clippy_lints/src/methods/unused_enumerate_index.rs diff --git a/clippy_lints/src/loops/unused_enumerate_index.rs b/clippy_lints/src/loops/unused_enumerate_index.rs index e86a63a2738..31f0f1cfeba 100644 --- a/clippy_lints/src/loops/unused_enumerate_index.rs +++ b/clippy_lints/src/loops/unused_enumerate_index.rs @@ -8,6 +8,8 @@ use rustc_middle::ty; /// Checks for the `UNUSED_ENUMERATE_INDEX` lint. +/// +/// The lint is also partially implemented in `clippy_lints/src/methods/unused_enumerate_index.rs`. pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, pat: &Pat<'tcx>, arg: &Expr<'_>, body: &'tcx Expr<'tcx>) { if let PatKind::Tuple([index, elem], _) = pat.kind && let ExprKind::MethodCall(_method, self_arg, [], _) = arg.kind diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 8a24ccea3a1..d6d9425027e 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -118,6 +118,7 @@ mod unnecessary_result_map_or_else; mod unnecessary_sort_by; mod unnecessary_to_owned; +mod unused_enumerate_index; mod unwrap_expect_used; mod useless_asref; mod utils; @@ -4403,6 +4404,7 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { zst_offset::check(cx, expr, recv); }, ("all", [arg]) => { + unused_enumerate_index::check(cx, expr, recv, arg); if let Some(("cloned", recv2, [], _, _)) = method_call(recv) { iter_overeager_cloned::check( cx, @@ -4421,23 +4423,26 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { unnecessary_lazy_eval::check(cx, expr, recv, arg, "and"); } }, - ("any", [arg]) => match method_call(recv) { - Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check( - cx, - expr, - recv, - recv2, - iter_overeager_cloned::Op::NeedlessMove(arg), - false, - ), - Some(("chars", recv, _, _, _)) - if let ExprKind::Closure(arg) = arg.kind - && let body = cx.tcx.hir().body(arg.body) - && let [param] = body.params => - { - string_lit_chars_any::check(cx, expr, recv, param, peel_blocks(body.value), &self.msrv); - }, - _ => {}, + ("any", [arg]) => { + unused_enumerate_index::check(cx, expr, recv, arg); + match method_call(recv) { + Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check( + cx, + expr, + recv, + recv2, + iter_overeager_cloned::Op::NeedlessMove(arg), + false, + ), + Some(("chars", recv, _, _, _)) + if let ExprKind::Closure(arg) = arg.kind + && let body = cx.tcx.hir().body(arg.body) + && let [param] = body.params => + { + string_lit_chars_any::check(cx, expr, recv, param, peel_blocks(body.value), &self.msrv); + }, + _ => {}, + } }, ("arg", [arg]) => { suspicious_command_arg_space::check(cx, recv, arg, span); @@ -4570,14 +4575,17 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { } }, ("filter_map", [arg]) => { + unused_enumerate_index::check(cx, expr, recv, arg); unnecessary_filter_map::check(cx, expr, arg, name); filter_map_bool_then::check(cx, expr, arg, call_span); filter_map_identity::check(cx, expr, arg, span); }, ("find_map", [arg]) => { + unused_enumerate_index::check(cx, expr, recv, arg); unnecessary_filter_map::check(cx, expr, arg, name); }, ("flat_map", [arg]) => { + unused_enumerate_index::check(cx, expr, recv, arg); flat_map_identity::check(cx, expr, arg, span); flat_map_option::check(cx, expr, arg, span); }, @@ -4599,17 +4607,20 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { manual_try_fold::check(cx, expr, init, acc, call_span, &self.msrv); unnecessary_fold::check(cx, expr, init, acc, span); }, - ("for_each", [arg]) => match method_call(recv) { - Some(("inspect", _, [_], span2, _)) => inspect_for_each::check(cx, expr, span2), - Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check( - cx, - expr, - recv, - recv2, - iter_overeager_cloned::Op::NeedlessMove(arg), - false, - ), - _ => {}, + ("for_each", [arg]) => { + unused_enumerate_index::check(cx, expr, recv, arg); + match method_call(recv) { + Some(("inspect", _, [_], span2, _)) => inspect_for_each::check(cx, expr, span2), + Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check( + cx, + expr, + recv, + recv2, + iter_overeager_cloned::Op::NeedlessMove(arg), + false, + ), + _ => {}, + } }, ("get", [arg]) => { get_first::check(cx, expr, recv, arg); @@ -4650,6 +4661,7 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { }, (name @ ("map" | "map_err"), [m_arg]) => { if name == "map" { + unused_enumerate_index::check(cx, expr, recv, m_arg); map_clone::check(cx, expr, recv, m_arg, &self.msrv); match method_call(recv) { Some((map_name @ ("iter" | "into_iter"), recv2, _, _, _)) => { diff --git a/clippy_lints/src/methods/unused_enumerate_index.rs b/clippy_lints/src/methods/unused_enumerate_index.rs new file mode 100644 index 00000000000..506a0f4304a --- /dev/null +++ b/clippy_lints/src/methods/unused_enumerate_index.rs @@ -0,0 +1,102 @@ +use clippy_utils::diagnostics::{multispan_sugg, span_lint_hir_and_then}; +use clippy_utils::paths::{CORE_ITER_ENUMERATE_METHOD, CORE_ITER_ENUMERATE_STRUCT}; +use clippy_utils::source::snippet; +use clippy_utils::{expr_or_init, is_trait_method, match_def_path, pat_is_wild}; +use rustc_hir::{Expr, ExprKind, PatKind}; +use rustc_lint::LateContext; +use rustc_middle::ty::AdtDef; +use rustc_span::sym; + +use crate::loops::UNUSED_ENUMERATE_INDEX; + +/// Check for the `UNUSED_ENUMERATE_INDEX` lint outside of loops. +/// +/// The lint is declared in `clippy_lints/src/loops/mod.rs`. There, the following pattern is +/// checked: +/// ```ignore +/// for (_, x) in some_iter.enumerate() { +/// // Index is ignored +/// } +/// ``` +/// +/// This `check` function checks for chained method calls constructs where we can detect that the +/// index is unused. Currently, this checks only for the following patterns: +/// ```ignore +/// 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`. +/// +/// # Preconditions +/// This function must be called not on the `enumerate` call expression itself, but on any of the +/// map functions listed above. It will ensure that `recv` is a `std::iter::Enumerate` instance and +/// that the method call is one of the `std::iter::Iterator` trait. +/// +/// * `call_expr`: The map function call expression +/// * `recv`: The receiver of the call +/// * `closure_arg`: The argument to the map function call containing the closure/function to apply +pub(super) fn check(cx: &LateContext<'_>, call_expr: &Expr<'_>, recv: &Expr<'_>, closure_arg: &Expr<'_>) { + let recv_ty = cx.typeck_results().expr_ty(recv); + if let Some(recv_ty_defid) = recv_ty.ty_adt_def().map(AdtDef::did) + // If we call a method on a `std::iter::Enumerate` instance + && match_def_path(cx, recv_ty_defid, &CORE_ITER_ENUMERATE_STRUCT) + // If we are calling a method of the `Iterator` trait + && is_trait_method(cx, call_expr, sym::Iterator) + // And the map argument is a closure + && let ExprKind::Closure(closure) = closure_arg.kind + && let closure_body = cx.tcx.hir().body(closure.body) + // And that closure has one argument ... + && let [closure_param] = closure_body.params + // .. which is a tuple of 2 elements + && let PatKind::Tuple([index, elem], ..) = closure_param.pat.kind + // And that the first element (the index) is either `_` or unused in the body + && pat_is_wild(cx, &index.kind, closure_body) + // Try to find the initializer for `recv`. This is needed in case `recv` is a local_binding. In the + // first example below, `expr_or_init` would return `recv`. + // ``` + // iter.enumerate().map(|(_, x)| x) + // ^^^^^^^^^^^^^^^^ `recv`, a call to `std::iter::Iterator::enumerate` + // + // let binding = iter.enumerate(); + // ^^^^^^^^^^^^^^^^ `recv_init_expr` + // binding.map(|(_, x)| x) + // ^^^^^^^ `recv`, not a call to `std::iter::Iterator::enumerate` + // ``` + && let recv_init_expr = expr_or_init(cx, recv) + // Make sure the initializer is a method call. It may be that the `Enumerate` comes from something + // that we cannot control. + // This would for instance happen with: + // ``` + // external_lib::some_function_returning_enumerate().map(|(_, x)| x) + // ``` + && let ExprKind::MethodCall(_, enumerate_recv, _, enumerate_span) = recv_init_expr.kind + && let Some(enumerate_defid) = cx.typeck_results().type_dependent_def_id(recv_init_expr.hir_id) + // Make sure the method call is `std::iter::Iterator::enumerate`. + && match_def_path(cx, enumerate_defid, &CORE_ITER_ENUMERATE_METHOD) + { + // Suggest removing the tuple from the closure and the preceding call to `enumerate`, whose span we + // can get from the `MethodCall`. + span_lint_hir_and_then( + cx, + UNUSED_ENUMERATE_INDEX, + recv_init_expr.hir_id, + enumerate_span, + "you seem to use `.enumerate()` and immediately discard the index", + |diag| { + multispan_sugg( + diag, + "remove the `.enumerate()` call", + vec![ + (closure_param.span, snippet(cx, elem.span, "..").into_owned()), + ( + enumerate_span.with_lo(enumerate_recv.span.source_callsite().hi()), + String::new(), + ), + ], + ); + }, + ); + } +} diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index 987f28192a8..456b8019e95 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -19,6 +19,8 @@ pub const CLONE_TRAIT_METHOD: [&str; 4] = ["core", "clone", "Clone", "clone"]; pub const CORE_ITER_CLONED: [&str; 6] = ["core", "iter", "traits", "iterator", "Iterator", "cloned"]; pub const CORE_ITER_COPIED: [&str; 6] = ["core", "iter", "traits", "iterator", "Iterator", "copied"]; +pub const CORE_ITER_ENUMERATE_METHOD: [&str; 6] = ["core", "iter", "traits", "iterator", "Iterator", "enumerate"]; +pub const CORE_ITER_ENUMERATE_STRUCT: [&str; 5] = ["core", "iter", "adapters", "enumerate", "Enumerate"]; pub const CORE_ITER_FILTER: [&str; 6] = ["core", "iter", "traits", "iterator", "Iterator", "filter"]; pub const CORE_RESULT_OK_METHOD: [&str; 4] = ["core", "result", "Result", "ok"]; pub const CSTRING_AS_C_STR: [&str; 5] = ["alloc", "ffi", "c_str", "CString", "as_c_str"]; diff --git a/tests/ui/unused_enumerate_index.fixed b/tests/ui/unused_enumerate_index.fixed index d079807ab58..1224eb54ba5 100644 --- a/tests/ui/unused_enumerate_index.fixed +++ b/tests/ui/unused_enumerate_index.fixed @@ -3,6 +3,10 @@ use std::iter::Enumerate; +fn get_enumerate() -> Enumerate> { + vec![1].into_iter().enumerate() +} + fn main() { let v = [1, 2, 3]; for x in v.iter() { @@ -55,4 +59,34 @@ fn main() { for x in dummy { println!("{x}"); } + + let _ = vec![1, 2, 3].into_iter().map(|x| println!("{x}")); + + let p = vec![1, 2, 3].into_iter(); + p.map(|x| println!("{x}")); + + // This shouldn't trigger the lint. `get_enumerate` may come from an external library on which we + // have no control. + let p = get_enumerate(); + p.map(|(_, x)| println!("{x}")); + + // This shouldn't trigger the lint. The `enumerate` call is in a different context. + macro_rules! mac { + () => { + [1].iter().enumerate() + }; + } + _ = mac!().map(|(_, v)| v); + + macro_rules! mac2 { + () => { + [1].iter() + }; + } + _ = mac2!().map(|_v| {}); + + // This shouldn't trigger the lint because of the `allow`. + #[allow(clippy::unused_enumerate_index)] + let v = [1].iter().enumerate(); + v.map(|(_, _x)| {}); } diff --git a/tests/ui/unused_enumerate_index.rs b/tests/ui/unused_enumerate_index.rs index 2d524da7632..66f98b690cb 100644 --- a/tests/ui/unused_enumerate_index.rs +++ b/tests/ui/unused_enumerate_index.rs @@ -3,6 +3,10 @@ use std::iter::Enumerate; +fn get_enumerate() -> Enumerate> { + vec![1].into_iter().enumerate() +} + fn main() { let v = [1, 2, 3]; for (_, x) in v.iter().enumerate() { @@ -55,4 +59,34 @@ fn size_hint(&self) -> (usize, Option) { for (_, x) in dummy.enumerate() { println!("{x}"); } + + let _ = vec![1, 2, 3].into_iter().enumerate().map(|(_, x)| println!("{x}")); + + let p = vec![1, 2, 3].into_iter().enumerate(); + p.map(|(_, x)| println!("{x}")); + + // This shouldn't trigger the lint. `get_enumerate` may come from an external library on which we + // have no control. + let p = get_enumerate(); + p.map(|(_, x)| println!("{x}")); + + // This shouldn't trigger the lint. The `enumerate` call is in a different context. + macro_rules! mac { + () => { + [1].iter().enumerate() + }; + } + _ = mac!().map(|(_, v)| v); + + macro_rules! mac2 { + () => { + [1].iter() + }; + } + _ = mac2!().enumerate().map(|(_, _v)| {}); + + // This shouldn't trigger the lint because of the `allow`. + #[allow(clippy::unused_enumerate_index)] + let v = [1].iter().enumerate(); + v.map(|(_, _x)| {}); } diff --git a/tests/ui/unused_enumerate_index.stderr b/tests/ui/unused_enumerate_index.stderr index 7bd7d29117e..f8babf428c0 100644 --- a/tests/ui/unused_enumerate_index.stderr +++ b/tests/ui/unused_enumerate_index.stderr @@ -1,5 +1,5 @@ error: you seem to use `.enumerate()` and immediately discard the index - --> tests/ui/unused_enumerate_index.rs:8:19 + --> tests/ui/unused_enumerate_index.rs:12:19 | LL | for (_, x) in v.iter().enumerate() { | ^^^^^^^^^^^^^^^^^^^^ @@ -12,7 +12,7 @@ LL | for x in v.iter() { | ~ ~~~~~~~~ error: you seem to use `.enumerate()` and immediately discard the index - --> tests/ui/unused_enumerate_index.rs:55:19 + --> tests/ui/unused_enumerate_index.rs:59:19 | LL | for (_, x) in dummy.enumerate() { | ^^^^^^^^^^^^^^^^^ @@ -22,5 +22,41 @@ help: remove the `.enumerate()` call LL | for x in dummy { | ~ ~~~~~ -error: aborting due to 2 previous errors +error: you seem to use `.enumerate()` and immediately discard the index + --> tests/ui/unused_enumerate_index.rs:63:39 + | +LL | let _ = vec![1, 2, 3].into_iter().enumerate().map(|(_, x)| println!("{x}")); + | ^^^^^^^^^^^ + | +help: remove the `.enumerate()` call + | +LL - let _ = vec![1, 2, 3].into_iter().enumerate().map(|(_, x)| println!("{x}")); +LL + let _ = vec![1, 2, 3].into_iter().map(|x| println!("{x}")); + | + +error: you seem to use `.enumerate()` and immediately discard the index + --> tests/ui/unused_enumerate_index.rs:65:39 + | +LL | let p = vec![1, 2, 3].into_iter().enumerate(); + | ^^^^^^^^^^^ + | +help: remove the `.enumerate()` call + | +LL ~ let p = vec![1, 2, 3].into_iter(); +LL ~ p.map(|x| println!("{x}")); + | + +error: you seem to use `.enumerate()` and immediately discard the index + --> tests/ui/unused_enumerate_index.rs:86:17 + | +LL | _ = mac2!().enumerate().map(|(_, _v)| {}); + | ^^^^^^^^^^^ + | +help: remove the `.enumerate()` call + | +LL - _ = mac2!().enumerate().map(|(_, _v)| {}); +LL + _ = mac2!().map(|_v| {}); + | + +error: aborting due to 5 previous errors