Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NLL fixes #46984

Merged
merged 3 commits into from
Jan 3, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/librustc_mir/borrow_check/flows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ impl<'b, 'gcx, 'tcx> fmt::Display for Flows<'b, 'gcx, 'tcx> {
};
saw_one = true;
let borrow_data = &self.borrows.operator().borrows()[borrow.borrow_index()];
s.push_str(&format!("{}", borrow_data));
s.push_str(&format!("{}{}", borrow_data,
if borrow.is_activation() { "@active" } else { "" }));
});
s.push_str("] ");

Expand Down
2 changes: 2 additions & 0 deletions src/librustc_mir/borrow_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2011,6 +2011,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
let borrowed = &data[i.borrow_index()];

if self.places_conflict(&borrowed.borrowed_place, place, access) {
debug!("each_borrow_involving_path: {:?} @ {:?} vs. {:?}/{:?}",
i, borrowed, place, access);
let ctrl = op(self, i, borrowed);
if ctrl == Control::Break {
return;
Expand Down
97 changes: 75 additions & 22 deletions src/librustc_mir/borrow_check/nll/constraint_generation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,38 +130,91 @@ impl<'cx, 'cg, 'gcx, 'tcx> ConstraintGeneration<'cx, 'cg, 'gcx, 'tcx> {
});
}

// Add the reborrow constraint at `location` so that `borrowed_place`
// is valid for `borrow_region`.
fn add_reborrow_constraint(
&mut self,
location: Location,
borrow_region: ty::Region<'tcx>,
borrowed_place: &Place<'tcx>,
) {
if let Projection(ref proj) = *borrowed_place {
let PlaceProjection { ref base, ref elem } = **proj;

if let ProjectionElem::Deref = *elem {
let tcx = self.infcx.tcx;
let base_ty = base.ty(self.mir, tcx).to_ty(tcx);
let base_sty = &base_ty.sty;

if let ty::TyRef(base_region, ty::TypeAndMut { ty: _, mutbl }) = *base_sty {
match mutbl {
hir::Mutability::MutImmutable => {}

hir::Mutability::MutMutable => {
self.add_reborrow_constraint(location, borrow_region, base);
let mut borrowed_place = borrowed_place;

debug!("add_reborrow_constraint({:?}, {:?}, {:?})",
location, borrow_region, borrowed_place);
while let Projection(box PlaceProjection { base, elem }) = borrowed_place {
debug!("add_reborrow_constraint - iteration {:?}", borrowed_place);

match *elem {
ProjectionElem::Deref => {
let tcx = self.infcx.tcx;
let base_ty = base.ty(self.mir, tcx).to_ty(tcx);

debug!("add_reborrow_constraint - base_ty = {:?}", base_ty);
match base_ty.sty {
ty::TyRef(ref_region, ty::TypeAndMut { ty: _, mutbl }) => {
let span = self.mir.source_info(location).span;
self.regioncx.add_outlives(
span,
ref_region.to_region_vid(),
borrow_region.to_region_vid(),
location.successor_within_block(),
);

match mutbl {
hir::Mutability::MutImmutable => {
// Immutable reference. We don't need the base
// to be valid for the entire lifetime of
// the borrow.
break
}
hir::Mutability::MutMutable => {
// Mutable reference. We *do* need the base
// to be valid, because after the base becomes
// invalid, someone else can use our mutable deref.

// This is in order to make the following function
// illegal:
// ```
// fn unsafe_deref<'a, 'b>(x: &'a &'b mut T) -> &'b mut T {
// &mut *x
// }
// ```
//
// As otherwise you could clone `&mut T` using the
// following function:
// ```
// fn bad(x: &mut T) -> (&mut T, &mut T) {
// let my_clone = unsafe_deref(&'a x);
// ENDREGION 'a;
// (my_clone, x)
// }
// ```
}
}
}
ty::TyRawPtr(..) => {
// deref of raw pointer, guaranteed to be valid
break
}
ty::TyAdt(def, _) if def.is_box() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

is the change here the inclusion of Box? (i.e., the behavior for other cases is the same?)

// deref of `Box`, need the base to be valid - propagate
}
_ => bug!("unexpected deref ty {:?} in {:?}", base_ty, borrowed_place)
}

let span = self.mir.source_info(location).span;
self.regioncx.add_outlives(
span,
base_region.to_region_vid(),
borrow_region.to_region_vid(),
location.successor_within_block(),
);
}
ProjectionElem::Field(..) |
ProjectionElem::Downcast(..) |
ProjectionElem::Index(..) |
ProjectionElem::ConstantIndex { .. } |
ProjectionElem::Subslice { .. } => {
// other field access
}
}

// The "propagate" case. We need to check that our base is valid
// for the borrow's lifetime.
borrowed_place = base;
}
}
}
24 changes: 24 additions & 0 deletions src/librustc_mir/dataflow/at_location.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,18 @@ impl<BD> FlowsAtLocation for FlowAtLocation<BD>
fn reconstruct_statement_effect(&mut self, loc: Location) {
self.stmt_gen.reset_to_empty();
self.stmt_kill.reset_to_empty();
{
let mut sets = BlockSets {
on_entry: &mut self.curr_state,
gen_set: &mut self.stmt_gen,
kill_set: &mut self.stmt_kill,
};
self.base_results
.operator()
.before_statement_effect(&mut sets, loc);
}
self.apply_local_effect(loc);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, so, I have the edge-effect implemented, but I'm not sure what I think about it. On the one hand, I think the work I did feels like a cleaner approach than the existing "special case" for "call-normal-return". On the other hand, adding edge effects is not quite sufficient to make everything work correctly: the problem has to do with how the "activation" checks in two-phase borrows work.

What I did on my branch is basically that the "statement effect" for some statement S kills all the borrows whose regions do not include the successor S'. In other words, when you check a statement like bb0[5], you kill borrows whose regions do not include bb0[6]. Then, on an edge targeting bb1, you would kill borrows that do not include bb1[0]. (No borrows are killed in the terminator effect.)

This works fine for most things, but it has a funny effect on the activation check. In particular, to determine at some statement S which borrows have been activated, we iterate through the gens that are present in the statement effect. But this isn't quite right: the statement effect here represents the state on entry to S', but we really want to see some point before that -- the statement before as we exit S, but before we enter S'. In particular, if you have some borrow that is activated by S, but goes out of scope in S', we want to see that activation as a "gen", but the setup I described above will gen the bit but then later kill it.

I can see two ways to fix this. The first is to allow gen/kill bits to both be set. In the specific case of activations, this would mean that statement S both gens and kills the same bit. This seems ok but only happens to work because in this case the kill comes temporally after the gen.

The other was adding a method like the one you wound up adding in this PR. And maybe that just suffices, then.

My only nit here is that I'd like to improve the comments on reconstruct_statement_effect -- it feels surprising to me that it "applies" an effect. I think the code is ultimately doing the right thing, though, we just need to be precise in terms of describing what is happening -- in particular, reconstruct_statement_effect is basically setting up the effects so that:

  • the state represents the point where we have "entered" S but not yet executed it
    • (which is why we do an apply here)
  • the gen/kill bits represent the net effect of executing S
    • (but not the effect of entering S')

My mental model then is that there are sort of two points in time for each statement, let's call them S-before and S-after. reconstruct_statement_effect is thus setting our state to S-before and setting up the gen/kill to transition to S-after.

It's probably worth noting somewhere (or maybe even with a debug-assertion) that there is an implicit cursor -- i.e., you can't reconstruct the statement effect for statement 1 if you have not done statement 0 first.

(In any case, we could take the edge effect changes just as a refactoring, but we don't have to. It makes the code more regular but perhaps a bit more complex. I can open the PR and @pnkfelix and I can discuss.)


let mut sets = BlockSets {
on_entry: &mut self.curr_state,
gen_set: &mut self.stmt_gen,
Expand All @@ -162,6 +174,18 @@ impl<BD> FlowsAtLocation for FlowAtLocation<BD>
fn reconstruct_terminator_effect(&mut self, loc: Location) {
self.stmt_gen.reset_to_empty();
self.stmt_kill.reset_to_empty();
{
let mut sets = BlockSets {
on_entry: &mut self.curr_state,
gen_set: &mut self.stmt_gen,
kill_set: &mut self.stmt_kill,
};
self.base_results
.operator()
.before_terminator_effect(&mut sets, loc);
}
self.apply_local_effect(loc);

let mut sets = BlockSets {
on_entry: &mut self.curr_state,
gen_set: &mut self.stmt_gen,
Expand Down
28 changes: 28 additions & 0 deletions src/librustc_mir/dataflow/impls/borrows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -635,13 +635,27 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Reservations<'a, 'gcx, 'tcx> {
// `_sets`.
}

fn before_statement_effect(&self,
sets: &mut BlockSets<ReserveOrActivateIndex>,
location: Location) {
debug!("Reservations::before_statement_effect sets: {:?} location: {:?}", sets, location);
self.0.kill_loans_out_of_scope_at_location(sets, location, false);
}

fn statement_effect(&self,
sets: &mut BlockSets<ReserveOrActivateIndex>,
location: Location) {
debug!("Reservations::statement_effect sets: {:?} location: {:?}", sets, location);
self.0.statement_effect_on_borrows(sets, location, false);
}

fn before_terminator_effect(&self,
sets: &mut BlockSets<ReserveOrActivateIndex>,
location: Location) {
debug!("Reservations::before_terminator_effect sets: {:?} location: {:?}", sets, location);
self.0.kill_loans_out_of_scope_at_location(sets, location, false);
}

fn terminator_effect(&self,
sets: &mut BlockSets<ReserveOrActivateIndex>,
location: Location) {
Expand Down Expand Up @@ -682,13 +696,27 @@ impl<'a, 'gcx, 'tcx> BitDenotation for ActiveBorrows<'a, 'gcx, 'tcx> {
// `_sets`.
}

fn before_statement_effect(&self,
sets: &mut BlockSets<ReserveOrActivateIndex>,
location: Location) {
debug!("ActiveBorrows::before_statement_effect sets: {:?} location: {:?}", sets, location);
self.0.kill_loans_out_of_scope_at_location(sets, location, true);
}

fn statement_effect(&self,
sets: &mut BlockSets<ReserveOrActivateIndex>,
location: Location) {
debug!("ActiveBorrows::statement_effect sets: {:?} location: {:?}", sets, location);
self.0.statement_effect_on_borrows(sets, location, true);
}

fn before_terminator_effect(&self,
sets: &mut BlockSets<ReserveOrActivateIndex>,
location: Location) {
debug!("ActiveBorrows::before_terminator_effect sets: {:?} location: {:?}", sets, location);
self.0.kill_loans_out_of_scope_at_location(sets, location, true);
}

fn terminator_effect(&self,
sets: &mut BlockSets<ReserveOrActivateIndex>,
location: Location) {
Expand Down
47 changes: 44 additions & 3 deletions src/librustc_mir/dataflow/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ impl<'a, 'tcx: 'a, BD> DataflowAnalysis<'a, 'tcx, BD> where BD: BitDenotation
}
for j_stmt in 0..statements.len() {
let location = Location { block: bb, statement_index: j_stmt };
self.flow_state.operator.before_statement_effect(sets, location);
self.flow_state.operator.statement_effect(sets, location);
if track_intrablock {
sets.apply_local_effect();
Expand All @@ -222,6 +223,7 @@ impl<'a, 'tcx: 'a, BD> DataflowAnalysis<'a, 'tcx, BD> where BD: BitDenotation

if terminator.is_some() {
let location = Location { block: bb, statement_index: statements.len() };
self.flow_state.operator.before_terminator_effect(sets, location);
self.flow_state.operator.terminator_effect(sets, location);
if track_intrablock {
sets.apply_local_effect();
Expand Down Expand Up @@ -365,9 +367,10 @@ pub(crate) trait DataflowResultsConsumer<'a, 'tcx: 'a> {
fn mir(&self) -> &'a Mir<'tcx>;
}

pub fn state_for_location<T: BitDenotation>(loc: Location,
analysis: &T,
result: &DataflowResults<T>)
pub fn state_for_location<'tcx, T: BitDenotation>(loc: Location,
analysis: &T,
result: &DataflowResults<T>,
mir: &Mir<'tcx>)
-> IdxSetBuf<T::Idx> {
let mut entry = result.sets().on_entry_set_for(loc.block.index()).to_owned();

Expand All @@ -381,8 +384,16 @@ pub fn state_for_location<T: BitDenotation>(loc: Location,
for stmt in 0..loc.statement_index {
let mut stmt_loc = loc;
stmt_loc.statement_index = stmt;
analysis.before_statement_effect(&mut sets, stmt_loc);
analysis.statement_effect(&mut sets, stmt_loc);
}

// Apply the pre-statement effect of the statement we're evaluating.
if loc.statement_index == mir[loc.block].statements.len() {
analysis.before_terminator_effect(&mut sets, loc);
} else {
analysis.before_statement_effect(&mut sets, loc);
}
}

entry
Expand Down Expand Up @@ -637,6 +648,21 @@ pub trait BitDenotation: BitwiseOperator {
/// (For example, establishing the call arguments.)
fn start_block_effect(&self, entry_set: &mut IdxSet<Self::Idx>);

/// Similar to `statement_effect`, except it applies
/// *just before* the statement rather than *just after* it.
///
/// This matters for "dataflow at location" APIs, because the
/// before-statement effect is visible while visiting the
/// statement, while the after-statement effect only becomes
/// visible at the next statement.
///
/// Both the before-statement and after-statement effects are
/// applied, in that order, before moving for the next
/// statement.
fn before_statement_effect(&self,
_sets: &mut BlockSets<Self::Idx>,
_location: Location) {}

/// Mutates the block-sets (the flow sets for the given
/// basic block) according to the effects of evaluating statement.
///
Expand All @@ -651,6 +677,21 @@ pub trait BitDenotation: BitwiseOperator {
sets: &mut BlockSets<Self::Idx>,
location: Location);

/// Similar to `terminator_effect`, except it applies
/// *just before* the terminator rather than *just after* it.
///
/// This matters for "dataflow at location" APIs, because the
/// before-terminator effect is visible while visiting the
/// terminator, while the after-terminator effect only becomes
/// visible at the terminator's successors.
///
/// Both the before-terminator and after-terminator effects are
/// applied, in that order, before moving for the next
/// terminator.
fn before_terminator_effect(&self,
_sets: &mut BlockSets<Self::Idx>,
_location: Location) {}

/// Mutates the block-sets (the flow sets for the given
/// basic block) according to the effects of evaluating
/// the terminator.
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/transform/generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ fn locals_live_across_suspend_points<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
statement_index: data.statements.len(),
};

let storage_liveness = state_for_location(loc, &analysis, &storage_live);
let storage_liveness = state_for_location(loc, &analysis, &storage_live, mir);

storage_liveness_map.insert(block, storage_liveness.clone());

Expand Down
9 changes: 8 additions & 1 deletion src/librustc_mir/transform/rustc_peek.rs
Original file line number Diff line number Diff line change
Expand Up @@ -203,11 +203,18 @@ fn each_block<'a, 'tcx, O>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
// reset GEN and KILL sets before emulating their effect.
for e in sets.gen_set.words_mut() { *e = 0; }
for e in sets.kill_set.words_mut() { *e = 0; }
results.0.operator.statement_effect(&mut sets, Location { block: bb, statement_index: j });
results.0.operator.before_statement_effect(
&mut sets, Location { block: bb, statement_index: j });
results.0.operator.statement_effect(
&mut sets, Location { block: bb, statement_index: j });
sets.on_entry.union(sets.gen_set);
sets.on_entry.subtract(sets.kill_set);
}

results.0.operator.before_terminator_effect(
&mut sets,
Location { block: bb, statement_index: statements.len() });

tcx.sess.span_err(span, &format!("rustc_peek: MIR did not match \
anticipated pattern; note that \
rustc_peek expects input of \
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ fn should_also_eventually_be_ok_with_nll() {
let _z = &x;
*y += 1;
//[lxl]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable
//[nll]~^^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable
}

fn main() { }
30 changes: 30 additions & 0 deletions src/test/ui/nll/borrow-use-issue-46875.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Copyright 2017 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)]

// run-pass

fn vec() {
let mut _x = vec!['c'];
let _y = &_x;
_x = Vec::new();
}

fn int() {
let mut _x = 5;
let _y = &_x;
_x = 7;
}

fn main() {
vec();
int();
}
Loading