Skip to content

Commit 028e684

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

File tree

5 files changed

+168
-47
lines changed

5 files changed

+168
-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
---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -26,6 +26,7 @@ name
2626
[collapsible_if](https://github.com/Manishearth/rust-clippy/wiki#collapsible_if) | warn | two nested `if`-expressions can be collapsed into one, e.g. `if x { if y { foo() } }` can be written as `if x && y { foo() }`
2727
[cyclomatic_complexity](https://github.com/Manishearth/rust-clippy/wiki#cyclomatic_complexity) | warn | finds functions that should be split up into multiple functions
2828
[deprecated_semver](https://github.com/Manishearth/rust-clippy/wiki#deprecated_semver) | warn | `Warn` on `#[deprecated(since = "x")]` where x is not semver
29+
[derive_copy_not_clone](https://github.com/Manishearth/rust-clippy/wiki#derive_copy_not_clone) | warn | deriving `Copy` but implementing `Clone` explicitly
2930
[derive_hash_not_eq](https://github.com/Manishearth/rust-clippy/wiki#derive_hash_not_eq) | warn | deriving `Hash` but implementing `PartialEq` explicitly
3031
[duplicate_underscore_argument](https://github.com/Manishearth/rust-clippy/wiki#duplicate_underscore_argument) | warn | Function arguments having names which only differ by an underscore
3132
[empty_loop](https://github.com/Manishearth/rust-clippy/wiki#empty_loop) | warn | empty `loop {}` detected

‎src/derive.rs

Lines changed: 122 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,143 @@ 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 must agree and the behaviour
41+
/// of `Copy` cannot be override.
42+
///
43+
/// **Known problems:** None.
44+
///
45+
/// **Example:**
46+
/// ```rust
47+
/// #[derive(Copy)]
48+
/// struct Foo;
49+
///
50+
/// impl Clone for Foo {
51+
/// ..
52+
/// }
53+
declare_lint! {
54+
pub DERIVE_COPY_NOT_CLONE,
55+
Warn,
56+
"deriving `Copy` but implementing `Clone` explicitly"
57+
}
58+
3659
pub struct Derive;
3760

3861
impl LintPass for Derive {
3962
fn get_lints(&self) -> LintArray {
40-
lint_array!(DERIVE_HASH_NOT_EQ)
63+
lint_array!(DERIVE_COPY_NOT_CLONE, DERIVE_HASH_NOT_EQ)
4164
}
4265
}
4366

4467
impl LateLintPass for Derive {
4568
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"
69+
let ast_ty_to_ty_cache = cx.tcx.ast_ty_to_ty_cache.borrow();
70+
71+
if_let_chain! {[
72+
let ItemImpl(_, _, _, Some(ref trait_ref), ref ast_ty, _) = item.node,
73+
let Some(&ty) = ast_ty_to_ty_cache.get(&ast_ty.id)
74+
], {
75+
if item.attrs.iter().any(is_automatically_derived) {
76+
check_hash_peq(cx, item.span, trait_ref, ty);
5077
}
5178
else {
52-
false
79+
check_copy_clone(cx, item.span, trait_ref, ty);
5380
}
54-
}
81+
}}
82+
}
83+
}
5584

56-
// If `item` is an automatically derived `Hash` implementation
85+
/// Implementation of the `DERIVE_HASH_NOT_EQ` lint.
86+
fn check_hash_peq(cx: &LateContext, span: Span, trait_ref: &TraitRef, ty: ty::Ty) {
87+
// If `item` is an automatically derived `Hash` implementation
88+
if_let_chain! {[
89+
match_path(&trait_ref.path, &HASH_PATH),
90+
let Some(peq_trait_def_id) = cx.tcx.lang_items.eq_trait()
91+
], {
92+
let peq_trait_def = cx.tcx.lookup_trait_def(peq_trait_def_id);
93+
94+
cx.tcx.populate_implementations_for_trait_if_necessary(peq_trait_def.trait_ref.def_id);
95+
let peq_impls = peq_trait_def.borrow_impl_lists(cx.tcx).1;
96+
97+
// Look for the PartialEq implementations for `ty`
5798
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()
99+
let Some(simpl_ty) = simplify_type(cx.tcx, ty, false),
100+
let Some(impl_ids) = peq_impls.get(&simpl_ty)
62101
], {
63-
let peq_trait_def = cx.tcx.lookup_trait_def(peq_trait_def_id);
102+
for &impl_id in impl_ids {
103+
let trait_ref = cx.tcx.impl_trait_ref(impl_id).expect("must be a trait implementation");
64104

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();
105+
// Only care about `impl PartialEq<Foo> for Foo`
106+
if trait_ref.input_types()[0] == ty &&
107+
!cx.tcx.get_attrs(impl_id).iter().any(is_automatically_derived) {
108+
span_lint_and_then(
109+
cx, DERIVE_HASH_NOT_EQ, span,
110+
"you are deriving `Hash` but have implemented `PartialEq` explicitly",
111+
|db| {
112+
if let Some(node_id) = cx.tcx.map.as_local_node_id(impl_id) {
113+
db.span_note(
114+
cx.tcx.map.span(node_id),
115+
"`PartialEq` implemented here"
116+
);
117+
}
118+
});
119+
}
120+
}
121+
}}
122+
}}
123+
}
68124

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

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");
130+
if ty.moves_by_default(&parameter_environment, span) {
131+
return; // ty is not Copy
132+
}
78133

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-
);
134+
// Some types are not Clone by default but could be cloned `by hand` if necessary
135+
match ty.sty {
136+
TypeVariants::TyEnum(def, substs) | TypeVariants::TyStruct(def, substs) => {
137+
for variant in &def.variants {
138+
for field in &variant.fields {
139+
match field.ty(cx.tcx, substs).sty {
140+
TypeVariants::TyArray(_, size) if size > 32 => {
141+
return;
91142
}
92-
});
143+
TypeVariants::TyBareFn(..) => {
144+
return;
145+
}
146+
TypeVariants::TyTuple(ref tys) if tys.len() > 12 => {
147+
return;
148+
}
149+
_ => (),
150+
}
93151
}
94152
}
95-
}}
96-
}}
153+
}
154+
_ => (),
155+
}
156+
157+
span_lint_and_then(
158+
cx, DERIVE_HASH_NOT_EQ, span,
159+
"you are implementing `Clone` explicitly on a `Copy` type",
160+
|db| {
161+
db.span_note(
162+
span,
163+
"consider deriving `Clone` or removing `Copy`"
164+
);
165+
});
166+
}
167+
}
168+
169+
/// Checks for the `#[automatically_derived]` attribute all `#[derive]`d implementations have.
170+
fn is_automatically_derived(attr: &Attribute) -> bool {
171+
if let MetaItem_::MetaWord(ref word) = attr.node.value.node {
172+
word == &"automatically_derived"
173+
}
174+
else {
175+
false
97176
}
98177
}

‎src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
172172
block_in_if_condition::BLOCK_IN_IF_CONDITION_STMT,
173173
collapsible_if::COLLAPSIBLE_IF,
174174
cyclomatic_complexity::CYCLOMATIC_COMPLEXITY,
175+
derive::DERIVE_COPY_NOT_CLONE,
175176
derive::DERIVE_HASH_NOT_EQ,
176177
entry::MAP_ENTRY,
177178
eq_op::EQ_OP,

‎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)