Auto merge of #4123 - Centril:rustup-let-chains-ast, r=Manishearth

Fix fallout from rust-lang/rust PR 60861 Fixes incoming breakage for unlanded rust-lang/rust#60861. Tests are passing locally; the Rust PR now needs to land first. @Manishearth also says we'll want to split out to a `collapsible_if_let` once we have let-chains working in Rust nightly or something.
rust-lang · Jun 24, 2019 · 8c80b65 · 8c80b65
2 parents c5d1ecd + 46a0e54
commit 8c80b65
Show file tree

Hide file tree

Showing 8 changed files with 73 additions and 40 deletions.
diff --git a/clippy_lints/src/collapsible_if.rs b/clippy_lints/src/collapsible_if.rs
@@ -84,18 +84,14 @@ impl EarlyLintPass for CollapsibleIf {
 }
 
 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_)) => {
+    if let ast::ExprKind::If(check, then, else_) = &expr.node {
+        if let Some(else_) = else_ {
             check_collapsible_maybe_if_let(cx, else_);
-        },
-        _ => (),
+        } else if let ast::ExprKind::Let(..) = check.node {
+            // Prevent triggering on `if let a = b { if c { .. } }`.
+        } else {
+            check_collapsible_no_if_let(cx, expr, check, then);
+        }
     }
 }
 
@@ -113,22 +109,18 @@ fn check_collapsible_maybe_if_let(cx: &EarlyContext<'_>, else_: &ast::Expr) {
         if !block_starts_with_comment(cx, block);
         if let Some(else_) = expr_block(block);
         if !in_macro_or_desugar(else_.span);
+        if let ast::ExprKind::If(..) = else_.node;
         then {
-            match else_.node {
-                ast::ExprKind::If(..) | ast::ExprKind::IfLet(..) => {
-                    let mut applicability = Applicability::MachineApplicable;
-                    span_lint_and_sugg(
-                        cx,
-                        COLLAPSIBLE_IF,
-                        block.span,
-                        "this `else { if .. }` block can be collapsed",
-                        "try",
-                        snippet_block_with_applicability(cx, else_.span, "..", &mut applicability).into_owned(),
-                        applicability,
-                    );
-                }
-                _ => (),
-            }
+            let mut applicability = Applicability::MachineApplicable;
+            span_lint_and_sugg(
+                cx,
+                COLLAPSIBLE_IF,
+                block.span,
+                "this `else { if .. }` block can be collapsed",
+                "try",
+                snippet_block_with_applicability(cx, else_.span, "..", &mut applicability).into_owned(),
+                applicability,
+            );
         }
     }
 }
@@ -139,6 +131,11 @@ fn check_collapsible_no_if_let(cx: &EarlyContext<'_>, expr: &ast::Expr, check: &
         if let Some(inner) = expr_block(then);
         if let ast::ExprKind::If(ref check_inner, ref content, None) = inner.node;
         then {
+            if let ast::ExprKind::Let(..) = check_inner.node {
+                // Prevent triggering on `if c { if let a = b { .. } }`.
+                return;
+            }
+
             if expr.span.ctxt() != inner.span.ctxt() {
                 return;
             }

diff --git a/clippy_lints/src/formatting.rs b/clippy_lints/src/formatting.rs
@@ -248,9 +248,7 @@ fn is_block(expr: &ast::Expr) -> bool {
 /// Match `if` or `if let` expressions and return the `then` and `else` block.
 fn unsugar_if(expr: &ast::Expr) -> Option<(&P<ast::Block>, &Option<P<ast::Expr>>)> {
     match expr.node {
-        ast::ExprKind::If(_, ref then, ref else_) | ast::ExprKind::IfLet(_, _, ref then, ref else_) => {
-            Some((then, else_))
-        },
+        ast::ExprKind::If(_, ref then, ref else_) => Some((then, else_)),
         _ => None,
     }
 }
diff --git a/clippy_lints/src/identity_conversion.rs b/clippy_lints/src/identity_conversion.rs
@@ -50,8 +50,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IdentityConversion {
                 };
                 if let ExprKind::Call(_, ref args) = e.node {
                     self.try_desugar_arm.push(args[0].hir_id);
-                } else {
-                    return;
                 }
             },
 

diff --git a/clippy_lints/src/needless_continue.rs b/clippy_lints/src/needless_continue.rs
@@ -209,12 +209,11 @@ fn with_loop_block<F>(expr: &ast::Expr, mut func: F)
 where
     F: FnMut(&ast::Block, Option<&ast::Label>),
 {
-    match expr.node {
-        ast::ExprKind::While(_, ref loop_block, ref label)
-        | ast::ExprKind::WhileLet(_, _, ref loop_block, ref label)
-        | ast::ExprKind::ForLoop(_, _, ref loop_block, ref label)
-        | ast::ExprKind::Loop(ref loop_block, ref label) => func(loop_block, label.as_ref()),
-        _ => {},
+    if let ast::ExprKind::While(_, loop_block, label)
+    | ast::ExprKind::ForLoop(_, _, loop_block, label)
+    | ast::ExprKind::Loop(loop_block, label) = &expr.node
+    {
+        func(loop_block, label.as_ref());
     }
 }
 

diff --git a/clippy_lints/src/utils/sugg.rs b/clippy_lints/src/utils/sugg.rs
@@ -135,7 +135,7 @@ impl<'a> Sugg<'a> {
             | ast::ExprKind::Box(..)
             | ast::ExprKind::Closure(..)
             | ast::ExprKind::If(..)
-            | ast::ExprKind::IfLet(..)
+            | ast::ExprKind::Let(..)
             | ast::ExprKind::Unary(..)
             | ast::ExprKind::Match(..) => Sugg::MaybeParen(snippet),
             ast::ExprKind::Async(..)
@@ -162,7 +162,6 @@ impl<'a> Sugg<'a> {
             | ast::ExprKind::Tup(..)
             | ast::ExprKind::Array(..)
             | ast::ExprKind::While(..)
-            | ast::ExprKind::WhileLet(..)
             | ast::ExprKind::Await(..)
             | ast::ExprKind::Err => Sugg::NonParen(snippet),
             ast::ExprKind::Range(.., RangeLimits::HalfOpen) => Sugg::BinOp(AssocOp::DotDot, snippet),

diff --git a/clippy_lints/src/utils/usage.rs b/clippy_lints/src/utils/usage.rs
@@ -37,7 +37,7 @@ pub fn is_potentially_mutated<'a, 'tcx>(variable: &'tcx Path, expr: &'tcx Expr,
     if let Res::Local(id) = variable.res {
         mutated_variables(expr, cx).map_or(true, |mutated| mutated.contains(&id))
     } else {
-        return true;
+        true
     }
 }
 

diff --git a/tests/ui/collapsible_if.fixed b/tests/ui/collapsible_if.fixed
@@ -172,4 +172,25 @@ else {
             println!("Hello world!");
         }
     }
+
+    // Test behavior wrt. `let_chains`.
+    // None of the cases below should be collapsed.
+    fn truth() -> bool { true }
+
+    // Prefix:
+    if let 0 = 1 {
+        if truth() {}
+    }
+
+    // Suffix:
+    if truth() {
+        if let 0 = 1 {}
+    }
+
+    // Midfix:
+    if truth() {
+        if let 0 = 1 {
+            if truth() {}
+        }
+    }
 }
diff --git a/tests/ui/collapsible_if.rs b/tests/ui/collapsible_if.rs
@@ -200,4 +200,25 @@ fn main() {
             println!("Hello world!");
         }
     }
+
+    // Test behavior wrt. `let_chains`.
+    // None of the cases below should be collapsed.
+    fn truth() -> bool { true }
+
+    // Prefix:
+    if let 0 = 1 {
+        if truth() {}
+    }
+
+    // Suffix:
+    if truth() {
+        if let 0 = 1 {}
+    }
+
+    // Midfix:
+    if truth() {
+        if let 0 = 1 {
+            if truth() {}
+        }
+    }
 }