From 0462666c7067d8992501aa8997d16e18da021ee5 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Fri, 16 Apr 2021 11:00:08 -0500 Subject: [PATCH 1/2] Add cloned_instead_of_copied lint --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 2 + .../src/methods/cloned_instead_of_copied.rs | 38 +++++++++++++++++++ clippy_lints/src/methods/mod.rs | 26 +++++++++++++ clippy_utils/src/ty.rs | 21 +++++++++- tests/ui/cloned_instead_of_copied.fixed | 15 ++++++++ tests/ui/cloned_instead_of_copied.rs | 15 ++++++++ tests/ui/cloned_instead_of_copied.stderr | 34 +++++++++++++++++ 8 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/methods/cloned_instead_of_copied.rs create mode 100644 tests/ui/cloned_instead_of_copied.fixed create mode 100644 tests/ui/cloned_instead_of_copied.rs create mode 100644 tests/ui/cloned_instead_of_copied.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 7885bdfb12c2b..483365b5e91c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2148,6 +2148,7 @@ Released 2018-09-13 [`clone_double_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_double_ref [`clone_on_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy [`clone_on_ref_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_ref_ptr +[`cloned_instead_of_copied`]: https://rust-lang.github.io/rust-clippy/master/index.html#cloned_instead_of_copied [`cmp_nan`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_nan [`cmp_null`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_null [`cmp_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_owned diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 93631c5669b27..0f48799d339bd 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -759,6 +759,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: methods::BYTES_NTH, methods::CHARS_LAST_CMP, methods::CHARS_NEXT_CMP, + methods::CLONED_INSTEAD_OF_COPIED, methods::CLONE_DOUBLE_REF, methods::CLONE_ON_COPY, methods::CLONE_ON_REF_PTR, @@ -1380,6 +1381,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(matches::MATCH_WILDCARD_FOR_SINGLE_VARIANTS), LintId::of(matches::MATCH_WILD_ERR_ARM), LintId::of(matches::SINGLE_MATCH_ELSE), + LintId::of(methods::CLONED_INSTEAD_OF_COPIED), LintId::of(methods::FILTER_MAP_NEXT), LintId::of(methods::IMPLICIT_CLONE), LintId::of(methods::INEFFICIENT_TO_STRING), diff --git a/clippy_lints/src/methods/cloned_instead_of_copied.rs b/clippy_lints/src/methods/cloned_instead_of_copied.rs new file mode 100644 index 0000000000000..ba97ab3900ca4 --- /dev/null +++ b/clippy_lints/src/methods/cloned_instead_of_copied.rs @@ -0,0 +1,38 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::is_trait_method; +use clippy_utils::ty::{get_iterator_item_ty, is_copy}; +use rustc_errors::Applicability; +use rustc_hir::Expr; +use rustc_lint::LateContext; +use rustc_middle::ty; +use rustc_span::{sym, Span}; + +use super::CLONED_INSTEAD_OF_COPIED; + +pub fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span: Span) { + let recv_ty = cx.typeck_results().expr_ty_adjusted(recv); + let inner_ty = match recv_ty.kind() { + // `Option` -> `T` + ty::Adt(adt, subst) if cx.tcx.is_diagnostic_item(sym::option_type, adt.did) => subst.type_at(0), + _ if is_trait_method(cx, expr, sym::Iterator) => match get_iterator_item_ty(cx, recv_ty) { + // ::Item + Some(ty) => ty, + _ => return, + }, + _ => return, + }; + match inner_ty.kind() { + // &T where T: Copy + ty::Ref(_, ty, _) if is_copy(cx, ty) => {}, + _ => return, + }; + span_lint_and_sugg( + cx, + CLONED_INSTEAD_OF_COPIED, + span, + "used `cloned` where `copied` could be used instead", + "try", + "copied".into(), + Applicability::MachineApplicable, + ) +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 6c21ff0f6c22c..49a9af2a46581 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -8,6 +8,7 @@ mod chars_next_cmp; mod chars_next_cmp_with_unwrap; mod clone_on_copy; mod clone_on_ref_ptr; +mod cloned_instead_of_copied; mod expect_fun_call; mod expect_used; mod filetype_is_file; @@ -73,6 +74,29 @@ use rustc_span::symbol::SymbolStr; use rustc_span::{sym, Span}; use rustc_typeck::hir_ty_to_ty; +declare_clippy_lint! { + /// **What it does:** Checks for usages of `cloned()` on an `Iterator` or `Option` where + /// `copied()` could be used instead. + /// + /// **Why is this bad?** `copied()` is better because it guarantees that the type being cloned + /// implements `Copy`. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// [1, 2, 3].iter().cloned(); + /// ``` + /// Use instead: + /// ```rust + /// [1, 2, 3].iter().copied(); + /// ``` + pub CLONED_INSTEAD_OF_COPIED, + pedantic, + "used `cloned` where `copied` could be used instead" +} + declare_clippy_lint! { /// **What it does:** Checks for `.unwrap()` calls on `Option`s and on `Result`s. /// @@ -1638,6 +1662,7 @@ impl_lint_pass!(Methods => [ CLONE_ON_COPY, CLONE_ON_REF_PTR, CLONE_DOUBLE_REF, + CLONED_INSTEAD_OF_COPIED, INEFFICIENT_TO_STRING, NEW_RET_NO_SELF, SINGLE_CHAR_PATTERN, @@ -1909,6 +1934,7 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio ("as_mut", []) => useless_asref::check(cx, expr, "as_mut", recv), ("as_ref", []) => useless_asref::check(cx, expr, "as_ref", recv), ("assume_init", []) => uninit_assumed_init::check(cx, expr, recv), + ("cloned", []) => cloned_instead_of_copied::check(cx, expr, recv, span), ("collect", []) => match method_call!(recv) { Some(("cloned", [recv2], _)) => iter_cloned_collect::check(cx, expr, recv2), Some(("map", [m_recv, m_arg], _)) => { diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index 807cfbc4c7f1f..64a80f2554fa4 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -13,7 +13,7 @@ use rustc_lint::LateContext; use rustc_middle::ty::subst::{GenericArg, GenericArgKind}; use rustc_middle::ty::{self, AdtDef, IntTy, Ty, TypeFoldable, UintTy}; use rustc_span::sym; -use rustc_span::symbol::Symbol; +use rustc_span::symbol::{Ident, Symbol}; use rustc_span::DUMMY_SP; use rustc_trait_selection::traits::query::normalize::AtExt; @@ -52,6 +52,25 @@ pub fn contains_adt_constructor(ty: Ty<'_>, adt: &AdtDef) -> bool { }) } +/// Resolves `::Item` for `T` +/// Do not invoke without first verifying that the type implements `Iterator` +pub fn get_iterator_item_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { + cx.tcx + .get_diagnostic_item(sym::Iterator) + .and_then(|iter_did| { + cx.tcx.associated_items(iter_did).find_by_name_and_kind( + cx.tcx, + Ident::from_str("Item"), + ty::AssocKind::Type, + iter_did, + ) + }) + .map(|assoc| { + let proj = cx.tcx.mk_projection(assoc.def_id, cx.tcx.mk_substs_trait(ty, &[])); + cx.tcx.normalize_erasing_regions(cx.param_env, proj) + }) +} + /// Returns true if ty has `iter` or `iter_mut` methods pub fn has_iter_method(cx: &LateContext<'_>, probably_ref_ty: Ty<'_>) -> Option { // FIXME: instead of this hard-coded list, we should check if `::iter` diff --git a/tests/ui/cloned_instead_of_copied.fixed b/tests/ui/cloned_instead_of_copied.fixed new file mode 100644 index 0000000000000..4eb999e18e64e --- /dev/null +++ b/tests/ui/cloned_instead_of_copied.fixed @@ -0,0 +1,15 @@ +// run-rustfix +#![warn(clippy::cloned_instead_of_copied)] + +fn main() { + // yay + let _ = [1].iter().copied(); + let _ = vec!["hi"].iter().copied(); + let _ = Some(&1).copied(); + let _ = Box::new([1].iter()).copied(); + let _ = Box::new(Some(&1)).copied(); + + // nay + let _ = [String::new()].iter().cloned(); + let _ = Some(&String::new()).cloned(); +} diff --git a/tests/ui/cloned_instead_of_copied.rs b/tests/ui/cloned_instead_of_copied.rs new file mode 100644 index 0000000000000..894496c0ebbb5 --- /dev/null +++ b/tests/ui/cloned_instead_of_copied.rs @@ -0,0 +1,15 @@ +// run-rustfix +#![warn(clippy::cloned_instead_of_copied)] + +fn main() { + // yay + let _ = [1].iter().cloned(); + let _ = vec!["hi"].iter().cloned(); + let _ = Some(&1).cloned(); + let _ = Box::new([1].iter()).cloned(); + let _ = Box::new(Some(&1)).cloned(); + + // nay + let _ = [String::new()].iter().cloned(); + let _ = Some(&String::new()).cloned(); +} diff --git a/tests/ui/cloned_instead_of_copied.stderr b/tests/ui/cloned_instead_of_copied.stderr new file mode 100644 index 0000000000000..e0707d3214689 --- /dev/null +++ b/tests/ui/cloned_instead_of_copied.stderr @@ -0,0 +1,34 @@ +error: used `cloned` where `copied` could be used instead + --> $DIR/cloned_instead_of_copied.rs:6:24 + | +LL | let _ = [1].iter().cloned(); + | ^^^^^^ help: try: `copied` + | + = note: `-D clippy::cloned-instead-of-copied` implied by `-D warnings` + +error: used `cloned` where `copied` could be used instead + --> $DIR/cloned_instead_of_copied.rs:7:31 + | +LL | let _ = vec!["hi"].iter().cloned(); + | ^^^^^^ help: try: `copied` + +error: used `cloned` where `copied` could be used instead + --> $DIR/cloned_instead_of_copied.rs:8:22 + | +LL | let _ = Some(&1).cloned(); + | ^^^^^^ help: try: `copied` + +error: used `cloned` where `copied` could be used instead + --> $DIR/cloned_instead_of_copied.rs:9:34 + | +LL | let _ = Box::new([1].iter()).cloned(); + | ^^^^^^ help: try: `copied` + +error: used `cloned` where `copied` could be used instead + --> $DIR/cloned_instead_of_copied.rs:10:32 + | +LL | let _ = Box::new(Some(&1)).cloned(); + | ^^^^^^ help: try: `copied` + +error: aborting due to 5 previous errors + From b049c88fbe661f2ef1f1f7806788fed59dc1bfa8 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Fri, 16 Apr 2021 11:07:08 -0500 Subject: [PATCH 2/2] Eat dogfood --- clippy_lints/src/booleans.rs | 2 +- clippy_lints/src/checked_conversions.rs | 4 ++-- clippy_lints/src/loops/never_loop.rs | 2 +- clippy_lints/src/matches.rs | 2 +- clippy_lints/src/needless_pass_by_value.rs | 2 +- clippy_lints/src/suspicious_operation_groupings.rs | 2 +- clippy_lints/src/write.rs | 2 +- clippy_utils/src/hir_utils.rs | 2 +- clippy_utils/src/lib.rs | 6 +++--- 9 files changed, 12 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/booleans.rs b/clippy_lints/src/booleans.rs index 58d9aa9c005c2..67f0e0c78700b 100644 --- a/clippy_lints/src/booleans.rs +++ b/clippy_lints/src/booleans.rs @@ -261,7 +261,7 @@ fn simplify_not(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { } METHODS_WITH_NEGATION .iter() - .cloned() + .copied() .flat_map(|(a, b)| vec![(a, b), (b, a)]) .find(|&(a, _)| { let path: &str = &path.ident.name.as_str(); diff --git a/clippy_lints/src/checked_conversions.rs b/clippy_lints/src/checked_conversions.rs index d7136f84cc3af..6a2666bc6c011 100644 --- a/clippy_lints/src/checked_conversions.rs +++ b/clippy_lints/src/checked_conversions.rs @@ -323,7 +323,7 @@ fn get_implementing_type<'a>(path: &QPath<'_>, candidates: &'a [&str], function: if let [int] = &*tp.segments; then { let name = &int.ident.name.as_str(); - candidates.iter().find(|c| name == *c).cloned() + candidates.iter().find(|c| name == *c).copied() } else { None } @@ -337,7 +337,7 @@ fn int_ty_to_sym<'tcx>(path: &QPath<'_>) -> Option<&'tcx str> { if let [ty] = &*path.segments; then { let name = &ty.ident.name.as_str(); - INTS.iter().find(|c| name == *c).cloned() + INTS.iter().find(|c| name == *c).copied() } else { None } diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs index 96720764e1658..e97b7c9417033 100644 --- a/clippy_lints/src/loops/never_loop.rs +++ b/clippy_lints/src/loops/never_loop.rs @@ -100,7 +100,7 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult { ExprKind::Binary(_, e1, e2) | ExprKind::Assign(e1, e2, _) | ExprKind::AssignOp(_, e1, e2) - | ExprKind::Index(e1, e2) => never_loop_expr_all(&mut [e1, e2].iter().cloned(), main_loop_id), + | ExprKind::Index(e1, e2) => never_loop_expr_all(&mut [e1, e2].iter().copied(), main_loop_id), ExprKind::Loop(b, _, _, _) => { // Break can come from the inner loop so remove them. absorb_break(&never_loop_block(b, main_loop_id)) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index e4d1451b36918..f4102709d7ee5 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -1124,7 +1124,7 @@ fn check_wild_enum_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>]) Applicability::MaybeIncorrect, ), variants => { - let mut suggestions: Vec<_> = variants.iter().cloned().map(format_suggestion).collect(); + let mut suggestions: Vec<_> = variants.iter().copied().map(format_suggestion).collect(); let message = if adt_def.is_variant_list_non_exhaustive() { suggestions.push("_".into()); "wildcard matches known variants and will also match future added variants" diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs index 780e224129347..e33a33e238633 100644 --- a/clippy_lints/src/needless_pass_by_value.rs +++ b/clippy_lints/src/needless_pass_by_value.rs @@ -279,7 +279,7 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByValue { spans.extend( deref_span .iter() - .cloned() + .copied() .map(|span| (span, format!("*{}", snippet(cx, span, "")))), ); spans.sort_by_key(|&(span, _)| span); diff --git a/clippy_lints/src/suspicious_operation_groupings.rs b/clippy_lints/src/suspicious_operation_groupings.rs index cb2237e531262..4272935bc310e 100644 --- a/clippy_lints/src/suspicious_operation_groupings.rs +++ b/clippy_lints/src/suspicious_operation_groupings.rs @@ -195,7 +195,7 @@ fn attempt_to_emit_no_difference_lint( i: usize, expected_loc: IdentLocation, ) { - if let Some(binop) = binops.get(i).cloned() { + if let Some(binop) = binops.get(i).copied() { // We need to try and figure out which identifier we should // suggest using instead. Since there could be multiple // replacement candidates in a given expression, and we're diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs index 12a47a6b7036d..7e962472c07f5 100644 --- a/clippy_lints/src/write.rs +++ b/clippy_lints/src/write.rs @@ -573,7 +573,7 @@ impl Write { diag.multipart_suggestion( "try this", iter::once((comma_span.to(token_expr.span), String::new())) - .chain(fmt_spans.iter().cloned().zip(iter::repeat(replacement))) + .chain(fmt_spans.iter().copied().zip(iter::repeat(replacement))) .collect(), Applicability::MachineApplicable, ); diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs index f695f1a61e716..3ad1ac75e560f 100644 --- a/clippy_utils/src/hir_utils.rs +++ b/clippy_utils/src/hir_utils.rs @@ -424,7 +424,7 @@ fn reduce_exprkind<'hir>(cx: &LateContext<'_>, kind: &'hir ExprKind<'hir>) -> &' TokenKind::LineComment { .. } | TokenKind::BlockComment { .. } | TokenKind::Whitespace ) }) - .ne([TokenKind::OpenBrace, TokenKind::CloseBrace].iter().cloned()) => + .ne([TokenKind::OpenBrace, TokenKind::CloseBrace].iter().copied()) => { kind }, diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 6c8a3302aa4af..a5986ed00daf5 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1053,7 +1053,7 @@ pub fn is_refutable(cx: &LateContext<'_>, pat: &Pat<'_>) -> bool { /// the function once on the given pattern. pub fn recurse_or_patterns<'tcx, F: FnMut(&'tcx Pat<'tcx>)>(pat: &'tcx Pat<'tcx>, mut f: F) { if let PatKind::Or(pats) = pat.kind { - pats.iter().cloned().for_each(f) + pats.iter().copied().for_each(f) } else { f(pat) } @@ -1230,14 +1230,14 @@ pub fn match_any_def_paths(cx: &LateContext<'_>, did: DefId, paths: &[&[&str]]) let search_path = cx.get_def_path(did); paths .iter() - .position(|p| p.iter().map(|x| Symbol::intern(x)).eq(search_path.iter().cloned())) + .position(|p| p.iter().map(|x| Symbol::intern(x)).eq(search_path.iter().copied())) } /// Checks if the given `DefId` matches the path. pub fn match_def_path<'tcx>(cx: &LateContext<'tcx>, did: DefId, syms: &[&str]) -> bool { // We should probably move to Symbols in Clippy as well rather than interning every time. let path = cx.get_def_path(did); - syms.iter().map(|x| Symbol::intern(x)).eq(path.iter().cloned()) + syms.iter().map(|x| Symbol::intern(x)).eq(path.iter().copied()) } pub fn match_panic_call(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {