Skip to content

Add help on reinitialization between move and access #85686

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

Merged
merged 1 commit into from
Jul 18, 2021
Merged
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
92 changes: 74 additions & 18 deletions compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use rustc_middle::mir::{
use rustc_middle::ty::{self, suggest_constraining_type_param, Ty};
use rustc_span::source_map::DesugaringKind;
use rustc_span::symbol::sym;
use rustc_span::{Span, DUMMY_SP};
use rustc_span::{MultiSpan, Span, DUMMY_SP};
use rustc_trait_selection::infer::InferCtxtExt;

use crate::dataflow::drop_flag_effects;
Expand Down Expand Up @@ -66,7 +66,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
self.move_spans(moved_place, location).or_else(|| self.borrow_spans(span, location));
let span = use_spans.args_or_use();

let move_site_vec = self.get_moved_indexes(location, mpi);
let (move_site_vec, maybe_reinitialized_locations) = self.get_moved_indexes(location, mpi);
debug!(
"report_use_of_moved_or_uninitialized: move_site_vec={:?} use_spans={:?}",
move_site_vec, use_spans
Expand Down Expand Up @@ -139,6 +139,32 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
self.describe_place_with_options(moved_place, IncludingDowncast(true)),
);

let reinit_spans = maybe_reinitialized_locations
.iter()
.take(3)
.map(|loc| {
self.move_spans(self.move_data.move_paths[mpi].place.as_ref(), *loc)
.args_or_use()
})
.collect::<Vec<Span>>();
let reinits = maybe_reinitialized_locations.len();
if reinits == 1 {
err.span_label(reinit_spans[0], "this reinitialization might get skipped");
} else if reinits > 1 {
err.span_note(
MultiSpan::from_spans(reinit_spans),
&if reinits <= 3 {
format!("these {} reinitializations might get skipped", reinits)
} else {
format!(
"these 3 reinitializations and {} other{} might get skipped",
reinits - 3,
if reinits == 4 { "" } else { "s" }
)
},
);
}

self.add_moved_or_invoked_closure_note(location, used_place, &mut err);

let mut is_loop_move = false;
Expand Down Expand Up @@ -219,7 +245,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
),
);
}
if is_option_or_result {
if is_option_or_result && maybe_reinitialized_locations.is_empty() {
err.span_suggestion_verbose(
fn_call_span.shrink_to_lo(),
"consider calling `.as_ref()` to borrow the type's contents",
Expand Down Expand Up @@ -260,19 +286,23 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
}
}

if let UseSpans::PatUse(span) = move_spans {
err.span_suggestion_verbose(
span.shrink_to_lo(),
&format!(
"borrow this field in the pattern to avoid moving {}",
self.describe_place(moved_place.as_ref())
.map(|n| format!("`{}`", n))
.unwrap_or_else(|| "the value".to_string())
),
"ref ".to_string(),
Applicability::MachineApplicable,
);
in_pattern = true;
if let (UseSpans::PatUse(span), []) =
(move_spans, &maybe_reinitialized_locations[..])
{
if maybe_reinitialized_locations.is_empty() {
err.span_suggestion_verbose(
span.shrink_to_lo(),
&format!(
"borrow this field in the pattern to avoid moving {}",
self.describe_place(moved_place.as_ref())
.map(|n| format!("`{}`", n))
.unwrap_or_else(|| "the value".to_string())
),
"ref ".to_string(),
Applicability::MachineApplicable,
);
in_pattern = true;
}
}

if let Some(DesugaringKind::ForLoop(_)) = move_span.desugaring_kind() {
Expand Down Expand Up @@ -1465,7 +1495,11 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
err
}

fn get_moved_indexes(&mut self, location: Location, mpi: MovePathIndex) -> Vec<MoveSite> {
fn get_moved_indexes(
&mut self,
location: Location,
mpi: MovePathIndex,
) -> (Vec<MoveSite>, Vec<Location>) {
fn predecessor_locations(
body: &'a mir::Body<'tcx>,
location: Location,
Expand All @@ -1488,6 +1522,8 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
}));

let mut visited = FxHashSet::default();
let mut move_locations = FxHashSet::default();
let mut reinits = vec![];
let mut result = vec![];

'dfs: while let Some((location, is_back_edge)) = stack.pop() {
Expand Down Expand Up @@ -1529,6 +1565,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
move_paths[path].place
);
result.push(MoveSite { moi: *moi, traversed_back_edge: is_back_edge });
move_locations.insert(location);

// Strictly speaking, we could continue our DFS here. There may be
// other moves that can reach the point of error. But it is kind of
Expand Down Expand Up @@ -1565,6 +1602,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
},
);
if any_match {
reinits.push(location);
continue 'dfs;
}

Expand All @@ -1574,7 +1612,25 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
}));
}

result
// Check if we can reach these reinits from a move location.
let reinits_reachable = reinits
.into_iter()
.filter(|reinit| {
let mut visited = FxHashSet::default();
let mut stack = vec![*reinit];
while let Some(location) = stack.pop() {
if !visited.insert(location) {
continue;
}
if move_locations.contains(&location) {
return true;
}
stack.extend(predecessor_locations(self.body, location));
}
false
})
.collect::<Vec<Location>>();
Comment on lines +1615 to +1632
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this happen unconditionally, even in the happy path? We might want to do a perf run just to be sure we don't regress things too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only place where get_moved_indexes is called is in report_use_of_moved_or_uninitialized. I am pretty sure that this function is only called if we know for sure that an error occurred. This shouldn't run in normal error free compilation.

(result, reinits_reachable)
}

pub(in crate::borrow_check) fn report_illegal_mutation_of_borrowed(
Expand Down
40 changes: 40 additions & 0 deletions src/test/ui/borrowck/issue-83760.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
struct Struct;

fn test1() {
let mut val = Some(Struct);
while let Some(foo) = val { //~ ERROR use of moved value
if true {
val = None;
} else {

}
}
}

fn test2() {
let mut foo = Some(Struct);
let _x = foo.unwrap();
if true {
foo = Some(Struct);
} else {
}
let _y = foo; //~ ERROR use of moved value: `foo`
}

fn test3() {
let mut foo = Some(Struct);
let _x = foo.unwrap();
if true {
foo = Some(Struct);
} else if true {
foo = Some(Struct);
} else if true {
foo = Some(Struct);
} else if true {
foo = Some(Struct);
} else {
}
let _y = foo; //~ ERROR use of moved value: `foo`
}

fn main() {}
62 changes: 62 additions & 0 deletions src/test/ui/borrowck/issue-83760.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
error[E0382]: use of moved value
--> $DIR/issue-83760.rs:5:20
|
LL | while let Some(foo) = val {
| ^^^ value moved here, in previous iteration of loop
LL | if true {
LL | val = None;
| ---------- this reinitialization might get skipped
|
= note: move occurs because value has type `Struct`, which does not implement the `Copy` trait

error[E0382]: use of moved value: `foo`
--> $DIR/issue-83760.rs:21:14
|
LL | let mut foo = Some(Struct);
| ------- move occurs because `foo` has type `Option<Struct>`, which does not implement the `Copy` trait
LL | let _x = foo.unwrap();
| -------- `foo` moved due to this method call
LL | if true {
LL | foo = Some(Struct);
| ------------------ this reinitialization might get skipped
...
LL | let _y = foo;
| ^^^ value used here after move
|
note: this function takes ownership of the receiver `self`, which moves `foo`
--> $SRC_DIR/core/src/option.rs:LL:COL
|
LL | pub const fn unwrap(self) -> T {
| ^^^^

error[E0382]: use of moved value: `foo`
--> $DIR/issue-83760.rs:37:14
|
LL | let mut foo = Some(Struct);
| ------- move occurs because `foo` has type `Option<Struct>`, which does not implement the `Copy` trait
LL | let _x = foo.unwrap();
| -------- `foo` moved due to this method call
...
LL | let _y = foo;
| ^^^ value used here after move
|
note: these 3 reinitializations and 1 other might get skipped
--> $DIR/issue-83760.rs:30:9
|
LL | foo = Some(Struct);
| ^^^^^^^^^^^^^^^^^^
LL | } else if true {
LL | foo = Some(Struct);
| ^^^^^^^^^^^^^^^^^^
LL | } else if true {
LL | foo = Some(Struct);
| ^^^^^^^^^^^^^^^^^^
note: this function takes ownership of the receiver `self`, which moves `foo`
--> $SRC_DIR/core/src/option.rs:LL:COL
|
LL | pub const fn unwrap(self) -> T {
| ^^^^

error: aborting due to 3 previous errors

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