Detect Expressions that evaluate to Unit and warn the user

clippy_lints/src/is_unit_expr.rs

@@ -0,0 +1,147 @@
+use rustc::lint::*;
+use syntax::ast::*;
+use syntax::ext::quote::rt::Span;
+use utils::span_note_and_lint;
+
+/// **What it does:** Checks for
+///  - () being assigned to a variable
+///  - () being passed to a function
+///
+/// **Why is this bad?** It is extremely unlikely that a user intended to
+/// assign '()' to valiable. Instead,
+/// Unit is what a block evaluates to when it returns nothing. This is
+/// typically caused by a trailing
+///   unintended semicolon.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// * `let x = {"foo" ;}` when the user almost certainly intended `let x
+/// ={"foo"}`
+
+declare_lint! {
+    pub UNIT_EXPR,
+    Warn,
+    "unintended assignment or use of a unit typed value"
+}
+
+#[derive(Copy, Clone)]
+pub struct UnitExpr;
+
+impl LintPass for UnitExpr {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(UNIT_EXPR)
+    }
+}
+
+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",
+                );
+            }
+        }
+        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",
+                    );
+                }
+            }
+        }
+        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",
+                    );
+                }
+            }
+        }
+    }
+
+    fn check_stmt(&mut self, cx: &EarlyContext, stmt: &Stmt) {
+        if let StmtKind::Local(ref local) = stmt.node {
+            if local.pat.node == PatKind::Wild {
+                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",
+                    );
+                }
+            }
+        }
+    }
+}
+fn is_unit_expr(expr: &Expr) -> Option<Span> {
+    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::If(_, ref then, ref else_) => {
+            let check_then = check_last_stmt_in_block(then);
+            if let Some(ref else_) = *else_ {
+                let check_else = is_unit_expr(else_);
+                if let Some(ref expr_else) = check_else {
+                    return Some(*expr_else);
+                }
+            }
+            if check_then {
+                Some(expr.span)
+            } else {
+                None
+            }
+        },
+        ExprKind::Match(ref _pattern, ref arms) => {
+            for arm in arms {
+                if let Some(expr) = is_unit_expr(&arm.body) {
+                    return Some(expr);
+                }
+            }
+            None
+        },
+        _ => None,
+    }
+}
+
+fn check_last_stmt_in_block(block: &Block) -> bool {
+    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::Semi(ref expr) => match expr.node {
+            ExprKind::Break(_, _) | ExprKind::Ret(_) => false,
+            _ => true,
+        },
+        _ => true,
+    }
+}

clippy_lints/src/lib.rs

@@ -91,6 +91,7 @@ pub mod functions;
 pub mod identity_op;
 pub mod if_let_redundant_pattern_matching;
 pub mod if_not_else;
+pub mod is_unit_expr;
 pub mod infinite_iter;
 pub mod items_after_statements;
 pub mod large_enum_variant;
@@ -233,6 +234,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_late_lint_pass(box approx_const::Pass);
     reg.register_late_lint_pass(box misc::Pass);
     reg.register_early_lint_pass(box precedence::Precedence);
+    reg.register_early_lint_pass(box is_unit_expr::UnitExpr);
     reg.register_early_lint_pass(box needless_continue::NeedlessContinue);
     reg.register_late_lint_pass(box eta_reduction::EtaPass);
     reg.register_late_lint_pass(box identity_op::IdentityOp);
@@ -504,6 +506,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         panic::PANIC_PARAMS,
         partialeq_ne_impl::PARTIALEQ_NE_IMPL,
         precedence::PRECEDENCE,
+        is_unit_expr::UNIT_EXPR,
         print::PRINT_WITH_NEWLINE,
         ptr::CMP_NULL,
         ptr::MUT_FROM_REF,

tests/ui/is_unit_expr.rs

@@ -0,0 +1,48 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+#![warn(unit_expr)]
+#[allow(unused_variables)]
+
+fn main() {
+    // lint should note removing the semicolon from "baz"
+    let x = {
+        "foo";
+        "baz";
+    };
+
+
+    // lint should ignore false positive.
+    let y = if true {
+        "foo"
+    } else {
+        return;
+    };
+
+    // lint should note removing semicolon from "bar"
+    let z = if true {
+        "foo";
+    } else {
+        "bar";
+    };
+
+
+    let a1 = Some(5);
+
+    // lint should ignore false positive
+    let a2 = match a1 {
+        Some(x) => x,
+        _ => {
+            return;
+        },
+    };
+
+    // lint should note removing the semicolon after `x;`
+    let a3 = match a1 {
+        Some(x) => {
+            x;
+        },
+        _ => {
+            0;
+        },
+    };
+}

tests/ui/is_unit_expr.stderr

@@ -0,0 +1,52 @@
+error: This expression evaluates to the Unit type ()
+  --> $DIR/is_unit_expr.rs:9:13
+   |
+9  |       let x = {
+   |  _____________^
+10 | |         "foo";
+11 | |         "baz";
+12 | |     };
+   | |_____^
+   |
+   = note: `-D unit-expr` implied by `-D warnings`
+note: Consider removing the trailing semicolon
+  --> $DIR/is_unit_expr.rs:11:9
+   |
+11 |         "baz";
+   |         ^^^^^^
+
+error: This expression evaluates to the Unit type ()
+  --> $DIR/is_unit_expr.rs:23:13
+   |
+23 |       let z = if true{
+   |  _____________^
+24 | |         "foo";
+25 | |     } else{
+26 | |         "bar";
+27 | |     };
+   | |_____^
+   |
+note: Consider removing the trailing semicolon
+  --> $DIR/is_unit_expr.rs:26:9
+   |
+26 |         "bar";
+   |         ^^^^^^
+
+error: This expression evaluates to the Unit type ()
+  --> $DIR/is_unit_expr.rs:39:14
+   |
+39 |       let a3 = match a1 {
+   |  ______________^
+40 | |         Some(x) => {x;},
+41 | |         _ => {0;},
+42 | |     };
+   | |_____^
+   |
+note: Consider removing the trailing semicolon
+  --> $DIR/is_unit_expr.rs:40:21
+   |
+40 |         Some(x) => {x;},
+   |                     ^^
+
+error: aborting due to 3 previous errors
+