From 5fca6eb89eebe00238c66bc57b1ab2ecf236dfdd Mon Sep 17 00:00:00 2001 From: HMPerson1 Date: Fri, 20 Oct 2017 21:26:41 -0400 Subject: [PATCH 1/3] Fix #2160 --- clippy_lints/src/is_unit_expr.rs | 1 + tests/ui/is_unit_expr.rs | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/clippy_lints/src/is_unit_expr.rs b/clippy_lints/src/is_unit_expr.rs index 3f94178e524..422d9739ef9 100644 --- a/clippy_lints/src/is_unit_expr.rs +++ b/clippy_lints/src/is_unit_expr.rs @@ -132,6 +132,7 @@ fn is_unit_expr(expr: &Expr) -> Option { } fn check_last_stmt_in_block(block: &Block) -> bool { + if block.stmts.is_empty() { return false; } let final_stmt = &block.stmts[block.stmts.len() - 1]; diff --git a/tests/ui/is_unit_expr.rs b/tests/ui/is_unit_expr.rs index 24a2587dc53..7e2cc4725f0 100644 --- a/tests/ui/is_unit_expr.rs +++ b/tests/ui/is_unit_expr.rs @@ -71,3 +71,7 @@ pub fn foo() -> i32 { }; 55 } + +pub fn issue_2160() { + let x = {}; +} From e2bc383383b211c63e5edf136c47e6d4449c8a6f Mon Sep 17 00:00:00 2001 From: HMPerson1 Date: Thu, 30 Nov 2017 19:38:29 -0500 Subject: [PATCH 2/3] Add linting for empty blocks too --- clippy_lints/src/is_unit_expr.rs | 109 ++++++++++++++----------------- tests/ui/is_unit_expr.rs | 4 +- 2 files changed, 53 insertions(+), 60 deletions(-) diff --git a/clippy_lints/src/is_unit_expr.rs b/clippy_lints/src/is_unit_expr.rs index 422d9739ef9..90e2ef760f7 100644 --- a/clippy_lints/src/is_unit_expr.rs +++ b/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 @@ "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 @@ fn get_lints(&self) -> LintArray { 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 @@ fn check_stmt(&mut self, cx: &EarlyContext, stmt: &Stmt) { 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 { +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 { 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,19 +122,19 @@ fn is_unit_expr(expr: &Expr) -> Option { } } -fn check_last_stmt_in_block(block: &Block) -> bool { - if block.stmts.is_empty() { return false; } +fn check_last_stmt_in_block(block: &Block) -> Option { + 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 } } diff --git a/tests/ui/is_unit_expr.rs b/tests/ui/is_unit_expr.rs index 7e2cc4725f0..6c8f108d9f6 100644 --- a/tests/ui/is_unit_expr.rs +++ b/tests/ui/is_unit_expr.rs @@ -73,5 +73,7 @@ pub fn foo() -> i32 { } pub fn issue_2160() { - let x = {}; + let x1 = {}; + let x2 = if true {} else {}; + let x3 = match None { Some(_) => {}, None => {}, }; } From c2c324ec657888667a71f361f3cdca48ff843a61 Mon Sep 17 00:00:00 2001 From: HMPerson1 Date: Thu, 30 Nov 2017 19:50:31 -0500 Subject: [PATCH 3/3] Update ui test --- tests/ui/is_unit_expr.stderr | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/ui/is_unit_expr.stderr b/tests/ui/is_unit_expr.stderr index 5524f866488..64a7ad86b70 100644 --- a/tests/ui/is_unit_expr.stderr +++ b/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 => {}, }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +