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.

Sign up for GitHub

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

MIR-borrowck: emit "foo does not live long enough" instead of borrow errors #45989

Merged
merged 5 commits into from
Nov 18, 2017
Merged

MIR-borrowck: emit "foo does not live long enough" instead of borrow errors #45989

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7db49b9
Deduplicated borrow checking errors.
davidtwco Nov 14, 2017
a98aff7
Updated test to reflect expected Mir output.
davidtwco Nov 17, 2017
ce8967a
Modified to output intended error.
davidtwco Nov 17, 2017
bf76ffe
Corrected error output with many labels.
davidtwco Nov 17, 2017
a1d55be
Fixed bug with the implementation that was suppressing expected error…
davidtwco Nov 17, 2017
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
119 changes: 99 additions & 20 deletions src/librustc_mir/borrow_check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use rustc::mir::{Mir, Mutability, Operand, Projection, ProjectionElem, Rvalue};
use rustc::mir::{Statement, StatementKind, Terminator, TerminatorKind};
use transform::nll;

use rustc_data_structures::fx::FxHashSet;
use rustc_data_structures::indexed_set::{self, IdxSetBuf};
use rustc_data_structures::indexed_vec::{Idx};

Expand Down Expand Up @@ -136,6 +137,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(infcx: &InferCtxt<'a, 'gcx, 'tcx>,
node_id: id,
move_data: &mdpe.move_data,
param_env: param_env,
storage_drop_or_dead_error_reported: FxHashSet(),
};

let mut state = InProgress::new(flow_borrows,
Expand All @@ -153,6 +155,10 @@ pub struct MirBorrowckCtxt<'cx, 'gcx: 'tcx, 'tcx: 'cx> {
node_id: ast::NodeId,
move_data: &'cx MoveData<'tcx>,
param_env: ParamEnv<'gcx>,
/// This field keeps track of when storage drop or dead 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.
storage_drop_or_dead_error_reported: FxHashSet<Local>,
}

// (forced to be `pub` due to its use as an associated type below.)
Expand Down Expand Up @@ -281,10 +287,15 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
}

StatementKind::StorageDead(local) => {
self.access_lvalue(ContextKind::StorageDead.new(location),
(&Lvalue::Local(local), span),
(Shallow(None), Write(WriteKind::StorageDead)),
flow_state);
if !self.storage_drop_or_dead_error_reported.contains(&local) {
let error_reported = self.access_lvalue(ContextKind::StorageDead.new(location),
(&Lvalue::Local(local), span),
(Shallow(None), Write(WriteKind::StorageDeadOrDrop)), flow_state);

if error_reported {
self.storage_drop_or_dead_error_reported.insert(local);
}
}
}
}
}
Expand Down Expand Up @@ -427,24 +438,30 @@ enum ReadKind {

#[derive(Copy, Clone, PartialEq, Eq, Debug)]
enum WriteKind {
StorageDead,
StorageDeadOrDrop,
MutableBorrow(BorrowKind),
Mutate,
Move,
}

impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
/// Checks an access to the given lvalue to see if it is allowed. Examines the set of borrows
/// that are in scope, as well as which paths have been initialized, to ensure that (a) the
/// lvalue is initialized and (b) it is not borrowed in some way that would prevent this
/// access.
///
/// Returns true if an error is reported, false otherwise.
fn access_lvalue(&mut self,
context: Context,
lvalue_span: (&Lvalue<'tcx>, Span),
kind: (ShallowOrDeep, ReadOrWrite),
flow_state: &InProgress<'cx, 'gcx, 'tcx>) {

flow_state: &InProgress<'cx, 'gcx, 'tcx>) -> bool {
let (sd, rw) = kind;

// Check permissions
self.check_access_permissions(lvalue_span, rw);

let mut error_reported = false;
self.each_borrow_involving_path(
context, (sd, lvalue_span.0), flow_state, |this, _index, borrow, common_prefix| {
match (rw, borrow.kind) {
Expand All @@ -454,13 +471,16 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
(Read(kind), BorrowKind::Unique) |
(Read(kind), BorrowKind::Mut) => {
match kind {
ReadKind::Copy =>
ReadKind::Copy => {
error_reported = true;
this.report_use_while_mutably_borrowed(
context, lvalue_span, borrow),
context, lvalue_span, borrow)
},
ReadKind::Borrow(bk) => {
let end_issued_loan_span =
flow_state.borrows.base_results.operator().opt_region_end_span(
&borrow.region);
error_reported = true;
this.report_conflicting_borrow(
context, common_prefix, lvalue_span, bk,
&borrow, end_issued_loan_span)
Expand All @@ -474,22 +494,35 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
let end_issued_loan_span =
flow_state.borrows.base_results.operator().opt_region_end_span(
&borrow.region);
error_reported = true;
this.report_conflicting_borrow(
context, common_prefix, lvalue_span, bk,
&borrow, end_issued_loan_span)
}
WriteKind::StorageDead |
WriteKind::Mutate =>
WriteKind::StorageDeadOrDrop => {
let end_span =
flow_state.borrows.base_results.operator().opt_region_end_span(
&borrow.region);
error_reported = true;
this.report_borrowed_value_does_not_live_long_enough(
context, lvalue_span, end_span)
},
WriteKind::Mutate => {
error_reported = true;
this.report_illegal_mutation_of_borrowed(
context, lvalue_span, borrow),
WriteKind::Move =>
context, lvalue_span, borrow)
},
WriteKind::Move => {
error_reported = true;
this.report_move_out_while_borrowed(
context, lvalue_span, &borrow),
context, lvalue_span, &borrow)
},
}
Control::Break
}
}
});
error_reported
}

fn mutate_lvalue(&mut self,
Expand Down Expand Up @@ -604,12 +637,37 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
let erased_ty = gcx.lift(&self.tcx.erase_regions(&ty)).unwrap();
let moves_by_default = erased_ty.moves_by_default(gcx, self.param_env, DUMMY_SP);

if moves_by_default {
// move of lvalue: check if this is move of already borrowed path
self.access_lvalue(context, lvalue_span, (Deep, Write(WriteKind::Move)), flow_state);
} else {
// copy of lvalue: check if this is "copy of frozen path" (FIXME: see check_loans.rs)
self.access_lvalue(context, lvalue_span, (Deep, Read(ReadKind::Copy)), flow_state);
// Check if error has already been reported to stop duplicate reporting.
let has_storage_drop_or_dead_error_reported = match *lvalue {
Lvalue::Local(local) => self.storage_drop_or_dead_error_reported.contains(&local),
_ => false,
};

// If the error has been reported already, then we don't need the access_lvalue call and we
// can set error_reported to false.
if !has_storage_drop_or_dead_error_reported {
Copy link
Contributor

Choose a reason for hiding this comment

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

So @arielb1 pointed out that there is a risk here. The calls to access_lvalue are not guaranteed to report an error -- and, in fact, DROP will report errors in a broader set of conditions than StorageDead (I think strictly broader). So it would be better to populate the map (and hence skip the latter check) only if an error was actually reported.

What @arielb1 suggested was issuing precisely the same error message in both cases, and letting rustc deduplicate (which it will do). That's an option, though I don't think it's the best path, since I think that the error messages should perhaps be different between the two cases.

Alternatively, we could make access_lvalue return a boolean or something, indicating whether an error was reported, and then do:

let has_storage_drop_or_dead_error_reported = self.storage_drop_or_dead_error_reported.contains_key(...);

let error_reported;
if has_storage_drop_or_dead_error_reported { error_reported = false; } else {
    if moves_by_default { error_reported = self.access_lvalue(..); } else { ... }
}
if error_reported { self.storage_drop_or_dead_error_reported.insert(local); }

It would also be good to comment about this. =)

It's hard to come up with a test case that would necesarily show off this problem though -- specifically, Drop will always come before StorageDead, and hence I imagine we'll visit it first, so the bug won't show itself. But I wouldn't want to rely on that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've modified what was there based on this, should be fine now.

let error_reported;

if moves_by_default {
let kind = match consume_via_drop {
ConsumeKind::Drop => WriteKind::StorageDeadOrDrop,
_ => WriteKind::Move,
};
// move of lvalue: check if this is move of already borrowed path
error_reported = self.access_lvalue(context, lvalue_span,
(Deep, Write(kind)), flow_state);
} else {
// copy of lvalue: check if this is "copy of frozen path"
// (FIXME: see check_loans.rs)
error_reported = self.access_lvalue(context, lvalue_span,
(Deep, Read(ReadKind::Copy)), flow_state);
}

if error_reported {
if let Lvalue::Local(local) = *lvalue {
self.storage_drop_or_dead_error_reported.insert(local);
}
}
}

// Finally, check if path was already moved.
Expand Down Expand Up @@ -1458,6 +1516,27 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
err.emit();
}

fn report_borrowed_value_does_not_live_long_enough(&mut self,
_: Context,
(lvalue, span): (&Lvalue, Span),
end_span: Option<Span>) {
let proper_span = match *lvalue {
Lvalue::Local(local) => self.mir.local_decls[local].source_info.span,
_ => span
};

let mut err = self.tcx.path_does_not_live_long_enough(span, "borrowed value", Origin::Mir);
err.span_label(proper_span, "temporary value created here");
err.span_label(span, "temporary value dropped here while still borrowed");
err.note("consider using a `let` binding to increase its lifetime");

if let Some(end) = end_span {
err.span_label(end, "temporary value needs to live until here");
}

err.emit();
}

fn report_illegal_mutation_of_borrowed(&mut self,
_: Context,
(lvalue, span): (&Lvalue<'tcx>, Span),
Expand Down
23 changes: 18 additions & 5 deletions src/test/compile-fail/issue-36082.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 emit-end-regions -Z borrowck-mir

use std::cell::RefCell;

fn main() {
Expand All @@ -16,10 +19,20 @@ fn main() {
let x = RefCell::new((&mut r,s));

let val: &_ = x.borrow().0;
//~^ ERROR borrowed value does not live long enough
//~| temporary value dropped here while still borrowed
//~| temporary value created here
//~| consider using a `let` binding to increase its lifetime
//[ast]~^ ERROR borrowed value does not live long enough [E0597]
//[ast]~| NOTE temporary value dropped here while still borrowed
//[ast]~| NOTE temporary value created here
//[ast]~| NOTE consider using a `let` binding to increase its lifetime
//[mir]~^^^^^ ERROR borrowed value does not live long enough (Ast) [E0597]
//[mir]~| NOTE temporary value dropped here while still borrowed
//[mir]~| NOTE temporary value created here
//[mir]~| NOTE consider using a `let` binding to increase its lifetime
//[mir]~| ERROR borrowed value does not live long enough (Mir) [E0597]
//[mir]~| NOTE temporary value dropped here while still borrowed
//[mir]~| NOTE temporary value created here
//[mir]~| NOTE consider using a `let` binding to increase its lifetime
println!("{}", val);
}
//~^ temporary value needs to live until here
//[ast]~^ NOTE temporary value needs to live until here
//[mir]~^^ NOTE temporary value needs to live until here
//[mir]~| NOTE temporary value needs to live until here