From 7eba13f45164f42892b1430badae197ec2a77c5a Mon Sep 17 00:00:00 2001 From: David Wood Date: Wed, 4 Jul 2018 20:08:15 +0100 Subject: [PATCH 1/3] Correctly handle an activation from dead code. --- src/librustc_mir/borrow_check/borrow_set.rs | 52 +++++++-------------- src/librustc_mir/borrow_check/path_utils.rs | 10 ++-- src/test/run-pass/issue-51345.rs | 17 +++++++ 3 files changed, 38 insertions(+), 41 deletions(-) create mode 100644 src/test/run-pass/issue-51345.rs diff --git a/src/librustc_mir/borrow_check/borrow_set.rs b/src/librustc_mir/borrow_check/borrow_set.rs index 3d6f49c377226..ce02a7dc16602 100644 --- a/src/librustc_mir/borrow_check/borrow_set.rs +++ b/src/librustc_mir/borrow_check/borrow_set.rs @@ -53,15 +53,13 @@ impl<'tcx> Index for BorrowSet<'tcx> { } } -/// Every two-phase borrow has *exactly one* use (or else it is not a -/// proper two-phase borrow under our current definition). However, not -/// all uses are actually ones that activate the reservation.. In -/// particular, a shared borrow of a `&mut` does not activate the -/// reservation. +/// Location where a two phase borrow is activated, if a borrow +/// is in fact a two phase borrow. #[derive(Copy, Clone, PartialEq, Eq, Debug)] -crate enum TwoPhaseUse { - MutActivate, - SharedUse, +crate enum TwoPhaseActivation { + NotTwoPhase, + NotActivated, + ActivatedAt(Location), } #[derive(Debug)] @@ -69,9 +67,8 @@ crate struct BorrowData<'tcx> { /// Location where the borrow reservation starts. /// In many cases, this will be equal to the activation location but not always. crate reserve_location: Location, - /// Location where the borrow is activated. None if this is not a - /// 2-phase borrow. - crate activation_location: Option<(TwoPhaseUse, Location)>, + /// Location where the borrow is activated. + crate activation_location: TwoPhaseActivation, /// What kind of borrow this is crate kind: mir::BorrowKind, /// The region for which this borrow is live @@ -116,19 +113,6 @@ impl<'tcx> BorrowSet<'tcx> { visitor.visit_basic_block_data(block, block_data); } - // Double check: We should have found an activation for every pending - // activation. - assert_eq!( - visitor - .pending_activations - .iter() - .find(|&(_local, &borrow_index)| visitor.idx_vec[borrow_index] - .activation_location - .is_none()), - None, - "never found an activation for this borrow!", - ); - BorrowSet { borrows: visitor.idx_vec, location_map: visitor.location_map, @@ -183,7 +167,7 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> { kind, region, reserve_location: location, - activation_location: None, + activation_location: TwoPhaseActivation::NotTwoPhase, borrowed_place: borrowed_place.clone(), assigned_place: assigned_place.clone(), }; @@ -232,38 +216,34 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> { return; } - if let Some(other_activation) = borrow_data.activation_location { + if let TwoPhaseActivation::ActivatedAt(other_location) = + borrow_data.activation_location { span_bug!( self.mir.source_info(location).span, "found two uses for 2-phase borrow temporary {:?}: \ {:?} and {:?}", temp, location, - other_activation, + other_location, ); } // Otherwise, this is the unique later use // that we expect. - - let two_phase_use; - - match context { + borrow_data.activation_location = match context { // The use of TMP in a shared borrow does not // count as an actual activation. PlaceContext::Borrow { kind: mir::BorrowKind::Shared, .. } => { - two_phase_use = TwoPhaseUse::SharedUse; + TwoPhaseActivation::NotActivated } _ => { - two_phase_use = TwoPhaseUse::MutActivate; self.activation_map .entry(location) .or_insert(Vec::new()) .push(borrow_index); + TwoPhaseActivation::ActivatedAt(location) } - } - - borrow_data.activation_location = Some((two_phase_use, location)); + }; } None => {} diff --git a/src/librustc_mir/borrow_check/path_utils.rs b/src/librustc_mir/borrow_check/path_utils.rs index 8ae98bde00344..ca2a120ceb737 100644 --- a/src/librustc_mir/borrow_check/path_utils.rs +++ b/src/librustc_mir/borrow_check/path_utils.rs @@ -8,7 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use borrow_check::borrow_set::{BorrowSet, BorrowData, TwoPhaseUse}; +use borrow_check::borrow_set::{BorrowSet, BorrowData, TwoPhaseActivation}; use borrow_check::places_conflict; use borrow_check::Context; use borrow_check::ShallowOrDeep; @@ -83,11 +83,11 @@ pub(super) fn is_active<'tcx>( let activation_location = match borrow_data.activation_location { // If this is not a 2-phase borrow, it is always active. - None => return true, + TwoPhaseActivation::NotTwoPhase => return true, // And if the unique 2-phase use is not an activation, then it is *never* active. - Some((TwoPhaseUse::SharedUse, _)) => return false, - // Otherwise, we derive info from the activation point `v`: - Some((TwoPhaseUse::MutActivate, v)) => v, + TwoPhaseActivation::NotActivated => return false, + // Otherwise, we derive info from the activation point `loc`: + TwoPhaseActivation::ActivatedAt(loc) => loc, }; // Otherwise, it is active for every location *except* in between diff --git a/src/test/run-pass/issue-51345.rs b/src/test/run-pass/issue-51345.rs new file mode 100644 index 0000000000000..7d392944685b8 --- /dev/null +++ b/src/test/run-pass/issue-51345.rs @@ -0,0 +1,17 @@ +// Copyright 2012 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)] + +fn main() { + let mut v = Vec::new(); + + loop { v.push(break) } +} From aeb16828945ef7d3cda88d27da96a1b475acd90a Mon Sep 17 00:00:00 2001 From: David Wood Date: Wed, 4 Jul 2018 21:35:38 +0100 Subject: [PATCH 2/3] Ensure that borrows wind up unactivated. --- src/librustc_mir/borrow_check/borrow_set.rs | 12 ++++++++++++ src/test/run-fail/issue-51345.rs | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 src/test/run-fail/issue-51345.rs diff --git a/src/librustc_mir/borrow_check/borrow_set.rs b/src/librustc_mir/borrow_check/borrow_set.rs index ce02a7dc16602..a427b9dd5b7d5 100644 --- a/src/librustc_mir/borrow_check/borrow_set.rs +++ b/src/librustc_mir/borrow_check/borrow_set.rs @@ -237,6 +237,14 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> { TwoPhaseActivation::NotActivated } _ => { + // Double check: We should have found an activation for every pending + // activation. + assert_eq!( + borrow_data.activation_location, + TwoPhaseActivation::NotActivated, + "never found an activation for this borrow!", + ); + self.activation_map .entry(location) .or_insert(Vec::new()) @@ -322,6 +330,10 @@ impl<'a, 'gcx, 'tcx> GatherBorrows<'a, 'gcx, 'tcx> { ); }; + // Consider the borrow not activated. + let borrow_data = &mut self.idx_vec[borrow_index]; + borrow_data.activation_location = TwoPhaseActivation::NotActivated; + // Insert `temp` into the list of pending activations. From // now on, we'll be on the lookout for a use of it. Note that // we are guaranteed that this use will come after the diff --git a/src/test/run-fail/issue-51345.rs b/src/test/run-fail/issue-51345.rs new file mode 100644 index 0000000000000..9efd08195a1bb --- /dev/null +++ b/src/test/run-fail/issue-51345.rs @@ -0,0 +1,18 @@ +// Copyright 2012 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. + +// error-pattern: thread 'main' panicked at 'explicit panic' + +#![feature(nll)] + +fn main() { + let mut vec = vec![]; + vec.push((vec.len(), panic!())); +} From f90eada1c97dd0d0a752d34090a957661789379a Mon Sep 17 00:00:00 2001 From: David Wood Date: Wed, 4 Jul 2018 21:48:25 +0100 Subject: [PATCH 3/3] Improve comments. --- src/librustc_mir/borrow_check/borrow_set.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/borrow_check/borrow_set.rs b/src/librustc_mir/borrow_check/borrow_set.rs index a427b9dd5b7d5..e9e0c5c361353 100644 --- a/src/librustc_mir/borrow_check/borrow_set.rs +++ b/src/librustc_mir/borrow_check/borrow_set.rs @@ -237,8 +237,9 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> { TwoPhaseActivation::NotActivated } _ => { - // Double check: We should have found an activation for every pending - // activation. + // Double check: This borrow is indeed a two-phase borrow (that is, + // we are 'transitioning' from `NotActivated` to `ActivatedAt`) and + // we've not found any other activations (checked above). assert_eq!( borrow_data.activation_location, TwoPhaseActivation::NotActivated, @@ -330,7 +331,8 @@ impl<'a, 'gcx, 'tcx> GatherBorrows<'a, 'gcx, 'tcx> { ); }; - // Consider the borrow not activated. + // Consider the borrow not activated to start. When we find an activation, we'll update + // this field. let borrow_data = &mut self.idx_vec[borrow_index]; borrow_data.activation_location = TwoPhaseActivation::NotActivated;