Skip to content

Commit

Permalink
Rollup merge of rust-lang#62736 - lqd:polonius_tests3, r=matthewjasper
Browse files Browse the repository at this point in the history
Polonius: fix some cases of `killed` fact generation, and most of the `ui` test suite

Since basic Polonius functionality was re-enabled by @matthewjasper in rust-lang#54468, some tests were still failing in the polonius compare-mode.

This PR fixes all but one test in the `ui` suite by:
- fixing some bugs in the fact generation code, related to the `killed` relation: Polonius would incorrectly reject some NLL-accepted code, because of these missing `killed` facts.
- ignoring some tests in the polonius compare-mode: a lot of those manually test the NLL or migrate mode, and the failures were mostly artifacts of the test revisions, e.g. that `-Z polonius` requires full NLLs. Some others were also both failing with NLL and succeeding with Polonius, which we can't encode in tests at the moment.
- blessing the output of some tests: whenever Polonius and NLL have basically the same errors, except for diagnostics differences, the Polonius output is blessed. Whenever we've advanced into a less experimental phase, we'll want to revisit these cases (much like we did on the NLL test suite last year) to specifically work on diagnostics.

Fact generation changes:
- we now kill loans on the destination place of `Call` terminators
- we now kill loans on the locals destroyed by `StorageDead`
- we now also handle assignments to projections: killing the loans on a either a deref-ed local, or the ones whose `borrowed_place` conflicts with the current place.

One failing test remains: an overflow during fact generation, on a case of polymorphic recursion (and which I'll continue investigating later).

This adds some tests for the fact generation changes, with some simple Polonius cases similar to the existing smoke tests, but also for some cases encountered in the wild (in the `rand` crate for example).

A more detailed write-up is available [here](https://hackmd.io/CjYB0fs4Q9CweyeTdKWyEg?view) with an explanation for each test failure, the steps taken to resolve it (as a commit in the current PR), NLL and Polonius outputs (and diff), etc.

Since they've worked on this before, and we've discussed some of these failures together:

r? @matthewjasper
  • Loading branch information
Centril authored Jul 24, 2019
2 parents 40be400 + e16bede commit a676a36
Show file tree
Hide file tree
Showing 42 changed files with 892 additions and 49 deletions.
137 changes: 120 additions & 17 deletions src/librustc_mir/borrow_check/nll/constraint_generation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,15 @@ use crate::borrow_check::location::LocationTable;
use crate::borrow_check::nll::ToRegionVid;
use crate::borrow_check::nll::facts::AllFacts;
use crate::borrow_check::nll::region_infer::values::LivenessValues;
use crate::borrow_check::places_conflict;
use rustc::infer::InferCtxt;
use rustc::mir::visit::TyContext;
use rustc::mir::visit::Visitor;
use rustc::mir::{BasicBlock, BasicBlockData, Location, Body, Place, PlaceBase, Rvalue};
use rustc::mir::{SourceInfo, Statement, Terminator};
use rustc::mir::UserTypeProjection;
use rustc::mir::{
BasicBlock, BasicBlockData, Body, Local, Location, Place, PlaceBase, Projection,
ProjectionElem, Rvalue, SourceInfo, Statement, StatementKind, Terminator, TerminatorKind,
UserTypeProjection,
};
use rustc::ty::fold::TypeFoldable;
use rustc::ty::{self, ClosureSubsts, GeneratorSubsts, RegionVid, Ty};
use rustc::ty::subst::SubstsRef;
Expand All @@ -27,6 +30,7 @@ pub(super) fn generate_constraints<'cx, 'tcx>(
liveness_constraints,
location_table,
all_facts,
body,
};

for (bb, data) in body.basic_blocks().iter_enumerated() {
Expand All @@ -41,6 +45,7 @@ struct ConstraintGeneration<'cg, 'cx, 'tcx> {
location_table: &'cg LocationTable,
liveness_constraints: &'cg mut LivenessValues<RegionVid>,
borrow_set: &'cg BorrowSet<'tcx>,
body: &'cg Body<'tcx>,
}

impl<'cg, 'cx, 'tcx> Visitor<'tcx> for ConstraintGeneration<'cg, 'cx, 'tcx> {
Expand Down Expand Up @@ -114,6 +119,17 @@ impl<'cg, 'cx, 'tcx> Visitor<'tcx> for ConstraintGeneration<'cg, 'cx, 'tcx> {
self.location_table
.start_index(location.successor_within_block()),
));

// If there are borrows on this now dead local, we need to record them as `killed`.
if let StatementKind::StorageDead(ref local) = statement.kind {
record_killed_borrows_for_local(
all_facts,
self.borrow_set,
self.location_table,
local,
location,
);
}
}

self.super_statement(statement, location);
Expand All @@ -127,20 +143,7 @@ impl<'cg, 'cx, 'tcx> Visitor<'tcx> for ConstraintGeneration<'cg, 'cx, 'tcx> {
) {
// When we see `X = ...`, then kill borrows of
// `(*X).foo` and so forth.
if let Some(all_facts) = self.all_facts {
if let Place {
base: PlaceBase::Local(temp),
projection: None,
} = place {
if let Some(borrow_indices) = self.borrow_set.local_map.get(temp) {
all_facts.killed.reserve(borrow_indices.len());
for &borrow_index in borrow_indices {
let location_index = self.location_table.mid_index(location);
all_facts.killed.push((borrow_index, location_index));
}
}
}
}
self.record_killed_borrows_for_place(place, location);

self.super_assign(place, rvalue, location);
}
Expand All @@ -167,6 +170,14 @@ impl<'cg, 'cx, 'tcx> Visitor<'tcx> for ConstraintGeneration<'cg, 'cx, 'tcx> {
}
}

// A `Call` terminator's return value can be a local which has borrows,
// so we need to record those as `killed` as well.
if let TerminatorKind::Call { ref destination, .. } = terminator.kind {
if let Some((place, _)) = destination {
self.record_killed_borrows_for_place(place, location);
}
}

self.super_terminator(terminator, location);
}

Expand Down Expand Up @@ -201,4 +212,96 @@ impl<'cx, 'cg, 'tcx> ConstraintGeneration<'cx, 'cg, 'tcx> {
self.liveness_constraints.add_element(vid, location);
});
}

/// When recording facts for Polonius, records the borrows on the specified place
/// as `killed`. For example, when assigning to a local, or on a call's return destination.
fn record_killed_borrows_for_place(&mut self, place: &Place<'tcx>, location: Location) {
if let Some(all_facts) = self.all_facts {
// Depending on the `Place` we're killing:
// - if it's a local, or a single deref of a local,
// we kill all the borrows on the local.
// - if it's a deeper projection, we have to filter which
// of the borrows are killed: the ones whose `borrowed_place`
// conflicts with the `place`.
match place {
Place {
base: PlaceBase::Local(local),
projection: None,
} |
Place {
base: PlaceBase::Local(local),
projection: Some(box Projection {
base: None,
elem: ProjectionElem::Deref,
}),
} => {
debug!(
"Recording `killed` facts for borrows of local={:?} at location={:?}",
local, location
);

record_killed_borrows_for_local(
all_facts,
self.borrow_set,
self.location_table,
local,
location,
);
}

Place {
base: PlaceBase::Static(_),
..
} => {
// Ignore kills of static or static mut variables.
}

Place {
base: PlaceBase::Local(local),
projection: Some(_),
} => {
// Kill conflicting borrows of the innermost local.
debug!(
"Recording `killed` facts for borrows of \
innermost projected local={:?} at location={:?}",
local, location
);

if let Some(borrow_indices) = self.borrow_set.local_map.get(local) {
for &borrow_index in borrow_indices {
let places_conflict = places_conflict::places_conflict(
self.infcx.tcx,
self.body,
&self.borrow_set.borrows[borrow_index].borrowed_place,
place,
places_conflict::PlaceConflictBias::NoOverlap,
);

if places_conflict {
let location_index = self.location_table.mid_index(location);
all_facts.killed.push((borrow_index, location_index));
}
}
}
}
}
}
}
}

/// When recording facts for Polonius, records the borrows on the specified local as `killed`.
fn record_killed_borrows_for_local(
all_facts: &mut AllFacts,
borrow_set: &BorrowSet<'_>,
location_table: &LocationTable,
local: &Local,
location: Location,
) {
if let Some(borrow_indices) = borrow_set.local_map.get(local) {
all_facts.killed.reserve(borrow_indices.len());
for &borrow_index in borrow_indices {
let location_index = location_table.mid_index(location);
all_facts.killed.push((borrow_index, location_index));
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
error[E0597]: `books` does not live long enough
--> $DIR/borrowck-escaping-closure-error-2.rs:11:17
|
LL | Box::new(|| books.push(4))
| ------------^^^^^---------
| | | |
| | | borrowed value does not live long enough
| | value captured here
| borrow later used here
LL |
LL | }
| - `books` dropped here while still borrowed

error: aborting due to previous error

For more information about this error, try `rustc --explain E0597`.
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
warning[E0507]: cannot move out of `foo` in pattern guard
--> $DIR/borrowck-migrate-to-nll.rs:25:18
--> $DIR/borrowck-migrate-to-nll.rs:26:18
|
LL | (|| { let bar = foo; bar.take() })();
| ^^ ---
Expand Down
1 change: 1 addition & 0 deletions src/test/ui/borrowck/borrowck-migrate-to-nll.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
// just ignore it instead:

// ignore-compare-mode-nll
// ignore-compare-mode-polonius

// revisions: zflag edition
//[zflag]compile-flags: -Z borrowck=migrate
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/borrowck/borrowck-migrate-to-nll.zflag.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
warning[E0507]: cannot move out of `foo` in pattern guard
--> $DIR/borrowck-migrate-to-nll.rs:25:18
--> $DIR/borrowck-migrate-to-nll.rs:26:18
|
LL | (|| { let bar = foo; bar.take() })();
| ^^ ---
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/borrowck/issue-45983.migrate.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: borrowed data cannot be stored outside of its closure
--> $DIR/issue-45983.rs:19:27
--> $DIR/issue-45983.rs:20:27
|
LL | let x = None;
| - borrowed data cannot be stored into here...
Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/borrowck/issue-45983.nll.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0521]: borrowed data escapes outside of closure
--> $DIR/issue-45983.rs:19:18
--> $DIR/issue-45983.rs:20:18
|
LL | let x = None;
| - `x` is declared here, outside of the closure body
Expand All @@ -9,7 +9,7 @@ LL | give_any(|y| x = Some(y));
| `y` is a reference that is only valid in the closure body

error[E0594]: cannot assign to `x`, as it is not declared as mutable
--> $DIR/issue-45983.rs:19:18
--> $DIR/issue-45983.rs:20:18
|
LL | let x = None;
| - help: consider changing this to be mutable: `mut x`
Expand Down
1 change: 1 addition & 0 deletions src/test/ui/borrowck/issue-45983.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
// revisions, don't worry about the --compare-mode=nll on this test.

// ignore-compare-mode-nll
// ignore-compare-mode-polonius

//[nll]compile-flags: -Z borrowck=mir

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-ref-mut-in-let-issue-46557.rs:5:21
|
LL | let ref mut x = 1234543;
| ^^^^^^^ creates a temporary which is freed while still in use
LL | x
| - borrow later used here
LL | }
| - temporary value is freed at the end of this statement
|
= note: consider using a `let` binding to create a longer lived value

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-ref-mut-in-let-issue-46557.rs:10:25
|
LL | let (ref mut x, ) = (1234543, );
| ^^^^^^^^^^^ creates a temporary which is freed while still in use
LL | x
| - borrow later used here
LL | }
| - temporary value is freed at the end of this statement
|
= note: consider using a `let` binding to create a longer lived value

error[E0515]: cannot return value referencing temporary value
--> $DIR/promote-ref-mut-in-let-issue-46557.rs:15:5
|
LL | match 1234543 {
| ^ ------- temporary value created here
| _____|
| |
LL | | ref mut x => x
LL | | }
| |_____^ returns a value referencing data owned by the current function

error[E0515]: cannot return value referencing temporary value
--> $DIR/promote-ref-mut-in-let-issue-46557.rs:21:5
|
LL | match (123443,) {
| ^ --------- temporary value created here
| _____|
| |
LL | | (ref mut x,) => x,
LL | | }
| |_____^ returns a value referencing data owned by the current function

error[E0515]: cannot return reference to temporary value
--> $DIR/promote-ref-mut-in-let-issue-46557.rs:27:5
|
LL | &mut 1234543
| ^^^^^-------
| | |
| | temporary value created here
| returns a reference to data owned by the current function

error: aborting due to 5 previous errors

Some errors have detailed explanations: E0515, E0716.
For more information about an error, try `rustc --explain E0515`.
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
--> $DIR/two-phase-reservation-sharing-interference-2.rs:18:5
--> $DIR/two-phase-reservation-sharing-interference-2.rs:19:5
|
LL | let shared = &v;
| -- immutable borrow occurs here
Expand All @@ -11,7 +11,7 @@ LL | v.extend(shared);
| mutable borrow occurs here

error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
--> $DIR/two-phase-reservation-sharing-interference-2.rs:28:5
--> $DIR/two-phase-reservation-sharing-interference-2.rs:29:5
|
LL | v.extend(&v);
| ^^------^--^
Expand All @@ -21,7 +21,7 @@ LL | v.extend(&v);
| mutable borrow occurs here

warning: cannot borrow `v` as mutable because it is also borrowed as immutable
--> $DIR/two-phase-reservation-sharing-interference-2.rs:39:5
--> $DIR/two-phase-reservation-sharing-interference-2.rs:40:5
|
LL | let shared = &v;
| -- immutable borrow occurs here
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
--> $DIR/two-phase-reservation-sharing-interference-2.rs:18:5
--> $DIR/two-phase-reservation-sharing-interference-2.rs:19:5
|
LL | let shared = &v;
| -- immutable borrow occurs here
Expand All @@ -11,7 +11,7 @@ LL | v.extend(shared);
| mutable borrow occurs here

error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
--> $DIR/two-phase-reservation-sharing-interference-2.rs:28:5
--> $DIR/two-phase-reservation-sharing-interference-2.rs:29:5
|
LL | v.extend(&v);
| ^^------^--^
Expand All @@ -21,7 +21,7 @@ LL | v.extend(&v);
| mutable borrow occurs here

warning: cannot borrow `v` as mutable because it is also borrowed as immutable
--> $DIR/two-phase-reservation-sharing-interference-2.rs:39:5
--> $DIR/two-phase-reservation-sharing-interference-2.rs:40:5
|
LL | let shared = &v;
| -- immutable borrow occurs here
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
--> $DIR/two-phase-reservation-sharing-interference-2.rs:18:5
--> $DIR/two-phase-reservation-sharing-interference-2.rs:19:5
|
LL | let shared = &v;
| -- immutable borrow occurs here
Expand All @@ -10,7 +10,7 @@ LL | v.extend(shared);
| mutable borrow occurs here

error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
--> $DIR/two-phase-reservation-sharing-interference-2.rs:28:5
--> $DIR/two-phase-reservation-sharing-interference-2.rs:29:5
|
LL | v.extend(&v);
| ^^------^--^
Expand All @@ -20,7 +20,7 @@ LL | v.extend(&v);
| mutable borrow occurs here

error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
--> $DIR/two-phase-reservation-sharing-interference-2.rs:39:5
--> $DIR/two-phase-reservation-sharing-interference-2.rs:40:5
|
LL | let shared = &v;
| -- immutable borrow occurs here
Expand Down
Loading

0 comments on commit a676a36

Please sign in to comment.