Skip to content

Commit 11a6d19

Browse files
borsbors
committed
Auto merge of #9479 - kraktus:manual_assert2, r=llogiq
[`manual_assert`]: Preserve comments in the suggestion close #7730 changelog: [`manual_assert`]: Preserve comments in the suggestion
2 parents ac12011 + 3ab02aa commit 11a6d19

File tree

7 files changed

+213
-27
lines changed

7 files changed

+213
-27
lines changed

‎clippy_lints/src/manual_assert.rs‎

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
1-
use clippy_utils::diagnostics::span_lint_and_sugg;
1+
use crate::rustc_lint::LintContext;
2+
use clippy_utils::diagnostics::span_lint_and_then;
23
use clippy_utils::macros::{root_macro_call, FormatArgsExpn};
34
use clippy_utils::source::snippet_with_applicability;
4-
use clippy_utils::{peel_blocks_with_stmt, sugg};
5+
use clippy_utils::{peel_blocks_with_stmt, span_extract_comment, sugg};
56
use rustc_errors::Applicability;
67
use rustc_hir::{Expr, ExprKind, UnOp};
78
use rustc_lint::{LateContext, LateLintPass};
@@ -50,20 +51,36 @@ impl<'tcx> LateLintPass<'tcx> for ManualAssert {
5051
let mut applicability = Applicability::MachineApplicable;
5152
let format_args_snip = snippet_with_applicability(cx, format_args.inputs_span(), "..", &mut applicability);
5253
let cond = cond.peel_drop_temps();
54+
let mut comments = span_extract_comment(cx.sess().source_map(), expr.span);
55+
if !comments.is_empty() {
56+
comments += "\n";
57+
}
5358
let (cond, not) = match cond.kind {
5459
ExprKind::Unary(UnOp::Not, e) => (e, ""),
5560
_ => (cond, "!"),
5661
};
5762
let cond_sugg = sugg::Sugg::hir_with_applicability(cx, cond, "..", &mut applicability).maybe_par();
5863
let sugg = format!("assert!({not}{cond_sugg}, {format_args_snip});");
59-
span_lint_and_sugg(
64+
// we show to the user the suggestion without the comments, but when applicating the fix, include the comments in the block
65+
span_lint_and_then(
6066
cx,
6167
MANUAL_ASSERT,
6268
expr.span,
6369
"only a `panic!` in `if`-then statement",
64-
"try",
65-
sugg,
66-
Applicability::MachineApplicable,
70+
|diag| {
71+
// comments can be noisy, do not show them to the user
72+
diag.tool_only_span_suggestion(
73+
expr.span.shrink_to_lo(),
74+
"add comments back",
75+
comments,
76+
applicability);
77+
diag.span_suggestion(
78+
expr.span,
79+
"try instead",
80+
sugg,
81+
applicability);
82+
}
83+
6784
);
6885
}
6986
}

‎clippy_utils/src/lib.rs‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2295,6 +2295,29 @@ pub fn span_contains_comment(sm: &SourceMap, span: Span) -> bool {
22952295
});
22962296
}
22972297

2298+
/// Return all the comments a given span contains
2299+
/// Comments are returned wrapped with their relevant delimiters
2300+
pub fn span_extract_comment(sm: &SourceMap, span: Span) -> String {
2301+
let snippet = sm.span_to_snippet(span).unwrap_or_default();
2302+
let mut comments_buf: Vec<String> = Vec::new();
2303+
let mut index: usize = 0;
2304+
2305+
for token in tokenize(&snippet) {
2306+
let token_range = index..(index + token.len as usize);
2307+
index += token.len as usize;
2308+
match token.kind {
2309+
TokenKind::BlockComment { .. } | TokenKind::LineComment { .. } => {
2310+
if let Some(comment) = snippet.get(token_range) {
2311+
comments_buf.push(comment.to_string());
2312+
}
2313+
},
2314+
_ => (),
2315+
}
2316+
}
2317+
2318+
comments_buf.join("\n")
2319+
}
2320+
22982321
macro_rules! op_utils {
22992322
($($name:ident $assign:ident)*) => {
23002323
/// Binary operation traits like `LangItem::Add`

‎tests/ui/manual_assert.edition2018.fixed‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
// run-rustfix
55

66
#![warn(clippy::manual_assert)]
7-
#![allow(clippy::nonminimal_bool)]
7+
#![allow(dead_code, unused_doc_comments, clippy::nonminimal_bool)]
88

99
macro_rules! one {
1010
() => {
@@ -50,3 +50,14 @@ fn main() {
5050
assert!(!(a.is_empty() || !b.is_empty()), "panic5");
5151
assert!(!a.is_empty(), "with expansion {}", one!());
5252
}
53+
54+
fn issue7730(a: u8) {
55+
// Suggestion should preserve comment
56+
// comment
57+
/* this is a
58+
multiline
59+
comment */
60+
/// Doc comment
61+
// comment after `panic!`
62+
assert!(!(a > 2), "panic with comment");
63+
}

‎tests/ui/manual_assert.edition2018.stderr‎

Lines changed: 65 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,65 +4,121 @@ error: only a `panic!` in `if`-then statement
44
LL | / if !a.is_empty() {
55
LL | | panic!("qaqaq{:?}", a);
66
LL | | }
7-
| |_____^ help: try: `assert!(a.is_empty(), "qaqaq{:?}", a);`
7+
| |_____^
88
|
99
= note: `-D clippy::manual-assert` implied by `-D warnings`
10+
help: try instead
11+
|
12+
LL | assert!(a.is_empty(), "qaqaq{:?}", a);
13+
|
1014

1115
error: only a `panic!` in `if`-then statement
1216
--> $DIR/manual_assert.rs:33:5
1317
|
1418
LL | / if !a.is_empty() {
1519
LL | | panic!("qwqwq");
1620
LL | | }
17-
| |_____^ help: try: `assert!(a.is_empty(), "qwqwq");`
21+
| |_____^
22+
|
23+
help: try instead
24+
|
25+
LL | assert!(a.is_empty(), "qwqwq");
26+
|
1827

1928
error: only a `panic!` in `if`-then statement
2029
--> $DIR/manual_assert.rs:50:5
2130
|
2231
LL | / if b.is_empty() {
2332
LL | | panic!("panic1");
2433
LL | | }
25-
| |_____^ help: try: `assert!(!b.is_empty(), "panic1");`
34+
| |_____^
35+
|
36+
help: try instead
37+
|
38+
LL | assert!(!b.is_empty(), "panic1");
39+
|
2640

2741
error: only a `panic!` in `if`-then statement
2842
--> $DIR/manual_assert.rs:53:5
2943
|
3044
LL | / if b.is_empty() && a.is_empty() {
3145
LL | | panic!("panic2");
3246
LL | | }
33-
| |_____^ help: try: `assert!(!(b.is_empty() && a.is_empty()), "panic2");`
47+
| |_____^
48+
|
49+
help: try instead
50+
|
51+
LL | assert!(!(b.is_empty() && a.is_empty()), "panic2");
52+
|
3453

3554
error: only a `panic!` in `if`-then statement
3655
--> $DIR/manual_assert.rs:56:5
3756
|
3857
LL | / if a.is_empty() && !b.is_empty() {
3958
LL | | panic!("panic3");
4059
LL | | }
41-
| |_____^ help: try: `assert!(!(a.is_empty() && !b.is_empty()), "panic3");`
60+
| |_____^
61+
|
62+
help: try instead
63+
|
64+
LL | assert!(!(a.is_empty() && !b.is_empty()), "panic3");
65+
|
4266

4367
error: only a `panic!` in `if`-then statement
4468
--> $DIR/manual_assert.rs:59:5
4569
|
4670
LL | / if b.is_empty() || a.is_empty() {
4771
LL | | panic!("panic4");
4872
LL | | }
49-
| |_____^ help: try: `assert!(!(b.is_empty() || a.is_empty()), "panic4");`
73+
| |_____^
74+
|
75+
help: try instead
76+
|
77+
LL | assert!(!(b.is_empty() || a.is_empty()), "panic4");
78+
|
5079

5180
error: only a `panic!` in `if`-then statement
5281
--> $DIR/manual_assert.rs:62:5
5382
|
5483
LL | / if a.is_empty() || !b.is_empty() {
5584
LL | | panic!("panic5");
5685
LL | | }
57-
| |_____^ help: try: `assert!(!(a.is_empty() || !b.is_empty()), "panic5");`
86+
| |_____^
87+
|
88+
help: try instead
89+
|
90+
LL | assert!(!(a.is_empty() || !b.is_empty()), "panic5");
91+
|
5892

5993
error: only a `panic!` in `if`-then statement
6094
--> $DIR/manual_assert.rs:65:5
6195
|
6296
LL | / if a.is_empty() {
6397
LL | | panic!("with expansion {}", one!())
6498
LL | | }
65-
| |_____^ help: try: `assert!(!a.is_empty(), "with expansion {}", one!());`
99+
| |_____^
100+
|
101+
help: try instead
102+
|
103+
LL | assert!(!a.is_empty(), "with expansion {}", one!());
104+
|
105+
106+
error: only a `panic!` in `if`-then statement
107+
--> $DIR/manual_assert.rs:72:5
108+
|
109+
LL | / if a > 2 {
110+
LL | | // comment
111+
LL | | /* this is a
112+
LL | | multiline
113+
... |
114+
LL | | panic!("panic with comment") // comment after `panic!`
115+
LL | | }
116+
| |_____^
117+
|
118+
help: try instead
119+
|
120+
LL | assert!(!(a > 2), "panic with comment");
121+
|
66122

67-
error: aborting due to 8 previous errors
123+
error: aborting due to 9 previous errors
68124

‎tests/ui/manual_assert.edition2021.fixed‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
// run-rustfix
55

66
#![warn(clippy::manual_assert)]
7-
#![allow(clippy::nonminimal_bool)]
7+
#![allow(dead_code, unused_doc_comments, clippy::nonminimal_bool)]
88

99
macro_rules! one {
1010
() => {
@@ -50,3 +50,14 @@ fn main() {
5050
assert!(!(a.is_empty() || !b.is_empty()), "panic5");
5151
assert!(!a.is_empty(), "with expansion {}", one!());
5252
}
53+
54+
fn issue7730(a: u8) {
55+
// Suggestion should preserve comment
56+
// comment
57+
/* this is a
58+
multiline
59+
comment */
60+
/// Doc comment
61+
// comment after `panic!`
62+
assert!(!(a > 2), "panic with comment");
63+
}

‎tests/ui/manual_assert.edition2021.stderr‎

Lines changed: 65 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,65 +4,121 @@ error: only a `panic!` in `if`-then statement
44
LL | / if !a.is_empty() {
55
LL | | panic!("qaqaq{:?}", a);
66
LL | | }
7-
| |_____^ help: try: `assert!(a.is_empty(), "qaqaq{:?}", a);`
7+
| |_____^
88
|
99
= note: `-D clippy::manual-assert` implied by `-D warnings`
10+
help: try instead
11+
|
12+
LL | assert!(a.is_empty(), "qaqaq{:?}", a);
13+
|
1014

1115
error: only a `panic!` in `if`-then statement
1216
--> $DIR/manual_assert.rs:33:5
1317
|
1418
LL | / if !a.is_empty() {
1519
LL | | panic!("qwqwq");
1620
LL | | }
17-
| |_____^ help: try: `assert!(a.is_empty(), "qwqwq");`
21+
| |_____^
22+
|
23+
help: try instead
24+
|
25+
LL | assert!(a.is_empty(), "qwqwq");
26+
|
1827

1928
error: only a `panic!` in `if`-then statement
2029
--> $DIR/manual_assert.rs:50:5
2130
|
2231
LL | / if b.is_empty() {
2332
LL | | panic!("panic1");
2433
LL | | }
25-
| |_____^ help: try: `assert!(!b.is_empty(), "panic1");`
34+
| |_____^
35+
|
36+
help: try instead
37+
|
38+
LL | assert!(!b.is_empty(), "panic1");
39+
|
2640

2741
error: only a `panic!` in `if`-then statement
2842
--> $DIR/manual_assert.rs:53:5
2943
|
3044
LL | / if b.is_empty() && a.is_empty() {
3145
LL | | panic!("panic2");
3246
LL | | }
33-
| |_____^ help: try: `assert!(!(b.is_empty() && a.is_empty()), "panic2");`
47+
| |_____^
48+
|
49+
help: try instead
50+
|
51+
LL | assert!(!(b.is_empty() && a.is_empty()), "panic2");
52+
|
3453

3554
error: only a `panic!` in `if`-then statement
3655
--> $DIR/manual_assert.rs:56:5
3756
|
3857
LL | / if a.is_empty() && !b.is_empty() {
3958
LL | | panic!("panic3");
4059
LL | | }
41-
| |_____^ help: try: `assert!(!(a.is_empty() && !b.is_empty()), "panic3");`
60+
| |_____^
61+
|
62+
help: try instead
63+
|
64+
LL | assert!(!(a.is_empty() && !b.is_empty()), "panic3");
65+
|
4266

4367
error: only a `panic!` in `if`-then statement
4468
--> $DIR/manual_assert.rs:59:5
4569
|
4670
LL | / if b.is_empty() || a.is_empty() {
4771
LL | | panic!("panic4");
4872
LL | | }
49-
| |_____^ help: try: `assert!(!(b.is_empty() || a.is_empty()), "panic4");`
73+
| |_____^
74+
|
75+
help: try instead
76+
|
77+
LL | assert!(!(b.is_empty() || a.is_empty()), "panic4");
78+
|
5079

5180
error: only a `panic!` in `if`-then statement
5281
--> $DIR/manual_assert.rs:62:5
5382
|
5483
LL | / if a.is_empty() || !b.is_empty() {
5584
LL | | panic!("panic5");
5685
LL | | }
57-
| |_____^ help: try: `assert!(!(a.is_empty() || !b.is_empty()), "panic5");`
86+
| |_____^
87+
|
88+
help: try instead
89+
|
90+
LL | assert!(!(a.is_empty() || !b.is_empty()), "panic5");
91+
|
5892

5993
error: only a `panic!` in `if`-then statement
6094
--> $DIR/manual_assert.rs:65:5
6195
|
6296
LL | / if a.is_empty() {
6397
LL | | panic!("with expansion {}", one!())
6498
LL | | }
65-
| |_____^ help: try: `assert!(!a.is_empty(), "with expansion {}", one!());`
99+
| |_____^
100+
|
101+
help: try instead
102+
|
103+
LL | assert!(!a.is_empty(), "with expansion {}", one!());
104+
|
105+
106+
error: only a `panic!` in `if`-then statement
107+
--> $DIR/manual_assert.rs:72:5
108+
|
109+
LL | / if a > 2 {
110+
LL | | // comment
111+
LL | | /* this is a
112+
LL | | multiline
113+
... |
114+
LL | | panic!("panic with comment") // comment after `panic!`
115+
LL | | }
116+
| |_____^
117+
|
118+
help: try instead
119+
|
120+
LL | assert!(!(a > 2), "panic with comment");
121+
|
66122

67-
error: aborting due to 8 previous errors
123+
error: aborting due to 9 previous errors
68124

0 commit comments

Comments
 (0)