Merge pull request #571 from mcarton/hash_eq

Add a lint about deriving Hash and implementing PartialEq

Author: Manishearth

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 95 lints included in this crate:
+There are 96 lints included in this crate:
 
 name                                                                                                           | default | meaning
 ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -26,6 +26,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() }`
 [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
 [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`)

src/derive.rs

@@ -0,0 +1,98 @@
+use rustc::lint::*;
+use rustc_front::hir::*;
+use syntax::ast::{Attribute, MetaItem_};
+use utils::{match_path, span_lint_and_then};
+use utils::HASH_PATH;
+
+use rustc::middle::ty::fast_reject::simplify_type;
+
+/// **What it does:** This lint warns about deriving `Hash` but implementing `PartialEq`
+/// explicitely.
+///
+/// **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
+/// an explicitely defined `PartialEq`. In particular, the following must hold for any type:
+///
+/// ```rust
+/// k1 == k2 -> hash(k1) == hash(k2)
+/// ```
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// #[derive(Hash)]
+/// struct Foo;
+///
+/// impl PartialEq for Foo {
+///     ..
+/// }
+declare_lint! {
+    pub DERIVE_HASH_NOT_EQ,
+    Warn,
+    "deriving `Hash` but implementing `PartialEq` explicitly"
+}
+
+pub struct Derive;
+
+impl LintPass for Derive {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(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
+            }
+        }
+
+        // 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 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<Foo> 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"
+                                );
+                            }
+                        });
+                    }
+                }
+            }}
+        }}
+    }
+}

src/lib.rs

@@ -75,6 +75,7 @@ pub mod entry;
 pub mod misc_early;
 pub mod array_indexing;
 pub mod panic;
+pub mod derive;
 
 mod reexport {
     pub use syntax::ast::{Name, NodeId};
@@ -136,6 +137,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_late_lint_pass(box array_indexing::ArrayIndexing);
     reg.register_late_lint_pass(box panic::PanicPass);
     reg.register_late_lint_pass(box strings::StringLitAsBytes);
+    reg.register_late_lint_pass(box derive::Derive);
 
 
     reg.register_lint_group("clippy_pedantic", vec![
@@ -168,6 +170,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         block_in_if_condition::BLOCK_IN_IF_CONDITION_STMT,
         collapsible_if::COLLAPSIBLE_IF,
         cyclomatic_complexity::CYCLOMATIC_COMPLEXITY,
+        derive::DERIVE_HASH_NOT_EQ,
         entry::MAP_ENTRY,
         eq_op::EQ_OP,
         escape::BOXED_LOCAL,

src/utils.rs

@@ -27,6 +27,7 @@ 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"];
 pub const HASHMAP_PATH: [&'static str; 5] = ["std", "collections", "hash", "map", "HashMap"];
+pub const HASH_PATH: [&'static str; 2] = ["hash", "Hash"];
 pub const LL_PATH: [&'static str; 3] = ["collections", "linked_list", "LinkedList"];
 pub const MUTEX_PATH: [&'static str; 4] = ["std", "sync", "mutex", "Mutex"];
 pub const OPEN_OPTIONS_PATH: [&'static str; 3] = ["std", "fs", "OpenOptions"];

tests/compile-fail/derive.rs

@@ -0,0 +1,29 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#![deny(warnings)]
+
+#[derive(PartialEq, Hash)]
+struct Foo;
+
+impl PartialEq<u64> for Foo {
+    fn eq(&self, _: &u64) -> bool { true }
+}
+
+#[derive(Hash)]
+//~^ ERROR you are deriving `Hash` but have implemented `PartialEq` explicitely
+struct Bar;
+
+impl PartialEq for Bar {
+    fn eq(&self, _: &Bar) -> bool { true }
+}
+
+#[derive(Hash)]
+//~^ ERROR you are deriving `Hash` but have implemented `PartialEq` explicitely
+struct Baz;
+
+impl PartialEq<Baz> for Baz {
+    fn eq(&self, _: &Baz) -> bool { true }
+}
+
+fn main() {}