From 4babfae9cba0409d2eed2c5caa623fae0445a50e Mon Sep 17 00:00:00 2001 From: surechen Date: Thu, 30 Sep 2021 19:06:42 +0800 Subject: [PATCH 1/2] fix issue #7731 Make if_then_else handle situation of cond.kind = ExprKind::DropTemps(ExprKind::Binary(BinOpKind::And || BinOpKind::Or, left, right), ..) = --- clippy_lints/src/if_then_panic.rs | 38 ++++++++++++++++++++++++---- tests/ui/if_then_panic.fixed | 6 +++++ tests/ui/if_then_panic.rs | 16 ++++++++++++ tests/ui/if_then_panic.stderr | 42 ++++++++++++++++++++++++++++++- 4 files changed, 96 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/if_then_panic.rs b/clippy_lints/src/if_then_panic.rs index ee575c81a8b..31e416bd4d6 100644 --- a/clippy_lints/src/if_then_panic.rs +++ b/clippy_lints/src/if_then_panic.rs @@ -3,7 +3,7 @@ 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_hir::{BinOpKind, Block, Expr, ExprKind, StmtKind, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -74,10 +74,38 @@ impl LateLintPass<'_> for IfThenPanic { }; 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() + //let mut cond_sugg = format!("!{}", snippet_with_applicability(cx, cond.span, "..", &mut applicability)); + let cond_sugg = if let ExprKind::DropTemps(e, ..) = cond.kind { + if let Expr{kind: ExprKind::Unary(UnOp::Not, not_expr), ..} = e { + snippet_with_applicability(cx, not_expr.span, "..", &mut applicability).to_string() + } else if let Expr{kind: ExprKind::Binary(op, left, right), ..} = e {//BinOp{BinOpKind::And, ..} + match op.node { + BinOpKind::And | BinOpKind::Or => { + let left_span = { + if let Expr{kind: ExprKind::Unary(UnOp::Not, not_expr), ..} = left { + snippet_with_applicability(cx, not_expr.span, "..", &mut applicability).to_string() + } else { + format!("!{}", snippet_with_applicability(cx, left.span, "..", &mut applicability)) + } + }; + let right_span = { + if let Expr{kind: ExprKind::Unary(UnOp::Not, not_expr), ..} = right { + snippet_with_applicability(cx, not_expr.span, "..", &mut applicability).to_string() + } else { + format!("!{}", snippet_with_applicability(cx, right.span, "..", &mut applicability)) + } + }; + if op.node == BinOpKind::And { + format!("{} || {}", left_span, right_span) + } else { + format!("{} && {}", left_span, right_span) + } + } + _ => format!("!({})", snippet_with_applicability(cx, cond.span, "..", &mut applicability)) + } + } else { + format!("!{}", snippet_with_applicability(cx, cond.span, "..", &mut applicability)) + } } else { format!("!{}", snippet_with_applicability(cx, cond.span, "..", &mut applicability)) }; diff --git a/tests/ui/if_then_panic.fixed b/tests/ui/if_then_panic.fixed index fc57ae0dfa5..f72bc6f5979 100644 --- a/tests/ui/if_then_panic.fixed +++ b/tests/ui/if_then_panic.fixed @@ -31,4 +31,10 @@ fn main() { } else { println!("qwq"); } + let b = vec![1, 2, 3]; + assert!(!b.is_empty(), "panic1"); + assert!(!b.is_empty() || !a.is_empty(), "panic2"); + assert!(!a.is_empty() || b.is_empty(), "panic3"); + assert!(!b.is_empty() && !a.is_empty(), "panic4"); + assert!(!a.is_empty() && b.is_empty(), "panic5"); } diff --git a/tests/ui/if_then_panic.rs b/tests/ui/if_then_panic.rs index d1ac93d8d41..10433c8d54f 100644 --- a/tests/ui/if_then_panic.rs +++ b/tests/ui/if_then_panic.rs @@ -35,4 +35,20 @@ fn main() { } else { println!("qwq"); } + let b = vec![1, 2, 3]; + if b.is_empty() { + panic!("panic1"); + } + if b.is_empty() && a.is_empty() { + panic!("panic2"); + } + 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"); + } } diff --git a/tests/ui/if_then_panic.stderr b/tests/ui/if_then_panic.stderr index b92c9bdf674..502ba10b8f4 100644 --- a/tests/ui/if_then_panic.stderr +++ b/tests/ui/if_then_panic.stderr @@ -16,5 +16,45 @@ LL | | panic!("qwqwq"); LL | | } | |_____^ help: try: `assert!(a.is_empty(), "qwqwq");` -error: aborting due to 2 previous errors +error: only a `panic!` in `if`-then statement + --> $DIR/if_then_panic.rs:39:5 + | +LL | / if b.is_empty() { +LL | | panic!("panic1"); +LL | | } + | |_____^ help: try: `assert!(!b.is_empty(), "panic1");` + +error: only a `panic!` in `if`-then statement + --> $DIR/if_then_panic.rs:42:5 + | +LL | / if b.is_empty() && a.is_empty() { +LL | | panic!("panic2"); +LL | | } + | |_____^ help: try: `assert!(!b.is_empty() || !a.is_empty(), "panic2");` + +error: only a `panic!` in `if`-then statement + --> $DIR/if_then_panic.rs:45:5 + | +LL | / if a.is_empty() && !b.is_empty() { +LL | | panic!("panic3"); +LL | | } + | |_____^ help: try: `assert!(!a.is_empty() || b.is_empty(), "panic3");` + +error: only a `panic!` in `if`-then statement + --> $DIR/if_then_panic.rs:48:5 + | +LL | / if b.is_empty() || a.is_empty() { +LL | | panic!("panic4"); +LL | | } + | |_____^ help: try: `assert!(!b.is_empty() && !a.is_empty(), "panic4");` + +error: only a `panic!` in `if`-then statement + --> $DIR/if_then_panic.rs:51:5 + | +LL | / if a.is_empty() || !b.is_empty() { +LL | | panic!("panic5"); +LL | | } + | |_____^ help: try: `assert!(!a.is_empty() && b.is_empty(), "panic5");` + +error: aborting due to 7 previous errors From 41e2c68a6e3e6d06c6e3549ae4e79f145fb4ba1b Mon Sep 17 00:00:00 2001 From: surechen Date: Fri, 1 Oct 2021 13:13:09 +0800 Subject: [PATCH 2/2] use sugg::Sugg::maybe_par --- clippy_lints/src/if_then_panic.rs | 36 +++++-------------------------- tests/ui/if_then_panic.fixed | 8 +++---- tests/ui/if_then_panic.stderr | 8 +++---- 3 files changed, 13 insertions(+), 39 deletions(-) diff --git a/clippy_lints/src/if_then_panic.rs b/clippy_lints/src/if_then_panic.rs index 31e416bd4d6..10bca59e6d0 100644 --- a/clippy_lints/src/if_then_panic.rs +++ b/clippy_lints/src/if_then_panic.rs @@ -1,9 +1,9 @@ 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 clippy_utils::{is_expn_of, sugg}; use rustc_errors::Applicability; -use rustc_hir::{BinOpKind, Block, Expr, ExprKind, StmtKind, UnOp}; +use rustc_hir::{Block, Expr, ExprKind, StmtKind, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -74,40 +74,14 @@ impl LateLintPass<'_> for IfThenPanic { }; let mut applicability = Applicability::MachineApplicable; let sugg = snippet_with_applicability(cx, span, "..", &mut applicability); - //let mut cond_sugg = format!("!{}", snippet_with_applicability(cx, cond.span, "..", &mut applicability)); let cond_sugg = if let ExprKind::DropTemps(e, ..) = cond.kind { if let Expr{kind: ExprKind::Unary(UnOp::Not, not_expr), ..} = e { - snippet_with_applicability(cx, not_expr.span, "..", &mut applicability).to_string() - } else if let Expr{kind: ExprKind::Binary(op, left, right), ..} = e {//BinOp{BinOpKind::And, ..} - match op.node { - BinOpKind::And | BinOpKind::Or => { - let left_span = { - if let Expr{kind: ExprKind::Unary(UnOp::Not, not_expr), ..} = left { - snippet_with_applicability(cx, not_expr.span, "..", &mut applicability).to_string() - } else { - format!("!{}", snippet_with_applicability(cx, left.span, "..", &mut applicability)) - } - }; - let right_span = { - if let Expr{kind: ExprKind::Unary(UnOp::Not, not_expr), ..} = right { - snippet_with_applicability(cx, not_expr.span, "..", &mut applicability).to_string() - } else { - format!("!{}", snippet_with_applicability(cx, right.span, "..", &mut applicability)) - } - }; - if op.node == BinOpKind::And { - format!("{} || {}", left_span, right_span) - } else { - format!("{} && {}", left_span, right_span) - } - } - _ => format!("!({})", snippet_with_applicability(cx, cond.span, "..", &mut applicability)) - } + sugg::Sugg::hir_with_applicability(cx, not_expr, "..", &mut applicability).maybe_par().to_string() } else { - format!("!{}", snippet_with_applicability(cx, cond.span, "..", &mut applicability)) + format!("!{}", sugg::Sugg::hir_with_applicability(cx, e, "..", &mut applicability).maybe_par().to_string()) } } else { - format!("!{}", snippet_with_applicability(cx, cond.span, "..", &mut applicability)) + format!("!{}", sugg::Sugg::hir_with_applicability(cx, cond, "..", &mut applicability).maybe_par().to_string()) }; span_lint_and_sugg( diff --git a/tests/ui/if_then_panic.fixed b/tests/ui/if_then_panic.fixed index f72bc6f5979..0998f8ffa9d 100644 --- a/tests/ui/if_then_panic.fixed +++ b/tests/ui/if_then_panic.fixed @@ -33,8 +33,8 @@ fn main() { } let b = vec![1, 2, 3]; assert!(!b.is_empty(), "panic1"); - assert!(!b.is_empty() || !a.is_empty(), "panic2"); - assert!(!a.is_empty() || b.is_empty(), "panic3"); - assert!(!b.is_empty() && !a.is_empty(), "panic4"); - assert!(!a.is_empty() && b.is_empty(), "panic5"); + assert!(!(b.is_empty() && a.is_empty()), "panic2"); + assert!(!(a.is_empty() && !b.is_empty()), "panic3"); + assert!(!(b.is_empty() || a.is_empty()), "panic4"); + assert!(!(a.is_empty() || !b.is_empty()), "panic5"); } diff --git a/tests/ui/if_then_panic.stderr b/tests/ui/if_then_panic.stderr index 502ba10b8f4..5bb62f87566 100644 --- a/tests/ui/if_then_panic.stderr +++ b/tests/ui/if_then_panic.stderr @@ -30,7 +30,7 @@ error: only a `panic!` in `if`-then statement LL | / if b.is_empty() && a.is_empty() { LL | | panic!("panic2"); LL | | } - | |_____^ help: try: `assert!(!b.is_empty() || !a.is_empty(), "panic2");` + | |_____^ help: try: `assert!(!(b.is_empty() && a.is_empty()), "panic2");` error: only a `panic!` in `if`-then statement --> $DIR/if_then_panic.rs:45:5 @@ -38,7 +38,7 @@ error: only a `panic!` in `if`-then statement LL | / if a.is_empty() && !b.is_empty() { LL | | panic!("panic3"); LL | | } - | |_____^ help: try: `assert!(!a.is_empty() || b.is_empty(), "panic3");` + | |_____^ help: try: `assert!(!(a.is_empty() && !b.is_empty()), "panic3");` error: only a `panic!` in `if`-then statement --> $DIR/if_then_panic.rs:48:5 @@ -46,7 +46,7 @@ error: only a `panic!` in `if`-then statement LL | / if b.is_empty() || a.is_empty() { LL | | panic!("panic4"); LL | | } - | |_____^ help: try: `assert!(!b.is_empty() && !a.is_empty(), "panic4");` + | |_____^ help: try: `assert!(!(b.is_empty() || a.is_empty()), "panic4");` error: only a `panic!` in `if`-then statement --> $DIR/if_then_panic.rs:51:5 @@ -54,7 +54,7 @@ error: only a `panic!` in `if`-then statement LL | / if a.is_empty() || !b.is_empty() { LL | | panic!("panic5"); LL | | } - | |_____^ help: try: `assert!(!a.is_empty() && b.is_empty(), "panic5");` + | |_____^ help: try: `assert!(!(a.is_empty() || !b.is_empty()), "panic5");` error: aborting due to 7 previous errors