Merge pull request #82 from Manishearth/collapsible_if

Fixed block check, also added macro test to collapsible_if and …

src/attrs.rs

@@ -4,8 +4,9 @@ use rustc::plugin::Registry;
 use rustc::lint::*;
 use syntax::ast::*;
 use syntax::ptr::P;
-use syntax::codemap::Span;
+use syntax::codemap::{Span, ExpnInfo};
 use syntax::parse::token::InternedString;
+use utils::in_macro;
 
 declare_lint! { pub INLINE_ALWAYS, Warn,
     "#[inline(always)] is usually a bad idea."}
@@ -20,19 +21,25 @@ impl LintPass for AttrPass {
     }
 
     fn check_item(&mut self, cx: &Context, item: &Item) {
-		check_attrs(cx, &item.ident, &item.attrs)
+		cx.sess().codemap().with_expn_info(item.span.expn_id, 
+			|info| check_attrs(cx, info, &item.ident, &item.attrs))
 	}
 
     fn check_impl_item(&mut self, cx: &Context, item: &ImplItem) { 
-		check_attrs(cx, &item.ident, &item.attrs)
+		cx.sess().codemap().with_expn_info(item.span.expn_id, 
+			|info| check_attrs(cx, info, &item.ident, &item.attrs))
 	}
 
 	fn check_trait_item(&mut self, cx: &Context, item: &TraitItem) {
-		check_attrs(cx, &item.ident, &item.attrs)
+		cx.sess().codemap().with_expn_info(item.span.expn_id, 
+			|info| check_attrs(cx, info, &item.ident, &item.attrs))
 	}
 }
 
-fn check_attrs(cx: &Context, ident: &Ident, attrs: &[Attribute]) {
+fn check_attrs(cx: &Context, info: Option<&ExpnInfo>, ident: &Ident, 
+		attrs: &[Attribute]) {
+	if in_macro(cx, info) { return; }
+
 	for attr in attrs {
 		if let MetaList(ref inline, ref values) = attr.node.value.node {
 			if values.len() != 1 || inline != &"inline" { continue; }

src/collapsible_if.rs

@@ -17,8 +17,9 @@ use rustc::lint::*;
 use rustc::middle::def::*;
 use syntax::ast::*;
 use syntax::ptr::P;
-use syntax::codemap::{Span, Spanned};
+use syntax::codemap::{Span, Spanned, ExpnInfo};
 use syntax::print::pprust::expr_to_string;
+use utils::in_macro;
 
 declare_lint! {
     pub COLLAPSIBLE_IF,
@@ -34,20 +35,23 @@ impl LintPass for CollapsibleIf {
         lint_array!(COLLAPSIBLE_IF)
     }
 
-    fn check_expr(&mut self, cx: &Context, e: &Expr) {
-        if let ExprIf(ref check, ref then_block, None) = e.node {
-            let expr = check_block(then_block);
-            let expr = match expr {
-                Some(e) => e,
-                None => return
-            };
-            if let ExprIf(ref check_inner, _, None) = expr.node {
-                let (check, check_inner) = (check_to_string(check), check_to_string(check_inner));
-                cx.span_lint(COLLAPSIBLE_IF, e.span,
-                             &format!("This if statement can be collapsed. Try: if {} && {}", check, check_inner));
-            }
-		    }
-    }
+	fn check_expr(&mut self, cx: &Context, expr: &Expr) {
+		cx.sess().codemap().with_expn_info(expr.span.expn_id, 
+			|info| check_expr_expd(cx, expr, info))
+	}
+}
+
+fn check_expr_expd(cx: &Context, e: &Expr, info: Option<&ExpnInfo>) {
+	if in_macro(cx, info) { return; }
+
+	if let ExprIf(ref check, ref then, None) = e.node {
+		if let Some(&Expr{ node: ExprIf(ref check_inner, _, None), ..}) = 
+				single_stmt_of_block(then) {
+			cx.span_lint(COLLAPSIBLE_IF, e.span, &format!(
+				"This if statement can be collapsed. Try: if {} && {}\n{:?}", 
+				check_to_string(check), check_to_string(check_inner), e));
+		}
+	}
 }
 
 fn requires_brackets(e: &Expr) -> bool {
@@ -65,16 +69,20 @@ fn check_to_string(e: &Expr) -> String {
     }
 }
 
-fn check_block(b: &Block) -> Option<&P<Expr>> {
-    if b.stmts.len() == 1 && b.expr.is_none() {
-        let stmt = &b.stmts[0];
-        return match stmt.node {
-            StmtExpr(ref e, _) => Some(e),
-            _ => None
-        };
-    }
-    if let Some(ref e) = b.expr {
-        return Some(e);
+fn single_stmt_of_block(block: &Block) -> Option<&Expr> {
+    if block.stmts.len() == 1 && block.expr.is_none() {
+        if let StmtExpr(ref expr, _) = block.stmts[0].node {
+            single_stmt_of_expr(expr)
+        } else { None }
+    } else {
+        if block.stmts.is_empty() {
+            if let Some(ref p) = block.expr { Some(&*p) } else { None }
+        } else { None }
     }
-    None
+}
+
+fn single_stmt_of_expr(expr: &Expr) -> Option<&Expr> {
+    if let ExprBlock(ref block) = expr.node {
+        single_stmt_of_block(block)
+    } else { Some(expr) }
 }

src/lib.rs

@@ -26,6 +26,7 @@ pub mod mut_mut;
 pub mod len_zero;
 pub mod attrs;
 pub mod collapsible_if;
+pub mod utils;
 
 #[plugin_registrar]
 pub fn plugin_registrar(reg: &mut Registry) {

src/mut_mut.rs

@@ -3,6 +3,7 @@ use syntax::ast::*;
 use rustc::lint::{Context, LintPass, LintArray, Lint};
 use rustc::middle::ty::{expr_ty, sty, ty_ptr, ty_rptr, mt};
 use syntax::codemap::{BytePos, ExpnInfo, MacroFormat, Span};
+use utils::in_macro;
 
 declare_lint!(pub MUT_MUT, Warn,
               "Warn on usage of double-mut refs, e.g. '&mut &mut ...'");
@@ -51,16 +52,6 @@ fn check_expr_expd(cx: &Context, expr: &Expr, info: Option<&ExpnInfo>) {
 	})
 }
 
-fn in_macro(cx: &Context, opt_info: Option<&ExpnInfo>) -> bool {
-	opt_info.map_or(false, |info| {
-		info.callee.span.map_or(true, |span| {
-			cx.sess().codemap().span_to_snippet(span).ok().map_or(true, |code| 
-				!code.starts_with("macro_rules")
-			)
-		})
-	})
-}
-
 fn unwrap_mut(ty : &Ty) -> Option<&Ty> {
 	match ty.node {
 		TyPtr(MutTy{ ty: ref pty, mutbl: MutMutable }) => Option::Some(pty),

src/utils.rs

@@ -0,0 +1,12 @@
+use rustc::lint::Context;
+use syntax::codemap::ExpnInfo;
+
+pub fn in_macro(cx: &Context, opt_info: Option<&ExpnInfo>) -> bool {
+	opt_info.map_or(false, |info| {
+		info.callee.span.map_or(true, |span| {
+			cx.sess().codemap().span_to_snippet(span).ok().map_or(true, |code| 
+				!code.starts_with("macro_rules")
+			)
+		})
+	})
+}

tests/compile-fail/collapsible_if.rs

@@ -34,4 +34,10 @@ fn main() {
         }
     }
 
+	if x == "hello" {
+		print!("Hello ");
+		if y == "world" {
+			println!("world!")
+		}
+	}
 }

tests/compile-test.rs

@@ -5,7 +5,7 @@ use std::path::PathBuf;
 fn run_mode(mode: &'static str) {
     let mut config = compiletest::default_config();
     let cfg_mode = mode.parse().ok().expect("Invalid mode");
-    config.target_rustcflags = Some("-l regex_macros -L target/debug/".to_string());
+    config.target_rustcflags = Some("-L target/debug/".to_string());
 
     config.mode = cfg_mode;
     config.src_base = PathBuf::from(format!("tests/{}", mode));