Merge pull request #2037 from Aaron1011/clone-rc

Add CLONE_ON_REF_PTR lint

clippy_lints/src/consts.rs

@@ -171,7 +171,7 @@ pub fn lit_to_constant<'a, 'tcx>(lit: &LitKind, tcx: TyCtxt<'a, 'tcx, 'tcx>, mut
     match *lit {
         LitKind::Str(ref is, style) => Constant::Str(is.to_string(), style),
         LitKind::Byte(b) => Constant::Int(ConstInt::U8(b)),
-        LitKind::ByteStr(ref s) => Constant::Binary(s.clone()),
+        LitKind::ByteStr(ref s) => Constant::Binary(Rc::clone(s)),
         LitKind::Char(c) => Constant::Char(c),
         LitKind::Int(n, hint) => match (&ty.sty, hint) {
             (&ty::TyInt(ity), _) | (_, Signed(ity)) => {

clippy_lints/src/methods.rs

@@ -310,6 +310,24 @@ declare_lint! {
     "using `clone` on a `Copy` type"
 }
 
+/// **What it does:** Checks for usage of `.clone()` on a ref-counted pointer,
+/// (Rc, Arc, rc::Weak, or sync::Weak), and suggests calling Clone on
+/// the corresponding trait instead.
+///
+/// **Why is this bad?**: Calling '.clone()' on an Rc, Arc, or Weak
+/// can obscure the fact that only the pointer is being cloned, not the underlying
+/// data.
+///
+/// **Example:**
+/// ```rust
+/// x.clone()
+/// ```
+declare_lint! {
+    pub CLONE_ON_REF_PTR,
+    Warn,
+    "using 'clone' on a ref-counted pointer"
+}
+
 /// **What it does:** Checks for usage of `.clone()` on an `&&T`.
 ///
 /// **Why is this bad?** Cloning an `&&T` copies the inner `&T`, instead of
@@ -539,6 +557,7 @@ impl LintPass for Pass {
             OR_FUN_CALL,
             CHARS_NEXT_CMP,
             CLONE_ON_COPY,
+            CLONE_ON_REF_PTR,
             CLONE_DOUBLE_REF,
             NEW_RET_NO_SELF,
             SINGLE_CHAR_PATTERN,
@@ -615,6 +634,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
                 let self_ty = cx.tables.expr_ty_adjusted(&args[0]);
                 if args.len() == 1 && method_call.name == "clone" {
                     lint_clone_on_copy(cx, expr, &args[0], self_ty);
+                    lint_clone_on_ref_ptr(cx, expr, &args[0]);
                 }
 
                 match self_ty.sty {
@@ -853,6 +873,34 @@ 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;
+    };
+
+    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, "_")
+        )
+    );
+
+}
+
+
 fn lint_string_extend(cx: &LateContext, expr: &hir::Expr, args: &[hir::Expr]) {
     let arg = &args[1];
     if let Some(arglists) = method_chain_args(arg, &["chars"]) {

clippy_lints/src/utils/paths.rs

@@ -2,6 +2,7 @@
 //! about.
 
 pub const ANY_TRAIT: [&'static str; 3] = ["std", "any", "Any"];
+pub const ARC: [&'static str; 3] = ["alloc", "arc", "Arc"];
 pub const ASMUT_TRAIT: [&'static str; 3] = ["core", "convert", "AsMut"];
 pub const ASREF_TRAIT: [&'static str; 3] = ["core", "convert", "AsRef"];
 pub const BEGIN_PANIC: [&'static str; 3] = ["std", "panicking", "begin_panic"];
@@ -58,6 +59,7 @@ pub const RANGE_TO: [&'static str; 3] = ["core", "ops", "RangeTo"];
 pub const RANGE_TO_INCLUSIVE: [&'static str; 3] = ["core", "ops", "RangeToInclusive"];
 pub const RANGE_TO_INCLUSIVE_STD: [&'static str; 3] = ["std", "ops", "RangeToInclusive"];
 pub const RANGE_TO_STD: [&'static str; 3] = ["std", "ops", "RangeTo"];
+pub const RC: [&'static str; 3] = ["alloc", "rc", "Rc"];
 pub const REGEX: [&'static str; 3] = ["regex", "re_unicode", "Regex"];
 pub const REGEX_BUILDER_NEW: [&'static str; 5] = ["regex", "re_builder", "unicode", "RegexBuilder", "new"];
 pub const REGEX_BYTES: [&'static str; 3] = ["regex", "re_bytes", "Regex"];
@@ -81,3 +83,5 @@ pub const TRY_INTO_RESULT: [&'static str; 4] = ["std", "ops", "Try", "into_resul
 pub const VEC: [&'static str; 3] = ["alloc", "vec", "Vec"];
 pub const VEC_DEQUE: [&'static str; 3] = ["alloc", "vec_deque", "VecDeque"];
 pub const VEC_FROM_ELEM: [&'static str; 3] = ["alloc", "vec", "from_elem"];
+pub const WEAK_ARC: [&'static str; 3] = ["alloc", "arc", "Weak"];
+pub const WEAK_RC: [&'static str; 3] = ["alloc", "rc", "Weak"];

tests/ui/methods.rs

@@ -11,6 +11,8 @@ use std::collections::HashSet;
 use std::collections::VecDeque;
 use std::ops::Mul;
 use std::iter::FromIterator;
+use std::rc::{self, Rc};
+use std::sync::{self, Arc};
 
 struct T;
 
@@ -456,6 +458,28 @@ fn clone_on_copy() {
     (&42).clone();
 }
 
+fn clone_on_ref_ptr() {
+    let rc = Rc::new(true);
+    let arc = Arc::new(true);
+
+    let rcweak = Rc::downgrade(&rc);
+    let arc_weak = Arc::downgrade(&arc);
+
+    rc.clone();
+    Rc::clone(&rc);
+
+    arc.clone();
+    Arc::clone(&arc);
+
+    rcweak.clone();
+    rc::Weak::clone(&rcweak);
+
+    arc_weak.clone();
+    sync::Weak::clone(&arc_weak);
+
+
+}
+
 fn clone_on_copy_generic<T: Copy>(t: T) {
     t.clone();