Skip to content

Commit a676a36

Browse files
CentrilCentril
authored
Rollup merge of rust-lang#62736 - lqd:polonius_tests3, r=matthewjasper
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
2 parents 40be400 + e16bede commit a676a36

File tree

42 files changed

+892
-49
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

42 files changed

+892
-49
lines changed

‎src/librustc_mir/borrow_check/nll/constraint_generation.rs

+120-17
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,15 @@ use crate::borrow_check::location::LocationTable;
33
use crate::borrow_check::nll::ToRegionVid;
44
use crate::borrow_check::nll::facts::AllFacts;
55
use crate::borrow_check::nll::region_infer::values::LivenessValues;
6+
use crate::borrow_check::places_conflict;
67
use rustc::infer::InferCtxt;
78
use rustc::mir::visit::TyContext;
89
use rustc::mir::visit::Visitor;
9-
use rustc::mir::{BasicBlock, BasicBlockData, Location, Body, Place, PlaceBase, Rvalue};
10-
use rustc::mir::{SourceInfo, Statement, Terminator};
11-
use rustc::mir::UserTypeProjection;
10+
use rustc::mir::{
11+
BasicBlock, BasicBlockData, Body, Local, Location, Place, PlaceBase, Projection,
12+
ProjectionElem, Rvalue, SourceInfo, Statement, StatementKind, Terminator, TerminatorKind,
13+
UserTypeProjection,
14+
};
1215
use rustc::ty::fold::TypeFoldable;
1316
use rustc::ty::{self, ClosureSubsts, GeneratorSubsts, RegionVid, Ty};
1417
use rustc::ty::subst::SubstsRef;
@@ -27,6 +30,7 @@ pub(super) fn generate_constraints<'cx, 'tcx>(
2730
liveness_constraints,
2831
location_table,
2932
all_facts,
33+
body,
3034
};
3135

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

4651
impl<'cg, 'cx, 'tcx> Visitor<'tcx> for ConstraintGeneration<'cg, 'cx, 'tcx> {
@@ -114,6 +119,17 @@ impl<'cg, 'cx, 'tcx> Visitor<'tcx> for ConstraintGeneration<'cg, 'cx, 'tcx> {
114119
self.location_table
115120
.start_index(location.successor_within_block()),
116121
));
122+
123+
// If there are borrows on this now dead local, we need to record them as `killed`.
124+
if let StatementKind::StorageDead(ref local) = statement.kind {
125+
record_killed_borrows_for_local(
126+
all_facts,
127+
self.borrow_set,
128+
self.location_table,
129+
local,
130+
location,
131+
);
132+
}
117133
}
118134

119135
self.super_statement(statement, location);
@@ -127,20 +143,7 @@ impl<'cg, 'cx, 'tcx> Visitor<'tcx> for ConstraintGeneration<'cg, 'cx, 'tcx> {
127143
) {
128144
// When we see `X = ...`, then kill borrows of
129145
// `(*X).foo` and so forth.
130-
if let Some(all_facts) = self.all_facts {
131-
if let Place {
132-
base: PlaceBase::Local(temp),
133-
projection: None,
134-
} = place {
135-
if let Some(borrow_indices) = self.borrow_set.local_map.get(temp) {
136-
all_facts.killed.reserve(borrow_indices.len());
137-
for &borrow_index in borrow_indices {
138-
let location_index = self.location_table.mid_index(location);
139-
all_facts.killed.push((borrow_index, location_index));
140-
}
141-
}
142-
}
143-
}
146+
self.record_killed_borrows_for_place(place, location);
144147

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

173+
// A `Call` terminator's return value can be a local which has borrows,
174+
// so we need to record those as `killed` as well.
175+
if let TerminatorKind::Call { ref destination, .. } = terminator.kind {
176+
if let Some((place, _)) = destination {
177+
self.record_killed_borrows_for_place(place, location);
178+
}
179+
}
180+
170181
self.super_terminator(terminator, location);
171182
}
172183

@@ -201,4 +212,96 @@ impl<'cx, 'cg, 'tcx> ConstraintGeneration<'cx, 'cg, 'tcx> {
201212
self.liveness_constraints.add_element(vid, location);
202213
});
203214
}
215+
216+
/// When recording facts for Polonius, records the borrows on the specified place
217+
/// as `killed`. For example, when assigning to a local, or on a call's return destination.
218+
fn record_killed_borrows_for_place(&mut self, place: &Place<'tcx>, location: Location) {
219+
if let Some(all_facts) = self.all_facts {
220+
// Depending on the `Place` we're killing:
221+
// - if it's a local, or a single deref of a local,
222+
// we kill all the borrows on the local.
223+
// - if it's a deeper projection, we have to filter which
224+
// of the borrows are killed: the ones whose `borrowed_place`
225+
// conflicts with the `place`.
226+
match place {
227+
Place {
228+
base: PlaceBase::Local(local),
229+
projection: None,
230+
} |
231+
Place {
232+
base: PlaceBase::Local(local),
233+
projection: Some(box Projection {
234+
base: None,
235+
elem: ProjectionElem::Deref,
236+
}),
237+
} => {
238+
debug!(
239+
"Recording `killed` facts for borrows of local={:?} at location={:?}",
240+
local, location
241+
);
242+
243+
record_killed_borrows_for_local(
244+
all_facts,
245+
self.borrow_set,
246+
self.location_table,
247+
local,
248+
location,
249+
);
250+
}
251+
252+
Place {
253+
base: PlaceBase::Static(_),
254+
..
255+
} => {
256+
// Ignore kills of static or static mut variables.
257+
}
258+
259+
Place {
260+
base: PlaceBase::Local(local),
261+
projection: Some(_),
262+
} => {
263+
// Kill conflicting borrows of the innermost local.
264+
debug!(
265+
"Recording `killed` facts for borrows of \
266+
innermost projected local={:?} at location={:?}",
267+
local, location
268+
);
269+
270+
if let Some(borrow_indices) = self.borrow_set.local_map.get(local) {
271+
for &borrow_index in borrow_indices {
272+
let places_conflict = places_conflict::places_conflict(
273+
self.infcx.tcx,
274+
self.body,
275+
&self.borrow_set.borrows[borrow_index].borrowed_place,
276+
place,
277+
places_conflict::PlaceConflictBias::NoOverlap,
278+
);
279+
280+
if places_conflict {
281+
let location_index = self.location_table.mid_index(location);
282+
all_facts.killed.push((borrow_index, location_index));
283+
}
284+
}
285+
}
286+
}
287+
}
288+
}
289+
}
290+
}
291+
292+
/// When recording facts for Polonius, records the borrows on the specified local as `killed`.
293+
fn record_killed_borrows_for_local(
294+
all_facts: &mut AllFacts,
295+
borrow_set: &BorrowSet<'_>,
296+
location_table: &LocationTable,
297+
local: &Local,
298+
location: Location,
299+
) {
300+
if let Some(borrow_indices) = borrow_set.local_map.get(local) {
301+
all_facts.killed.reserve(borrow_indices.len());
302+
for &borrow_index in borrow_indices {
303+
let location_index = location_table.mid_index(location);
304+
all_facts.killed.push((borrow_index, location_index));
305+
}
306+
}
204307
}

‎src/test/ui/borrowck/borrowck-escaping-closure-error-2.polonius.stderr

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
error[E0597]: `books` does not live long enough
2+
--> $DIR/borrowck-escaping-closure-error-2.rs:11:17
3+
|
4+
LL | Box::new(|| books.push(4))
5+
| ------------^^^^^---------
6+
| | | |
7+
| | | borrowed value does not live long enough
8+
| | value captured here
9+
| borrow later used here
10+
LL |
11+
LL | }
12+
| - `books` dropped here while still borrowed
13+
14+
error: aborting due to previous error
15+
16+
For more information about this error, try `rustc --explain E0597`.

‎src/test/ui/borrowck/borrowck-migrate-to-nll.edition.stderr

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
warning[E0507]: cannot move out of `foo` in pattern guard
2-
--> $DIR/borrowck-migrate-to-nll.rs:25:18
2+
--> $DIR/borrowck-migrate-to-nll.rs:26:18
33
|
44
LL | (|| { let bar = foo; bar.take() })();
55
| ^^ ---

‎src/test/ui/borrowck/borrowck-migrate-to-nll.rs

+1
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
// just ignore it instead:
1111

1212
// ignore-compare-mode-nll
13+
// ignore-compare-mode-polonius
1314

1415
// revisions: zflag edition
1516
//[zflag]compile-flags: -Z borrowck=migrate

‎src/test/ui/borrowck/borrowck-migrate-to-nll.zflag.stderr

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
warning[E0507]: cannot move out of `foo` in pattern guard
2-
--> $DIR/borrowck-migrate-to-nll.rs:25:18
2+
--> $DIR/borrowck-migrate-to-nll.rs:26:18
33
|
44
LL | (|| { let bar = foo; bar.take() })();
55
| ^^ ---

‎src/test/ui/borrowck/issue-45983.migrate.stderr

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: borrowed data cannot be stored outside of its closure
2-
--> $DIR/issue-45983.rs:19:27
2+
--> $DIR/issue-45983.rs:20:27
33
|
44
LL | let x = None;
55
| - borrowed data cannot be stored into here...

‎src/test/ui/borrowck/issue-45983.nll.stderr

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error[E0521]: borrowed data escapes outside of closure
2-
--> $DIR/issue-45983.rs:19:18
2+
--> $DIR/issue-45983.rs:20:18
33
|
44
LL | let x = None;
55
| - `x` is declared here, outside of the closure body
@@ -9,7 +9,7 @@ LL | give_any(|y| x = Some(y));
99
| `y` is a reference that is only valid in the closure body
1010

1111
error[E0594]: cannot assign to `x`, as it is not declared as mutable
12-
--> $DIR/issue-45983.rs:19:18
12+
--> $DIR/issue-45983.rs:20:18
1313
|
1414
LL | let x = None;
1515
| - help: consider changing this to be mutable: `mut x`

‎src/test/ui/borrowck/issue-45983.rs

+1
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
// revisions, don't worry about the --compare-mode=nll on this test.
88

99
// ignore-compare-mode-nll
10+
// ignore-compare-mode-polonius
1011

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

‎src/test/ui/borrowck/promote-ref-mut-in-let-issue-46557.polonius.stderr

+59
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
error[E0716]: temporary value dropped while borrowed
2+
--> $DIR/promote-ref-mut-in-let-issue-46557.rs:5:21
3+
|
4+
LL | let ref mut x = 1234543;
5+
| ^^^^^^^ creates a temporary which is freed while still in use
6+
LL | x
7+
| - borrow later used here
8+
LL | }
9+
| - temporary value is freed at the end of this statement
10+
|
11+
= note: consider using a `let` binding to create a longer lived value
12+
13+
error[E0716]: temporary value dropped while borrowed
14+
--> $DIR/promote-ref-mut-in-let-issue-46557.rs:10:25
15+
|
16+
LL | let (ref mut x, ) = (1234543, );
17+
| ^^^^^^^^^^^ creates a temporary which is freed while still in use
18+
LL | x
19+
| - borrow later used here
20+
LL | }
21+
| - temporary value is freed at the end of this statement
22+
|
23+
= note: consider using a `let` binding to create a longer lived value
24+
25+
error[E0515]: cannot return value referencing temporary value
26+
--> $DIR/promote-ref-mut-in-let-issue-46557.rs:15:5
27+
|
28+
LL | match 1234543 {
29+
| ^ ------- temporary value created here
30+
| _____|
31+
| |
32+
LL | | ref mut x => x
33+
LL | | }
34+
| |_____^ returns a value referencing data owned by the current function
35+
36+
error[E0515]: cannot return value referencing temporary value
37+
--> $DIR/promote-ref-mut-in-let-issue-46557.rs:21:5
38+
|
39+
LL | match (123443,) {
40+
| ^ --------- temporary value created here
41+
| _____|
42+
| |
43+
LL | | (ref mut x,) => x,
44+
LL | | }
45+
| |_____^ returns a value referencing data owned by the current function
46+
47+
error[E0515]: cannot return reference to temporary value
48+
--> $DIR/promote-ref-mut-in-let-issue-46557.rs:27:5
49+
|
50+
LL | &mut 1234543
51+
| ^^^^^-------
52+
| | |
53+
| | temporary value created here
54+
| returns a reference to data owned by the current function
55+
56+
error: aborting due to 5 previous errors
57+
58+
Some errors have detailed explanations: E0515, E0716.
59+
For more information about an error, try `rustc --explain E0515`.

‎src/test/ui/borrowck/two-phase-reservation-sharing-interference-2.migrate2015.stderr

+3-3
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
2-
--> $DIR/two-phase-reservation-sharing-interference-2.rs:18:5
2+
--> $DIR/two-phase-reservation-sharing-interference-2.rs:19:5
33
|
44
LL | let shared = &v;
55
| -- immutable borrow occurs here
@@ -11,7 +11,7 @@ LL | v.extend(shared);
1111
| mutable borrow occurs here
1212

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

2323
warning: cannot borrow `v` as mutable because it is also borrowed as immutable
24-
--> $DIR/two-phase-reservation-sharing-interference-2.rs:39:5
24+
--> $DIR/two-phase-reservation-sharing-interference-2.rs:40:5
2525
|
2626
LL | let shared = &v;
2727
| -- immutable borrow occurs here

‎src/test/ui/borrowck/two-phase-reservation-sharing-interference-2.migrate2018.stderr

+3-3
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
2-
--> $DIR/two-phase-reservation-sharing-interference-2.rs:18:5
2+
--> $DIR/two-phase-reservation-sharing-interference-2.rs:19:5
33
|
44
LL | let shared = &v;
55
| -- immutable borrow occurs here
@@ -11,7 +11,7 @@ LL | v.extend(shared);
1111
| mutable borrow occurs here
1212

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

2323
warning: cannot borrow `v` as mutable because it is also borrowed as immutable
24-
--> $DIR/two-phase-reservation-sharing-interference-2.rs:39:5
24+
--> $DIR/two-phase-reservation-sharing-interference-2.rs:40:5
2525
|
2626
LL | let shared = &v;
2727
| -- immutable borrow occurs here

‎src/test/ui/borrowck/two-phase-reservation-sharing-interference-2.nll2015.stderr

+3-3
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable
2-
--> $DIR/two-phase-reservation-sharing-interference-2.rs:18:5
2+
--> $DIR/two-phase-reservation-sharing-interference-2.rs:19:5
33
|
44
LL | let shared = &v;
55
| -- immutable borrow occurs here
@@ -10,7 +10,7 @@ LL | v.extend(shared);
1010
| mutable borrow occurs here
1111

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

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

0 commit comments

Comments
 (0)