diff --git a/clippy_lints/src/manual_assert.rs b/clippy_lints/src/manual_assert.rs index 26b53ab5d683..9934c06e7238 100644 --- a/clippy_lints/src/manual_assert.rs +++ b/clippy_lints/src/manual_assert.rs @@ -1,9 +1,10 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; +use crate::rustc_lint::LintContext; +use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::macros::{root_macro_call, FormatArgsExpn}; use clippy_utils::source::snippet_with_applicability; -use clippy_utils::{peel_blocks_with_stmt, sugg}; +use clippy_utils::{peel_blocks_with_stmt, span_extract_comment, sugg}; use rustc_errors::Applicability; -use rustc_hir::{Expr, ExprKind, UnOp}; +use rustc_hir::{Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::sym; @@ -50,20 +51,33 @@ impl<'tcx> LateLintPass<'tcx> for ManualAssert { let mut applicability = Applicability::MachineApplicable; let format_args_snip = snippet_with_applicability(cx, format_args.inputs_span(), "..", &mut applicability); let cond = cond.peel_drop_temps(); - let (cond, not) = match cond.kind { - ExprKind::Unary(UnOp::Not, e) => (e, ""), - _ => (cond, "!"), - }; - let cond_sugg = sugg::Sugg::hir_with_applicability(cx, cond, "..", &mut applicability).maybe_par(); - let sugg = format!("assert!({not}{cond_sugg}, {format_args_snip});"); - span_lint_and_sugg( + let mut comments = span_extract_comment(cx.sess().source_map(), expr.span); + if !comments.is_empty() { + comments += "\n"; + } + // we need to negate the expression because `assert!` panics when is `false`, wherease original pattern panicked when evaluating to `true` + let cond_sugg = !sugg::Sugg::hir_with_applicability(cx, cond, "..", &mut applicability); + let sugg = format!("assert!({cond_sugg}, {format_args_snip});"); + // we show to the user the suggestion without the comments, but when applicating the fix, include the comments in the block + span_lint_and_then( cx, MANUAL_ASSERT, expr.span, "only a `panic!` in `if`-then statement", - "try", - sugg, - Applicability::MachineApplicable, + |diag| { + // comments can be noisy, do not show them to the user + diag.tool_only_span_suggestion( + expr.span.shrink_to_lo(), + "add comments back", + comments, + applicability); + diag.span_suggestion( + expr.span, + "try instead", + sugg, + applicability); + } + ); } } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index c2c52d08a3c1..d67ceaec0358 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -2295,6 +2295,29 @@ pub fn span_contains_comment(sm: &SourceMap, span: Span) -> bool { }); } +/// Return all the comments a given span contains +/// Comments are returned wrapped with their relevant delimiters +pub fn span_extract_comment(sm: &SourceMap, span: Span) -> String { + let snippet = sm.span_to_snippet(span).unwrap_or_default(); + let mut comments_buf: Vec = Vec::new(); + let mut index: usize = 0; + + for token in tokenize(&snippet) { + let token_range = index..(index + token.len as usize); + index += token.len as usize; + match token.kind { + TokenKind::BlockComment { .. } | TokenKind::LineComment { .. } => { + if let Some(comment) = snippet.get(token_range) { + comments_buf.push(comment.to_string()); + } + }, + _ => (), + } + } + + comments_buf.join("\n") +} + macro_rules! op_utils { ($($name:ident $assign:ident)*) => { /// Binary operation traits like `LangItem::Add` diff --git a/tests/ui/manual_assert.edition2018.fixed b/tests/ui/manual_assert.edition2018.fixed index 65598f1eaccc..b5ccc07e49e2 100644 --- a/tests/ui/manual_assert.edition2018.fixed +++ b/tests/ui/manual_assert.edition2018.fixed @@ -5,6 +5,7 @@ #![warn(clippy::manual_assert)] #![allow(clippy::nonminimal_bool)] +#![allow(dead_code)] macro_rules! one { () => { @@ -27,8 +28,8 @@ fn main() { { panic!("qaqaq{:?}", a); } - assert!(a.is_empty(), "qaqaq{:?}", a); - assert!(a.is_empty(), "qwqwq"); + assert!(!(!a.is_empty()), "qaqaq{:?}", a); + assert!(!(!a.is_empty()), "qwqwq"); if a.len() == 3 { println!("qwq"); println!("qwq"); @@ -50,3 +51,14 @@ fn main() { assert!(!(a.is_empty() || !b.is_empty()), "panic5"); assert!(!a.is_empty(), "with expansion {}", one!()); } + +fn issue7730() { + // Suggestion should preserve comment + // comment +/* this is a + multiline + comment */ +/// Doc comment +// comment after `panic!` +assert!(!true, "panic with comment"); +} diff --git a/tests/ui/manual_assert.edition2018.stderr b/tests/ui/manual_assert.edition2018.stderr index a0f31afd6ebf..df22e0114be0 100644 --- a/tests/ui/manual_assert.edition2018.stderr +++ b/tests/ui/manual_assert.edition2018.stderr @@ -1,68 +1,124 @@ error: only a `panic!` in `if`-then statement - --> $DIR/manual_assert.rs:30:5 + --> $DIR/manual_assert.rs:31:5 | LL | / if !a.is_empty() { LL | | panic!("qaqaq{:?}", a); LL | | } - | |_____^ help: try: `assert!(a.is_empty(), "qaqaq{:?}", a);` + | |_____^ | = note: `-D clippy::manual-assert` implied by `-D warnings` +help: try instead + | +LL | assert!(!(!a.is_empty()), "qaqaq{:?}", a); + | error: only a `panic!` in `if`-then statement - --> $DIR/manual_assert.rs:33:5 + --> $DIR/manual_assert.rs:34:5 | LL | / if !a.is_empty() { LL | | panic!("qwqwq"); LL | | } - | |_____^ help: try: `assert!(a.is_empty(), "qwqwq");` + | |_____^ + | +help: try instead + | +LL | assert!(!(!a.is_empty()), "qwqwq"); + | error: only a `panic!` in `if`-then statement - --> $DIR/manual_assert.rs:50:5 + --> $DIR/manual_assert.rs:51:5 | LL | / if b.is_empty() { LL | | panic!("panic1"); LL | | } - | |_____^ help: try: `assert!(!b.is_empty(), "panic1");` + | |_____^ + | +help: try instead + | +LL | assert!(!b.is_empty(), "panic1"); + | error: only a `panic!` in `if`-then statement - --> $DIR/manual_assert.rs:53:5 + --> $DIR/manual_assert.rs:54:5 | LL | / if b.is_empty() && a.is_empty() { LL | | panic!("panic2"); LL | | } - | |_____^ help: try: `assert!(!(b.is_empty() && a.is_empty()), "panic2");` + | |_____^ + | +help: try instead + | +LL | assert!(!(b.is_empty() && a.is_empty()), "panic2"); + | error: only a `panic!` in `if`-then statement - --> $DIR/manual_assert.rs:56:5 + --> $DIR/manual_assert.rs:57:5 | LL | / if a.is_empty() && !b.is_empty() { LL | | panic!("panic3"); LL | | } - | |_____^ help: try: `assert!(!(a.is_empty() && !b.is_empty()), "panic3");` + | |_____^ + | +help: try instead + | +LL | assert!(!(a.is_empty() && !b.is_empty()), "panic3"); + | error: only a `panic!` in `if`-then statement - --> $DIR/manual_assert.rs:59:5 + --> $DIR/manual_assert.rs:60:5 | LL | / if b.is_empty() || a.is_empty() { LL | | panic!("panic4"); LL | | } - | |_____^ help: try: `assert!(!(b.is_empty() || a.is_empty()), "panic4");` + | |_____^ + | +help: try instead + | +LL | assert!(!(b.is_empty() || a.is_empty()), "panic4"); + | error: only a `panic!` in `if`-then statement - --> $DIR/manual_assert.rs:62:5 + --> $DIR/manual_assert.rs:63:5 | LL | / if a.is_empty() || !b.is_empty() { LL | | panic!("panic5"); LL | | } - | |_____^ help: try: `assert!(!(a.is_empty() || !b.is_empty()), "panic5");` + | |_____^ + | +help: try instead + | +LL | assert!(!(a.is_empty() || !b.is_empty()), "panic5"); + | error: only a `panic!` in `if`-then statement - --> $DIR/manual_assert.rs:65:5 + --> $DIR/manual_assert.rs:66:5 | LL | / if a.is_empty() { LL | | panic!("with expansion {}", one!()) LL | | } - | |_____^ help: try: `assert!(!a.is_empty(), "with expansion {}", one!());` + | |_____^ + | +help: try instead + | +LL | assert!(!a.is_empty(), "with expansion {}", one!()); + | -error: aborting due to 8 previous errors +error: only a `panic!` in `if`-then statement + --> $DIR/manual_assert.rs:73:5 + | +LL | / if true { +LL | | // comment +LL | | /* this is a +LL | | multiline +... | +LL | | panic!("panic with comment") // comment after `panic!` +LL | | } + | |_____^ + | +help: try instead + | +LL | assert!(!true, "panic with comment"); + | + +error: aborting due to 9 previous errors diff --git a/tests/ui/manual_assert.edition2021.fixed b/tests/ui/manual_assert.edition2021.fixed index 65598f1eaccc..b5ccc07e49e2 100644 --- a/tests/ui/manual_assert.edition2021.fixed +++ b/tests/ui/manual_assert.edition2021.fixed @@ -5,6 +5,7 @@ #![warn(clippy::manual_assert)] #![allow(clippy::nonminimal_bool)] +#![allow(dead_code)] macro_rules! one { () => { @@ -27,8 +28,8 @@ fn main() { { panic!("qaqaq{:?}", a); } - assert!(a.is_empty(), "qaqaq{:?}", a); - assert!(a.is_empty(), "qwqwq"); + assert!(!(!a.is_empty()), "qaqaq{:?}", a); + assert!(!(!a.is_empty()), "qwqwq"); if a.len() == 3 { println!("qwq"); println!("qwq"); @@ -50,3 +51,14 @@ fn main() { assert!(!(a.is_empty() || !b.is_empty()), "panic5"); assert!(!a.is_empty(), "with expansion {}", one!()); } + +fn issue7730() { + // Suggestion should preserve comment + // comment +/* this is a + multiline + comment */ +/// Doc comment +// comment after `panic!` +assert!(!true, "panic with comment"); +} diff --git a/tests/ui/manual_assert.edition2021.stderr b/tests/ui/manual_assert.edition2021.stderr index a0f31afd6ebf..df22e0114be0 100644 --- a/tests/ui/manual_assert.edition2021.stderr +++ b/tests/ui/manual_assert.edition2021.stderr @@ -1,68 +1,124 @@ error: only a `panic!` in `if`-then statement - --> $DIR/manual_assert.rs:30:5 + --> $DIR/manual_assert.rs:31:5 | LL | / if !a.is_empty() { LL | | panic!("qaqaq{:?}", a); LL | | } - | |_____^ help: try: `assert!(a.is_empty(), "qaqaq{:?}", a);` + | |_____^ | = note: `-D clippy::manual-assert` implied by `-D warnings` +help: try instead + | +LL | assert!(!(!a.is_empty()), "qaqaq{:?}", a); + | error: only a `panic!` in `if`-then statement - --> $DIR/manual_assert.rs:33:5 + --> $DIR/manual_assert.rs:34:5 | LL | / if !a.is_empty() { LL | | panic!("qwqwq"); LL | | } - | |_____^ help: try: `assert!(a.is_empty(), "qwqwq");` + | |_____^ + | +help: try instead + | +LL | assert!(!(!a.is_empty()), "qwqwq"); + | error: only a `panic!` in `if`-then statement - --> $DIR/manual_assert.rs:50:5 + --> $DIR/manual_assert.rs:51:5 | LL | / if b.is_empty() { LL | | panic!("panic1"); LL | | } - | |_____^ help: try: `assert!(!b.is_empty(), "panic1");` + | |_____^ + | +help: try instead + | +LL | assert!(!b.is_empty(), "panic1"); + | error: only a `panic!` in `if`-then statement - --> $DIR/manual_assert.rs:53:5 + --> $DIR/manual_assert.rs:54:5 | LL | / if b.is_empty() && a.is_empty() { LL | | panic!("panic2"); LL | | } - | |_____^ help: try: `assert!(!(b.is_empty() && a.is_empty()), "panic2");` + | |_____^ + | +help: try instead + | +LL | assert!(!(b.is_empty() && a.is_empty()), "panic2"); + | error: only a `panic!` in `if`-then statement - --> $DIR/manual_assert.rs:56:5 + --> $DIR/manual_assert.rs:57:5 | LL | / if a.is_empty() && !b.is_empty() { LL | | panic!("panic3"); LL | | } - | |_____^ help: try: `assert!(!(a.is_empty() && !b.is_empty()), "panic3");` + | |_____^ + | +help: try instead + | +LL | assert!(!(a.is_empty() && !b.is_empty()), "panic3"); + | error: only a `panic!` in `if`-then statement - --> $DIR/manual_assert.rs:59:5 + --> $DIR/manual_assert.rs:60:5 | LL | / if b.is_empty() || a.is_empty() { LL | | panic!("panic4"); LL | | } - | |_____^ help: try: `assert!(!(b.is_empty() || a.is_empty()), "panic4");` + | |_____^ + | +help: try instead + | +LL | assert!(!(b.is_empty() || a.is_empty()), "panic4"); + | error: only a `panic!` in `if`-then statement - --> $DIR/manual_assert.rs:62:5 + --> $DIR/manual_assert.rs:63:5 | LL | / if a.is_empty() || !b.is_empty() { LL | | panic!("panic5"); LL | | } - | |_____^ help: try: `assert!(!(a.is_empty() || !b.is_empty()), "panic5");` + | |_____^ + | +help: try instead + | +LL | assert!(!(a.is_empty() || !b.is_empty()), "panic5"); + | error: only a `panic!` in `if`-then statement - --> $DIR/manual_assert.rs:65:5 + --> $DIR/manual_assert.rs:66:5 | LL | / if a.is_empty() { LL | | panic!("with expansion {}", one!()) LL | | } - | |_____^ help: try: `assert!(!a.is_empty(), "with expansion {}", one!());` + | |_____^ + | +help: try instead + | +LL | assert!(!a.is_empty(), "with expansion {}", one!()); + | -error: aborting due to 8 previous errors +error: only a `panic!` in `if`-then statement + --> $DIR/manual_assert.rs:73:5 + | +LL | / if true { +LL | | // comment +LL | | /* this is a +LL | | multiline +... | +LL | | panic!("panic with comment") // comment after `panic!` +LL | | } + | |_____^ + | +help: try instead + | +LL | assert!(!true, "panic with comment"); + | + +error: aborting due to 9 previous errors diff --git a/tests/ui/manual_assert.rs b/tests/ui/manual_assert.rs index 4d2706dd6211..e9dd81a758a8 100644 --- a/tests/ui/manual_assert.rs +++ b/tests/ui/manual_assert.rs @@ -5,6 +5,7 @@ #![warn(clippy::manual_assert)] #![allow(clippy::nonminimal_bool)] +#![allow(dead_code)] macro_rules! one { () => { @@ -66,3 +67,15 @@ fn main() { panic!("with expansion {}", one!()) } } + +fn issue7730() { + // Suggestion should preserve comment + if true { + // comment + /* this is a + multiline + comment */ + /// Doc comment + panic!("panic with comment") // comment after `panic!` + } +}