From 16158139605059b76bf72f948e90f063ee918414 Mon Sep 17 00:00:00 2001 From: Adam Lusch Date: Sun, 14 Jan 2018 19:58:09 -0800 Subject: [PATCH 1/3] Moves `clone_on_ref_ptr` to be a restriction lint Also updates the suggestion to include the full type (e.g. `Arc::clone(&rc)`) and adds a case using trait objects to the UI tests. --- clippy_lints/src/methods.rs | 39 ++++++++++++++++++----------------- tests/ui/unnecessary_clone.rs | 8 ++++++- 2 files changed, 27 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 64335f81a..8803c4e30 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -361,9 +361,8 @@ declare_lint! { /// ```rust /// x.clone() /// ``` -declare_lint! { +declare_restriction_lint! { pub CLONE_ON_REF_PTR, - Warn, "using 'clone' on a ref-counted pointer" } @@ -1013,24 +1012,26 @@ fn lint_clone_on_copy(cx: &LateContext, expr: &hir::Expr, arg: &hir::Expr, arg_t fn lint_clone_on_ref_ptr(cx: &LateContext, expr: &hir::Expr, arg: &hir::Expr) { let (obj_ty, _) = walk_ptrs_ty_depth(cx.tables.expr_ty(arg)); - let caller_type = if match_type(cx, obj_ty, &paths::RC) { - "Rc" - } else if match_type(cx, obj_ty, &paths::ARC) { - "Arc" - } else if match_type(cx, obj_ty, &paths::WEAK_RC) || match_type(cx, obj_ty, &paths::WEAK_ARC) { - "Weak" - } else { - return; - }; + if let ty::TyAdt(_, subst) = obj_ty.sty { + let caller_type = if match_type(cx, obj_ty, &paths::RC) { + "Rc" + } else if match_type(cx, obj_ty, &paths::ARC) { + "Arc" + } else if match_type(cx, obj_ty, &paths::WEAK_RC) || match_type(cx, obj_ty, &paths::WEAK_ARC) { + "Weak" + } else { + return; + }; - span_lint_and_sugg( - cx, - CLONE_ON_REF_PTR, - expr.span, - "using '.clone()' on a ref-counted pointer", - "try this", - format!("{}::clone(&{})", caller_type, snippet(cx, arg.span, "_")), - ); + span_lint_and_sugg( + cx, + CLONE_ON_REF_PTR, + expr.span, + "using '.clone()' on a ref-counted pointer", + "try this", + format!("{}<{}>::clone(&{})", caller_type, subst.type_at(0), snippet(cx, arg.span, "_")), + ); + } } diff --git a/tests/ui/unnecessary_clone.rs b/tests/ui/unnecessary_clone.rs index f33def9eb..96166ed4f 100644 --- a/tests/ui/unnecessary_clone.rs +++ b/tests/ui/unnecessary_clone.rs @@ -1,3 +1,4 @@ +#![warn(clone_on_ref_ptr)] #![allow(unused)] use std::collections::HashSet; @@ -5,6 +6,10 @@ use std::collections::VecDeque; use std::rc::{self, Rc}; use std::sync::{self, Arc}; +trait SomeTrait {} +struct SomeImpl; +impl SomeTrait for SomeImpl {} + fn main() {} fn clone_on_copy() { @@ -34,7 +39,8 @@ fn clone_on_ref_ptr() { arc_weak.clone(); sync::Weak::clone(&arc_weak); - + let x = Arc::new(SomeImpl); + let _: Arc = x.clone(); } fn clone_on_copy_generic(t: T) { From 30de2e71065dc0af710de34d72db20679ddd5c6a Mon Sep 17 00:00:00 2001 From: Adam Lusch Date: Sun, 14 Jan 2018 20:10:36 -0800 Subject: [PATCH 2/3] Update UI test expected output --- tests/ui/unnecessary_clone.stderr | 58 +++++++++++++++++-------------- 1 file changed, 32 insertions(+), 26 deletions(-) diff --git a/tests/ui/unnecessary_clone.stderr b/tests/ui/unnecessary_clone.stderr index 437df1ee9..298a93934 100644 --- a/tests/ui/unnecessary_clone.stderr +++ b/tests/ui/unnecessary_clone.stderr @@ -1,75 +1,81 @@ error: using `clone` on a `Copy` type - --> $DIR/unnecessary_clone.rs:11:5 + --> $DIR/unnecessary_clone.rs:16:5 | -11 | 42.clone(); +16 | 42.clone(); | ^^^^^^^^^^ help: try removing the `clone` call: `42` | = note: `-D clone-on-copy` implied by `-D warnings` error: using `clone` on a `Copy` type - --> $DIR/unnecessary_clone.rs:15:5 + --> $DIR/unnecessary_clone.rs:20:5 | -15 | (&42).clone(); +20 | (&42).clone(); | ^^^^^^^^^^^^^ help: try dereferencing it: `*(&42)` error: using '.clone()' on a ref-counted pointer - --> $DIR/unnecessary_clone.rs:25:5 + --> $DIR/unnecessary_clone.rs:30:5 | -25 | rc.clone(); - | ^^^^^^^^^^ help: try this: `Rc::clone(&rc)` +30 | rc.clone(); + | ^^^^^^^^^^ help: try this: `Rc::clone(&rc)` | = note: `-D clone-on-ref-ptr` implied by `-D warnings` error: using '.clone()' on a ref-counted pointer - --> $DIR/unnecessary_clone.rs:28:5 + --> $DIR/unnecessary_clone.rs:33:5 | -28 | arc.clone(); - | ^^^^^^^^^^^ help: try this: `Arc::clone(&arc)` +33 | arc.clone(); + | ^^^^^^^^^^^ help: try this: `Arc::clone(&arc)` error: using '.clone()' on a ref-counted pointer - --> $DIR/unnecessary_clone.rs:31:5 + --> $DIR/unnecessary_clone.rs:36:5 | -31 | rcweak.clone(); - | ^^^^^^^^^^^^^^ help: try this: `Weak::clone(&rcweak)` +36 | rcweak.clone(); + | ^^^^^^^^^^^^^^ help: try this: `Weak::clone(&rcweak)` error: using '.clone()' on a ref-counted pointer - --> $DIR/unnecessary_clone.rs:34:5 + --> $DIR/unnecessary_clone.rs:39:5 | -34 | arc_weak.clone(); - | ^^^^^^^^^^^^^^^^ help: try this: `Weak::clone(&arc_weak)` +39 | arc_weak.clone(); + | ^^^^^^^^^^^^^^^^ help: try this: `Weak::clone(&arc_weak)` + +error: using '.clone()' on a ref-counted pointer + --> $DIR/unnecessary_clone.rs:43:29 + | +43 | let _: Arc = x.clone(); + | ^^^^^^^^^ help: try this: `Arc::clone(&x)` error: using `clone` on a `Copy` type - --> $DIR/unnecessary_clone.rs:41:5 + --> $DIR/unnecessary_clone.rs:47:5 | -41 | t.clone(); +47 | t.clone(); | ^^^^^^^^^ help: try removing the `clone` call: `t` error: using `clone` on a `Copy` type - --> $DIR/unnecessary_clone.rs:43:5 + --> $DIR/unnecessary_clone.rs:49:5 | -43 | Some(t).clone(); +49 | 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:49:22 + --> $DIR/unnecessary_clone.rs:55:22 | -49 | let z: &Vec<_> = y.clone(); +55 | let z: &Vec<_> = y.clone(); | ^^^^^^^^^ | = note: `-D clone-double-ref` implied by `-D warnings` help: try dereferencing it | -49 | let z: &Vec<_> = &(*y).clone(); +55 | let z: &Vec<_> = &(*y).clone(); | ^^^^^^^^^^^^^ help: or try being explicit about what type to clone | -49 | let z: &Vec<_> = &std::vec::Vec::clone(y); +55 | let z: &Vec<_> = &std::vec::Vec::clone(y); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: called `cloned().collect()` on a slice to create a `Vec`. Calling `to_vec()` is both faster and more readable - --> $DIR/unnecessary_clone.rs:56:27 + --> $DIR/unnecessary_clone.rs:62:27 | -56 | let v2 : Vec = v.iter().cloned().collect(); +62 | let v2 : Vec = v.iter().cloned().collect(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D iter-cloned-collect` implied by `-D warnings` From f343cd22f63d0d769dd3bfd44b53347d2d4a58c0 Mon Sep 17 00:00:00 2001 From: Adam Lusch Date: Sun, 14 Jan 2018 20:19:55 -0800 Subject: [PATCH 3/3] Adds the missing turbofish --- clippy_lints/src/methods.rs | 2 +- tests/ui/unnecessary_clone.stderr | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 8803c4e30..8a92e4934 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -1029,7 +1029,7 @@ fn lint_clone_on_ref_ptr(cx: &LateContext, expr: &hir::Expr, arg: &hir::Expr) { expr.span, "using '.clone()' on a ref-counted pointer", "try this", - format!("{}<{}>::clone(&{})", caller_type, subst.type_at(0), snippet(cx, arg.span, "_")), + format!("{}::<{}>::clone(&{})", caller_type, subst.type_at(0), snippet(cx, arg.span, "_")), ); } } diff --git a/tests/ui/unnecessary_clone.stderr b/tests/ui/unnecessary_clone.stderr index 298a93934..bb78bfa16 100644 --- a/tests/ui/unnecessary_clone.stderr +++ b/tests/ui/unnecessary_clone.stderr @@ -16,7 +16,7 @@ error: using '.clone()' on a ref-counted pointer --> $DIR/unnecessary_clone.rs:30:5 | 30 | rc.clone(); - | ^^^^^^^^^^ help: try this: `Rc::clone(&rc)` + | ^^^^^^^^^^ help: try this: `Rc::::clone(&rc)` | = note: `-D clone-on-ref-ptr` implied by `-D warnings` @@ -24,25 +24,25 @@ error: using '.clone()' on a ref-counted pointer --> $DIR/unnecessary_clone.rs:33:5 | 33 | arc.clone(); - | ^^^^^^^^^^^ help: try this: `Arc::clone(&arc)` + | ^^^^^^^^^^^ help: try this: `Arc::::clone(&arc)` error: using '.clone()' on a ref-counted pointer --> $DIR/unnecessary_clone.rs:36:5 | 36 | rcweak.clone(); - | ^^^^^^^^^^^^^^ help: try this: `Weak::clone(&rcweak)` + | ^^^^^^^^^^^^^^ help: try this: `Weak::::clone(&rcweak)` error: using '.clone()' on a ref-counted pointer --> $DIR/unnecessary_clone.rs:39:5 | 39 | arc_weak.clone(); - | ^^^^^^^^^^^^^^^^ help: try this: `Weak::clone(&arc_weak)` + | ^^^^^^^^^^^^^^^^ help: try this: `Weak::::clone(&arc_weak)` error: using '.clone()' on a ref-counted pointer --> $DIR/unnecessary_clone.rs:43:29 | 43 | let _: Arc = x.clone(); - | ^^^^^^^^^ help: try this: `Arc::clone(&x)` + | ^^^^^^^^^ help: try this: `Arc::::clone(&x)` error: using `clone` on a `Copy` type --> $DIR/unnecessary_clone.rs:47:5