Lint explicit Clone implementations on Copy type

README.md

@@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
 [Jump to usage instructions](#usage)
 
 ##Lints
-There are 97 lints included in this crate:
+There are 98 lints included in this crate:
 
 name                                                                                                           | default | meaning
 ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -30,6 +30,7 @@ name
 [duplicate_underscore_argument](https://github.com/Manishearth/rust-clippy/wiki#duplicate_underscore_argument) | warn    | Function arguments having names which only differ by an underscore
 [empty_loop](https://github.com/Manishearth/rust-clippy/wiki#empty_loop)                                       | warn    | empty `loop {}` detected
 [eq_op](https://github.com/Manishearth/rust-clippy/wiki#eq_op)                                                 | warn    | equal operands on both sides of a comparison or bitwise combination (e.g. `x == x`)
+[expl_impl_clone_on_copy](https://github.com/Manishearth/rust-clippy/wiki#expl_impl_clone_on_copy)             | warn    | implementing `Clone` explicitly on `Copy` types
 [explicit_counter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_counter_loop)                 | warn    | for-looping with an explicit counter when `_.enumerate()` would do
 [explicit_iter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_iter_loop)                       | warn    | for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do
 [filter_next](https://github.com/Manishearth/rust-clippy/wiki#filter_next)                                     | warn    | using `filter(p).next()`, which is more succinctly expressed as `.find(p)`

src/derive.rs

@@ -1,13 +1,15 @@
 use rustc::lint::*;
+use rustc::middle::ty::fast_reject::simplify_type;
+use rustc::middle::ty;
 use rustc_front::hir::*;
 use syntax::ast::{Attribute, MetaItem_};
+use syntax::codemap::Span;
+use utils::{CLONE_TRAIT_PATH, HASH_PATH};
 use utils::{match_path, span_lint_and_then};
-use utils::HASH_PATH;
+use rustc::middle::ty::TypeVariants;
-
-use rustc::middle::ty::fast_reject::simplify_type;
 
 /// **What it does:** This lint warns about deriving `Hash` but implementing `PartialEq`
-/// explicitely.
+/// explicitly.
 ///
 /// **Why is this bad?** The implementation of these traits must agree (for example for use with
 /// `HashMap`) so it’s probably a bad idea to use a default-generated `Hash` implementation  with
@@ -33,66 +35,145 @@ declare_lint! {
     "deriving `Hash` but implementing `PartialEq` explicitly"
 }
 
+/// **What it does:** This lint warns about explicit `Clone` implementation for `Copy` types.
+///
+/// **Why is this bad?** To avoid surprising behaviour, these traits should agree and the behaviour
+/// of `Copy` cannot be overridden. In almost all situations a `Copy` type should have a `Clone`
+/// implementation that does nothing more than copy the object, which is what
+/// `#[derive(Copy, Clone)]` gets you.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// #[derive(Copy)]
+/// struct Foo;
+///
+/// impl Clone for Foo {
+///     ..
+/// }
+declare_lint! {
+    pub EXPL_IMPL_CLONE_ON_COPY,
+    Warn,
+    "implementing `Clone` explicitly on `Copy` types"
+}
+
 pub struct Derive;
 
 impl LintPass for Derive {
     fn get_lints(&self) -> LintArray {
-        lint_array!(DERIVE_HASH_NOT_EQ)
+        lint_array!(EXPL_IMPL_CLONE_ON_COPY, DERIVE_HASH_NOT_EQ)
     }
 }
 
 impl LateLintPass for Derive {
     fn check_item(&mut self, cx: &LateContext, item: &Item) {
-        /// A `#[derive]`d implementation has a `#[automatically_derived]` attribute.
+        let ast_ty_to_ty_cache = cx.tcx.ast_ty_to_ty_cache.borrow();
-        fn is_automatically_derived(attr: &Attribute) -> bool {
-            if let MetaItem_::MetaWord(ref word) = attr.node.value.node {
-                word == &"automatically_derived"
-            }
-            else {
-                false
-            }
-        }
 
-        // If `item` is an automatically derived `Hash` implementation
         if_let_chain! {[
             let ItemImpl(_, _, _, Some(ref trait_ref), ref ast_ty, _) = item.node,
-            match_path(&trait_ref.path, &HASH_PATH),
+            let Some(&ty) = ast_ty_to_ty_cache.get(&ast_ty.id)
-            item.attrs.iter().any(is_automatically_derived),
-            let Some(peq_trait_def_id) = cx.tcx.lang_items.eq_trait()
         ], {
-            let peq_trait_def = cx.tcx.lookup_trait_def(peq_trait_def_id);
+            if item.attrs.iter().any(is_automatically_derived) {
-
+                check_hash_peq(cx, item.span, trait_ref, ty);
-            cx.tcx.populate_implementations_for_trait_if_necessary(peq_trait_def.trait_ref.def_id);
+            }
-            let peq_impls = peq_trait_def.borrow_impl_lists(cx.tcx).1;
+            else {
-            let ast_ty_to_ty_cache = cx.tcx.ast_ty_to_ty_cache.borrow();
+                check_copy_clone(cx, item.span, trait_ref, ty);
-
+            }
-
-            // Look for the PartialEq implementations for `ty`
-            if_let_chain! {[
-                let Some(ty) = ast_ty_to_ty_cache.get(&ast_ty.id),
-                let Some(simpl_ty) = simplify_type(cx.tcx, ty, false),
-                let Some(impl_ids) = peq_impls.get(&simpl_ty)
-            ], {
-                for &impl_id in impl_ids {
-                    let trait_ref = cx.tcx.impl_trait_ref(impl_id).expect("must be a trait implementation");
-
-                    // Only care about `impl PartialEq<Foo> for Foo`
-                    if trait_ref.input_types()[0] == *ty &&
-                      !cx.tcx.get_attrs(impl_id).iter().any(is_automatically_derived) {
-                        span_lint_and_then(
-                            cx, DERIVE_HASH_NOT_EQ, item.span,
-                            &format!("you are deriving `Hash` but have implemented \
-                                      `PartialEq` explicitely"), |db| {
-                            if let Some(node_id) = cx.tcx.map.as_local_node_id(impl_id) {
-                                db.span_note(
-                                    cx.tcx.map.span(node_id),
-                                    "`PartialEq` implemented here"
-                                );
-                            }
-                        });
-                    }
-                }
-            }}
         }}
     }
 }
+
+/// Implementation of the `DERIVE_HASH_NOT_EQ` lint.
+fn check_hash_peq(cx: &LateContext, span: Span, trait_ref: &TraitRef, ty: ty::Ty) {
+    // If `item` is an automatically derived `Hash` implementation
+    if_let_chain! {[
+        match_path(&trait_ref.path, &HASH_PATH),
+        let Some(peq_trait_def_id) = cx.tcx.lang_items.eq_trait()
+    ], {
+        let peq_trait_def = cx.tcx.lookup_trait_def(peq_trait_def_id);
+
+        cx.tcx.populate_implementations_for_trait_if_necessary(peq_trait_def.trait_ref.def_id);
+        let peq_impls = peq_trait_def.borrow_impl_lists(cx.tcx).1;
+
+        // Look for the PartialEq implementations for `ty`
+        if_let_chain! {[
+            let Some(simpl_ty) = simplify_type(cx.tcx, ty, false),
+            let Some(impl_ids) = peq_impls.get(&simpl_ty)
+        ], {
+            for &impl_id in impl_ids {
+                let trait_ref = cx.tcx.impl_trait_ref(impl_id).expect("must be a trait implementation");
+
+                // Only care about `impl PartialEq<Foo> for Foo`
+                if trait_ref.input_types()[0] == ty &&
+                  !cx.tcx.get_attrs(impl_id).iter().any(is_automatically_derived) {
+                    span_lint_and_then(
+                        cx, DERIVE_HASH_NOT_EQ, span,
+                        "you are deriving `Hash` but have implemented `PartialEq` explicitly",
+                        |db| {
+                        if let Some(node_id) = cx.tcx.map.as_local_node_id(impl_id) {
+                            db.span_note(
+                                cx.tcx.map.span(node_id),
+                                "`PartialEq` implemented here"
+                            );
+                        }
+                    });
+                }
+            }
+        }}
+    }}
+}
+
+/// Implementation of the `EXPL_IMPL_CLONE_ON_COPY` lint.
+fn check_copy_clone<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, span: Span, trait_ref: &TraitRef, ty: ty::Ty<'tcx>) {
+    if match_path(&trait_ref.path, &CLONE_TRAIT_PATH) {
+        let parameter_environment = cx.tcx.empty_parameter_environment();
+
+        if ty.moves_by_default(&parameter_environment, span) {
+            return; // ty is not Copy
+        }
+
+        // Some types are not Clone by default but could be cloned `by hand` if necessary
+        match ty.sty {
+            TypeVariants::TyEnum(def, substs) | TypeVariants::TyStruct(def, substs) => {
+                for variant in &def.variants {
+                    for field in &variant.fields {
+                        match field.ty(cx.tcx, substs).sty {
+                            TypeVariants::TyArray(_, size) if size > 32 => {
+                                return;
+                            }
+                            TypeVariants::TyBareFn(..) => {
+                                return;
+                            }
+                            TypeVariants::TyTuple(ref tys) if tys.len() > 12 => {
+                                return;
+                            }
+                            _ => (),
+                        }
+                    }
+                }
+            }
+            _ => (),
+        }
+
+        span_lint_and_then(
+            cx, DERIVE_HASH_NOT_EQ, span,
+            "you are implementing `Clone` explicitly on a `Copy` type",
+            |db| {
+                db.span_note(
+                    span,
+                    "consider deriving `Clone` or removing `Copy`"
+                );
+        });
+    }
+}
+
+/// Checks for the `#[automatically_derived]` attribute all `#[derive]`d implementations have.
+fn is_automatically_derived(attr: &Attribute) -> bool {
+    if let MetaItem_::MetaWord(ref word) = attr.node.value.node {
+        word == &"automatically_derived"
+    }
+    else {
+        false
+    }
+}

src/lib.rs

@@ -173,6 +173,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         collapsible_if::COLLAPSIBLE_IF,
         cyclomatic_complexity::CYCLOMATIC_COMPLEXITY,
         derive::DERIVE_HASH_NOT_EQ,
+        derive::EXPL_IMPL_CLONE_ON_COPY,
         entry::MAP_ENTRY,
         eq_op::EQ_OP,
         escape::BOXED_LOCAL,

src/utils.rs

@@ -22,7 +22,8 @@ pub type MethodArgs = HirVec<P<Expr>>;
 pub const BEGIN_UNWIND: [&'static str; 3] = ["std", "rt", "begin_unwind"];
 pub const BTREEMAP_ENTRY_PATH: [&'static str; 4] = ["collections", "btree", "map", "Entry"];
 pub const BTREEMAP_PATH: [&'static str; 4] = ["collections", "btree", "map", "BTreeMap"];
-pub const CLONE_PATH: [&'static str; 2] = ["Clone", "clone"];
+pub const CLONE_PATH: [&'static str; 3] = ["clone", "Clone", "clone"];
+pub const CLONE_TRAIT_PATH: [&'static str; 2] = ["clone", "Clone"];
 pub const COW_PATH: [&'static str; 3] = ["collections", "borrow", "Cow"];
 pub const DEFAULT_TRAIT_PATH: [&'static str; 3] = ["core", "default", "Default"];
 pub const HASHMAP_ENTRY_PATH: [&'static str; 5] = ["std", "collections", "hash", "map", "Entry"];

tests/compile-fail/derive.rs

@@ -2,6 +2,7 @@
 #![plugin(clippy)]
 
 #![deny(warnings)]
+#![allow(dead_code)]
 
 #[derive(PartialEq, Hash)]
 struct Foo;
@@ -11,7 +12,7 @@ impl PartialEq<u64> for Foo {
 }
 
 #[derive(Hash)]
-//~^ ERROR you are deriving `Hash` but have implemented `PartialEq` explicitely
+//~^ ERROR you are deriving `Hash` but have implemented `PartialEq` explicitly
 struct Bar;
 
 impl PartialEq for Bar {
@@ -19,11 +20,49 @@ impl PartialEq for Bar {
 }
 
 #[derive(Hash)]
-//~^ ERROR you are deriving `Hash` but have implemented `PartialEq` explicitely
+//~^ ERROR you are deriving `Hash` but have implemented `PartialEq` explicitly
 struct Baz;
 
 impl PartialEq<Baz> for Baz {
     fn eq(&self, _: &Baz) -> bool { true }
 }
 
+#[derive(Copy)]
+struct Qux;
+
+impl Clone for Qux {
+//~^ ERROR you are implementing `Clone` explicitly on a `Copy` type
+    fn clone(&self) -> Self { Qux }
+}
+
+// Ok, `Clone` cannot be derived because of the big array
+#[derive(Copy)]
+struct BigArray {
+    a: [u8; 65],
+}
+
+impl Clone for BigArray {
+    fn clone(&self) -> Self { unimplemented!() }
+}
+
+// Ok, function pointers are not always Clone
+#[derive(Copy)]
+struct FnPtr {
+    a: fn() -> !,
+}
+
+impl Clone for FnPtr {
+    fn clone(&self) -> Self { unimplemented!() }
+}
+
+// Ok, generics
+#[derive(Copy)]
+struct Generic<T> {
+    a: T,
+}
+
+impl<T> Clone for Generic<T> {
+    fn clone(&self) -> Self { unimplemented!() }
+}
+
 fn main() {}