Merge pull request #81 from Manishearth/len_zero

Len zero extended with check for is_empty() (so we should no longer get false positives).

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()));
+			}
+		}
+	}
+}

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<TraitItem>]) {
 	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<TraitItem>]) {
 		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<TraitItem>]) {
 
 fn check_impl_items(cx: &Context, item: &Item, impl_items: &[P<ImplItem>]) {
 	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<ImplItem>]) {
 	}
 }
 
+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<Expr>], 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,
+	}
+}

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,
                                            ]);
 }

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

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()
+}

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() -> bool {
+	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
+	}
+	
+	#[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!");
+	}
 }