Merge pull request #1032 from Manishearth/mut_mut

llogiq · web-flow · commit d6e3fa8f4947 · 2016-06-29T20:41:03.000+02:00
Improve `mut_mut` and `collapsible_if`
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@ All notable changes to this project will be documented in this file.
 * New lints: [`wrong_transmute`]
 * For compatibility, `cargo clippy` does not defines the `clippy` feature
   introduced in 0.0.76 anymore
+* [`collapsible_if`] now considers `if let`
 
 ## 0.0.77 — 2016-06-21
 * Rustup to *rustc 1.11.0-nightly (5522e678b 2016-06-20)*
diff --git a/clippy_lints/src/collapsible_if.rs b/clippy_lints/src/collapsible_if.rs
@@ -13,9 +13,9 @@
 //! This lint is **warn** by default
 
 use rustc::lint::*;
-use rustc::hir::*;
 use std::borrow::Cow;
 use syntax::codemap::Spanned;
+use syntax::ast;
 
 use utils::{in_macro, snippet, snippet_block, span_lint_and_then};
 
@@ -45,66 +45,92 @@ impl LintPass for CollapsibleIf {
     }
 }
 
-impl LateLintPass for CollapsibleIf {
-    fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
+impl EarlyLintPass for CollapsibleIf {
+    fn check_expr(&mut self, cx: &EarlyContext, expr: &ast::Expr) {
         if !in_macro(cx, expr.span) {
             check_if(cx, expr)
         }
     }
 }
 
-fn check_if(cx: &LateContext, e: &Expr) {
-    if let ExprIf(ref check, ref then, ref else_) = e.node {
-        if let Some(ref else_) = *else_ {
-            if_let_chain! {[
-                let ExprBlock(ref block) = else_.node,
-                block.stmts.is_empty(),
-                block.rules == BlockCheckMode::DefaultBlock,
-                let Some(ref else_) = block.expr,
-                let ExprIf(_, _, _) = else_.node
-            ], {
+fn check_if(cx: &EarlyContext, expr: &ast::Expr) {
+    match expr.node {
+        ast::ExprKind::If(ref check, ref then, ref else_) => {
+            if let Some(ref else_) = *else_ {
+                check_collapsible_maybe_if_let(cx, else_);
+            } else {
+                check_collapsible_no_if_let(cx, expr, check, then);
+            }
+        }
+        ast::ExprKind::IfLet(_, _, _, Some(ref else_)) => {
+            check_collapsible_maybe_if_let(cx, else_);
+        }
+        _ => (),
+    }
+}
+
+fn check_collapsible_maybe_if_let(cx: &EarlyContext, else_: &ast::Expr) {
+    if_let_chain! {[
+        let ast::ExprKind::Block(ref block) = else_.node,
+        block.stmts.is_empty(),
+        let Some(ref else_) = block.expr,
+    ], {
+        match else_.node {
+            ast::ExprKind::If(..) | ast::ExprKind::IfLet(..) => {
                 span_lint_and_then(cx,
                                    COLLAPSIBLE_IF,
                                    block.span,
                                    "this `else { if .. }` block can be collapsed", |db| {
                     db.span_suggestion(block.span, "try", snippet_block(cx, else_.span, "..").into_owned());
                 });
-            }}
-        } else if let Some(&Expr { node: ExprIf(ref check_inner, ref content, None), span: sp, .. }) =
-               single_stmt_of_block(then) {
-            if e.span.expn_id != sp.expn_id {
-                return;
             }
-            span_lint_and_then(cx, COLLAPSIBLE_IF, e.span, "this if statement can be collapsed", |db| {
-                db.span_suggestion(e.span,
-                                   "try",
-                                   format!("if {} && {} {}",
-                                           check_to_string(cx, check),
-                                           check_to_string(cx, check_inner),
-                                           snippet_block(cx, content.span, "..")));
-            });
+            _ => (),
         }
-    }
+    }}
+}
+
+fn check_collapsible_no_if_let(
+    cx: &EarlyContext,
+    expr: &ast::Expr,
+    check: &ast::Expr,
+    then: &ast::Block,
+) {
+    if_let_chain! {[
+        let Some(inner) = single_stmt_of_block(then),
+        let ast::ExprKind::If(ref check_inner, ref content, None) = inner.node,
+    ], {
+        if expr.span.expn_id != inner.span.expn_id {
+            return;
+        }
+        span_lint_and_then(cx, COLLAPSIBLE_IF, expr.span, "this if statement can be collapsed", |db| {
+            db.span_suggestion(expr.span,
+                               "try",
+                               format!("if {} && {} {}",
+                                       check_to_string(cx, check),
+                                       check_to_string(cx, check_inner),
+                                       snippet_block(cx, content.span, "..")));
+        });
+    }}
 }
 
-fn requires_brackets(e: &Expr) -> bool {
+fn requires_brackets(e: &ast::Expr) -> bool {
     match e.node {
-        ExprBinary(Spanned { node: n, .. }, _, _) if n == BiEq => false,
+        ast::ExprKind::Binary(Spanned { node: n, .. }, _, _) if n == ast::BinOpKind::Eq => false,
         _ => true,
     }
 }
 
-fn check_to_string(cx: &LateContext, e: &Expr) -> Cow<'static, str> {
+fn check_to_string(cx: &EarlyContext, e: &ast::Expr) -> Cow<'static, str> {
     if requires_brackets(e) {
         format!("({})", snippet(cx, e.span, "..")).into()
     } else {
         snippet(cx, e.span, "..")
     }
 }
 
-fn single_stmt_of_block(block: &Block) -> Option<&Expr> {
+fn single_stmt_of_block(block: &ast::Block) -> Option<&ast::Expr> {
     if block.stmts.len() == 1 && block.expr.is_none() {
-        if let StmtExpr(ref expr, _) = block.stmts[0].node {
+        if let ast::StmtKind::Expr(ref expr, _) = block.stmts[0].node {
             single_stmt_of_expr(expr)
         } else {
             None
@@ -120,8 +146,8 @@ fn single_stmt_of_block(block: &Block) -> Option<&Expr> {
     }
 }
 
-fn single_stmt_of_expr(expr: &Expr) -> Option<&Expr> {
-    if let ExprBlock(ref block) = expr.node {
+fn single_stmt_of_expr(expr: &ast::Expr) -> Option<&ast::Expr> {
+    if let ast::ExprKind::Block(ref block) = expr.node {
         single_stmt_of_block(block)
     } else {
         Some(expr)
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -189,7 +189,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_late_lint_pass(box len_zero::LenZero);
     reg.register_late_lint_pass(box misc::CmpOwned);
     reg.register_late_lint_pass(box attrs::AttrPass);
-    reg.register_late_lint_pass(box collapsible_if::CollapsibleIf);
+    reg.register_early_lint_pass(box collapsible_if::CollapsibleIf);
     reg.register_late_lint_pass(box block_in_if_condition::BlockInIfCondition);
     reg.register_late_lint_pass(box misc::ModuloOne);
     reg.register_late_lint_pass(box unicode::Unicode);
diff --git a/clippy_lints/src/minmax.rs b/clippy_lints/src/minmax.rs
@@ -83,11 +83,9 @@ fn fetch_const(args: &[P<Expr>], m: MinMax) -> Option<(MinMax, Constant, &Expr)>
         } else {
             None
         }
+    } else if let Some(c) = constant_simple(&args[1]) {
+        Some((m, c, &args[0]))
     } else {
-        if let Some(c) = constant_simple(&args[1]) {
-            Some((m, c, &args[0]))
-        } else {
-            None
-        }
+        None
     }
 }
diff --git a/clippy_lints/src/mut_mut.rs b/clippy_lints/src/mut_mut.rs
@@ -1,7 +1,8 @@
+use rustc::hir;
+use rustc::hir::intravisit;
 use rustc::lint::*;
 use rustc::ty::{TypeAndMut, TyRef};
-use rustc::hir::*;
-use utils::{in_external_macro, span_lint};
+use utils::{in_external_macro, recover_for_loop, span_lint};
 
 /// **What it does:** This lint checks for instances of `mut mut` references.
 ///
@@ -27,30 +28,56 @@ impl LintPass for MutMut {
 }
 
 impl LateLintPass for MutMut {
-    fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
-        if in_external_macro(cx, expr.span) {
+    fn check_block(&mut self, cx: &LateContext, block: &hir::Block) {
+        intravisit::walk_block(&mut MutVisitor { cx: cx }, block);
+    }
+
+    fn check_ty(&mut self, cx: &LateContext, ty: &hir::Ty) {
+        use rustc::hir::intravisit::Visitor;
+
+        MutVisitor { cx: cx }.visit_ty(ty);
+    }
+}
+
+pub struct MutVisitor<'a, 'tcx: 'a> {
+    cx: &'a LateContext<'a, 'tcx>,
+}
+
+impl<'a, 'tcx, 'v> intravisit::Visitor<'v> for MutVisitor<'a, 'tcx> {
+    fn visit_expr(&mut self, expr: &'v hir::Expr) {
+        if in_external_macro(self.cx, expr.span) {
             return;
         }
 
-        if let ExprAddrOf(MutMutable, ref e) = expr.node {
-            if let ExprAddrOf(MutMutable, _) = e.node {
-                span_lint(cx, MUT_MUT, expr.span, "generally you want to avoid `&mut &mut _` if possible");
-            } else {
-                if let TyRef(_, TypeAndMut { mutbl: MutMutable, .. }) = cx.tcx.expr_ty(e).sty {
-                    span_lint(cx,
-                              MUT_MUT,
-                              expr.span,
-                              "this expression mutably borrows a mutable reference. Consider reborrowing");
-                }
+        if let Some((_, arg, body)) = recover_for_loop(expr) {
+            // A `for` loop lowers to:
+            // ```rust
+            // match ::std::iter::Iterator::next(&mut iter) {
+            // //                                ^^^^
+            // ```
+            // Let's ignore the generated code.
+            intravisit::walk_expr(self, arg);
+            intravisit::walk_expr(self, body);
+        } else if let hir::ExprAddrOf(hir::MutMutable, ref e) = expr.node {
+            if let hir::ExprAddrOf(hir::MutMutable, _) = e.node {
+                span_lint(self.cx, MUT_MUT, expr.span, "generally you want to avoid `&mut &mut _` if possible");
+            } else if let TyRef(_, TypeAndMut { mutbl: hir::MutMutable, .. }) = self.cx.tcx.expr_ty(e).sty {
+                span_lint(self.cx,
+                          MUT_MUT,
+                          expr.span,
+                          "this expression mutably borrows a mutable reference. Consider reborrowing");
             }
         }
     }
 
-    fn check_ty(&mut self, cx: &LateContext, ty: &Ty) {
-        if let TyRptr(_, MutTy { ty: ref pty, mutbl: MutMutable }) = ty.node {
-            if let TyRptr(_, MutTy { mutbl: MutMutable, .. }) = pty.node {
-                span_lint(cx, MUT_MUT, ty.span, "generally you want to avoid `&mut &mut _` if possible");
+    fn visit_ty(&mut self, ty: &hir::Ty) {
+        if let hir::TyRptr(_, hir::MutTy { ty: ref pty, mutbl: hir::MutMutable }) = ty.node {
+            if let hir::TyRptr(_, hir::MutTy { mutbl: hir::MutMutable, .. }) = pty.node {
+                span_lint(self.cx, MUT_MUT, ty.span, "generally you want to avoid `&mut &mut _` if possible");
             }
+
         }
+
+        intravisit::walk_ty(self, ty);
     }
 }
diff --git a/tests/compile-fail/collapsible_if.rs b/tests/compile-fail/collapsible_if.rs
@@ -26,19 +26,32 @@ fn main() {
     // Collaspe `else { if .. }` to `else if ..`
     if x == "hello" {
         print!("Hello ");
-    } else { //~ERROR: this `else { if .. }`
-             //~| HELP try
-             //~| SUGGESTION } else if y == "world"
+    } else {
+        //~^ ERROR: this `else { if .. }`
+        //~| HELP try
+        //~| SUGGESTION } else if y == "world"
         if y == "world" {
             println!("world!")
         }
     }
 
     if x == "hello" {
         print!("Hello ");
-    } else { //~ERROR this `else { if .. }`
-             //~| HELP try
-             //~| SUGGESTION } else if y == "world"
+    } else {
+        //~^ ERROR: this `else { if .. }`
+        //~| HELP try
+        //~| SUGGESTION } else if let Some(42)
+        if let Some(42) = Some(42) {
+            println!("world!")
+        }
+    }
+
+    if x == "hello" {
+        print!("Hello ");
+    } else {
+        //~^ ERROR this `else { if .. }`
+        //~| HELP try
+        //~| SUGGESTION } else if y == "world"
         if y == "world" {
             println!("world")
         }
@@ -47,6 +60,62 @@ fn main() {
         }
     }
 
+    if x == "hello" {
+        print!("Hello ");
+    } else {
+        //~^ ERROR this `else { if .. }`
+        //~| HELP try
+        //~| SUGGESTION } else if let Some(42)
+        if let Some(42) = Some(42) {
+            println!("world")
+        }
+        else {
+            println!("!")
+        }
+    }
+
+    if let Some(42) = Some(42) {
+        print!("Hello ");
+    } else {
+        //~^ ERROR this `else { if .. }`
+        //~| HELP try
+        //~| SUGGESTION } else if let Some(42)
+        if let Some(42) = Some(42) {
+            println!("world")
+        }
+        else {
+            println!("!")
+        }
+    }
+
+    if let Some(42) = Some(42) {
+        print!("Hello ");
+    } else {
+        //~^ ERROR this `else { if .. }`
+        //~| HELP try
+        //~| SUGGESTION } else if x == "hello"
+        if x == "hello" {
+            println!("world")
+        }
+        else {
+            println!("!")
+        }
+    }
+
+    if let Some(42) = Some(42) {
+        print!("Hello ");
+    } else {
+        //~^ ERROR this `else { if .. }`
+        //~| HELP try
+        //~| SUGGESTION } else if let Some(42)
+        if let Some(42) = Some(42) {
+            println!("world")
+        }
+        else {
+            println!("!")
+        }
+    }
+
     // Works because any if with an else statement cannot be collapsed.
     if x == "hello" {
         if y == "world" {
diff --git a/tests/compile-fail/copies.rs b/tests/compile-fail/copies.rs
@@ -8,6 +8,7 @@
 #![allow(unused_variables)]
 #![allow(cyclomatic_complexity)]
 #![allow(blacklisted_name)]
+#![allow(collapsible_if)]
 
 fn bar<T>(_: T) {}
 fn foo() -> bool { unimplemented!() }
diff --git a/tests/compile-fail/matches.rs b/tests/compile-fail/matches.rs
diff --git a/tests/compile-fail/mut_mut.rs b/tests/compile-fail/mut_mut.rs

Original file line number	Diff line number	Diff line change
`@@ -83,11 +83,9 @@ fn fetch_const(args: &[P<Expr>], m: MinMax) -> Option<(MinMax, Constant, &Expr)>`
`83`	`83`	`} else {`
`84`	`84`	`None`
`85`	`85`	`}`
	`86`	`+ } else if let Some(c) = constant_simple(&args[1]) {`
	`87`	`+ Some((m, c, &args[0]))`
`86`	`88`	`} else {`
`87`		`- if let Some(c) = constant_simple(&args[1]) {`
`88`		`- Some((m, c, &args[0]))`
`89`		`- } else {`
`90`		`- None`
`91`		`- }`
	`89`	`+ None`
`92`	`90`	`}`
`93`	`91`	`}`