From 3557084b0137f04c285197c2253e097c772cf047 Mon Sep 17 00:00:00 2001 From: Matthias Seiffert Date: Wed, 2 Oct 2019 22:48:19 +0200 Subject: [PATCH 1/6] Add check for assert_eq macros to unit_cmp lint --- clippy_lints/src/types.rs | 26 ++++++++++++++++++++++++++ tests/ui/unit_cmp.rs | 6 ++++++ tests/ui/unit_cmp.stderr | 34 +++++++++++++++++++++++++++++++++- tests/ui/unused_unit.fixed | 1 + tests/ui/unused_unit.rs | 1 + tests/ui/unused_unit.stderr | 4 ++-- 6 files changed, 69 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index e976b055791d..b235b3eab2ae 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}; @@ -527,6 +529,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 48c22f7f875a..71c4348a2a18 100644 --- a/tests/ui/unit_cmp.rs +++ b/tests/ui/unit_cmp.rs @@ -20,4 +20,10 @@ fn main() { } > { false; } {} + + assert_eq!((), ()); + debug_assert_eq!((), ()); + + assert_ne!((), ()); + debug_assert_ne!((), ()); } diff --git a/tests/ui/unit_cmp.stderr b/tests/ui/unit_cmp.stderr index 56293403043d..e2bab3eab60b 100644 --- a/tests/ui/unit_cmp.stderr +++ b/tests/ui/unit_cmp.stderr @@ -22,5 +22,37 @@ 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!((), ()); + | ^^^^^^^^^^^^^^^^^^^ + | + = 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:25:5 + | +LL | debug_assert_eq!((), ()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = 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:27:5 + | +LL | assert_ne!((), ()); + | ^^^^^^^^^^^^^^^^^^^ + | + = 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:28:5 + | +LL | debug_assert_ne!((), ()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = 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 17c1a5de5973..3f63624720f7 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 e04c52573375..8fc072ebd69f 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 6ef6dc4f5d6c..c489b13bf27b 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 `()` From 288f02da4439602d831c62082728ced10fdfe5bd Mon Sep 17 00:00:00 2001 From: Matthias Seiffert Date: Thu, 3 Oct 2019 11:43:39 +0200 Subject: [PATCH 2/6] Print assert macro name in backticks Co-Authored-By: Philipp Krones --- clippy_lints/src/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index b235b3eab2ae..a42a50e553a5 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -544,7 +544,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnitCmp { UNIT_CMP, expr.span, &format!( - "{} of unit values detected. This will always {}", + "`{}` of unit values detected. This will always {}", symbol.as_str(), result ), From 320d17aa63f1516041beb9d42588dfd0bde90136 Mon Sep 17 00:00:00 2001 From: Matthias Seiffert Date: Thu, 3 Oct 2019 12:01:02 +0200 Subject: [PATCH 3/6] Update the .stderr to include the backticks --- tests/ui/unit_cmp.stderr | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/ui/unit_cmp.stderr b/tests/ui/unit_cmp.stderr index e2bab3eab60b..eeeef05d0021 100644 --- a/tests/ui/unit_cmp.stderr +++ b/tests/ui/unit_cmp.stderr @@ -22,7 +22,7 @@ LL | | false; LL | | } {} | |_____^ -error: assert_eq of unit values detected. This will always succeed +error: `assert_eq` of unit values detected. This will always succeed --> $DIR/unit_cmp.rs:24:5 | LL | assert_eq!((), ()); @@ -30,7 +30,7 @@ LL | assert_eq!((), ()); | = 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 +error: `debug_assert_eq` of unit values detected. This will always succeed --> $DIR/unit_cmp.rs:25:5 | LL | debug_assert_eq!((), ()); @@ -38,7 +38,7 @@ LL | debug_assert_eq!((), ()); | = 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 +error: `assert_ne` of unit values detected. This will always fail --> $DIR/unit_cmp.rs:27:5 | LL | assert_ne!((), ()); @@ -46,7 +46,7 @@ LL | assert_ne!((), ()); | = 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 +error: `debug_assert_ne` of unit values detected. This will always fail --> $DIR/unit_cmp.rs:28:5 | LL | debug_assert_ne!((), ()); From fb25d5679949e26621f43cfbbd7f5864a60e2bd9 Mon Sep 17 00:00:00 2001 From: Matthias Seiffert Date: Thu, 3 Oct 2019 14:35:05 +0200 Subject: [PATCH 4/6] Mention asserts in doc for unit_cmp lint --- clippy_lints/src/types.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index a42a50e553a5..2b3c02ce74e1 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -487,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 @@ -519,6 +520,20 @@ declare_clippy_lint! { /// baz(); /// } /// ``` + /// + /// For asserts: + /// ```rust + /// # fn foo() {}; + /// # fn bar() {}; + /// assert_eq!({ foo(); }, { bar(); }); + /// ``` + /// will always succeed + /// ```rust + /// # fn foo() {}; + /// # fn bar() {}; + /// assert_ne!({ foo(); }, { bar(); }); + /// ``` + /// will always fail pub UNIT_CMP, correctness, "comparing unit values" From 024dfee33c89297ef862fefdf8e1db78574c5b5c Mon Sep 17 00:00:00 2001 From: Matthias Seiffert Date: Thu, 3 Oct 2019 14:38:04 +0200 Subject: [PATCH 5/6] Update unit_cmp tests to include blocks for asserts --- tests/ui/unit_cmp.rs | 36 +++++++++++++++++++++++++++---- tests/ui/unit_cmp.stderr | 46 ++++++++++++++++++++++++++++++---------- 2 files changed, 67 insertions(+), 15 deletions(-) diff --git a/tests/ui/unit_cmp.rs b/tests/ui/unit_cmp.rs index 71c4348a2a18..8d3a4eed82e3 100644 --- a/tests/ui/unit_cmp.rs +++ b/tests/ui/unit_cmp.rs @@ -21,9 +21,37 @@ fn main() { false; } {} - assert_eq!((), ()); - debug_assert_eq!((), ()); + assert_eq!( + { + true; + }, + { + false; + } + ); + debug_assert_eq!( + { + true; + }, + { + false; + } + ); - assert_ne!((), ()); - debug_assert_ne!((), ()); + assert_ne!( + { + true; + }, + { + false; + } + ); + debug_assert_ne!( + { + true; + }, + { + false; + } + ); } diff --git a/tests/ui/unit_cmp.stderr b/tests/ui/unit_cmp.stderr index eeeef05d0021..578a6218ee03 100644 --- a/tests/ui/unit_cmp.stderr +++ b/tests/ui/unit_cmp.stderr @@ -25,32 +25,56 @@ LL | | } {} error: `assert_eq` of unit values detected. This will always succeed --> $DIR/unit_cmp.rs:24:5 | -LL | assert_eq!((), ()); - | ^^^^^^^^^^^^^^^^^^^ +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:25:5 + --> $DIR/unit_cmp.rs:32:5 | -LL | debug_assert_eq!((), ()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^ +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:27:5 + --> $DIR/unit_cmp.rs:41:5 | -LL | assert_ne!((), ()); - | ^^^^^^^^^^^^^^^^^^^ +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:28:5 + --> $DIR/unit_cmp.rs:49:5 | -LL | debug_assert_ne!((), ()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^ +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) From 5a0a2b383c71931bcb4a0431ead16e075c057b0a Mon Sep 17 00:00:00 2001 From: Matthias Seiffert Date: Thu, 3 Oct 2019 19:53:41 +0200 Subject: [PATCH 6/6] Remove assert_ne example from doc --- clippy_lints/src/types.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 2b3c02ce74e1..2e960952e690 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -528,12 +528,6 @@ declare_clippy_lint! { /// assert_eq!({ foo(); }, { bar(); }); /// ``` /// will always succeed - /// ```rust - /// # fn foo() {}; - /// # fn bar() {}; - /// assert_ne!({ foo(); }, { bar(); }); - /// ``` - /// will always fail pub UNIT_CMP, correctness, "comparing unit values"