diff --git a/README.md b/README.md index 5848759c2b8f..83ff6daa9f58 100644 --- a/README.md +++ b/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)` diff --git a/src/derive.rs b/src/derive.rs index 6306bb8f09d5..b1c0bde40a2e 100644 --- a/src/derive.rs +++ b/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::fast_reject::simplify_type; +use rustc::middle::ty::TypeVariants; /// **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. - fn is_automatically_derived(attr: &Attribute) -> bool { - if let MetaItem_::MetaWord(ref word) = attr.node.value.node { - word == &"automatically_derived" - } - else { - false - } - } + let ast_ty_to_ty_cache = cx.tcx.ast_ty_to_ty_cache.borrow(); - // 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), - item.attrs.iter().any(is_automatically_derived), - let Some(peq_trait_def_id) = cx.tcx.lang_items.eq_trait() + let Some(&ty) = ast_ty_to_ty_cache.get(&ast_ty.id) ], { - 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; - let ast_ty_to_ty_cache = cx.tcx.ast_ty_to_ty_cache.borrow(); - - - // 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 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" - ); - } - }); - } - } - }} + if item.attrs.iter().any(is_automatically_derived) { + check_hash_peq(cx, item.span, trait_ref, ty); + } + else { + check_copy_clone(cx, item.span, trait_ref, ty); + } }} } } + +/// 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 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(¶meter_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 + } +} diff --git a/src/lib.rs b/src/lib.rs index cd69ac23c19a..c43c01268fea 100644 --- a/src/lib.rs +++ b/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, diff --git a/src/utils.rs b/src/utils.rs index 7b9866d85302..139e94dbc912 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -22,7 +22,8 @@ pub type MethodArgs = HirVec>; 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"]; diff --git a/tests/compile-fail/derive.rs b/tests/compile-fail/derive.rs index a879be282925..66b04a66d0fc 100755 --- a/tests/compile-fail/derive.rs +++ b/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 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 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 { + a: T, +} + +impl Clone for Generic { + fn clone(&self) -> Self { unimplemented!() } +} + fn main() {}