Auto merge of #7669 - Labelray:if_then_panic, r=giraffate

New lint `if_then_panic`

changelog: add the new lint [`if_then_panic`]
fix #7645

CHANGELOG.md

@@ -2688,6 +2688,7 @@ Released 2018-09-13
 [`if_let_some_result`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_let_some_result
 [`if_not_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_not_else
 [`if_same_then_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else
+[`if_then_panic`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_then_panic
 [`if_then_some_else_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_then_some_else_none
 [`ifs_same_cond`]: https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond
 [`implicit_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_clone

clippy_lints/src/if_then_panic.rs

@@ -0,0 +1,97 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::higher::PanicExpn;
+use clippy_utils::is_expn_of;
+use clippy_utils::source::snippet_with_applicability;
+use rustc_errors::Applicability;
+use rustc_hir::{Block, Expr, ExprKind, StmtKind, UnOp};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Detects `if`-then-`panic!` that can be replaced with `assert!`.
+    ///
+    /// ### Why is this bad?
+    /// `assert!` is simpler than `if`-then-`panic!`.
+    ///
+    /// ### Example
+    /// ```rust
+    /// let sad_people: Vec<&str> = vec![];
+    /// if !sad_people.is_empty() {
+    ///     panic!("there are sad people: {:?}", sad_people);
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let sad_people: Vec<&str> = vec![];
+    /// assert!(sad_people.is_empty(), "there are sad people: {:?}", sad_people);
+    /// ```
+    pub IF_THEN_PANIC,
+    style,
+    "`panic!` and only a `panic!` in `if`-then statement"
+}
+
+declare_lint_pass!(IfThenPanic => [IF_THEN_PANIC]);
+
+impl LateLintPass<'_> for IfThenPanic {
+    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
+        if_chain! {
+            if let Expr {
+                kind: ExprKind:: If(cond, Expr {
+                    kind: ExprKind::Block(
+                        Block {
+                            stmts: [stmt],
+                            ..
+                        },
+                        _),
+                    ..
+                }, None),
+                ..
+            } = &expr;
+            if is_expn_of(stmt.span, "panic").is_some();
+            if !matches!(cond.kind, ExprKind::Let(_, _, _));
+            if let StmtKind::Semi(semi) = stmt.kind;
+            if !cx.tcx.sess.source_map().is_multiline(cond.span);
+
+            then {
+                let span = if let Some(panic_expn) = PanicExpn::parse(semi) {
+                    match *panic_expn.format_args.value_args {
+                        [] => panic_expn.format_args.format_string_span,
+                        [.., last] => panic_expn.format_args.format_string_span.to(last.span),
+                    }
+                } else {
+                    if_chain! {
+                        if let ExprKind::Block(block, _) = semi.kind;
+                        if let Some(init) = block.expr;
+                        if let ExprKind::Call(_, [format_args]) = init.kind;
+
+                        then {
+                            format_args.span
+                        } else {
+                            return
+                        }
+                    }
+                };
+                let mut applicability = Applicability::MachineApplicable;
+                let sugg = snippet_with_applicability(cx, span, "..", &mut applicability);
+
+                let cond_sugg =
+                if let ExprKind::DropTemps(Expr{kind: ExprKind::Unary(UnOp::Not, not_expr), ..}) = cond.kind {
+                    snippet_with_applicability(cx, not_expr.span, "..", &mut applicability).to_string()
+                } else {
+                    format!("!{}", snippet_with_applicability(cx, cond.span, "..", &mut applicability))
+                };
+
+                span_lint_and_sugg(
+                    cx,
+                    IF_THEN_PANIC,
+                    expr.span,
+                    "only a `panic!` in `if`-then statement",
+                    "try",
+                    format!("assert!({}, {});", cond_sugg, sugg),
+                    Applicability::MachineApplicable,
+                );
+            }
+        }
+    }
+}

clippy_lints/src/lib.rs

@@ -227,6 +227,7 @@ macro_rules! declare_clippy_lint {
 mod if_let_mutex;
 mod if_let_some_result;
 mod if_not_else;
+mod if_then_panic;
 mod if_then_some_else_none;
 mod implicit_hasher;
 mod implicit_return;
@@ -659,6 +660,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         if_let_mutex::IF_LET_MUTEX,
         if_let_some_result::IF_LET_SOME_RESULT,
         if_not_else::IF_NOT_ELSE,
+        if_then_panic::IF_THEN_PANIC,
         if_then_some_else_none::IF_THEN_SOME_ELSE_NONE,
         implicit_hasher::IMPLICIT_HASHER,
         implicit_return::IMPLICIT_RETURN,
@@ -1257,6 +1259,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(identity_op::IDENTITY_OP),
         LintId::of(if_let_mutex::IF_LET_MUTEX),
         LintId::of(if_let_some_result::IF_LET_SOME_RESULT),
+        LintId::of(if_then_panic::IF_THEN_PANIC),
         LintId::of(indexing_slicing::OUT_OF_BOUNDS_INDEXING),
         LintId::of(infinite_iter::INFINITE_ITER),
         LintId::of(inherent_to_string::INHERENT_TO_STRING),
@@ -1511,6 +1514,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(functions::MUST_USE_UNIT),
         LintId::of(functions::RESULT_UNIT_ERR),
         LintId::of(if_let_some_result::IF_LET_SOME_RESULT),
+        LintId::of(if_then_panic::IF_THEN_PANIC),
         LintId::of(inherent_to_string::INHERENT_TO_STRING),
         LintId::of(len_zero::COMPARISON_TO_EMPTY),
         LintId::of(len_zero::LEN_WITHOUT_IS_EMPTY),
@@ -2138,6 +2142,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(move || Box::new(self_named_constructors::SelfNamedConstructors));
     store.register_late_pass(move || Box::new(feature_name::FeatureName));
     store.register_late_pass(move || Box::new(iter_not_returning_iterator::IterNotReturningIterator));
+    store.register_late_pass(move || Box::new(if_then_panic::IfThenPanic));
 }
 
 #[rustfmt::skip]

clippy_lints/src/utils/internal_lints.rs

@@ -561,9 +561,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
 
 impl EarlyLintPass for ProduceIce {
     fn check_fn(&mut self, _: &EarlyContext<'_>, fn_kind: FnKind<'_>, _: Span, _: NodeId) {
-        if is_trigger_fn(fn_kind) {
+        assert!(!is_trigger_fn(fn_kind), "Would you like some help with that?");
-            panic!("Would you like some help with that?");
-        }
     }
 }
 

clippy_utils/src/higher.rs

@@ -608,3 +608,33 @@ pub fn is_from_for_desugar(local: &hir::Local<'_>) -> bool {
 
     false
 }
+
+/// A parsed `panic!` expansion
+pub struct PanicExpn<'tcx> {
+    /// Span of `panic!(..)`
+    pub call_site: Span,
+    /// Inner `format_args!` expansion
+    pub format_args: FormatArgsExpn<'tcx>,
+}
+
+impl PanicExpn<'tcx> {
+    /// Parses an expanded `panic!` invocation
+    pub fn parse(expr: &'tcx Expr<'tcx>) -> Option<Self> {
+        if_chain! {
+            if let ExprKind::Block(block, _) = expr.kind;
+            if let Some(init) = block.expr;
+            if let ExprKind::Call(_, [format_args]) = init.kind;
+            let expn_data = expr.span.ctxt().outer_expn_data();
+            if let ExprKind::AddrOf(_, _, format_args) = format_args.kind;
+            if let Some(format_args) = FormatArgsExpn::parse(format_args);
+            then {
+                Some(PanicExpn {
+                    call_site: expn_data.call_site,
+                    format_args,
+                })
+            } else {
+                None
+            }
+        }
+    }
+}

tests/compile-test.rs

@@ -90,9 +90,11 @@ fn extern_flags() -> String {
         .copied()
         .filter(|n| !crates.contains_key(n))
         .collect();
-    if !not_found.is_empty() {
+    assert!(
-        panic!("dependencies not found in depinfo: {:?}", not_found);
+        not_found.is_empty(),
-    }
+        "dependencies not found in depinfo: {:?}",
+        not_found
+    );
     crates
         .into_iter()
         .map(|(name, path)| format!(" --extern {}={}", name, path))

tests/integration.rs

@@ -74,8 +74,11 @@ fn integration_test() {
         panic!("incompatible crate versions");
     } else if stderr.contains("failed to run `rustc` to learn about target-specific information") {
         panic!("couldn't find librustc_driver, consider setting `LD_LIBRARY_PATH`");
-    } else if stderr.contains("toolchain") && stderr.contains("is not installed") {
+    } else {
-        panic!("missing required toolchain");
+        assert!(
+            !stderr.contains("toolchain") || !stderr.contains("is not installed"),
+            "missing required toolchain"
+        );
     }
 
     match output.status.code() {

tests/ui/fallible_impl_from.rs

@@ -1,4 +1,5 @@
 #![deny(clippy::fallible_impl_from)]
+#![allow(clippy::if_then_panic)]
 
 // docs example
 struct Foo(i32);

tests/ui/fallible_impl_from.stderr

@@ -1,5 +1,5 @@
 error: consider implementing `TryFrom` instead
-  --> $DIR/fallible_impl_from.rs:5:1
+  --> $DIR/fallible_impl_from.rs:6:1
    |
 LL | / impl From<String> for Foo {
 LL | |     fn from(s: String) -> Self {
@@ -15,13 +15,13 @@ LL | #![deny(clippy::fallible_impl_from)]
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: `From` is intended for infallible conversions only. Use `TryFrom` if there's a possibility for the conversion to fail
 note: potential failure(s)
-  --> $DIR/fallible_impl_from.rs:7:13
+  --> $DIR/fallible_impl_from.rs:8:13
    |
 LL |         Foo(s.parse().unwrap())
    |             ^^^^^^^^^^^^^^^^^^
 
 error: consider implementing `TryFrom` instead
-  --> $DIR/fallible_impl_from.rs:26:1
+  --> $DIR/fallible_impl_from.rs:27:1
    |
 LL | / impl From<usize> for Invalid {
 LL | |     fn from(i: usize) -> Invalid {
@@ -34,14 +34,14 @@ LL | | }
    |
    = help: `From` is intended for infallible conversions only. Use `TryFrom` if there's a possibility for the conversion to fail
 note: potential failure(s)
-  --> $DIR/fallible_impl_from.rs:29:13
+  --> $DIR/fallible_impl_from.rs:30:13
    |
 LL |             panic!();
    |             ^^^^^^^^^
    = note: this error originates in the macro `$crate::panic::panic_2015` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 error: consider implementing `TryFrom` instead
-  --> $DIR/fallible_impl_from.rs:35:1
+  --> $DIR/fallible_impl_from.rs:36:1
    |
 LL | / impl From<Option<String>> for Invalid {
 LL | |     fn from(s: Option<String>) -> Invalid {
@@ -54,7 +54,7 @@ LL | | }
    |
    = help: `From` is intended for infallible conversions only. Use `TryFrom` if there's a possibility for the conversion to fail
 note: potential failure(s)
-  --> $DIR/fallible_impl_from.rs:37:17
+  --> $DIR/fallible_impl_from.rs:38:17
    |
 LL |         let s = s.unwrap();
    |                 ^^^^^^^^^^
@@ -68,7 +68,7 @@ LL |             panic!("{:?}", s);
    = note: this error originates in the macro `$crate::panic::panic_2015` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 error: consider implementing `TryFrom` instead
-  --> $DIR/fallible_impl_from.rs:53:1
+  --> $DIR/fallible_impl_from.rs:54:1
    |
 LL | / impl<'a> From<&'a mut <Box<u32> as ProjStrTrait>::ProjString> for Invalid {
 LL | |     fn from(s: &'a mut <Box<u32> as ProjStrTrait>::ProjString) -> Invalid {
@@ -81,7 +81,7 @@ LL | | }
    |
    = help: `From` is intended for infallible conversions only. Use `TryFrom` if there's a possibility for the conversion to fail
 note: potential failure(s)
-  --> $DIR/fallible_impl_from.rs:55:12
+  --> $DIR/fallible_impl_from.rs:56:12
    |
 LL |         if s.parse::<u32>().ok().unwrap() != 42 {
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

tests/ui/if_then_panic.fixed

@@ -0,0 +1,34 @@
+// run-rustfix
+#![warn(clippy::if_then_panic)]
+
+fn main() {
+    let a = vec![1, 2, 3];
+    let c = Some(2);
+    if !a.is_empty()
+        && a.len() == 3
+        && c != None
+        && !a.is_empty()
+        && a.len() == 3
+        && !a.is_empty()
+        && a.len() == 3
+        && !a.is_empty()
+        && a.len() == 3
+    {
+        panic!("qaqaq{:?}", a);
+    }
+    assert!(a.is_empty(), "qaqaq{:?}", a);
+    assert!(a.is_empty(), "qwqwq");
+    if a.len() == 3 {
+        println!("qwq");
+        println!("qwq");
+        println!("qwq");
+    }
+    if let Some(b) = c {
+        panic!("orz {}", b);
+    }
+    if a.len() == 3 {
+        panic!("qaqaq");
+    } else {
+        println!("qwq");
+    }
+}

tests/ui/if_then_panic.rs

@@ -0,0 +1,38 @@
+// run-rustfix
+#![warn(clippy::if_then_panic)]
+
+fn main() {
+    let a = vec![1, 2, 3];
+    let c = Some(2);
+    if !a.is_empty()
+        && a.len() == 3
+        && c != None
+        && !a.is_empty()
+        && a.len() == 3
+        && !a.is_empty()
+        && a.len() == 3
+        && !a.is_empty()
+        && a.len() == 3
+    {
+        panic!("qaqaq{:?}", a);
+    }
+    if !a.is_empty() {
+        panic!("qaqaq{:?}", a);
+    }
+    if !a.is_empty() {
+        panic!("qwqwq");
+    }
+    if a.len() == 3 {
+        println!("qwq");
+        println!("qwq");
+        println!("qwq");
+    }
+    if let Some(b) = c {
+        panic!("orz {}", b);
+    }
+    if a.len() == 3 {
+        panic!("qaqaq");
+    } else {
+        println!("qwq");
+    }
+}

tests/ui/if_then_panic.stderr

@@ -0,0 +1,20 @@
+error: only a `panic!` in `if`-then statement
+  --> $DIR/if_then_panic.rs:19:5
+   |
+LL | /     if !a.is_empty() {
+LL | |         panic!("qaqaq{:?}", a);
+LL | |     }
+   | |_____^ help: try: `assert!(a.is_empty(), "qaqaq{:?}", a);`
+   |
+   = note: `-D clippy::if-then-panic` implied by `-D warnings`
+
+error: only a `panic!` in `if`-then statement
+  --> $DIR/if_then_panic.rs:22:5
+   |
+LL | /     if !a.is_empty() {
+LL | |         panic!("qwqwq");
+LL | |     }
+   | |_____^ help: try: `assert!(a.is_empty(), "qwqwq");`
+
+error: aborting due to 2 previous errors
+

tests/ui/ptr_arg.rs

@@ -1,4 +1,9 @@
-#![allow(unused, clippy::redundant_clone)]
+#![allow(
+    unused,
+    clippy::many_single_char_names,
+    clippy::redundant_clone,
+    clippy::if_then_panic
+)]
 #![warn(clippy::ptr_arg)]
 
 use std::borrow::Cow;

tests/ui/ptr_arg.stderr

@@ -1,5 +1,5 @@
 error: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices
-  --> $DIR/ptr_arg.rs:7:14
+  --> $DIR/ptr_arg.rs:12:14
    |
 LL | fn do_vec(x: &Vec<i64>) {
    |              ^^^^^^^^^ help: change this to: `&[i64]`
@@ -7,25 +7,25 @@ LL | fn do_vec(x: &Vec<i64>) {
    = note: `-D clippy::ptr-arg` implied by `-D warnings`
 
 error: writing `&String` instead of `&str` involves a new object where a slice will do
-  --> $DIR/ptr_arg.rs:16:14
+  --> $DIR/ptr_arg.rs:21:14
    |
 LL | fn do_str(x: &String) {
    |              ^^^^^^^ help: change this to: `&str`
 
 error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do
-  --> $DIR/ptr_arg.rs:25:15
+  --> $DIR/ptr_arg.rs:30:15
    |
 LL | fn do_path(x: &PathBuf) {
    |               ^^^^^^^^ help: change this to: `&Path`
 
 error: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices
-  --> $DIR/ptr_arg.rs:38:18
+  --> $DIR/ptr_arg.rs:43:18
    |
 LL |     fn do_vec(x: &Vec<i64>);
    |                  ^^^^^^^^^ help: change this to: `&[i64]`
 
 error: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices
-  --> $DIR/ptr_arg.rs:51:14
+  --> $DIR/ptr_arg.rs:56:14
    |
 LL | fn cloned(x: &Vec<u8>) -> Vec<u8> {
    |              ^^^^^^^^
@@ -44,7 +44,7 @@ LL |     x.to_owned()
    |
 
 error: writing `&String` instead of `&str` involves a new object where a slice will do
-  --> $DIR/ptr_arg.rs:60:18
+  --> $DIR/ptr_arg.rs:65:18
    |
 LL | fn str_cloned(x: &String) -> String {
    |                  ^^^^^^^
@@ -67,7 +67,7 @@ LL |     x.to_string()
    |
 
 error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do
-  --> $DIR/ptr_arg.rs:68:19
+  --> $DIR/ptr_arg.rs:73:19
    |
 LL | fn path_cloned(x: &PathBuf) -> PathBuf {
    |                   ^^^^^^^^
@@ -90,7 +90,7 @@ LL |     x.to_path_buf()
    |
 
 error: writing `&String` instead of `&str` involves a new object where a slice will do
-  --> $DIR/ptr_arg.rs:76:44
+  --> $DIR/ptr_arg.rs:81:44
    |
 LL | fn false_positive_capacity(x: &Vec<u8>, y: &String) {
    |                                            ^^^^^^^
@@ -109,13 +109,13 @@ LL |     let c = y;
    |             ~
 
 error: using a reference to `Cow` is not recommended
-  --> $DIR/ptr_arg.rs:90:25
+  --> $DIR/ptr_arg.rs:95:25
    |
 LL | fn test_cow_with_ref(c: &Cow<[i32]>) {}
    |                         ^^^^^^^^^^^ help: change this to: `&[i32]`
 
 error: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices
-  --> $DIR/ptr_arg.rs:143:21
+  --> $DIR/ptr_arg.rs:148:21
    |
 LL |     fn foo_vec(vec: &Vec<u8>) {
    |                     ^^^^^^^^
@@ -134,7 +134,7 @@ LL |         let _ = vec.to_owned().clone();
    |                 ~~~~~~~~~~~~~~
 
 error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do
-  --> $DIR/ptr_arg.rs:148:23
+  --> $DIR/ptr_arg.rs:153:23
    |
 LL |     fn foo_path(path: &PathBuf) {
    |                       ^^^^^^^^
@@ -153,7 +153,7 @@ LL |         let _ = path.to_path_buf().clone();
    |                 ~~~~~~~~~~~~~~~~~~
 
 error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do
-  --> $DIR/ptr_arg.rs:153:21
+  --> $DIR/ptr_arg.rs:158:21
    |
 LL |     fn foo_str(str: &PathBuf) {
    |                     ^^^^^^^^