Skip to content
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

fixes move analysis #49392

Merged
merged 3 commits into from
Apr 7, 2018
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
17 changes: 17 additions & 0 deletions src/librustc_mir/borrow_check/error_reporting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,23 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
.collect::<Vec<_>>();

if mois.is_empty() {
let root_place = self.prefixes(&place, PrefixSet::All)
.last()
.unwrap();

if self.moved_error_reported
.contains(&root_place.clone())
{
debug!(
"report_use_of_moved_or_uninitialized place: error about {:?} suppressed",
root_place
);
return;
}

self.moved_error_reported
.insert(root_place.clone());

let item_msg = match self.describe_place(place) {
Some(name) => format!("`{}`", name),
None => "value".to_owned(),
Expand Down
92 changes: 69 additions & 23 deletions src/librustc_mir/borrow_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
},
access_place_error_reported: FxHashSet(),
reservation_error_reported: FxHashSet(),
moved_error_reported: FxHashSet(),
nonlexical_regioncx: opt_regioncx,
nonlexical_cause_info: None,
};
Expand Down Expand Up @@ -285,6 +286,9 @@ pub struct MirBorrowckCtxt<'cx, 'gcx: 'tcx, 'tcx: 'cx> {
/// but it is currently inconvenient to track down the BorrowIndex
/// at the time we detect and report a reservation error.
reservation_error_reported: FxHashSet<Place<'tcx>>,
/// This field keeps track of errors reported in the checking of moved variables,
/// so that we don't report report seemingly duplicate errors.
moved_error_reported: FxHashSet<Place<'tcx>>,
/// Non-lexical region inference context, if NLL is enabled. This
/// contains the results from region inference and lets us e.g.
/// find out which CFG points are contained in each borrow region.
Expand Down Expand Up @@ -368,7 +372,7 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
LocalMutationIsAllowed::No,
flow_state,
);
self.check_if_path_is_moved(
self.check_if_path_or_subpath_is_moved(
context,
InitializationRequiringAction::Use,
(output, span),
Expand Down Expand Up @@ -963,7 +967,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
// Write of P[i] or *P, or WriteAndRead of any P, requires P init'd.
match mode {
MutateMode::WriteAndRead => {
self.check_if_path_is_moved(
self.check_if_path_or_subpath_is_moved(
context,
InitializationRequiringAction::Update,
place_span,
Expand Down Expand Up @@ -1023,7 +1027,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
flow_state,
);

self.check_if_path_is_moved(
self.check_if_path_or_subpath_is_moved(
context,
InitializationRequiringAction::Borrow,
(place, span),
Expand Down Expand Up @@ -1051,7 +1055,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
LocalMutationIsAllowed::No,
flow_state,
);
self.check_if_path_is_moved(
self.check_if_path_or_subpath_is_moved(
context,
InitializationRequiringAction::Use,
(place, span),
Expand Down Expand Up @@ -1098,7 +1102,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
);

// Finally, check if path was already moved.
self.check_if_path_is_moved(
self.check_if_path_or_subpath_is_moved(
context,
InitializationRequiringAction::Use,
(place, span),
Expand All @@ -1116,7 +1120,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
);

// Finally, check if path was already moved.
self.check_if_path_is_moved(
self.check_if_path_or_subpath_is_moved(
context,
InitializationRequiringAction::Use,
(place, span),
Expand Down Expand Up @@ -1267,7 +1271,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
LocalMutationIsAllowed::No,
flow_state,
);
// We do not need to call `check_if_path_is_moved`
// We do not need to call `check_if_path_or_subpath_is_moved`
// again, as we already called it when we made the
// initial reservation.
}
Expand Down Expand Up @@ -1302,7 +1306,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
}
}

fn check_if_path_is_moved(
fn check_if_full_path_is_moved(
&mut self,
context: Context,
desired_action: InitializationRequiringAction,
Expand All @@ -1320,18 +1324,17 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rename check_if_path_is_moved to check_if_full_path_is_moved?

// 1. Move of `a.b.c`, use of `a.b.c`
// 2. Move of `a.b.c`, use of `a.b.c.d` (without first reinitializing `a.b.c.d`)
// 3. Move of `a.b.c`, use of `a` or `a.b`
// 4. Uninitialized `(a.b.c: &_)`, use of `*a.b.c`; note that with
// 3. Uninitialized `(a.b.c: &_)`, use of `*a.b.c`; note that with
// partial initialization support, one might have `a.x`
// initialized but not `a.b`.
//
// OK scenarios:
//
// 5. Move of `a.b.c`, use of `a.b.d`
// 6. Uninitialized `a.x`, initialized `a.b`, use of `a.b`
// 7. Copied `(a.b: &_)`, use of `*(a.b).c`; note that `a.b`
// 4. Move of `a.b.c`, use of `a.b.d`
// 5. Uninitialized `a.x`, initialized `a.b`, use of `a.b`
// 6. Copied `(a.b: &_)`, use of `*(a.b).c`; note that `a.b`
// must have been initialized for the use to be sound.
// 8. Move of `a.b.c` then reinit of `a.b.c.d`, use of `a.b.c.d`
// 7. Move of `a.b.c` then reinit of `a.b.c.d`, use of `a.b.c.d`

// The dataflow tracks shallow prefixes distinctly (that is,
// field-accesses on P distinctly from P itself), in order to
Expand All @@ -1350,9 +1353,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
// have a MovePath, that should capture the initialization
// state for the place scenario.
//
// This code covers scenarios 1, 2, and 4.
// This code covers scenarios 1, 2, and 3.

debug!("check_if_path_is_moved part1 place: {:?}", place);
debug!("check_if_full_path_is_moved place: {:?}", place);
match self.move_path_closest_to(place) {
Ok(mpi) => {
if maybe_uninits.contains(&mpi) {
Expand All @@ -1372,18 +1375,52 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
// ancestors; dataflow recurs on children when parents
// move (to support partial (re)inits).
//
// (I.e. querying parents breaks scenario 8; but may want
// (I.e. querying parents breaks scenario 7; but may want
// to do such a query based on partial-init feature-gate.)
}
}

fn check_if_path_or_subpath_is_moved(
&mut self,
context: Context,
desired_action: InitializationRequiringAction,
place_span: (&Place<'tcx>, Span),
flow_state: &Flows<'cx, 'gcx, 'tcx>,
) {
// FIXME: analogous code in check_loans first maps `place` to
Copy link
Contributor

Choose a reason for hiding this comment

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

what code are you referring to exactly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly same code let place = self.base_path(place_span.0); is in check_if_full_path_is_moved also with the comment. So If somebody decides to refactor this he will probably do both check_if_full_path_is_moved, check_if_path_or_subpath_is_moved

// its base_path ... but is that what we want here?
let place = self.base_path(place_span.0);

let maybe_uninits = &flow_state.uninits;
let curr_move_outs = &flow_state.move_outs;

// Bad scenarios:
//
// 1. Move of `a.b.c`, use of `a` or `a.b`
// partial initialization support, one might have `a.x`
// initialized but not `a.b`.
// 2. All bad scenarios from `check_if_full_path_is_moved`
//
// OK scenarios:
//
// 3. Move of `a.b.c`, use of `a.b.d`
// 4. Uninitialized `a.x`, initialized `a.b`, use of `a.b`
// 5. Copied `(a.b: &_)`, use of `*(a.b).c`; note that `a.b`
// must have been initialized for the use to be sound.
// 6. Move of `a.b.c` then reinit of `a.b.c.d`, use of `a.b.c.d`

self.check_if_full_path_is_moved(context, desired_action, place_span, flow_state);

// A move of any shallow suffix of `place` also interferes
// with an attempt to use `place`. This is scenario 3 above.
//
// (Distinct from handling of scenarios 1+2+4 above because
// `place` does not interfere with suffixes of its prefixes,
// e.g. `a.b.c` does not interfere with `a.b.d`)
//
// This code covers scenario 1.

debug!("check_if_path_is_moved part2 place: {:?}", place);
debug!("check_if_path_or_subpath_is_moved place: {:?}", place);
if let Some(mpi) = self.move_path_for_place(place) {
if let Some(child_mpi) = maybe_uninits.has_any_child_of(mpi) {
self.report_use_of_moved_or_uninitialized(
Expand Down Expand Up @@ -1443,7 +1480,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
(place, span): (&Place<'tcx>, Span),
flow_state: &Flows<'cx, 'gcx, 'tcx>,
) {
// recur down place; dispatch to check_if_path_is_moved when necessary
debug!("check_if_assigned_path_is_moved place: {:?}", place);
// recur down place; dispatch to external checks when necessary
let mut place = place;
loop {
match *place {
Expand All @@ -1454,17 +1492,25 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
Place::Projection(ref proj) => {
let Projection { ref base, ref elem } = **proj;
match *elem {
ProjectionElem::Deref |
// assigning to *P requires `P` initialized.
ProjectionElem::Index(_/*operand*/) |
ProjectionElem::ConstantIndex { .. } |
// assigning to P[i] requires `P` initialized.
// assigning to P[i] requires P to be valid.
ProjectionElem::Downcast(_/*adt_def*/, _/*variant_idx*/) =>
// assigning to (P->variant) is okay if assigning to `P` is okay
//
// FIXME: is this true even if P is a adt with a dtor?
{ }

// assigning to (*P) requires P to be initialized
ProjectionElem::Deref => {
self.check_if_full_path_is_moved(
context, InitializationRequiringAction::Use,
(base, span), flow_state);
// (base initialized; no need to
// recur further)
break;
}

ProjectionElem::Subslice { .. } => {
panic!("we don't allow assignments to subslices, context: {:?}",
context);
Expand All @@ -1482,7 +1528,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
// check_loans.rs first maps
// `base` to its base_path.

self.check_if_path_is_moved(
self.check_if_path_or_subpath_is_moved(
context, InitializationRequiringAction::Assignment,
(base, span), flow_state);

Expand Down
38 changes: 38 additions & 0 deletions src/test/compile-fail/borrowck/borrowck-issue-48962.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(nll)]

struct Node {
elem: i32,
next: Option<Box<Node>>,
}

fn a() {
let mut node = Node {
elem: 5,
next: None,
};

let mut src = &mut node;
{src};
src.next = None; //~ ERROR use of moved value: `src` [E0382]
}

fn b() {
let mut src = &mut (22, 44);
{src};
src.0 = 66; //~ ERROR use of moved value: `src` [E0382]
}

fn main() {
a();
b();
}
43 changes: 43 additions & 0 deletions src/test/run-pass/issue-48962.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// Test that we are able to reinitilize box with moved referent
#![feature(nll)]
static mut ORDER: [usize; 3] = [0, 0, 0];
static mut INDEX: usize = 0;

struct Dropee (usize);

impl Drop for Dropee {
fn drop(&mut self) {
unsafe {
ORDER[INDEX] = self.0;
INDEX = INDEX + 1;
}
}
}

fn add_sentintel() {
unsafe {
ORDER[INDEX] = 2;
INDEX = INDEX + 1;
}
}

fn main() {
let mut x = Box::new(Dropee(1));
*x; // move out from `*x`
add_sentintel();
*x = Dropee(3); // re-initialize `*x`
{x}; // drop value
unsafe {
assert_eq!(ORDER, [1, 2, 3]);
}
}