lint on unnecessary collections

CHANGELOG.md

@@ -6722,6 +6722,7 @@ Released 2018-09-13
 [`unnecessary_box_returns`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns
 [`unnecessary_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
 [`unnecessary_clippy_cfg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_clippy_cfg
+[`unnecessary_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_collect
 [`unnecessary_debug_formatting`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_debug_formatting
 [`unnecessary_fallible_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_fallible_conversions
 [`unnecessary_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_filter_map

clippy_config/src/conf.rs

@@ -1,3 +1,4 @@
+#![allow(clippy::unnecessary_collect)]
 use crate::ClippyConfiguration;
 use crate::types::{
     DisallowedPath, DisallowedPathWithoutReplacement, MacroMatcher, MatchLintBehaviour, PubUnderscoreFieldsBehaviour,
@@ -959,10 +960,10 @@ fn deserialize(file: &SourceFile) -> TryConf {
                         // info for the user instead.
 
                         let names = conf.conf.module_item_order_groupings.grouping_names();
-                        let suggestion = suggest_candidate(grouping, names.iter().map(String::as_str))
+                        let suggestion = suggest_candidate(grouping, names.clone())
                             .map(|s| format!(" perhaps you meant `{s}`?"))
                             .unwrap_or_default();
-                        let names = names.iter().map(|s| format!("`{s}`")).join(", ");
+                        let names = names.map(|s| format!("`{s}`")).join(", ");
                         let message = format!(
                             "unknown ordering group: `{grouping}` was not specified in `module-items-ordered-within-groupings`,{suggestion} expected one of: {names}"
                         );

clippy_config/src/types.rs

@@ -422,8 +422,8 @@ impl SourceItemOrderingModuleItemGroupings {
         self.back_lut.get(item)
     }
 
-    pub fn grouping_names(&self) -> Vec<String> {
-        self.groups.iter().map(|(name, _)| name.clone()).collect()
+    pub fn grouping_names(&self) -> impl Iterator<Item = &str> + Clone {
+        self.groups.iter().map(|(name, _)| &**name)
     }
 
     pub fn is_grouping(&self, grouping: &str) -> bool {

clippy_lints/src/declared_lints.rs

@@ -749,6 +749,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::unit_types::UNIT_ARG_INFO,
     crate::unit_types::UNIT_CMP_INFO,
     crate::unnecessary_box_returns::UNNECESSARY_BOX_RETURNS_INFO,
+    crate::unnecessary_collect::UNNECESSARY_COLLECT_INFO,
     crate::unnecessary_literal_bound::UNNECESSARY_LITERAL_BOUND_INFO,
     crate::unnecessary_map_on_constructor::UNNECESSARY_MAP_ON_CONSTRUCTOR_INFO,
     crate::unnecessary_mut_passed::UNNECESSARY_MUT_PASSED_INFO,

clippy_lints/src/doc/suspicious_doc_comments.rs

@@ -32,7 +32,7 @@ pub fn check(cx: &LateContext<'_>, attrs: &[Attribute]) -> bool {
         false
     }
 }
-
+#[expect(clippy::unnecessary_collect)]
 fn collect_doc_replacements(attrs: &[Attribute]) -> Vec<(Span, String)> {
     attrs
         .iter()

clippy_lints/src/lib.rs

@@ -371,6 +371,7 @@ mod uninit_vec;
 mod unit_return_expecting_ord;
 mod unit_types;
 mod unnecessary_box_returns;
+mod unnecessary_collect;
 mod unnecessary_literal_bound;
 mod unnecessary_map_on_constructor;
 mod unnecessary_mut_passed;
@@ -832,5 +833,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
     store.register_late_pass(|_| Box::new(coerce_container_to_any::CoerceContainerToAny));
     store.register_late_pass(|_| Box::new(toplevel_ref_arg::ToplevelRefArg));
     store.register_late_pass(|_| Box::new(volatile_composites::VolatileComposites));
+    store.register_late_pass(move |_| Box::new(unnecessary_collect::UnnecessaryCollect::new(conf)));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }

clippy_lints/src/lifetimes.rs

@@ -394,7 +394,7 @@ fn non_elidable_self_type<'tcx>(cx: &LateContext<'tcx>, func: &FnDecl<'tcx>, ide
         let mut visitor = RefVisitor::new(cx);
         visitor.visit_ty_unambig(self_ty);
 
-        !visitor.all_lts().is_empty()
+        visitor.all_lts().next().is_some()
     } else {
         false
     }
@@ -444,12 +444,8 @@ impl<'a, 'tcx> RefVisitor<'a, 'tcx> {
         }
     }
 
-    fn all_lts(&self) -> Vec<Lifetime> {
-        self.lts
-            .iter()
-            .chain(self.nested_elision_site_lts.iter())
-            .copied()
-            .collect::<Vec<_>>()
+    fn all_lts(&self) -> impl Iterator<Item = Lifetime> {
+        self.lts.iter().chain(self.nested_elision_site_lts.iter()).copied()
     }
 
     fn abort(&self) -> bool {
@@ -472,7 +468,7 @@ impl<'tcx> Visitor<'tcx> for RefVisitor<'_, 'tcx> {
         {
             let mut sub_visitor = RefVisitor::new(self.cx);
             sub_visitor.visit_trait_ref(trait_ref);
-            self.nested_elision_site_lts.append(&mut sub_visitor.all_lts());
+            self.nested_elision_site_lts.extend(sub_visitor.all_lts());
         } else {
             walk_poly_trait_ref(self, poly_tref);
         }
@@ -483,7 +479,7 @@ impl<'tcx> Visitor<'tcx> for RefVisitor<'_, 'tcx> {
             TyKind::FnPtr(&FnPtrTy { decl, .. }) => {
                 let mut sub_visitor = RefVisitor::new(self.cx);
                 sub_visitor.visit_fn_decl(decl);
-                self.nested_elision_site_lts.append(&mut sub_visitor.all_lts());
+                self.nested_elision_site_lts.extend(sub_visitor.all_lts());
             },
             TyKind::TraitObject(bounds, lt) => {
                 if !lt.is_elided() {
@@ -509,7 +505,7 @@ fn has_where_lifetimes<'tcx>(cx: &LateContext<'tcx>, generics: &'tcx Generics<'_
                 let mut visitor = RefVisitor::new(cx);
                 // walk the type F, it may not contain LT refs
                 walk_unambig_ty(&mut visitor, pred.bounded_ty);
-                if !visitor.all_lts().is_empty() {
+                if visitor.all_lts().next().is_some() {
                     return true;
                 }
                 // if the bounds define new lifetimes, they are fine to occur

clippy_lints/src/non_send_fields_in_send_ty.rs

@@ -107,7 +107,7 @@ impl<'tcx> LateLintPass<'tcx> for NonSendFieldInSendTy {
                         non_send_fields.push(NonSendField {
                             def: field_def,
                             ty: field_ty,
-                            generic_params: collect_generic_params(field_ty),
+                            generic_params: collect_generic_params(field_ty).collect(),
                         });
                     }
                 }
@@ -170,14 +170,13 @@ impl NonSendField<'_> {
 
 /// Given a type, collect all of its generic parameters.
 /// Example: `MyStruct<P, Box<Q, R>>` => `vec![P, Q, R]`
-fn collect_generic_params(ty: Ty<'_>) -> Vec<Ty<'_>> {
+fn collect_generic_params(ty: Ty<'_>) -> impl Iterator<Item = Ty<'_>> {
     ty.walk()
         .filter_map(|inner| match inner.kind() {
             GenericArgKind::Type(inner_ty) => Some(inner_ty),
             _ => None,
         })
         .filter(|&inner_ty| is_ty_param(inner_ty))
-        .collect()
 }
 
 /// Be more strict when the heuristic is disabled

clippy_lints/src/unnecessary_collect.rs

@@ -0,0 +1,123 @@
+use clippy_config::Conf;
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::sym;
+use clippy_utils::ty::{get_iterator_item_ty, has_non_owning_mutable_access, implements_trait};
+use rustc_errors::Applicability;
+use rustc_hir::def_id::LocalDefId;
+use rustc_hir::intravisit::FnKind;
+use rustc_lint::LateContext;
+use rustc_middle::ty::{self};
+use rustc_span::Span;
+
+use rustc_hir::{Body, ExprKind, FnDecl, Stmt, StmtKind};
+use rustc_lint::LateLintPass;
+use rustc_session::impl_lint_pass;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for functions returning `Vec<T>` where the `T` is produced from a `_.collect::<Vec<T>>()`
+    ///
+    /// ### Why is this bad?
+    ///
+    /// This might not be necessary, and its more idiomatic to simply return the iterator, giving the caller the option to use it as they see fit.
+    /// Its often silly to have the equivalent of `iterator.collect::<Vec<_>>().into_iter()` in your code.
+    ///
+    /// ### Potential Issues
+    ///
+    /// In some cases, there may be some lifetimes attached to your iterator that make it difficult, or even impossible, to avoid a collection.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// fn squares() -> Vec<(u8, u8)> {
+    ///   (0..8u8)
+    ///     .flat_map(|x| (0..8).map(move |y| (x, y)))
+    ///     .collect::<Vec<_>>()
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// fn squares() -> impl Iterator<Item = (u8, u8)> {
+    ///   (0..8u8).flat_map(|x| (0..8).map(move |y| (x, y)))
+    /// }
+    /// ```
+    #[clippy::version = "1.92.0"]
+    pub UNNECESSARY_COLLECT,
+    restriction,
+    "checks for functions returning vecs produced from vec::from_iter"
+}
+
+pub struct UnnecessaryCollect {
+    avoid_breaking_exported_api: bool,
+}
+
+impl UnnecessaryCollect {
+    pub fn new(conf: &'static Conf) -> Self {
+        Self {
+            avoid_breaking_exported_api: conf.avoid_breaking_exported_api,
+        }
+    }
+}
+
+impl_lint_pass!(UnnecessaryCollect => [UNNECESSARY_COLLECT]);
+
+impl<'tcx> LateLintPass<'tcx> for UnnecessaryCollect {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        kind: FnKind<'tcx>,
+        decl: &'tcx FnDecl<'tcx>,
+        body: &'tcx Body<'tcx>,
+        _: Span,
+        def_id: LocalDefId,
+    ) {
+        let expr = match kind {
+            FnKind::ItemFn(..) | FnKind::Method(..) if let ExprKind::Block(block, _) = &body.value.kind => {
+                if let Some(block_expr) = block.expr {
+                    block_expr
+                } else if let Some(Stmt {
+                    kind: StmtKind::Expr(expr),
+                    ..
+                }) = block.stmts.last()
+                {
+                    expr
+                } else {
+                    return;
+                }
+            },
+            _ => return,
+        }
+        .peel_drop_temps();
+        if let ExprKind::MethodCall(path, iter_expr, args, call_span) = expr.kind
+            && !(self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(def_id))
+            && !args.iter().any(|e| e.span.from_expansion())
+            && !iter_expr.span.from_expansion()
+            && path.ident.name == sym::collect
+            && let iter_ty = cx.typeck_results().expr_ty(iter_expr)
+            && !has_non_owning_mutable_access(cx, iter_ty)
+            && let Some(iterator_trait_id) = cx.tcx.get_diagnostic_item(sym::Iterator)
+            && implements_trait(cx, iter_ty, iterator_trait_id, &[])
+            && let Some(item) = get_iterator_item_ty(cx, iter_ty)
+            && let vec_ty = cx.typeck_results().expr_ty(expr)
+            && let ty::Adt(v, args) = vec_ty.kind()
+            && cx.tcx.is_diagnostic_item(sym::Vec, v.did())
+            // make sure we're collecting to just a vec, and not a vec<result> String or other such tomfoolery
+            && args.first().and_then(|x| x.as_type()) == Some(item)
+        {
+            span_lint_and_then(
+                cx,
+                UNNECESSARY_COLLECT,
+                call_span,
+                "this collection may not be necessary. return the iterator itself".to_string(),
+                |diag| {
+                    diag.span_label(call_span, "remove this collect");
+                    diag.span_suggestion(
+                        decl.output.span(),
+                        "consider",
+                        format!("impl Iterator<Item = {item}>"),
+                        Applicability::MaybeIncorrect,
+                    );
+                },
+            );
+        }
+    }
+}

clippy_utils/src/lib.rs

@@ -2876,12 +2876,13 @@ pub fn span_extract_comment(sm: &SourceMap, span: Span) -> String {
 /// Returns all the comments a given span contains.
 ///
 /// Comments are returned wrapped with their relevant delimiters.
+#[allow(clippy::unnecessary_collect)]
 pub fn span_extract_comments(sm: &SourceMap, span: Span) -> Vec<String> {
     let snippet = sm.span_to_snippet(span).unwrap_or_default();
     tokenize_with_text(&snippet)
         .filter(|(t, ..)| matches!(t, TokenKind::BlockComment { .. } | TokenKind::LineComment { .. }))
         .map(|(_, s, _)| s.to_string())
-        .collect::<Vec<_>>()
+        .collect()
 }
 
 pub fn span_find_starting_semi(sm: &SourceMap, span: Span) -> Span {

clippy_utils/src/str_utils.rs

@@ -153,6 +153,7 @@ pub fn camel_case_indices(s: &str) -> Vec<StrIndex> {
 /// # use clippy_utils::str_utils::{camel_case_split, StrIndex};
 /// assert_eq!(camel_case_split("AbcDef"), vec!["Abc", "Def"]);
 /// ```
+#[allow(clippy::unnecessary_collect)]
 pub fn camel_case_split(s: &str) -> Vec<&str> {
     let mut offsets = camel_case_indices(s)
         .iter()

tests/compile-test.rs

@@ -44,6 +44,7 @@ static INTERNAL_TEST_DEPENDENCIES: &[&str] = &["clippy_config", "clippy_lints",
 /// dependencies must be added to Cargo.toml at the project root. Test
 /// dependencies that are not *directly* used by this test module require an
 /// `extern crate` declaration.
+#[allow(clippy::unnecessary_collect)]
 fn internal_extern_flags() -> Vec<String> {
     let current_exe_path = env::current_exe().unwrap();
     let deps_path = current_exe_path.parent().unwrap();

tests/ui/unnecessary_collect.rs

@@ -0,0 +1,21 @@
+#![warn(clippy::unnecessary_collect)]
+//@no-rustfix
+
+fn bad() -> Vec<u32> {
+    (0..5).collect()
+    //~^ unnecessary_collect
+}
+unsafe fn bad2() -> Vec<(u8, u8)> {
+    (0..8).flat_map(|x| (0..8).map(move |y| (x, y))).collect()
+    //~^ unnecessary_collect
+}
+fn okay() -> String {
+    ('a'..='z').collect()
+}
+fn hmm() -> std::collections::HashSet<u32> {
+    (0..5).chain(3..12).collect()
+}
+fn good() -> impl Iterator<Item = u32> {
+    (5..10).rev()
+}
+fn main() {}

tests/ui/unnecessary_collect.stderr

@@ -0,0 +1,21 @@
+error: this collection may not be necessary. return the iterator itself
+  --> tests/ui/unnecessary_collect.rs:5:12
+   |
+LL | fn bad() -> Vec<u32> {
+   |             -------- help: consider: `impl Iterator<Item = u32>`
+LL |     (0..5).collect()
+   |            ^^^^^^^^^ remove this collect
+   |
+   = note: `-D clippy::unnecessary-collect` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::unnecessary_collect)]`
+
+error: this collection may not be necessary. return the iterator itself
+  --> tests/ui/unnecessary_collect.rs:9:54
+   |
+LL | unsafe fn bad2() -> Vec<(u8, u8)> {
+   |                     ------------- help: consider: `impl Iterator<Item = (u8, u8)>`
+LL |     (0..8).flat_map(|x| (0..8).map(move |y| (x, y))).collect()
+   |                                                      ^^^^^^^^^ remove this collect
+
+error: aborting due to 2 previous errors
+