Merge pull request rust-lang#2166 from HMPerson1/fix_2160

Fix rust-lang#2160

Author: oli-obk

clippy_lints/src/is_unit_expr.rs

@@ -1,7 +1,7 @@
 use rustc::lint::*;
 use syntax::ast::*;
 use syntax::ext::quote::rt::Span;
-use utils::span_note_and_lint;
+use utils::{span_lint, span_note_and_lint};
 
 /// **What it does:** Checks for
 ///  - () being assigned to a variable
@@ -24,6 +24,12 @@ declare_lint! {
     "unintended assignment or use of a unit typed value"
 }
 
+#[derive(Copy, Clone)]
+enum UnitCause {
+    SemiColon,
+    EmptyBlock,
+}
+
 #[derive(Copy, Clone)]
 pub struct UnitExpr;
 
@@ -36,43 +42,16 @@ impl LintPass for UnitExpr {
 impl EarlyLintPass for UnitExpr {
     fn check_expr(&mut self, cx: &EarlyContext, expr: &Expr) {
         if let ExprKind::Assign(ref _left, ref right) = expr.node {
-            if let Some(span) = is_unit_expr(right) {
-                span_note_and_lint(
-                    cx,
-                    UNIT_EXPR,
-                    expr.span,
-                    "This expression evaluates to the Unit type ()",
-                    span,
-                    "Consider removing the trailing semicolon",
-                );
-            }
+            check_for_unit(cx, right);
         }
         if let ExprKind::MethodCall(ref _left, ref args) = expr.node {
             for arg in args {
-                if let Some(span) = is_unit_expr(arg) {
-                    span_note_and_lint(
-                        cx,
-                        UNIT_EXPR,
-                        expr.span,
-                        "This expression evaluates to the Unit type ()",
-                        span,
-                        "Consider removing the trailing semicolon",
-                    );
-                }
+                check_for_unit(cx, arg);
             }
         }
         if let ExprKind::Call(_, ref args) = expr.node {
             for arg in args {
-                if let Some(span) = is_unit_expr(arg) {
-                    span_note_and_lint(
-                        cx,
-                        UNIT_EXPR,
-                        expr.span,
-                        "This expression evaluates to the Unit type ()",
-                        span,
-                        "Consider removing the trailing semicolon",
-                    );
-                }
+                check_for_unit(cx, arg);
             }
         }
     }
@@ -83,28 +62,41 @@ impl EarlyLintPass for UnitExpr {
                 return;
             }
             if let Some(ref expr) = local.init {
-                if let Some(span) = is_unit_expr(expr) {
-                    span_note_and_lint(
-                        cx,
-                        UNIT_EXPR,
-                        expr.span,
-                        "This expression evaluates to the Unit type ()",
-                        span,
-                        "Consider removing the trailing semicolon",
-                    );
-                }
+                check_for_unit(cx, expr);
             }
         }
     }
 }
 
-fn is_unit_expr(expr: &Expr) -> Option<Span> {
+fn check_for_unit(cx: &EarlyContext, expr: &Expr) {
+    match is_unit_expr(expr) {
+        Some((span, UnitCause::SemiColon)) => span_note_and_lint(
+            cx,
+            UNIT_EXPR,
+            expr.span,
+            "This expression evaluates to the Unit type ()",
+            span,
+            "Consider removing the trailing semicolon",
+        ),
+        Some((_span, UnitCause::EmptyBlock)) => span_lint(
+            cx,
+            UNIT_EXPR,
+            expr.span,
+            "This expression evaluates to the Unit type ()",
+        ),
+        None => (),
+    }
+}
+
+fn is_unit_expr(expr: &Expr) -> Option<(Span, UnitCause)> {
     match expr.node {
-        ExprKind::Block(ref block) => if check_last_stmt_in_block(block) {
-            Some(block.stmts[block.stmts.len() - 1].span)
-        } else {
-            None
-        },
+        ExprKind::Block(ref block) => match check_last_stmt_in_block(block) {
+            Some(UnitCause::SemiColon) =>
+                Some((block.stmts[block.stmts.len() - 1].span, UnitCause::SemiColon)),
+            Some(UnitCause::EmptyBlock) =>
+                Some((block.span, UnitCause::EmptyBlock)),
+            None => None
+        }
         ExprKind::If(_, ref then, ref else_) => {
             let check_then = check_last_stmt_in_block(then);
             if let Some(ref else_) = *else_ {
@@ -113,16 +105,15 @@ fn is_unit_expr(expr: &Expr) -> Option<Span> {
                     return Some(*expr_else);
                 }
             }
-            if check_then {
-                Some(expr.span)
-            } else {
-                None
+            match check_then {
+                Some(c) => Some((expr.span, c)),
+                None => None,
             }
         },
         ExprKind::Match(ref _pattern, ref arms) => {
             for arm in arms {
-                if let Some(expr) = is_unit_expr(&arm.body) {
-                    return Some(expr);
+                if let Some(r) = is_unit_expr(&arm.body) {
+                    return Some(r);
                 }
             }
             None
@@ -131,18 +122,19 @@ fn is_unit_expr(expr: &Expr) -> Option<Span> {
     }
 }
 
-fn check_last_stmt_in_block(block: &Block) -> bool {
+fn check_last_stmt_in_block(block: &Block) -> Option<UnitCause> {
+    if block.stmts.is_empty() { return Some(UnitCause::EmptyBlock); }
     let final_stmt = &block.stmts[block.stmts.len() - 1];
 
 
     // Made a choice here to risk false positives on divergent macro invocations
     // like `panic!()`
     match final_stmt.node {
-        StmtKind::Expr(_) => false,
+        StmtKind::Expr(_) => None,
         StmtKind::Semi(ref expr) => match expr.node {
-            ExprKind::Break(_, _) | ExprKind::Continue(_) | ExprKind::Ret(_) => false,
-            _ => true,
+            ExprKind::Break(_, _) | ExprKind::Continue(_) | ExprKind::Ret(_) => None,
+            _ => Some(UnitCause::SemiColon),
         },
-        _ => true,
+        _ => Some(UnitCause::SemiColon), // not sure what's happening here
     }
 }

tests/ui/is_unit_expr.rs

@@ -71,3 +71,9 @@ pub fn foo() -> i32 {
     };
     55
 }
+
+pub fn issue_2160() {
+    let x1 = {};
+    let x2 = if true {} else {};
+    let x3 = match None { Some(_) => {}, None => {}, };
+}

tests/ui/is_unit_expr.stderr

@@ -51,3 +51,21 @@ note: Consider removing the trailing semicolon
 42 |             x;
    |             ^^
 
+error: This expression evaluates to the Unit type ()
+  --> $DIR/is_unit_expr.rs:76:14
+   |
+76 |     let x1 = {};
+   |              ^^
+
+error: This expression evaluates to the Unit type ()
+  --> $DIR/is_unit_expr.rs:77:14
+   |
+77 |     let x2 = if true {} else {};
+   |              ^^^^^^^^^^^^^^^^^^
+
+error: This expression evaluates to the Unit type ()
+  --> $DIR/is_unit_expr.rs:78:14
+   |
+78 |     let x3 = match None { Some(_) => {}, None => {}, };
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+