From 6bc4b5051190940ea180fdf4ff8bd93aac473d9b Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Tue, 5 Dec 2017 15:08:10 +0200 Subject: [PATCH 01/11] MIR borrowck: implement union-and-array-compatible semantics Fixes #44831. Fixes #44834. Fixes #45537. Fixes #45696 (by implementing DerefPure semantics, which is what we want going forward). --- src/librustc_mir/borrow_check/mod.rs | 408 +++++++++++++++--- ...-thread-local-static-borrow-outlives-fn.rs | 1 + .../borrowck/borrowck-union-borrow.rs | 13 +- .../borrowck-vec-pattern-move-tail.rs | 3 +- src/test/compile-fail/issue-25579.rs | 2 +- 5 files changed, 368 insertions(+), 59 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 446aba3d3d72c..5d68b30df7019 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -36,6 +36,8 @@ use dataflow::move_paths::{IllegalMoveOriginKind, MoveError}; use dataflow::move_paths::{HasMoveData, LookupResult, MoveData, MoveOutIndex, MovePathIndex}; use util::borrowck_errors::{BorrowckErrors, Origin}; +use std::iter; + use self::MutateMode::{JustWrite, WriteAndRead}; pub(crate) mod nll; @@ -710,7 +712,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { context, (sd, place_span.0), flow_state, - |this, _index, borrow, common_prefix| match (rw, borrow.kind) { + |this, _index, borrow| match (rw, borrow.kind) { (Read(_), BorrowKind::Shared) => Control::Continue, (Read(kind), BorrowKind::Unique) | (Read(kind), BorrowKind::Mut) => { match kind { @@ -727,7 +729,6 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { error_reported = true; this.report_conflicting_borrow( context, - common_prefix, place_span, bk, &borrow, @@ -748,7 +749,6 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { error_reported = true; this.report_conflicting_borrow( context, - common_prefix, place_span, bk, &borrow, @@ -1478,7 +1478,356 @@ enum NoMovePathFound { ReachedStatic, } +/// The degree of overlap between 2 places for borrow-checking. +enum Overlap { + /// The places might partially overlap - in this case, we give + /// up and say that they might conflict. This occurs when + /// different fields of a union are borrowed. For example, + /// if `u` is a union, we have no way of telling how disjoint + /// `u.a.x` and `a.b.y` are. + Arbitrary, + /// The places are either completely disjoint or equal - this + /// is the "base case" on which we recur for extensions of + /// the place. + EqualOrDisjoint, + /// The places are disjoint, so we know all extensions of them + /// will also be disjoint. + Disjoint, +} + impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { + // Given that the bases of `elem1` and `elem2` are always either equal + // or disjoint (and have the same type!), return the overlap situation + // between `elem1` and `elem2`. + fn place_element_conflict(&self, + elem1: &Place<'tcx>, + elem2: &Place<'tcx>) + -> Overlap + { + match (elem1, elem2) { + (Place::Local(l1), Place::Local(l2)) => { + if l1 == l2 { + // the same local - base case, equal + debug!("place_element_conflict: DISJOINT-OR-EQ-LOCAL"); + Overlap::EqualOrDisjoint + } else { + // different locals - base case, disjoint + debug!("place_element_conflict: DISJOINT-LOCAL"); + Overlap::Disjoint + } + } + (Place::Static(statik1), Place::Static(statik2)) => { + // We ignore borrows of mutable statics elsewhere, but + // we need to keep track of thread-locals so we can + // complain if they live loner than the function. + if statik1.def_id == statik2.def_id { + debug!("place_element_conflict: DISJOINT-OR-EQ-STATIC"); + Overlap::EqualOrDisjoint + } else { + debug!("place_element_conflict: DISJOINT-STATIC"); + Overlap::Disjoint + } + } + (Place::Local(_), Place::Static(_)) | + (Place::Static(_), Place::Local(_)) => { + debug!("place_element_conflict: DISJOINT-STATIC-LOCAL"); + Overlap::Disjoint + } + (Place::Projection(pi1), Place::Projection(pi2)) => { + match (&pi1.elem, &pi2.elem) { + (ProjectionElem::Deref, ProjectionElem::Deref) => { + // derefs (e.g. `*x` vs. `*x`) - recur. + debug!("place_element_conflict: DISJOINT-OR-EQ-DEREF"); + Overlap::EqualOrDisjoint + } + (ProjectionElem::Field(f1, _), ProjectionElem::Field(f2, _)) => { + if f1 == f2 { + // same field (e.g. `a.y` vs. `a.y`) - recur. + debug!("place_element_conflict: DISJOINT-OR-EQ-FIELD"); + Overlap::EqualOrDisjoint + } else { + let ty = pi1.base.ty(self.mir, self.tcx).to_ty(self.tcx); + match ty.sty { + ty::TyAdt(def, _) if def.is_union() => { + // Different fields of a union, we are basically stuck. + debug!("place_element_conflict: STUCK-UNION"); + Overlap::Arbitrary + } + _ => { + // Different fields of a struct (`a.x` vs. `a.y`). Disjoint! + debug!("place_element_conflict: DISJOINT-FIELD"); + Overlap::Disjoint + } + } + } + } + (ProjectionElem::Downcast(_, v1), ProjectionElem::Downcast(_, v2)) => { + // different variants are treated as having disjoint fields, + // even if they occupy the same "space", because it's + // impossible for 2 variants of the same enum to exist + // (and therefore, to be borrowed) at the same time. + // + // Note that this is different from unions - we *do* allow + // this code to compile: + // + // ``` + // fn foo(x: &mut Result) { + // let mut v = None; + // if let Ok(ref mut a) = *x { + // v = Some(a); + // } + // // here, you would *think* that the + // // *entirety* of `x` would be borrowed, + // // but in fact only the `Ok` variant is, + // // so the `Err` variant is *entirely free*: + // if let Err(ref mut a) = *x { + // v = Some(a); + // } + // drop(v); + // } + // ``` + if v1 == v2 { + debug!("place_element_conflict: DISJOINT-OR-EQ-FIELD"); + Overlap::EqualOrDisjoint + } else { + debug!("place_element_conflict: DISJOINT-FIELD"); + Overlap::Disjoint + } + } + (ProjectionElem::Index(..), ProjectionElem::Index(..)) | + (ProjectionElem::Index(..), ProjectionElem::ConstantIndex { .. }) | + (ProjectionElem::Index(..), ProjectionElem::Subslice { .. }) | + (ProjectionElem::ConstantIndex { .. }, ProjectionElem::Index(..)) | + (ProjectionElem::ConstantIndex { .. }, ProjectionElem::ConstantIndex { .. }) | + (ProjectionElem::ConstantIndex { .. }, ProjectionElem::Subslice { .. }) | + (ProjectionElem::Subslice { .. }, ProjectionElem::Index(..)) | + (ProjectionElem::Subslice { .. }, ProjectionElem::ConstantIndex { .. }) | + (ProjectionElem::Subslice { .. }, ProjectionElem::Subslice { .. }) => { + // Array indexes (`a[0]` vs. `a[i]`). These can either be disjoint + // (if the indexes differ) or equal (if they are the same), so this + // is the recursive case that gives "equal *or* disjoint" its meaning. + // + // Note that by construction, MIR at borrowck can't subdivide + // `Subslice` accesses (e.g. `a[2..3][i]` will never be present) - they + // are only present in slice patterns, and we "merge together" nested + // slice patterns. That means we don't have to think about these. It's + // probably a good idea to assert this somewhere, but I'm too lazy. + // + // FIXME(#8636) we might want to return Disjoint if + // both projections are constant and disjoint. + debug!("place_element_conflict: DISJOINT-OR-EQ-ARRAY"); + Overlap::EqualOrDisjoint + } + + (ProjectionElem::Deref, _) | + (ProjectionElem::Field(..), _) | + (ProjectionElem::Index(..), _) | + (ProjectionElem::ConstantIndex { .. }, _) | + (ProjectionElem::Subslice { .. }, _) | + (ProjectionElem::Downcast(..), _) => { + bug!("mismatched projections in place_element_conflict: {:?} and {:?}", + + elem1, elem2) + } + } + } + (Place::Projection(_), _) | + (_, Place::Projection(_)) => { + bug!("unexpected elements in place_element_conflict: {:?} and {:?}", + elem1, elem2) + } + } + } + fn borrow_conflicts_with_place(&mut self, + borrow: &BorrowData<'tcx>, + place: &Place<'tcx>, + access: ShallowOrDeep) + -> bool + { + debug!("borrow_conflicts_with_place({:?},{:?},{:?})", borrow, place, access); + + // Return all the prefixes of `place` in reverse order, including + // downcasts. + fn place_elements<'a, 'tcx>(place: &'a Place<'tcx>) -> Vec<&'a Place<'tcx>> + { + let mut result = vec![]; + let mut place = place; + loop { + result.push(place); + match place { + Place::Projection(interior) => { + place = &interior.base; + } + _ => { + result.reverse(); + return result; + } + } + } + } + + let borrow_components = place_elements(&borrow.place); + let access_components = place_elements(place); + debug!("borrow_conflicts_with_place: components {:?} / {:?}", + borrow_components, access_components); + + let borrow_components = borrow_components.into_iter() + .map(Some).chain(iter::repeat(None)); + let access_components = access_components.into_iter() + .map(Some).chain(iter::repeat(None)); + // The borrowck rules for proving disjointness are applied from the "root" of the + // borrow forwards, iterating over "similar" projections in lockstep until + // we can prove overlap one way or another. Essentially, we treat `Overlap` as + // a monoid and report a conflict if the product ends up not being `Disjoint`. + // + // At each step, if we didn't run out of borrow or place, we know that our elements + // have the same type, and that they only overlap if they are the identical. + // + // For example, if we are comparing these: + // BORROW: (*x1[2].y).z.a + // ACCESS: (*x1[i].y).w.b + // + // Then our steps are: + // x1 | x1 -- places are the same + // x1[2] | x1[i] -- equal or disjoint (disjoint if indexes differ) + // x1[2].y | x1[i].y -- equal or disjoint + // *x1[2].y | *x1[i].y -- equal or disjoint + // (*x1[2].y).z | (*x1[i].y).w -- we are disjoint and don't need to check more! + // + // Because `zip` does potentially bad things to the iterator inside, this loop + // also handles the case where the access might be a *prefix* of the borrow, e.g. + // + // BORROW: (*x1[2].y).z.a + // ACCESS: x1[i].y + // + // Then our steps are: + // x1 | x1 -- places are the same + // x1[2] | x1[i] -- equal or disjoint (disjoint if indexes differ) + // x1[2].y | x1[i].y -- equal or disjoint + // + // -- here we run out of access - the borrow can access a part of it. If this + // is a full deep access, then we *know* the borrow conflicts with it. However, + // if the access is shallow, then we can proceed: + // + // x1[2].y | (*x1[i].y) -- a deref! the access can't get past this, so we + // are disjoint + // + // Our invariant is, that at each step of the iteration: + // - If we didn't run out of access to match, our borrow and access are comparable + // and either equal or disjoint. + // - If we did run out of accesss, the borrow can access a part of it. + for (borrow_c, access_c) in borrow_components.zip(access_components) { + // loop invariant: borrow_c is always either equal to access_c or disjoint from it. + debug!("borrow_conflicts_with_place: {:?} vs. {:?}", borrow_c, access_c); + match (borrow_c, access_c) { + (None, _) => { + // If we didn't run out of access, the borrow can access all of our + // place (e.g. a borrow of `a.b` with an access to `a.b.c`), + // so we have a conflict. + // + // If we did, then we still know that the borrow can access a *part* + // of our place that our access cares about (a borrow of `a.b.c` + // with an access to `a.b`), so we still have a conflict. + // + // FIXME: Differs from AST-borrowck; includes drive-by fix + // to #38899. Will probably need back-compat mode flag. + debug!("borrow_conflict_with_place: full borrow, CONFLICT"); + return true; + } + (Some(borrow_c), None) => { + // We know that the borrow can access a part of our place. This + // is a conflict if that is a part our access cares about. + + let (base, elem) = match borrow_c { + Place::Projection(box Projection { base, elem }) => (base, elem), + _ => bug!("place has no base?") + }; + let base_ty = base.ty(self.mir, self.tcx).to_ty(self.tcx); + + match (elem, &base_ty.sty, access) { + (_, _, Shallow(Some(ArtificialField::Discriminant))) | + (_, _, Shallow(Some(ArtificialField::ArrayLength))) => { + // The discriminant and array length are like + // additional fields on the type; they do not + // overlap any existing data there. Furthermore, + // they cannot actually be a prefix of any + // borrowed place (at least in MIR as it is + // currently.) + // + // e.g. a (mutable) borrow of `a[5]` while we read the + // array length of `a`. + debug!("borrow_conflicts_with_place: implicit field"); + return false; + } + + (ProjectionElem::Deref, _, Shallow(None)) => { + // e.g. a borrow of `*x.y` while we shallowly access `x.y` or some + // prefix thereof - the shallow access can't touch anything behind + // the pointer. + debug!("borrow_conflicts_with_place: shallow access behind ptr"); + return false; + } + (ProjectionElem::Deref, ty::TyRef(_, ty::TypeAndMut { + ty: _, mutbl: hir::MutImmutable + }), _) => { + // the borrow goes through a dereference of a shared reference. + // + // I'm not sure why we are tracking these borrows - shared + // references can *always* be aliased, which means the + // permission check already account for this borrow. + debug!("borrow_conflicts_with_place: behind a shared ref"); + return false; + } + + (ProjectionElem::Deref, _, Deep) | + (ProjectionElem::Field { .. }, _, _) | + (ProjectionElem::Index { ..}, _, _) | + (ProjectionElem::ConstantIndex { .. }, _, _) | + (ProjectionElem::Subslice { .. }, _, _) | + (ProjectionElem::Downcast { .. }, _, _) => { + // Recursive case. This can still be disjoint on a + // further iteration if this a shallow access and + // there's a deref later on, e.g. a borrow + // of `*x.y` while accessing `x`. + } + } + } + (Some(borrow_c), Some(access_c)) => { + match self.place_element_conflict(&borrow_c, access_c) { + Overlap::Arbitrary => { + // We have encountered different fields of potentially + // the same union - the borrow now partially overlaps. + // + // There is no *easy* way of comparing the fields + // further on, because they might have different types + // (e.g. borrows of `u.a.0` and `u.b.y` where `.0` and + // `.y` come from different structs). + // + // We could try to do some things here - e.g. count + // dereferences - but that's probably not a good + // idea, at least for now, so just give up and + // report a conflict. This is unsafe code anyway so + // the user could always use raw pointers. + debug!("borrow_conflicts_with_place: arbitrary -> conflict"); + return true; + } + Overlap::EqualOrDisjoint => { + // This is the recursive case - proceed to the next element. + } + Overlap::Disjoint => { + // We have proven the borrow disjoint - further + // projections will remain disjoint. + debug!("borrow_conflicts_with_place: disjoint"); + return false; + } + } + + } + } + } + unreachable!("iter::repeat returned None") + } + fn each_borrow_involving_path( &mut self, _context: Context, @@ -1486,7 +1835,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { flow_state: &InProgress<'cx, 'gcx, 'tcx>, mut op: F, ) where - F: FnMut(&mut Self, BorrowIndex, &BorrowData<'tcx>, &Place<'tcx>) -> Control, + F: FnMut(&mut Self, BorrowIndex, &BorrowData<'tcx>) -> Control, { let (access, place) = access_place; @@ -1501,47 +1850,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { 'next_borrow: for i in flow_state.borrows.elems_incoming() { let borrowed = &data[i]; - // Is `place` (or a prefix of it) already borrowed? If - // so, that's relevant. - // - // FIXME: Differs from AST-borrowck; includes drive-by fix - // to #38899. Will probably need back-compat mode flag. - for accessed_prefix in self.prefixes(place, PrefixSet::All) { - if *accessed_prefix == borrowed.place { - // FIXME: pass in enum describing case we are in? - let ctrl = op(self, i, borrowed, accessed_prefix); - if ctrl == Control::Break { - return; - } - } - } - - // Is `place` a prefix (modulo access type) of the - // `borrowed.place`? If so, that's relevant. - - let prefix_kind = match access { - Shallow(Some(ArtificialField::Discriminant)) | - Shallow(Some(ArtificialField::ArrayLength)) => { - // The discriminant and array length are like - // additional fields on the type; they do not - // overlap any existing data there. Furthermore, - // they cannot actually be a prefix of any - // borrowed place (at least in MIR as it is - // currently.) - continue 'next_borrow; - } - Shallow(None) => PrefixSet::Shallow, - Deep => PrefixSet::Supporting, - }; - - for borrowed_prefix in self.prefixes(&borrowed.place, prefix_kind) { - if borrowed_prefix == place { - // FIXME: pass in enum describing case we are in? - let ctrl = op(self, i, borrowed, borrowed_prefix); - if ctrl == Control::Break { - return; - } - } + if self.borrow_conflicts_with_place(borrowed, place, access) { + let ctrl = op(self, i, borrowed); + if ctrl == Control::Break { return; } } } } @@ -1595,6 +1906,7 @@ mod prefixes { } #[derive(Copy, Clone, PartialEq, Eq, Debug)] + #[allow(dead_code)] pub(super) enum PrefixSet { /// Doesn't stop until it returns the base case (a Local or /// Static prefix). @@ -1907,17 +2219,11 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { fn report_conflicting_borrow( &mut self, context: Context, - common_prefix: &Place<'tcx>, (place, span): (&Place<'tcx>, Span), gen_borrow_kind: BorrowKind, issued_borrow: &BorrowData, end_issued_loan_span: Option, ) { - use self::prefixes::IsPrefixOf; - - assert!(common_prefix.is_prefix_of(place)); - assert!(common_prefix.is_prefix_of(&issued_borrow.place)); - let issued_span = self.retrieve_borrow_span(issued_borrow); let new_closure_span = self.find_closure_span(span, context.loc); diff --git a/src/test/compile-fail/borrowck/borrowck-thread-local-static-borrow-outlives-fn.rs b/src/test/compile-fail/borrowck/borrowck-thread-local-static-borrow-outlives-fn.rs index f2e6d51d064d1..df0a05dfaee0e 100644 --- a/src/test/compile-fail/borrowck/borrowck-thread-local-static-borrow-outlives-fn.rs +++ b/src/test/compile-fail/borrowck/borrowck-thread-local-static-borrow-outlives-fn.rs @@ -8,6 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +// ignore-test will be fixed later // revisions: ast mir //[mir]compile-flags: -Z borrowck=mir diff --git a/src/test/compile-fail/borrowck/borrowck-union-borrow.rs b/src/test/compile-fail/borrowck/borrowck-union-borrow.rs index 975de8b6c41f9..0241b3870c7e6 100644 --- a/src/test/compile-fail/borrowck/borrowck-union-borrow.rs +++ b/src/test/compile-fail/borrowck/borrowck-union-borrow.rs @@ -52,12 +52,12 @@ fn main() { { let ra = &u.a; let rmb = &mut u.b; //[ast]~ ERROR cannot borrow `u` (via `u.b`) as mutable because `u` is also borrowed as immutable (via `u.a`) - // FIXME Error for MIR (needs support for union) + //[mir]~^ ERROR cannot borrow `u.b` as mutable because it is also borrowed as immutable } { let ra = &u.a; u.b = 1; //[ast]~ ERROR cannot assign to `u.b` because it is borrowed - // FIXME Error for MIR (needs support for union) + //[mir]~^ ERROR cannot assign to `u.b` because it is borrowed } // Mut borrow, same field { @@ -84,22 +84,23 @@ fn main() { { let rma = &mut u.a; let rb = &u.b; //[ast]~ ERROR cannot borrow `u` (via `u.b`) as immutable because `u` is also borrowed as mutable (via `u.a`) - // FIXME Error for MIR (needs support for union) + //[mir]~^ ERROR cannot borrow `u.b` as immutable because it is also borrowed as mutable } { let ra = &mut u.a; let b = u.b; //[ast]~ ERROR cannot use `u.b` because it was mutably borrowed - // FIXME Error for MIR (needs support for union) + //[mir]~^ ERROR cannot use `u.b` because it was mutably borrowed + } { let rma = &mut u.a; let rmb2 = &mut u.b; //[ast]~ ERROR cannot borrow `u` (via `u.b`) as mutable more than once at a time - // FIXME Error for MIR (needs support for union) + //[mir]~^ ERROR cannot borrow `u.b` as mutable more than once at a time } { let rma = &mut u.a; u.b = 1; //[ast]~ ERROR cannot assign to `u.b` because it is borrowed - // FIXME Error for MIR (needs support for union) + //[mir]~^ ERROR cannot assign to `u.b` because it is borrowed } } } diff --git a/src/test/compile-fail/borrowck/borrowck-vec-pattern-move-tail.rs b/src/test/compile-fail/borrowck/borrowck-vec-pattern-move-tail.rs index 304a41c14ed33..94877b27d5888 100644 --- a/src/test/compile-fail/borrowck/borrowck-vec-pattern-move-tail.rs +++ b/src/test/compile-fail/borrowck/borrowck-vec-pattern-move-tail.rs @@ -1,3 +1,4 @@ + // Copyright 2014 The Rust Project Developers. See the COPYRIGHT // file at the top-level directory of this distribution and at // http://rust-lang.org/COPYRIGHT. @@ -22,7 +23,7 @@ fn main() { println!("t[0]: {}", t[0]); a[2] = 0; //[ast]~ ERROR cannot assign to `a[..]` because it is borrowed //[cmp]~^ ERROR cannot assign to `a[..]` because it is borrowed (Ast) - // FIXME Error for MIR (error missed) + //[cmp]~| ERROR cannot assign to `a[..]` because it is borrowed (Mir) println!("t[0]: {}", t[0]); t[0]; } diff --git a/src/test/compile-fail/issue-25579.rs b/src/test/compile-fail/issue-25579.rs index 9e12d5b5de157..b4a5cd72d86de 100644 --- a/src/test/compile-fail/issue-25579.rs +++ b/src/test/compile-fail/issue-25579.rs @@ -24,8 +24,8 @@ fn causes_ice(mut l: &mut Sexpression) { //[mir]~| ERROR [E0499] l = &mut **expr; //[ast]~ ERROR [E0506] //[mir]~^ ERROR [E0506] - //[mir]~| ERROR [E0506] //[mir]~| ERROR [E0499] + //[mir]~| ERROR [E0506] //[mir]~| ERROR [E0499] } }} From 87a8a70d0d208e845cd49c3304bd56a2f3415d5a Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Sun, 3 Dec 2017 15:15:29 +0200 Subject: [PATCH 02/11] MIR borrowck: avoid formatting state when it is not needed This improves performance on large functions. --- src/librustc_mir/borrow_check/mod.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 5d68b30df7019..fdcee3a6cade0 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -36,6 +36,7 @@ use dataflow::move_paths::{IllegalMoveOriginKind, MoveError}; use dataflow::move_paths::{HasMoveData, LookupResult, MoveData, MoveOutIndex, MovePathIndex}; use util::borrowck_errors::{BorrowckErrors, Origin}; +use std::fmt; use std::iter; use self::MutateMode::{JustWrite, WriteAndRead}; @@ -308,8 +309,7 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx } fn visit_block_entry(&mut self, bb: BasicBlock, flow_state: &Self::FlowState) { - let summary = flow_state.summary(); - debug!("MirBorrowckCtxt::process_block({:?}): {}", bb, summary); + debug!("MirBorrowckCtxt::process_block({:?}): {}", bb, flow_state); } fn visit_statement_entry( @@ -318,12 +318,11 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx stmt: &Statement<'tcx>, flow_state: &Self::FlowState, ) { - let summary = flow_state.summary(); debug!( "MirBorrowckCtxt::process_statement({:?}, {:?}): {}", location, stmt, - summary + flow_state ); let span = stmt.source_info.span; match stmt.kind { @@ -425,12 +424,11 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx flow_state: &Self::FlowState, ) { let loc = location; - let summary = flow_state.summary(); debug!( "MirBorrowckCtxt::process_terminator({:?}, {:?}): {}", location, term, - summary + flow_state ); let span = term.source_info.span; match term.kind { @@ -2679,8 +2677,10 @@ impl<'b, 'gcx, 'tcx> InProgress<'b, 'gcx, 'tcx> { xform_move_outs(&mut self.move_outs); xform_ever_inits(&mut self.ever_inits); } +} - fn summary(&self) -> String { +impl<'b, 'gcx, 'tcx> fmt::Display for InProgress<'b, 'gcx, 'tcx> { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { let mut s = String::new(); s.push_str("borrows in effect: ["); @@ -2757,7 +2757,7 @@ impl<'b, 'gcx, 'tcx> InProgress<'b, 'gcx, 'tcx> { }); s.push_str("]"); - return s; + fmt::Display::fmt(&s, fmt) } } From f8c35d9e4171ea94b93dd7c6cacaab6c15da6dc0 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Sun, 3 Dec 2017 16:08:28 +0200 Subject: [PATCH 03/11] fix handling of consts in borrow-checking I'm not sure how correct it this, but it gets whatever needs to compile to compile. --- src/librustc_mir/borrow_check/mod.rs | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index fdcee3a6cade0..890680b8b293b 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -192,6 +192,11 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( node_id: id, move_data: &mdpe.move_data, param_env: param_env, + locals_are_invalidated_at_exit: match tcx.hir.body_owner_kind(id) { + hir::BodyOwnerKind::Const | + hir::BodyOwnerKind::Static(_) => false, + hir::BodyOwnerKind::Fn => true, + }, storage_dead_or_drop_error_reported: FxHashSet(), }; @@ -223,6 +228,9 @@ pub struct MirBorrowckCtxt<'cx, 'gcx: 'tcx, 'tcx: 'cx> { node_id: ast::NodeId, move_data: &'cx MoveData<'tcx>, param_env: ParamEnv<'gcx>, + /// This keeps track of whether local variables are free-ed when the function + /// exits even without a `StorageDead`. + locals_are_invalidated_at_exit: bool, /// This field keeps track of when storage dead or drop errors are reported /// in order to stop duplicate error reporting and identify the conditions required /// for a "temporary value dropped here while still borrowed" error. See #45360. @@ -957,6 +965,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // FIXME(nll-rfc#40): do more precise destructor tracking here. For now // we just know that all locals are dropped at function exit (otherwise // we'll have a memory leak) and assume that all statics have a destructor. + // + // FIXME: allow thread-locals to borrow other thread locals?x let (might_be_alive, will_be_dropped) = match root_place { Place::Static(statik) => { // Thread-locals might be dropped after the function exits, but @@ -971,7 +981,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { Place::Local(_) => { // Locals are always dropped at function exit, and if they // have a destructor it would've been called already. - (false, true) + (false, self.locals_are_invalidated_at_exit) } Place::Projection(..) => { bug!("root of {:?} is a projection ({:?})?", place, root_place) @@ -1514,17 +1524,10 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { Overlap::Disjoint } } - (Place::Static(statik1), Place::Static(statik2)) => { - // We ignore borrows of mutable statics elsewhere, but - // we need to keep track of thread-locals so we can - // complain if they live loner than the function. - if statik1.def_id == statik2.def_id { - debug!("place_element_conflict: DISJOINT-OR-EQ-STATIC"); - Overlap::EqualOrDisjoint - } else { - debug!("place_element_conflict: DISJOINT-STATIC"); - Overlap::Disjoint - } + (Place::Static(..), Place::Static(..)) => { + // Borrows of statics do not have to be tracked here. + debug!("place_element_conflict: IGNORED-STATIC"); + Overlap::Disjoint } (Place::Local(_), Place::Static(_)) | (Place::Static(_), Place::Local(_)) => { From cbcae7f69414fd0e5de6bad771df12bfde4f2908 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Sun, 3 Dec 2017 17:55:41 +0200 Subject: [PATCH 04/11] improve conflict error reporting --- src/librustc_mir/borrow_check/mod.rs | 28 +++++++++++++------ .../borrowck-local-borrow-outlives-fn.rs | 2 +- ...wck-local-borrow-with-panic-outlives-fn.rs | 2 +- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 890680b8b293b..61e7eb5933915 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -548,14 +548,13 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx if self.place_is_invalidated_at_exit(&borrow.place) { debug!("borrow conflicts at exit {:?}", borrow); - let borrow_span = self.mir.source_info(borrow.location).span; // FIXME: should be talking about the region lifetime instead // of just a span here. let end_span = domain.opt_region_end_span(&borrow.region); self.report_borrowed_value_does_not_live_long_enough( ContextKind::StorageDead.new(loc), - (&borrow.place, borrow_span), + (&borrow.place, end_span.unwrap_or(span)), end_span, ) } @@ -958,7 +957,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { /// Returns whether a borrow of this place is invalidated when the function /// exits - fn place_is_invalidated_at_exit(&self, place: &Place<'tcx>) -> bool { + fn place_is_invalidated_at_exit(&mut self, place: &Place<'tcx>) -> bool { debug!("place_is_invalidated_at_exit({:?})", place); let root_place = self.prefixes(place, PrefixSet::All).last().unwrap(); @@ -967,7 +966,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // we'll have a memory leak) and assume that all statics have a destructor. // // FIXME: allow thread-locals to borrow other thread locals?x - let (might_be_alive, will_be_dropped) = match root_place { + let (might_be_alive, will_be_dropped, local) = match root_place { Place::Static(statik) => { // Thread-locals might be dropped after the function exits, but // "true" statics will never be. @@ -976,12 +975,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { .iter() .any(|attr| attr.check_name("thread_local")); - (true, is_thread_local) + (true, is_thread_local, None) } - Place::Local(_) => { + Place::Local(local) => { // Locals are always dropped at function exit, and if they // have a destructor it would've been called already. - (false, self.locals_are_invalidated_at_exit) + (false, self.locals_are_invalidated_at_exit, Some(*local)) } Place::Projection(..) => { bug!("root of {:?} is a projection ({:?})?", place, root_place) @@ -1004,8 +1003,19 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { PrefixSet::Shallow }; - self.prefixes(place, prefix_set) - .any(|prefix| prefix == root_place) + let result = + self.prefixes(place, prefix_set).any(|prefix| prefix == root_place); + + if result { + if let Some(local) = local { + if let Some(_) = self.storage_dead_or_drop_error_reported.replace(local) { + debug!("place_is_invalidated_at_exit({:?}) - suppressed", place); + return false; + } + } + } + + result } } diff --git a/src/test/compile-fail/borrowck/borrowck-local-borrow-outlives-fn.rs b/src/test/compile-fail/borrowck/borrowck-local-borrow-outlives-fn.rs index 3f5725029d887..fa80a2efdf83d 100644 --- a/src/test/compile-fail/borrowck/borrowck-local-borrow-outlives-fn.rs +++ b/src/test/compile-fail/borrowck/borrowck-local-borrow-outlives-fn.rs @@ -13,7 +13,7 @@ fn cplusplus_mode(x: isize) -> &'static isize { &x //[ast]~ ERROR `x` does not live long enough - //[mir]~^ ERROR borrowed value does not live long enough } +//[mir]~^ ERROR borrowed value does not live long enough fn main() {} diff --git a/src/test/compile-fail/borrowck/borrowck-local-borrow-with-panic-outlives-fn.rs b/src/test/compile-fail/borrowck/borrowck-local-borrow-with-panic-outlives-fn.rs index 7009c6f33e61a..78f0d321e0d3d 100644 --- a/src/test/compile-fail/borrowck/borrowck-local-borrow-with-panic-outlives-fn.rs +++ b/src/test/compile-fail/borrowck/borrowck-local-borrow-with-panic-outlives-fn.rs @@ -14,9 +14,9 @@ fn cplusplus_mode_exceptionally_unsafe(x: &mut Option<&'static mut isize>) { let mut z = (0, 0); *x = Some(&mut z.1); //[ast]~ ERROR [E0597] - //[mir]~^ ERROR [E0597] panic!("catch me for a dangling pointer!") } +//[mir]~^ ERROR [E0597] fn main() { cplusplus_mode_exceptionally_unsafe(&mut None); From 243c5a5faaed0c34f0f236c1f168e10b4cc97d25 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Mon, 4 Dec 2017 00:56:06 +0200 Subject: [PATCH 05/11] fix handling of CallScopeData This fixes the tests for issue #29793 --- src/librustc_mir/borrow_check/mod.rs | 9 +++++- src/librustc_mir/build/cfg.rs | 11 +++++++ src/librustc_mir/dataflow/impls/borrows.rs | 29 +++++++++++++++++-- .../region-borrow-params-issue-29793-big.rs | 10 +++++-- 4 files changed, 53 insertions(+), 6 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 61e7eb5933915..a8af5f5d32b97 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -12,6 +12,7 @@ use rustc::hir; use rustc::hir::def_id::DefId; +use rustc::hir::map::definitions::DefPathData; use rustc::infer::InferCtxt; use rustc::ty::{self, ParamEnv, TyCtxt}; use rustc::ty::maps::Providers; @@ -131,6 +132,12 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( move_data: move_data, param_env: param_env, }; + let body_id = match tcx.def_key(def_id).disambiguated_data.data { + DefPathData::StructCtor | + DefPathData::EnumVariant(_) => None, + _ => Some(tcx.hir.body_owned_by(id)) + }; + let dead_unwinds = IdxSetBuf::new_empty(mir.basic_blocks().len()); let mut flow_inits = FlowInProgress::new(do_dataflow( tcx, @@ -206,7 +213,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( id, &attributes, &dead_unwinds, - Borrows::new(tcx, mir, opt_regioncx), + Borrows::new(tcx, mir, opt_regioncx, def_id, body_id), |bd, i| bd.location(i), )); diff --git a/src/librustc_mir/build/cfg.rs b/src/librustc_mir/build/cfg.rs index d1bb1f39e221c..932aad0bb1d84 100644 --- a/src/librustc_mir/build/cfg.rs +++ b/src/librustc_mir/build/cfg.rs @@ -51,6 +51,17 @@ impl<'tcx> CFG<'tcx> { source_info: SourceInfo, region_scope: region::Scope) { if tcx.sess.emit_end_regions() { + if let region::ScopeData::CallSite(_) = region_scope.data() { + // The CallSite scope (aka the root scope) is sort of weird, in that it is + // supposed to "separate" the "interior" and "exterior" of a closure. Being + // that, it is not really a part of the region hierarchy, but for some + // reason it *is* considered a part of it. + // + // It should die a hopefully painful death with NLL, so let's leave this hack + // for now so that nobody can complain about soundness. + return + } + self.push(block, Statement { source_info, kind: StatementKind::EndRegion(region_scope), diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index 2bba3e263f3c6..b3b06f7f6cdb0 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -8,6 +8,9 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +use rustc::hir; +use rustc::hir::def_id::DefId; +use rustc::middle::region; use rustc::mir::{self, Location, Mir}; use rustc::mir::visit::Visitor; use rustc::ty::{self, Region, TyCtxt}; @@ -27,6 +30,7 @@ use borrow_check::nll::ToRegionVid; use syntax_pos::Span; use std::fmt; +use std::rc::Rc; // `Borrows` maps each dataflow bit to an `Rvalue::Ref`, which can be // uniquely identified in the MIR by the `Location` of the assigment @@ -34,6 +38,8 @@ use std::fmt; pub struct Borrows<'a, 'gcx: 'tcx, 'tcx: 'a> { tcx: TyCtxt<'a, 'gcx, 'tcx>, mir: &'a Mir<'tcx>, + scope_tree: Rc, + root_scope: Option, borrows: IndexVec>, location_map: FxHashMap, region_map: FxHashMap, FxHashSet>, @@ -69,8 +75,14 @@ impl<'tcx> fmt::Display for BorrowData<'tcx> { impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { pub fn new(tcx: TyCtxt<'a, 'gcx, 'tcx>, mir: &'a Mir<'tcx>, - nonlexical_regioncx: Option>) + nonlexical_regioncx: Option>, + def_id: DefId, + body_id: Option) -> Self { + let scope_tree = tcx.region_scope_tree(def_id); + let root_scope = body_id.map(|body_id| { + region::Scope::CallSite(tcx.hir.body(body_id).value.hir_id.local_id) + }); let mut visitor = GatherBorrows { tcx, mir, @@ -83,6 +95,8 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { return Borrows { tcx: tcx, mir: mir, borrows: visitor.idx_vec, + scope_tree, + root_scope, location_map: visitor.location_map, region_map: visitor.region_map, region_span_map: visitor.region_span_map, @@ -253,8 +267,17 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { // like unwind paths, we do not always emit `EndRegion` statements, so we // add some kills here as a "backup" and to avoid spurious error messages. for (borrow_index, borrow_data) in self.borrows.iter_enumerated() { - if let ReScope(..) = borrow_data.region { - sets.kill(&borrow_index); + if let ReScope(scope) = borrow_data.region { + // Check that the scope is not actually a scope from a function that is + // a parent of our closure. Note that the CallSite scope itself is + // *outside* of the closure, for some weird reason. + if let Some(root_scope) = self.root_scope { + if *scope != root_scope && + self.scope_tree.is_subscope_of(*scope, root_scope) + { + sets.kill(&borrow_index); + } + } } } } diff --git a/src/test/compile-fail/region-borrow-params-issue-29793-big.rs b/src/test/compile-fail/region-borrow-params-issue-29793-big.rs index 887f7836ee143..a4dc00bd2b1e7 100644 --- a/src/test/compile-fail/region-borrow-params-issue-29793-big.rs +++ b/src/test/compile-fail/region-borrow-params-issue-29793-big.rs @@ -16,6 +16,10 @@ // behavior (because the improperly accepted closure was actually // able to be invoked). +// ignore-tidy-linelength +// revisions: ast mir +//[mir]compile-flags: -Z borrowck=mir + struct WrapA(Option); impl WrapA { @@ -75,9 +79,11 @@ impl WrapA fn main() { let mut w = WrapA::new().set(|x: usize, y: usize| { WrapB::new().set(|t: bool| if t { x } else { y }) // (separate errors for `x` vs `y`) - //~^ ERROR `x` does not live long enough - //~| ERROR `y` does not live long enough + //[ast]~^ ERROR `x` does not live long enough + //[ast]~| ERROR `y` does not live long enough }); + //[mir]~^ ERROR borrowed value does not live long enough + //[mir]~| ERROR borrowed value does not live long enough w.handle(); // This works // w.handle_ref(); // This doesn't From 210f76816fe54ff43ed7eca91a78096589fae953 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Mon, 4 Dec 2017 01:00:46 +0200 Subject: [PATCH 06/11] handle gen/kill sets together --- src/librustc_mir/dataflow/impls/mod.rs | 66 +++++-------------- src/librustc_mir/dataflow/mod.rs | 30 +++++++++ .../borrowck-move-moved-value-into-closure.rs | 6 +- 3 files changed, 51 insertions(+), 51 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/mod.rs b/src/librustc_mir/dataflow/impls/mod.rs index 50c8df3c2e3d0..e7a25c212c367 100644 --- a/src/librustc_mir/dataflow/impls/mod.rs +++ b/src/librustc_mir/dataflow/impls/mod.rs @@ -14,7 +14,6 @@ use rustc::ty::TyCtxt; use rustc::mir::{self, Mir, Location}; -use rustc_data_structures::bitslice::BitSlice; // adds set_bit/get_bit to &[usize] bitvector rep. use rustc_data_structures::bitslice::{BitwiseOperator}; use rustc_data_structures::indexed_set::{IdxSet}; use rustc_data_structures::indexed_vec::Idx; @@ -504,7 +503,6 @@ impl<'a, 'gcx, 'tcx> BitDenotation for MovingOutStatements<'a, 'gcx, 'tcx> { let stmt = &mir[location.block].statements[location.statement_index]; let loc_map = &move_data.loc_map; let path_map = &move_data.path_map; - let bits_per_block = self.bits_per_block(); match stmt.kind { // this analysis only tries to find moves explicitly @@ -515,21 +513,15 @@ impl<'a, 'gcx, 'tcx> BitDenotation for MovingOutStatements<'a, 'gcx, 'tcx> { _ => { debug!("stmt {:?} at loc {:?} moves out of move_indexes {:?}", stmt, location, &loc_map[location]); - for move_index in &loc_map[location] { - // Every path deinitialized by a *particular move* - // has corresponding bit, "gen'ed" (i.e. set) - // here, in dataflow vector - zero_to_one(sets.gen_set.words_mut(), *move_index); - } + // Every path deinitialized by a *particular move* + // has corresponding bit, "gen'ed" (i.e. set) + // here, in dataflow vector + sets.gen_all_and_assert_dead(&loc_map[location]); } } for_location_inits(tcx, mir, move_data, location, - |mpi| for moi in &path_map[mpi] { - assert!(moi.index() < bits_per_block); - sets.kill_set.add(&moi); - } - ); + |mpi| sets.kill_all(&path_map[mpi])); } fn terminator_effect(&self, @@ -543,18 +535,10 @@ impl<'a, 'gcx, 'tcx> BitDenotation for MovingOutStatements<'a, 'gcx, 'tcx> { debug!("terminator {:?} at loc {:?} moves out of move_indexes {:?}", term, location, &loc_map[location]); - let bits_per_block = self.bits_per_block(); - for move_index in &loc_map[location] { - assert!(move_index.index() < bits_per_block); - zero_to_one(sets.gen_set.words_mut(), *move_index); - } + sets.gen_all_and_assert_dead(&loc_map[location]); for_location_inits(tcx, mir, move_data, location, - |mpi| for moi in &path_map[mpi] { - assert!(moi.index() < bits_per_block); - sets.kill_set.add(&moi); - } - ); + |mpi| sets.kill_all(&path_map[mpi])); } fn propagate_call_return(&self, @@ -585,11 +569,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for EverInitializedLvals<'a, 'gcx, 'tcx> { } fn start_block_effect(&self, sets: &mut BlockSets) { - let bits_per_block = self.bits_per_block(); - for init_index in (0..self.mir.arg_count).map(InitIndex::new) { - assert!(init_index.index() < bits_per_block); - sets.gen_set.add(&init_index); - } + sets.gen_all((0..self.mir.arg_count).map(InitIndex::new)); } fn statement_effect(&self, sets: &mut BlockSets, @@ -599,14 +579,10 @@ impl<'a, 'gcx, 'tcx> BitDenotation for EverInitializedLvals<'a, 'gcx, 'tcx> { let init_path_map = &move_data.init_path_map; let init_loc_map = &move_data.init_loc_map; let rev_lookup = &move_data.rev_lookup; - let bits_per_block = self.bits_per_block(); debug!("statement {:?} at loc {:?} initializes move_indexes {:?}", stmt, location, &init_loc_map[location]); - for init_index in &init_loc_map[location] { - assert!(init_index.index() < bits_per_block); - sets.gen_set.add(init_index); - } + sets.gen_all(&init_loc_map[location]); match stmt.kind { mir::StatementKind::StorageDead(local) => { @@ -614,11 +590,8 @@ impl<'a, 'gcx, 'tcx> BitDenotation for EverInitializedLvals<'a, 'gcx, 'tcx> { // be reinitialized on the next iteration of the loop. if let LookupResult::Exact(mpi) = rev_lookup.find(&mir::Place::Local(local)) { debug!("stmt {:?} at loc {:?} clears the ever initialized status of {:?}", - stmt, location, &init_path_map[mpi]); - for ii in &init_path_map[mpi] { - assert!(ii.index() < bits_per_block); - sets.kill_set.add(&ii); - } + stmt, location, &init_path_map[mpi]); + sets.kill_all(&init_path_map[mpi]); } } _ => {} @@ -634,13 +607,11 @@ impl<'a, 'gcx, 'tcx> BitDenotation for EverInitializedLvals<'a, 'gcx, 'tcx> { let init_loc_map = &move_data.init_loc_map; debug!("terminator {:?} at loc {:?} initializes move_indexes {:?}", term, location, &init_loc_map[location]); - let bits_per_block = self.bits_per_block(); - for init_index in &init_loc_map[location] { - if move_data.inits[*init_index].kind != InitKind::NonPanicPathOnly { - assert!(init_index.index() < bits_per_block); - sets.gen_set.add(init_index); - } - } + sets.gen_all( + init_loc_map[location].iter().filter(|init_index| { + move_data.inits[**init_index].kind != InitKind::NonPanicPathOnly + }) + ); } fn propagate_call_return(&self, @@ -663,11 +634,6 @@ impl<'a, 'gcx, 'tcx> BitDenotation for EverInitializedLvals<'a, 'gcx, 'tcx> { } } -fn zero_to_one(bitvec: &mut [usize], move_index: MoveOutIndex) { - let retval = bitvec.set_bit(move_index.index()); - assert!(retval); -} - impl<'a, 'gcx, 'tcx> BitwiseOperator for MaybeInitializedLvals<'a, 'gcx, 'tcx> { #[inline] fn join(&self, pred1: usize, pred2: usize) -> usize { diff --git a/src/librustc_mir/dataflow/mod.rs b/src/librustc_mir/dataflow/mod.rs index 6be006b1ea972..12722979706fc 100644 --- a/src/librustc_mir/dataflow/mod.rs +++ b/src/librustc_mir/dataflow/mod.rs @@ -18,6 +18,7 @@ use rustc::ty::{self, TyCtxt}; use rustc::mir::{self, Mir, BasicBlock, BasicBlockData, Location, Statement, Terminator}; use rustc::session::Session; +use std::borrow::Borrow; use std::fmt::{self, Debug}; use std::io; use std::mem; @@ -492,10 +493,39 @@ impl<'a, E:Idx> BlockSets<'a, E> { self.gen_set.add(e); self.kill_set.remove(e); } + fn gen_all(&mut self, i: I) + where I: IntoIterator, + I::Item: Borrow + { + for j in i { + self.gen(j.borrow()); + } + } + + fn gen_all_and_assert_dead(&mut self, i: I) + where I: IntoIterator, + I::Item: Borrow + { + for j in i { + let j = j.borrow(); + let retval = self.gen_set.add(j); + self.kill_set.remove(j); + assert!(retval); + } + } + fn kill(&mut self, e: &E) { self.gen_set.remove(e); self.kill_set.add(e); } + fn kill_all(&mut self, i: I) + where I: IntoIterator, + I::Item: Borrow + { + for j in i { + self.kill(j.borrow()); + } + } } impl AllSets { diff --git a/src/test/compile-fail/borrowck/borrowck-move-moved-value-into-closure.rs b/src/test/compile-fail/borrowck/borrowck-move-moved-value-into-closure.rs index c46bcbb32b9cf..2bd6f75df1b1f 100644 --- a/src/test/compile-fail/borrowck/borrowck-move-moved-value-into-closure.rs +++ b/src/test/compile-fail/borrowck/borrowck-move-moved-value-into-closure.rs @@ -8,6 +8,9 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +// revisions: ast mir +//[mir]compile-flags: -Z borrowck=mir + #![feature(box_syntax)] fn call_f isize>(f: F) -> isize { @@ -18,5 +21,6 @@ fn main() { let t: Box<_> = box 3; call_f(move|| { *t + 1 }); - call_f(move|| { *t + 1 }); //~ ERROR capture of moved value + call_f(move|| { *t + 1 }); //[ast]~ ERROR capture of moved value + //[mir]~^ ERROR use of moved value } From 82f3fc5dbca4d2d696ce66b902cb339f4507ae9f Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Mon, 4 Dec 2017 13:01:50 +0200 Subject: [PATCH 07/11] fix handling of immutable variables --- src/librustc_mir/borrow_check/mod.rs | 44 ++++++++++++++++++---------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index a8af5f5d32b97..0f4585c8c040a 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -655,8 +655,9 @@ enum WriteKind { /// - Take flow state into consideration in `is_assignable()` for local variables #[derive(Copy, Clone, PartialEq, Eq, Debug)] enum LocalMutationIsAllowed { + Move, Yes, - No, + No } #[derive(Copy, Clone)] @@ -946,7 +947,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { context, (place, span), (Deep, Write(WriteKind::Move)), - LocalMutationIsAllowed::Yes, + LocalMutationIsAllowed::Move, flow_state, ); @@ -1368,7 +1369,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { let local = &self.mir.local_decls[local]; match local.mutability { Mutability::Not => match is_local_mutation_allowed { - LocalMutationIsAllowed::Yes => Ok(()), + LocalMutationIsAllowed::Yes | + LocalMutationIsAllowed::Move => Ok(()), LocalMutationIsAllowed::No => Err(place), }, Mutability::Mut => Ok(()), @@ -1393,10 +1395,14 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // Mutably borrowed data is mutable, but only if we have a // unique path to the `&mut` hir::MutMutable => { - if self.is_upvar_field_projection(&proj.base).is_some() { - self.is_mutable(&proj.base, is_local_mutation_allowed) - } else { - self.is_unique(&proj.base) + match self.is_upvar_field_projection(&proj.base) { + Some(field) if { + self.mir.upvar_decls[field.index()].by_ref + } => { + self.is_mutable(&proj.base, + is_local_mutation_allowed) + } + _ => self.is_unique(&proj.base) } } } @@ -1412,7 +1418,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } // `Box` owns its content, so mutable if its location is mutable _ if base_ty.is_box() => { - self.is_mutable(&proj.base, LocalMutationIsAllowed::No) + self.is_mutable(&proj.base, is_local_mutation_allowed) } // Deref should only be for reference, pointers or boxes _ => bug!("Deref of unexpected type: {:?}", base_ty), @@ -1429,14 +1435,17 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { if let Some(field) = field_projection { let decl = &self.mir.upvar_decls[field.index()]; - - return match decl.mutability { - Mutability::Mut => self.is_unique(&proj.base), - Mutability::Not => Err(place), + debug!("decl.mutability={:?} local_mutation_is_allowed={:?} place={:?}", + decl, is_local_mutation_allowed, place); + return match (decl.mutability, is_local_mutation_allowed) { + (Mutability::Not, LocalMutationIsAllowed::No) | + (Mutability::Not, LocalMutationIsAllowed::Yes) => Err(place), + (Mutability::Not, LocalMutationIsAllowed::Move) | + (Mutability::Mut, _) => self.is_unique(&proj.base), }; } - self.is_mutable(&proj.base, LocalMutationIsAllowed::No) + self.is_mutable(&proj.base, is_local_mutation_allowed) } } } @@ -1450,9 +1459,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // Local variables are unique Ok(()) } - Place::Static(..) => { - // Static variables are not - Err(place) + Place::Static(ref static_) => { + if !self.tcx.is_static_mut(static_.def_id) { + Err(place) + } else { + Ok(()) + } } Place::Projection(ref proj) => { match proj.elem { From 6b9fbb2ab85914820da703182a5c51acf59088cf Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Mon, 4 Dec 2017 13:21:28 +0200 Subject: [PATCH 08/11] fix borrows across loops, libcore *almost* compiles --- src/librustc_mir/dataflow/impls/borrows.rs | 31 +++++++++++++++++-- .../borrowck-mut-borrow-linear-errors.rs | 10 ++---- src/test/compile-fail/issue-25579.rs | 6 +--- 3 files changed, 32 insertions(+), 15 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index b3b06f7f6cdb0..c27cb43eff77b 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -43,6 +43,7 @@ pub struct Borrows<'a, 'gcx: 'tcx, 'tcx: 'a> { borrows: IndexVec>, location_map: FxHashMap, region_map: FxHashMap, FxHashSet>, + local_map: FxHashMap>, region_span_map: FxHashMap, nonlexical_regioncx: Option>, } @@ -89,6 +90,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { idx_vec: IndexVec::new(), location_map: FxHashMap(), region_map: FxHashMap(), + local_map: FxHashMap(), region_span_map: FxHashMap() }; visitor.visit_mir(mir); @@ -99,6 +101,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { root_scope, location_map: visitor.location_map, region_map: visitor.region_map, + local_map: visitor.local_map, region_span_map: visitor.region_span_map, nonlexical_regioncx }; @@ -108,6 +111,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { idx_vec: IndexVec>, location_map: FxHashMap, region_map: FxHashMap, FxHashSet>, + local_map: FxHashMap>, region_span_map: FxHashMap, } @@ -115,6 +119,14 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { fn visit_rvalue(&mut self, rvalue: &mir::Rvalue<'tcx>, location: mir::Location) { + fn root_local(mut p: &mir::Place<'_>) -> Option { + loop { match p { + mir::Place::Projection(pi) => p = &pi.base, + mir::Place::Static(_) => return None, + mir::Place::Local(l) => return Some(*l) + }} + } + if let mir::Rvalue::Ref(region, kind, ref place) = *rvalue { if is_unsafe_place(self.tcx, self.mir, place) { return; } @@ -123,8 +135,14 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { }; let idx = self.idx_vec.push(borrow); self.location_map.insert(location, idx); + let borrows = self.region_map.entry(region).or_insert(FxHashSet()); borrows.insert(idx); + + if let Some(local) = root_local(place) { + let borrows = self.local_map.entry(local).or_insert(FxHashSet()); + borrows.insert(idx); + } } } @@ -213,7 +231,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { mir::StatementKind::EndRegion(region_scope) => { if let Some(borrow_indexes) = self.region_map.get(&ReScope(region_scope)) { assert!(self.nonlexical_regioncx.is_none()); - for idx in borrow_indexes { sets.kill(&idx); } + sets.kill_all(borrow_indexes); } else { // (if there is no entry, then there are no borrows to be tracked) } @@ -238,10 +256,19 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { } } + mir::StatementKind::StorageDead(local) => { + // Make sure there are no remaining borrows for locals that + // are gone out of scope. + // + // FIXME: expand this to variables that are assigned over. + if let Some(borrow_indexes) = self.local_map.get(&local) { + sets.kill_all(borrow_indexes); + } + } + mir::StatementKind::InlineAsm { .. } | mir::StatementKind::SetDiscriminant { .. } | mir::StatementKind::StorageLive(..) | - mir::StatementKind::StorageDead(..) | mir::StatementKind::Validate(..) | mir::StatementKind::Nop => {} diff --git a/src/test/compile-fail/borrowck/borrowck-mut-borrow-linear-errors.rs b/src/test/compile-fail/borrowck/borrowck-mut-borrow-linear-errors.rs index 6896d166e7a4a..63bb04a0e4c3a 100644 --- a/src/test/compile-fail/borrowck/borrowck-mut-borrow-linear-errors.rs +++ b/src/test/compile-fail/borrowck/borrowck-mut-borrow-linear-errors.rs @@ -23,15 +23,9 @@ fn main() { 1 => { addr = &mut x; } //[ast]~ ERROR [E0499] //[mir]~^ ERROR [E0499] 2 => { addr = &mut x; } //[ast]~ ERROR [E0499] - //[mir]~^ ERROR [E0506] - //[mir]~| ERROR [E0499] - //[mir]~| ERROR [E0499] + //[mir]~^ ERROR [E0499] _ => { addr = &mut x; } //[ast]~ ERROR [E0499] - //[mir]~^ ERROR [E0506] - //[mir]~| ERROR [E0499] - //[mir]~| ERROR [E0499] + //[mir]~^ ERROR [E0499] } } } - - diff --git a/src/test/compile-fail/issue-25579.rs b/src/test/compile-fail/issue-25579.rs index b4a5cd72d86de..5f5a58ed75942 100644 --- a/src/test/compile-fail/issue-25579.rs +++ b/src/test/compile-fail/issue-25579.rs @@ -20,13 +20,9 @@ fn causes_ice(mut l: &mut Sexpression) { loop { match l { &mut Sexpression::Num(ref mut n) => {}, &mut Sexpression::Cons(ref mut expr) => { //[ast]~ ERROR [E0499] - //[mir]~^ ERROR [E0506] - //[mir]~| ERROR [E0499] + //[mir]~^ ERROR [E0499] l = &mut **expr; //[ast]~ ERROR [E0506] //[mir]~^ ERROR [E0506] - //[mir]~| ERROR [E0499] - //[mir]~| ERROR [E0506] - //[mir]~| ERROR [E0499] } }} } From 37df5e0b917a9c66f586a40e899fca8727efaac5 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Tue, 5 Dec 2017 14:09:16 +0200 Subject: [PATCH 09/11] adjust libcore --- src/libcore/cell.rs | 6 ++++-- src/libcore/iter/mod.rs | 18 ++++++++++++------ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/libcore/cell.rs b/src/libcore/cell.rs index d4cd3f6264efc..4a66506fec729 100644 --- a/src/libcore/cell.rs +++ b/src/libcore/cell.rs @@ -1084,9 +1084,11 @@ impl<'b, T: ?Sized> RefMut<'b, T> { pub fn map(orig: RefMut<'b, T>, f: F) -> RefMut<'b, U> where F: FnOnce(&mut T) -> &mut U { + // FIXME: fix borrow-check + let RefMut { value, borrow } = orig; RefMut { - value: f(orig.value), - borrow: orig.borrow, + value: f(value), + borrow: borrow, } } } diff --git a/src/libcore/iter/mod.rs b/src/libcore/iter/mod.rs index e173f43b5e6ea..1f7651b75cc83 100644 --- a/src/libcore/iter/mod.rs +++ b/src/libcore/iter/mod.rs @@ -1776,12 +1776,18 @@ impl Iterator for Peekable { #[inline] fn nth(&mut self, n: usize) -> Option { - match self.peeked.take() { - // the .take() below is just to avoid "move into pattern guard" - Some(ref mut v) if n == 0 => v.take(), - Some(None) => None, - Some(Some(_)) => self.iter.nth(n - 1), - None => self.iter.nth(n), + // FIXME: merge these when borrow-checking gets better. + if n == 0 { + match self.peeked.take() { + Some(v) => v, + None => self.iter.nth(n), + } + } else { + match self.peeked.take() { + Some(None) => None, + Some(Some(_)) => self.iter.nth(n - 1), + None => self.iter.nth(n), + } } } From 66c032cb2d5748e36c416e25c75b80f137f3a7fc Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Wed, 6 Dec 2017 00:51:47 +0200 Subject: [PATCH 10/11] more comments --- src/libcore/cell.rs | 2 +- src/libcore/iter/mod.rs | 2 +- src/librustc_mir/borrow_check/mod.rs | 17 +++++++++++------ 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/libcore/cell.rs b/src/libcore/cell.rs index 4a66506fec729..93cfc845b1f34 100644 --- a/src/libcore/cell.rs +++ b/src/libcore/cell.rs @@ -1084,7 +1084,7 @@ impl<'b, T: ?Sized> RefMut<'b, T> { pub fn map(orig: RefMut<'b, T>, f: F) -> RefMut<'b, U> where F: FnOnce(&mut T) -> &mut U { - // FIXME: fix borrow-check + // FIXME(nll-rfc#40): fix borrow-check let RefMut { value, borrow } = orig; RefMut { value: f(value), diff --git a/src/libcore/iter/mod.rs b/src/libcore/iter/mod.rs index 1f7651b75cc83..06c29b47bf921 100644 --- a/src/libcore/iter/mod.rs +++ b/src/libcore/iter/mod.rs @@ -1776,7 +1776,7 @@ impl Iterator for Peekable { #[inline] fn nth(&mut self, n: usize) -> Option { - // FIXME: merge these when borrow-checking gets better. + // FIXME(#6393): merge these when borrow-checking gets better. if n == 0 { match self.peeked.take() { Some(v) => v, diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 0f4585c8c040a..8d3491bd1d988 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -236,7 +236,11 @@ pub struct MirBorrowckCtxt<'cx, 'gcx: 'tcx, 'tcx: 'cx> { move_data: &'cx MoveData<'tcx>, param_env: ParamEnv<'gcx>, /// This keeps track of whether local variables are free-ed when the function - /// exits even without a `StorageDead`. + /// exits even without a `StorageDead`, which appears to be the case for + /// constants. + /// + /// I'm not sure this is the right approach - @eddyb could you try and + /// figure this out? locals_are_invalidated_at_exit: bool, /// This field keeps track of when storage dead or drop errors are reported /// in order to stop duplicate error reporting and identify the conditions required @@ -973,7 +977,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // we just know that all locals are dropped at function exit (otherwise // we'll have a memory leak) and assume that all statics have a destructor. // - // FIXME: allow thread-locals to borrow other thread locals?x + // FIXME: allow thread-locals to borrow other thread locals? let (might_be_alive, will_be_dropped, local) = match root_place { Place::Static(statik) => { // Thread-locals might be dropped after the function exits, but @@ -1523,9 +1527,10 @@ enum Overlap { /// if `u` is a union, we have no way of telling how disjoint /// `u.a.x` and `a.b.y` are. Arbitrary, - /// The places are either completely disjoint or equal - this - /// is the "base case" on which we recur for extensions of - /// the place. + /// The places have the same type, and are either completely disjoint + /// or equal - i.e. they can't "partially" overlap as can occur with + /// unions. This is the "base case" on which we recur for extensions + /// of the place. EqualOrDisjoint, /// The places are disjoint, so we know all extensions of them /// will also be disjoint. @@ -1688,7 +1693,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { Place::Projection(interior) => { place = &interior.base; } - _ => { + Place::Local(_) | Place::Static(_) => { result.reverse(); return result; } From 9d3558725b9110beaa6740e88847e3addc9c2d0d Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Wed, 6 Dec 2017 02:10:24 +0200 Subject: [PATCH 11/11] work around weird match arm lifetimes --- src/librustc_mir/dataflow/impls/mod.rs | 26 ++++++++++++++++++++++--- src/test/run-pass/match-pipe-binding.rs | 1 + 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/mod.rs b/src/librustc_mir/dataflow/impls/mod.rs index e7a25c212c367..db8ca0628c4ed 100644 --- a/src/librustc_mir/dataflow/impls/mod.rs +++ b/src/librustc_mir/dataflow/impls/mod.rs @@ -585,9 +585,29 @@ impl<'a, 'gcx, 'tcx> BitDenotation for EverInitializedLvals<'a, 'gcx, 'tcx> { sets.gen_all(&init_loc_map[location]); match stmt.kind { - mir::StatementKind::StorageDead(local) => { - // End inits for StorageDead, so that an immutable variable can - // be reinitialized on the next iteration of the loop. + mir::StatementKind::StorageDead(local) | + mir::StatementKind::StorageLive(local) => { + // End inits for StorageDead and StorageLive, so that an immutable + // variable can be reinitialized on the next iteration of the loop. + // + // FIXME(#46525): We *need* to do this for StorageLive as well as + // StorageDead, because lifetimes of match bindings with guards are + // weird - i.e. this code + // + // ``` + // fn main() { + // match 0 { + // a | a + // if { println!("a={}", a); false } => {} + // _ => {} + // } + // } + // ``` + // + // runs the guard twice, using the same binding for `a`, and only + // storagedeads after everything ends, so if we don't regard the + // storagelive as killing storage, we would have a multiple assignment + // to immutable data error. if let LookupResult::Exact(mpi) = rev_lookup.find(&mir::Place::Local(local)) { debug!("stmt {:?} at loc {:?} clears the ever initialized status of {:?}", stmt, location, &init_path_map[mpi]); diff --git a/src/test/run-pass/match-pipe-binding.rs b/src/test/run-pass/match-pipe-binding.rs index bda90d3aaecb4..9592da77a1b58 100644 --- a/src/test/run-pass/match-pipe-binding.rs +++ b/src/test/run-pass/match-pipe-binding.rs @@ -8,6 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +// compile-flags: -Z borrowck=compare fn test1() { // from issue 6338