Point at continue and break that might be in the wrong place
Sometimes move errors are because of a misplaced `continue`, but we didn't
surface that anywhere. Now when there are more than one set of nested loops
we show them out and point at the `continue` and `break` expressions within
that might need to go elsewhere.
```
error[E0382]: use of moved value: `foo`
--> $DIR/nested-loop-moved-value-wrong-continue.rs:46:18
|
LL | for foo in foos {
| ---
| |
| this reinitialization might get skipped
| move occurs because `foo` has type `String`, which does not implement the `Copy` trait
...
LL | for bar in &bars {
| ---------------- inside of this loop
...
LL | baz.push(foo);
| --- value moved here, in previous iteration of loop
...
LL | qux.push(foo);
| ^^^ value used here after move
|
note: verify that your loop breaking logic is correct
--> $DIR/nested-loop-moved-value-wrong-continue.rs:41:17
|
LL | for foo in foos {
| ---------------
...
LL | for bar in &bars {
| ----------------
...
LL | continue;
| ^^^^^^^^ this `continue` advances the loop at line 33
help: consider moving the expression out of the loop so it is only moved once
|
LL ~ let mut value = baz.push(foo);
LL ~ for bar in &bars {
LL |
...
LL | if foo == *bar {
LL ~ value;
|
help: consider cloning the value if the performance cost is acceptable
|
LL | baz.push(foo.clone());
| ++++++++
```
Fix #92531.
This commit is contained in:
Esteban Küber
parent
14473adf42
commit
78d29ad8d6
@ -9,7 +9,7 @@ use rustc_data_structures::fx::FxIndexSet;
|
||||
use rustc_errors::{codes::*, struct_span_code_err, Applicability, Diag, MultiSpan};
|
||||
use rustc_hir as hir;
|
||||
use rustc_hir::def::{DefKind, Res};
|
||||
use rustc_hir::intravisit::{walk_block, walk_expr, Visitor};
|
||||
use rustc_hir::intravisit::{walk_block, walk_expr, Map, Visitor};
|
||||
use rustc_hir::{CoroutineDesugaring, PatField};
|
||||
use rustc_hir::{CoroutineKind, CoroutineSource, LangItem};
|
||||
use rustc_middle::hir::nested_filter::OnlyBodies;
|
||||
@ -799,9 +799,9 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
|
||||
let e = match node {
|
||||
hir::Node::Expr(e) => e,
|
||||
hir::Node::Local(hir::Local { els: Some(els), .. }) => {
|
||||
let mut finder = BreakFinder { found_break: false };
|
||||
let mut finder = BreakFinder { found_breaks: vec![], found_continues: vec![] };
|
||||
finder.visit_block(els);
|
||||
if finder.found_break {
|
||||
if !finder.found_breaks.is_empty() {
|
||||
// Don't suggest clone as it could be will likely end in an infinite
|
||||
// loop.
|
||||
// let Some(_) = foo(non_copy.clone()) else { break; }
|
||||
@ -850,23 +850,122 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
let loop_count: usize = tcx
|
||||
.hir()
|
||||
.parent_iter(expr.hir_id)
|
||||
.map(|(_, node)| match node {
|
||||
hir::Node::Expr(hir::Expr { kind: hir::ExprKind::Loop(..), .. }) => 1,
|
||||
_ => 0,
|
||||
})
|
||||
.sum();
|
||||
|
||||
/// Look for `break` expressions within any arbitrary expressions. We'll do this to infer
|
||||
/// whether this is a case where the moved value would affect the exit of a loop, making it
|
||||
/// unsuitable for a `.clone()` suggestion.
|
||||
struct BreakFinder {
|
||||
found_break: bool,
|
||||
found_breaks: Vec<(hir::Destination, Span)>,
|
||||
found_continues: Vec<(hir::Destination, Span)>,
|
||||
}
|
||||
impl<'hir> Visitor<'hir> for BreakFinder {
|
||||
fn visit_expr(&mut self, ex: &'hir hir::Expr<'hir>) {
|
||||
if let hir::ExprKind::Break(..) = ex.kind {
|
||||
self.found_break = true;
|
||||
match ex.kind {
|
||||
hir::ExprKind::Break(destination, _) => {
|
||||
self.found_breaks.push((destination, ex.span));
|
||||
}
|
||||
hir::ExprKind::Continue(destination) => {
|
||||
self.found_continues.push((destination, ex.span));
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
hir::intravisit::walk_expr(self, ex);
|
||||
}
|
||||
}
|
||||
if let Some(in_loop) = outer_most_loop
|
||||
&& let Some(parent) = parent
|
||||
|
||||
let sm = tcx.sess.source_map();
|
||||
if let Some(in_loop) = outer_most_loop {
|
||||
let mut finder = BreakFinder { found_breaks: vec![], found_continues: vec![] };
|
||||
finder.visit_expr(in_loop);
|
||||
// All of the spans for `break` and `continue` expressions.
|
||||
let spans = finder
|
||||
.found_breaks
|
||||
.iter()
|
||||
.chain(finder.found_continues.iter())
|
||||
.map(|(_, span)| *span)
|
||||
.filter(|span| {
|
||||
!matches!(
|
||||
span.desugaring_kind(),
|
||||
Some(DesugaringKind::ForLoop | DesugaringKind::WhileLoop)
|
||||
)
|
||||
})
|
||||
.collect::<Vec<Span>>();
|
||||
// All of the spans for the loops above the expression with the move error.
|
||||
let loop_spans: Vec<_> = tcx
|
||||
.hir()
|
||||
.parent_iter(expr.hir_id)
|
||||
.filter_map(|(_, node)| match node {
|
||||
hir::Node::Expr(hir::Expr { span, kind: hir::ExprKind::Loop(..), .. }) => {
|
||||
Some(*span)
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
.collect();
|
||||
// It is possible that a user written `break` or `continue` is in the wrong place. We
|
||||
// point them out at the user for them to make a determination. (#92531)
|
||||
if !spans.is_empty() && loop_count > 1 {
|
||||
// Getting fancy: if the spans of the loops *do not* overlap, we only use the line
|
||||
// number when referring to them. If there *are* overlaps (multiple loops on the
|
||||
// same line) then we use the more verbose span output (`file.rs:col:ll`).
|
||||
let mut lines: Vec<_> =
|
||||
loop_spans.iter().map(|sp| sm.lookup_char_pos(sp.lo()).line).collect();
|
||||
lines.sort();
|
||||
lines.dedup();
|
||||
let fmt_span = |span: Span| {
|
||||
if lines.len() == loop_spans.len() {
|
||||
format!("line {}", sm.lookup_char_pos(span.lo()).line)
|
||||
} else {
|
||||
sm.span_to_diagnostic_string(span)
|
||||
}
|
||||
};
|
||||
let mut spans: MultiSpan = spans.clone().into();
|
||||
// Point at all the `continue`s and explicit `break`s in the relevant loops.
|
||||
for (desc, elements) in [
|
||||
("`break` exits", &finder.found_breaks),
|
||||
("`continue` advances", &finder.found_continues),
|
||||
] {
|
||||
for (destination, sp) in elements {
|
||||
if let Ok(hir_id) = destination.target_id
|
||||
&& let hir::Node::Expr(expr) = tcx.hir().hir_node(hir_id)
|
||||
&& !matches!(
|
||||
sp.desugaring_kind(),
|
||||
Some(DesugaringKind::ForLoop | DesugaringKind::WhileLoop)
|
||||
)
|
||||
{
|
||||
spans.push_span_label(
|
||||
*sp,
|
||||
format!("this {desc} the loop at {}", fmt_span(expr.span)),
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
// Point at all the loops that are between this move and the parent item.
|
||||
for span in loop_spans {
|
||||
spans.push_span_label(sm.guess_head_span(span), "");
|
||||
}
|
||||
|
||||
// note: verify that your loop breaking logic is correct
|
||||
// --> $DIR/nested-loop-moved-value-wrong-continue.rs:41:17
|
||||
// |
|
||||
// 28 | for foo in foos {
|
||||
// | ---------------
|
||||
// ...
|
||||
// 33 | for bar in &bars {
|
||||
// | ----------------
|
||||
// ...
|
||||
// 41 | continue;
|
||||
// | ^^^^^^^^ this `continue` advances the loop at line 33
|
||||
err.span_note(spans, "verify that your loop breaking logic is correct");
|
||||
}
|
||||
if let Some(parent) = parent
|
||||
&& let hir::ExprKind::MethodCall(..) | hir::ExprKind::Call(..) = parent.kind
|
||||
{
|
||||
// FIXME: We could check that the call's *parent* takes `&mut val` to make the
|
||||
@ -874,10 +973,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
|
||||
// check for wheter to suggest `let value` or `let mut value`.
|
||||
|
||||
let span = in_loop.span;
|
||||
let mut finder = BreakFinder { found_break: false };
|
||||
finder.visit_expr(in_loop);
|
||||
let sm = tcx.sess.source_map();
|
||||
if (finder.found_break || true)
|
||||
if !finder.found_breaks.is_empty()
|
||||
&& let Ok(value) = sm.span_to_snippet(parent.span)
|
||||
{
|
||||
// We know with high certainty that this move would affect the early return of a
|
||||
@ -897,6 +993,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
can_suggest_clone
|
||||
}
|
||||
|
||||
|
||||
@ -16,12 +16,6 @@ LL | fn take(_x: Box<isize>) {}
|
||||
| ---- ^^^^^^^^^^ this parameter takes ownership of the value
|
||||
| |
|
||||
| in this function
|
||||
help: consider moving the expression out of the loop so it is only moved once
|
||||
|
|
||||
LL ~ let mut value = take(x);
|
||||
LL ~ loop {
|
||||
LL ~ value;
|
||||
|
|
||||
help: consider cloning the value if the performance cost is acceptable
|
||||
|
|
||||
LL | take(x.clone());
|
||||
|
||||
@ -16,12 +16,6 @@ LL | fn take(_x: Box<isize>) {}
|
||||
| ---- ^^^^^^^^^^ this parameter takes ownership of the value
|
||||
| |
|
||||
| in this function
|
||||
help: consider moving the expression out of the loop so it is only moved once
|
||||
|
|
||||
LL ~ let mut value = take(x);
|
||||
LL ~ loop {
|
||||
LL ~ value;
|
||||
|
|
||||
help: consider cloning the value if the performance cost is acceptable
|
||||
|
|
||||
LL | take(x.clone());
|
||||
|
||||
@ -1,5 +1,5 @@
|
||||
fn takes_fn(f: impl Fn()) {
|
||||
loop { //~ HELP consider moving the expression out of the loop so it is only computed once
|
||||
loop {
|
||||
takes_fnonce(f);
|
||||
//~^ ERROR use of moved value
|
||||
//~| HELP consider borrowing
|
||||
|
||||
@ -15,12 +15,6 @@ LL | fn takes_fnonce(_: impl FnOnce()) {}
|
||||
| ------------ ^^^^^^^^^^^^^ this parameter takes ownership of the value
|
||||
| |
|
||||
| in this function
|
||||
help: consider moving the expression out of the loop so it is only moved once
|
||||
|
|
||||
LL ~ let mut value = takes_fnonce(f);
|
||||
LL ~ loop {
|
||||
LL ~ value;
|
||||
|
|
||||
help: consider borrowing `f`
|
||||
|
|
||||
LL | takes_fnonce(&f);
|
||||
|
||||
@ -1,3 +1,27 @@
|
||||
fn foo() {
|
||||
let foos = vec![String::new()];
|
||||
let bars = vec![""];
|
||||
let mut baz = vec![];
|
||||
let mut qux = vec![];
|
||||
for foo in foos { for bar in &bars { if foo == *bar {
|
||||
//~^ NOTE this reinitialization might get skipped
|
||||
//~| NOTE move occurs because `foo` has type `String`
|
||||
//~| NOTE inside of this loop
|
||||
//~| HELP consider moving the expression out of the loop
|
||||
//~| NOTE in this expansion of desugaring of `for` loop
|
||||
baz.push(foo);
|
||||
//~^ NOTE value moved here
|
||||
//~| HELP consider cloning the value
|
||||
continue;
|
||||
//~^ NOTE verify that your loop breaking logic is correct
|
||||
//~| NOTE this `continue` advances the loop at $DIR/nested-loop-moved-value-wrong-continue.rs:6:23
|
||||
} }
|
||||
qux.push(foo);
|
||||
//~^ ERROR use of moved value
|
||||
//~| NOTE value used here
|
||||
}
|
||||
}
|
||||
|
||||
fn main() {
|
||||
let foos = vec![String::new()];
|
||||
let bars = vec![""];
|
||||
@ -15,6 +39,8 @@ fn main() {
|
||||
//~^ NOTE value moved here
|
||||
//~| HELP consider cloning the value
|
||||
continue;
|
||||
//~^ NOTE verify that your loop breaking logic is correct
|
||||
//~| NOTE this `continue` advances the loop at line 33
|
||||
}
|
||||
}
|
||||
qux.push(foo);
|
||||
|
||||
@ -1,5 +1,42 @@
|
||||
error[E0382]: use of moved value: `foo`
|
||||
--> $DIR/nested-loop-moved-value-wrong-continue.rs:20:18
|
||||
--> $DIR/nested-loop-moved-value-wrong-continue.rs:19:14
|
||||
|
|
||||
LL | for foo in foos { for bar in &bars { if foo == *bar {
|
||||
| --- ---------------- inside of this loop
|
||||
| |
|
||||
| this reinitialization might get skipped
|
||||
| move occurs because `foo` has type `String`, which does not implement the `Copy` trait
|
||||
...
|
||||
LL | baz.push(foo);
|
||||
| --- value moved here, in previous iteration of loop
|
||||
...
|
||||
LL | qux.push(foo);
|
||||
| ^^^ value used here after move
|
||||
|
|
||||
note: verify that your loop breaking logic is correct
|
||||
--> $DIR/nested-loop-moved-value-wrong-continue.rs:15:9
|
||||
|
|
||||
LL | for foo in foos { for bar in &bars { if foo == *bar {
|
||||
| --------------- ----------------
|
||||
...
|
||||
LL | continue;
|
||||
| ^^^^^^^^ this `continue` advances the loop at $DIR/nested-loop-moved-value-wrong-continue.rs:6:23: 18:8
|
||||
help: consider moving the expression out of the loop so it is only moved once
|
||||
|
|
||||
LL ~ for foo in foos { let mut value = baz.push(foo);
|
||||
LL ~ for bar in &bars { if foo == *bar {
|
||||
LL |
|
||||
...
|
||||
LL |
|
||||
LL ~ value;
|
||||
|
|
||||
help: consider cloning the value if the performance cost is acceptable
|
||||
|
|
||||
LL | baz.push(foo.clone());
|
||||
| ++++++++
|
||||
|
||||
error[E0382]: use of moved value: `foo`
|
||||
--> $DIR/nested-loop-moved-value-wrong-continue.rs:46:18
|
||||
|
|
||||
LL | for foo in foos {
|
||||
| ---
|
||||
@ -16,6 +53,17 @@ LL | baz.push(foo);
|
||||
LL | qux.push(foo);
|
||||
| ^^^ value used here after move
|
||||
|
|
||||
note: verify that your loop breaking logic is correct
|
||||
--> $DIR/nested-loop-moved-value-wrong-continue.rs:41:17
|
||||
|
|
||||
LL | for foo in foos {
|
||||
| ---------------
|
||||
...
|
||||
LL | for bar in &bars {
|
||||
| ----------------
|
||||
...
|
||||
LL | continue;
|
||||
| ^^^^^^^^ this `continue` advances the loop at line 33
|
||||
help: consider moving the expression out of the loop so it is only moved once
|
||||
|
|
||||
LL ~ let mut value = baz.push(foo);
|
||||
@ -30,6 +78,6 @@ help: consider cloning the value if the performance cost is acceptable
|
||||
LL | baz.push(foo.clone());
|
||||
| ++++++++
|
||||
|
||||
error: aborting due to 1 previous error
|
||||
error: aborting due to 2 previous errors
|
||||
|
||||
For more information about this error, try `rustc --explain E0382`.
|
||||
|
||||
@ -31,6 +31,7 @@ fn qux() {
|
||||
loop {
|
||||
if let Some(item) = iter(vec).next() { //~ ERROR use of moved value
|
||||
println!("{:?}", item);
|
||||
break;
|
||||
}
|
||||
}
|
||||
}
|
||||
@ -42,6 +43,7 @@ fn zap() {
|
||||
loop {
|
||||
if let Some(item) = iter(vec).next() { //~ ERROR use of moved value
|
||||
println!("{:?}", item);
|
||||
break;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@ -99,7 +99,7 @@ LL ~ if let Some(item) = value.next() {
|
||||
|
|
||||
|
||||
error[E0382]: use of moved value: `vec`
|
||||
--> $DIR/recreating-value-in-loop-condition.rs:43:46
|
||||
--> $DIR/recreating-value-in-loop-condition.rs:44:46
|
||||
|
|
||||
LL | let vec = vec!["one", "two", "three"];
|
||||
| --- move occurs because `vec` has type `Vec<&str>`, which does not implement the `Copy` trait
|
||||
@ -119,6 +119,21 @@ LL | fn iter<T>(vec: Vec<T>) -> impl Iterator<Item = T> {
|
||||
| ---- ^^^^^^ this parameter takes ownership of the value
|
||||
| |
|
||||
| in this function
|
||||
note: verify that your loop breaking logic is correct
|
||||
--> $DIR/recreating-value-in-loop-condition.rs:46:25
|
||||
|
|
||||
LL | loop {
|
||||
| ----
|
||||
LL | let vec = vec!["one", "two", "three"];
|
||||
LL | loop {
|
||||
| ----
|
||||
LL | loop {
|
||||
| ----
|
||||
LL | loop {
|
||||
| ----
|
||||
...
|
||||
LL | break;
|
||||
| ^^^^^ this `break` exits the loop at line 43
|
||||
help: consider moving the expression out of the loop so it is only moved once
|
||||
|
|
||||
LL ~ let mut value = iter(vec);
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user