From 4954e3e828377a1034d5e434e0a2b69426497ea4 Mon Sep 17 00:00:00 2001 From: Peter Hrvola Date: Mon, 26 Mar 2018 22:00:14 +0200 Subject: [PATCH 1/3] =?UTF-8?q?fixes=20internal=20compiler=20error:=20libr?= =?UTF-8?q?ustc=5Fmir/transform/elaborate=5Fdrops.rs=20=E2=80=94=20drop=20?= =?UTF-8?q?of=20untracked,=20uninitialized=20value?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix #48962 r? @nikomatsakis --- .../borrow_check/error_reporting.rs | 17 ++++ src/librustc_mir/borrow_check/mod.rs | 84 ++++++++++++++----- .../borrowck/borrowck-issue-48962.rs | 38 +++++++++ src/test/run-pass/issue-48962.rs | 43 ++++++++++ 4 files changed, 161 insertions(+), 21 deletions(-) create mode 100644 src/test/compile-fail/borrowck/borrowck-issue-48962.rs create mode 100644 src/test/run-pass/issue-48962.rs diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs index 56371d809b26a..57847697ba8ed 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/src/librustc_mir/borrow_check/error_reporting.rs @@ -37,6 +37,23 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { .collect::>(); 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: {:?} errors was 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(), diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 6e1a798910dc6..46b01f823a913 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -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, }; @@ -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>, + /// 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>, /// 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. @@ -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), @@ -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, @@ -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), @@ -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), @@ -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), @@ -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), @@ -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. } @@ -1320,18 +1324,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 @@ -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_path_is_moved place: {:?}", place); match self.move_path_closest_to(place) { Ok(mpi) => { if maybe_uninits.contains(&mpi) { @@ -1372,9 +1375,41 @@ 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_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_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. @@ -1382,8 +1417,10 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // (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( @@ -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 { @@ -1454,8 +1492,6 @@ 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. @@ -1465,6 +1501,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // FIXME: is this true even if P is a adt with a dtor? { } + ProjectionElem::Deref => { + self.check_if_path_is_moved( + context, InitializationRequiringAction::Use, + (base, span), flow_state); + } + ProjectionElem::Subslice { .. } => { panic!("we don't allow assignments to subslices, context: {:?}", context); @@ -1482,7 +1524,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); diff --git a/src/test/compile-fail/borrowck/borrowck-issue-48962.rs b/src/test/compile-fail/borrowck/borrowck-issue-48962.rs new file mode 100644 index 0000000000000..e3bbfd9d5fef6 --- /dev/null +++ b/src/test/compile-fail/borrowck/borrowck-issue-48962.rs @@ -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 or the MIT license +// , 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>, +} + +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(); +} diff --git a/src/test/run-pass/issue-48962.rs b/src/test/run-pass/issue-48962.rs new file mode 100644 index 0000000000000..a603dfbfe5fa5 --- /dev/null +++ b/src/test/run-pass/issue-48962.rs @@ -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 or the MIT license +// , 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]); + } +} From dc41851882b4d847308badc7fbb0da4975023bb2 Mon Sep 17 00:00:00 2001 From: Peter Hrvola Date: Sun, 1 Apr 2018 10:01:51 +0200 Subject: [PATCH 2/3] Fixed nits from PR review #49392 --- src/librustc_mir/borrow_check/mod.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 46b01f823a913..0e41ede6aa580 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -1306,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, @@ -1355,7 +1355,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // // This code covers scenarios 1, 2, and 3. - debug!("check_if_path_is_moved 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) { @@ -1399,7 +1399,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // 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_path_is_moved` + // 2. All bad scenarios from `check_if_full_path_is_moved` // // OK scenarios: // @@ -1409,7 +1409,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // 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_path_is_moved(context, desired_action, place_span, flow_state); + 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. @@ -1494,17 +1494,21 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { match *elem { 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_path_is_moved( + self.check_if_full_path_is_moved( context, InitializationRequiringAction::Use, (base, span), flow_state); + // (base initialized; no need to + // recur further) + break; } ProjectionElem::Subslice { .. } => { From 9056c7a8498b73844d2196e43e81120db75db789 Mon Sep 17 00:00:00 2001 From: Peter Hrvola Date: Mon, 2 Apr 2018 09:01:58 +0200 Subject: [PATCH 3/3] Fixed error message from PR review #49392 --- src/librustc_mir/borrow_check/error_reporting.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs index 57847697ba8ed..aaed1dd871bac 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/src/librustc_mir/borrow_check/error_reporting.rs @@ -45,7 +45,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { .contains(&root_place.clone()) { debug!( - "report_use_of_moved_or_uninitialized place: {:?} errors was suppressed", + "report_use_of_moved_or_uninitialized place: error about {:?} suppressed", root_place ); return;