Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[unused_enumerate_index]: trigger on method calls #12432

Merged
merged 3 commits into from Mar 14, 2024
Merged

[unused_enumerate_index]: trigger on method calls #12432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
43f4ec3
[`unused_enumerate_index`]: Use if-let chain
Ethiraric Mar 7, 2024
7cdeac5
[`unused_enumerate_index`]: trigger on method calls
Ethiraric Mar 7, 2024
dadcd94
[`unused_enumerate_index`]: Keep explicit element type
Ethiraric Mar 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
81 changes: 30 additions & 51 deletions clippy_lints/src/loops/unused_enumerate_index.rs
View file
Open in desktop
@@ -1,62 +1,41 @@
use super::UNUSED_ENUMERATE_INDEX;
use clippy_utils::diagnostics::{multispan_sugg, span_lint_and_then};
use clippy_utils::source::snippet;
use clippy_utils::{pat_is_wild, sugg};
use clippy_utils::{match_def_path, pat_is_wild, sugg};
use rustc_hir::def::DefKind;
use rustc_hir::{Expr, ExprKind, Pat, PatKind};
use rustc_lint::LateContext;
use rustc_middle::ty;

/// Checks for the `UNUSED_ENUMERATE_INDEX` lint.
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, arg: &'tcx Expr<'_>, body: &'tcx Expr<'_>) {
let PatKind::Tuple([index, elem], _) = pat.kind else {
return;
};

let ExprKind::MethodCall(_method, self_arg, [], _) = arg.kind else {
return;
};

let ty = cx.typeck_results().expr_ty(arg);

if !pat_is_wild(cx, &index.kind, body) {
return;
}

let name = match *ty.kind() {
ty::Adt(base, _substs) => cx.tcx.def_path_str(base.did()),
_ => return,
};

if name != "std::iter::Enumerate" && name != "core::iter::Enumerate" {
return;
///
/// 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
&& let ty = cx.typeck_results().expr_ty(arg)
&& pat_is_wild(cx, &index.kind, body)
&& let ty::Adt(base, _) = *ty.kind()
&& match_def_path(cx, base.did(), &clippy_utils::paths::CORE_ITER_ENUMERATE_STRUCT)
&& let Some((DefKind::AssocFn, call_id)) = cx.typeck_results().type_dependent_def(arg.hir_id)
&& match_def_path(cx, call_id, &clippy_utils::paths::CORE_ITER_ENUMERATE_METHOD)
{
span_lint_and_then(
cx,
UNUSED_ENUMERATE_INDEX,
arg.span,
"you seem to use `.enumerate()` and immediately discard the index",
|diag| {
let base_iter = sugg::Sugg::hir(cx, self_arg, "base iter");
multispan_sugg(
diag,
"remove the `.enumerate()` call",
vec![
(pat.span, snippet(cx, elem.span, "..").into_owned()),
(arg.span, base_iter.to_string()),
],
);
},
);
}

let Some((DefKind::AssocFn, call_id)) = cx.typeck_results().type_dependent_def(arg.hir_id) else {
return;
};

let call_name = cx.tcx.def_path_str(call_id);

if call_name != "std::iter::Iterator::enumerate" && call_name != "core::iter::Iterator::enumerate" {
return;
}

span_lint_and_then(
cx,
UNUSED_ENUMERATE_INDEX,
arg.span,
"you seem to use `.enumerate()` and immediately discard the index",
|diag| {
let base_iter = sugg::Sugg::hir(cx, self_arg, "base iter");
multispan_sugg(
diag,
"remove the `.enumerate()` call",
vec![
(pat.span, snippet(cx, elem.span, "..").into_owned()),
(arg.span, base_iter.to_string()),
],
);
},
);
}
68 changes: 40 additions & 28 deletions clippy_lints/src/methods/mod.rs
View file
Open in desktop
Expand Up @@ -118,6 +118,7 @@ mod unnecessary_literal_unwrap;
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;
Expand Down Expand Up @@ -4403,6 +4404,7 @@ impl Methods {
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,
Expand All @@ -4421,23 +4423,26 @@ impl Methods {
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);
Expand Down Expand Up @@ -4570,14 +4575,17 @@ impl Methods {
}
},
("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);
},
Expand All @@ -4599,17 +4607,20 @@ impl Methods {
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);
Expand Down Expand Up @@ -4650,6 +4661,7 @@ impl Methods {
},
(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, _, _, _)) => {
Expand Down
135 changes: 135 additions & 0 deletions clippy_lints/src/methods/unused_enumerate_index.rs
View file
Open in desktop
@@ -0,0 +1,135 @@
use clippy_utils::diagnostics::{multispan_sugg_with_applicability, span_lint_hir_and_then};
use clippy_utils::paths::{CORE_ITER_ENUMERATE_METHOD, CORE_ITER_ENUMERATE_STRUCT};
use clippy_utils::source::{snippet, snippet_opt};
use clippy_utils::{expr_or_init, is_trait_method, match_def_path, pat_is_wild};
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind, FnDecl, PatKind, TyKind};
use rustc_lint::LateContext;
use rustc_middle::ty::AdtDef;
use rustc_span::{sym, Span};

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
y21 marked this conversation as resolved.
Show resolved Hide resolved
// 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)
{
// Check if the tuple type was explicit. It may be the type system _needs_ the type of the element
// that would be explicited in the closure.
let new_closure_param = match find_elem_explicit_type_span(closure.fn_decl) {
// We have an explicit type. Get its snippet, that of the binding name, and do `binding: ty`.
// Fallback to `..` if we fail getting either snippet.
Some(ty_span) => snippet_opt(cx, elem.span)
.and_then(|binding_name| snippet_opt(cx, ty_span).map(|ty_name| format!("{binding_name}: {ty_name}")))
.unwrap_or_else(|| "..".to_string()),
// Otherwise, we have no explicit type. We can replace with the binding name of the element.
None => snippet(cx, elem.span, "..").into_owned(),
};

// 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_with_applicability(
diag,
"remove the `.enumerate()` call",
Applicability::MachineApplicable,
vec![
(closure_param.span, new_closure_param),
(
enumerate_span.with_lo(enumerate_recv.span.source_callsite().hi()),
String::new(),
),
],
);
},
);
}
}

/// Find the span of the explicit type of the element.
///
/// # Returns
/// If the tuple argument:
/// * Has no explicit type, returns `None`
/// * Has an explicit tuple type with an implicit element type (`(usize, _)`), returns `None`
/// * Has an explicit tuple type with an explicit element type (`(_, i32)`), returns the span for
/// the element type.
fn find_elem_explicit_type_span(fn_decl: &FnDecl<'_>) -> Option<Span> {
if let [tuple_ty] = fn_decl.inputs
&& let TyKind::Tup([_idx_ty, elem_ty]) = tuple_ty.kind
&& !matches!(elem_ty.kind, TyKind::Err(..) | TyKind::Infer)
{
Some(elem_ty.span)
} else {
None
}
}
2 changes: 2 additions & 0 deletions clippy_utils/src/paths.rs
View file
Open in desktop
Expand Up @@ -19,6 +19,8 @@ pub const BTREESET_ITER: [&str; 6] = ["alloc", "collections", "btree", "set", "B
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"];
Expand Down