From b5ba621f61a07193d3f7b0e7bd04204cc0dec4b4 Mon Sep 17 00:00:00 2001 From: mcarton Date: Mon, 15 Feb 2016 23:38:09 +0100 Subject: [PATCH] Make DERIVE_HASH_NOT_EQ symmetric --- README.md | 2 +- src/derive.rs | 43 +++++++++++++++++++++++++----------- src/lib.rs | 2 +- tests/compile-fail/derive.rs | 10 +++++++++ 4 files changed, 42 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 483182a68f6..05737f142fa 100644 --- a/README.md +++ b/README.md @@ -33,7 +33,7 @@ name [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() }` and an `else { if .. } expression can be collapsed to `else if` [cyclomatic_complexity](https://github.com/Manishearth/rust-clippy/wiki#cyclomatic_complexity) | warn | finds functions that should be split up into multiple functions [deprecated_semver](https://github.com/Manishearth/rust-clippy/wiki#deprecated_semver) | warn | `Warn` on `#[deprecated(since = "x")]` where x is not semver -[derive_hash_not_eq](https://github.com/Manishearth/rust-clippy/wiki#derive_hash_not_eq) | warn | deriving `Hash` but implementing `PartialEq` explicitly +[derive_hash_xor_eq](https://github.com/Manishearth/rust-clippy/wiki#derive_hash_xor_eq) | warn | deriving `Hash` but implementing `PartialEq` explicitly [drop_ref](https://github.com/Manishearth/rust-clippy/wiki#drop_ref) | warn | call to `std::mem::drop` with a reference instead of an owned value, which will not call the `Drop::drop` method on the underlying value [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 diff --git a/src/derive.rs b/src/derive.rs index e8816731219..084d00d409f 100644 --- a/src/derive.rs +++ b/src/derive.rs @@ -32,7 +32,7 @@ use utils::{match_path, span_lint_and_then}; /// } /// ``` declare_lint! { - pub DERIVE_HASH_NOT_EQ, + pub DERIVE_HASH_XOR_EQ, Warn, "deriving `Hash` but implementing `PartialEq` explicitly" } @@ -65,7 +65,7 @@ pub struct Derive; impl LintPass for Derive { fn get_lints(&self) -> LintArray { - lint_array!(EXPL_IMPL_CLONE_ON_COPY, DERIVE_HASH_NOT_EQ) + lint_array!(EXPL_IMPL_CLONE_ON_COPY, DERIVE_HASH_XOR_EQ) } } @@ -75,19 +75,25 @@ impl LateLintPass for Derive { let ItemImpl(_, _, _, Some(ref trait_ref), _, _) = item.node ], { let ty = cx.tcx.lookup_item_type(cx.tcx.map.local_def_id(item.id)).ty; - if item.attrs.iter().any(is_automatically_derived) { - check_hash_peq(cx, item.span, trait_ref, ty); - } - else { + let is_automatically_derived = item.attrs.iter().any(is_automatically_derived); + + check_hash_peq(cx, item.span, trait_ref, ty, is_automatically_derived); + + if !is_automatically_derived { check_copy_clone(cx, item, 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 +/// Implementation of the `DERIVE_HASH_XOR_EQ` lint. +fn check_hash_peq( + cx: &LateContext, + span: Span, + trait_ref: &TraitRef, + ty: ty::Ty, + hash_is_automatically_derived: bool +) { if_let_chain! {[ match_path(&trait_ref.path, &HASH_PATH), let Some(peq_trait_def_id) = cx.tcx.lang_items.eq_trait() @@ -103,14 +109,25 @@ fn check_hash_peq(cx: &LateContext, span: Span, trait_ref: &TraitRef, ty: ty::Ty let Some(impl_ids) = peq_impls.get(&simpl_ty) ], { for &impl_id in impl_ids { + let peq_is_automatically_derived = cx.tcx.get_attrs(impl_id).iter().any(is_automatically_derived); + + if peq_is_automatically_derived == hash_is_automatically_derived { + return; + } + 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) { + if trait_ref.input_types()[0] == ty { + let mess = if peq_is_automatically_derived { + "you are implementing `Hash` explicitly but have derived `PartialEq`" + } else { + "you are deriving `Hash` but have implemented `PartialEq` explicitly" + }; + span_lint_and_then( - cx, DERIVE_HASH_NOT_EQ, span, - "you are deriving `Hash` but have implemented `PartialEq` explicitly", + cx, DERIVE_HASH_XOR_EQ, span, + mess, |db| { if let Some(node_id) = cx.tcx.map.as_local_node_id(impl_id) { db.span_note( diff --git a/src/lib.rs b/src/lib.rs index 8d29d6ab68a..bd13190f859 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -198,7 +198,7 @@ pub fn plugin_registrar(reg: &mut Registry) { copies::IFS_SAME_COND, copies::MATCH_SAME_ARMS, cyclomatic_complexity::CYCLOMATIC_COMPLEXITY, - derive::DERIVE_HASH_NOT_EQ, + derive::DERIVE_HASH_XOR_EQ, derive::EXPL_IMPL_CLONE_ON_COPY, drop_ref::DROP_REF, entry::MAP_ENTRY, diff --git a/tests/compile-fail/derive.rs b/tests/compile-fail/derive.rs index 14b1106add5..06f1388dc05 100755 --- a/tests/compile-fail/derive.rs +++ b/tests/compile-fail/derive.rs @@ -4,6 +4,8 @@ #![deny(warnings)] #![allow(dead_code)] +use std::hash::{Hash, Hasher}; + #[derive(PartialEq, Hash)] struct Foo; @@ -27,6 +29,14 @@ impl PartialEq for Baz { fn eq(&self, _: &Baz) -> bool { true } } +#[derive(PartialEq)] +struct Bah; + +impl Hash for Bah { +//~^ ERROR you are implementing `Hash` explicitly but have derived `PartialEq` + fn hash(&self, _: &mut H) {} +} + #[derive(Copy)] struct Qux;