diff --git a/CHANGELOG.md b/CHANGELOG.md index 59a3dc651..040c906a7 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/if_then_panic.rs b/clippy_lints/src/if_then_panic.rs new file mode 100644 index 000000000..ee575c81a --- /dev/null +++ b/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, + ); + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index c7e59cb16..1a69cf0dc 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -227,6 +227,7 @@ mod identity_op; 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] diff --git a/clippy_lints/src/utils/internal_lints.rs b/clippy_lints/src/utils/internal_lints.rs index 756c33d70..273684b8e 100644 --- a/clippy_lints/src/utils/internal_lints.rs +++ b/clippy_lints/src/utils/internal_lints.rs @@ -561,9 +561,7 @@ declare_lint_pass!(ProduceIce => [PRODUCE_ICE]); impl EarlyLintPass for ProduceIce { fn check_fn(&mut self, _: &EarlyContext<'_>, fn_kind: FnKind<'_>, _: Span, _: NodeId) { - if is_trigger_fn(fn_kind) { - panic!("Would you like some help with that?"); - } + assert!(!is_trigger_fn(fn_kind), "Would you like some help with that?"); } } diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs index 05a4a0143..64bd4223c 100644 --- a/clippy_utils/src/higher.rs +++ b/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 { + 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 + } + } + } +} diff --git a/tests/compile-test.rs b/tests/compile-test.rs index c9d101115..d7596f6ff 100644 --- a/tests/compile-test.rs +++ b/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() { - panic!("dependencies not found in depinfo: {:?}", not_found); - } + assert!( + not_found.is_empty(), + "dependencies not found in depinfo: {:?}", + not_found + ); crates .into_iter() .map(|(name, path)| format!(" --extern {}={}", name, path)) diff --git a/tests/integration.rs b/tests/integration.rs index 7e3eff3c7..c64425fa0 100644 --- a/tests/integration.rs +++ b/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") { - panic!("missing required toolchain"); + } else { + assert!( + !stderr.contains("toolchain") || !stderr.contains("is not installed"), + "missing required toolchain" + ); } match output.status.code() { diff --git a/tests/ui/fallible_impl_from.rs b/tests/ui/fallible_impl_from.rs index 5d5af4e46..495cd97e0 100644 --- a/tests/ui/fallible_impl_from.rs +++ b/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); diff --git a/tests/ui/fallible_impl_from.stderr b/tests/ui/fallible_impl_from.stderr index 64c8ea857..8b8054586 100644 --- a/tests/ui/fallible_impl_from.stderr +++ b/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 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 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> for Invalid { LL | | fn from(s: Option) -> 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 as ProjStrTrait>::ProjString> for Invalid { LL | | fn from(s: &'a mut 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::().ok().unwrap() != 42 { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/tests/ui/if_then_panic.fixed b/tests/ui/if_then_panic.fixed new file mode 100644 index 000000000..fc57ae0df --- /dev/null +++ b/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"); + } +} diff --git a/tests/ui/if_then_panic.rs b/tests/ui/if_then_panic.rs new file mode 100644 index 000000000..d1ac93d8d --- /dev/null +++ b/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"); + } +} diff --git a/tests/ui/if_then_panic.stderr b/tests/ui/if_then_panic.stderr new file mode 100644 index 000000000..b92c9bdf6 --- /dev/null +++ b/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 + diff --git a/tests/ui/ptr_arg.rs b/tests/ui/ptr_arg.rs index 8656c17f2..99e6d2aad 100644 --- a/tests/ui/ptr_arg.rs +++ b/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; diff --git a/tests/ui/ptr_arg.stderr b/tests/ui/ptr_arg.stderr index 64594eb87..42183447e 100644 --- a/tests/ui/ptr_arg.stderr +++ b/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) { | ^^^^^^^^^ help: change this to: `&[i64]` @@ -7,25 +7,25 @@ LL | fn do_vec(x: &Vec) { = 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); | ^^^^^^^^^ 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) -> Vec { | ^^^^^^^^ @@ -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, 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) { | ^^^^^^^^ @@ -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) { | ^^^^^^^^