From 6b5312bfb63818b4f8c224811eeae5fa50466fb4 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 15 Apr 2021 23:38:55 +0200 Subject: [PATCH] Switch from macro_expansion to early pass --- clippy_lints/src/bool_assert_comparison.rs | 124 ++++++--------------- clippy_lints/src/lib.rs | 2 +- clippy_utils/src/ast_utils.rs | 34 ++++++ tests/ui/bool_assert_comparison.stderr | 20 ++-- 4 files changed, 81 insertions(+), 99 deletions(-) diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs index 810e69d79bdb..8105a48ea45b 100644 --- a/clippy_lints/src/bool_assert_comparison.rs +++ b/clippy_lints/src/bool_assert_comparison.rs @@ -1,12 +1,10 @@ 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}; +use rustc_span::DUMMY_SP; declare_clippy_lint! { /// **What it does:** This lint warns about boolean comparisons in assert-like macros. @@ -32,96 +30,46 @@ declare_clippy_lint! { declare_lint_pass!(BoolAssertComparison => [BOOL_ASSERT_COMPARISON]); -fn is_bool(tt: Option<&TokenTree>) -> bool { - match tt { - Some(TokenTree::Token(Token { - kind: TokenKind::Ident(i, _), - .. - })) => *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 is_direct_expn_of(e.span, mac).is_some() { + if let Some(macro_args) = ast_utils::extract_assert_macro_args(e) { + let mut nb_bool_args = 0; + let mut bool_span = DUMMY_SP; + for arg in macro_args { + if let ExprKind::Lit(Lit { + kind: LitKind::Bool(_), .. + }) = arg.kind + { + nb_bool_args += 1; + bool_span = arg.span; + } } - } - 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(' '); + #[allow(clippy::if_same_then_else)] // It allows better code reability here. + if nb_bool_args > 1 { + // Don't know what's going on here, but better leave things as is... + return; + } else if nb_bool_args == 0 { + // No problem detected! + return; } - 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, - ); + span_lint_and_sugg( + cx, + BOOL_ASSERT_COMPARISON, + bool_span, + "assert macro with a boolean comparison", + "replace it with", + format!("{}!", &mac[..mac.len() - 3]), + Applicability::MachineApplicable, + ); + } + break; + } } } } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 549139052741..484fba6f85c3 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), diff --git a/clippy_utils/src/ast_utils.rs b/clippy_utils/src/ast_utils.rs index eaea3e636f9c..bd51f1e0d5da 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,36 @@ 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<'tcx>(e: &'tcx Expr) -> Option>> { + fn get_args(e: &'tcx Expr) -> Option>> { + if_chain! { + if let ExprKind::Block(ref block, _) = e.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 { + return Some(tup.iter().filter_map(|t| match t.kind { + ExprKind::AddrOf(_, _, ref expr) => Some(expr), + _ => None, + }).collect::>()); + } + } + None + } + if_chain! { + if let ExprKind::If(_, ref block, _) = e.kind; + if let StmtKind::Semi(ref e) = block.stmts.get(0)?.kind; + then { + return get_args(e); + } + } + get_args(e) +} diff --git a/tests/ui/bool_assert_comparison.stderr b/tests/ui/bool_assert_comparison.stderr index 70cea32d1bfc..1ce8c42aacb6 100644 --- a/tests/ui/bool_assert_comparison.stderr +++ b/tests/ui/bool_assert_comparison.stderr @@ -2,7 +2,7 @@ error: assert macro with a boolean comparison --> $DIR/bool_assert_comparison.rs:5:32 | 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` @@ -10,7 +10,7 @@ error: assert macro with a boolean comparison --> $DIR/bool_assert_comparison.rs:6:31 | 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 @@ -22,13 +22,13 @@ error: assert macro with a boolean comparison --> $DIR/bool_assert_comparison.rs:10:32 | 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 | 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 @@ -40,13 +40,13 @@ error: assert macro with a boolean comparison --> $DIR/bool_assert_comparison.rs:15:38 | 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 | 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 @@ -58,13 +58,13 @@ error: assert macro with a boolean comparison --> $DIR/bool_assert_comparison.rs:20:38 | 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 | 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 @@ -76,13 +76,13 @@ error: assert macro with a boolean comparison --> $DIR/bool_assert_comparison.rs:27:32 | 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 | 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