Improve clone_on_copy

Jarcho · Jarcho · commit db5bd9f7ea31 · 2021-03-29T13:45:57.000-04:00
Lint on `_.clone().method()` when method takes self by value
Set applicability correctly
Correct suggestion when the cloned value is a macro call. e.g. `m!(x).clone()`
Don't lint when not using the `Clone` trait
diff --git a/clippy_lints/src/methods/clone_on_copy.rs b/clippy_lints/src/methods/clone_on_copy.rs
@@ -1,23 +1,38 @@
-use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
+use clippy_utils::get_parent_node;
+use clippy_utils::source::snippet_with_context;
 use clippy_utils::sugg;
 use clippy_utils::ty::is_copy;
 use rustc_errors::Applicability;
-use rustc_hir as hir;
+use rustc_hir::{BindingAnnotation, Expr, ExprKind, MatchSource, Node, PatKind};
 use rustc_lint::LateContext;
-use rustc_middle::ty;
+use rustc_middle::ty::{self, adjustment::Adjust};
 use rustc_span::symbol::{sym, Symbol};
 use std::iter;
 
 use super::CLONE_DOUBLE_REF;
 use super::CLONE_ON_COPY;
 
 /// Checks for the `CLONE_ON_COPY` lint.
-pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Symbol, args: &[hir::Expr<'_>]) {
-    if !(args.len() == 1 && method_name == sym::clone) {
+pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, method_name: Symbol, args: &[Expr<'_>]) {
+    let arg = match args {
+        [arg] if method_name == sym::clone => arg,
+        _ => return,
+    };
+    if cx
+        .typeck_results()
+        .type_dependent_def_id(expr.hir_id)
+        .and_then(|id| cx.tcx.trait_of_item(id))
+        .zip(cx.tcx.lang_items().clone_trait())
+        .map_or(true, |(x, y)| x != y)
+    {
         return;
     }
-    let arg = &args[0];
-    let arg_ty = cx.typeck_results().expr_ty_adjusted(&args[0]);
+    let arg_adjustments = cx.typeck_results().expr_adjustments(arg);
+    let arg_ty = arg_adjustments
+        .last()
+        .map_or_else(|| cx.typeck_results().expr_ty(arg), |a| a.target);
+
     let ty = cx.typeck_results().expr_ty(expr);
     if let ty::Ref(_, inner, _) = arg_ty.kind() {
         if let ty::Ref(_, innermost, _) = inner.kind() {
@@ -61,57 +76,57 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Sym
     }
 
     if is_copy(cx, ty) {
-        let snip;
-        if let Some(snippet) = sugg::Sugg::hir_opt(cx, arg) {
-            let parent = cx.tcx.hir().get_parent_node(expr.hir_id);
-            match &cx.tcx.hir().get(parent) {
-                hir::Node::Expr(parent) => match parent.kind {
-                    // &*x is a nop, &x.clone() is not
-                    hir::ExprKind::AddrOf(..) => return,
-                    // (*x).func() is useless, x.clone().func() can work in case func borrows mutably
-                    hir::ExprKind::MethodCall(_, _, parent_args, _) if expr.hir_id == parent_args[0].hir_id => {
-                        return;
-                    },
-
-                    _ => {},
-                },
-                hir::Node::Stmt(stmt) => {
-                    if let hir::StmtKind::Local(ref loc) = stmt.kind {
-                        if let hir::PatKind::Ref(..) = loc.pat.kind {
-                            // let ref y = *x borrows x, let ref y = x.clone() does not
-                            return;
-                        }
-                    }
-                },
-                _ => {},
+        let parent_is_suffix_expr = match get_parent_node(cx.tcx, expr.hir_id) {
+            Some(Node::Expr(parent)) => match parent.kind {
+                // &*x is a nop, &x.clone() is not
+                ExprKind::AddrOf(..) => return,
+                // (*x).func() is useless, x.clone().func() can work in case func borrows self
+                ExprKind::MethodCall(_, _, [self_arg, ..], _)
+                    if expr.hir_id == self_arg.hir_id && ty != cx.typeck_results().expr_ty_adjusted(expr) =>
+                {
+                    return;
+                }
+                ExprKind::MethodCall(_, _, [self_arg, ..], _) if expr.hir_id == self_arg.hir_id => true,
+                ExprKind::Match(_, _, MatchSource::TryDesugar | MatchSource::AwaitDesugar)
+                | ExprKind::Field(..)
+                | ExprKind::Index(..) => true,
+                _ => false,
+            },
+            // local binding capturing a reference
+            Some(Node::Local(l))
+                if matches!(
+                    l.pat.kind,
+                    PatKind::Binding(BindingAnnotation::Ref | BindingAnnotation::RefMut, ..)
+                ) =>
+            {
+                return;
             }
+            _ => false,
+        };
 
-            // x.clone() might have dereferenced x, possibly through Deref impls
-            if cx.typeck_results().expr_ty(arg) == ty {
-                snip = Some(("try removing the `clone` call", format!("{}", snippet)));
-            } else {
-                let deref_count = cx
-                    .typeck_results()
-                    .expr_adjustments(arg)
-                    .iter()
-                    .filter(|adj| matches!(adj.kind, ty::adjustment::Adjust::Deref(_)))
-                    .count();
-                let derefs: String = iter::repeat('*').take(deref_count).collect();
-                snip = Some(("try dereferencing it", format!("{}{}", derefs, snippet)));
-            }
+        let mut app = Applicability::MachineApplicable;
+        let snip = snippet_with_context(cx, arg.span, expr.span.ctxt(), "_", &mut app).0;
+
+        let deref_count = arg_adjustments
+            .iter()
+            .take_while(|adj| matches!(adj.kind, Adjust::Deref(_)))
+            .count();
+        let (help, sugg) = if deref_count == 0 {
+            ("try removing the `clone` call", snip.into())
+        } else if parent_is_suffix_expr {
+            ("try dereferencing it", format!("({}{})", "*".repeat(deref_count), snip))
         } else {
-            snip = None;
-        }
-        span_lint_and_then(
+            ("try dereferencing it", format!("{}{}", "*".repeat(deref_count), snip))
+        };
+
+        span_lint_and_sugg(
             cx,
             CLONE_ON_COPY,
             expr.span,
             &format!("using `clone` on type `{}` which implements the `Copy` trait", ty),
-            |diag| {
-                if let Some((text, snip)) = snip {
-                    diag.span_suggestion(expr.span, text, snip, Applicability::MachineApplicable);
-                }
-            },
+            help,
+            sugg,
+            app,
         );
     }
 }
diff --git a/tests/ui/clone_on_copy.fixed b/tests/ui/clone_on_copy.fixed
@@ -6,7 +6,8 @@
     clippy::deref_addrof,
     clippy::no_effect,
     clippy::unnecessary_operation,
-    clippy::vec_init_then_push
+    clippy::vec_init_then_push,
+    clippy::toplevel_ref_arg
 )]
 
 use std::cell::RefCell;
@@ -29,6 +30,37 @@ fn clone_on_copy() {
     let rc = RefCell::new(0);
     *rc.borrow();
 
+    let x = 0u32;
+    x.rotate_left(1);
+
+    #[derive(Clone, Copy)]
+    struct Foo;
+    impl Foo {
+        fn clone(&self) -> u32 {
+            0
+        }
+    }
+    Foo.clone(); // ok, this is not the clone trait
+
+    macro_rules! m {
+        ($e:expr) => {{ $e }};
+    }
+    m!(42);
+
+    struct Wrap([u32; 2]);
+    impl core::ops::Deref for Wrap {
+        type Target = [u32; 2];
+        fn deref(&self) -> &[u32; 2] {
+            &self.0
+        }
+    }
+    let x = Wrap([0, 0]);
+    (*x)[0];
+
+    let x = 42;
+    let ref y = x.clone(); // ok, binds by reference
+    let ref mut y = x.clone(); // ok, binds by reference
+
     // Issue #4348
     let mut x = 43;
     let _ = &x.clone(); // ok, getting a ref
diff --git a/tests/ui/clone_on_copy.rs b/tests/ui/clone_on_copy.rs
@@ -6,7 +6,8 @@
     clippy::deref_addrof,
     clippy::no_effect,
     clippy::unnecessary_operation,
-    clippy::vec_init_then_push
+    clippy::vec_init_then_push,
+    clippy::toplevel_ref_arg
 )]
 
 use std::cell::RefCell;
@@ -29,6 +30,37 @@ fn clone_on_copy() {
     let rc = RefCell::new(0);
     rc.borrow().clone();
 
+    let x = 0u32;
+    x.clone().rotate_left(1);
+
+    #[derive(Clone, Copy)]
+    struct Foo;
+    impl Foo {
+        fn clone(&self) -> u32 {
+            0
+        }
+    }
+    Foo.clone(); // ok, this is not the clone trait
+
+    macro_rules! m {
+        ($e:expr) => {{ $e }};
+    }
+    m!(42).clone();
+
+    struct Wrap([u32; 2]);
+    impl core::ops::Deref for Wrap {
+        type Target = [u32; 2];
+        fn deref(&self) -> &[u32; 2] {
+            &self.0
+        }
+    }
+    let x = Wrap([0, 0]);
+    x.clone()[0];
+
+    let x = 42;
+    let ref y = x.clone(); // ok, binds by reference
+    let ref mut y = x.clone(); // ok, binds by reference
+
     // Issue #4348
     let mut x = 43;
     let _ = &x.clone(); // ok, getting a ref
diff --git a/tests/ui/clone_on_copy.stderr b/tests/ui/clone_on_copy.stderr
@@ -1,34 +1,52 @@
 error: using `clone` on type `i32` which implements the `Copy` trait
-  --> $DIR/clone_on_copy.rs:23:5
+  --> $DIR/clone_on_copy.rs:24:5
    |
 LL |     42.clone();
    |     ^^^^^^^^^^ help: try removing the `clone` call: `42`
    |
    = note: `-D clippy::clone-on-copy` implied by `-D warnings`
 
 error: using `clone` on type `i32` which implements the `Copy` trait
-  --> $DIR/clone_on_copy.rs:27:5
+  --> $DIR/clone_on_copy.rs:28:5
    |
 LL |     (&42).clone();
    |     ^^^^^^^^^^^^^ help: try dereferencing it: `*(&42)`
 
 error: using `clone` on type `i32` which implements the `Copy` trait
-  --> $DIR/clone_on_copy.rs:30:5
+  --> $DIR/clone_on_copy.rs:31:5
    |
 LL |     rc.borrow().clone();
    |     ^^^^^^^^^^^^^^^^^^^ help: try dereferencing it: `*rc.borrow()`
 
+error: using `clone` on type `u32` which implements the `Copy` trait
+  --> $DIR/clone_on_copy.rs:34:5
+   |
+LL |     x.clone().rotate_left(1);
+   |     ^^^^^^^^^ help: try removing the `clone` call: `x`
+
+error: using `clone` on type `i32` which implements the `Copy` trait
+  --> $DIR/clone_on_copy.rs:48:5
+   |
+LL |     m!(42).clone();
+   |     ^^^^^^^^^^^^^^ help: try removing the `clone` call: `m!(42)`
+
+error: using `clone` on type `[u32; 2]` which implements the `Copy` trait
+  --> $DIR/clone_on_copy.rs:58:5
+   |
+LL |     x.clone()[0];
+   |     ^^^^^^^^^ help: try dereferencing it: `(*x)`
+
 error: using `clone` on type `char` which implements the `Copy` trait
-  --> $DIR/clone_on_copy.rs:36:14
+  --> $DIR/clone_on_copy.rs:68:14
    |
 LL |     is_ascii('z'.clone());
    |              ^^^^^^^^^^^ help: try removing the `clone` call: `'z'`
 
 error: using `clone` on type `i32` which implements the `Copy` trait
-  --> $DIR/clone_on_copy.rs:40:14
+  --> $DIR/clone_on_copy.rs:72:14
    |
 LL |     vec.push(42.clone());
    |              ^^^^^^^^^^ help: try removing the `clone` call: `42`
 
-error: aborting due to 5 previous errors
+error: aborting due to 8 previous errors
 
diff --git a/tests/ui/clone_on_copy_mut.rs b/tests/ui/clone_on_copy_mut.rs
