Merge pull request #1307 from Kha/partialeq_ne

Implement 'Re-implementing `PartialEq::ne`' lint

Author: oli-obk

CHANGELOG.md

@@ -325,6 +325,7 @@ All notable changes to this project will be documented in this file.
 [`out_of_bounds_indexing`]: https://github.com/Manishearth/rust-clippy/wiki#out_of_bounds_indexing
 [`overflow_check_conditional`]: https://github.com/Manishearth/rust-clippy/wiki#overflow_check_conditional
 [`panic_params`]: https://github.com/Manishearth/rust-clippy/wiki#panic_params
+[`partialeq_ne_impl`]: https://github.com/Manishearth/rust-clippy/wiki#partialeq_ne_impl
 [`precedence`]: https://github.com/Manishearth/rust-clippy/wiki#precedence
 [`print_stdout`]: https://github.com/Manishearth/rust-clippy/wiki#print_stdout
 [`print_with_newline`]: https://github.com/Manishearth/rust-clippy/wiki#print_with_newline

README.md

@@ -182,7 +182,7 @@ You can check out this great service at [clippy.bashy.io](https://clippy.bashy.i
 
 ## Lints
 
-There are 175 lints included in this crate:
+There are 176 lints included in this crate:
 
 name                                                                                                                   | default | triggers on
 -----------------------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------
@@ -301,6 +301,7 @@ name
 [out_of_bounds_indexing](https://github.com/Manishearth/rust-clippy/wiki#out_of_bounds_indexing)                       | deny    | out of bounds constant indexing
 [overflow_check_conditional](https://github.com/Manishearth/rust-clippy/wiki#overflow_check_conditional)               | warn    | overflow checks inspired by C which are likely to panic
 [panic_params](https://github.com/Manishearth/rust-clippy/wiki#panic_params)                                           | warn    | missing parameters in `panic!` calls
+[partialeq_ne_impl](https://github.com/Manishearth/rust-clippy/wiki#partialeq_ne_impl)                                 | warn    | re-implementing `PartialEq::ne`
 [precedence](https://github.com/Manishearth/rust-clippy/wiki#precedence)                                               | warn    | operations where precedence may be unclear
 [print_stdout](https://github.com/Manishearth/rust-clippy/wiki#print_stdout)                                           | allow   | printing on stdout
 [print_with_newline](https://github.com/Manishearth/rust-clippy/wiki#print_with_newline)                               | warn    | using `print!()` with a format string that ends in a newline

clippy_lints/src/derive.rs

@@ -3,10 +3,9 @@ use rustc::ty::subst::Subst;
 use rustc::ty::TypeVariants;
 use rustc::ty;
 use rustc::hir::*;
-use syntax::ast::{Attribute, MetaItemKind};
 use syntax::codemap::Span;
 use utils::paths;
-use utils::{match_path, span_lint_and_then};
+use utils::{is_automatically_derived, match_path, span_lint_and_then};
 
 /// **What it does:** Checks for deriving `Hash` but implementing `PartialEq`
 /// explicitly.
@@ -75,7 +74,7 @@ impl LateLintPass for Derive {
     fn check_item(&mut self, cx: &LateContext, item: &Item) {
         if let ItemImpl(_, _, _, Some(ref trait_ref), _, _) = item.node {
             let ty = cx.tcx.lookup_item_type(cx.tcx.map.local_def_id(item.id)).ty;
-            let is_automatically_derived = item.attrs.iter().any(is_automatically_derived);
+            let is_automatically_derived = is_automatically_derived(&*item.attrs);
 
             check_hash_peq(cx, item.span, trait_ref, ty, is_automatically_derived);
 
@@ -97,7 +96,7 @@ fn check_hash_peq<'a, 'tcx: 'a>(cx: &LateContext<'a, 'tcx>, span: Span, trait_re
 
         // Look for the PartialEq implementations for `ty`
         peq_trait_def.for_each_relevant_impl(cx.tcx, ty, |impl_id| {
-            let peq_is_automatically_derived = cx.tcx.get_attrs(impl_id).iter().any(is_automatically_derived);
+            let peq_is_automatically_derived = is_automatically_derived(&cx.tcx.get_attrs(impl_id));
 
             if peq_is_automatically_derived == hash_is_automatically_derived {
                 return;
@@ -174,12 +173,3 @@ fn check_copy_clone<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, item: &Item, trait_ref
                            });
     }
 }
-
-/// Checks for the `#[automatically_derived]` attribute all `#[derive]`d implementations have.
-fn is_automatically_derived(attr: &Attribute) -> bool {
-    if let MetaItemKind::Word(ref word) = attr.node.value.node {
-        word == &"automatically_derived"
-    } else {
-        false
-    }
-}

clippy_lints/src/lib.rs

@@ -111,6 +111,7 @@ pub mod ok_if_let;
 pub mod open_options;
 pub mod overflow_check_conditional;
 pub mod panic;
+pub mod partialeq_ne_impl;
 pub mod precedence;
 pub mod print;
 pub mod ptr;
@@ -265,6 +266,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_late_lint_pass(box missing_doc::MissingDoc::new());
     reg.register_late_lint_pass(box ok_if_let::Pass);
     reg.register_late_lint_pass(box if_let_redundant_pattern_matching::Pass);
+    reg.register_late_lint_pass(box partialeq_ne_impl::Pass);
 
     reg.register_lint_group("clippy_restrictions", vec![
         arithmetic::FLOAT_ARITHMETIC,
@@ -421,6 +423,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         open_options::NONSENSICAL_OPEN_OPTIONS,
         overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL,
         panic::PANIC_PARAMS,
+        partialeq_ne_impl::PARTIALEQ_NE_IMPL,
         precedence::PRECEDENCE,
         print::PRINT_WITH_NEWLINE,
         ptr::CMP_NULL,

clippy_lints/src/partialeq_ne_impl.rs

@@ -0,0 +1,55 @@
+use rustc::lint::*;
+use rustc::hir::*;
+use utils::{is_automatically_derived, span_lint};
+
+/// **What it does:** Checks for manual re-implementations of `PartialEq::ne`.
+///
+/// **Why is this bad?** `PartialEq::ne` is required to always return the
+/// negated result of `PartialEq::eq`, which is exactly what the default
+/// implementation does. Therefore, there should never be any need to
+/// re-implement it.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// struct Foo;
+///
+/// impl PartialEq for Foo {
+///    fn eq(&self, other: &Foo) -> bool { ... }
+///    fn ne(&self, other: &Foo) -> bool { !(self == other) }
+/// }
+/// ```
+declare_lint! {
+    pub PARTIALEQ_NE_IMPL,
+    Warn,
+    "re-implementing `PartialEq::ne`"
+}
+
+#[derive(Clone, Copy)]
+pub struct Pass;
+
+impl LintPass for Pass {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(PARTIALEQ_NE_IMPL)
+    }
+}
+
+impl LateLintPass for Pass {
+    fn check_item(&mut self, cx: &LateContext, item: &Item) {
+        if_let_chain! {[
+            let ItemImpl(_, _, _, Some(ref trait_ref), _, ref impl_items) = item.node,
+            !is_automatically_derived(&*item.attrs),
+            cx.tcx.expect_def(trait_ref.ref_id).def_id() == cx.tcx.lang_items.eq_trait().unwrap(),
+        ], {
+            for impl_item in impl_items {
+                if &*impl_item.name.as_str() == "ne" {
+                    span_lint(cx,
+                              PARTIALEQ_NE_IMPL,
+                              impl_item.span,
+                              "re-implementing `PartialEq::ne` is unnecessary")
+                }
+            }
+        }};
+    }
+}

clippy_lints/src/utils/inspector.rs

@@ -4,7 +4,8 @@
 
 use rustc::lint::*;
 use rustc::hir;
-use syntax::ast::{Attribute, MetaItemKind};
+use syntax::ast::Attribute;
+use syntax::attr;
 
 /// **What it does:** Dumps every ast/hir node which has the `#[clippy_dump]` attribute
 ///
@@ -128,10 +129,7 @@ impl LateLintPass for Pass {
 }
 
 fn has_attr(attrs: &[Attribute]) -> bool {
-    attrs.iter().any(|attr| match attr.node.value.node {
-        MetaItemKind::Word(ref word) => word == "clippy_dump",
-        _ => false,
-    })
+    attr::contains_name(attrs, "clippy_dump")
 }
 
 fn print_decl(cx: &LateContext, decl: &hir::Decl) {
@@ -381,12 +379,12 @@ fn print_item(cx: &LateContext, item: &hir::Item) {
             }
         },
         hir::ItemDefaultImpl(_, ref trait_ref) => {
-            let trait_did = cx.tcx.map.local_def_id(trait_ref.ref_id);
-            println!("default impl for `{:?}`", cx.tcx.item_path_str(trait_did));
+            let trait_did = cx.tcx.expect_def(trait_ref.ref_id).def_id();
+            println!("default impl for `{}`", cx.tcx.item_path_str(trait_did));
         },
         hir::ItemImpl(_, _, _, Some(ref trait_ref), _, _) => {
-            let trait_did = cx.tcx.map.local_def_id(trait_ref.ref_id);
-            println!("impl of trait `{:?}`", cx.tcx.item_path_str(trait_did));
+            let trait_did = cx.tcx.expect_def(trait_ref.ref_id).def_id();
+            println!("impl of trait `{}`", cx.tcx.item_path_str(trait_did));
         },
         hir::ItemImpl(_, _, _, None, _, _) => {
             println!("impl");

clippy_lints/src/utils/mod.rs

@@ -14,6 +14,7 @@ use std::env;
 use std::mem;
 use std::str::FromStr;
 use syntax::ast::{self, LitKind};
+use syntax::attr;
 use syntax::codemap::{ExpnFormat, ExpnInfo, MultiSpan, Span, DUMMY_SP};
 use syntax::errors::DiagnosticBuilder;
 use syntax::ptr::P;
@@ -761,3 +762,8 @@ pub fn is_refutable(cx: &LateContext, pat: &Pat) -> bool {
         }
     }
 }
+
+/// Checks for the `#[automatically_derived]` attribute all `#[derive]`d implementations have.
+pub fn is_automatically_derived(attrs: &[ast::Attribute]) -> bool {
+    attr::contains_name(attrs, "automatically_derived")
+}

tests/compile-fail/partialeq_ne_impl.rs

@@ -0,0 +1,15 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#![deny(warnings)]
+#![allow(dead_code)]
+
+struct Foo;
+
+impl PartialEq for Foo {
+    fn eq(&self, _: &Foo) -> bool { true }
+    fn ne(&self, _: &Foo) -> bool { false }
+    //~^ ERROR re-implementing `PartialEq::ne` is unnecessary
+}
+
+fn main() {}

util/dogfood.sh

@@ -1,5 +1,5 @@
 #!/bin/sh
 rm -rf target*/*so
-cargo build --lib && cp -R target target_recur && cargo rustc -- -Zextra-plugins=clippy -Ltarget_recur/debug -Dclippy_pedantic -Dclippy || exit 1
+cargo build --lib && cp -R target target_recur && cargo rustc --lib -- -Zextra-plugins=clippy -Ltarget_recur/debug -Dclippy_pedantic -Dclippy || exit 1
 rm -rf target_recur