diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 133001e35..eabc5ca73 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -17,6 +17,8 @@ use rustc_target::spec::abi::Abi; use rustc_typeck::hir_ty_to_ty; use syntax::ast::{FloatTy, IntTy, LitIntType, LitKind, UintTy}; use syntax::errors::DiagnosticBuilder; +use syntax::ext::base::MacroKind; +use syntax::ext::hygiene::ExpnKind; use syntax::source_map::Span; use syntax::symbol::{sym, Symbol}; @@ -485,7 +487,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LetUnitValue { } declare_clippy_lint! { - /// **What it does:** Checks for comparisons to unit. + /// **What it does:** Checks for comparisons to unit. This includes all binary + /// comparisons (like `==` and `<`) and asserts. /// /// **Why is this bad?** Unit is always equal to itself, and thus is just a /// clumsily written constant. Mostly this happens when someone accidentally @@ -517,6 +520,14 @@ declare_clippy_lint! { /// baz(); /// } /// ``` + /// + /// For asserts: + /// ```rust + /// # fn foo() {}; + /// # fn bar() {}; + /// assert_eq!({ foo(); }, { bar(); }); + /// ``` + /// will always succeed pub UNIT_CMP, correctness, "comparing unit values" @@ -527,6 +538,30 @@ declare_lint_pass!(UnitCmp => [UNIT_CMP]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnitCmp { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { if expr.span.from_expansion() { + if let Some(callee) = expr.span.source_callee() { + if let ExpnKind::Macro(MacroKind::Bang, symbol) = callee.kind { + if let ExprKind::Binary(ref cmp, ref left, _) = expr.kind { + let op = cmp.node; + if op.is_comparison() && is_unit(cx.tables.expr_ty(left)) { + let result = match &*symbol.as_str() { + "assert_eq" | "debug_assert_eq" => "succeed", + "assert_ne" | "debug_assert_ne" => "fail", + _ => return, + }; + span_lint( + cx, + UNIT_CMP, + expr.span, + &format!( + "`{}` of unit values detected. This will always {}", + symbol.as_str(), + result + ), + ); + } + } + } + } return; } if let ExprKind::Binary(ref cmp, ref left, _) = expr.kind { diff --git a/tests/ui/unit_cmp.rs b/tests/ui/unit_cmp.rs index 48c22f7f8..8d3a4eed8 100644 --- a/tests/ui/unit_cmp.rs +++ b/tests/ui/unit_cmp.rs @@ -20,4 +20,38 @@ fn main() { } > { false; } {} + + assert_eq!( + { + true; + }, + { + false; + } + ); + debug_assert_eq!( + { + true; + }, + { + false; + } + ); + + assert_ne!( + { + true; + }, + { + false; + } + ); + debug_assert_ne!( + { + true; + }, + { + false; + } + ); } diff --git a/tests/ui/unit_cmp.stderr b/tests/ui/unit_cmp.stderr index 562934030..578a6218e 100644 --- a/tests/ui/unit_cmp.stderr +++ b/tests/ui/unit_cmp.stderr @@ -22,5 +22,61 @@ LL | | false; LL | | } {} | |_____^ -error: aborting due to 2 previous errors +error: `assert_eq` of unit values detected. This will always succeed + --> $DIR/unit_cmp.rs:24:5 + | +LL | / assert_eq!( +LL | | { +LL | | true; +LL | | }, +... | +LL | | } +LL | | ); + | |______^ + | + = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + +error: `debug_assert_eq` of unit values detected. This will always succeed + --> $DIR/unit_cmp.rs:32:5 + | +LL | / debug_assert_eq!( +LL | | { +LL | | true; +LL | | }, +... | +LL | | } +LL | | ); + | |______^ + | + = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + +error: `assert_ne` of unit values detected. This will always fail + --> $DIR/unit_cmp.rs:41:5 + | +LL | / assert_ne!( +LL | | { +LL | | true; +LL | | }, +... | +LL | | } +LL | | ); + | |______^ + | + = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + +error: `debug_assert_ne` of unit values detected. This will always fail + --> $DIR/unit_cmp.rs:49:5 + | +LL | / debug_assert_ne!( +LL | | { +LL | | true; +LL | | }, +... | +LL | | } +LL | | ); + | |______^ + | + = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) + +error: aborting due to 6 previous errors diff --git a/tests/ui/unused_unit.fixed b/tests/ui/unused_unit.fixed index 17c1a5de5..3f6362472 100644 --- a/tests/ui/unused_unit.fixed +++ b/tests/ui/unused_unit.fixed @@ -34,6 +34,7 @@ fn return_unit() { } #[allow(clippy::needless_return)] #[allow(clippy::never_loop)] +#[allow(clippy::unit_cmp)] fn main() { let u = Unitter; assert_eq!(u.get_unit(|| {}, return_unit), u.into()); diff --git a/tests/ui/unused_unit.rs b/tests/ui/unused_unit.rs index e04c52573..8fc072ebd 100644 --- a/tests/ui/unused_unit.rs +++ b/tests/ui/unused_unit.rs @@ -35,6 +35,7 @@ fn return_unit() -> () { () } #[allow(clippy::needless_return)] #[allow(clippy::never_loop)] +#[allow(clippy::unit_cmp)] fn main() { let u = Unitter; assert_eq!(u.get_unit(|| {}, return_unit), u.into()); diff --git a/tests/ui/unused_unit.stderr b/tests/ui/unused_unit.stderr index 6ef6dc4f5..c489b13bf 100644 --- a/tests/ui/unused_unit.stderr +++ b/tests/ui/unused_unit.stderr @@ -37,13 +37,13 @@ LL | fn return_unit() -> () { () } | ^^ help: remove the final `()` error: unneeded `()` - --> $DIR/unused_unit.rs:43:14 + --> $DIR/unused_unit.rs:44:14 | LL | break(); | ^^ help: remove the `()` error: unneeded `()` - --> $DIR/unused_unit.rs:45:11 + --> $DIR/unused_unit.rs:46:11 | LL | return(); | ^^ help: remove the `()`