From 68a1af540cc65c7ebc8804f615ade76359b7d2dc Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Mon, 19 Aug 2019 08:17:53 +0200 Subject: [PATCH] Fix `clone_on_copy` false positives Closes #4384 --- clippy_lints/src/methods/mod.rs | 40 +++++++++++++++---------------- tests/ui/unnecessary_clone.rs | 5 ++++ tests/ui/unnecessary_clone.stderr | 24 +++++++++---------- 3 files changed, 37 insertions(+), 32 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index c8e3ca14ab11..92ee50310f2b 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1521,30 +1521,30 @@ fn lint_clone_on_copy(cx: &LateContext<'_, '_>, expr: &hir::Expr, arg: &hir::Exp 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.node { + // &*x is a nop, &x.clone() is not + hir::ExprKind::AddrOf(..) | + // (*x).func() is useless, x.clone().func() can work in case func borrows mutably + hir::ExprKind::MethodCall(..) => return, + _ => {}, + }, + hir::Node::Stmt(stmt) => { + if let hir::StmtKind::Local(ref loc) = stmt.node { + if let hir::PatKind::Ref(..) = loc.pat.node { + // let ref y = *x borrows x, let ref y = x.clone() does not + return; + } + } + }, + _ => {}, + } + // x.clone() might have dereferenced x, possibly through Deref impls if cx.tables.expr_ty(arg) == ty { snip = Some(("try removing the `clone` call", format!("{}", snippet))); } else { - let parent = cx.tcx.hir().get_parent_node(expr.hir_id); - match cx.tcx.hir().get(parent) { - hir::Node::Expr(parent) => match parent.node { - // &*x is a nop, &x.clone() is not - hir::ExprKind::AddrOf(..) | - // (*x).func() is useless, x.clone().func() can work in case func borrows mutably - hir::ExprKind::MethodCall(..) => return, - _ => {}, - }, - hir::Node::Stmt(stmt) => { - if let hir::StmtKind::Local(ref loc) = stmt.node { - if let hir::PatKind::Ref(..) = loc.pat.node { - // let ref y = *x borrows x, let ref y = x.clone() does not - return; - } - } - }, - _ => {}, - } - let deref_count = cx .tables .expr_adjustments(arg) diff --git a/tests/ui/unnecessary_clone.rs b/tests/ui/unnecessary_clone.rs index 2e0fc1227781..4ff835115909 100644 --- a/tests/ui/unnecessary_clone.rs +++ b/tests/ui/unnecessary_clone.rs @@ -22,6 +22,11 @@ fn clone_on_copy() { let rc = RefCell::new(0); rc.borrow().clone(); + + // Issue #4348 + let mut x = 43; + let _ = &x.clone(); // ok, getting a ref + 'a'.clone().make_ascii_uppercase(); // ok, clone and then mutate } fn clone_on_ref_ptr() { diff --git a/tests/ui/unnecessary_clone.stderr b/tests/ui/unnecessary_clone.stderr index 79b99ff1d1d2..b1388044c42c 100644 --- a/tests/ui/unnecessary_clone.stderr +++ b/tests/ui/unnecessary_clone.stderr @@ -19,7 +19,7 @@ LL | rc.borrow().clone(); | ^^^^^^^^^^^^^^^^^^^ help: try dereferencing it: `*rc.borrow()` error: using '.clone()' on a ref-counted pointer - --> $DIR/unnecessary_clone.rs:34:5 + --> $DIR/unnecessary_clone.rs:39:5 | LL | rc.clone(); | ^^^^^^^^^^ help: try this: `Rc::::clone(&rc)` @@ -27,43 +27,43 @@ LL | rc.clone(); = note: `-D clippy::clone-on-ref-ptr` implied by `-D warnings` error: using '.clone()' on a ref-counted pointer - --> $DIR/unnecessary_clone.rs:37:5 + --> $DIR/unnecessary_clone.rs:42:5 | LL | arc.clone(); | ^^^^^^^^^^^ help: try this: `Arc::::clone(&arc)` error: using '.clone()' on a ref-counted pointer - --> $DIR/unnecessary_clone.rs:40:5 + --> $DIR/unnecessary_clone.rs:45:5 | LL | rcweak.clone(); | ^^^^^^^^^^^^^^ help: try this: `Weak::::clone(&rcweak)` error: using '.clone()' on a ref-counted pointer - --> $DIR/unnecessary_clone.rs:43:5 + --> $DIR/unnecessary_clone.rs:48:5 | LL | arc_weak.clone(); | ^^^^^^^^^^^^^^^^ help: try this: `Weak::::clone(&arc_weak)` error: using '.clone()' on a ref-counted pointer - --> $DIR/unnecessary_clone.rs:47:33 + --> $DIR/unnecessary_clone.rs:52:33 | LL | let _: Arc = x.clone(); | ^^^^^^^^^ help: try this: `Arc::::clone(&x)` error: using `clone` on a `Copy` type - --> $DIR/unnecessary_clone.rs:51:5 + --> $DIR/unnecessary_clone.rs:56:5 | LL | t.clone(); | ^^^^^^^^^ help: try removing the `clone` call: `t` error: using `clone` on a `Copy` type - --> $DIR/unnecessary_clone.rs:53:5 + --> $DIR/unnecessary_clone.rs:58:5 | LL | Some(t).clone(); | ^^^^^^^^^^^^^^^ help: try removing the `clone` call: `Some(t)` error: using `clone` on a double-reference; this will copy the reference instead of cloning the inner type - --> $DIR/unnecessary_clone.rs:59:22 + --> $DIR/unnecessary_clone.rs:64:22 | LL | let z: &Vec<_> = y.clone(); | ^^^^^^^^^ @@ -79,7 +79,7 @@ LL | let z: &Vec<_> = &std::vec::Vec::clone(y); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: called `iter().cloned().collect()` on a slice to create a `Vec`. Calling `to_vec()` is both faster and more readable - --> $DIR/unnecessary_clone.rs:66:27 + --> $DIR/unnecessary_clone.rs:71:27 | LL | let v2: Vec = v.iter().cloned().collect(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.to_vec()` @@ -87,13 +87,13 @@ LL | let v2: Vec = v.iter().cloned().collect(); = note: `-D clippy::iter-cloned-collect` implied by `-D warnings` error: called `iter().cloned().collect()` on a slice to create a `Vec`. Calling `to_vec()` is both faster and more readable - --> $DIR/unnecessary_clone.rs:71:38 + --> $DIR/unnecessary_clone.rs:76:38 | LL | let _: Vec = vec![1, 2, 3].iter().cloned().collect(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.to_vec()` error: called `iter().cloned().collect()` on a slice to create a `Vec`. Calling `to_vec()` is both faster and more readable - --> $DIR/unnecessary_clone.rs:76:24 + --> $DIR/unnecessary_clone.rs:81:24 | LL | .to_bytes() | ________________________^ @@ -103,7 +103,7 @@ LL | | .collect(); | |______________________^ help: try: `.to_vec()` error: using `clone` on a `Copy` type - --> $DIR/unnecessary_clone.rs:114:20 + --> $DIR/unnecessary_clone.rs:119:20 | LL | let _: E = a.clone(); | ^^^^^^^^^ help: try dereferencing it: `*****a`