diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs index 810e69d79bdb..e72ea76a930b 100644 --- a/clippy_lints/src/bool_assert_comparison.rs +++ b/clippy_lints/src/bool_assert_comparison.rs @@ -1,12 +1,9 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::snippet; -use rustc_ast::ast::{MacArgs, MacCall}; -use rustc_ast::token::{Token, TokenKind}; -use rustc_ast::tokenstream::TokenTree; +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}; -use rustc_span::symbol::kw::{False, True}; declare_clippy_lint! { /// **What it does:** This lint warns about boolean comparisons in assert-like macros. @@ -26,102 +23,56 @@ declare_clippy_lint! { /// assert!(!"a".is_empty()); /// ``` pub BOOL_ASSERT_COMPARISON, - pedantic, - "Using a boolean as comparison value when there is no need" + 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(tt: Option<&TokenTree>) -> bool { - match tt { - Some(TokenTree::Token(Token { - kind: TokenKind::Ident(i, _), +fn is_bool_lit(e: &Expr) -> bool { + matches!( + e.kind, + ExprKind::Lit(Lit { + kind: LitKind::Bool(_), .. - })) => *i == True || *i == False, - _ => false, - } + }) + ) } impl EarlyLintPass for BoolAssertComparison { - fn check_mac(&mut self, cx: &EarlyContext<'_>, mac: &MacCall) { + fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &Expr) { let macros = ["assert_eq", "debug_assert_eq"]; let inverted_macros = ["assert_ne", "debug_assert_ne"]; - if let MacArgs::Delimited(_, _, ts) = &*mac.args { - let name; - if let [seg] = &*mac.path.segments { - name = seg.ident.name.as_str().to_string(); - if !macros.contains(&name.as_str()) && !inverted_macros.contains(&name.as_str()) { - return; - } - } else { - return; - } - let mut has_true = false; - let mut has_false = false; - let mut bool_span = None; - let mut spans = Vec::new(); - let mut nb_comma = 0; - let mut iter = ts.trees().peekable(); - while let Some(tt) = iter.next() { - if let TokenTree::Token(ref token) = tt { - match token.kind { - TokenKind::Ident(i, _) => { - // We only want to check the comparison arguments, nothing else! - if nb_comma < 2 { - if i == True { - has_true = true; - bool_span = Some(token.span); - continue; - } else if i == False { - has_false = true; - bool_span = Some(token.span); - continue; - } - } - }, - TokenKind::Comma => { - nb_comma += 1; - if nb_comma > 1 || !is_bool(iter.peek()) { - spans.push((token.span, true)); - } - continue; - }, - _ => {}, + for mac in macros.iter().chain(inverted_macros.iter()) { + if let Some(span) = is_direct_expn_of(e.span, mac) { + if let Some(args) = ast_utils::extract_assert_macro_args(e) { + // We are only interested in case there are two arguments. + if let [a, b] = args.as_slice() { + 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; } - spans.push((tt.span(), false)); - } - #[allow(clippy::if_same_then_else)] // It allows better code reability here. - if has_false && has_true { - // Don't know what's going on here, but better leave things as is... - return; - } else if !has_true && !has_false { - // No problem detected! - return; } - let text = spans - .into_iter() - .map(|(s, needs_whitespace)| { - let mut s = snippet(cx, s, "arg").to_string(); - if needs_whitespace { - s.push(' '); - } - s - }) - .collect::(); - let is_cond_inverted = inverted_macros.contains(&name.as_str()); - let extra = (has_true && is_cond_inverted) || has_false; - - span_lint_and_sugg( - cx, - BOOL_ASSERT_COMPARISON, - bool_span.expect("failed to get the bool span"), - "assert macro with a boolean comparison", - "replace it with", - format!("{}!({}{})", &name[..name.len() - 3], if extra { "!" } else { "" }, text), - Applicability::MachineApplicable, - ); } } } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 549139052741..f08388f90371 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -394,7 +394,6 @@ pub fn register_pre_expansion_lints(store: &mut rustc_lint::LintStore) { store.register_pre_expansion_pass(|| box write::Write::default()); store.register_pre_expansion_pass(|| box attrs::EarlyAttributes); store.register_pre_expansion_pass(|| box dbg_macro::DbgMacro); - store.register_pre_expansion_pass(|| box bool_assert_comparison::BoolAssertComparison); } #[doc(hidden)] @@ -1273,6 +1272,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), @@ -1335,7 +1335,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(await_holding_invalid::AWAIT_HOLDING_LOCK), LintId::of(await_holding_invalid::AWAIT_HOLDING_REFCELL_REF), LintId::of(bit_mask::VERBOSE_BIT_MASK), - LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON), LintId::of(bytecount::NAIVE_BYTECOUNT), LintId::of(case_sensitive_file_extension_comparisons::CASE_SENSITIVE_FILE_EXTENSION_COMPARISONS), LintId::of(casts::CAST_LOSSLESS), @@ -1452,6 +1451,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), @@ -1738,6 +1738,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 eaea3e636f9c..b095d20f537e 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,39 @@ pub fn eq_mac_args(l: &MacArgs, r: &MacArgs) -> bool { _ => false, } } + +/// Extract args from an assert-like macro. +/// Currently working with: +/// - `assert!`, `assert_eq!` and `assert_ne!` +/// - `debug_assert!`, `debug_assert_eq!` and `debug_assert_ne!` +/// For example: +/// `assert!(expr)` will return Some([expr]) +/// `debug_assert_eq!(a, b)` will return Some([a, b]) +pub fn extract_assert_macro_args(mut expr: &Expr) -> Option> { + 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; + then { + if let [a, b, ..] = tup.as_slice() { + if let (&ExprKind::AddrOf(_, _, ref a), &ExprKind::AddrOf(_, _, ref b)) = (&a.kind, &b.kind) { + return Some(vec![&*a, &*b]); + } + } + if let [a, ..] = tup.as_slice() { + if let ExprKind::AddrOf(_, _, ref a) = a.kind { + return Some(vec![&*a]); + } + } + } + } + None +} diff --git a/tests/ui/bool_assert_comparison.rs b/tests/ui/bool_assert_comparison.rs index fd0fcef3f25a..0c12d9785356 100644 --- a/tests/ui/bool_assert_comparison.rs +++ b/tests/ui/bool_assert_comparison.rs @@ -1,25 +1,40 @@ #![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_ne!("a".len(), 1); assert_ne!("a".is_empty(), false); assert_ne!("".is_empty(), true); assert_ne!(true, "".is_empty()); + assert_ne!(a!(), 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_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!()); // assert with error messages assert_eq!("a".len(), 1, "tadam {}", 1); @@ -27,4 +42,10 @@ fn main() { 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 index 70cea32d1bfc..3ed429c34dfe 100644 --- a/tests/ui/bool_assert_comparison.stderr +++ b/tests/ui/bool_assert_comparison.stderr @@ -1,94 +1,112 @@ -error: assert macro with a boolean comparison - --> $DIR/bool_assert_comparison.rs:5:32 +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!(!"a".is_empty())` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` | = note: `-D clippy::bool-assert-comparison` implied by `-D warnings` -error: assert macro with a boolean comparison - --> $DIR/bool_assert_comparison.rs:6:31 +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!("".is_empty())` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` -error: assert macro with a boolean comparison - --> $DIR/bool_assert_comparison.rs:7:16 +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!` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` -error: assert macro with a boolean comparison - --> $DIR/bool_assert_comparison.rs:10:32 +error: used `assert_ne!` with a literal bool + --> $DIR/bool_assert_comparison.rs:22:5 | LL | assert_ne!("a".is_empty(), false); - | ^^^^^ help: replace it with: `assert!(!"a".is_empty())` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` -error: assert macro with a boolean comparison - --> $DIR/bool_assert_comparison.rs:11:31 +error: used `assert_ne!` with a literal bool + --> $DIR/bool_assert_comparison.rs:23:5 | LL | assert_ne!("".is_empty(), true); - | ^^^^ help: replace it with: `assert!(!"".is_empty())` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` -error: assert macro with a boolean comparison - --> $DIR/bool_assert_comparison.rs:12:16 +error: used `assert_ne!` with a literal bool + --> $DIR/bool_assert_comparison.rs:24:5 | LL | assert_ne!(true, "".is_empty()); - | ^^^^ help: replace it with: `assert!` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` -error: assert macro with a boolean comparison - --> $DIR/bool_assert_comparison.rs:15:38 +error: used `debug_assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:28:5 | LL | debug_assert_eq!("a".is_empty(), false); - | ^^^^^ help: replace it with: `debug_assert!(!"a".is_empty())` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` -error: assert macro with a boolean comparison - --> $DIR/bool_assert_comparison.rs:16:37 +error: used `debug_assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:29:5 | LL | debug_assert_eq!("".is_empty(), true); - | ^^^^ help: replace it with: `debug_assert!("".is_empty())` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` -error: assert macro with a boolean comparison - --> $DIR/bool_assert_comparison.rs:17:22 +error: used `debug_assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:30:5 | LL | debug_assert_eq!(true, "".is_empty()); - | ^^^^ help: replace it with: `debug_assert!` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` -error: assert macro with a boolean comparison - --> $DIR/bool_assert_comparison.rs:20:38 +error: used `debug_assert_ne!` with a literal bool + --> $DIR/bool_assert_comparison.rs:34:5 | LL | debug_assert_ne!("a".is_empty(), false); - | ^^^^^ help: replace it with: `debug_assert!(!"a".is_empty())` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` -error: assert macro with a boolean comparison - --> $DIR/bool_assert_comparison.rs:21:37 +error: used `debug_assert_ne!` with a literal bool + --> $DIR/bool_assert_comparison.rs:35:5 | LL | debug_assert_ne!("".is_empty(), true); - | ^^^^ help: replace it with: `debug_assert!(!"".is_empty())` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` -error: assert macro with a boolean comparison - --> $DIR/bool_assert_comparison.rs:22:22 +error: used `debug_assert_ne!` with a literal bool + --> $DIR/bool_assert_comparison.rs:36:5 | LL | debug_assert_ne!(true, "".is_empty()); - | ^^^^ help: replace it with: `debug_assert!` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` -error: assert macro with a boolean comparison - --> $DIR/bool_assert_comparison.rs:27:32 +error: used `assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:42:5 | LL | assert_eq!("a".is_empty(), false, "tadam {}", 1); - | ^^^^^ help: replace it with: `assert!(!"a".is_empty(), "tadam {}", 1)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` -error: assert macro with a boolean comparison - --> $DIR/bool_assert_comparison.rs:28:32 +error: used `assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:43:5 | LL | assert_eq!("a".is_empty(), false, "tadam {}", true); - | ^^^^^ help: replace it with: `assert!(!"a".is_empty(), "tadam {}", true)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` -error: assert macro with a boolean comparison - --> $DIR/bool_assert_comparison.rs:29:16 +error: used `assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:44:5 | LL | assert_eq!(false, "a".is_empty(), "tadam {}", true); - | ^^^^^ help: replace it with: `assert!` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` -error: aborting due to 15 previous errors +error: used `debug_assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:48: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:49: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:50:5 + | +LL | debug_assert_eq!(false, "a".is_empty(), "tadam {}", true); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + +error: aborting due to 18 previous errors