Skip to content

Commit 8ef0b86

Browse files
mcartonmcarton
committed
Lint explicit Clone implementations on Copy type
1 parent 30c7ea8 commit 8ef0b86

File tree

5 files changed

+170
-47
lines changed

5 files changed

+170
-47
lines changed

‎README.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
66
[Jump to usage instructions](#usage)
77

88
##Lints
9-
There are 97 lints included in this crate:
9+
There are 98 lints included in this crate:
1010

1111
name | default | meaning
1212
---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -30,6 +30,7 @@ name
3030
[duplicate_underscore_argument](https://github.com/Manishearth/rust-clippy/wiki#duplicate_underscore_argument) | warn | Function arguments having names which only differ by an underscore
3131
[empty_loop](https://github.com/Manishearth/rust-clippy/wiki#empty_loop) | warn | empty `loop {}` detected
3232
[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`)
33+
[expl_impl_clone_on_copy](https://github.com/Manishearth/rust-clippy/wiki#expl_impl_clone_on_copy) | warn | implementing `Clone` explicitly on `Copy` types
3334
[explicit_counter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_counter_loop) | warn | for-looping with an explicit counter when `_.enumerate()` would do
3435
[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
3536
[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

Lines changed: 124 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,15 @@
11
use rustc::lint::*;
2+
use rustc::middle::ty::fast_reject::simplify_type;
3+
use rustc::middle::ty;
24
use rustc_front::hir::*;
35
use syntax::ast::{Attribute, MetaItem_};
6+
use syntax::codemap::Span;
7+
use utils::{CLONE_TRAIT_PATH, HASH_PATH};
48
use utils::{match_path, span_lint_and_then};
5-
use utils::HASH_PATH;
6-
7-
use rustc::middle::ty::fast_reject::simplify_type;
9+
use rustc::middle::ty::TypeVariants;
810

911
/// **What it does:** This lint warns about deriving `Hash` but implementing `PartialEq`
10-
/// explicitely.
12+
/// explicitly.
1113
///
1214
/// **Why is this bad?** The implementation of these traits must agree (for example for use with
1315
/// `HashMap`) so it’s probably a bad idea to use a default-generated `Hash` implementation with
@@ -33,66 +35,145 @@ declare_lint! {
3335
"deriving `Hash` but implementing `PartialEq` explicitly"
3436
}
3537

38+
/// **What it does:** This lint warns about explicit `Clone` implementation for `Copy` types.
39+
///
40+
/// **Why is this bad?** To avoid surprising behaviour, these traits should agree and the behaviour
41+
/// of `Copy` cannot be overridden. In almost all situations a `Copy` type should have a `Clone`
42+
/// implementation that does nothing more than copy the object, which is what
43+
/// `#[derive(Copy, Clone)]` gets you.
44+
///
45+
/// **Known problems:** None.
46+
///
47+
/// **Example:**
48+
/// ```rust
49+
/// #[derive(Copy)]
50+
/// struct Foo;
51+
///
52+
/// impl Clone for Foo {
53+
/// ..
54+
/// }
55+
declare_lint! {
56+
pub EXPL_IMPL_CLONE_ON_COPY,
57+
Warn,
58+
"implementing `Clone` explicitly on `Copy` types"
59+
}
60+
3661
pub struct Derive;
3762

3863
impl LintPass for Derive {
3964
fn get_lints(&self) -> LintArray {
40-
lint_array!(DERIVE_HASH_NOT_EQ)
65+
lint_array!(EXPL_IMPL_CLONE_ON_COPY, DERIVE_HASH_NOT_EQ)
4166
}
4267
}
4368

4469
impl LateLintPass for Derive {
4570
fn check_item(&mut self, cx: &LateContext, item: &Item) {
46-
/// A `#[derive]`d implementation has a `#[automatically_derived]` attribute.
47-
fn is_automatically_derived(attr: &Attribute) -> bool {
48-
if let MetaItem_::MetaWord(ref word) = attr.node.value.node {
49-
word == &"automatically_derived"
71+
let ast_ty_to_ty_cache = cx.tcx.ast_ty_to_ty_cache.borrow();
72+
73+
if_let_chain! {[
74+
let ItemImpl(_, _, _, Some(ref trait_ref), ref ast_ty, _) = item.node,
75+
let Some(&ty) = ast_ty_to_ty_cache.get(&ast_ty.id)
76+
], {
77+
if item.attrs.iter().any(is_automatically_derived) {
78+
check_hash_peq(cx, item.span, trait_ref, ty);
5079
}
5180
else {
52-
false
81+
check_copy_clone(cx, item.span, trait_ref, ty);
5382
}
54-
}
83+
}}
84+
}
85+
}
5586

56-
// If `item` is an automatically derived `Hash` implementation
87+
/// Implementation of the `DERIVE_HASH_NOT_EQ` lint.
88+
fn check_hash_peq(cx: &LateContext, span: Span, trait_ref: &TraitRef, ty: ty::Ty) {
89+
// If `item` is an automatically derived `Hash` implementation
90+
if_let_chain! {[
91+
match_path(&trait_ref.path, &HASH_PATH),
92+
let Some(peq_trait_def_id) = cx.tcx.lang_items.eq_trait()
93+
], {
94+
let peq_trait_def = cx.tcx.lookup_trait_def(peq_trait_def_id);
95+
96+
cx.tcx.populate_implementations_for_trait_if_necessary(peq_trait_def.trait_ref.def_id);
97+
let peq_impls = peq_trait_def.borrow_impl_lists(cx.tcx).1;
98+
99+
// Look for the PartialEq implementations for `ty`
57100
if_let_chain! {[
58-
let ItemImpl(_, _, _, Some(ref trait_ref), ref ast_ty, _) = item.node,
59-
match_path(&trait_ref.path, &HASH_PATH),
60-
item.attrs.iter().any(is_automatically_derived),
61-
let Some(peq_trait_def_id) = cx.tcx.lang_items.eq_trait()
101+
let Some(simpl_ty) = simplify_type(cx.tcx, ty, false),
102+
let Some(impl_ids) = peq_impls.get(&simpl_ty)
62103
], {
63-
let peq_trait_def = cx.tcx.lookup_trait_def(peq_trait_def_id);
104+
for &impl_id in impl_ids {
105+
let trait_ref = cx.tcx.impl_trait_ref(impl_id).expect("must be a trait implementation");
64106

65-
cx.tcx.populate_implementations_for_trait_if_necessary(peq_trait_def.trait_ref.def_id);
66-
let peq_impls = peq_trait_def.borrow_impl_lists(cx.tcx).1;
67-
let ast_ty_to_ty_cache = cx.tcx.ast_ty_to_ty_cache.borrow();
107+
// Only care about `impl PartialEq<Foo> for Foo`
108+
if trait_ref.input_types()[0] == ty &&
109+
!cx.tcx.get_attrs(impl_id).iter().any(is_automatically_derived) {
110+
span_lint_and_then(
111+
cx, DERIVE_HASH_NOT_EQ, span,
112+
"you are deriving `Hash` but have implemented `PartialEq` explicitly",
113+
|db| {
114+
if let Some(node_id) = cx.tcx.map.as_local_node_id(impl_id) {
115+
db.span_note(
116+
cx.tcx.map.span(node_id),
117+
"`PartialEq` implemented here"
118+
);
119+
}
120+
});
121+
}
122+
}
123+
}}
124+
}}
125+
}
68126

127+
/// Implementation of the `EXPL_IMPL_CLONE_ON_COPY` lint.
128+
fn check_copy_clone<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, span: Span, trait_ref: &TraitRef, ty: ty::Ty<'tcx>) {
129+
if match_path(&trait_ref.path, &CLONE_TRAIT_PATH) {
130+
let parameter_environment = cx.tcx.empty_parameter_environment();
69131

70-
// Look for the PartialEq implementations for `ty`
71-
if_let_chain! {[
72-
let Some(ty) = ast_ty_to_ty_cache.get(&ast_ty.id),
73-
let Some(simpl_ty) = simplify_type(cx.tcx, ty, false),
74-
let Some(impl_ids) = peq_impls.get(&simpl_ty)
75-
], {
76-
for &impl_id in impl_ids {
77-
let trait_ref = cx.tcx.impl_trait_ref(impl_id).expect("must be a trait implementation");
132+
if ty.moves_by_default(&parameter_environment, span) {
133+
return; // ty is not Copy
134+
}
78135

79-
// Only care about `impl PartialEq<Foo> for Foo`
80-
if trait_ref.input_types()[0] == *ty &&
81-
!cx.tcx.get_attrs(impl_id).iter().any(is_automatically_derived) {
82-
span_lint_and_then(
83-
cx, DERIVE_HASH_NOT_EQ, item.span,
84-
&format!("you are deriving `Hash` but have implemented \
85-
`PartialEq` explicitely"), |db| {
86-
if let Some(node_id) = cx.tcx.map.as_local_node_id(impl_id) {
87-
db.span_note(
88-
cx.tcx.map.span(node_id),
89-
"`PartialEq` implemented here"
90-
);
136+
// Some types are not Clone by default but could be cloned `by hand` if necessary
137+
match ty.sty {
138+
TypeVariants::TyEnum(def, substs) | TypeVariants::TyStruct(def, substs) => {
139+
for variant in &def.variants {
140+
for field in &variant.fields {
141+
match field.ty(cx.tcx, substs).sty {
142+
TypeVariants::TyArray(_, size) if size > 32 => {
143+
return;
91144
}
92-
});
145+
TypeVariants::TyBareFn(..) => {
146+
return;
147+
}
148+
TypeVariants::TyTuple(ref tys) if tys.len() > 12 => {
149+
return;
150+
}
151+
_ => (),
152+
}
93153
}
94154
}
95-
}}
96-
}}
155+
}
156+
_ => (),
157+
}
158+
159+
span_lint_and_then(
160+
cx, DERIVE_HASH_NOT_EQ, span,
161+
"you are implementing `Clone` explicitly on a `Copy` type",
162+
|db| {
163+
db.span_note(
164+
span,
165+
"consider deriving `Clone` or removing `Copy`"
166+
);
167+
});
168+
}
169+
}
170+
171+
/// Checks for the `#[automatically_derived]` attribute all `#[derive]`d implementations have.
172+
fn is_automatically_derived(attr: &Attribute) -> bool {
173+
if let MetaItem_::MetaWord(ref word) = attr.node.value.node {
174+
word == &"automatically_derived"
175+
}
176+
else {
177+
false
97178
}
98179
}

‎src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
173173
collapsible_if::COLLAPSIBLE_IF,
174174
cyclomatic_complexity::CYCLOMATIC_COMPLEXITY,
175175
derive::DERIVE_HASH_NOT_EQ,
176+
derive::EXPL_IMPL_CLONE_ON_COPY,
176177
entry::MAP_ENTRY,
177178
eq_op::EQ_OP,
178179
escape::BOXED_LOCAL,

‎src/utils.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@ pub type MethodArgs = HirVec<P<Expr>>;
2222
pub const BEGIN_UNWIND: [&'static str; 3] = ["std", "rt", "begin_unwind"];
2323
pub const BTREEMAP_ENTRY_PATH: [&'static str; 4] = ["collections", "btree", "map", "Entry"];
2424
pub const BTREEMAP_PATH: [&'static str; 4] = ["collections", "btree", "map", "BTreeMap"];
25-
pub const CLONE_PATH: [&'static str; 2] = ["Clone", "clone"];
25+
pub const CLONE_PATH: [&'static str; 3] = ["clone", "Clone", "clone"];
26+
pub const CLONE_TRAIT_PATH: [&'static str; 2] = ["clone", "Clone"];
2627
pub const COW_PATH: [&'static str; 3] = ["collections", "borrow", "Cow"];
2728
pub const DEFAULT_TRAIT_PATH: [&'static str; 3] = ["core", "default", "Default"];
2829
pub const HASHMAP_ENTRY_PATH: [&'static str; 5] = ["std", "collections", "hash", "map", "Entry"];

‎tests/compile-fail/derive.rs

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
#![plugin(clippy)]
33

44
#![deny(warnings)]
5+
#![allow(dead_code)]
56

67
#[derive(PartialEq, Hash)]
78
struct Foo;
@@ -11,19 +12,57 @@ impl PartialEq<u64> for Foo {
1112
}
1213

1314
#[derive(Hash)]
14-
//~^ ERROR you are deriving `Hash` but have implemented `PartialEq` explicitely
15+
//~^ ERROR you are deriving `Hash` but have implemented `PartialEq` explicitly
1516
struct Bar;
1617

1718
impl PartialEq for Bar {
1819
fn eq(&self, _: &Bar) -> bool { true }
1920
}
2021

2122
#[derive(Hash)]
22-
//~^ ERROR you are deriving `Hash` but have implemented `PartialEq` explicitely
23+
//~^ ERROR you are deriving `Hash` but have implemented `PartialEq` explicitly
2324
struct Baz;
2425

2526
impl PartialEq<Baz> for Baz {
2627
fn eq(&self, _: &Baz) -> bool { true }
2728
}
2829

30+
#[derive(Copy)]
31+
struct Qux;
32+
33+
impl Clone for Qux {
34+
//~^ ERROR you are implementing `Clone` explicitly on a `Copy` type
35+
fn clone(&self) -> Self { Qux }
36+
}
37+
38+
// Ok, `Clone` cannot be derived because of the big array
39+
#[derive(Copy)]
40+
struct BigArray {
41+
a: [u8; 65],
42+
}
43+
44+
impl Clone for BigArray {
45+
fn clone(&self) -> Self { unimplemented!() }
46+
}
47+
48+
// Ok, function pointers are not always Clone
49+
#[derive(Copy)]
50+
struct FnPtr {
51+
a: fn() -> !,
52+
}
53+
54+
impl Clone for FnPtr {
55+
fn clone(&self) -> Self { unimplemented!() }
56+
}
57+
58+
// Ok, generics
59+
#[derive(Copy)]
60+
struct Generic<T> {
61+
a: T,
62+
}
63+
64+
impl<T> Clone for Generic<T> {
65+
fn clone(&self) -> Self { unimplemented!() }
66+
}
67+
2968
fn main() {}

0 commit comments

Comments
 (0)