Merge pull request #62 from Manishearth/len_zero_is_empty

Here goes nothing... 😄

Author: llogiq

README.md

@@ -23,6 +23,8 @@ Lints included in this crate:
  - `redundant_closure`: Warns on usage of eta-reducible closures like `|a| foo(a)` (which can be written as just `foo`)
  - `identity_op`: Warns on identity operations like `x + 0` or `y / 1` (which can be reduced to `x` and `y`, respectively)
  - `mut_mut`: Warns on `&mut &mut` which is either a copy'n'paste error, or shows a fundamental misunderstanding of references
+ - `len_zero`: Warns on `_.len() == 0` and suggests using `_.is_empty()` (or similar comparisons with `>` or `!=`)
+ - `len_without_is_empty`: Warns on traits or impls that have a `.len()` but no `.is_empty()` method
 
 To use, add the following lines to your Cargo.toml:
 

src/len_zero.rs

@@ -0,0 +1,101 @@
+extern crate rustc_typeck as typeck;
+
+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::middle::def::{DefTy, DefStruct, DefTrait};
+use syntax::codemap::{Span, Spanned};
+
+declare_lint!(pub LEN_ZERO, Warn,
+              "Warn on usage of double-mut refs, e.g. '&mut &mut ...'");
+
+declare_lint!(pub LEN_WITHOUT_IS_EMPTY, Warn,
+              "Warn on traits and impls that have .len() but not .is_empty()");
+
+#[derive(Copy,Clone)]
+pub struct LenZero;
+
+impl LintPass for LenZero {
+	fn get_lints(&self) -> LintArray {
+        lint_array!(LEN_ZERO, LEN_WITHOUT_IS_EMPTY)
+	}
+	
+	fn check_item(&mut self, cx: &Context, item: &Item) {
+		match &item.node {
+			&ItemTrait(_, _, _, ref trait_items) => 
+				check_trait_items(cx, item, trait_items),
+			&ItemImpl(_, _, _, None, _, ref impl_items) => // only non-trait
+				check_impl_items(cx, item, impl_items),
+			_ => ()
+		}
+	}
+	
+	fn check_expr(&mut self, cx: &Context, expr: &Expr) {
+		if let &ExprBinary(Spanned{node: cmp, ..}, ref left, ref right) = 
+				&expr.node {
+			match cmp {
+				BiEq => check_cmp(cx, expr.span, left, right, ""),
+				BiGt | BiNe => check_cmp(cx, expr.span, left, right, "!"),
+				_ => ()
+			}
+		}
+	}
+}
+
+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
+	}
+
+	if !trait_items.iter().any(|i| is_named_self(i, "is_empty")) {
+		//cx.span_lint(LEN_WITHOUT_IS_EMPTY, item.span, &format!("trait {}", item.ident.as_str()));
+		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.", 
+						item.ident.as_str()));
+			}
+		};
+	}
+}
+
+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
+	}
+
+	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.", 
+						item.ident.as_str()));
+				return;
+			}
+		}
+	}
+}
+
+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)) => 
+			check_len_zero(cx, span, method, args, lit, empty),
+		(&ExprMethodCall(ref method, _, ref args), &ExprLit(ref lit)) => 
+			check_len_zero(cx, span, method, args, lit, empty),
+		_ => ()
+	}
+}
+
+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 {
+			cx.span_lint(LEN_ZERO, span, &format!(
+				"Consider replacing the len comparison with '{}_.is_empty()' if available",
+					empty))
+		}
+	}
+}

src/lib.rs

@@ -24,6 +24,7 @@ pub mod approx_const;
 pub mod eta_reduction;
 pub mod identity_op;
 pub mod mut_mut;
+pub mod len_zero;
 
 #[plugin_registrar]
 pub fn plugin_registrar(reg: &mut Registry) {
@@ -42,6 +43,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_lint_pass(box eta_reduction::EtaPass as LintPassObject);
     reg.register_lint_pass(box identity_op::IdentityOp as LintPassObject);
     reg.register_lint_pass(box mut_mut::MutMut as LintPassObject);
+    reg.register_lint_pass(box len_zero::LenZero as LintPassObject);
 
     reg.register_lint_group("clippy", vec![types::BOX_VEC, types::LINKEDLIST,
                                            misc::SINGLE_MATCH, misc::STR_TO_STRING,
@@ -56,5 +58,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
                                            eta_reduction::REDUNDANT_CLOSURE,
                                            identity_op::IDENTITY_OP,
                                            mut_mut::MUT_MUT,
+                                           len_zero::LEN_ZERO,
+                                           len_zero::LEN_WITHOUT_IS_EMPTY,
                                            ]);
 }

tests/compile-fail/len_zero.rs

@@ -0,0 +1,56 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+struct One;
+
+#[deny(len_without_is_empty)]
+impl One {
+	fn len(self: &Self) -> isize { //~ERROR Item 'One' has a '.len()' method
+		1
+	}
+}
+
+#[deny(len_without_is_empty)]
+trait TraitsToo {
+	fn len(self: &Self) -> isize; //~ERROR Trait 'TraitsToo' has a '.len()' method,
+}
+
+impl TraitsToo for One {
+	fn len(self: &Self) -> isize {
+		0
+	}
+}
+
+#[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 {
+		false
+	}
+}
+
+#[deny(len_zero)]
+fn main() {
+	let x = [1, 2];
+	if x.len() == 0 { //~ERROR Consider replacing the len comparison
+		println!("This should not happen!");
+	}
+	
+	let y = One;
+	// false positives here
+	if y.len()  == 0 { //~ERROR Consider replacing the len comparison
+		println!("This should not happen either!");
+	}
+	
+	let z : &TraitsToo = &y;
+	if z.len() > 0 { //~ERROR Consider replacing the len comparison
+		println!("Nor should this!");
+	}
+}