diff --git a/CHANGELOG.md b/CHANGELOG.md index b6b374e26c96..a86d1556456f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 843aa6a40f09..029a1b981666 100644 --- a/clippy_config/src/conf.rs +++ b/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}" ); diff --git a/clippy_config/src/types.rs b/clippy_config/src/types.rs index 0dd65dfcfd6e..e993f26009ec 100644 --- a/clippy_config/src/types.rs +++ b/clippy_config/src/types.rs @@ -422,8 +422,8 @@ impl SourceItemOrderingModuleItemGroupings { self.back_lut.get(item) } - pub fn grouping_names(&self) -> Vec { - self.groups.iter().map(|(name, _)| name.clone()).collect() + pub fn grouping_names(&self) -> impl Iterator + Clone { + self.groups.iter().map(|(name, _)| &**name) } pub fn is_grouping(&self, grouping: &str) -> bool { diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index dd5c5dcf4d1f..f48dda095649 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/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, diff --git a/clippy_lints/src/doc/suspicious_doc_comments.rs b/clippy_lints/src/doc/suspicious_doc_comments.rs index 47d91b80e7ee..bcf0538641a0 100644 --- a/clippy_lints/src/doc/suspicious_doc_comments.rs +++ b/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() diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index e3168b8cfe02..e85fac26eb5e 100644 --- a/clippy_lints/src/lib.rs +++ b/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` } diff --git a/clippy_lints/src/lifetimes.rs b/clippy_lints/src/lifetimes.rs index 519ec228c884..3a2a6c36ec5e 100644 --- a/clippy_lints/src/lifetimes.rs +++ b/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 { - self.lts - .iter() - .chain(self.nested_elision_site_lts.iter()) - .copied() - .collect::>() + fn all_lts(&self) -> impl Iterator { + 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 diff --git a/clippy_lints/src/non_send_fields_in_send_ty.rs b/clippy_lints/src/non_send_fields_in_send_ty.rs index b810bc01fbdc..49c2e110a3de 100644 --- a/clippy_lints/src/non_send_fields_in_send_ty.rs +++ b/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>` => `vec![P, Q, R]` -fn collect_generic_params(ty: Ty<'_>) -> Vec> { +fn collect_generic_params(ty: Ty<'_>) -> impl Iterator> { 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 diff --git a/clippy_lints/src/unnecessary_collect.rs b/clippy_lints/src/unnecessary_collect.rs new file mode 100644 index 000000000000..8929907ab68e --- /dev/null +++ b/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` where the `T` is produced from a `_.collect::>()` + /// + /// ### 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::>().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::>() + /// } + /// ``` + /// Use instead: + /// ```no_run + /// fn squares() -> impl Iterator { + /// (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 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"), + Applicability::MaybeIncorrect, + ); + }, + ); + } + } +} diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 9ae366305772..f189902bbf23 100644 --- a/clippy_utils/src/lib.rs +++ b/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 { 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::>() + .collect() } pub fn span_find_starting_semi(sm: &SourceMap, span: Span) -> Span { diff --git a/clippy_utils/src/str_utils.rs b/clippy_utils/src/str_utils.rs index f0f82c8dddcf..93502a6746ae 100644 --- a/clippy_utils/src/str_utils.rs +++ b/clippy_utils/src/str_utils.rs @@ -153,6 +153,7 @@ pub fn camel_case_indices(s: &str) -> Vec { /// # 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() diff --git a/tests/compile-test.rs b/tests/compile-test.rs index 1ac688935278..1907d6ebdc52 100644 --- a/tests/compile-test.rs +++ b/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 { let current_exe_path = env::current_exe().unwrap(); let deps_path = current_exe_path.parent().unwrap(); diff --git a/tests/ui/unnecessary_collect.rs b/tests/ui/unnecessary_collect.rs new file mode 100644 index 000000000000..c80d69c9bfef --- /dev/null +++ b/tests/ui/unnecessary_collect.rs @@ -0,0 +1,21 @@ +#![warn(clippy::unnecessary_collect)] +//@no-rustfix + +fn bad() -> Vec { + (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 { + (0..5).chain(3..12).collect() +} +fn good() -> impl Iterator { + (5..10).rev() +} +fn main() {} diff --git a/tests/ui/unnecessary_collect.stderr b/tests/ui/unnecessary_collect.stderr new file mode 100644 index 000000000000..b9c65e2cbb0f --- /dev/null +++ b/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 { + | -------- help: consider: `impl Iterator` +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` +LL | (0..8).flat_map(|x| (0..8).map(move |y| (x, y))).collect() + | ^^^^^^^^^ remove this collect + +error: aborting due to 2 previous errors +