Skip to content

Commit beaf155

Browse files
authored
while_let_loop: Include let assignment in suggestion (#14756)
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). Closes #362 changelog: [`while_let_loop`]: include `let` assignment in suggestion
2 parents b90880d + d14c6f2 commit beaf155

File tree

3 files changed

+271
-40
lines changed

3 files changed

+271
-40
lines changed
Lines changed: 65 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,68 +1,65 @@
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

45-
/// Returns `true` if expr contains a single break expression without a label or eub-expression.
52+
/// Returns `true` if expr contains a single break expression without a label or sub-expression,
53+
/// possibly embedded in blocks.
4654
fn is_simple_break_expr(e: &Expr<'_>) -> bool {
47-
matches!(peel_blocks(e).kind, ExprKind::Break(dest, None) if dest.label.is_none())
48-
}
49-
50-
/// Removes any blocks containing only a single expression.
51-
fn peel_blocks<'tcx>(e: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> {
5255
if let ExprKind::Block(b, _) = e.kind {
5356
match (b.stmts, b.expr) {
54-
([s], None) => {
55-
if let StmtKind::Expr(e) | StmtKind::Semi(e) = s.kind {
56-
peel_blocks(e)
57-
} else {
58-
e
59-
}
60-
},
61-
([], Some(e)) => peel_blocks(e),
62-
_ => e,
57+
([s], None) => matches!(s.kind, StmtKind::Expr(e) | StmtKind::Semi(e) if is_simple_break_expr(e)),
58+
([], Some(e)) => is_simple_break_expr(e),
59+
_ => false,
6360
}
6461
} else {
65-
e
62+
matches!(e.kind, ExprKind::Break(dest, None) if dest.label.is_none())
6663
}
6764
}
6865

@@ -72,6 +69,8 @@ fn could_be_while_let<'tcx>(
7269
let_pat: &'tcx Pat<'_>,
7370
let_expr: &'tcx Expr<'_>,
7471
has_trailing_exprs: bool,
72+
let_info: Option<(&Pat<'_>, Option<&Ty<'_>>)>,
73+
inner_expr: &Expr<'_>,
7574
) {
7675
if has_trailing_exprs
7776
&& (needs_ordered_drop(cx, cx.typeck_results().expr_ty(let_expr))
@@ -86,18 +85,46 @@ fn could_be_while_let<'tcx>(
8685
// 1) it was ugly with big bodies;
8786
// 2) it was not indented properly;
8887
// 3) it wasn’t very smart (see #675).
89-
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+
90106
span_lint_and_sugg(
91107
cx,
92108
WHILE_LET_LOOP,
93109
expr.span,
94110
"this loop could be written as a `while let` loop",
95111
"try",
96112
format!(
97-
"while let {} = {} {{ .. }}",
98-
snippet_with_applicability(cx, let_pat.span, "..", &mut applicability),
99-
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, ".."),
100116
),
101-
applicability,
117+
Applicability::HasPlaceholders,
102118
);
103119
}
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)