Skip to content

Commit b342f00

Browse files
davidtwcodavidtwco
committed
Only annotate if borrow is returned.
Error now correctly checks whether the borrow that does not live long enough is being returned before annotating the error with the arguments and return type from the signature - as this would not be relevant if the borrow was not being returned.
1 parent ef10e94 commit b342f00

9 files changed

+276
-67
lines changed

‎src/librustc/mir/mod.rs

+12
Original file line numberDiff line numberDiff line change
@@ -1883,6 +1883,18 @@ impl<'tcx> Place<'tcx> {
18831883
pub fn elem(self, elem: PlaceElem<'tcx>) -> Place<'tcx> {
18841884
Place::Projection(Box::new(PlaceProjection { base: self, elem }))
18851885
}
1886+
1887+
/// Find the innermost `Local` from this `Place`.
1888+
pub fn local(&self) -> Option<Local> {
1889+
match self {
1890+
Place::Local(local) |
1891+
Place::Projection(box Projection {
1892+
base: Place::Local(local),
1893+
elem: ProjectionElem::Deref,
1894+
}) => Some(*local),
1895+
_ => None,
1896+
}
1897+
}
18861898
}
18871899

18881900
impl<'tcx> Debug for Place<'tcx> {

‎src/librustc_mir/borrow_check/error_reporting.rs

+198-40
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,9 @@ use borrow_check::prefixes::IsPrefixOf;
1313
use borrow_check::nll::explain_borrow::BorrowExplanation;
1414
use rustc::middle::region::ScopeTree;
1515
use rustc::mir::{
16-
AggregateKind, BindingForm, BorrowKind, ClearCrossCrate, FakeReadCause, Field, Local,
16+
self, AggregateKind, BindingForm, BorrowKind, ClearCrossCrate, FakeReadCause, Field, Local,
1717
LocalDecl, LocalKind, Location, Operand, Place, ProjectionElem, Rvalue, Statement,
18-
StatementKind, VarBindingForm,
18+
StatementKind, TerminatorKind, VarBindingForm,
1919
};
2020
use rustc::hir;
2121
use rustc::hir::def_id::DefId;
@@ -518,14 +518,14 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
518518

519519
err.span_label(
520520
borrow_span,
521-
format!("`{}` would have to be valid for `{}`", name, region_name)
521+
format!("`{}` would have to be valid for `{}`...", name, region_name)
522522
);
523523

524524
if let Some(fn_node_id) = self.infcx.tcx.hir.as_local_node_id(self.mir_def_id) {
525525
err.span_label(
526526
drop_span,
527527
format!(
528-
"but `{}` will be dropped here, when the function `{}` returns",
528+
"...but `{}` will be dropped here, when the function `{}` returns",
529529
name, self.infcx.tcx.hir.name(fn_node_id),
530530
)
531531
);
@@ -1173,54 +1173,211 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
11731173
&self,
11741174
borrow: &BorrowData<'tcx>,
11751175
) -> Option<AnnotatedBorrowFnSignature> {
1176-
// There are two cases that need handled: when a closure is involved and
1177-
// when a closure is not involved.
1178-
let location = borrow.reserve_location;
1179-
let is_closure = self.infcx.tcx.is_closure(self.mir_def_id);
1176+
// Define a fallback for when we can't match a closure.
1177+
let fallback = || {
1178+
let is_closure = self.infcx.tcx.is_closure(self.mir_def_id);
1179+
if is_closure {
1180+
None
1181+
} else {
1182+
let ty = self.infcx.tcx.type_of(self.mir_def_id);
1183+
match ty.sty {
1184+
ty::TyKind::FnDef(_, _) | ty::TyKind::FnPtr(_) =>
1185+
self.annotate_fn_sig(
1186+
self.mir_def_id,
1187+
self.infcx.tcx.fn_sig(self.mir_def_id)
1188+
),
1189+
_ => None,
1190+
}
1191+
}
1192+
};
11801193

1181-
match self.mir[location.block].statements.get(location.statement_index) {
1182-
// When a closure is involved, we expect the reserve location to be an assignment
1183-
// to a temporary local, which will be followed by a closure.
1194+
// In order to determine whether we need to annotate, we need to check whether the reserve
1195+
// place was an assignment into a temporary.
1196+
//
1197+
// If it was, we check whether or not that temporary is eventually assigned into the return
1198+
// place. If it was, we can add annotations about the function's return type and arguments
1199+
// and it'll make sense.
1200+
let location = borrow.reserve_location;
1201+
debug!("annotate_argument_and_return_for_borrow: location={:?}", location);
1202+
match &self.mir[location.block].statements.get(location.statement_index) {
11841203
Some(&Statement {
1185-
kind: StatementKind::Assign(Place::Local(local), _),
1204+
kind: StatementKind::Assign(ref reservation, _),
11861205
..
1187-
}) if self.mir.local_kind(local) == LocalKind::Temp => {
1188-
// Look for the statements within this block after assigning to a local to see
1189-
// if we have a closure. If we do, then annotate it.
1206+
}) => {
1207+
debug!("annotate_argument_and_return_for_borrow: reservation={:?}", reservation);
1208+
// Check that the initial assignment of the reserve location is into a temporary.
1209+
let mut target = *match reservation {
1210+
Place::Local(local) if self.mir.local_kind(*local) == LocalKind::Temp => local,
1211+
_ => return None,
1212+
};
1213+
1214+
// Next, look through the rest of the block, checking if we are assigning the
1215+
// `target` (that is, the place that contains our borrow) to anything.
1216+
let mut annotated_closure = None;
11901217
for stmt in &self.mir[location.block].statements[location.statement_index + 1..] {
1218+
debug!(
1219+
"annotate_argument_and_return_for_borrow: target={:?} stmt={:?}",
1220+
target, stmt
1221+
);
11911222
if let StatementKind::Assign(
1192-
_,
1193-
Rvalue::Aggregate(
1194-
box AggregateKind::Closure(def_id, substs),
1195-
_
1196-
)
1197-
) = stmt.kind {
1198-
return self.annotate_fn_sig(
1199-
def_id,
1200-
self.infcx.closure_sig(def_id, substs)
1223+
Place::Local(assigned_to),
1224+
rvalue,
1225+
) = &stmt.kind {
1226+
debug!("annotate_argument_and_return_for_borrow: assigned_to={:?} \
1227+
rvalue={:?}", assigned_to, rvalue);
1228+
// Check if our `target` was captured by a closure.
1229+
if let Rvalue::Aggregate(
1230+
box AggregateKind::Closure(def_id, substs),
1231+
operands,
1232+
) = rvalue {
1233+
for operand in operands {
1234+
let assigned_from = match operand {
1235+
Operand::Copy(assigned_from) |
1236+
Operand::Move(assigned_from) => assigned_from,
1237+
_ => continue,
1238+
};
1239+
debug!(
1240+
"annotate_argument_and_return_for_borrow: assigned_from={:?}",
1241+
assigned_from
1242+
);
1243+
1244+
// Find the local from the operand.
1245+
let assigned_from_local = match assigned_from.local() {
1246+
Some(local) => local,
1247+
None => continue,
1248+
};
1249+
1250+
if assigned_from_local != target {
1251+
continue;
1252+
}
1253+
1254+
// If a closure captured our `target` and then assigned
1255+
// into a place then we should annotate the closure in
1256+
// case it ends up being assigned into the return place.
1257+
annotated_closure = self.annotate_fn_sig(
1258+
*def_id,
1259+
self.infcx.closure_sig(*def_id, *substs)
1260+
);
1261+
debug!(
1262+
"annotate_argument_and_return_for_borrow: \
1263+
annotated_closure={:?} assigned_from_local={:?} \
1264+
assigned_to={:?}",
1265+
annotated_closure, assigned_from_local, assigned_to
1266+
);
1267+
1268+
if *assigned_to == mir::RETURN_PLACE {
1269+
// If it was assigned directly into the return place, then
1270+
// return now.
1271+
return annotated_closure;
1272+
} else {
1273+
// Otherwise, update the target.
1274+
target = *assigned_to;
1275+
}
1276+
}
1277+
1278+
// If none of our closure's operands matched, then skip to the next
1279+
// statement.
1280+
continue;
1281+
}
1282+
1283+
// Otherwise, look at other types of assignment.
1284+
let assigned_from = match rvalue {
1285+
Rvalue::Ref(_, _, assigned_from) => assigned_from,
1286+
Rvalue::Use(operand) => match operand {
1287+
Operand::Copy(assigned_from) |
1288+
Operand::Move(assigned_from) => assigned_from,
1289+
_ => continue,
1290+
},
1291+
_ => continue,
1292+
};
1293+
debug!(
1294+
"annotate_argument_and_return_for_borrow: \
1295+
assigned_from={:?}", assigned_from,
1296+
);
1297+
1298+
// Find the local from the rvalue.
1299+
let assigned_from_local = match assigned_from.local() {
1300+
Some(local) => local,
1301+
None => continue,
1302+
};
1303+
debug!(
1304+
"annotate_argument_and_return_for_borrow: \
1305+
assigned_from_local={:?}", assigned_from_local,
12011306
);
1307+
1308+
// Check if our local matches the target - if so, we've assigned our
1309+
// borrow to a new place.
1310+
if assigned_from_local != target {
1311+
continue;
1312+
}
1313+
1314+
// If we assigned our `target` into a new place, then we should
1315+
// check if it was the return place.
1316+
debug!(
1317+
"annotate_argument_and_return_for_borrow: \
1318+
assigned_from_local={:?} assigned_to={:?}",
1319+
assigned_from_local, assigned_to
1320+
);
1321+
if *assigned_to == mir::RETURN_PLACE {
1322+
// If it was then return the annotated closure if there was one,
1323+
// else, annotate this function.
1324+
return annotated_closure.or_else(fallback);
1325+
}
1326+
1327+
// If we didn't assign into the return place, then we just update
1328+
// the target.
1329+
target = *assigned_to;
1330+
}
1331+
}
1332+
1333+
// Check the terminator if we didn't find anything in the statements.
1334+
let terminator = &self.mir[location.block].terminator();
1335+
debug!(
1336+
"annotate_argument_and_return_for_borrow: target={:?} terminator={:?}",
1337+
target, terminator
1338+
);
1339+
if let TerminatorKind::Call {
1340+
destination: Some((Place::Local(assigned_to), _)),
1341+
args,
1342+
..
1343+
} = &terminator.kind {
1344+
debug!(
1345+
"annotate_argument_and_return_for_borrow: assigned_to={:?} args={:?}",
1346+
assigned_to, args
1347+
);
1348+
for operand in args {
1349+
let assigned_from = match operand {
1350+
Operand::Copy(assigned_from) |
1351+
Operand::Move(assigned_from) => assigned_from,
1352+
_ => continue,
1353+
};
1354+
debug!(
1355+
"annotate_argument_and_return_for_borrow: assigned_from={:?}",
1356+
assigned_from,
1357+
);
1358+
1359+
if let Some(assigned_from_local) = assigned_from.local() {
1360+
debug!(
1361+
"annotate_argument_and_return_for_borrow: assigned_from_local={:?}",
1362+
assigned_from_local,
1363+
);
1364+
1365+
if *assigned_to == mir::RETURN_PLACE &&
1366+
assigned_from_local == target
1367+
{
1368+
return annotated_closure.or_else(fallback);
1369+
}
1370+
}
12021371
}
12031372
}
12041373
}
12051374
_ => {}
12061375
}
12071376

1208-
// If this is not the case, then return if we're currently on a closure (as we
1209-
// don't have a substs to get the PolyFnSig) or attempt to get the arguments
1210-
// and return type of the function.
1211-
if is_closure {
1212-
None
1213-
} else {
1214-
let ty = self.infcx.tcx.type_of(self.mir_def_id);
1215-
match ty.sty {
1216-
ty::TyKind::FnDef(_, _) | ty::TyKind::FnPtr(_) =>
1217-
self.annotate_fn_sig(
1218-
self.mir_def_id,
1219-
self.infcx.tcx.fn_sig(self.mir_def_id)
1220-
),
1221-
_ => None,
1222-
}
1223-
}
1377+
// If we haven't found an assignment into the return place, then we need not add
1378+
// any annotations.
1379+
debug!("annotate_argument_and_return_for_borrow: none found");
1380+
None
12241381
}
12251382

12261383
/// Annotate the first argument and return type of a function signature if they are
@@ -1230,6 +1387,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
12301387
did: DefId,
12311388
sig: ty::PolyFnSig<'tcx>,
12321389
) -> Option<AnnotatedBorrowFnSignature> {
1390+
debug!("annotate_fn_sig: did={:?} sig={:?}", did, sig);
12331391
let is_closure = self.infcx.tcx.is_closure(did);
12341392
let fn_node_id = self.infcx.tcx.hir.as_local_node_id(did)?;
12351393
let fn_decl = self.infcx.tcx.hir.fn_decl(fn_node_id)?;

‎src/test/ui/issues/issue-30438-c.nll.stderr

+2-2
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,10 @@ LL | fn silly<'y, 'z>(_s: &'y Test<'z>) -> &'y <Test<'z> as Trait>::Out where 'z
77
| has lifetime `'y`
88
LL | let x = Test { s: "this cannot last" };
99
LL | &x
10-
| ^^ `x` would have to be valid for `'y`
10+
| ^^ `x` would have to be valid for `'y`...
1111
LL | //~^ ERROR: `x` does not live long enough
1212
LL | }
13-
| - but `x` will be dropped here, when the function `silly` returns
13+
| - ...but `x` will be dropped here, when the function `silly` returns
1414
|
1515
= help: use data from the highlighted arguments which match the `'y` lifetime of the return type
1616
= note: functions cannot return a borrow to data owned within the function's scope, functions can only return borrows to data passed as arguments

‎src/test/ui/nll/borrowed-universal-error-2.stderr

+2-2
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,10 @@ LL | fn foo<'a>(x: &'a (u32,)) -> &'a u32 {
77
| has lifetime `'a`
88
LL | let v = 22;
99
LL | &v
10-
| ^^ `v` would have to be valid for `'a`
10+
| ^^ `v` would have to be valid for `'a`...
1111
LL | //~^ ERROR `v` does not live long enough [E0597]
1212
LL | }
13-
| - but `v` will be dropped here, when the function `foo` returns
13+
| - ...but `v` will be dropped here, when the function `foo` returns
1414
|
1515
= help: use data from the highlighted arguments which match the `'a` lifetime of the return type
1616
= note: functions cannot return a borrow to data owned within the function's scope, functions can only return borrows to data passed as arguments

0 commit comments

Comments
 (0)