diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | clippy_lints/src/bool_assert_comparison.rs | 75 | ||||
| -rw-r--r-- | clippy_lints/src/lib.rs | 5 | ||||
| -rw-r--r-- | clippy_utils/src/ast_utils.rs | 32 | ||||
| -rw-r--r-- | tests/ui/bool_assert_comparison.rs | 59 | ||||
| -rw-r--r-- | tests/ui/bool_assert_comparison.stderr | 112 |
6 files changed, 284 insertions, 0 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 56cdd356469..fc60432369d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2124,6 +2124,7 @@ Released 2018-09-13 [`blacklisted_name`]: https://rust-lang.github.io/rust-clippy/master/index.html#blacklisted_name [`blanket_clippy_restriction_lints`]: https://rust-lang.github.io/rust-clippy/master/index.html#blanket_clippy_restriction_lints [`blocks_in_if_conditions`]: https://rust-lang.github.io/rust-clippy/master/index.html#blocks_in_if_conditions +[`bool_assert_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison [`bool_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_comparison [`borrow_interior_mutable_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrow_interior_mutable_const [`borrowed_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrowed_box diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs new file mode 100644 index 00000000000..bee706ed402 --- /dev/null +++ b/clippy_lints/src/bool_assert_comparison.rs @@ -0,0 +1,75 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::{ast_utils, is_direct_expn_of}; +use rustc_ast::ast::{Expr, ExprKind, Lit, LitKind}; +use rustc_errors::Applicability; +use rustc_lint::{EarlyContext, EarlyLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// **What it does:** This lint warns about boolean comparisons in assert-like macros. + /// + /// **Why is this bad?** It is shorter to use the equivalent. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// // Bad + /// assert_eq!("a".is_empty(), false); + /// assert_ne!("a".is_empty(), true); + /// + /// // Good + /// assert!(!"a".is_empty()); + /// ``` + pub BOOL_ASSERT_COMPARISON, + style, + "Using a boolean as comparison value in an assert_* macro when there is no need" +} + +declare_lint_pass!(BoolAssertComparison => [BOOL_ASSERT_COMPARISON]); + +fn is_bool_lit(e: &Expr) -> bool { + matches!( + e.kind, + ExprKind::Lit(Lit { + kind: LitKind::Bool(_), + .. + }) + ) && !e.span.from_expansion() +} + +impl EarlyLintPass for BoolAssertComparison { + fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &Expr) { + let macros = ["assert_eq", "debug_assert_eq"]; + let inverted_macros = ["assert_ne", "debug_assert_ne"]; + + for mac in macros.iter().chain(inverted_macros.iter()) { + if let Some(span) = is_direct_expn_of(e.span, mac) { + if let Some([a, b]) = ast_utils::extract_assert_macro_args(e) { + let nb_bool_args = is_bool_lit(a) as usize + is_bool_lit(b) as usize; + + if nb_bool_args != 1 { + // If there are two boolean arguments, we definitely don't understand + // what's going on, so better leave things as is... + // + // Or there is simply no boolean and then we can leave things as is! + return; + } + + let non_eq_mac = &mac[..mac.len() - 3]; + span_lint_and_sugg( + cx, + BOOL_ASSERT_COMPARISON, + span, + &format!("used `{}!` with a literal bool", mac), + "replace it with", + format!("{}!(..)", non_eq_mac), + Applicability::MaybeIncorrect, + ); + return; + } + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a7871b7aba7..b18001d0f26 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -178,6 +178,7 @@ mod await_holding_invalid; mod bit_mask; mod blacklisted_name; mod blocks_in_if_conditions; +mod bool_assert_comparison; mod booleans; mod bytecount; mod cargo_common_metadata; @@ -568,6 +569,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: bit_mask::VERBOSE_BIT_MASK, blacklisted_name::BLACKLISTED_NAME, blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS, + bool_assert_comparison::BOOL_ASSERT_COMPARISON, booleans::LOGIC_BUG, booleans::NONMINIMAL_BOOL, bytecount::NAIVE_BYTECOUNT, @@ -1273,6 +1275,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box from_str_radix_10::FromStrRadix10); store.register_late_pass(|| box manual_map::ManualMap); store.register_late_pass(move || box if_then_some_else_none::IfThenSomeElseNone::new(msrv)); + store.register_early_pass(|| box bool_assert_comparison::BoolAssertComparison); store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ LintId::of(arithmetic::FLOAT_ARITHMETIC), @@ -1453,6 +1456,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(bit_mask::INEFFECTIVE_BIT_MASK), LintId::of(blacklisted_name::BLACKLISTED_NAME), LintId::of(blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS), + LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON), LintId::of(booleans::LOGIC_BUG), LintId::of(booleans::NONMINIMAL_BOOL), LintId::of(casts::CAST_REF_TO_MUT), @@ -1739,6 +1743,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(attrs::BLANKET_CLIPPY_RESTRICTION_LINTS), LintId::of(blacklisted_name::BLACKLISTED_NAME), LintId::of(blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS), + LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON), LintId::of(casts::FN_TO_NUMERIC_CAST), LintId::of(casts::FN_TO_NUMERIC_CAST_WITH_TRUNCATION), LintId::of(collapsible_if::COLLAPSIBLE_ELSE_IF), diff --git a/clippy_utils/src/ast_utils.rs b/clippy_utils/src/ast_utils.rs index eaea3e636f9..93e10c836cc 100644 --- a/clippy_utils/src/ast_utils.rs +++ b/clippy_utils/src/ast_utils.rs @@ -5,6 +5,7 @@ #![allow(clippy::similar_names, clippy::wildcard_imports, clippy::enum_glob_use)] use crate::{both, over}; +use if_chain::if_chain; use rustc_ast::ptr::P; use rustc_ast::{self as ast, *}; use rustc_span::symbol::Ident; @@ -571,3 +572,34 @@ pub fn eq_mac_args(l: &MacArgs, r: &MacArgs) -> bool { _ => false, } } + +/// Extract args from an assert-like macro. +/// +/// Currently working with: +/// - `assert_eq!` and `assert_ne!` +/// - `debug_assert_eq!` and `debug_assert_ne!` +/// +/// For example: +/// +/// `debug_assert_eq!(a, b)` will return Some([a, b]) +pub fn extract_assert_macro_args(mut expr: &Expr) -> Option<[&Expr; 2]> { + if_chain! { + if let ExprKind::If(_, ref block, _) = expr.kind; + if let StmtKind::Semi(ref e) = block.stmts.get(0)?.kind; + then { + expr = e; + } + } + if_chain! { + if let ExprKind::Block(ref block, _) = expr.kind; + if let StmtKind::Expr(ref expr) = block.stmts.get(0)?.kind; + if let ExprKind::Match(ref match_expr, _) = expr.kind; + if let ExprKind::Tup(ref tup) = match_expr.kind; + if let [a, b, ..] = tup.as_slice(); + if let (&ExprKind::AddrOf(_, _, ref a), &ExprKind::AddrOf(_, _, ref b)) = (&a.kind, &b.kind); + then { + return Some([&*a, &*b]); + } + } + None +} diff --git a/tests/ui/bool_assert_comparison.rs b/tests/ui/bool_assert_comparison.rs new file mode 100644 index 00000000000..2de402fae8c --- /dev/null +++ b/tests/ui/bool_assert_comparison.rs @@ -0,0 +1,59 @@ +#![warn(clippy::bool_assert_comparison)] + +macro_rules! a { + () => { + true + }; +} +macro_rules! b { + () => { + true + }; +} + +fn main() { + assert_eq!("a".len(), 1); + assert_eq!("a".is_empty(), false); + assert_eq!("".is_empty(), true); + assert_eq!(true, "".is_empty()); + assert_eq!(a!(), b!()); + assert_eq!(a!(), "".is_empty()); + assert_eq!("".is_empty(), b!()); + + assert_ne!("a".len(), 1); + assert_ne!("a".is_empty(), false); + assert_ne!("".is_empty(), true); + assert_ne!(true, "".is_empty()); + assert_ne!(a!(), b!()); + assert_ne!(a!(), "".is_empty()); + assert_ne!("".is_empty(), b!()); + + debug_assert_eq!("a".len(), 1); + debug_assert_eq!("a".is_empty(), false); + debug_assert_eq!("".is_empty(), true); + debug_assert_eq!(true, "".is_empty()); + debug_assert_eq!(a!(), b!()); + debug_assert_eq!(a!(), "".is_empty()); + debug_assert_eq!("".is_empty(), b!()); + + debug_assert_ne!("a".len(), 1); + debug_assert_ne!("a".is_empty(), false); + debug_assert_ne!("".is_empty(), true); + debug_assert_ne!(true, "".is_empty()); + debug_assert_ne!(a!(), b!()); + debug_assert_ne!(a!(), "".is_empty()); + debug_assert_ne!("".is_empty(), b!()); + + // assert with error messages + assert_eq!("a".len(), 1, "tadam {}", 1); + assert_eq!("a".len(), 1, "tadam {}", true); + assert_eq!("a".is_empty(), false, "tadam {}", 1); + assert_eq!("a".is_empty(), false, "tadam {}", true); + assert_eq!(false, "a".is_empty(), "tadam {}", true); + + debug_assert_eq!("a".len(), 1, "tadam {}", 1); + debug_assert_eq!("a".len(), 1, "tadam {}", true); + debug_assert_eq!("a".is_empty(), false, "tadam {}", 1); + debug_assert_eq!("a".is_empty(), false, "tadam {}", true); + debug_assert_eq!(false, "a".is_empty(), "tadam {}", true); +} diff --git a/tests/ui/bool_assert_comparison.stderr b/tests/ui/bool_assert_comparison.stderr new file mode 100644 index 00000000000..f57acf520d5 --- /dev/null +++ b/tests/ui/bool_assert_comparison.stderr @@ -0,0 +1,112 @@ +error: used `assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:16:5 + | +LL | assert_eq!("a".is_empty(), false); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + | + = note: `-D clippy::bool-assert-comparison` implied by `-D warnings` + +error: used `assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:17:5 + | +LL | assert_eq!("".is_empty(), true); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + +error: used `assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:18:5 + | +LL | assert_eq!(true, "".is_empty()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + +error: used `assert_ne!` with a literal bool + --> $DIR/bool_assert_comparison.rs:24:5 + | +LL | assert_ne!("a".is_empty(), false); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + +error: used `assert_ne!` with a literal bool + --> $DIR/bool_assert_comparison.rs:25:5 + | +LL | assert_ne!("".is_empty(), true); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + +error: used `assert_ne!` with a literal bool + --> $DIR/bool_assert_comparison.rs:26:5 + | +LL | assert_ne!(true, "".is_empty()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + +error: used `debug_assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:32:5 + | +LL | debug_assert_eq!("a".is_empty(), false); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + +error: used `debug_assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:33:5 + | +LL | debug_assert_eq!("".is_empty(), true); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + +error: used `debug_assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:34:5 + | +LL | debug_assert_eq!(true, "".is_empty()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + +error: used `debug_assert_ne!` with a literal bool + --> $DIR/bool_assert_comparison.rs:40:5 + | +LL | debug_assert_ne!("a".is_empty(), false); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + +error: used `debug_assert_ne!` with a literal bool + --> $DIR/bool_assert_comparison.rs:41:5 + | +LL | debug_assert_ne!("".is_empty(), true); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + +error: used `debug_assert_ne!` with a literal bool + --> $DIR/bool_assert_comparison.rs:42:5 + | +LL | debug_assert_ne!(true, "".is_empty()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + +error: used `assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:50:5 + | +LL | assert_eq!("a".is_empty(), false, "tadam {}", 1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + +error: used `assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:51:5 + | +LL | assert_eq!("a".is_empty(), false, "tadam {}", true); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + +error: used `assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:52:5 + | +LL | assert_eq!(false, "a".is_empty(), "tadam {}", true); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + +error: used `debug_assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:56:5 + | +LL | debug_assert_eq!("a".is_empty(), false, "tadam {}", 1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + +error: used `debug_assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:57:5 + | +LL | debug_assert_eq!("a".is_empty(), false, "tadam {}", true); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + +error: used `debug_assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:58:5 + | +LL | debug_assert_eq!(false, "a".is_empty(), "tadam {}", true); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + +error: aborting due to 18 previous errors + |