Switch from macro_expansion to early pass

GuillaumeGomez · GuillaumeGomez · commit cbb0039935e0 · 2021-04-17T15:00:51.000+02:00
diff --git 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,53 @@ 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 is_direct_expn_of(e.span, mac).is_some() {
+                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,
+                        e.span,
+                        &format!("used `{}!` with a literal bool", mac),
+                        "replace it with",
+                        format!("{}!", non_eq_mac),
+                        Applicability::HasPlaceholders,
+                    );
                 }
-                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::<String>();
-            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
@@ -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),
@@ -1738,6 +1737,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
@@ -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,32 @@ 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<[&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 (Some(ref a), Some(ref b)) = (tup.get(0), tup.get(1));
+        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
@@ -1,30 +1,47 @@
 #![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);
     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
