Auto merge of rust-lang#7098 - camsteffen:cloned-copied, r=Manishearth

Add `cloned_instead_of_copied` lint Don't go cloning all willy-nilly. Featuring a new `get_iterator_item_ty` util! changelog: Add cloned_instead_of_copied lint Closes rust-lang#3870
flip1995 · Apr 16, 2021 · 28dbcd8 · 28dbcd8
2 parents 7f2068c + b049c88
commit 28dbcd8
Show file tree

Hide file tree

Showing 17 changed files with 163 additions and 13 deletions.
diff --git 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/booleans.rs b/clippy_lints/src/booleans.rs
@@ -261,7 +261,7 @@ fn simplify_not(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<String> {
             }
             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
@@ -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/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/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
@@ -1129,7 +1129,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/methods/cloned_instead_of_copied.rs 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>` -> `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) {
+            // <T as Iterator>::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
@@ -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_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, "<expr>")))),
                             );
                             spans.sort_by_key(|&(span, _)| span);

diff --git 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
@@ -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
@@ -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
@@ -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>> {

diff --git 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 `<T as Iterator>::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<Ty<'tcx>> {
+    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<Symbol> {
     // FIXME: instead of this hard-coded list, we should check if `<adt>::iter`

diff --git a/tests/ui/cloned_instead_of_copied.fixed 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
@@ -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
@@ -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
+