From 90117a03b31dc9ea289cd90bdce3ca5a78019491 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ali=C3=A9nore=20Bouttefeux?= Date: Wed, 1 Sep 2021 11:31:17 +0200 Subject: [PATCH 1/8] =?UTF-8?q?=EF=BB=BFimprove=20suggestion=20for=20bool?= =?UTF-8?q?=5Fassert=5Fcomparaison?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- clippy_lints/src/bool_assert_comparison.rs | 64 ++++++++++++++++------ tests/ui/bool_assert_comparison.stderr | 44 +++++++-------- 2 files changed, 68 insertions(+), 40 deletions(-) diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs index cdc192a47e48..a468479d0ff0 100644 --- a/clippy_lints/src/bool_assert_comparison.rs +++ b/clippy_lints/src/bool_assert_comparison.rs @@ -1,4 +1,4 @@ -use clippy_utils::{diagnostics::span_lint_and_sugg, higher, is_direct_expn_of, ty::implements_trait}; +use clippy_utils::{diagnostics::span_lint_and_sugg, higher, is_direct_expn_of, source, ty::implements_trait}; use rustc_ast::ast::LitKind; use rustc_errors::Applicability; use rustc_hir::{Expr, ExprKind, Lit}; @@ -30,14 +30,19 @@ declare_clippy_lint! { declare_lint_pass!(BoolAssertComparison => [BOOL_ASSERT_COMPARISON]); -fn is_bool_lit(e: &Expr<'_>) -> bool { - matches!( - e.kind, +fn bool_lit(e: &Expr<'_>) -> Option { + match e.kind { ExprKind::Lit(Lit { - node: LitKind::Bool(_), - .. - }) - ) && !e.span.from_expansion() + node: LitKind::Bool(b), .. + }) => { + if e.span.from_expansion() { + None + } else { + Some(b) + } + }, + _ => None, + } } fn is_impl_not_trait_with_bool_out(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool { @@ -68,19 +73,27 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison { let macros = ["assert_eq", "debug_assert_eq"]; let inverted_macros = ["assert_ne", "debug_assert_ne"]; - for mac in macros.iter().chain(inverted_macros.iter()) { + for (mac, is_eq) in macros + .iter() + .map(|el| (el, true)) + .chain(inverted_macros.iter().map(|el| (el, false))) + { if let Some(span) = is_direct_expn_of(expr.span, mac) { if let Some(args) = higher::extract_assert_macro_args(expr) { if let [a, b, ..] = args[..] { - let nb_bool_args = is_bool_lit(a) as usize + is_bool_lit(b) as usize; + //let nb_bool_args = is_bool_lit(a) as usize + is_bool_lit(b) as usize; - if nb_bool_args != 1 { - // If there are two boolean arguments, we definitely don't understand - // what's going on, so better leave things as is... - // - // Or there is simply no boolean and then we can leave things as is! - return; - } + let (lit_value, other_expr) = match (bool_lit(a), bool_lit(b)) { + (Some(lit), None) => (lit, b), + (None, Some(lit)) => (lit, a), + _ => { + // If there are two boolean arguments, we definitely don't understand + // what's going on, so better leave things as is... + // + // Or there is simply no boolean and then we can leave things as is! + return; + }, + }; if !is_impl_not_trait_with_bool_out(cx, a) || !is_impl_not_trait_with_bool_out(cx, b) { // At this point the expression which is not a boolean @@ -90,13 +103,28 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison { } let non_eq_mac = &mac[..mac.len() - 3]; + let expr_string = if lit_value ^ is_eq { + format!("!({})", source::snippet(cx, other_expr.span, "")) + } else { + source::snippet(cx, other_expr.span, "").to_string() + }; + + let suggestion = if args.len() <= 2 { + format!("{}!({})", non_eq_mac, expr_string) + } else { + let mut str = format!("{}!({}", non_eq_mac, expr_string); + for el in &args[2..] { + str = format!("{}, {}", str, source::snippet(cx, el.span, "")); + } + format!("{})", str) + }; span_lint_and_sugg( cx, BOOL_ASSERT_COMPARISON, span, &format!("used `{}!` with a literal bool", mac), "replace it with", - format!("{}!(..)", non_eq_mac), + suggestion, Applicability::MaybeIncorrect, ); return; diff --git a/tests/ui/bool_assert_comparison.stderr b/tests/ui/bool_assert_comparison.stderr index 377d51be4cde..a90009a421f3 100644 --- a/tests/ui/bool_assert_comparison.stderr +++ b/tests/ui/bool_assert_comparison.stderr @@ -2,7 +2,7 @@ error: used `assert_eq!` with a literal bool --> $DIR/bool_assert_comparison.rs:69:5 | LL | assert_eq!("a".is_empty(), false); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("a".is_empty()))` | = note: `-D clippy::bool-assert-comparison` implied by `-D warnings` @@ -10,127 +10,127 @@ error: used `assert_eq!` with a literal bool --> $DIR/bool_assert_comparison.rs:70:5 | LL | assert_eq!("".is_empty(), true); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!("".is_empty())` error: used `assert_eq!` with a literal bool --> $DIR/bool_assert_comparison.rs:71:5 | LL | assert_eq!(true, "".is_empty()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!("".is_empty())` error: used `assert_eq!` with a literal bool --> $DIR/bool_assert_comparison.rs:76:5 | LL | assert_eq!(b, true); - | ^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + | ^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(b)` error: used `assert_ne!` with a literal bool --> $DIR/bool_assert_comparison.rs:79:5 | LL | assert_ne!("a".is_empty(), false); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!("a".is_empty())` error: used `assert_ne!` with a literal bool --> $DIR/bool_assert_comparison.rs:80:5 | LL | assert_ne!("".is_empty(), true); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("".is_empty()))` error: used `assert_ne!` with a literal bool --> $DIR/bool_assert_comparison.rs:81:5 | LL | assert_ne!(true, "".is_empty()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("".is_empty()))` error: used `assert_ne!` with a literal bool --> $DIR/bool_assert_comparison.rs:86:5 | LL | assert_ne!(b, true); - | ^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + | ^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!(b))` error: used `debug_assert_eq!` with a literal bool --> $DIR/bool_assert_comparison.rs:89:5 | LL | debug_assert_eq!("a".is_empty(), false); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("a".is_empty()))` error: used `debug_assert_eq!` with a literal bool --> $DIR/bool_assert_comparison.rs:90:5 | LL | debug_assert_eq!("".is_empty(), true); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!("".is_empty())` error: used `debug_assert_eq!` with a literal bool --> $DIR/bool_assert_comparison.rs:91:5 | LL | debug_assert_eq!(true, "".is_empty()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!("".is_empty())` error: used `debug_assert_eq!` with a literal bool --> $DIR/bool_assert_comparison.rs:96:5 | LL | debug_assert_eq!(b, true); - | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(b)` error: used `debug_assert_ne!` with a literal bool --> $DIR/bool_assert_comparison.rs:99:5 | LL | debug_assert_ne!("a".is_empty(), false); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!("a".is_empty())` error: used `debug_assert_ne!` with a literal bool --> $DIR/bool_assert_comparison.rs:100:5 | LL | debug_assert_ne!("".is_empty(), true); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("".is_empty()))` error: used `debug_assert_ne!` with a literal bool --> $DIR/bool_assert_comparison.rs:101:5 | LL | debug_assert_ne!(true, "".is_empty()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("".is_empty()))` error: used `debug_assert_ne!` with a literal bool --> $DIR/bool_assert_comparison.rs:106:5 | LL | debug_assert_ne!(b, true); - | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!(b))` error: used `assert_eq!` with a literal bool --> $DIR/bool_assert_comparison.rs:111:5 | LL | assert_eq!("a".is_empty(), false, "tadam {}", 1); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("a".is_empty()))` error: used `assert_eq!` with a literal bool --> $DIR/bool_assert_comparison.rs:112:5 | LL | assert_eq!("a".is_empty(), false, "tadam {}", true); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("a".is_empty()))` error: used `assert_eq!` with a literal bool --> $DIR/bool_assert_comparison.rs:113:5 | LL | assert_eq!(false, "a".is_empty(), "tadam {}", true); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("a".is_empty()))` error: used `debug_assert_eq!` with a literal bool --> $DIR/bool_assert_comparison.rs:118:5 | LL | debug_assert_eq!("a".is_empty(), false, "tadam {}", 1); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("a".is_empty()))` error: used `debug_assert_eq!` with a literal bool --> $DIR/bool_assert_comparison.rs:119:5 | LL | debug_assert_eq!("a".is_empty(), false, "tadam {}", true); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("a".is_empty()))` error: used `debug_assert_eq!` with a literal bool --> $DIR/bool_assert_comparison.rs:120:5 | LL | debug_assert_eq!(false, "a".is_empty(), "tadam {}", true); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(..)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("a".is_empty()))` error: aborting due to 22 previous errors From e696ba268e9adcaa9fffdcdf56dd816825edd6e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ali=C3=A9nore=20Bouttefeux?= Date: Mon, 6 Sep 2021 13:52:12 +0200 Subject: [PATCH 2/8] =?UTF-8?q?=EF=BB=BFadd=20extraction=20of=20fmt=20para?= =?UTF-8?q?meter?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- clippy_utils/src/higher.rs | 34 ++++++++++++++++++++++++-- tests/ui/bool_assert_comparison.rs | 2 ++ tests/ui/bool_assert_comparison.stderr | 32 ++++++++++++++++-------- 3 files changed, 56 insertions(+), 12 deletions(-) diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs index 60c4cb361aa6..7005a7d4332f 100644 --- a/clippy_utils/src/higher.rs +++ b/clippy_utils/src/higher.rs @@ -430,12 +430,42 @@ pub fn extract_assert_macro_args<'tcx>(e: &'tcx Expr<'tcx>) -> Option) -> Option>> { if_chain! { - if let ExprKind::Match(headerexpr, _, _) = &matchblock_expr.kind; + if let ExprKind::Match(headerexpr, arms, _) = &matchblock_expr.kind; if let ExprKind::Tup([lhs, rhs]) = &headerexpr.kind; if let ExprKind::AddrOf(BorrowKind::Ref, _, lhs) = lhs.kind; if let ExprKind::AddrOf(BorrowKind::Ref, _, rhs) = rhs.kind; then { - return Some(vec![lhs, rhs]); + let mut vec_arg = vec![lhs, rhs]; + if_chain! { + if !arms.is_empty(); + if let ExprKind::Block(Block{expr: Some(if_expr),..},_) = arms[0].body.kind; + if let ExprKind::If(_, if_block, _) = if_expr.kind; + if let ExprKind::Block(Block{stmts: stmts_if_block,..},_) = if_block.kind; + if stmts_if_block.len() >= 2; + if let StmtKind::Expr(call_assert_failed) + | StmtKind::Semi(call_assert_failed) = stmts_if_block[1].kind; + if let ExprKind::Call(_, args_assert_failed) = call_assert_failed.kind; + if args_assert_failed.len() >= 4; + if let ExprKind::Call(_, args) = args_assert_failed[3].kind; + if !args.is_empty(); + if let ExprKind::Call(_, args_fmt) = args[0].kind; + if !args_fmt.is_empty(); + then { + vec_arg.push(&args_fmt[0]); + if_chain! { + if args_fmt.len() >= 2; + if let ExprKind::AddrOf(_, _, expr_match) = args_fmt[1].kind; + if let ExprKind::Match(tup_match, _, _) = expr_match.kind; + if let ExprKind::Tup(tup_args_list) = tup_match.kind; + then{ + for arg in tup_args_list { + vec_arg.push(arg); + } + } + } + } + } + return Some(vec_arg); } } None diff --git a/tests/ui/bool_assert_comparison.rs b/tests/ui/bool_assert_comparison.rs index ec4d6f3ff840..0d0f27874eec 100644 --- a/tests/ui/bool_assert_comparison.rs +++ b/tests/ui/bool_assert_comparison.rs @@ -112,6 +112,7 @@ fn main() { assert_eq!("a".is_empty(), false, "tadam {}", true); assert_eq!(false, "a".is_empty(), "tadam {}", true); assert_eq!(a, true, "tadam {}", false); + assert_eq!("a".is_empty(), true, "tadam {} {}", false, 6); debug_assert_eq!("a".len(), 1, "tadam {}", 1); debug_assert_eq!("a".len(), 1, "tadam {}", true); @@ -119,4 +120,5 @@ fn main() { debug_assert_eq!("a".is_empty(), false, "tadam {}", true); debug_assert_eq!(false, "a".is_empty(), "tadam {}", true); debug_assert_eq!(a, true, "tadam {}", false); + debug_assert_eq!("a".is_empty(), true, "tadam {} {}", false, "b"); } diff --git a/tests/ui/bool_assert_comparison.stderr b/tests/ui/bool_assert_comparison.stderr index a90009a421f3..11c7e6a687fe 100644 --- a/tests/ui/bool_assert_comparison.stderr +++ b/tests/ui/bool_assert_comparison.stderr @@ -100,37 +100,49 @@ error: used `assert_eq!` with a literal bool --> $DIR/bool_assert_comparison.rs:111:5 | LL | assert_eq!("a".is_empty(), false, "tadam {}", 1); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("a".is_empty()))` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("a".is_empty()), "tadam {}", 1)` error: used `assert_eq!` with a literal bool --> $DIR/bool_assert_comparison.rs:112:5 | LL | assert_eq!("a".is_empty(), false, "tadam {}", true); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("a".is_empty()))` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("a".is_empty()), "tadam {}", true)` error: used `assert_eq!` with a literal bool --> $DIR/bool_assert_comparison.rs:113:5 | LL | assert_eq!(false, "a".is_empty(), "tadam {}", true); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("a".is_empty()))` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("a".is_empty()), "tadam {}", true)` + +error: used `assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:115:5 + | +LL | assert_eq!("a".is_empty(), true, "tadam {} {}", false, 6); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!("a".is_empty(), "tadam {} {}", false, 6)` error: used `debug_assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:118:5 + --> $DIR/bool_assert_comparison.rs:119:5 | LL | debug_assert_eq!("a".is_empty(), false, "tadam {}", 1); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("a".is_empty()))` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("a".is_empty()), "tadam {}", 1)` error: used `debug_assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:119:5 + --> $DIR/bool_assert_comparison.rs:120:5 | LL | debug_assert_eq!("a".is_empty(), false, "tadam {}", true); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("a".is_empty()))` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("a".is_empty()), "tadam {}", true)` error: used `debug_assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:120:5 + --> $DIR/bool_assert_comparison.rs:121:5 | LL | debug_assert_eq!(false, "a".is_empty(), "tadam {}", true); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("a".is_empty()))` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("a".is_empty()), "tadam {}", true)` + +error: used `debug_assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:123:5 + | +LL | debug_assert_eq!("a".is_empty(), true, "tadam {} {}", false, "b"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!("a".is_empty(), "tadam {} {}", false, "b")` -error: aborting due to 22 previous errors +error: aborting due to 24 previous errors From 3d17ede7a6223a8af8ea16f4edb46101d376babd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ali=C3=A9nore=20Bouttefeux?= Date: Tue, 21 Sep 2021 12:49:23 +0200 Subject: [PATCH 3/8] =?UTF-8?q?=EF=BB=BFextract=20the=20formaing=20string?= =?UTF-8?q?=20and=20the=20values?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- clippy_utils/src/higher.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs index 7005a7d4332f..5a8d6a17cb17 100644 --- a/clippy_utils/src/higher.rs +++ b/clippy_utils/src/higher.rs @@ -534,6 +534,8 @@ impl FormatExpn<'tcx> { /// A parsed `format_args!` expansion pub struct FormatArgsExpn<'tcx> { + /// The fist argument, the fromat string, as an expr + pub format_string: &'tcx Expr<'tcx>, /// Span of the first argument, the format string pub format_string_span: Span, /// Values passed after the format string @@ -590,6 +592,7 @@ impl FormatArgsExpn<'tcx> { if let ExprKind::Array(args) = arm.body.kind; then { Some(FormatArgsExpn { + format_string:strs_ref, format_string_span: strs_ref.span, value_args, format_string_parts, From b0796609495f5c3dfa9b84f33fe0b07bdbc59ff9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ali=C3=A9nore=20Bouttefeux?= Date: Wed, 22 Sep 2021 15:36:07 +0200 Subject: [PATCH 4/8] =?UTF-8?q?=EF=BB=BFchange=20to=20a=20structure=20for?= =?UTF-8?q?=20parsing=20assert=20macro?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- clippy_lints/src/bool_assert_comparison.rs | 43 ++++-- clippy_lints/src/eq_op.rs | 4 +- clippy_lints/src/mutable_debug_assertion.rs | 4 +- clippy_utils/src/higher.rs | 136 +++++++++++-------- tests/ui/bool_assert_comparison.fixed | 138 ++++++++++++++++++++ tests/ui/bool_assert_comparison.rs | 20 ++- tests/ui/bool_assert_comparison.stderr | 56 ++++---- 7 files changed, 305 insertions(+), 96 deletions(-) create mode 100644 tests/ui/bool_assert_comparison.fixed diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs index a468479d0ff0..02232278e06f 100644 --- a/clippy_lints/src/bool_assert_comparison.rs +++ b/clippy_lints/src/bool_assert_comparison.rs @@ -1,4 +1,6 @@ -use clippy_utils::{diagnostics::span_lint_and_sugg, higher, is_direct_expn_of, source, ty::implements_trait}; +use clippy_utils::{ + diagnostics::span_lint_and_sugg, higher::AssertExpn, is_direct_expn_of, source, sugg::Sugg, ty::implements_trait, +}; use rustc_ast::ast::LitKind; use rustc_errors::Applicability; use rustc_hir::{Expr, ExprKind, Lit}; @@ -79,10 +81,8 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison { .chain(inverted_macros.iter().map(|el| (el, false))) { if let Some(span) = is_direct_expn_of(expr.span, mac) { - if let Some(args) = higher::extract_assert_macro_args(expr) { - if let [a, b, ..] = args[..] { - //let nb_bool_args = is_bool_lit(a) as usize + is_bool_lit(b) as usize; - + if let Some(args) = AssertExpn::parse(expr).map(|v| v.argument_vector()) { + if let [a, b, ref fmt_args @ ..] = args[..] { let (lit_value, other_expr) = match (bool_lit(a), bool_lit(b)) { (Some(lit), None) => (lit, b), (None, Some(lit)) => (lit, a), @@ -103,20 +103,37 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison { } let non_eq_mac = &mac[..mac.len() - 3]; + let mut applicability = Applicability::MachineApplicable; let expr_string = if lit_value ^ is_eq { format!("!({})", source::snippet(cx, other_expr.span, "")) } else { source::snippet(cx, other_expr.span, "").to_string() }; - let suggestion = if args.len() <= 2 { - format!("{}!({})", non_eq_mac, expr_string) + let arg_span = match fmt_args { + [] => None, + [a] => Some(format!( + "{}", + Sugg::hir_with_applicability(cx, a, "..", &mut applicability) + )), + _ => { + let mut args = format!( + "{}", + Sugg::hir_with_applicability(cx, fmt_args[0], "..", &mut applicability) + ); + for el in &fmt_args[1..] { + args.push_str(&format!( + ", {}", + Sugg::hir_with_applicability(cx, el, "..", &mut applicability) + )); + } + Some(args) + }, + }; + let suggestion = if let Some(spans) = arg_span { + format!("{}!({}, {})", non_eq_mac, expr_string, spans) } else { - let mut str = format!("{}!({}", non_eq_mac, expr_string); - for el in &args[2..] { - str = format!("{}, {}", str, source::snippet(cx, el.span, "")); - } - format!("{})", str) + format!("{}!({})", non_eq_mac, expr_string) }; span_lint_and_sugg( cx, @@ -125,7 +142,7 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison { &format!("used `{}!` with a literal bool", mac), "replace it with", suggestion, - Applicability::MaybeIncorrect, + applicability, ); return; } diff --git a/clippy_lints/src/eq_op.rs b/clippy_lints/src/eq_op.rs index 655560afd425..d68f377c6267 100644 --- a/clippy_lints/src/eq_op.rs +++ b/clippy_lints/src/eq_op.rs @@ -2,7 +2,7 @@ use clippy_utils::diagnostics::{multispan_sugg, span_lint, span_lint_and_then}; use clippy_utils::source::snippet; use clippy_utils::ty::{implements_trait, is_copy}; use clippy_utils::{ - ast_utils::is_useless_with_eq_exprs, eq_expr_value, higher, in_macro, is_expn_of, is_in_test_function, + ast_utils::is_useless_with_eq_exprs, eq_expr_value, higher::AssertExpn, in_macro, is_expn_of, is_in_test_function, }; use if_chain::if_chain; use rustc_errors::Applicability; @@ -79,7 +79,7 @@ impl<'tcx> LateLintPass<'tcx> for EqOp { if_chain! { if is_expn_of(stmt.span, amn).is_some(); if let StmtKind::Semi(matchexpr) = stmt.kind; - if let Some(macro_args) = higher::extract_assert_macro_args(matchexpr); + if let Some(macro_args) = AssertExpn::parse(matchexpr).map(|v| v.argument_vector()); if macro_args.len() == 2; let (lhs, rhs) = (macro_args[0], macro_args[1]); if eq_expr_value(cx, lhs, rhs); diff --git a/clippy_lints/src/mutable_debug_assertion.rs b/clippy_lints/src/mutable_debug_assertion.rs index ee50891cc310..a240bd2262a2 100644 --- a/clippy_lints/src/mutable_debug_assertion.rs +++ b/clippy_lints/src/mutable_debug_assertion.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint; -use clippy_utils::{higher, is_direct_expn_of}; +use clippy_utils::{higher::AssertExpn, is_direct_expn_of}; use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; use rustc_hir::{BorrowKind, Expr, ExprKind, MatchSource, Mutability}; use rustc_lint::{LateContext, LateLintPass}; @@ -39,7 +39,7 @@ impl<'tcx> LateLintPass<'tcx> for DebugAssertWithMutCall { fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { for dmn in &DEBUG_MACRO_NAMES { if is_direct_expn_of(e.span, dmn).is_some() { - if let Some(macro_args) = higher::extract_assert_macro_args(e) { + if let Some(macro_args) = AssertExpn::parse(e).map(|v| v.argument_vector()) { for arg in macro_args { let mut visitor = MutArgVisitor::new(cx); visitor.visit_expr(arg); diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs index 5a8d6a17cb17..f5afcde5ec7f 100644 --- a/clippy_utils/src/higher.rs +++ b/clippy_utils/src/higher.rs @@ -418,24 +418,73 @@ pub fn binop(op: hir::BinOpKind) -> ast::BinOpKind { } } -/// Extract args from an assert-like macro. -/// Currently working with: -/// - `assert!`, `assert_eq!` and `assert_ne!` -/// - `debug_assert!`, `debug_assert_eq!` and `debug_assert_ne!` -/// For example: -/// `assert!(expr)` will return `Some([expr])` -/// `debug_assert_eq!(a, b)` will return `Some([a, b])` -pub fn extract_assert_macro_args<'tcx>(e: &'tcx Expr<'tcx>) -> Option>> { +/// A parsed +/// assert!`, `assert_eq!` or `assert_ne!`, +/// debug_assert!`, `debug_assert_eq!` or `debug_assert_ne!` +/// macro. +pub struct AssertExpn<'tcx> { + /// the first agrument of the assret e.g. `var` in element `assert!(var, ...)` + pub first_assert_argument: &'tcx Expr<'tcx>, + /// second argument of the asset for case `assert_eq!`, + /// `assert_ne!` etc ... Eg var_2 in `debug_assert_eq!(x, var_2,..)` + pub second_assert_argument: Option<&'tcx Expr<'tcx>>, + /// The format argument passed at the end of the macro + pub format_arg: Option>, +} + +impl<'tcx> AssertExpn<'tcx> { + /// Extract args from an assert-like macro. + /// Currently working with: + /// - `assert!`, `assert_eq!` and `assert_ne!` + /// - `debug_assert!`, `debug_assert_eq!` and `debug_assert_ne!` + /// For example: + /// `assert!(expr)` will return `Some(AssertExpn { first_assert_argument: expr, + /// second_assert_argument: None, format_arg:None })` `debug_assert_eq!(a, b)` will return + /// `Some(AssertExpn { first_assert_argument: a, second_assert_argument: Some(b), + /// format_arg:None })` + pub fn parse(e: &'tcx Expr<'tcx>) -> Option { + if let ExprKind::Block(block, _) = e.kind { + if block.stmts.len() == 1 { + if let StmtKind::Semi(matchexpr) = block.stmts.get(0)?.kind { + // macros with unique arg: `{debug_}assert!` (e.g., `debug_assert!(some_condition)`) + if_chain! { + if let Some(If { cond, .. }) = If::hir(matchexpr); + if let ExprKind::Unary(UnOp::Not, condition) = cond.kind; + then { + return Some(Self { + first_assert_argument: condition, + second_assert_argument: None, + format_arg: None, // FIXME actually parse the aguments + }); + } + } + + // debug macros with two args: `debug_assert_{ne, eq}` (e.g., `assert_ne!(a, b)`) + if_chain! { + if let ExprKind::Block(matchblock,_) = matchexpr.kind; + if let Some(matchblock_expr) = matchblock.expr; + then { + return Self::ast_matchblock(matchblock_expr); + } + } + } + } else if let Some(matchblock_expr) = block.expr { + // macros with two args: `assert_{ne, eq}` (e.g., `assert_ne!(a, b)`) + return Self::ast_matchblock(matchblock_expr); + } + } + None + } + /// Try to match the AST for a pattern that contains a match, for example when two args are /// compared - fn ast_matchblock(matchblock_expr: &'tcx Expr<'tcx>) -> Option>> { + fn ast_matchblock(matchblock_expr: &'tcx Expr<'tcx>) -> Option { if_chain! { if let ExprKind::Match(headerexpr, arms, _) = &matchblock_expr.kind; if let ExprKind::Tup([lhs, rhs]) = &headerexpr.kind; if let ExprKind::AddrOf(BorrowKind::Ref, _, lhs) = lhs.kind; if let ExprKind::AddrOf(BorrowKind::Ref, _, rhs) = rhs.kind; then { - let mut vec_arg = vec![lhs, rhs]; if_chain! { if !arms.is_empty(); if let ExprKind::Block(Block{expr: Some(if_expr),..},_) = arms[0].body.kind; @@ -446,58 +495,43 @@ pub fn extract_assert_macro_args<'tcx>(e: &'tcx Expr<'tcx>) -> Option= 4; - if let ExprKind::Call(_, args) = args_assert_failed[3].kind; - if !args.is_empty(); - if let ExprKind::Call(_, args_fmt) = args[0].kind; - if !args_fmt.is_empty(); + if let ExprKind::Call(_, [arg, ..]) = args_assert_failed[3].kind; + if let Some(format_arg_expn) = FormatArgsExpn::parse(&arg); then { - vec_arg.push(&args_fmt[0]); - if_chain! { - if args_fmt.len() >= 2; - if let ExprKind::AddrOf(_, _, expr_match) = args_fmt[1].kind; - if let ExprKind::Match(tup_match, _, _) = expr_match.kind; - if let ExprKind::Tup(tup_args_list) = tup_match.kind; - then{ - for arg in tup_args_list { - vec_arg.push(arg); - } - } - } + return Some(AssertExpn { + first_assert_argument: lhs, + second_assert_argument: Some(rhs), + format_arg: Some(format_arg_expn) + }); + } + else { + return Some(AssertExpn { + first_assert_argument: lhs, + second_assert_argument: + Some(rhs), + format_arg: None + }); } } - return Some(vec_arg); } } None } - if let ExprKind::Block(block, _) = e.kind { - if block.stmts.len() == 1 { - if let StmtKind::Semi(matchexpr) = block.stmts.get(0)?.kind { - // macros with unique arg: `{debug_}assert!` (e.g., `debug_assert!(some_condition)`) - if_chain! { - if let Some(If { cond, .. }) = If::hir(matchexpr); - if let ExprKind::Unary(UnOp::Not, condition) = cond.kind; - then { - return Some(vec![condition]); - } - } - - // debug macros with two args: `debug_assert_{ne, eq}` (e.g., `assert_ne!(a, b)`) - if_chain! { - if let ExprKind::Block(matchblock,_) = matchexpr.kind; - if let Some(matchblock_expr) = matchblock.expr; - then { - return ast_matchblock(matchblock_expr); - } - } + /// Gives the argument as a vector + pub fn argument_vector(&self) -> Vec<&'tcx Expr<'tcx>> { + let mut expr_vec = vec![self.first_assert_argument]; + if let Some(sec_agr) = self.second_assert_argument { + expr_vec.push(sec_agr); + } + if let Some(ref format_arg) = self.format_arg { + expr_vec.push(format_arg.format_string); + for arg in &format_arg.value_args { + expr_vec.push(arg) } - } else if let Some(matchblock_expr) = block.expr { - // macros with two args: `assert_{ne, eq}` (e.g., `assert_ne!(a, b)`) - return ast_matchblock(matchblock_expr); } + expr_vec } - None } /// A parsed `format!` expansion diff --git a/tests/ui/bool_assert_comparison.fixed b/tests/ui/bool_assert_comparison.fixed new file mode 100644 index 000000000000..e82959b10c3a --- /dev/null +++ b/tests/ui/bool_assert_comparison.fixed @@ -0,0 +1,138 @@ +// run-rustfix + +#![warn(clippy::bool_assert_comparison)] + +use std::ops::Not; + +macro_rules! a { + () => { + true + }; +} +macro_rules! b { + () => { + true + }; +} + +// Implements the Not trait but with an output type +// that's not bool. Should not suggest a rewrite +#[derive(Debug, Copy, Clone)] +#[allow(dead_code)] +enum ImplNotTraitWithoutBool { + VariantX(bool), + VariantY(u32), +} + +impl PartialEq for ImplNotTraitWithoutBool { + fn eq(&self, other: &bool) -> bool { + match *self { + ImplNotTraitWithoutBool::VariantX(b) => b == *other, + _ => false, + } + } +} + +impl Not for ImplNotTraitWithoutBool { + type Output = Self; + + fn not(self) -> Self::Output { + match self { + ImplNotTraitWithoutBool::VariantX(b) => ImplNotTraitWithoutBool::VariantX(!b), + ImplNotTraitWithoutBool::VariantY(0) => ImplNotTraitWithoutBool::VariantY(1), + ImplNotTraitWithoutBool::VariantY(_) => ImplNotTraitWithoutBool::VariantY(0), + } + } +} + +// This type implements the Not trait with an Output of +// type bool. Using assert!(..) must be suggested +#[derive(Debug, Clone, Copy)] +struct ImplNotTraitWithBool; + +impl PartialEq for ImplNotTraitWithBool { + fn eq(&self, _other: &bool) -> bool { + false + } +} + +impl Not for ImplNotTraitWithBool { + type Output = bool; + + fn not(self) -> Self::Output { + true + } +} + +fn main() { + let a = ImplNotTraitWithoutBool::VariantX(true); + let b = ImplNotTraitWithBool; + + assert_eq!("a".len(), 1); + assert!(!("a".is_empty())); + assert!("".is_empty()); + assert!("".is_empty()); + assert_eq!(a!(), b!()); + assert_eq!(a!(), "".is_empty()); + assert_eq!("".is_empty(), b!()); + assert_eq!(a, true); + assert!(b); + + assert_ne!("a".len(), 1); + assert!("a".is_empty()); + assert!(!("".is_empty())); + assert!(!("".is_empty())); + assert_ne!(a!(), b!()); + assert_ne!(a!(), "".is_empty()); + assert_ne!("".is_empty(), b!()); + assert_ne!(a, true); + assert!(!(b)); + + debug_assert_eq!("a".len(), 1); + debug_assert!(!("a".is_empty())); + debug_assert!("".is_empty()); + debug_assert!("".is_empty()); + debug_assert_eq!(a!(), b!()); + debug_assert_eq!(a!(), "".is_empty()); + debug_assert_eq!("".is_empty(), b!()); + debug_assert_eq!(a, true); + debug_assert!(b); + + debug_assert_ne!("a".len(), 1); + debug_assert!("a".is_empty()); + debug_assert!(!("".is_empty())); + debug_assert!(!("".is_empty())); + debug_assert_ne!(a!(), b!()); + debug_assert_ne!(a!(), "".is_empty()); + debug_assert_ne!("".is_empty(), b!()); + debug_assert_ne!(a, true); + debug_assert!(!(b)); + + // assert with error messages + assert_eq!("a".len(), 1, "tadam {}", 1); + assert_eq!("a".len(), 1, "tadam {}", true); + assert!(!("a".is_empty()), "tadam {}", 1); + assert!(!("a".is_empty()), "tadam {}", true); + assert!(!("a".is_empty()), "tadam {}", true); + assert_eq!(a, true, "tadam {}", false); + assert!("a".is_empty(), "tadam {} {}", false, 6); + + debug_assert_eq!("a".len(), 1, "tadam {}", 1); + debug_assert_eq!("a".len(), 1, "tadam {}", true); + debug_assert!(!("a".is_empty()), "tadam {}", 1); + debug_assert!(!("a".is_empty()), "tadam {}", true); + debug_assert!(!("a".is_empty()), "tadam {}", true); + debug_assert_eq!(a, true, "tadam {}", false); + debug_assert!("a".is_empty(), "tadam {} {}", false, "b"); + + // Without ; at the end + { + debug_assert!(!("a".is_empty()), "tadam {}", true) + }; + { + assert_eq!("a".len(), 1, "tadam {}", 1) + }; + { + assert_ne!("a".len(), 1, "tadam {}", true) + }; +} diff --git a/tests/ui/bool_assert_comparison.rs b/tests/ui/bool_assert_comparison.rs index 0d0f27874eec..2a84cca7cd8f 100644 --- a/tests/ui/bool_assert_comparison.rs +++ b/tests/ui/bool_assert_comparison.rs @@ -1,3 +1,5 @@ +// run-rustfix + #![warn(clippy::bool_assert_comparison)] use std::ops::Not; @@ -15,7 +17,8 @@ macro_rules! b { // Implements the Not trait but with an output type // that's not bool. Should not suggest a rewrite -#[derive(Debug)] +#[derive(Debug, Copy, Clone)] +#[allow(dead_code)] enum ImplNotTraitWithoutBool { VariantX(bool), VariantY(u32), @@ -44,11 +47,11 @@ impl Not for ImplNotTraitWithoutBool { // This type implements the Not trait with an Output of // type bool. Using assert!(..) must be suggested -#[derive(Debug)] +#[derive(Debug, Clone, Copy)] struct ImplNotTraitWithBool; impl PartialEq for ImplNotTraitWithBool { - fn eq(&self, other: &bool) -> bool { + fn eq(&self, _other: &bool) -> bool { false } } @@ -121,4 +124,15 @@ fn main() { debug_assert_eq!(false, "a".is_empty(), "tadam {}", true); debug_assert_eq!(a, true, "tadam {}", false); debug_assert_eq!("a".is_empty(), true, "tadam {} {}", false, "b"); + + // Without ; at the end + { + debug_assert_eq!(false, "a".is_empty(), "tadam {}", true) + }; + { + assert_eq!("a".len(), 1, "tadam {}", 1) + }; + { + assert_ne!("a".len(), 1, "tadam {}", true) + }; } diff --git a/tests/ui/bool_assert_comparison.stderr b/tests/ui/bool_assert_comparison.stderr index 11c7e6a687fe..2f76f9d64b9e 100644 --- a/tests/ui/bool_assert_comparison.stderr +++ b/tests/ui/bool_assert_comparison.stderr @@ -1,5 +1,5 @@ error: used `assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:69:5 + --> $DIR/bool_assert_comparison.rs:72:5 | LL | assert_eq!("a".is_empty(), false); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("a".is_empty()))` @@ -7,142 +7,148 @@ LL | assert_eq!("a".is_empty(), false); = note: `-D clippy::bool-assert-comparison` implied by `-D warnings` error: used `assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:70:5 + --> $DIR/bool_assert_comparison.rs:73:5 | LL | assert_eq!("".is_empty(), true); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!("".is_empty())` error: used `assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:71:5 + --> $DIR/bool_assert_comparison.rs:74:5 | LL | assert_eq!(true, "".is_empty()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!("".is_empty())` error: used `assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:76:5 + --> $DIR/bool_assert_comparison.rs:79:5 | LL | assert_eq!(b, true); | ^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(b)` error: used `assert_ne!` with a literal bool - --> $DIR/bool_assert_comparison.rs:79:5 + --> $DIR/bool_assert_comparison.rs:82:5 | LL | assert_ne!("a".is_empty(), false); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!("a".is_empty())` error: used `assert_ne!` with a literal bool - --> $DIR/bool_assert_comparison.rs:80:5 + --> $DIR/bool_assert_comparison.rs:83:5 | LL | assert_ne!("".is_empty(), true); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("".is_empty()))` error: used `assert_ne!` with a literal bool - --> $DIR/bool_assert_comparison.rs:81:5 + --> $DIR/bool_assert_comparison.rs:84:5 | LL | assert_ne!(true, "".is_empty()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("".is_empty()))` error: used `assert_ne!` with a literal bool - --> $DIR/bool_assert_comparison.rs:86:5 + --> $DIR/bool_assert_comparison.rs:89:5 | LL | assert_ne!(b, true); | ^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!(b))` error: used `debug_assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:89:5 + --> $DIR/bool_assert_comparison.rs:92:5 | LL | debug_assert_eq!("a".is_empty(), false); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("a".is_empty()))` error: used `debug_assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:90:5 + --> $DIR/bool_assert_comparison.rs:93:5 | LL | debug_assert_eq!("".is_empty(), true); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!("".is_empty())` error: used `debug_assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:91:5 + --> $DIR/bool_assert_comparison.rs:94:5 | LL | debug_assert_eq!(true, "".is_empty()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!("".is_empty())` error: used `debug_assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:96:5 + --> $DIR/bool_assert_comparison.rs:99:5 | LL | debug_assert_eq!(b, true); | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(b)` error: used `debug_assert_ne!` with a literal bool - --> $DIR/bool_assert_comparison.rs:99:5 + --> $DIR/bool_assert_comparison.rs:102:5 | LL | debug_assert_ne!("a".is_empty(), false); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!("a".is_empty())` error: used `debug_assert_ne!` with a literal bool - --> $DIR/bool_assert_comparison.rs:100:5 + --> $DIR/bool_assert_comparison.rs:103:5 | LL | debug_assert_ne!("".is_empty(), true); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("".is_empty()))` error: used `debug_assert_ne!` with a literal bool - --> $DIR/bool_assert_comparison.rs:101:5 + --> $DIR/bool_assert_comparison.rs:104:5 | LL | debug_assert_ne!(true, "".is_empty()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("".is_empty()))` error: used `debug_assert_ne!` with a literal bool - --> $DIR/bool_assert_comparison.rs:106:5 + --> $DIR/bool_assert_comparison.rs:109:5 | LL | debug_assert_ne!(b, true); | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!(b))` error: used `assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:111:5 + --> $DIR/bool_assert_comparison.rs:114:5 | LL | assert_eq!("a".is_empty(), false, "tadam {}", 1); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("a".is_empty()), "tadam {}", 1)` error: used `assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:112:5 + --> $DIR/bool_assert_comparison.rs:115:5 | LL | assert_eq!("a".is_empty(), false, "tadam {}", true); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("a".is_empty()), "tadam {}", true)` error: used `assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:113:5 + --> $DIR/bool_assert_comparison.rs:116:5 | LL | assert_eq!(false, "a".is_empty(), "tadam {}", true); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!(!("a".is_empty()), "tadam {}", true)` error: used `assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:115:5 + --> $DIR/bool_assert_comparison.rs:118:5 | LL | assert_eq!("a".is_empty(), true, "tadam {} {}", false, 6); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `assert!("a".is_empty(), "tadam {} {}", false, 6)` error: used `debug_assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:119:5 + --> $DIR/bool_assert_comparison.rs:122:5 | LL | debug_assert_eq!("a".is_empty(), false, "tadam {}", 1); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("a".is_empty()), "tadam {}", 1)` error: used `debug_assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:120:5 + --> $DIR/bool_assert_comparison.rs:123:5 | LL | debug_assert_eq!("a".is_empty(), false, "tadam {}", true); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("a".is_empty()), "tadam {}", true)` error: used `debug_assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:121:5 + --> $DIR/bool_assert_comparison.rs:124:5 | LL | debug_assert_eq!(false, "a".is_empty(), "tadam {}", true); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("a".is_empty()), "tadam {}", true)` error: used `debug_assert_eq!` with a literal bool - --> $DIR/bool_assert_comparison.rs:123:5 + --> $DIR/bool_assert_comparison.rs:126:5 | LL | debug_assert_eq!("a".is_empty(), true, "tadam {} {}", false, "b"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!("a".is_empty(), "tadam {} {}", false, "b")` -error: aborting due to 24 previous errors +error: used `debug_assert_eq!` with a literal bool + --> $DIR/bool_assert_comparison.rs:130:9 + | +LL | debug_assert_eq!(false, "a".is_empty(), "tadam {}", true) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `debug_assert!(!("a".is_empty()), "tadam {}", true)` + +error: aborting due to 25 previous errors From 5f5ea419112e5923a0adb00f802dcc245ef5cdb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ali=C3=A9nore=20Bouttefeux?= Date: Wed, 22 Sep 2021 17:15:36 +0200 Subject: [PATCH 5/8] =?UTF-8?q?=EF=BB=BFremove=20the=20format=5Fstring=5Fe?= =?UTF-8?q?xpr=20from=20the=20FormatArgsExpn?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- clippy_lints/src/bool_assert_comparison.rs | 23 ++---- clippy_lints/src/eq_op.rs | 2 +- clippy_lints/src/mutable_debug_assertion.rs | 2 +- clippy_utils/src/higher.rs | 89 ++++++++++++++++----- 4 files changed, 78 insertions(+), 38 deletions(-) diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs index 02232278e06f..c2796e728e62 100644 --- a/clippy_lints/src/bool_assert_comparison.rs +++ b/clippy_lints/src/bool_assert_comparison.rs @@ -81,8 +81,8 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison { .chain(inverted_macros.iter().map(|el| (el, false))) { if let Some(span) = is_direct_expn_of(expr.span, mac) { - if let Some(args) = AssertExpn::parse(expr).map(|v| v.argument_vector()) { - if let [a, b, ref fmt_args @ ..] = args[..] { + if let Some(parse_assert) = AssertExpn::parse(expr) { + if let [a, b] = parse_assert.assert_arguments()[..] { let (lit_value, other_expr) = match (bool_lit(a), bool_lit(b)) { (Some(lit), None) => (lit, b), (None, Some(lit)) => (lit, a), @@ -109,23 +109,14 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison { } else { source::snippet(cx, other_expr.span, "").to_string() }; - - let arg_span = match fmt_args { + let fmt_args = parse_assert.format_arguments(cx, &mut applicability); + let arg_span = match &fmt_args[..] { [] => None, - [a] => Some(format!( - "{}", - Sugg::hir_with_applicability(cx, a, "..", &mut applicability) - )), + [a] => Some(a.to_string()), _ => { - let mut args = format!( - "{}", - Sugg::hir_with_applicability(cx, fmt_args[0], "..", &mut applicability) - ); + let mut args = fmt_args[0].to_string(); for el in &fmt_args[1..] { - args.push_str(&format!( - ", {}", - Sugg::hir_with_applicability(cx, el, "..", &mut applicability) - )); + args.push_str(&format!(", {}", el)); } Some(args) }, diff --git a/clippy_lints/src/eq_op.rs b/clippy_lints/src/eq_op.rs index d68f377c6267..3dfa09ad9edc 100644 --- a/clippy_lints/src/eq_op.rs +++ b/clippy_lints/src/eq_op.rs @@ -79,7 +79,7 @@ impl<'tcx> LateLintPass<'tcx> for EqOp { if_chain! { if is_expn_of(stmt.span, amn).is_some(); if let StmtKind::Semi(matchexpr) = stmt.kind; - if let Some(macro_args) = AssertExpn::parse(matchexpr).map(|v| v.argument_vector()); + if let Some(macro_args) = AssertExpn::parse(matchexpr).map(|v| v.assert_arguments()); if macro_args.len() == 2; let (lhs, rhs) = (macro_args[0], macro_args[1]); if eq_expr_value(cx, lhs, rhs); diff --git a/clippy_lints/src/mutable_debug_assertion.rs b/clippy_lints/src/mutable_debug_assertion.rs index a240bd2262a2..3d87f6ce464c 100644 --- a/clippy_lints/src/mutable_debug_assertion.rs +++ b/clippy_lints/src/mutable_debug_assertion.rs @@ -39,7 +39,7 @@ impl<'tcx> LateLintPass<'tcx> for DebugAssertWithMutCall { fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { for dmn in &DEBUG_MACRO_NAMES { if is_direct_expn_of(e.span, dmn).is_some() { - if let Some(macro_args) = AssertExpn::parse(e).map(|v| v.argument_vector()) { + if let Some(macro_args) = AssertExpn::parse(e).map(|v| v.assert_arguments()) { for arg in macro_args { let mut visitor = MutArgVisitor::new(cx); visitor.visit_expr(arg); diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs index f5afcde5ec7f..3f8467cd9f6a 100644 --- a/clippy_utils/src/higher.rs +++ b/clippy_utils/src/higher.rs @@ -3,9 +3,10 @@ #![deny(clippy::missing_docs_in_private_items)] use crate::ty::is_type_diagnostic_item; -use crate::{is_expn_of, last_path_segment, match_def_path, paths}; +use crate::{is_expn_of, last_path_segment, match_def_path, paths, source::snippet_with_applicability}; use if_chain::if_chain; use rustc_ast::ast::{self, LitKind}; +use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::{ Arm, Block, BorrowKind, Expr, ExprKind, HirId, LoopSource, MatchSource, Node, Pat, QPath, StmtKind, UnOp, @@ -13,6 +14,8 @@ use rustc_hir::{ use rustc_lint::LateContext; use rustc_span::{sym, symbol, ExpnKind, Span, Symbol}; +use std::borrow::Cow; + /// The essential nodes of a desugared for loop as well as the entire span: /// `for pat in arg { body }` becomes `(pat, arg, body)`. Return `(pat, arg, body, span)`. pub struct ForLoop<'tcx> { @@ -419,8 +422,8 @@ pub fn binop(op: hir::BinOpKind) -> ast::BinOpKind { } /// A parsed -/// assert!`, `assert_eq!` or `assert_ne!`, -/// debug_assert!`, `debug_assert_eq!` or `debug_assert_ne!` +/// `assert!`, `assert_eq!` or `assert_ne!`, +/// `debug_assert!`, `debug_assert_eq!` or `debug_assert_ne!` /// macro. pub struct AssertExpn<'tcx> { /// the first agrument of the assret e.g. `var` in element `assert!(var, ...)` @@ -448,14 +451,33 @@ impl<'tcx> AssertExpn<'tcx> { if let StmtKind::Semi(matchexpr) = block.stmts.get(0)?.kind { // macros with unique arg: `{debug_}assert!` (e.g., `debug_assert!(some_condition)`) if_chain! { - if let Some(If { cond, .. }) = If::hir(matchexpr); + if let Some(If { cond, then, .. }) = If::hir(matchexpr); if let ExprKind::Unary(UnOp::Not, condition) = cond.kind; then { - return Some(Self { - first_assert_argument: condition, - second_assert_argument: None, - format_arg: None, // FIXME actually parse the aguments - }); + if_chain! { + if let ExprKind::Block(block, _) = then.kind; + if let [statement, ..] = block.stmts; + if let StmtKind::Expr(call_assert_failed) + | StmtKind::Semi(call_assert_failed) = statement.kind; + if let ExprKind::Call(_, args_assert_failed) = call_assert_failed.kind; + if !args_assert_failed.is_empty(); + if let ExprKind::Call(_, [arg, ..]) = args_assert_failed[0].kind; + if let Some(format_arg_expn) = FormatArgsExpn::parse(arg); + then { + return Some(Self { + first_assert_argument: condition, + second_assert_argument: None, + format_arg: Some(format_arg_expn), // FIXME actually parse the aguments + }); + } + else{ + return Some(Self { + first_assert_argument: condition, + second_assert_argument: None, + format_arg: None, + }); + } + } } } @@ -496,7 +518,7 @@ impl<'tcx> AssertExpn<'tcx> { if let ExprKind::Call(_, args_assert_failed) = call_assert_failed.kind; if args_assert_failed.len() >= 4; if let ExprKind::Call(_, [arg, ..]) = args_assert_failed[3].kind; - if let Some(format_arg_expn) = FormatArgsExpn::parse(&arg); + if let Some(format_arg_expn) = FormatArgsExpn::parse(arg); then { return Some(AssertExpn { first_assert_argument: lhs, @@ -518,19 +540,49 @@ impl<'tcx> AssertExpn<'tcx> { None } - /// Gives the argument as a vector - pub fn argument_vector(&self) -> Vec<&'tcx Expr<'tcx>> { + /// Gives the argument in the comparaison as a vector leaving the format + pub fn assert_arguments(&self) -> Vec<&'tcx Expr<'tcx>> { let mut expr_vec = vec![self.first_assert_argument]; if let Some(sec_agr) = self.second_assert_argument { expr_vec.push(sec_agr); } - if let Some(ref format_arg) = self.format_arg { - expr_vec.push(format_arg.format_string); - for arg in &format_arg.value_args { - expr_vec.push(arg) + expr_vec + } + + /// Gives the argument passed to the macro as a string + pub fn all_arguments_string( + &self, + cx: &LateContext<'_>, + applicability: &mut Applicability, + ) -> Vec> { + let mut vec_arg = vec![snippet_with_applicability( + cx, + self.first_assert_argument.span, + "..", + applicability, + )]; + if let Some(sec_agr) = self.second_assert_argument { + vec_arg.push(snippet_with_applicability(cx, sec_agr.span, "..", applicability)); + } + vec_arg.append(&mut self.format_arguments(cx, applicability)); + vec_arg + } + + /// Returns only the format agruments + pub fn format_arguments(&self, cx: &LateContext<'_>, applicability: &mut Applicability) -> Vec> { + let mut vec_arg = vec![]; + if let Some(ref fmt_arg) = self.format_arg { + vec_arg.push(snippet_with_applicability( + cx, + fmt_arg.format_string_span, + "..", + applicability, + )); + for arg in &fmt_arg.value_args { + vec_arg.push(snippet_with_applicability(cx, arg.span, "..", applicability)); } } - expr_vec + vec_arg } } @@ -568,8 +620,6 @@ impl FormatExpn<'tcx> { /// A parsed `format_args!` expansion pub struct FormatArgsExpn<'tcx> { - /// The fist argument, the fromat string, as an expr - pub format_string: &'tcx Expr<'tcx>, /// Span of the first argument, the format string pub format_string_span: Span, /// Values passed after the format string @@ -626,7 +676,6 @@ impl FormatArgsExpn<'tcx> { if let ExprKind::Array(args) = arm.body.kind; then { Some(FormatArgsExpn { - format_string:strs_ref, format_string_span: strs_ref.span, value_args, format_string_parts, From 256e6c57c8bdf07756da89baa39ab2376e265aac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ali=C3=A9nore=20Bouttefeux?= Date: Fri, 15 Oct 2021 11:29:18 +0200 Subject: [PATCH 6/8] change based on review and debug_assert --- clippy_utils/src/higher.rs | 97 +++++++++++++++++++------------------- 1 file changed, 49 insertions(+), 48 deletions(-) diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs index 3f8467cd9f6a..65b954782ffe 100644 --- a/clippy_utils/src/higher.rs +++ b/clippy_utils/src/higher.rs @@ -421,18 +421,25 @@ pub fn binop(op: hir::BinOpKind) -> ast::BinOpKind { } } +/// Kind of assert macros +pub enum AssertExpnKind<'tcx> { + /// Boolean macro like `assert!` or `debug_assert!` + Bool(&'tcx Expr<'tcx>), + /// Comparaison maacro like `assert_eq!`, `assert_ne!`, `debug_assert_eq!` or `debug_assert_ne!` + Eq(&'tcx Expr<'tcx>, &'tcx Expr<'tcx>), +} + /// A parsed /// `assert!`, `assert_eq!` or `assert_ne!`, /// `debug_assert!`, `debug_assert_eq!` or `debug_assert_ne!` /// macro. pub struct AssertExpn<'tcx> { - /// the first agrument of the assret e.g. `var` in element `assert!(var, ...)` - pub first_assert_argument: &'tcx Expr<'tcx>, - /// second argument of the asset for case `assert_eq!`, - /// `assert_ne!` etc ... Eg var_2 in `debug_assert_eq!(x, var_2,..)` - pub second_assert_argument: Option<&'tcx Expr<'tcx>>, + /// Kind of assert macro + pub kind: AssertExpnKind<'tcx>, /// The format argument passed at the end of the macro pub format_arg: Option>, + /// is a debug macro + pub is_debug: bool, } impl<'tcx> AssertExpn<'tcx> { @@ -445,39 +452,38 @@ impl<'tcx> AssertExpn<'tcx> { /// second_assert_argument: None, format_arg:None })` `debug_assert_eq!(a, b)` will return /// `Some(AssertExpn { first_assert_argument: a, second_assert_argument: Some(b), /// format_arg:None })` + /// FIXME assert! pub fn parse(e: &'tcx Expr<'tcx>) -> Option { if let ExprKind::Block(block, _) = e.kind { if block.stmts.len() == 1 { if let StmtKind::Semi(matchexpr) = block.stmts.get(0)?.kind { - // macros with unique arg: `{debug_}assert!` (e.g., `debug_assert!(some_condition)`) + // debug macros with unique arg: `debug_assert!` (e.g., `debug_assert!(some_condition)`) if_chain! { if let Some(If { cond, then, .. }) = If::hir(matchexpr); if let ExprKind::Unary(UnOp::Not, condition) = cond.kind; then { if_chain! { if let ExprKind::Block(block, _) = then.kind; - if let [statement, ..] = block.stmts; - if let StmtKind::Expr(call_assert_failed) - | StmtKind::Semi(call_assert_failed) = statement.kind; - if let ExprKind::Call(_, args_assert_failed) = call_assert_failed.kind; - if !args_assert_failed.is_empty(); - if let ExprKind::Call(_, [arg, ..]) = args_assert_failed[0].kind; + if let Some(begin_panic_fmt_block) = block.expr; + if let ExprKind::Block(block,_) = begin_panic_fmt_block.kind; + if let Some(expr) = block.expr; + if let ExprKind::Call(_, args_begin_panic_fmt) = expr.kind; + if !args_begin_panic_fmt.is_empty(); + if let ExprKind::AddrOf(_, _, arg) = args_begin_panic_fmt[0].kind; if let Some(format_arg_expn) = FormatArgsExpn::parse(arg); then { return Some(Self { - first_assert_argument: condition, - second_assert_argument: None, - format_arg: Some(format_arg_expn), // FIXME actually parse the aguments - }); - } - else{ - return Some(Self { - first_assert_argument: condition, - second_assert_argument: None, - format_arg: None, + kind: AssertExpnKind::Bool(condition), + format_arg: Some(format_arg_expn), + is_debug: true, }); } } + return Some(Self { + kind: AssertExpnKind::Bool(condition), + format_arg: None, + is_debug: true, + }); } } @@ -486,13 +492,13 @@ impl<'tcx> AssertExpn<'tcx> { if let ExprKind::Block(matchblock,_) = matchexpr.kind; if let Some(matchblock_expr) = matchblock.expr; then { - return Self::ast_matchblock(matchblock_expr); + return Self::ast_matchblock(matchblock_expr, true); } } } } else if let Some(matchblock_expr) = block.expr { // macros with two args: `assert_{ne, eq}` (e.g., `assert_ne!(a, b)`) - return Self::ast_matchblock(matchblock_expr); + return Self::ast_matchblock(matchblock_expr, false); } } None @@ -500,7 +506,7 @@ impl<'tcx> AssertExpn<'tcx> { /// Try to match the AST for a pattern that contains a match, for example when two args are /// compared - fn ast_matchblock(matchblock_expr: &'tcx Expr<'tcx>) -> Option { + fn ast_matchblock(matchblock_expr: &'tcx Expr<'tcx>, is_debug: bool) -> Option { if_chain! { if let ExprKind::Match(headerexpr, arms, _) = &matchblock_expr.kind; if let ExprKind::Tup([lhs, rhs]) = &headerexpr.kind; @@ -521,20 +527,17 @@ impl<'tcx> AssertExpn<'tcx> { if let Some(format_arg_expn) = FormatArgsExpn::parse(arg); then { return Some(AssertExpn { - first_assert_argument: lhs, - second_assert_argument: Some(rhs), - format_arg: Some(format_arg_expn) - }); - } - else { - return Some(AssertExpn { - first_assert_argument: lhs, - second_assert_argument: - Some(rhs), - format_arg: None + kind: AssertExpnKind::Eq(lhs,rhs), + format_arg: Some(format_arg_expn), + is_debug, }); } } + return Some(AssertExpn { + kind: AssertExpnKind::Eq(lhs,rhs), + format_arg: None, + is_debug, + }); } } None @@ -542,11 +545,14 @@ impl<'tcx> AssertExpn<'tcx> { /// Gives the argument in the comparaison as a vector leaving the format pub fn assert_arguments(&self) -> Vec<&'tcx Expr<'tcx>> { - let mut expr_vec = vec![self.first_assert_argument]; - if let Some(sec_agr) = self.second_assert_argument { - expr_vec.push(sec_agr); + match self.kind { + AssertExpnKind::Bool(expr) => { + vec![expr] + }, + AssertExpnKind::Eq(lhs, rhs) => { + vec![lhs, rhs] + }, } - expr_vec } /// Gives the argument passed to the macro as a string @@ -555,14 +561,9 @@ impl<'tcx> AssertExpn<'tcx> { cx: &LateContext<'_>, applicability: &mut Applicability, ) -> Vec> { - let mut vec_arg = vec![snippet_with_applicability( - cx, - self.first_assert_argument.span, - "..", - applicability, - )]; - if let Some(sec_agr) = self.second_assert_argument { - vec_arg.push(snippet_with_applicability(cx, sec_agr.span, "..", applicability)); + let mut vec_arg = vec![]; + for arg in self.assert_arguments() { + vec_arg.push(snippet_with_applicability(cx, arg.span, "..", applicability)); } vec_arg.append(&mut self.format_arguments(cx, applicability)); vec_arg From 073dd990e79764bf8d42b6d974f1ccf2ee20cc9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ali=C3=A9nore=20Bouttefeux?= Date: Fri, 15 Oct 2021 12:14:48 +0200 Subject: [PATCH 7/8] add assert to the parsing --- clippy_utils/src/higher.rs | 69 +++++++++++++++++++++----------------- 1 file changed, 38 insertions(+), 31 deletions(-) diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs index 65b954782ffe..d5f58848efe5 100644 --- a/clippy_utils/src/higher.rs +++ b/clippy_utils/src/higher.rs @@ -452,41 +452,10 @@ impl<'tcx> AssertExpn<'tcx> { /// second_assert_argument: None, format_arg:None })` `debug_assert_eq!(a, b)` will return /// `Some(AssertExpn { first_assert_argument: a, second_assert_argument: Some(b), /// format_arg:None })` - /// FIXME assert! pub fn parse(e: &'tcx Expr<'tcx>) -> Option { if let ExprKind::Block(block, _) = e.kind { if block.stmts.len() == 1 { if let StmtKind::Semi(matchexpr) = block.stmts.get(0)?.kind { - // debug macros with unique arg: `debug_assert!` (e.g., `debug_assert!(some_condition)`) - if_chain! { - if let Some(If { cond, then, .. }) = If::hir(matchexpr); - if let ExprKind::Unary(UnOp::Not, condition) = cond.kind; - then { - if_chain! { - if let ExprKind::Block(block, _) = then.kind; - if let Some(begin_panic_fmt_block) = block.expr; - if let ExprKind::Block(block,_) = begin_panic_fmt_block.kind; - if let Some(expr) = block.expr; - if let ExprKind::Call(_, args_begin_panic_fmt) = expr.kind; - if !args_begin_panic_fmt.is_empty(); - if let ExprKind::AddrOf(_, _, arg) = args_begin_panic_fmt[0].kind; - if let Some(format_arg_expn) = FormatArgsExpn::parse(arg); - then { - return Some(Self { - kind: AssertExpnKind::Bool(condition), - format_arg: Some(format_arg_expn), - is_debug: true, - }); - } - } - return Some(Self { - kind: AssertExpnKind::Bool(condition), - format_arg: None, - is_debug: true, - }); - } - } - // debug macros with two args: `debug_assert_{ne, eq}` (e.g., `assert_ne!(a, b)`) if_chain! { if let ExprKind::Block(matchblock,_) = matchexpr.kind; @@ -495,11 +464,49 @@ impl<'tcx> AssertExpn<'tcx> { return Self::ast_matchblock(matchblock_expr, true); } } + // debug macros with unique arg: `debug_assert!` (e.g., `debug_assert!(some_condition)`) + return Self::ast_ifblock(matchexpr, true); } } else if let Some(matchblock_expr) = block.expr { // macros with two args: `assert_{ne, eq}` (e.g., `assert_ne!(a, b)`) return Self::ast_matchblock(matchblock_expr, false); } + } else { + // assert! macro + return Self::ast_ifblock(e, false); + } + None + } + + /// Try to parse the pattern for an assert macro with a single argument like `{debug_}assert!` + fn ast_ifblock(ifblock_expr: &'tcx Expr<'tcx>, is_debug: bool) -> Option { + if_chain! { + if let Some(If { cond, then, .. }) = If::hir(ifblock_expr); + if let ExprKind::Unary(UnOp::Not, condition) = cond.kind; + then { + if_chain! { + if let ExprKind::Block(block, _) = then.kind; + if let Some(begin_panic_fmt_block) = block.expr; + if let ExprKind::Block(block,_) = begin_panic_fmt_block.kind; + if let Some(expr) = block.expr; + if let ExprKind::Call(_, args_begin_panic_fmt) = expr.kind; + if !args_begin_panic_fmt.is_empty(); + if let ExprKind::AddrOf(_, _, arg) = args_begin_panic_fmt[0].kind; + if let Some(format_arg_expn) = FormatArgsExpn::parse(arg); + then { + return Some(Self { + kind: AssertExpnKind::Bool(condition), + format_arg: Some(format_arg_expn), + is_debug + }); + } + } + return Some(Self { + kind: AssertExpnKind::Bool(condition), + format_arg: None, + is_debug + }); + } } None } From 103ac52cba3adfcb5b0b20289a05b7f9f338a848 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ali=C3=A9nore=20Bouttefeux?= Date: Thu, 21 Oct 2021 15:28:53 +0200 Subject: [PATCH 8/8] reomove unecessary import --- clippy_lints/src/bool_assert_comparison.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/bool_assert_comparison.rs b/clippy_lints/src/bool_assert_comparison.rs index c2796e728e62..572648b4241c 100644 --- a/clippy_lints/src/bool_assert_comparison.rs +++ b/clippy_lints/src/bool_assert_comparison.rs @@ -1,5 +1,5 @@ use clippy_utils::{ - diagnostics::span_lint_and_sugg, higher::AssertExpn, is_direct_expn_of, source, sugg::Sugg, ty::implements_trait, + diagnostics::span_lint_and_sugg, higher::AssertExpn, is_direct_expn_of, source, ty::implements_trait, }; use rustc_ast::ast::LitKind; use rustc_errors::Applicability;