Skip to content

Commit d14c6f2

Browse files
samueltardieusamueltardieu
committed
Include let assignment in suggestion
Placeholders are still given for the content of the whole block. However, if the result of the original `if let` or `match` expression was assigned, the assignment is reflected in the suggestion. No-op assignments (`let x = x;`) are skipped though, unless they contain an explicit type which might help the compiler (`let x: u32 = x;` is kept).
1 parent 38d2387 commit d14c6f2

File tree

3 files changed

+265
-24
lines changed

3 files changed

+265
-24
lines changed

‎clippy_lints/src/loops/while_let_loop.rs

Lines changed: 59 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,44 +1,51 @@
11
use super::WHILE_LET_LOOP;
22
use clippy_utils::diagnostics::span_lint_and_sugg;
3-
use clippy_utils::higher;
4-
use clippy_utils::source::snippet_with_applicability;
3+
use clippy_utils::source::{snippet, snippet_indent, snippet_opt};
54
use clippy_utils::ty::needs_ordered_drop;
65
use clippy_utils::visitors::any_temporaries_need_ordered_drop;
6+
use clippy_utils::{higher, peel_blocks};
7+
use rustc_ast::BindingMode;
78
use rustc_errors::Applicability;
8-
use rustc_hir::{Block, Expr, ExprKind, LetStmt, MatchSource, Pat, StmtKind};
9+
use rustc_hir::{Block, Expr, ExprKind, LetStmt, MatchSource, Pat, PatKind, Path, QPath, StmtKind, Ty};
910
use rustc_lint::LateContext;
1011

1112
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, loop_block: &'tcx Block<'_>) {
12-
let (init, has_trailing_exprs) = match (loop_block.stmts, loop_block.expr) {
13-
([stmt, stmts @ ..], expr) => {
14-
if let StmtKind::Let(&LetStmt {
13+
let (init, let_info) = match (loop_block.stmts, loop_block.expr) {
14+
([stmt, ..], _) => match stmt.kind {
15+
StmtKind::Let(LetStmt {
1516
init: Some(e),
1617
els: None,
18+
pat,
19+
ty,
1720
..
18-
})
19-
| StmtKind::Semi(e)
20-
| StmtKind::Expr(e) = stmt.kind
21-
{
22-
(e, !stmts.is_empty() || expr.is_some())
23-
} else {
24-
return;
25-
}
21+
}) => (*e, Some((*pat, *ty))),
22+
StmtKind::Semi(e) | StmtKind::Expr(e) => (e, None),
23+
_ => return,
2624
},
27-
([], Some(e)) => (e, false),
25+
([], Some(e)) => (e, None),
2826
_ => return,
2927
};
28+
let has_trailing_exprs = loop_block.stmts.len() + usize::from(loop_block.expr.is_some()) > 1;
3029

3130
if let Some(if_let) = higher::IfLet::hir(cx, init)
3231
&& let Some(else_expr) = if_let.if_else
3332
&& is_simple_break_expr(else_expr)
3433
{
35-
could_be_while_let(cx, expr, if_let.let_pat, if_let.let_expr, has_trailing_exprs);
34+
could_be_while_let(
35+
cx,
36+
expr,
37+
if_let.let_pat,
38+
if_let.let_expr,
39+
has_trailing_exprs,
40+
let_info,
41+
if_let.if_then,
42+
);
3643
} else if let ExprKind::Match(scrutinee, [arm1, arm2], MatchSource::Normal) = init.kind
3744
&& arm1.guard.is_none()
3845
&& arm2.guard.is_none()
3946
&& is_simple_break_expr(arm2.body)
4047
{
41-
could_be_while_let(cx, expr, arm1.pat, scrutinee, has_trailing_exprs);
48+
could_be_while_let(cx, expr, arm1.pat, scrutinee, has_trailing_exprs, let_info, arm1.body);
4249
}
4350
}
4451

@@ -62,6 +69,8 @@ fn could_be_while_let<'tcx>(
6269
let_pat: &'tcx Pat<'_>,
6370
let_expr: &'tcx Expr<'_>,
6471
has_trailing_exprs: bool,
72+
let_info: Option<(&Pat<'_>, Option<&Ty<'_>>)>,
73+
inner_expr: &Expr<'_>,
6574
) {
6675
if has_trailing_exprs
6776
&& (needs_ordered_drop(cx, cx.typeck_results().expr_ty(let_expr))
@@ -76,18 +85,46 @@ fn could_be_while_let<'tcx>(
7685
// 1) it was ugly with big bodies;
7786
// 2) it was not indented properly;
7887
// 3) it wasn’t very smart (see #675).
79-
let mut applicability = Applicability::HasPlaceholders;
88+
let inner_content = if let Some((pat, ty)) = let_info
89+
// Prevent trivial reassignments such as `let x = x;` or `let _ = …;`, but
90+
// keep them if the type has been explicitly specified.
91+
&& (!is_trivial_assignment(pat, peel_blocks(inner_expr)) || ty.is_some())
92+
&& let Some(pat_str) = snippet_opt(cx, pat.span)
93+
&& let Some(init_str) = snippet_opt(cx, peel_blocks(inner_expr).span)
94+
{
95+
let ty_str = ty
96+
.map(|ty| format!(": {}", snippet(cx, ty.span, "_")))
97+
.unwrap_or_default();
98+
format!(
99+
"\n{indent} let {pat_str}{ty_str} = {init_str};\n{indent} ..\n{indent}",
100+
indent = snippet_indent(cx, expr.span).unwrap_or_default(),
101+
)
102+
} else {
103+
" .. ".into()
104+
};
105+
80106
span_lint_and_sugg(
81107
cx,
82108
WHILE_LET_LOOP,
83109
expr.span,
84110
"this loop could be written as a `while let` loop",
85111
"try",
86112
format!(
87-
"while let {} = {} {{ .. }}",
88-
snippet_with_applicability(cx, let_pat.span, "..", &mut applicability),
89-
snippet_with_applicability(cx, let_expr.span, "..", &mut applicability),
113+
"while let {} = {} {{{inner_content}}}",
114+
snippet(cx, let_pat.span, ".."),
115+
snippet(cx, let_expr.span, ".."),
90116
),
91-
applicability,
117+
Applicability::HasPlaceholders,
92118
);
93119
}
120+
121+
fn is_trivial_assignment(pat: &Pat<'_>, init: &Expr<'_>) -> bool {
122+
match (pat.kind, init.kind) {
123+
(PatKind::Wild, _) => true,
124+
(
125+
PatKind::Binding(BindingMode::NONE, _, pat_ident, None),
126+
ExprKind::Path(QPath::Resolved(None, Path { segments: [init], .. })),
127+
) => pat_ident.name == init.ident.name,
128+
_ => false,
129+
}
130+
}

‎tests/ui/while_let_loop.rs

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,3 +154,89 @@ fn issue_5715(mut m: core::cell::RefCell<Option<u32>>) {
154154
m = core::cell::RefCell::new(Some(x + 1));
155155
}
156156
}
157+
158+
mod issue_362 {
159+
pub fn merge_sorted<T>(xs: Vec<T>, ys: Vec<T>) -> Vec<T>
160+
where
161+
T: PartialOrd,
162+
{
163+
let total_len = xs.len() + ys.len();
164+
let mut res = Vec::with_capacity(total_len);
165+
let mut ix = xs.into_iter().peekable();
166+
let mut iy = ys.into_iter().peekable();
167+
loop {
168+
//~^ while_let_loop
169+
let lt = match (ix.peek(), iy.peek()) {
170+
(Some(x), Some(y)) => x < y,
171+
_ => break,
172+
};
173+
res.push(if lt { &mut ix } else { &mut iy }.next().unwrap());
174+
}
175+
res.extend(ix);
176+
res.extend(iy);
177+
res
178+
}
179+
}
180+
181+
fn let_assign() {
182+
loop {
183+
//~^ while_let_loop
184+
let x = if let Some(y) = Some(3) {
185+
y
186+
} else {
187+
break;
188+
};
189+
if x == 3 {
190+
break;
191+
}
192+
}
193+
194+
loop {
195+
//~^ while_let_loop
196+
let x: u32 = if let Some(y) = Some(3) {
197+
y
198+
} else {
199+
break;
200+
};
201+
if x == 3 {
202+
break;
203+
}
204+
}
205+
206+
loop {
207+
//~^ while_let_loop
208+
let x = if let Some(x) = Some(3) {
209+
x
210+
} else {
211+
break;
212+
};
213+
if x == 3 {
214+
break;
215+
}
216+
}
217+
218+
loop {
219+
//~^ while_let_loop
220+
let x: u32 = if let Some(x) = Some(3) {
221+
x
222+
} else {
223+
break;
224+
};
225+
if x == 3 {
226+
break;
227+
}
228+
}
229+
230+
loop {
231+
//~^ while_let_loop
232+
let x = if let Some(x) = Some(2) {
233+
let t = 1;
234+
t + x
235+
} else {
236+
break;
237+
};
238+
if x == 3 {
239+
break;
240+
}
241+
}
242+
}

‎tests/ui/while_let_loop.stderr

Lines changed: 120 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,125 @@ LL | | let (e, l) = match "".split_whitespace().next() {
5757
... |
5858
LL | | let _ = (e, l);
5959
LL | | }
60-
| |_____^ help: try: `while let Some(word) = "".split_whitespace().next() { .. }`
60+
| |_____^
61+
|
62+
help: try
63+
|
64+
LL ~ while let Some(word) = "".split_whitespace().next() {
65+
LL + let (e, l) = (word.is_empty(), word.len());
66+
LL + ..
67+
LL + }
68+
|
69+
70+
error: this loop could be written as a `while let` loop
71+
--> tests/ui/while_let_loop.rs:167:9
72+
|
73+
LL | / loop {
74+
LL | |
75+
LL | | let lt = match (ix.peek(), iy.peek()) {
76+
LL | | (Some(x), Some(y)) => x < y,
77+
... |
78+
LL | | res.push(if lt { &mut ix } else { &mut iy }.next().unwrap());
79+
LL | | }
80+
| |_________^
81+
|
82+
help: try
83+
|
84+
LL ~ while let (Some(x), Some(y)) = (ix.peek(), iy.peek()) {
85+
LL + let lt = x < y;
86+
LL + ..
87+
LL + }
88+
|
89+
90+
error: this loop could be written as a `while let` loop
91+
--> tests/ui/while_let_loop.rs:182:5
92+
|
93+
LL | / loop {
94+
LL | |
95+
LL | | let x = if let Some(y) = Some(3) {
96+
LL | | y
97+
... |
98+
LL | | }
99+
| |_____^
100+
|
101+
help: try
102+
|
103+
LL ~ while let Some(y) = Some(3) {
104+
LL + let x = y;
105+
LL + ..
106+
LL + }
107+
|
108+
109+
error: this loop could be written as a `while let` loop
110+
--> tests/ui/while_let_loop.rs:194:5
111+
|
112+
LL | / loop {
113+
LL | |
114+
LL | | let x: u32 = if let Some(y) = Some(3) {
115+
LL | | y
116+
... |
117+
LL | | }
118+
| |_____^
119+
|
120+
help: try
121+
|
122+
LL ~ while let Some(y) = Some(3) {
123+
LL + let x: u32 = y;
124+
LL + ..
125+
LL + }
126+
|
127+
128+
error: this loop could be written as a `while let` loop
129+
--> tests/ui/while_let_loop.rs:206:5
130+
|
131+
LL | / loop {
132+
LL | |
133+
LL | | let x = if let Some(x) = Some(3) {
134+
LL | | x
135+
... |
136+
LL | | }
137+
| |_____^ help: try: `while let Some(x) = Some(3) { .. }`
138+
139+
error: this loop could be written as a `while let` loop
140+
--> tests/ui/while_let_loop.rs:218:5
141+
|
142+
LL | / loop {
143+
LL | |
144+
LL | | let x: u32 = if let Some(x) = Some(3) {
145+
LL | | x
146+
... |
147+
LL | | }
148+
| |_____^
149+
|
150+
help: try
151+
|
152+
LL ~ while let Some(x) = Some(3) {
153+
LL + let x: u32 = x;
154+
LL + ..
155+
LL + }
156+
|
157+
158+
error: this loop could be written as a `while let` loop
159+
--> tests/ui/while_let_loop.rs:230:5
160+
|
161+
LL | / loop {
162+
LL | |
163+
LL | | let x = if let Some(x) = Some(2) {
164+
LL | | let t = 1;
165+
... |
166+
LL | | }
167+
| |_____^
168+
|
169+
help: try
170+
|
171+
LL ~ while let Some(x) = Some(2) {
172+
LL + let x = {
173+
LL + let t = 1;
174+
LL + t + x
175+
LL + };
176+
LL + ..
177+
LL + }
178+
|
61179

62-
error: aborting due to 5 previous errors
180+
error: aborting due to 11 previous errors
63181

0 commit comments

Comments
 (0)