Skip to content

Commit 9e3f709

Browse files
ada4aada4a
committed
make sure we don't swallow comments
1 parent 18f8a05 commit 9e3f709

File tree

4 files changed

+183
-32
lines changed

4 files changed

+183
-32
lines changed

‎clippy_lints/src/duplicate_match_guards.rs‎

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
2-
use clippy_utils::eq_expr_value;
3-
use clippy_utils::source::snippet_with_applicability;
2+
use clippy_utils::source::{HasSession, snippet_with_applicability};
3+
use clippy_utils::{eq_expr_value, span_contains_comment};
44
use rustc_errors::Applicability;
55
use rustc_hir::{Arm, ExprKind};
66
use rustc_lint::{LateContext, LateLintPass};
77
use rustc_session::declare_lint_pass;
8-
use rustc_span::Span;
8+
use rustc_span::{BytePos, Span};
99

1010
declare_clippy_lint! {
1111
/// ### What it does
@@ -73,7 +73,7 @@ impl<'tcx> LateLintPass<'tcx> for DuplicateMatchGuards {
7373
if let ExprKind::If(cond, then, None) = arm_body_expr.kind
7474
&& eq_expr_value(cx, guard, cond.peel_drop_temps())
7575
{
76-
let ExprKind::Block(then, _) = then.kind else {
76+
let ExprKind::Block(then_without_curlies, _) = then.kind else {
7777
unreachable!("the `then` expr in `ExprKind::If` is always `ExprKind::Block`")
7878
};
7979

@@ -82,7 +82,7 @@ impl<'tcx> LateLintPass<'tcx> for DuplicateMatchGuards {
8282
// with the one in the body
8383
let mut applicability = Applicability::MaybeIncorrect;
8484

85-
let sugg = snippet_with_applicability(cx, then.span, "..", &mut applicability);
85+
let sugg = snippet_with_applicability(cx, then_without_curlies.span, "..", &mut applicability);
8686

8787
if body_has_block {
8888
// the common case:
@@ -98,6 +98,19 @@ impl<'tcx> LateLintPass<'tcx> for DuplicateMatchGuards {
9898
//
9999
// suggest removing the `if` _and_ the curlies of the inner brace,
100100
// since the arm body already has braces
101+
102+
// make sure that we won't swallow any comments. be extra conservative and bail out on _any_ comment
103+
// outside of `then`:
104+
//
105+
// <pat> if <guard> => { if <cond> <then> }
106+
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^
107+
let sm = cx.sess().source_map();
108+
if span_contains_comment(sm, arm.span.with_hi(then.span.lo()))
109+
|| span_contains_comment(sm, arm.span.with_lo(then.span.hi()))
110+
{
111+
return;
112+
}
113+
101114
span_lint_and_then(
102115
cx,
103116
DUPLICATE_MATCH_GUARDS,
@@ -106,7 +119,14 @@ impl<'tcx> LateLintPass<'tcx> for DuplicateMatchGuards {
106119
|diag| {
107120
diag.multipart_suggestion_verbose(
108121
"remove the condition",
109-
vec![(remove_block_curlies(arm_body_expr.span), sugg.to_string())],
122+
vec![
123+
// <pat> if <guard> => { if <cond> { <then_without_curlies> } }
124+
// ^^^^^^^^^^^
125+
(arm_body_expr.span.with_hi(then.span.lo() + BytePos(1)), String::new()),
126+
// <pat> if <guard> => { if <cond> { <then_without_curlies> } }
127+
// ^^
128+
(arm_body_expr.span.with_lo(then.span.hi() - BytePos(1)), String::new()),
129+
],
110130
applicability,
111131
);
112132
},

‎tests/ui/duplicate_match_guards.fixed‎

Lines changed: 64 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,30 +8,40 @@ fn main() {
88

99
match 0u32 {
1010
0 if true => {
11-
{
11+
1212
//~^ duplicate_match_guards
1313
return;
14-
}
14+
1515
},
1616
0 if a > b => {
17-
{
17+
1818
//~^ duplicate_match_guards
1919
return;
20-
}
20+
2121
},
2222
// not _identical_, but the meaning is the same
2323
0 if a > b => {
24-
{
24+
2525
//~^ duplicate_match_guards
2626
return;
27-
}
27+
2828
},
2929
// a bit more complicated
3030
0 if a > 0 && b > 0 => {
31-
{
31+
3232
//~^ duplicate_match_guards
3333
return;
34-
}
34+
35+
},
36+
// comments inside the inner block are preserved
37+
// (for comments _outside_ the inner block, see below)
38+
#[rustfmt::skip]
39+
0 if a > b => {
40+
//~ duplicate_match_guards
41+
/*before*/
42+
return;
43+
/* after
44+
*/
3545
},
3646
// no curlies around arm body
3747
#[rustfmt::skip] // would add the outer curlies
@@ -66,6 +76,52 @@ fn main() {
6676
return;
6777
}
6878
},
79+
// comments (putting them right next to the tested element to check for off-by-one errors):
80+
// - before arrow
81+
#[rustfmt::skip]
82+
0 if a > b/* comment */=> {
83+
if a > b {
84+
return;
85+
}
86+
},
87+
// - before outer opening curly
88+
#[rustfmt::skip]
89+
0 if a > b =>/* comment */{
90+
if a > b {
91+
return;
92+
}
93+
},
94+
// - before `if`
95+
#[rustfmt::skip]
96+
0 if a > b => {/*
97+
comment
98+
*/if a > b {
99+
return;
100+
}
101+
},
102+
// - before condition
103+
#[rustfmt::skip]
104+
0 if a > b => {
105+
if/*
106+
comment */a > b {
107+
return;
108+
}
109+
},
110+
#[rustfmt::skip]
111+
// - before inner opening curly
112+
0 if a > b => {
113+
if a > b/*
114+
comment */{
115+
return;
116+
}
117+
},
118+
#[rustfmt::skip]
119+
// - before closing outer curly
120+
0 if a > b => {
121+
if a > b {
122+
return;
123+
}/* comment
124+
*/},
69125
_ => todo!(),
70126
};
71127
}

‎tests/ui/duplicate_match_guards.rs‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,16 @@ fn main() {
3333
return;
3434
}
3535
},
36+
// comments inside the inner block are preserved
37+
// (for comments _outside_ the inner block, see below)
38+
#[rustfmt::skip]
39+
0 if a > b => {
40+
if a > b {//~ duplicate_match_guards
41+
/*before*/
42+
return;
43+
/* after
44+
*/}
45+
},
3646
// no curlies around arm body
3747
#[rustfmt::skip] // would add the outer curlies
3848
0 if true => if true {
@@ -66,6 +76,52 @@ fn main() {
6676
return;
6777
}
6878
},
79+
// comments (putting them right next to the tested element to check for off-by-one errors):
80+
// - before arrow
81+
#[rustfmt::skip]
82+
0 if a > b/* comment */=> {
83+
if a > b {
84+
return;
85+
}
86+
},
87+
// - before outer opening curly
88+
#[rustfmt::skip]
89+
0 if a > b =>/* comment */{
90+
if a > b {
91+
return;
92+
}
93+
},
94+
// - before `if`
95+
#[rustfmt::skip]
96+
0 if a > b => {/*
97+
comment
98+
*/if a > b {
99+
return;
100+
}
101+
},
102+
// - before condition
103+
#[rustfmt::skip]
104+
0 if a > b => {
105+
if/*
106+
comment */a > b {
107+
return;
108+
}
109+
},
110+
#[rustfmt::skip]
111+
// - before inner opening curly
112+
0 if a > b => {
113+
if a > b/*
114+
comment */{
115+
return;
116+
}
117+
},
118+
#[rustfmt::skip]
119+
// - before closing outer curly
120+
0 if a > b => {
121+
if a > b {
122+
return;
123+
}/* comment
124+
*/},
69125
_ => todo!(),
70126
};
71127
}

‎tests/ui/duplicate_match_guards.stderr‎

Lines changed: 37 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,10 @@ LL | | }
1111
= help: to override `-D warnings` add `#[allow(clippy::duplicate_match_guards)]`
1212
help: remove the condition
1313
|
14-
LL ~ {
15-
LL +
16-
LL + return;
17-
LL + }
14+
LL ~
15+
LL |
16+
LL | return;
17+
LL ~
1818
|
1919

2020
error: condition duplicates match guard
@@ -28,10 +28,10 @@ LL | | }
2828
|
2929
help: remove the condition
3030
|
31-
LL ~ {
32-
LL +
33-
LL + return;
34-
LL + }
31+
LL ~
32+
LL |
33+
LL | return;
34+
LL ~
3535
|
3636

3737
error: condition duplicates match guard
@@ -45,10 +45,10 @@ LL | | }
4545
|
4646
help: remove the condition
4747
|
48-
LL ~ {
49-
LL +
50-
LL + return;
51-
LL + }
48+
LL ~
49+
LL |
50+
LL | return;
51+
LL ~
5252
|
5353

5454
error: condition duplicates match guard
@@ -62,14 +62,33 @@ LL | | }
6262
|
6363
help: remove the condition
6464
|
65-
LL ~ {
66-
LL +
67-
LL + return;
68-
LL + }
65+
LL ~
66+
LL |
67+
LL | return;
68+
LL ~
69+
|
70+
71+
error: condition duplicates match guard
72+
--> tests/ui/duplicate_match_guards.rs:40:13
73+
|
74+
LL | / if a > b {
75+
LL | | /*before*/
76+
LL | | return;
77+
LL | | /* after
78+
LL | | */}
79+
| |_______________^
80+
|
81+
help: remove the condition
82+
|
83+
LL ~
84+
LL | /*before*/
85+
LL | return;
86+
LL | /* after
87+
LL ~ */
6988
|
7089

7190
error: condition duplicates match guard
72-
--> tests/ui/duplicate_match_guards.rs:38:22
91+
--> tests/ui/duplicate_match_guards.rs:48:22
7392
|
7493
LL | 0 if true => if true {
7594
| ______________________^
@@ -86,5 +105,5 @@ LL + return;
86105
LL ~ },
87106
|
88107

89-
error: aborting due to 5 previous errors
108+
error: aborting due to 6 previous errors
90109

0 commit comments

Comments
 (0)