Skip to content

Check definitions of arguments before suggest move expr out of loop #142189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 54 additions & 1 deletion compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1102,8 +1102,61 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
// suggestion more targeted to the `mk_iter(val).next()` case. Maybe do that only to
// check for whether to suggest `let value` or `let mut value`.

// This closure checks if `cur_use_expr` is defined in scope visible to `new_use_expr`.
// Used to check *if the `value` can be moved out of the loop*.
let is_var_visible_to_new_use = |cur_use_expr: &hir::Expr<'_>,
new_use_expr: &hir::Expr<'_>|
-> bool {
let new_use_hir_id = new_use_expr.hir_id;
let new_use_span = new_use_expr.span;

if let hir::ExprKind::Path(hir::QPath::Resolved(_, path)) = cur_use_expr.kind {
if let hir::def::Res::Local(def_hir_id) = path.res {
if let Some(def_direct_parent) = tcx.hir_get_enclosing_scope(def_hir_id)
{
let mut first = true;
let mut id = new_use_hir_id;
// iterate over the parents of the new use expression
while let Some(parent_hir_id) = tcx.hir_get_enclosing_scope(id) {
if first {
first = false;
// if they are in the same scope,
// the parent of the new use expr is the same as the parent of the definition,
// the definition should be before the new use expression
if parent_hir_id == def_direct_parent {
return tcx.hir_span(def_hir_id).lo()
< new_use_span.lo();
}
} else if parent_hir_id == def_direct_parent {
// if they are not in the same scope,
// but the parent of the definition is the ancestor of the new use expr,
// the definition is visible to the new use expr
return true;
}
id = parent_hir_id;
}
}
}
}
false
};

// Issue: https://github.com/rust-lang/rust/issues/141880
let mut permit = true;
if let hir::ExprKind::MethodCall(_, obj, args, ..) = parent.kind {
if !is_var_visible_to_new_use(obj, in_loop) {
permit = false;
}
for arg in args {
if !is_var_visible_to_new_use(arg, in_loop) {
permit = false;
}
}
}

let span = in_loop.span;
if !finder.found_breaks.is_empty()
if permit
&& !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
Expand Down
35 changes: 35 additions & 0 deletions tests/ui/borrowck/suggest-move-expr-out-of-loop-issue-141880.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#[derive(Default)]
struct Foo{
x: String,
}

impl Foo {
fn foo(&mut self, name: String) {
self.x = name;
}
}



fn main() {
let name1 = String::from("foo");
for _ in 0..3 {
let mut foo = Foo::default();
foo.foo(name1); //~ ERROR use of moved value: `name1` [E0382]
println!("{}", foo.x);
}

let name2 = String::from("bar");
for mut foo in [1,2,3].iter_mut().map(|m| Foo::default()) {
foo.foo(name2); //~ ERROR use of moved value: `name2` [E0382]
println!("{}", foo.x);
}


let name3 = String::from("baz");
let mut foo = Foo::default();
for _ in 0..10 {
foo.foo(name3); //~ ERROR use of moved value: `name3` [E0382]
println!("{}", foo.x);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
error[E0382]: use of moved value: `name1`
--> $DIR/suggest-move-expr-out-of-loop-issue-141880.rs:18:17
|
LL | let name1 = String::from("foo");
| ----- move occurs because `name1` has type `String`, which does not implement the `Copy` trait
LL | for _ in 0..3 {
| ------------- inside of this loop
LL | let mut foo = Foo::default();
LL | foo.foo(name1);
| ^^^^^ value moved here, in previous iteration of loop
|
note: consider changing this parameter type in method `foo` to borrow instead if owning the value isn't necessary
--> $DIR/suggest-move-expr-out-of-loop-issue-141880.rs:7:29
|
LL | fn foo(&mut self, name: String) {
| --- in this method ^^^^^^ this parameter takes ownership of the value
help: consider cloning the value if the performance cost is acceptable
|
LL | foo.foo(name1.clone());
| ++++++++

error[E0382]: use of moved value: `name2`
--> $DIR/suggest-move-expr-out-of-loop-issue-141880.rs:24:17
|
LL | let name2 = String::from("bar");
| ----- move occurs because `name2` has type `String`, which does not implement the `Copy` trait
LL | for mut foo in [1,2,3].iter_mut().map(|m| Foo::default()) {
| ---------------------------------------------------------- inside of this loop
LL | foo.foo(name2);
| ^^^^^ value moved here, in previous iteration of loop
|
note: consider changing this parameter type in method `foo` to borrow instead if owning the value isn't necessary
--> $DIR/suggest-move-expr-out-of-loop-issue-141880.rs:7:29
|
LL | fn foo(&mut self, name: String) {
| --- in this method ^^^^^^ this parameter takes ownership of the value
help: consider cloning the value if the performance cost is acceptable
|
LL | foo.foo(name2.clone());
| ++++++++

error[E0382]: use of moved value: `name3`
--> $DIR/suggest-move-expr-out-of-loop-issue-141880.rs:32:17
|
LL | let name3 = String::from("baz");
| ----- move occurs because `name3` has type `String`, which does not implement the `Copy` trait
LL | let mut foo = Foo::default();
LL | for _ in 0..10 {
| -------------- inside of this loop
LL | foo.foo(name3);
| ^^^^^ value moved here, in previous iteration of loop
|
note: consider changing this parameter type in method `foo` to borrow instead if owning the value isn't necessary
--> $DIR/suggest-move-expr-out-of-loop-issue-141880.rs:7:29
|
LL | fn foo(&mut self, name: String) {
| --- in this method ^^^^^^ this parameter takes ownership of the value
help: consider moving the expression out of the loop so it is only moved once
|
LL ~ let mut value = foo.foo(name3);
LL ~ for _ in 0..10 {
LL ~ value;
|
help: consider cloning the value if the performance cost is acceptable
|
LL | foo.foo(name3.clone());
| ++++++++

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0382`.
Loading