Auto merge of #10276 - m-ou-se:manual-assert, r=Alexendoo

Don't depend on FormatArgsExpn in ManualAssert.

Part of https://github.com/rust-lang/rust-clippy/issues/10233

changelog: none

clippy_lints/src/manual_assert.rs

@@ -1,7 +1,6 @@
 use crate::rustc_lint::LintContext;
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::macros::{root_macro_call, FormatArgsExpn};
+use clippy_utils::macros::root_macro_call;
-use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::{is_else_clause, peel_blocks_with_stmt, span_extract_comment, sugg};
 use rustc_errors::Applicability;
 use rustc_hir::{Expr, ExprKind, UnOp};
@@ -38,57 +37,57 @@ declare_lint_pass!(ManualAssert => [MANUAL_ASSERT]);
 
 impl<'tcx> LateLintPass<'tcx> for ManualAssert {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
-        if_chain! {
+        if let ExprKind::If(cond, then, None) = expr.kind
-            if let ExprKind::If(cond, then, None) = expr.kind;
+            && !matches!(cond.kind, ExprKind::Let(_))
-            if !matches!(cond.kind, ExprKind::Let(_));
+            && !expr.span.from_expansion()
-            if !expr.span.from_expansion();
+            && let then = peel_blocks_with_stmt(then)
-            let then = peel_blocks_with_stmt(then);
+            && let Some(macro_call) = root_macro_call(then.span)
-            if let Some(macro_call) = root_macro_call(then.span);
+            && cx.tcx.item_name(macro_call.def_id) == sym::panic
-            if cx.tcx.item_name(macro_call.def_id) == sym::panic;
+            && !cx.tcx.sess.source_map().is_multiline(cond.span)
-            if !cx.tcx.sess.source_map().is_multiline(cond.span);
+            && let Ok(panic_snippet) = cx.sess().source_map().span_to_snippet(macro_call.span)
-            if let Some(format_args) = FormatArgsExpn::find_nested(cx, then, macro_call.expn);
+            && let Some(panic_snippet) = panic_snippet.strip_suffix(')')
+            && let Some((_, format_args_snip)) = panic_snippet.split_once('(')
             // Don't change `else if foo { panic!(..) }` to `else { assert!(foo, ..) }` as it just
             // shuffles the condition around.
             // Should this have a config value?
-            if !is_else_clause(cx.tcx, expr);
+            && !is_else_clause(cx.tcx, expr)
-            then {
+        {
-                let mut applicability = Applicability::MachineApplicable;
+            let mut applicability = Applicability::MachineApplicable;
-                let format_args_snip = snippet_with_applicability(cx, format_args.inputs_span(), "..", &mut applicability);
+            let cond = cond.peel_drop_temps();
-                let cond = cond.peel_drop_temps();
+            let mut comments = span_extract_comment(cx.sess().source_map(), expr.span);
-                let mut comments = span_extract_comment(cx.sess().source_map(), expr.span);
+            if !comments.is_empty() {
-                if !comments.is_empty() {
+                comments += "\n";
-                    comments += "\n";
-                }
-                let (cond, not) = match cond.kind {
-                    ExprKind::Unary(UnOp::Not, e) => (e, ""),
-                    _ => (cond, "!"),
-                };
-                let cond_sugg = sugg::Sugg::hir_with_applicability(cx, cond, "..", &mut applicability).maybe_par();
-                let sugg = format!("assert!({not}{cond_sugg}, {format_args_snip});");
-                // we show to the user the suggestion without the comments, but when applicating the fix, include the comments in the block
-                span_lint_and_then(
-                    cx,
-                    MANUAL_ASSERT,
-                    expr.span,
-                    "only a `panic!` in `if`-then statement",
-                    |diag| {
-                        // comments can be noisy, do not show them to the user
-                        if !comments.is_empty() {
-                            diag.tool_only_span_suggestion(
-                                        expr.span.shrink_to_lo(),
-                                        "add comments back",
-                                        comments,
-                                        applicability);
-                        }
-                        diag.span_suggestion(
-                                    expr.span,
-                                    "try instead",
-                                    sugg,
-                                    applicability);
-                                     }
-
-                );
             }
+            let (cond, not) = match cond.kind {
+                ExprKind::Unary(UnOp::Not, e) => (e, ""),
+                _ => (cond, "!"),
+            };
+            let cond_sugg = sugg::Sugg::hir_with_applicability(cx, cond, "..", &mut applicability).maybe_par();
+            let sugg = format!("assert!({not}{cond_sugg}, {format_args_snip});");
+            // we show to the user the suggestion without the comments, but when applicating the fix, include the comments in the block
+            span_lint_and_then(
+                cx,
+                MANUAL_ASSERT,
+                expr.span,
+                "only a `panic!` in `if`-then statement",
+                |diag| {
+                    // comments can be noisy, do not show them to the user
+                    if !comments.is_empty() {
+                        diag.tool_only_span_suggestion(
+                            expr.span.shrink_to_lo(),
+                            "add comments back",
+                            comments,
+                            applicability
+                        );
+                    }
+                    diag.span_suggestion(
+                        expr.span,
+                        "try instead",
+                        sugg,
+                        applicability
+                    );
+                }
+            );
         }
     }
 }

tests/ui/manual_assert.edition2018.fixed

@@ -29,9 +29,7 @@ fn main() {
         panic!("qaqaq{:?}", a);
     }
     assert!(a.is_empty(), "qaqaq{:?}", a);
-    if !a.is_empty() {
+    assert!(a.is_empty(), "qwqwq");
-        panic!("qwqwq");
-    }
     if a.len() == 3 {
         println!("qwq");
         println!("qwq");
@@ -46,21 +44,11 @@ fn main() {
         println!("qwq");
     }
     let b = vec![1, 2, 3];
-    if b.is_empty() {
+    assert!(!b.is_empty(), "panic1");
-        panic!("panic1");
+    assert!(!(b.is_empty() && a.is_empty()), "panic2");
-    }
+    assert!(!(a.is_empty() && !b.is_empty()), "panic3");
-    if b.is_empty() && a.is_empty() {
+    assert!(!(b.is_empty() || a.is_empty()), "panic4");
-        panic!("panic2");
+    assert!(!(a.is_empty() || !b.is_empty()), "panic5");
-    }
-    if a.is_empty() && !b.is_empty() {
-        panic!("panic3");
-    }
-    if b.is_empty() || a.is_empty() {
-        panic!("panic4");
-    }
-    if a.is_empty() || !b.is_empty() {
-        panic!("panic5");
-    }
     assert!(!a.is_empty(), "with expansion {}", one!());
     if a.is_empty() {
         let _ = 0;
@@ -71,12 +59,11 @@ fn main() {
 
 fn issue7730(a: u8) {
     // Suggestion should preserve comment
-    if a > 2 {
+    // comment
-        // comment
+/* this is a
-        /* this is a
         multiline
         comment */
-        /// Doc comment
+/// Doc comment
-        panic!("panic with comment") // comment after `panic!`
+// comment after `panic!`
-    }
+assert!(!(a > 2), "panic with comment");
 }

tests/ui/manual_assert.edition2018.stderr

@@ -8,6 +8,54 @@ LL | |     }
    |
    = note: `-D clippy::manual-assert` implied by `-D warnings`
 
+error: only a `panic!` in `if`-then statement
+  --> $DIR/manual_assert.rs:34:5
+   |
+LL | /     if !a.is_empty() {
+LL | |         panic!("qwqwq");
+LL | |     }
+   | |_____^ help: try instead: `assert!(a.is_empty(), "qwqwq");`
+
+error: only a `panic!` in `if`-then statement
+  --> $DIR/manual_assert.rs:51:5
+   |
+LL | /     if b.is_empty() {
+LL | |         panic!("panic1");
+LL | |     }
+   | |_____^ help: try instead: `assert!(!b.is_empty(), "panic1");`
+
+error: only a `panic!` in `if`-then statement
+  --> $DIR/manual_assert.rs:54:5
+   |
+LL | /     if b.is_empty() && a.is_empty() {
+LL | |         panic!("panic2");
+LL | |     }
+   | |_____^ help: try instead: `assert!(!(b.is_empty() && a.is_empty()), "panic2");`
+
+error: only a `panic!` in `if`-then statement
+  --> $DIR/manual_assert.rs:57:5
+   |
+LL | /     if a.is_empty() && !b.is_empty() {
+LL | |         panic!("panic3");
+LL | |     }
+   | |_____^ help: try instead: `assert!(!(a.is_empty() && !b.is_empty()), "panic3");`
+
+error: only a `panic!` in `if`-then statement
+  --> $DIR/manual_assert.rs:60:5
+   |
+LL | /     if b.is_empty() || a.is_empty() {
+LL | |         panic!("panic4");
+LL | |     }
+   | |_____^ help: try instead: `assert!(!(b.is_empty() || a.is_empty()), "panic4");`
+
+error: only a `panic!` in `if`-then statement
+  --> $DIR/manual_assert.rs:63:5
+   |
+LL | /     if a.is_empty() || !b.is_empty() {
+LL | |         panic!("panic5");
+LL | |     }
+   | |_____^ help: try instead: `assert!(!(a.is_empty() || !b.is_empty()), "panic5");`
+
 error: only a `panic!` in `if`-then statement
   --> $DIR/manual_assert.rs:66:5
    |
@@ -16,5 +64,22 @@ LL | |         panic!("with expansion {}", one!())
 LL | |     }
    | |_____^ help: try instead: `assert!(!a.is_empty(), "with expansion {}", one!());`
 
-error: aborting due to 2 previous errors
+error: only a `panic!` in `if`-then statement
+  --> $DIR/manual_assert.rs:78:5
+   |
+LL | /     if a > 2 {
+LL | |         // comment
+LL | |         /* this is a
+LL | |         multiline
+...  |
+LL | |         panic!("panic with comment") // comment after `panic!`
+LL | |     }
+   | |_____^
+   |
+help: try instead
+   |
+LL |     assert!(!(a > 2), "panic with comment");
+   |
+
+error: aborting due to 9 previous errors