Skip to content

Commit

Permalink
Auto merge of #49392 - retep007:nll-issue-48962, r=nikomatsakis
Browse files Browse the repository at this point in the history
fixes move analysis

Fixed compiler error by correct checking when dereferencing

Fix #48962

r? @nikomatsakis
  • Loading branch information
bors committed Apr 6, 2018
2 parents eeea94c + 9056c7a commit 0e35ddd
Show file tree
Hide file tree
Showing 4 changed files with 167 additions and 23 deletions.
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 @@ -965,7 +969,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 @@ -1025,7 +1029,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 @@ -1053,7 +1057,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 @@ -1100,7 +1104,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 @@ -1118,7 +1122,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 @@ -1269,7 +1273,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 @@ -1304,7 +1308,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 @@ -1322,18 +1326,17 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
//
// 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 @@ -1352,9 +1355,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 @@ -1374,18 +1377,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
// 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 @@ -1445,7 +1482,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 @@ -1456,17 +1494,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 @@ -1484,7 +1530,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]);
}
}

0 comments on commit 0e35ddd

Please sign in to comment.