From 5e448eed2996286709891fece31ea3f25cf65782 Mon Sep 17 00:00:00 2001 From: Scott Gerring Date: Fri, 31 Jan 2025 15:52:22 +0100 Subject: [PATCH] chore: make manual_assert use multipart suggestions --- clippy_lints/src/manual_assert.rs | 24 ++++--- tests/ui/manual_assert.edition2018.fixed | 76 +++++++++++++++++++++++ tests/ui/manual_assert.edition2018.stderr | 48 +++++++------- tests/ui/manual_assert.edition2021.fixed | 76 +++++++++++++++++++++++ tests/ui/manual_assert.edition2021.stderr | 48 +++++++------- tests/ui/manual_assert.rs | 2 - 6 files changed, 217 insertions(+), 57 deletions(-) create mode 100644 tests/ui/manual_assert.edition2018.fixed create mode 100644 tests/ui/manual_assert.edition2021.fixed diff --git a/clippy_lints/src/manual_assert.rs b/clippy_lints/src/manual_assert.rs index 83c16d4466d0..0c84be250ca5 100644 --- a/clippy_lints/src/manual_assert.rs +++ b/clippy_lints/src/manual_assert.rs @@ -1,5 +1,6 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::macros::{is_panic, root_macro_call}; +use clippy_utils::source::{indent_of, reindent_multiline}; use clippy_utils::{is_else_clause, is_parent_stmt, peel_blocks_with_stmt, span_extract_comment, sugg}; use rustc_errors::Applicability; use rustc_hir::{Expr, ExprKind, UnOp}; @@ -62,25 +63,22 @@ impl<'tcx> LateLintPass<'tcx> for ManualAssert { }; let cond_sugg = sugg::Sugg::hir_with_applicability(cx, cond, "..", &mut applicability).maybe_par(); let semicolon = if is_parent_stmt(cx, expr.hir_id) { ";" } else { "" }; - let sugg = format!("assert!({not}{cond_sugg}, {format_args_snip}){semicolon}"); - // we show to the user the suggestion without the comments, but when applying the fix, include the - // comments in the block + let base_sugg = format!("assert!({not}{cond_sugg}, {format_args_snip}){semicolon}"); + + let indent = indent_of(cx, expr.span); + let full_sugg = reindent_multiline(format!("{comments}{base_sugg}").into(), true, indent); + span_lint_and_then( cx, MANUAL_ASSERT, expr.span, "only a `panic!` in `if`-then statement", |diag| { - // comments can be noisy, do not show them to the user - if !comments.is_empty() { - diag.tool_only_span_suggestion( - expr.span.shrink_to_lo(), - "add comments back", - comments, - applicability, - ); - } - diag.span_suggestion(expr.span, "try instead", sugg, applicability); + diag.multipart_suggestion( + "replace `if`-then-`panic!` with `assert!`", + vec![(expr.span, full_sugg.into())], + applicability, + ); }, ); } diff --git a/tests/ui/manual_assert.edition2018.fixed b/tests/ui/manual_assert.edition2018.fixed new file mode 100644 index 000000000000..9520413beda7 --- /dev/null +++ b/tests/ui/manual_assert.edition2018.fixed @@ -0,0 +1,76 @@ +//@revisions: edition2018 edition2021 +//@[edition2018] edition:2018 +//@[edition2021] edition:2021 + +#![warn(clippy::manual_assert)] +#![allow(dead_code, unused_doc_comments)] +#![allow(clippy::nonminimal_bool, clippy::uninlined_format_args, clippy::useless_vec)] + +macro_rules! one { + () => { + 1 + }; +} + +fn main() { + let a = vec![1, 2, 3]; + let c = Some(2); + if !a.is_empty() + && a.len() == 3 + && c.is_some() + && !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"); + } + 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!(!a.is_empty(), "with expansion {}", one!()); + if a.is_empty() { + let _ = 0; + } else if a.len() == 1 { + panic!("panic6"); + } +} + +fn issue7730(a: u8) { + // Suggestion should preserve comment + // comment + /* this is a + multiline + comment */ + /// Doc comment + // comment after `panic!` + assert!(!(a > 2), "panic with comment"); +} + +fn issue12505() { + struct Foo(T); + + impl Foo { + const BAR: () = assert!(!(N == 0), ); + } +} diff --git a/tests/ui/manual_assert.edition2018.stderr b/tests/ui/manual_assert.edition2018.stderr index 004463720e26..98001a7982ae 100644 --- a/tests/ui/manual_assert.edition2018.stderr +++ b/tests/ui/manual_assert.edition2018.stderr @@ -1,72 +1,72 @@ error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:32:5 + --> tests/ui/manual_assert.rs:30:5 | LL | / if !a.is_empty() { LL | | panic!("qaqaq{:?}", a); LL | | } - | |_____^ help: try instead: `assert!(a.is_empty(), "qaqaq{:?}", a);` + | |_____^ help: replace `if`-then-`panic!` with `assert!`: `assert!(a.is_empty(), "qaqaq{:?}", a);` | = note: `-D clippy::manual-assert` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_assert)]` error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:35:5 + --> tests/ui/manual_assert.rs:33:5 | LL | / if !a.is_empty() { LL | | panic!("qwqwq"); LL | | } - | |_____^ help: try instead: `assert!(a.is_empty(), "qwqwq");` + | |_____^ help: replace `if`-then-`panic!` with `assert!`: `assert!(a.is_empty(), "qwqwq");` error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:52:5 + --> tests/ui/manual_assert.rs:50:5 | LL | / if b.is_empty() { LL | | panic!("panic1"); LL | | } - | |_____^ help: try instead: `assert!(!b.is_empty(), "panic1");` + | |_____^ help: replace `if`-then-`panic!` with `assert!`: `assert!(!b.is_empty(), "panic1");` error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:55:5 + --> tests/ui/manual_assert.rs:53:5 | LL | / if b.is_empty() && a.is_empty() { LL | | panic!("panic2"); LL | | } - | |_____^ help: try instead: `assert!(!(b.is_empty() && a.is_empty()), "panic2");` + | |_____^ help: replace `if`-then-`panic!` with `assert!`: `assert!(!(b.is_empty() && a.is_empty()), "panic2");` error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:58:5 + --> tests/ui/manual_assert.rs:56:5 | LL | / if a.is_empty() && !b.is_empty() { LL | | panic!("panic3"); LL | | } - | |_____^ help: try instead: `assert!(!(a.is_empty() && !b.is_empty()), "panic3");` + | |_____^ help: replace `if`-then-`panic!` with `assert!`: `assert!(!(a.is_empty() && !b.is_empty()), "panic3");` error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:61:5 + --> tests/ui/manual_assert.rs:59:5 | LL | / if b.is_empty() || a.is_empty() { LL | | panic!("panic4"); LL | | } - | |_____^ help: try instead: `assert!(!(b.is_empty() || a.is_empty()), "panic4");` + | |_____^ help: replace `if`-then-`panic!` with `assert!`: `assert!(!(b.is_empty() || a.is_empty()), "panic4");` error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:64:5 + --> tests/ui/manual_assert.rs:62:5 | LL | / if a.is_empty() || !b.is_empty() { LL | | panic!("panic5"); LL | | } - | |_____^ help: try instead: `assert!(!(a.is_empty() || !b.is_empty()), "panic5");` + | |_____^ help: replace `if`-then-`panic!` with `assert!`: `assert!(!(a.is_empty() || !b.is_empty()), "panic5");` error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:67:5 + --> tests/ui/manual_assert.rs:65:5 | LL | / if a.is_empty() { LL | | panic!("with expansion {}", one!()) LL | | } - | |_____^ help: try instead: `assert!(!a.is_empty(), "with expansion {}", one!());` + | |_____^ help: replace `if`-then-`panic!` with `assert!`: `assert!(!a.is_empty(), "with expansion {}", one!());` error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:79:5 + --> tests/ui/manual_assert.rs:77:5 | LL | / if a > 2 { LL | | // comment @@ -77,19 +77,25 @@ LL | | panic!("panic with comment") // comment after `panic!` LL | | } | |_____^ | -help: try instead +help: replace `if`-then-`panic!` with `assert!` | -LL | assert!(!(a > 2), "panic with comment"); +LL ~ // comment +LL + /* this is a +LL + multiline +LL + comment */ +LL + /// Doc comment +LL + // comment after `panic!` +LL + assert!(!(a > 2), "panic with comment"); | error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:93:25 + --> tests/ui/manual_assert.rs:91:25 | LL | const BAR: () = if N == 0 { | _________________________^ LL | | panic!() LL | | }; - | |_________^ help: try instead: `assert!(!(N == 0), )` + | |_________^ help: replace `if`-then-`panic!` with `assert!`: `assert!(!(N == 0), )` error: aborting due to 10 previous errors diff --git a/tests/ui/manual_assert.edition2021.fixed b/tests/ui/manual_assert.edition2021.fixed new file mode 100644 index 000000000000..9520413beda7 --- /dev/null +++ b/tests/ui/manual_assert.edition2021.fixed @@ -0,0 +1,76 @@ +//@revisions: edition2018 edition2021 +//@[edition2018] edition:2018 +//@[edition2021] edition:2021 + +#![warn(clippy::manual_assert)] +#![allow(dead_code, unused_doc_comments)] +#![allow(clippy::nonminimal_bool, clippy::uninlined_format_args, clippy::useless_vec)] + +macro_rules! one { + () => { + 1 + }; +} + +fn main() { + let a = vec![1, 2, 3]; + let c = Some(2); + if !a.is_empty() + && a.len() == 3 + && c.is_some() + && !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"); + } + 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!(!a.is_empty(), "with expansion {}", one!()); + if a.is_empty() { + let _ = 0; + } else if a.len() == 1 { + panic!("panic6"); + } +} + +fn issue7730(a: u8) { + // Suggestion should preserve comment + // comment + /* this is a + multiline + comment */ + /// Doc comment + // comment after `panic!` + assert!(!(a > 2), "panic with comment"); +} + +fn issue12505() { + struct Foo(T); + + impl Foo { + const BAR: () = assert!(!(N == 0), ); + } +} diff --git a/tests/ui/manual_assert.edition2021.stderr b/tests/ui/manual_assert.edition2021.stderr index 004463720e26..98001a7982ae 100644 --- a/tests/ui/manual_assert.edition2021.stderr +++ b/tests/ui/manual_assert.edition2021.stderr @@ -1,72 +1,72 @@ error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:32:5 + --> tests/ui/manual_assert.rs:30:5 | LL | / if !a.is_empty() { LL | | panic!("qaqaq{:?}", a); LL | | } - | |_____^ help: try instead: `assert!(a.is_empty(), "qaqaq{:?}", a);` + | |_____^ help: replace `if`-then-`panic!` with `assert!`: `assert!(a.is_empty(), "qaqaq{:?}", a);` | = note: `-D clippy::manual-assert` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::manual_assert)]` error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:35:5 + --> tests/ui/manual_assert.rs:33:5 | LL | / if !a.is_empty() { LL | | panic!("qwqwq"); LL | | } - | |_____^ help: try instead: `assert!(a.is_empty(), "qwqwq");` + | |_____^ help: replace `if`-then-`panic!` with `assert!`: `assert!(a.is_empty(), "qwqwq");` error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:52:5 + --> tests/ui/manual_assert.rs:50:5 | LL | / if b.is_empty() { LL | | panic!("panic1"); LL | | } - | |_____^ help: try instead: `assert!(!b.is_empty(), "panic1");` + | |_____^ help: replace `if`-then-`panic!` with `assert!`: `assert!(!b.is_empty(), "panic1");` error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:55:5 + --> tests/ui/manual_assert.rs:53:5 | LL | / if b.is_empty() && a.is_empty() { LL | | panic!("panic2"); LL | | } - | |_____^ help: try instead: `assert!(!(b.is_empty() && a.is_empty()), "panic2");` + | |_____^ help: replace `if`-then-`panic!` with `assert!`: `assert!(!(b.is_empty() && a.is_empty()), "panic2");` error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:58:5 + --> tests/ui/manual_assert.rs:56:5 | LL | / if a.is_empty() && !b.is_empty() { LL | | panic!("panic3"); LL | | } - | |_____^ help: try instead: `assert!(!(a.is_empty() && !b.is_empty()), "panic3");` + | |_____^ help: replace `if`-then-`panic!` with `assert!`: `assert!(!(a.is_empty() && !b.is_empty()), "panic3");` error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:61:5 + --> tests/ui/manual_assert.rs:59:5 | LL | / if b.is_empty() || a.is_empty() { LL | | panic!("panic4"); LL | | } - | |_____^ help: try instead: `assert!(!(b.is_empty() || a.is_empty()), "panic4");` + | |_____^ help: replace `if`-then-`panic!` with `assert!`: `assert!(!(b.is_empty() || a.is_empty()), "panic4");` error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:64:5 + --> tests/ui/manual_assert.rs:62:5 | LL | / if a.is_empty() || !b.is_empty() { LL | | panic!("panic5"); LL | | } - | |_____^ help: try instead: `assert!(!(a.is_empty() || !b.is_empty()), "panic5");` + | |_____^ help: replace `if`-then-`panic!` with `assert!`: `assert!(!(a.is_empty() || !b.is_empty()), "panic5");` error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:67:5 + --> tests/ui/manual_assert.rs:65:5 | LL | / if a.is_empty() { LL | | panic!("with expansion {}", one!()) LL | | } - | |_____^ help: try instead: `assert!(!a.is_empty(), "with expansion {}", one!());` + | |_____^ help: replace `if`-then-`panic!` with `assert!`: `assert!(!a.is_empty(), "with expansion {}", one!());` error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:79:5 + --> tests/ui/manual_assert.rs:77:5 | LL | / if a > 2 { LL | | // comment @@ -77,19 +77,25 @@ LL | | panic!("panic with comment") // comment after `panic!` LL | | } | |_____^ | -help: try instead +help: replace `if`-then-`panic!` with `assert!` | -LL | assert!(!(a > 2), "panic with comment"); +LL ~ // comment +LL + /* this is a +LL + multiline +LL + comment */ +LL + /// Doc comment +LL + // comment after `panic!` +LL + assert!(!(a > 2), "panic with comment"); | error: only a `panic!` in `if`-then statement - --> tests/ui/manual_assert.rs:93:25 + --> tests/ui/manual_assert.rs:91:25 | LL | const BAR: () = if N == 0 { | _________________________^ LL | | panic!() LL | | }; - | |_________^ help: try instead: `assert!(!(N == 0), )` + | |_________^ help: replace `if`-then-`panic!` with `assert!`: `assert!(!(N == 0), )` error: aborting due to 10 previous errors diff --git a/tests/ui/manual_assert.rs b/tests/ui/manual_assert.rs index 6337920a3eea..363bafdf05d0 100644 --- a/tests/ui/manual_assert.rs +++ b/tests/ui/manual_assert.rs @@ -2,8 +2,6 @@ //@[edition2018] edition:2018 //@[edition2021] edition:2021 -//@no-rustfix: need to change the suggestion to a multipart suggestion - #![warn(clippy::manual_assert)] #![allow(dead_code, unused_doc_comments)] #![allow(clippy::nonminimal_bool, clippy::uninlined_format_args, clippy::useless_vec)]