Merge pull request #676 from mcarton/hash_eq

Make DERIVE_HASH_NOT_EQ symmetric

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

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<Foo> 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(

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,

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<Baz> 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<H: Hasher>(&self, _: &mut H) {}
+}
+
 #[derive(Copy)]
 struct Qux;