lint on unnecessary collections

CHANGELOG.md

@@ -6966,6 +6966,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, InherentImplLintScope, MacroMatcher, MatchLintBehaviour,
@@ -966,10 +967,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

@@ -751,6 +751,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

@@ -361,6 +361,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;
@@ -847,6 +848,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
  Box::new(|_| Box::new(coerce_container_to_any::CoerceContainerToAny)),
  Box::new(|_| Box::new(toplevel_ref_arg::ToplevelRefArg)),
  Box::new(|_| Box::new(volatile_composites::VolatileComposites)),
+ Box::new(move |_| Box::new(unnecessary_collect::UnnecessaryCollect::new(conf))),
  Box::new(|_| Box::<replace_box::ReplaceBox>::default()),
  // add late passes here, used by `cargo dev 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).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: iter_generic_params(field_ty).collect(),
  });
  }
  }
@@ -169,15 +169,14 @@ 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<'_>> {
+/// Example: `MyStruct<P, Box<Q, R>>` => `[P, Q, R]`
+fn iter_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,118 @@
+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;
+use rustc_span::Span;
+
+use rustc_hir::{Block, 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 often more performant to 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.
+ /// If all uses of this function `collect` anyways, one ought simply `#[expect(clippy::unnecessary_collect)]`.
+ ///
+ /// ### 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,
+ nursery,
+ "checks for functions returning vecs produced from vec::from_iter"
+}
+
+pub struct UnnecessaryCollect {
+ avoid_breaking_exported_api: bool,
+}
+
+impl UnnecessaryCollect {
+ pub fn new(
+ &Conf {
+ avoid_breaking_exported_api,
+ ..
+ }: &'static Conf,
+ ) -> Self {
+ Self {
+ 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,
+ ) {
+ if let FnKind::ItemFn(..) | FnKind::Method(..) = kind
+ && let ExprKind::Block(
+ Block { expr: Some(expr), .. }
+ | Block { stmts: [.., Stmt { kind: StmtKind::Expr(expr), .. }], .. },
+ _,
+ ) = &body.value.kind
+ && let expr = expr.peel_drop_temps()
+ && 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())
+ && 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

@@ -2777,12 +2777,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
+