From 6a76101d3a46fb1dc21b2ee3044dfac30e3d9a1d Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Wed, 24 May 2023 15:56:28 +0200 Subject: [PATCH] update docs, simplify arg->param map, dont lint on chain --- clippy_lints/src/explicit_into_iter_fn_arg.rs | 124 +++++++++--------- tests/ui/explicit_into_iter_fn_arg.fixed | 17 ++- tests/ui/explicit_into_iter_fn_arg.rs | 17 ++- tests/ui/explicit_into_iter_fn_arg.stderr | 6 +- 4 files changed, 90 insertions(+), 74 deletions(-) diff --git a/clippy_lints/src/explicit_into_iter_fn_arg.rs b/clippy_lints/src/explicit_into_iter_fn_arg.rs index a56d72f0dead..ff4144df9b9a 100644 --- a/clippy_lints/src/explicit_into_iter_fn_arg.rs +++ b/clippy_lints/src/explicit_into_iter_fn_arg.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::source::snippet; +use clippy_utils::source::snippet_with_applicability; use clippy_utils::{get_parent_expr, is_trait_method}; use rustc_errors::Applicability; use rustc_hir::def_id::DefId; @@ -16,50 +16,26 @@ declare_clippy_lint! { /// in a call argument that accepts `IntoIterator`. /// /// ### Why is this bad? - /// If a function has a generic parameter with an `IntoIterator` trait bound, it means that the function - /// will *have* to call `.into_iter()` to get an iterator out of it, thereby making the call to `.into_iter()` - /// at call site redundant. - /// - /// Consider this example: - /// ```rs,ignore - /// fn foo>(iter: T) { - /// let it = iter.into_iter(); - /// ^^^^^^^^^^^^ the function has to call `.into_iter()` to get the iterator - /// } - /// - /// foo(vec![1, 2, 3].into_iter()); - /// ^^^^^^^^^^^^ ... making this `.into_iter()` call redundant. - /// ``` - /// - /// The reason for why calling `.into_iter()` twice (once at call site and again inside of the function) works in the first place - /// is because there is a blanket implementation of `IntoIterator` for all types that implement `Iterator` in the standard library, - /// in which it simply returns itself, effectively making the second call to `.into_iter()` a "no-op": - /// ```rust,ignore - /// impl IntoIterator for I { - /// type Item = I::Item; - /// type IntoIter = I; - /// - /// fn into_iter(self) -> I { - /// self - /// } - /// } - /// ``` + /// If a generic parameter has an `IntoIterator` bound, there is no need to call `.into_iter()` at call site. + /// Calling `IntoIterator::into_iter()` on a value implies that its type already implements `IntoIterator`, + /// so you can just pass the value as is. /// /// ### Example /// ```rust - /// fn even_sum>(iter: I) -> u32 { + /// fn even_sum>(iter: I) -> i32 { /// iter.into_iter().filter(|&x| x % 2 == 0).sum() /// } /// - /// let _ = even_sum(vec![1, 2, 3].into_iter()); + /// let _ = even_sum([1, 2, 3].into_iter()); + /// // ^^^^^^^^^^^^ redundant. `[i32; 3]` implements `IntoIterator` /// ``` /// Use instead: /// ```rust - /// fn even_sum>(iter: I) -> u32 { + /// fn even_sum>(iter: I) -> i32 { /// iter.into_iter().filter(|&x| x % 2 == 0).sum() /// } /// - /// let _ = even_sum(vec![1, 2, 3]); + /// let _ = even_sum([1, 2, 3]); /// ``` #[clippy::version = "1.71.0"] pub EXPLICIT_INTO_ITER_FN_ARG, @@ -73,23 +49,41 @@ enum MethodOrFunction { Function, } +impl MethodOrFunction { + /// Maps the argument position in `pos` to the parameter position. + /// For methods, `self` is skipped. + fn param_pos(self, pos: usize) -> usize { + match self { + MethodOrFunction::Method => pos + 1, + MethodOrFunction::Function => pos, + } + } +} + /// Returns the span of the `IntoIterator` trait bound in the function pointed to by `fn_did` -fn into_iter_bound(cx: &LateContext<'_>, fn_did: DefId, param_index: u32) -> Option { - if let Some(into_iter_did) = cx.tcx.get_diagnostic_item(sym::IntoIterator) { - cx.tcx - .predicates_of(fn_did) - .predicates - .iter() - .find_map(|&(ref pred, span)| { - if let ty::PredicateKind::Clause(ty::Clause::Trait(tr)) = pred.kind().skip_binder() - && tr.def_id() == into_iter_did - && tr.self_ty().is_param(param_index) - { - Some(span) - } else { - None - } - }) +fn into_iter_bound(cx: &LateContext<'_>, fn_did: DefId, into_iter_did: DefId, param_index: u32) -> Option { + cx.tcx + .predicates_of(fn_did) + .predicates + .iter() + .find_map(|&(ref pred, span)| { + if let ty::PredicateKind::Clause(ty::Clause::Trait(tr)) = pred.kind().skip_binder() + && tr.def_id() == into_iter_did + && tr.self_ty().is_param(param_index) + { + Some(span) + } else { + None + } + }) +} + +fn into_iter_call<'hir>(cx: &LateContext<'_>, expr: &'hir Expr<'hir>) -> Option<&'hir Expr<'hir>> { + if let ExprKind::MethodCall(name, recv, _, _) = expr.kind + && is_trait_method(cx, expr, sym::IntoIterator) + && name.ident.name == sym::into_iter + { + Some(recv) } else { None } @@ -101,40 +95,44 @@ impl<'tcx> LateLintPass<'tcx> for ExplicitIntoIterFnArg { return; } - if let ExprKind::MethodCall(name, recv, ..) = expr.kind - && is_trait_method(cx, expr, sym::IntoIterator) - && name.ident.name == sym::into_iter - && let Some(parent_expr) = get_parent_expr(cx, expr) + if let Some(recv) = into_iter_call(cx, expr) + && let Some(parent) = get_parent_expr(cx, expr) + // Make sure that this is not a chained into_iter call (i.e. `x.into_iter().into_iter()`) + // That case is already covered by `useless_conversion` and we don't want to lint twice + // with two contradicting suggestions. + && into_iter_call(cx, parent).is_none() + && into_iter_call(cx, recv).is_none() + && let Some(into_iter_did) = cx.tcx.get_diagnostic_item(sym::IntoIterator) { - let parent_expr = match parent_expr.kind { + + let parent = match parent.kind { ExprKind::Call(recv, args) if let ExprKind::Path(ref qpath) = recv.kind => { cx.qpath_res(qpath, recv.hir_id).opt_def_id() .map(|did| (did, args, MethodOrFunction::Function)) } ExprKind::MethodCall(.., args, _) => { - cx.typeck_results().type_dependent_def_id(parent_expr.hir_id) + cx.typeck_results().type_dependent_def_id(parent.hir_id) .map(|did| (did, args, MethodOrFunction::Method)) } _ => None, }; - if let Some((parent_fn_did, args, kind)) = parent_expr + if let Some((parent_fn_did, args, kind)) = parent && let sig = cx.tcx.fn_sig(parent_fn_did).skip_binder().skip_binder() && let Some(arg_pos) = args.iter().position(|x| x.hir_id == expr.hir_id) - && let Some(&into_iter_param) = sig.inputs().get(match kind { - MethodOrFunction::Function => arg_pos, - MethodOrFunction::Method => arg_pos + 1, // skip self arg - }) + && let Some(&into_iter_param) = sig.inputs().get(kind.param_pos(arg_pos)) && let ty::Param(param) = into_iter_param.kind() - && let Some(span) = into_iter_bound(cx, parent_fn_did, param.index) + && let Some(span) = into_iter_bound(cx, parent_fn_did, into_iter_did, param.index) { - let sugg = snippet(cx, recv.span.source_callsite(), "").into_owned(); + let mut applicability = Applicability::MachineApplicable; + let sugg = snippet_with_applicability(cx, recv.span.source_callsite(), "", &mut applicability).into_owned(); + span_lint_and_then(cx, EXPLICIT_INTO_ITER_FN_ARG, expr.span, "explicit call to `.into_iter()` in function argument accepting `IntoIterator`", |diag| { diag.span_suggestion( expr.span, "consider removing `.into_iter()`", sugg, - Applicability::MachineApplicable, + applicability, ); diag.span_note(span, "this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`"); }); diff --git a/tests/ui/explicit_into_iter_fn_arg.fixed b/tests/ui/explicit_into_iter_fn_arg.fixed index 219b60bd051b..9d778faf8e0c 100644 --- a/tests/ui/explicit_into_iter_fn_arg.fixed +++ b/tests/ui/explicit_into_iter_fn_arg.fixed @@ -1,6 +1,6 @@ //@run-rustfix -#![allow(unused)] +#![allow(unused, clippy::useless_conversion)] #![warn(clippy::explicit_into_iter_fn_arg)] fn a(_: T) {} @@ -11,7 +11,6 @@ where T: IntoIterator, { } -fn e>(_: T) {} fn f(_: std::vec::IntoIter) {} fn main() { @@ -19,6 +18,16 @@ fn main() { b(vec![1, 2]); c(vec![1, 2]); d(vec![1, 2]); - e([&1, &2, &3].into_iter().cloned()); - f(vec![1, 2].into_iter()); + b([&1, &2, &3].into_iter().cloned()); + + // Don't lint chained `.into_iter().into_iter()` calls. Covered by useless_conversion. + b(vec![1, 2].into_iter().into_iter()); + b(vec![1, 2].into_iter().into_iter().into_iter()); + + macro_rules! macro_generated { + () => { + vec![1, 2].into_iter() + }; + } + b(macro_generated!()); } diff --git a/tests/ui/explicit_into_iter_fn_arg.rs b/tests/ui/explicit_into_iter_fn_arg.rs index 358f4bc661d1..1e051fd06ecf 100644 --- a/tests/ui/explicit_into_iter_fn_arg.rs +++ b/tests/ui/explicit_into_iter_fn_arg.rs @@ -1,6 +1,6 @@ //@run-rustfix -#![allow(unused)] +#![allow(unused, clippy::useless_conversion)] #![warn(clippy::explicit_into_iter_fn_arg)] fn a(_: T) {} @@ -11,7 +11,6 @@ where T: IntoIterator, { } -fn e>(_: T) {} fn f(_: std::vec::IntoIter) {} fn main() { @@ -19,6 +18,16 @@ fn main() { b(vec![1, 2].into_iter()); c(vec![1, 2].into_iter()); d(vec![1, 2].into_iter()); - e([&1, &2, &3].into_iter().cloned()); - f(vec![1, 2].into_iter()); + b([&1, &2, &3].into_iter().cloned()); + + // Don't lint chained `.into_iter().into_iter()` calls. Covered by useless_conversion. + b(vec![1, 2].into_iter().into_iter()); + b(vec![1, 2].into_iter().into_iter().into_iter()); + + macro_rules! macro_generated { + () => { + vec![1, 2].into_iter() + }; + } + b(macro_generated!()); } diff --git a/tests/ui/explicit_into_iter_fn_arg.stderr b/tests/ui/explicit_into_iter_fn_arg.stderr index a0bd81c21eac..14b95fb097a0 100644 --- a/tests/ui/explicit_into_iter_fn_arg.stderr +++ b/tests/ui/explicit_into_iter_fn_arg.stderr @@ -1,5 +1,5 @@ error: explicit call to `.into_iter()` in function argument accepting `IntoIterator` - --> $DIR/explicit_into_iter_fn_arg.rs:19:7 + --> $DIR/explicit_into_iter_fn_arg.rs:18:7 | LL | b(vec![1, 2].into_iter()); | ^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `vec![1, 2]` @@ -12,7 +12,7 @@ LL | fn b>(_: T) {} = note: `-D clippy::explicit-into-iter-fn-arg` implied by `-D warnings` error: explicit call to `.into_iter()` in function argument accepting `IntoIterator` - --> $DIR/explicit_into_iter_fn_arg.rs:20:7 + --> $DIR/explicit_into_iter_fn_arg.rs:19:7 | LL | c(vec![1, 2].into_iter()); | ^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `vec![1, 2]` @@ -24,7 +24,7 @@ LL | fn c(_: impl IntoIterator) {} | ^^^^^^^^^^^^^^^^^^^^^^^^ error: explicit call to `.into_iter()` in function argument accepting `IntoIterator` - --> $DIR/explicit_into_iter_fn_arg.rs:21:7 + --> $DIR/explicit_into_iter_fn_arg.rs:20:7 | LL | d(vec![1, 2].into_iter()); | ^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `vec![1, 2]`