Skip to content

Commit fe75faa

Browse files
committed
Fix while_let_on_iterator
Reborrow mutable references rather then take a reference to them.
1 parent f6a5889 commit fe75faa

File tree

4 files changed

+71
-4
lines changed

4 files changed

+71
-4
lines changed

clippy_lints/src/loops/while_let_on_iterator.rs

+11-2
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use clippy_utils::{
77
use if_chain::if_chain;
88
use rustc_errors::Applicability;
99
use rustc_hir::intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor};
10-
use rustc_hir::{def::Res, Expr, ExprKind, HirId, Local, MatchSource, Node, PatKind, QPath, UnOp};
10+
use rustc_hir::{def::Res, Expr, ExprKind, HirId, Local, MatchSource, Mutability, Node, PatKind, QPath, UnOp};
1111
use rustc_lint::LateContext;
1212
use rustc_span::{symbol::sym, Span, Symbol};
1313

@@ -48,7 +48,12 @@ pub(super) fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
4848
// borrowed mutably. TODO: If the struct can be partially moved from and the struct isn't used
4949
// afterwards a mutable borrow of a field isn't necessary.
5050
let ref_mut = if !iter_expr.fields.is_empty() || needs_mutable_borrow(cx, &iter_expr, loop_expr) {
51-
"&mut "
51+
if cx.typeck_results().node_type(iter_expr.hir_id).ref_mutability() == Some(Mutability::Mut) {
52+
// Reborrow for mutable references. It may not be possible to get a mutable reference here.
53+
"&mut *"
54+
} else {
55+
"&mut "
56+
}
5257
} else {
5358
""
5459
};
@@ -69,6 +74,8 @@ pub(super) fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
6974
struct IterExpr {
7075
/// The span of the whole expression, not just the path and fields stored here.
7176
span: Span,
77+
/// The HIR id of the whole expression, not just the path and fields stored here.
78+
hir_id: HirId,
7279
/// The fields used, in order of child to parent.
7380
fields: Vec<Symbol>,
7481
/// The path being used.
@@ -78,12 +85,14 @@ struct IterExpr {
7885
/// the expression might have side effects.
7986
fn try_parse_iter_expr(cx: &LateContext<'_>, mut e: &Expr<'_>) -> Option<IterExpr> {
8087
let span = e.span;
88+
let hir_id = e.hir_id;
8189
let mut fields = Vec::new();
8290
loop {
8391
match e.kind {
8492
ExprKind::Path(ref path) => {
8593
break Some(IterExpr {
8694
span,
95+
hir_id,
8796
fields,
8897
path: cx.qpath_res(path, e.hir_id),
8998
});

tests/ui/while_let_on_iterator.fixed

+23
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,29 @@ fn issue7249() {
334334
x();
335335
}
336336

337+
fn issue7510() {
338+
let mut it = 0..10;
339+
let it = &mut it;
340+
// Needs to reborrow `it` as the binding isn't mutable
341+
for x in &mut *it {
342+
if x % 2 == 0 {
343+
break;
344+
}
345+
}
346+
println!("{}", it.next().unwrap());
347+
348+
struct S<T>(T);
349+
let mut it = 0..10;
350+
let it = S(&mut it);
351+
// Needs to reborrow `it.0` as the binding isn't mutable
352+
for x in &mut *it.0 {
353+
if x % 2 == 0 {
354+
break;
355+
}
356+
}
357+
println!("{}", it.0.next().unwrap());
358+
}
359+
337360
fn main() {
338361
let mut it = 0..20;
339362
for _ in it {

tests/ui/while_let_on_iterator.rs

+23
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,29 @@ fn issue7249() {
334334
x();
335335
}
336336

337+
fn issue7510() {
338+
let mut it = 0..10;
339+
let it = &mut it;
340+
// Needs to reborrow `it` as the binding isn't mutable
341+
while let Some(x) = it.next() {
342+
if x % 2 == 0 {
343+
break;
344+
}
345+
}
346+
println!("{}", it.next().unwrap());
347+
348+
struct S<T>(T);
349+
let mut it = 0..10;
350+
let it = S(&mut it);
351+
// Needs to reborrow `it.0` as the binding isn't mutable
352+
while let Some(x) = it.0.next() {
353+
if x % 2 == 0 {
354+
break;
355+
}
356+
}
357+
println!("{}", it.0.next().unwrap());
358+
}
359+
337360
fn main() {
338361
let mut it = 0..20;
339362
while let Some(..) = it.next() {

tests/ui/while_let_on_iterator.stderr

+14-2
Original file line numberDiff line numberDiff line change
@@ -111,10 +111,22 @@ LL | while let Some(x) = it.next() {
111111
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for x in &mut it`
112112

113113
error: this loop could be written as a `for` loop
114-
--> $DIR/while_let_on_iterator.rs:339:5
114+
--> $DIR/while_let_on_iterator.rs:341:5
115+
|
116+
LL | while let Some(x) = it.next() {
117+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for x in &mut *it`
118+
119+
error: this loop could be written as a `for` loop
120+
--> $DIR/while_let_on_iterator.rs:352:5
121+
|
122+
LL | while let Some(x) = it.0.next() {
123+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for x in &mut *it.0`
124+
125+
error: this loop could be written as a `for` loop
126+
--> $DIR/while_let_on_iterator.rs:362:5
115127
|
116128
LL | while let Some(..) = it.next() {
117129
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for _ in it`
118130

119-
error: aborting due to 19 previous errors
131+
error: aborting due to 21 previous errors
120132

0 commit comments

Comments
 (0)