diff --git a/clippy_lints/src/is_unit_expr.rs b/clippy_lints/src/is_unit_expr.rs new file mode 100644 index 000000000000..4f3c755d7312 --- /dev/null +++ b/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 { + 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, + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 19b8816cd1e0..5b484aceed89 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/tests/ui/is_unit_expr.rs b/tests/ui/is_unit_expr.rs new file mode 100644 index 000000000000..d1f45d517b05 --- /dev/null +++ b/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; + }, + }; +} diff --git a/tests/ui/is_unit_expr.stderr b/tests/ui/is_unit_expr.stderr new file mode 100644 index 000000000000..dafebb1c82af --- /dev/null +++ b/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 +