diff --git a/clippy_lints/src/is_unit_expr.rs b/clippy_lints/src/is_unit_expr.rs deleted file mode 100644 index 90e2ef760f7..00000000000 --- a/clippy_lints/src/is_unit_expr.rs +++ /dev/null @@ -1,140 +0,0 @@ -use rustc::lint::*; -use syntax::ast::*; -use syntax::ext::quote::rt::Span; -use utils::{span_lint, 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)] -enum UnitCause { - SemiColon, - EmptyBlock, -} - -#[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 { - check_for_unit(cx, right); - } - if let ExprKind::MethodCall(ref _left, ref args) = expr.node { - for arg in args { - check_for_unit(cx, arg); - } - } - if let ExprKind::Call(_, ref args) = expr.node { - for arg in args { - check_for_unit(cx, arg); - } - } - } - - 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 { - check_for_unit(cx, expr); - } - } - } -} - -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) => 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_ { - let check_else = is_unit_expr(else_); - if let Some(ref expr_else) = check_else { - return Some(*expr_else); - } - } - match check_then { - Some(c) => Some((expr.span, c)), - None => None, - } - }, - ExprKind::Match(ref _pattern, ref arms) => { - for arm in arms { - if let Some(r) = is_unit_expr(&arm.body) { - return Some(r); - } - } - None - }, - _ => None, - } -} - -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(_) => None, - StmtKind::Semi(ref expr) => match expr.node { - ExprKind::Break(_, _) | ExprKind::Continue(_) | ExprKind::Ret(_) => None, - _ => Some(UnitCause::SemiColon), - }, - _ => Some(UnitCause::SemiColon), // not sure what's happening here - } -} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 11afefd6d4b..c1472484078 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -111,7 +111,6 @@ pub mod if_not_else; pub mod infinite_iter; pub mod int_plus_one; pub mod invalid_ref; -pub mod is_unit_expr; pub mod items_after_statements; pub mod large_enum_variant; pub mod len_zero; @@ -268,7 +267,6 @@ 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); @@ -365,6 +363,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_early_lint_pass(box const_static_lifetime::StaticConst); reg.register_late_lint_pass(box fallible_impl_from::FallibleImplFrom); reg.register_late_lint_pass(box replace_consts::ReplaceConsts); + reg.register_late_lint_pass(box types::UnitArg); reg.register_lint_group("clippy_restrictions", vec![ arithmetic::FLOAT_ARITHMETIC, @@ -478,7 +477,6 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { if_let_redundant_pattern_matching::IF_LET_REDUNDANT_PATTERN_MATCHING, infinite_iter::INFINITE_ITER, invalid_ref::INVALID_REF, - is_unit_expr::UNIT_EXPR, large_enum_variant::LARGE_ENUM_VARIANT, len_zero::LEN_WITHOUT_IS_EMPTY, len_zero::LEN_ZERO, @@ -607,6 +605,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { types::LINKEDLIST, types::TYPE_COMPLEXITY, types::UNIT_CMP, + types::UNIT_ARG, types::UNNECESSARY_CAST, unicode::ZERO_WIDTH_SPACE, unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME, diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index c0c72408c7d..8d88217b924 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -322,25 +322,22 @@ declare_lint! { fn check_let_unit(cx: &LateContext, decl: &Decl) { if let DeclLocal(ref local) = decl.node { - match cx.tables.pat_ty(&local.pat).sty { - ty::TyTuple(slice, _) if slice.is_empty() => { - if in_external_macro(cx, decl.span) || in_macro(local.pat.span) { - return; - } - if higher::is_from_for_desugar(decl) { - return; - } - span_lint( - cx, - LET_UNIT_VALUE, - decl.span, - &format!( - "this let-binding has unit value. Consider omitting `let {} =`", - snippet(cx, local.pat.span, "..") - ), - ); - }, - _ => (), + if is_unit(cx.tables.pat_ty(&local.pat)) { + if in_external_macro(cx, decl.span) || in_macro(local.pat.span) { + return; + } + if higher::is_from_for_desugar(decl) { + return; + } + span_lint( + cx, + LET_UNIT_VALUE, + decl.span, + &format!( + "this let-binding has unit value. Consider omitting `let {} =`", + snippet(cx, local.pat.span, "..") + ), + ); } } } @@ -395,31 +392,118 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnitCmp { } if let ExprBinary(ref cmp, ref left, _) = expr.node { let op = cmp.node; - if op.is_comparison() { - match cx.tables.expr_ty(left).sty { - ty::TyTuple(slice, _) if slice.is_empty() => { - let result = match op { - BiEq | BiLe | BiGe => "true", - _ => "false", - }; - span_lint( - cx, - UNIT_CMP, - expr.span, - &format!( - "{}-comparison of unit values detected. This will always be {}", - op.as_str(), - result - ), - ); - }, - _ => (), - } + if op.is_comparison() && is_unit(cx.tables.expr_ty(left)) { + let result = match op { + BiEq | BiLe | BiGe => "true", + _ => "false", + }; + span_lint( + cx, + UNIT_CMP, + expr.span, + &format!( + "{}-comparison of unit values detected. This will always be {}", + op.as_str(), + result + ), + ); } } } } +/// **What it does:** Checks for passing a unit value as an argument to a function without using a unit literal (`()`). +/// +/// **Why is this bad?** This is likely the result of an accidental semicolon. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// foo({ +/// let a = bar(); +/// baz(a); +/// }) +/// ``` +declare_lint! { + pub UNIT_ARG, + Warn, + "passing unit to a function" +} + +pub struct UnitArg; + +impl LintPass for UnitArg { + fn get_lints(&self) -> LintArray { + lint_array!(UNIT_ARG) + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnitArg { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { + if in_macro(expr.span) { + return; + } + match expr.node { + ExprCall(_, ref args) | ExprMethodCall(_, _, ref args) => { + for arg in args { + if is_unit(cx.tables.expr_ty(arg)) && !is_unit_literal(arg) { + let map = &cx.tcx.hir; + // apparently stuff in the desugaring of `?` can trigger this + // so check for that here + // only the calls to `Try::from_error` is marked as desugared, + // so we need to check both the current Expr and its parent. + if !is_questionmark_desugar_marked_call(expr) { + if_chain!{ + let opt_parent_node = map.find(map.get_parent_node(expr.id)); + if let Some(hir::map::NodeExpr(parent_expr)) = opt_parent_node; + if is_questionmark_desugar_marked_call(parent_expr); + then {} + else { + // `expr` and `parent_expr` where _both_ not from + // desugaring `?`, so lint + span_lint_and_sugg( + cx, + UNIT_ARG, + arg.span, + "passing a unit value to a function", + "if you intended to pass a unit value, use a unit literal instead", + "()".to_string(), + ); + } + } + } + } + } + }, + _ => (), + } + } +} + +fn is_questionmark_desugar_marked_call(expr: &Expr) -> bool { + use syntax_pos::hygiene::CompilerDesugaringKind; + if let ExprCall(ref callee, _) = expr.node { + callee.span.is_compiler_desugaring(CompilerDesugaringKind::QuestionMark) + } else { + false + } +} + +fn is_unit(ty: Ty) -> bool { + match ty.sty { + ty::TyTuple(slice, _) if slice.is_empty() => true, + _ => false, + } +} + +fn is_unit_literal(expr: &Expr) -> bool { + match expr.node { + ExprTup(ref slice) if slice.is_empty() => true, + _ => false, + } +} + pub struct CastPass; /// **What it does:** Checks for casts from any numerical to a float type where diff --git a/tests/ui/is_unit_expr.rs b/tests/ui/is_unit_expr.rs deleted file mode 100644 index 6c8f108d9f6..00000000000 --- a/tests/ui/is_unit_expr.rs +++ /dev/null @@ -1,79 +0,0 @@ - - -#![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; - }, - }; - - loop { - let a2 = match a1 { - Some(x) => x, - _ => { - break; - }, - }; - let a2 = match a1 { - Some(x) => x, - _ => { - continue; - }, - }; - } -} - -pub fn foo() -> i32 { - let a2 = match None { - Some(x) => x, - _ => { - return 42; - }, - }; - 55 -} - -pub fn issue_2160() { - let x1 = {}; - let x2 = if true {} else {}; - let x3 = match None { Some(_) => {}, None => {}, }; -} diff --git a/tests/ui/is_unit_expr.stderr b/tests/ui/is_unit_expr.stderr deleted file mode 100644 index 7a16fe971f3..00000000000 --- a/tests/ui/is_unit_expr.stderr +++ /dev/null @@ -1,73 +0,0 @@ -error: This expression evaluates to the Unit type () - --> $DIR/is_unit_expr.rs:8:13 - | -8 | let x = { - | _____________^ -9 | | "foo"; -10 | | "baz"; -11 | | }; - | |_____^ - | - = note: `-D unit-expr` implied by `-D warnings` -note: Consider removing the trailing semicolon - --> $DIR/is_unit_expr.rs:10:9 - | -10 | "baz"; - | ^^^^^^ - -error: This expression evaluates to the Unit type () - --> $DIR/is_unit_expr.rs:22:13 - | -22 | let z = if true { - | _____________^ -23 | | "foo"; -24 | | } else { -25 | | "bar"; -26 | | }; - | |_____^ - | -note: Consider removing the trailing semicolon - --> $DIR/is_unit_expr.rs:25:9 - | -25 | "bar"; - | ^^^^^^ - -error: This expression evaluates to the Unit type () - --> $DIR/is_unit_expr.rs:40:14 - | -40 | let a3 = match a1 { - | ______________^ -41 | | Some(x) => { -42 | | x; -43 | | }, -... | -46 | | }, -47 | | }; - | |_____^ - | -note: Consider removing the trailing semicolon - --> $DIR/is_unit_expr.rs:42:13 - | -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 => {}, }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: aborting due to 6 previous errors - diff --git a/tests/ui/unit_arg.rs b/tests/ui/unit_arg.rs new file mode 100644 index 00000000000..8f290446b5e --- /dev/null +++ b/tests/ui/unit_arg.rs @@ -0,0 +1,55 @@ +#![warn(unit_arg)] +#![allow(no_effect)] + +use std::fmt::Debug; + +fn foo(t: T) { + println!("{:?}", t); +} + +fn foo3(t1: T1, t2: T2, t3: T3) { + println!("{:?}, {:?}, {:?}", t1, t2, t3); +} + +struct Bar; + +impl Bar { + fn bar(&self, t: T) { + println!("{:?}", t); + } +} + +fn bad() { + foo({}); + foo({ 1; }); + foo(foo(1)); + foo({ + foo(1); + foo(2); + }); + foo3({}, 2, 2); + let b = Bar; + b.bar({ 1; }); +} + +fn ok() { + foo(()); + foo(1); + foo({ 1 }); + foo3("a", 3, vec![3]); + let b = Bar; + b.bar({ 1 }); + b.bar(()); + question_mark(); +} + +fn question_mark() -> Result<(), ()> { + Ok(Ok(())?)?; + Ok(Ok(()))??; + Ok(()) +} + +fn main() { + bad(); + ok(); +} diff --git a/tests/ui/unit_arg.stderr b/tests/ui/unit_arg.stderr new file mode 100644 index 00000000000..ca48f39263b --- /dev/null +++ b/tests/ui/unit_arg.stderr @@ -0,0 +1,68 @@ +error: passing a unit value to a function + --> $DIR/unit_arg.rs:23:9 + | +23 | foo({}); + | ^^ + | + = note: `-D unit-arg` implied by `-D warnings` +help: if you intended to pass a unit value, use a unit literal instead + | +23 | foo(()); + | ^^ + +error: passing a unit value to a function + --> $DIR/unit_arg.rs:24:9 + | +24 | foo({ 1; }); + | ^^^^^^ +help: if you intended to pass a unit value, use a unit literal instead + | +24 | foo(()); + | ^^ + +error: passing a unit value to a function + --> $DIR/unit_arg.rs:25:9 + | +25 | foo(foo(1)); + | ^^^^^^ +help: if you intended to pass a unit value, use a unit literal instead + | +25 | foo(()); + | ^^ + +error: passing a unit value to a function + --> $DIR/unit_arg.rs:26:9 + | +26 | foo({ + | _________^ +27 | | foo(1); +28 | | foo(2); +29 | | }); + | |_____^ +help: if you intended to pass a unit value, use a unit literal instead + | +26 | foo(()); + | ^^ + +error: passing a unit value to a function + --> $DIR/unit_arg.rs:30:10 + | +30 | foo3({}, 2, 2); + | ^^ +help: if you intended to pass a unit value, use a unit literal instead + | +30 | foo3((), 2, 2); + | ^^ + +error: passing a unit value to a function + --> $DIR/unit_arg.rs:32:11 + | +32 | b.bar({ 1; }); + | ^^^^^^ +help: if you intended to pass a unit value, use a unit literal instead + | +32 | b.bar(()); + | ^^ + +error: aborting due to 6 previous errors +