Skip to content

Commit

Permalink
Add new lint if_then_panic
Browse files Browse the repository at this point in the history
  • Loading branch information
Labelray committed Sep 22, 2021
1 parent b556398 commit 44ad749
Show file tree
Hide file tree
Showing 12 changed files with 275 additions and 10 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
97 changes: 97 additions & 0 deletions clippy_lints/src/if_then_panic.rs
Original file line number Diff line number Diff line change
@@ -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,
);
}
}
}
}
5 changes: 5 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -658,6 +659,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,
Expand Down Expand Up @@ -1253,6 +1255,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),
Expand Down Expand Up @@ -1508,6 +1511,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),
Expand Down Expand Up @@ -2135,6 +2139,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]
Expand Down
4 changes: 1 addition & 3 deletions clippy_lints/src/utils/internal_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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?");
}
}

Expand Down
30 changes: 30 additions & 0 deletions clippy_utils/src/higher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
}
}
8 changes: 5 additions & 3 deletions tests/compile-test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
7 changes: 5 additions & 2 deletions tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
29 changes: 28 additions & 1 deletion tests/ui/fallible_impl_from.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,16 @@ 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: only a `panic!` in `if`-then statement
--> $DIR/fallible_impl_from.rs:28:9
|
LL | / if i != 42 {
LL | | panic!();
LL | | }
| |_________^ help: try: `assert!(!i != 42, "explicit panic");`
|
= note: `-D clippy::if-then-panic` implied by `-D warnings`

error: consider implementing `TryFrom` instead
--> $DIR/fallible_impl_from.rs:35:1
|
Expand Down Expand Up @@ -67,6 +77,15 @@ 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: only a `panic!` in `if`-then statement
--> $DIR/fallible_impl_from.rs:40:16
|
LL | } else if s.parse::<u32>().unwrap() != 42 {
| ________________^
LL | | panic!("{:?}", s);
LL | | }
| |_________^ help: try: `assert!(!s.parse::<u32>().unwrap() != 42, "{:?}", s);`

error: consider implementing `TryFrom` instead
--> $DIR/fallible_impl_from.rs:53:1
|
Expand All @@ -89,5 +108,13 @@ 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: aborting due to 4 previous errors
error: only a `panic!` in `if`-then statement
--> $DIR/fallible_impl_from.rs:55:9
|
LL | / if s.parse::<u32>().ok().unwrap() != 42 {
LL | | panic!("{:?}", s);
LL | | }
| |_________^ help: try: `assert!(!s.parse::<u32>().ok().unwrap() != 42, "{:?}", s);`

error: aborting due to 7 previous errors

34 changes: 34 additions & 0 deletions tests/ui/if_then_panic.fixed
Original file line number Diff line number Diff line change
@@ -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");
}
}
38 changes: 38 additions & 0 deletions tests/ui/if_then_panic.rs
Original file line number Diff line number Diff line change
@@ -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");
}
}
20 changes: 20 additions & 0 deletions tests/ui/if_then_panic.stderr
Original file line number Diff line number Diff line change
@@ -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

12 changes: 11 additions & 1 deletion tests/ui/ptr_arg.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,16 @@ help: change `y.as_str()` to
LL | let c = y;
| ~

error: only a `panic!` in `if`-then statement
--> $DIR/ptr_arg.rs:83:5
|
LL | / if x.capacity() > 1024 {
LL | | panic!("Too large!");
LL | | }
| |_____^ help: try: `assert!(!x.capacity() > 1024, "Too large!");`
|
= note: `-D clippy::if-then-panic` implied by `-D warnings`

error: using a reference to `Cow` is not recommended
--> $DIR/ptr_arg.rs:90:25
|
Expand Down Expand Up @@ -171,5 +181,5 @@ help: change `str.clone()` to
LL | let _ = str.to_path_buf().clone();
| ~~~~~~~~~~~~~~~~~

error: aborting due to 12 previous errors
error: aborting due to 13 previous errors

0 comments on commit 44ad749

Please sign in to comment.