[bool_assert_comparaison] improve suggestion

clippy_lints/src/bool_assert_comparison.rs

@@ -1,4 +1,6 @@
-use clippy_utils::{diagnostics::span_lint_and_sugg, higher, is_direct_expn_of, ty::implements_trait};
+use clippy_utils::{
+    diagnostics::span_lint_and_sugg, higher::AssertExpn, is_direct_expn_of, source, ty::implements_trait,
+};
 use rustc_ast::ast::LitKind;
 use rustc_errors::Applicability;
 use rustc_hir::{Expr, ExprKind, Lit};
@@ -30,14 +32,19 @@ declare_clippy_lint! {
 
 declare_lint_pass!(BoolAssertComparison => [BOOL_ASSERT_COMPARISON]);
 
-fn is_bool_lit(e: &Expr<'_>) -> bool {
-    matches!(
-        e.kind,
+fn bool_lit(e: &Expr<'_>) -> Option<bool> {
+    match e.kind {
         ExprKind::Lit(Lit {
-            node: LitKind::Bool(_),
-            ..
-        })
-    ) && !e.span.from_expansion()
+            node: LitKind::Bool(b), ..
+        }) => {
+            if e.span.from_expansion() {
+                None
+            } else {
+                Some(b)
+            }
+        },
+        _ => None,
+    }
 }
 
 fn is_impl_not_trait_with_bool_out(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool {
@@ -68,19 +75,25 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
         let macros = ["assert_eq", "debug_assert_eq"];
         let inverted_macros = ["assert_ne", "debug_assert_ne"];
 
-        for mac in macros.iter().chain(inverted_macros.iter()) {
+        for (mac, is_eq) in macros
+            .iter()
+            .map(|el| (el, true))
+            .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 [a, b, ..] = args[..] {
-                        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;
-                        }
+                if let Some(parse_assert) = AssertExpn::parse(expr) {
+                    if let [a, b] = parse_assert.assert_arguments()[..] {
+                        let (lit_value, other_expr) = match (bool_lit(a), bool_lit(b)) {
+                            (Some(lit), None) => (lit, b),
+                            (None, Some(lit)) => (lit, a),
+                            _ => {
+                                // 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;
+                            },
+                        };
 
                         if !is_impl_not_trait_with_bool_out(cx, a) || !is_impl_not_trait_with_bool_out(cx, b) {
                             // At this point the expression which is not a boolean
@@ -90,14 +103,37 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
                         }
 
                         let non_eq_mac = &mac[..mac.len() - 3];
+                        let mut applicability = Applicability::MachineApplicable;
+                        let expr_string = if lit_value ^ is_eq {
+                            format!("!({})", source::snippet(cx, other_expr.span, ""))
+                        } else {
+                            source::snippet(cx, other_expr.span, "").to_string()
+                        };
+                        let fmt_args = parse_assert.format_arguments(cx, &mut applicability);
+                        let arg_span = match &fmt_args[..] {
+                            [] => None,
+                            [a] => Some(a.to_string()),
+                            _ => {
+                                let mut args = fmt_args[0].to_string();
+                                for el in &fmt_args[1..] {
+                                    args.push_str(&format!(", {}", el));
+                                }
+                                Some(args)
+                            },
+                        };
+                        let suggestion = if let Some(spans) = arg_span {
+                            format!("{}!({}, {})", non_eq_mac, expr_string, spans)
+                        } else {
+                            format!("{}!({})", non_eq_mac, expr_string)
+                        };
                         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,
+                            suggestion,
+                            applicability,
                         );
                         return;
                     }

clippy_lints/src/eq_op.rs

@@ -2,7 +2,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, is_in_test_function,
+    ast_utils::is_useless_with_eq_exprs, eq_expr_value, higher::AssertExpn, in_macro, is_expn_of, is_in_test_function,
 };
 use if_chain::if_chain;
 use rustc_errors::Applicability;
@@ -79,7 +79,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.assert_arguments());
                         if macro_args.len() == 2;
                         let (lhs, rhs) = (macro_args[0], macro_args[1]);
                         if eq_expr_value(cx, lhs, rhs);

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.assert_arguments()) {
                     for arg in macro_args {
                         let mut visitor = MutArgVisitor::new(cx);
                         visitor.visit_expr(arg);

clippy_utils/src/higher.rs

@@ -3,16 +3,19 @@
 #![deny(clippy::missing_docs_in_private_items)]
 
 use crate::ty::is_type_diagnostic_item;
-use crate::{is_expn_of, last_path_segment, match_def_path, paths};
+use crate::{is_expn_of, last_path_segment, match_def_path, paths, source::snippet_with_applicability};
 use if_chain::if_chain;
 use rustc_ast::ast::{self, LitKind};
+use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_hir::{
     Arm, Block, BorrowKind, Expr, ExprKind, HirId, LoopSource, MatchSource, Node, Pat, QPath, StmtKind, UnOp,
 };
 use rustc_lint::LateContext;
 use rustc_span::{sym, symbol, ExpnKind, Span, Symbol};
 
+use std::borrow::Cow;
+
 /// The essential nodes of a desugared for loop as well as the entire span:
 /// `for pat in arg { body }` becomes `(pat, arg, body)`. Return `(pat, arg, body, span)`.
 pub struct ForLoop<'tcx> {
@@ -418,56 +421,177 @@ 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>>> {
-    /// 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<'_>>> {
-        if_chain! {
-            if let ExprKind::Match(headerexpr, _, _) = &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 {
-                return Some(vec![lhs, rhs]);
+/// Kind of assert macros
+pub enum AssertExpnKind<'tcx> {
+    /// Boolean macro like `assert!` or `debug_assert!`
+    Bool(&'tcx Expr<'tcx>),
+    /// Comparaison maacro like `assert_eq!`, `assert_ne!`, `debug_assert_eq!` or `debug_assert_ne!`
+    Eq(&'tcx Expr<'tcx>, &'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> {
+    /// Kind of assert macro
+    pub kind: AssertExpnKind<'tcx>,
+    /// The format argument passed at the end of the macro
+    pub format_arg: Option<FormatArgsExpn<'tcx>>,
+    /// is a debug macro
+    pub is_debug: bool,
+}
+
+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 {
+                    // 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, true);
+                        }
+                    }
+                    // debug macros with unique arg: `debug_assert!` (e.g., `debug_assert!(some_condition)`)
+                    return Self::ast_ifblock(matchexpr, true);
+                }
+            } 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, false);
             }
+        } else {
+            // assert! macro
+            return Self::ast_ifblock(e, false);
         }
         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)`)
+    /// Try to parse the pattern for an assert macro with a single argument like `{debug_}assert!`
+    fn ast_ifblock(ifblock_expr: &'tcx Expr<'tcx>, is_debug: bool) -> Option<Self> {
+        if_chain! {
+            if let Some(If { cond, then, .. }) = If::hir(ifblock_expr);
+            if let ExprKind::Unary(UnOp::Not, condition) = cond.kind;
+            then {
                 if_chain! {
-                    if let Some(If { cond, .. }) = If::hir(matchexpr);
-                    if let ExprKind::Unary(UnOp::Not, condition) = cond.kind;
+                    if let ExprKind::Block(block, _) = then.kind;
+                    if let Some(begin_panic_fmt_block) = block.expr;
+                    if let ExprKind::Block(block,_) = begin_panic_fmt_block.kind;
+                    if let Some(expr) = block.expr;
+                    if let ExprKind::Call(_, args_begin_panic_fmt) = expr.kind;
+                    if !args_begin_panic_fmt.is_empty();
+                    if let ExprKind::AddrOf(_, _, arg) = args_begin_panic_fmt[0].kind;
+                    if let Some(format_arg_expn) = FormatArgsExpn::parse(arg);
                     then {
-                        return Some(vec![condition]);
+                        return Some(Self {
+                            kind: AssertExpnKind::Bool(condition),
+                            format_arg: Some(format_arg_expn),
+                            is_debug
+                        });
                     }
                 }
+                return Some(Self {
+                    kind: AssertExpnKind::Bool(condition),
+                    format_arg: None,
+                    is_debug
+                });
+            }
+        }
+        None
+    }
 
-                // debug macros with two args: `debug_assert_{ne, eq}` (e.g., `assert_ne!(a, b)`)
+    /// 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>, is_debug: bool) -> 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 {
                 if_chain! {
-                    if let ExprKind::Block(matchblock,_) = matchexpr.kind;
-                    if let Some(matchblock_expr) = matchblock.expr;
+                    if !arms.is_empty();
+                    if let ExprKind::Block(Block{expr: Some(if_expr),..},_) = arms[0].body.kind;
+                    if let ExprKind::If(_, if_block, _) = if_expr.kind;
+                    if let ExprKind::Block(Block{stmts: stmts_if_block,..},_) = if_block.kind;
+                    if stmts_if_block.len() >= 2;
+                    if let StmtKind::Expr(call_assert_failed)
+                        | 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(_, [arg, ..]) =  args_assert_failed[3].kind;
+                    if let Some(format_arg_expn) = FormatArgsExpn::parse(arg);
                     then {
-                        return ast_matchblock(matchblock_expr);
+                        return Some(AssertExpn {
+                            kind: AssertExpnKind::Eq(lhs,rhs),
+                            format_arg: Some(format_arg_expn),
+                            is_debug,
+                        });
                     }
                 }
+                return Some(AssertExpn {
+                    kind: AssertExpnKind::Eq(lhs,rhs),
+                    format_arg: None,
+                    is_debug,
+                });
             }
-        } 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);
         }
+        None
+    }
+
+    /// Gives the argument in the comparaison as a vector leaving the format
+    pub fn assert_arguments(&self) -> Vec<&'tcx Expr<'tcx>> {
+        match self.kind {
+            AssertExpnKind::Bool(expr) => {
+                vec![expr]
+            },
+            AssertExpnKind::Eq(lhs, rhs) => {
+                vec![lhs, rhs]
+            },
+        }
+    }
+
+    /// Gives the argument passed to the macro as a string
+    pub fn all_arguments_string(
+        &self,
+        cx: &LateContext<'_>,
+        applicability: &mut Applicability,
+    ) -> Vec<Cow<'static, str>> {
+        let mut vec_arg = vec![];
+        for arg in self.assert_arguments() {
+            vec_arg.push(snippet_with_applicability(cx, arg.span, "..", applicability));
+        }
+        vec_arg.append(&mut self.format_arguments(cx, applicability));
+        vec_arg
+    }
+
+    /// Returns only the format agruments
+    pub fn format_arguments(&self, cx: &LateContext<'_>, applicability: &mut Applicability) -> Vec<Cow<'static, str>> {
+        let mut vec_arg = vec![];
+        if let Some(ref fmt_arg) = self.format_arg {
+            vec_arg.push(snippet_with_applicability(
+                cx,
+                fmt_arg.format_string_span,
+                "..",
+                applicability,
+            ));
+            for arg in &fmt_arg.value_args {
+                vec_arg.push(snippet_with_applicability(cx, arg.span, "..", applicability));
+            }
+        }
+        vec_arg
     }
-    None
 }
 
 /// A parsed `format!` expansion