diff --git a/CHANGELOG.md b/CHANGELOG.md index ce5980391..f50236251 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index df96a2fed..c51c17567 100644 --- a/README.md +++ b/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 diff --git a/clippy_lints/src/derive.rs b/clippy_lints/src/derive.rs index 89e90bff6..d95c72c58 100644 --- a/clippy_lints/src/derive.rs +++ b/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 - } -} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 79256b0a7..18e38a6db 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/clippy_lints/src/partialeq_ne_impl.rs b/clippy_lints/src/partialeq_ne_impl.rs new file mode 100644 index 000000000..fdee5c256 --- /dev/null +++ b/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") + } + } + }}; + } +} diff --git a/clippy_lints/src/utils/inspector.rs b/clippy_lints/src/utils/inspector.rs index 2af1d8dec..a4d9e8452 100644 --- a/clippy_lints/src/utils/inspector.rs +++ b/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"); diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index ba57512fc..edb0f1cf1 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/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") +} diff --git a/tests/compile-fail/partialeq_ne_impl.rs b/tests/compile-fail/partialeq_ne_impl.rs new file mode 100644 index 000000000..f4ecdd426 --- /dev/null +++ b/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() {} diff --git a/util/dogfood.sh b/util/dogfood.sh index 5ba8b4efa..358fc46c8 100755 --- a/util/dogfood.sh +++ b/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