Auto merge of rust-lang#10814 - y21:issue10743, r=llogiq

new lint: `explicit_into_iter_fn_arg` Closes rust-lang#10743. This adds a lint that looks for `.into_iter()` calls in a call expression to a function that already expects an `IntoIterator`. In those cases, explicitly calling `.into_iter()` is unnecessary. There were a few instances of this in clippy itself so I fixed those as well in this PR. changelog: new lint [`explicit_into_iter_fn_arg`]
flip1995 · Jun 3, 2023 · 8a30f2f · 8a30f2f
2 parents 2490de4 + d816eba
commit 8a30f2f
Show file tree

Hide file tree

Showing 7 changed files with 286 additions and 71 deletions.
diff --git a/clippy_lints/src/booleans.rs b/clippy_lints/src/booleans.rs
@@ -440,7 +440,7 @@ impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> {
                             diag.span_suggestions(
                                 e.span,
                                 "try",
-                                suggestions.into_iter(),
+                                suggestions,
                                 // nonminimal_bool can produce minimal but
                                 // not human readable expressions (#3141)
                                 Applicability::Unspecified,

diff --git a/clippy_lints/src/loops/manual_memcpy.rs b/clippy_lints/src/loops/manual_memcpy.rs
@@ -51,7 +51,7 @@ pub(super) fn check<'tcx>(
                 iter_b = Some(get_assignment(body));
             }
 
-            let assignments = iter_a.into_iter().flatten().chain(iter_b.into_iter());
+            let assignments = iter_a.into_iter().flatten().chain(iter_b);
 
             let big_sugg = assignments
                 // The only statements in the for loops can be indexed assignments from
@@ -402,7 +402,7 @@ fn get_assignments<'a, 'tcx>(
             StmtKind::Local(..) | StmtKind::Item(..) => None,
             StmtKind::Expr(e) | StmtKind::Semi(e) => Some(e),
         })
-        .chain((*expr).into_iter())
+        .chain(*expr)
         .filter(move |e| {
             if let ExprKind::AssignOp(_, place, _) = e.kind {
                 path_to_local(place).map_or(false, |id| {

diff --git a/clippy_lints/src/useless_conversion.rs b/clippy_lints/src/useless_conversion.rs
@@ -1,16 +1,17 @@
-use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
+use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::is_ty_alias;
-use clippy_utils::source::{snippet, snippet_with_context};
+use clippy_utils::source::{snippet, snippet_with_applicability, snippet_with_context};
 use clippy_utils::sugg::Sugg;
 use clippy_utils::ty::{is_copy, is_type_diagnostic_item, same_type_and_consts};
 use clippy_utils::{get_parent_expr, is_trait_method, match_def_path, path_to_local, paths};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
+use rustc_hir::def_id::DefId;
 use rustc_hir::{BindingAnnotation, Expr, ExprKind, HirId, MatchSource, Node, PatKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
-use rustc_span::sym;
+use rustc_span::{sym, Span};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -43,6 +44,65 @@ pub struct UselessConversion {
 
 impl_lint_pass!(UselessConversion => [USELESS_CONVERSION]);
 
+enum MethodOrFunction {
+    Method,
+    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, into_iter_did: DefId, param_index: u32) -> Option<Span> {
+    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
+            }
+        })
+}
+
+/// Extracts the receiver of a `.into_iter()` method call.
+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
+    }
+}
+
+/// Same as [`into_iter_call`], but tries to look for the innermost `.into_iter()` call, e.g.:
+/// `foo.into_iter().into_iter()`
+///  ^^^  we want this expression
+fn into_iter_deep_call<'hir>(cx: &LateContext<'_>, mut expr: &'hir Expr<'hir>) -> Option<&'hir Expr<'hir>> {
+    loop {
+        if let Some(recv) = into_iter_call(cx, expr) {
+            expr = recv;
+        } else {
+            return Some(expr);
+        }
+    }
+}
+
 #[expect(clippy::too_many_lines)]
 impl<'tcx> LateLintPass<'tcx> for UselessConversion {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
@@ -82,9 +142,58 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion {
                         );
                     }
                 }
-                if is_trait_method(cx, e, sym::IntoIterator) && name.ident.name == sym::into_iter {
-                    if get_parent_expr(cx, e).is_some() &&
-                       let Some(id) = path_to_local(recv) &&
+                if let Some(into_iter_recv) = into_iter_call(cx, e)
+                    // Make sure that there is no parent expression, or if there is, make sure it's not a `.into_iter()` call.
+                    // The reason for that is that we only want to lint once (the outermost call)
+                    // in cases like `foo.into_iter().into_iter()`
+                    && get_parent_expr(cx, e)
+                        .and_then(|parent| into_iter_call(cx, parent))
+                        .is_none()
+                {
+                    if let Some(parent) = get_parent_expr(cx, e) {
+                        let parent_fn = 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.hir_id)
+                                    .map(|did| (did, args, MethodOrFunction::Method))
+                            }
+                            _ => None,
+                        };
+
+                        if let Some((parent_fn_did, args, kind)) = parent_fn
+                            && let Some(into_iter_did) = cx.tcx.get_diagnostic_item(sym::IntoIterator)
+                            && let sig = cx.tcx.fn_sig(parent_fn_did).skip_binder().skip_binder()
+                            && let Some(arg_pos) = args.iter().position(|x| x.hir_id == e.hir_id)
+                            && 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, into_iter_did, param.index)
+                            // Get the "innermost" `.into_iter()` call, e.g. given this expression:
+                            // `foo.into_iter().into_iter()`
+                            //  ^^^
+                            //  We want this span
+                            && let Some(into_iter_recv) = into_iter_deep_call(cx, into_iter_recv)
+                        {
+                            let mut applicability = Applicability::MachineApplicable;
+                            let sugg = snippet_with_applicability(cx, into_iter_recv.span.source_callsite(), "<expr>", &mut applicability).into_owned();
+                            span_lint_and_then(cx, USELESS_CONVERSION, e.span, "explicit call to `.into_iter()` in function argument accepting `IntoIterator`", |diag| {
+                                diag.span_suggestion(
+                                    e.span,
+                                    "consider removing `.into_iter()`",
+                                    sugg,
+                                    applicability,
+                                );
+                                diag.span_note(span, "this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`");
+                            });
+
+                            // Early return to avoid linting again with contradicting suggestions
+                            return;
+                        }
+                    }
+
+                    if let Some(id) = path_to_local(recv) &&
                        let Node::Pat(pat) = cx.tcx.hir().get(id) &&
                        let PatKind::Binding(ann, ..) = pat.kind &&
                        ann != BindingAnnotation::MUT

diff --git a/tests/ui/auxiliary/proc_macro_derive.rs b/tests/ui/auxiliary/proc_macro_derive.rs
@@ -90,70 +90,58 @@ pub fn extra_lifetime(_input: TokenStream) -> TokenStream {
 #[allow(unused)]
 #[proc_macro_derive(ArithmeticDerive)]
 pub fn arithmetic_derive(_: TokenStream) -> TokenStream {
-    <TokenStream as FromIterator<TokenTree>>::from_iter(
-        [
-            Ident::new("fn", Span::call_site()).into(),
-            Ident::new("_foo", Span::call_site()).into(),
-            Group::new(Delimiter::Parenthesis, TokenStream::new()).into(),
-            Group::new(
-                Delimiter::Brace,
-                <TokenStream as FromIterator<TokenTree>>::from_iter(
-                    [
-                        Ident::new("let", Span::call_site()).into(),
-                        Ident::new("mut", Span::call_site()).into(),
-                        Ident::new("_n", Span::call_site()).into(),
-                        Punct::new('=', Spacing::Alone).into(),
-                        Literal::i32_unsuffixed(9).into(),
-                        Punct::new(';', Spacing::Alone).into(),
-                        Ident::new("_n", Span::call_site()).into(),
-                        Punct::new('=', Spacing::Alone).into(),
-                        Literal::i32_unsuffixed(9).into(),
-                        Punct::new('/', Spacing::Alone).into(),
-                        Literal::i32_unsuffixed(2).into(),
-                        Punct::new(';', Spacing::Alone).into(),
-                        Ident::new("_n", Span::call_site()).into(),
-                        Punct::new('=', Spacing::Alone).into(),
-                        Punct::new('-', Spacing::Alone).into(),
-                        Ident::new("_n", Span::call_site()).into(),
-                        Punct::new(';', Spacing::Alone).into(),
-                    ]
-                    .into_iter(),
-                ),
-            )
-            .into(),
-        ]
-        .into_iter(),
-    )
+    <TokenStream as FromIterator<TokenTree>>::from_iter([
+        Ident::new("fn", Span::call_site()).into(),
+        Ident::new("_foo", Span::call_site()).into(),
+        Group::new(Delimiter::Parenthesis, TokenStream::new()).into(),
+        Group::new(
+            Delimiter::Brace,
+            <TokenStream as FromIterator<TokenTree>>::from_iter([
+                Ident::new("let", Span::call_site()).into(),
+                Ident::new("mut", Span::call_site()).into(),
+                Ident::new("_n", Span::call_site()).into(),
+                Punct::new('=', Spacing::Alone).into(),
+                Literal::i32_unsuffixed(9).into(),
+                Punct::new(';', Spacing::Alone).into(),
+                Ident::new("_n", Span::call_site()).into(),
+                Punct::new('=', Spacing::Alone).into(),
+                Literal::i32_unsuffixed(9).into(),
+                Punct::new('/', Spacing::Alone).into(),
+                Literal::i32_unsuffixed(2).into(),
+                Punct::new(';', Spacing::Alone).into(),
+                Ident::new("_n", Span::call_site()).into(),
+                Punct::new('=', Spacing::Alone).into(),
+                Punct::new('-', Spacing::Alone).into(),
+                Ident::new("_n", Span::call_site()).into(),
+                Punct::new(';', Spacing::Alone).into(),
+            ]),
+        )
+        .into(),
+    ])
 }
 
 #[allow(unused)]
 #[proc_macro_derive(ShadowDerive)]
 pub fn shadow_derive(_: TokenStream) -> TokenStream {
-    <TokenStream as FromIterator<TokenTree>>::from_iter(
-        [
-            Ident::new("fn", Span::call_site()).into(),
-            Ident::new("_foo", Span::call_site()).into(),
-            Group::new(Delimiter::Parenthesis, TokenStream::new()).into(),
-            Group::new(
-                Delimiter::Brace,
-                <TokenStream as FromIterator<TokenTree>>::from_iter(
-                    [
-                        Ident::new("let", Span::call_site()).into(),
-                        Ident::new("_x", Span::call_site()).into(),
-                        Punct::new('=', Spacing::Alone).into(),
-                        Literal::i32_unsuffixed(2).into(),
-                        Punct::new(';', Spacing::Alone).into(),
-                        Ident::new("let", Span::call_site()).into(),
-                        Ident::new("_x", Span::call_site()).into(),
-                        Punct::new('=', Spacing::Alone).into(),
-                        Ident::new("_x", Span::call_site()).into(),
-                        Punct::new(';', Spacing::Alone).into(),
-                    ]
-                    .into_iter(),
-                ),
-            )
-            .into(),
-        ]
-        .into_iter(),
-    )
+    <TokenStream as FromIterator<TokenTree>>::from_iter([
+        Ident::new("fn", Span::call_site()).into(),
+        Ident::new("_foo", Span::call_site()).into(),
+        Group::new(Delimiter::Parenthesis, TokenStream::new()).into(),
+        Group::new(
+            Delimiter::Brace,
+            <TokenStream as FromIterator<TokenTree>>::from_iter([
+                Ident::new("let", Span::call_site()).into(),
+                Ident::new("_x", Span::call_site()).into(),
+                Punct::new('=', Spacing::Alone).into(),
+                Literal::i32_unsuffixed(2).into(),
+                Punct::new(';', Spacing::Alone).into(),
+                Ident::new("let", Span::call_site()).into(),
+                Ident::new("_x", Span::call_site()).into(),
+                Punct::new('=', Spacing::Alone).into(),
+                Ident::new("_x", Span::call_site()).into(),
+                Punct::new(';', Spacing::Alone).into(),
+            ]),
+        )
+        .into(),
+    ])
 }
diff --git a/tests/ui/useless_conversion.fixed b/tests/ui/useless_conversion.fixed
@@ -155,6 +155,35 @@ fn main() {
     let _ = vec![s4, s4, s4].into_iter();
 }
 
+#[allow(dead_code)]
+fn explicit_into_iter_fn_arg() {
+    fn a<T>(_: T) {}
+    fn b<T: IntoIterator<Item = i32>>(_: T) {}
+    fn c(_: impl IntoIterator<Item = i32>) {}
+    fn d<T>(_: T)
+    where
+        T: IntoIterator<Item = i32>,
+    {
+    }
+    fn f(_: std::vec::IntoIter<i32>) {}
+
+    a(vec![1, 2].into_iter());
+    b(vec![1, 2]);
+    c(vec![1, 2]);
+    d(vec![1, 2]);
+    b([&1, &2, &3].into_iter().cloned());
+
+    b(vec![1, 2]);
+    b(vec![1, 2]);
+
+    macro_rules! macro_generated {
+        () => {
+            vec![1, 2].into_iter()
+        };
+    }
+    b(macro_generated!());
+}
+
 #[derive(Copy, Clone)]
 struct Foo<const C: char>;
 

diff --git a/tests/ui/useless_conversion.rs b/tests/ui/useless_conversion.rs
@@ -155,6 +155,35 @@ fn main() {
     let _ = vec![s4, s4, s4].into_iter().into_iter();
 }
 
+#[allow(dead_code)]
+fn explicit_into_iter_fn_arg() {
+    fn a<T>(_: T) {}
+    fn b<T: IntoIterator<Item = i32>>(_: T) {}
+    fn c(_: impl IntoIterator<Item = i32>) {}
+    fn d<T>(_: T)
+    where
+        T: IntoIterator<Item = i32>,
+    {
+    }
+    fn f(_: std::vec::IntoIter<i32>) {}
+
+    a(vec![1, 2].into_iter());
+    b(vec![1, 2].into_iter());
+    c(vec![1, 2].into_iter());
+    d(vec![1, 2].into_iter());
+    b([&1, &2, &3].into_iter().cloned());
+
+    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!());
+}
+
 #[derive(Copy, Clone)]
 struct Foo<const C: char>;