change to a structure for parsing assert macro

ABouttefeux · ABouttefeux · commit cd7027ce3af8 · 2021-09-22T15:36:07.000+02:00
diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs
@@ -1,4 +1,6 @@
-use clippy_utils::{diagnostics::span_lint_and_sugg, higher, is_direct_expn_of, sugg::Sugg, ty::implements_trait};
+use clippy_utils::{
+    diagnostics::span_lint_and_sugg, higher::AssertExpn, is_direct_expn_of, sugg::Sugg, ty::implements_trait,
+};
 use rustc_ast::ast::LitKind;
 use rustc_errors::Applicability;
 use rustc_hir::{Expr, ExprKind, Lit};
@@ -79,7 +81,7 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
             .chain(inverted_macros.iter().map(|el| (el, false)))
         {
             if let Some(span) = is_direct_expn_of(expr.span, mac) {
-                if let Some(args) = higher::extract_assert_macro_args(expr) {
+                if let Some(args) = AssertExpn::parse(expr).map(|v| v.argument_vector()) {
                     if let [a, b, ref fmt_args @ ..] = args[..] {
                         let (lit_value, other_expr) = match (bool_lit(a), bool_lit(b)) {
                             (Some(lit), None) => (lit, b),
@@ -101,7 +103,7 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
                         }
 
                         let non_eq_mac = &mac[..mac.len() - 3];
-                        let mut applicability = Applicability::MaybeIncorrect;
+                        let mut applicability = Applicability::MachineApplicable;
                         let sugg = Sugg::hir_with_applicability(cx, other_expr, "..", &mut applicability);
                         let expr_string = if lit_value ^ is_eq {
                             format!("!{}", sugg.maybe_par())
@@ -130,9 +132,9 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
                             },
                         };
                         let suggestion = if let Some(spans) = arg_span {
-                            format!("{}!({}, {})", non_eq_mac, expr_string, spans)
+                            format!("{}!({}, {});", non_eq_mac, expr_string, spans)
                         } else {
-                            format!("{}!({})", non_eq_mac, expr_string)
+                            format!("{}!({});", non_eq_mac, expr_string)
                         };
                         span_lint_and_sugg(
                             cx,
@@ -141,7 +143,7 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
                             &format!("used `{}!` with a literal bool", mac),
                             "replace it with",
                             suggestion,
-                            Applicability::MaybeIncorrect,
+                            applicability,
                         );
                         return;
                     }
diff --git a/clippy_lints/src/eq_op.rs b/clippy_lints/src/eq_op.rs
@@ -1,7 +1,7 @@
 use clippy_utils::diagnostics::{multispan_sugg, span_lint, span_lint_and_then};
 use clippy_utils::source::snippet;
 use clippy_utils::ty::{implements_trait, is_copy};
-use clippy_utils::{ast_utils::is_useless_with_eq_exprs, eq_expr_value, higher, in_macro, is_expn_of};
+use clippy_utils::{ast_utils::is_useless_with_eq_exprs, eq_expr_value, higher::AssertExpn, in_macro, is_expn_of};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, StmtKind};
@@ -77,7 +77,7 @@ impl<'tcx> LateLintPass<'tcx> for EqOp {
                     if_chain! {
                         if is_expn_of(stmt.span, amn).is_some();
                         if let StmtKind::Semi(matchexpr) = stmt.kind;
-                        if let Some(macro_args) = higher::extract_assert_macro_args(matchexpr);
+                        if let Some(macro_args) = AssertExpn::parse(matchexpr).map(|v| v.argument_vector());
                         if macro_args.len() == 2;
                         let (lhs, rhs) = (macro_args[0], macro_args[1]);
                         if eq_expr_value(cx, lhs, rhs);
diff --git a/clippy_lints/src/mutable_debug_assertion.rs b/clippy_lints/src/mutable_debug_assertion.rs
@@ -1,5 +1,5 @@
 use clippy_utils::diagnostics::span_lint;
-use clippy_utils::{higher, is_direct_expn_of};
+use clippy_utils::{higher::AssertExpn, is_direct_expn_of};
 use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
 use rustc_hir::{BorrowKind, Expr, ExprKind, MatchSource, Mutability};
 use rustc_lint::{LateContext, LateLintPass};
@@ -39,7 +39,7 @@ impl<'tcx> LateLintPass<'tcx> for DebugAssertWithMutCall {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
         for dmn in &DEBUG_MACRO_NAMES {
             if is_direct_expn_of(e.span, dmn).is_some() {
-                if let Some(macro_args) = higher::extract_assert_macro_args(e) {
+                if let Some(macro_args) = AssertExpn::parse(e).map(|v| v.argument_vector()) {
                     for arg in macro_args {
                         let mut visitor = MutArgVisitor::new(cx);
                         visitor.visit_expr(arg);
diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs
@@ -416,24 +416,73 @@ pub fn binop(op: hir::BinOpKind) -> ast::BinOpKind {
     }
 }
 
-/// 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<'tcx>) -> Option<Vec<&'tcx Expr<'tcx>>> {
+/// A parsed
+/// assert!`, `assert_eq!` or `assert_ne!`,
+/// debug_assert!`, `debug_assert_eq!` or `debug_assert_ne!`
+/// macro.
+pub struct AssertExpn<'tcx> {
+    /// the first agrument of the assret e.g. `var` in element `assert!(var, ...)`
+    pub first_assert_argument: &'tcx Expr<'tcx>,
+    /// second argument of the asset for case `assert_eq!`,
+    /// `assert_ne!` etc ... Eg var_2 in `debug_assert_eq!(x, var_2,..)`
+    pub second_assert_argument: Option<&'tcx Expr<'tcx>>,
+    /// The format argument passed at the end of the macro
+    pub format_arg: Option<FormatArgsExpn<'tcx>>,
+}
+
+impl<'tcx> AssertExpn<'tcx> {
+    /// 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(AssertExpn { first_assert_argument: expr,
+    /// second_assert_argument: None, format_arg:None })` `debug_assert_eq!(a, b)` will return
+    /// `Some(AssertExpn { first_assert_argument: a, second_assert_argument: Some(b),
+    /// format_arg:None })`
+    pub fn parse(e: &'tcx Expr<'tcx>) -> Option<Self> {
+        if let ExprKind::Block(block, _) = e.kind {
+            if block.stmts.len() == 1 {
+                if let StmtKind::Semi(matchexpr) = block.stmts.get(0)?.kind {
+                    // macros with unique arg: `{debug_}assert!` (e.g., `debug_assert!(some_condition)`)
+                    if_chain! {
+                        if let Some(If { cond, .. }) = If::hir(matchexpr);
+                        if let ExprKind::Unary(UnOp::Not, condition) = cond.kind;
+                        then {
+                            return Some(Self {
+                                first_assert_argument: condition,
+                                second_assert_argument: None,
+                                format_arg: None, // FIXME actually parse the aguments
+                            });
+                        }
+                    }
+
+                    // debug macros with two args: `debug_assert_{ne, eq}` (e.g., `assert_ne!(a, b)`)
+                    if_chain! {
+                        if let ExprKind::Block(matchblock,_) = matchexpr.kind;
+                        if let Some(matchblock_expr) = matchblock.expr;
+                        then {
+                            return Self::ast_matchblock(matchblock_expr);
+                        }
+                    }
+                }
+            } else if let Some(matchblock_expr) = block.expr {
+                // macros with two args: `assert_{ne, eq}` (e.g., `assert_ne!(a, b)`)
+                return Self::ast_matchblock(matchblock_expr);
+            }
+        }
+        None
+    }
+
     /// Try to match the AST for a pattern that contains a match, for example when two args are
     /// compared
-    fn ast_matchblock(matchblock_expr: &'tcx Expr<'tcx>) -> Option<Vec<&Expr<'_>>> {
+    fn ast_matchblock(matchblock_expr: &'tcx Expr<'tcx>) -> Option<Self> {
         if_chain! {
             if let ExprKind::Match(headerexpr, arms, _) = &matchblock_expr.kind;
             if let ExprKind::Tup([lhs, rhs]) = &headerexpr.kind;
             if let ExprKind::AddrOf(BorrowKind::Ref, _, lhs) = lhs.kind;
             if let ExprKind::AddrOf(BorrowKind::Ref, _, rhs) = rhs.kind;
             then {
-                let mut vec_arg = vec![lhs, rhs];
                 if_chain! {
                     if !arms.is_empty();
                     if let ExprKind::Block(Block{expr: Some(if_expr),..},_) = arms[0].body.kind;
@@ -444,47 +493,43 @@ pub fn extract_assert_macro_args<'tcx>(e: &'tcx Expr<'tcx>) -> Option<Vec<&'tcx
                         | StmtKind::Semi(call_assert_failed) = stmts_if_block[1].kind;
                     if let ExprKind::Call(_, args_assert_failed) = call_assert_failed.kind;
                     if args_assert_failed.len() >= 4;
-                    if let ExprKind::Call(_, args) =  args_assert_failed[3].kind;
-                    if !args.is_empty();
-                    if let Some (mut format_arg_expn) = FormatArgsExpn::parse(&args[0]);
+                    if let ExprKind::Call(_, [arg, ..]) =  args_assert_failed[3].kind;
+                    if let Some(format_arg_expn) = FormatArgsExpn::parse(&arg);
                     then {
-                        vec_arg.push(format_arg_expn.format_string);
-                        vec_arg.append(&mut format_arg_expn.value_args);
+                        return Some(AssertExpn {
+                            first_assert_argument: lhs,
+                            second_assert_argument: Some(rhs),
+                            format_arg: Some(format_arg_expn)
+                        });
+                    }
+                    else {
+                        return Some(AssertExpn {
+                            first_assert_argument: lhs,
+                            second_assert_argument:
+                            Some(rhs),
+                            format_arg: None
+                        });
                     }
                 }
-                return Some(vec_arg);
             }
         }
         None
     }
 
-    if let ExprKind::Block(block, _) = e.kind {
-        if block.stmts.len() == 1 {
-            if let StmtKind::Semi(matchexpr) = block.stmts.get(0)?.kind {
-                // macros with unique arg: `{debug_}assert!` (e.g., `debug_assert!(some_condition)`)
-                if_chain! {
-                    if let Some(If { cond, .. }) = If::hir(matchexpr);
-                    if let ExprKind::Unary(UnOp::Not, condition) = cond.kind;
-                    then {
-                        return Some(vec![condition]);
-                    }
-                }
-
-                // debug macros with two args: `debug_assert_{ne, eq}` (e.g., `assert_ne!(a, b)`)
-                if_chain! {
-                    if let ExprKind::Block(matchblock,_) = matchexpr.kind;
-                    if let Some(matchblock_expr) = matchblock.expr;
-                    then {
-                        return ast_matchblock(matchblock_expr);
-                    }
-                }
+    /// Gives the argument as a vector
+    pub fn argument_vector(&self) -> Vec<&'tcx Expr<'tcx>> {
+        let mut expr_vec = vec![self.first_assert_argument];
+        if let Some(sec_agr) = self.second_assert_argument {
+            expr_vec.push(sec_agr);
+        }
+        if let Some(ref format_arg) = self.format_arg {
+            expr_vec.push(format_arg.format_string);
+            for arg in &format_arg.value_args {
+                expr_vec.push(arg)
             }
-        } else if let Some(matchblock_expr) = block.expr {
-            // macros with two args: `assert_{ne, eq}` (e.g., `assert_ne!(a, b)`)
-            return ast_matchblock(matchblock_expr);
         }
+        expr_vec
     }
-    None
 }
 
 /// A parsed `format!` expansion
diff --git a/tests/ui/bool_assert_comparison.fixed b/tests/ui/bool_assert_comparison.fixed
@@ -0,0 +1,138 @@
+// run-rustfix
+
+#![warn(clippy::bool_assert_comparison)]
+
+use std::ops::Not;
+
+macro_rules! a {
+    () => {
+        true
+    };
+}
+macro_rules! b {
+    () => {
+        true
+    };
+}
+
+// Implements the Not trait but with an output type
+// that's not bool. Should not suggest a rewrite
+#[derive(Debug, Copy, Clone)]
+#[allow(dead_code)]
+enum ImplNotTraitWithoutBool {
+    VariantX(bool),
+    VariantY(u32),
+}
+
+impl PartialEq<bool> for ImplNotTraitWithoutBool {
+    fn eq(&self, other: &bool) -> bool {
+        match *self {
+            ImplNotTraitWithoutBool::VariantX(b) => b == *other,
+            _ => false,
+        }
+    }
+}
+
+impl Not for ImplNotTraitWithoutBool {
+    type Output = Self;
+
+    fn not(self) -> Self::Output {
+        match self {
+            ImplNotTraitWithoutBool::VariantX(b) => ImplNotTraitWithoutBool::VariantX(!b),
+            ImplNotTraitWithoutBool::VariantY(0) => ImplNotTraitWithoutBool::VariantY(1),
+            ImplNotTraitWithoutBool::VariantY(_) => ImplNotTraitWithoutBool::VariantY(0),
+        }
+    }
+}
+
+// This type implements the Not trait with an Output of
+// type bool. Using assert!(..) must be suggested
+#[derive(Debug, Clone, Copy)]
+struct ImplNotTraitWithBool;
+
+impl PartialEq<bool> for ImplNotTraitWithBool {
+    fn eq(&self, _other: &bool) -> bool {
+        false
+    }
+}
+
+impl Not for ImplNotTraitWithBool {
+    type Output = bool;
+
+    fn not(self) -> Self::Output {
+        true
+    }
+}
+
+fn main() {
+    let a = ImplNotTraitWithoutBool::VariantX(true);
+    let b = ImplNotTraitWithBool;
+
+    assert_eq!("a".len(), 1);
+    assert!(!"a".is_empty());
+    assert!("".is_empty());
+    assert!("".is_empty());
+    assert_eq!(a!(), b!());
+    assert_eq!(a!(), "".is_empty());
+    assert_eq!("".is_empty(), b!());
+    assert_eq!(a, true);
+    assert!(b);
+
+    assert_ne!("a".len(), 1);
+    assert!("a".is_empty());
+    assert!(!"".is_empty());
+    assert!(!"".is_empty());
+    assert_ne!(a!(), b!());
+    assert_ne!(a!(), "".is_empty());
+    assert_ne!("".is_empty(), b!());
+    assert_ne!(a, true);
+    assert!(!b);
+
+    debug_assert_eq!("a".len(), 1);
+    debug_assert!(!"a".is_empty());
+    debug_assert!("".is_empty());
+    debug_assert!("".is_empty());
+    debug_assert_eq!(a!(), b!());
+    debug_assert_eq!(a!(), "".is_empty());
+    debug_assert_eq!("".is_empty(), b!());
+    debug_assert_eq!(a, true);
+    debug_assert!(b);
+
+    debug_assert_ne!("a".len(), 1);
+    debug_assert!("a".is_empty());
+    debug_assert!(!"".is_empty());
+    debug_assert!(!"".is_empty());
+    debug_assert_ne!(a!(), b!());
+    debug_assert_ne!(a!(), "".is_empty());
+    debug_assert_ne!("".is_empty(), b!());
+    debug_assert_ne!(a, true);
+    debug_assert!(!b);
+
+    // assert with error messages
+    assert_eq!("a".len(), 1, "tadam {}", 1);
+    assert_eq!("a".len(), 1, "tadam {}", true);
+    assert!(!"a".is_empty(), "tadam {}", 1);
+    assert!(!"a".is_empty(), "tadam {}", true);
+    assert!(!"a".is_empty(), "tadam {}", true);
+    assert_eq!(a, true, "tadam {}", false);
+    assert!("a".is_empty(), "tadam {} {}", false, 6);
+
+    debug_assert_eq!("a".len(), 1, "tadam {}", 1);
+    debug_assert_eq!("a".len(), 1, "tadam {}", true);
+    debug_assert!(!"a".is_empty(), "tadam {}", 1);
+    debug_assert!(!"a".is_empty(), "tadam {}", true);
+    debug_assert!(!"a".is_empty(), "tadam {}", true);
+    debug_assert_eq!(a, true, "tadam {}", false);
+    debug_assert!("a".is_empty(), "tadam {} {}", false, "b");
+
+    // Without ; at the end
+    {
+        debug_assert!(!"a".is_empty(), "tadam {}", true);
+    };
+    {
+        assert_eq!("a".len(), 1, "tadam {}", 1)
+    };
+    {
+        assert_ne!("a".len(), 1, "tadam {}", true)
+    };
+}
diff --git a/tests/ui/bool_assert_comparison.rs b/tests/ui/bool_assert_comparison.rs
diff --git a/tests/ui/bool_assert_comparison.stderr b/tests/ui/bool_assert_comparison.stderr
