diff --git a/src/attrs.rs b/src/attrs.rs new file mode 100644 index 00000000000..3ad3889f9db --- /dev/null +++ b/src/attrs.rs @@ -0,0 +1,48 @@ +/// checks for attributes + +use rustc::plugin::Registry; +use rustc::lint::*; +use syntax::ast::*; +use syntax::ptr::P; +use syntax::codemap::Span; +use syntax::parse::token::InternedString; + +declare_lint! { pub INLINE_ALWAYS, Warn, + "#[inline(always)] is usually a bad idea."} + + +#[derive(Copy,Clone)] +pub struct AttrPass; + +impl LintPass for AttrPass { + fn get_lints(&self) -> LintArray { + lint_array!(INLINE_ALWAYS) + } + + fn check_item(&mut self, cx: &Context, item: &Item) { + check_attrs(cx, &item.ident, &item.attrs) + } + + fn check_impl_item(&mut self, cx: &Context, item: &ImplItem) { + check_attrs(cx, &item.ident, &item.attrs) + } + + fn check_trait_item(&mut self, cx: &Context, item: &TraitItem) { + check_attrs(cx, &item.ident, &item.attrs) + } +} + +fn check_attrs(cx: &Context, ident: &Ident, attrs: &[Attribute]) { + for attr in attrs { + if let MetaList(ref inline, ref values) = attr.node.value.node { + if values.len() != 1 || inline != &"inline" { continue; } + if let MetaWord(ref always) = values[0].node { + if always != &"always" { continue; } + cx.span_lint(INLINE_ALWAYS, attr.span, &format!( + "You have declared #[inline(always)] on {}. This \ + is usually a bad idea. Are you sure?", + ident.as_str())); + } + } + } +} diff --git a/src/len_zero.rs b/src/len_zero.rs index 2ed0a6a9fab..5d97efdf02c 100644 --- a/src/len_zero.rs +++ b/src/len_zero.rs @@ -1,11 +1,16 @@ extern crate rustc_typeck as typeck; +use std::rc::Rc; +use std::cell::RefCell; use syntax::ptr::P; -use syntax::ast::*; use rustc::lint::{Context, LintPass, LintArray, Lint}; -use rustc::middle::ty::{self, node_id_to_type, sty, ty_ptr, ty_rptr, mt, MethodTraitItemId}; +use rustc::util::nodemap::DefIdMap; +use rustc::middle::ty::{self, node_id_to_type, sty, ty_ptr, ty_rptr, expr_ty, + mt, ty_to_def_id, impl_or_trait_item, MethodTraitItemId, ImplOrTraitItemId}; use rustc::middle::def::{DefTy, DefStruct, DefTrait}; use syntax::codemap::{Span, Spanned}; +use syntax::ast::*; +use misc::walk_ty; declare_lint!(pub LEN_ZERO, Warn, "Warn when .is_empty() could be used instead of checking .len()"); @@ -45,7 +50,8 @@ impl LintPass for LenZero { fn check_trait_items(cx: &Context, item: &Item, trait_items: &[P]) { fn is_named_self(item: &TraitItem, name: &str) -> bool { - item.ident.as_str() == name && item.attrs.len() == 0 + item.ident.as_str() == name && if let MethodTraitItem(ref sig, _) = + item.node { is_self_sig(sig) } else { false } } if !trait_items.iter().any(|i| is_named_self(i, "is_empty")) { @@ -53,8 +59,8 @@ fn check_trait_items(cx: &Context, item: &Item, trait_items: &[P]) { for i in trait_items { if is_named_self(i, "len") { cx.span_lint(LEN_WITHOUT_IS_EMPTY, i.span, - &format!("Trait '{}' has a '.len()' method, but no \ - '.is_empty()' method. Consider adding one.", + &format!("Trait '{}' has a '.len(_: &Self)' method, but no \ + '.is_empty(_: &Self)' method. Consider adding one.", item.ident.as_str())); } }; @@ -63,15 +69,18 @@ fn check_trait_items(cx: &Context, item: &Item, trait_items: &[P]) { fn check_impl_items(cx: &Context, item: &Item, impl_items: &[P]) { fn is_named_self(item: &ImplItem, name: &str) -> bool { - item.ident.as_str() == name && item.attrs.len() == 0 + item.ident.as_str() == name && if let MethodImplItem(ref sig, _) = + item.node { is_self_sig(sig) } else { false } } if !impl_items.iter().any(|i| is_named_self(i, "is_empty")) { for i in impl_items { if is_named_self(i, "len") { - cx.span_lint(LEN_WITHOUT_IS_EMPTY, i.span, - &format!("Item '{}' has a '.len()' method, but no \ - '.is_empty()' method. Consider adding one.", + let s = i.span; + cx.span_lint(LEN_WITHOUT_IS_EMPTY, + Span{ lo: s.lo, hi: s.lo, expn_id: s.expn_id }, + &format!("Item '{}' has a '.len(_: &Self)' method, but no \ + '.is_empty(_: &Self)' method. Consider adding one.", item.ident.as_str())); return; } @@ -79,6 +88,11 @@ fn check_impl_items(cx: &Context, item: &Item, impl_items: &[P]) { } } +fn is_self_sig(sig: &MethodSig) -> bool { + if let SelfStatic = sig.explicit_self.node { + false } else { sig.decl.inputs.len() == 1 } +} + fn check_cmp(cx: &Context, span: Span, left: &Expr, right: &Expr, empty: &str) { match (&left.node, &right.node) { (&ExprLit(ref lit), &ExprMethodCall(ref method, _, ref args)) => @@ -92,10 +106,46 @@ fn check_cmp(cx: &Context, span: Span, left: &Expr, right: &Expr, empty: &str) { fn check_len_zero(cx: &Context, span: Span, method: &SpannedIdent, args: &[P], lit: &Lit, empty: &str) { if let &Spanned{node: LitInt(0, _), ..} = lit { - if method.node.as_str() == "len" && args.len() == 1 { + if method.node.as_str() == "len" && args.len() == 1 && + has_is_empty(cx, &*args[0]) { cx.span_lint(LEN_ZERO, span, &format!( - "Consider replacing the len comparison with '{}_.is_empty()' if available", + "Consider replacing the len comparison with '{}_.is_empty()'", empty)) } } } + +/// check if this type has an is_empty method +fn has_is_empty(cx: &Context, expr: &Expr) -> bool { + /// get a ImplOrTraitItem and return true if it matches is_empty(self) + fn is_is_empty(cx: &Context, id: &ImplOrTraitItemId) -> bool { + if let &MethodTraitItemId(def_id) = id { + if let ty::MethodTraitItem(ref method) = + ty::impl_or_trait_item(cx.tcx, def_id) { + method.name.as_str() == "is_empty" + && method.fty.sig.skip_binder().inputs.len() == 1 + } else { false } + } else { false } + } + + /// check the inherent impl's items for an is_empty(self) method + fn has_is_empty_impl(cx: &Context, id: &DefId) -> bool { + let impl_items = cx.tcx.impl_items.borrow(); + cx.tcx.inherent_impls.borrow().get(id).map_or(false, + |ids| ids.iter().any(|iid| impl_items.get(iid).map_or(false, + |iids| iids.iter().any(|i| is_is_empty(cx, i))))) + } + + let ty = &walk_ty(&expr_ty(cx.tcx, expr)); + match ty.sty { + ty::ty_trait(_) => cx.tcx.trait_item_def_ids.borrow().get( + &ty::ty_to_def_id(ty).expect("trait impl not found")).map_or(false, + |ids| ids.iter().any(|i| is_is_empty(cx, i))), + ty::ty_projection(_) => ty::ty_to_def_id(ty).map_or(false, + |id| has_is_empty_impl(cx, &id)), + ty::ty_enum(ref id, _) | ty::ty_struct(ref id, _) => + has_is_empty_impl(cx, id), + ty::ty_vec(..) => true, + _ => false, + } +} diff --git a/src/lib.rs b/src/lib.rs index 27d097118fc..37cdf6a9c58 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -24,6 +24,7 @@ pub mod eta_reduction; pub mod identity_op; pub mod mut_mut; pub mod len_zero; +pub mod attrs; pub mod collapsible_if; #[plugin_registrar] @@ -45,6 +46,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_lint_pass(box mut_mut::MutMut as LintPassObject); reg.register_lint_pass(box len_zero::LenZero as LintPassObject); reg.register_lint_pass(box misc::CmpOwned as LintPassObject); + reg.register_lint_pass(box attrs::AttrPass as LintPassObject); reg.register_lint_pass(box collapsible_if::CollapsibleIf as LintPassObject); reg.register_lint_group("clippy", vec![types::BOX_VEC, types::LINKEDLIST, @@ -62,6 +64,7 @@ pub fn plugin_registrar(reg: &mut Registry) { mut_mut::MUT_MUT, len_zero::LEN_ZERO, len_zero::LEN_WITHOUT_IS_EMPTY, + attrs::INLINE_ALWAYS, collapsible_if::COLLAPSIBLE_IF, ]); } diff --git a/src/misc.rs b/src/misc.rs index b8ce6d725f4..5d0d79544e9 100644 --- a/src/misc.rs +++ b/src/misc.rs @@ -10,7 +10,7 @@ use syntax::codemap::{Span, Spanned}; use types::span_note_and_lint; -fn walk_ty<'t>(ty: ty::Ty<'t>) -> ty::Ty<'t> { +pub fn walk_ty<'t>(ty: ty::Ty<'t>) -> ty::Ty<'t> { match ty.sty { ty_ptr(ref tm) | ty_rptr(_, ref tm) => walk_ty(tm.ty), _ => ty diff --git a/tests/compile-fail/attrs.rs b/tests/compile-fail/attrs.rs new file mode 100644 index 00000000000..30ce191d3db --- /dev/null +++ b/tests/compile-fail/attrs.rs @@ -0,0 +1,12 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#[deny(inline_always)] +#[inline(always)] //~ERROR You have declared #[inline(always)] on test_attr_lint. +fn test_attr_lint() { + assert!(true) +} + +fn main() { + test_attr_lint() +} diff --git a/tests/compile-fail/len_zero.rs b/tests/compile-fail/len_zero.rs index 14f2506ec8b..e64010d334d 100644 --- a/tests/compile-fail/len_zero.rs +++ b/tests/compile-fail/len_zero.rs @@ -5,14 +5,14 @@ struct One; #[deny(len_without_is_empty)] impl One { - fn len(self: &Self) -> isize { //~ERROR Item 'One' has a '.len()' method + fn len(self: &Self) -> isize { //~ERROR Item 'One' has a '.len(_: &Self)' 1 } } #[deny(len_without_is_empty)] trait TraitsToo { - fn len(self: &Self) -> isize; //~ERROR Trait 'TraitsToo' has a '.len()' method, + fn len(self: &Self) -> isize; //~ERROR Trait 'TraitsToo' has a '.len(_: } impl TraitsToo for One { @@ -21,17 +21,47 @@ impl TraitsToo for One { } } -#[allow(dead_code)] struct HasIsEmpty; #[deny(len_without_is_empty)] -#[allow(dead_code)] impl HasIsEmpty { fn len(self: &Self) -> isize { 1 } + + fn is_empty(self: &Self) -> bool { + false + } +} + +struct Wither; + +#[deny(len_without_is_empty)] +trait WithIsEmpty { + fn len(self: &Self) -> isize; + fn is_empty(self: &Self) -> bool; +} + +impl WithIsEmpty for Wither { + fn len(self: &Self) -> isize { + 1 + } + + fn is_empty(self: &Self) -> bool { + false + } +} + +struct HasWrongIsEmpty; + +#[deny(len_without_is_empty)] +impl HasWrongIsEmpty { + fn len(self: &Self) -> isize { //~ERROR Item 'HasWrongIsEmpty' has a '.len(_: &Self)' + 1 + } - fn is_empty() -> bool { + #[allow(dead_code, unused)] + fn is_empty(self: &Self, x : u32) -> bool { false } } @@ -44,13 +74,29 @@ fn main() { } let y = One; - // false positives here - if y.len() == 0 { //~ERROR Consider replacing the len comparison + if y.len() == 0 { //no error because One does not have .is_empty() println!("This should not happen either!"); } let z : &TraitsToo = &y; - if z.len() > 0 { //~ERROR Consider replacing the len comparison + if z.len() > 0 { //no error, because TraitsToo has no .is_empty() method println!("Nor should this!"); } + + let hie = HasIsEmpty; + if hie.len() == 0 { //~ERROR Consider replacing the len comparison + println!("Or this!"); + } + assert!(!hie.is_empty()); + + let wie : &WithIsEmpty = &Wither; + if wie.len() == 0 { //~ERROR Consider replacing the len comparison + println!("Or this!"); + } + assert!(!wie.is_empty()); + + let hwie = HasWrongIsEmpty; + if hwie.len() == 0 { //no error as HasWrongIsEmpty does not have .is_empty() + println!("Or this!"); + } } diff --git a/tests/run-pass.rs b/tests/mut_mut_macro.rs similarity index 100% rename from tests/run-pass.rs rename to tests/mut_mut_macro.rs