Skip to content

Commit

Permalink
Label definition of captured variables in errors.
Browse files Browse the repository at this point in the history
  • Loading branch information
davidtwco committed Aug 14, 2018
1 parent 51b0552 commit 2c6b0e5
Show file tree
Hide file tree
Showing 9 changed files with 95 additions and 63 deletions.
27 changes: 22 additions & 5 deletions src/librustc/mir/tcx.rs
Expand Up @@ -127,22 +127,24 @@ impl<'tcx> Place<'tcx> {
/// of a closure type.
pub fn is_upvar_field_projection<'cx, 'gcx>(&self, mir: &'cx Mir<'tcx>,
tcx: &TyCtxt<'cx, 'gcx, 'tcx>) -> Option<Field> {
let place = if let Place::Projection(ref proj) = self {
let (place, by_ref) = if let Place::Projection(ref proj) = self {
if let ProjectionElem::Deref = proj.elem {
&proj.base
(&proj.base, true)
} else {
self
(self, false)
}
} else {
self
(self, false)
};

match place {
Place::Projection(ref proj) => match proj.elem {
ProjectionElem::Field(field, _ty) => {
let base_ty = proj.base.ty(mir, *tcx).to_ty(*tcx);

if base_ty.is_closure() || base_ty.is_generator() {
if (base_ty.is_closure() || base_ty.is_generator()) &&
(!by_ref || mir.upvar_decls[field.index()].by_ref)
{
Some(field)
} else {
None
Expand All @@ -153,6 +155,21 @@ impl<'tcx> Place<'tcx> {
_ => None,
}
}

/// Strip the deref projections from a `Place`. For example, given `(*(*((*_1).0: &&T)))`, this
/// will return `((*_1).0)`. Once stripped of any deref projections, places can then be
/// checked as upvar field projections using `is_upvar_field_projection`.
pub fn strip_deref_projections(&self) -> &Place<'tcx> {
let mut current = self;
while let Place::Projection(ref proj) = current {
if let ProjectionElem::Deref = proj.elem {
current = &proj.base;
} else {
break;
}
}
current
}
}

pub enum RvalueInitializationState {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/borrow_check/mod.rs
Expand Up @@ -151,7 +151,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
let location_table = &LocationTable::new(mir);

let mut errors_buffer = Vec::new();
let (move_data, move_errors): (MoveData<'tcx>, Option<Vec<MoveError<'tcx>>>) =
let (move_data, move_errors): (MoveData<'tcx>, Option<Vec<(Place<'tcx>, MoveError<'tcx>)>>) =
match MoveData::gather_moves(mir, tcx) {
Ok(move_data) => (move_data, None),
Err((move_data, move_errors)) => (move_data, Some(move_errors)),
Expand Down
74 changes: 51 additions & 23 deletions src/librustc_mir/borrow_check/move_errors.rs
Expand Up @@ -11,8 +11,8 @@
use rustc::hir;
use rustc::mir::*;
use rustc::ty;
use rustc_data_structures::indexed_vec::Idx;
use rustc_errors::DiagnosticBuilder;
use rustc_data_structures::indexed_vec::Idx;
use syntax_pos::Span;

use borrow_check::MirBorrowckCtxt;
Expand All @@ -38,6 +38,7 @@ enum GroupedMoveError<'tcx> {
// Match place can't be moved from
// e.g. match x[0] { s => (), } where x: &[String]
MovesFromMatchPlace {
original_path: Place<'tcx>,
span: Span,
move_from: Place<'tcx>,
kind: IllegalMoveOriginKind<'tcx>,
Expand All @@ -46,37 +47,43 @@ enum GroupedMoveError<'tcx> {
// Part of a pattern can't be moved from,
// e.g. match &String::new() { &x => (), }
MovesFromPattern {
original_path: Place<'tcx>,
span: Span,
move_from: MovePathIndex,
kind: IllegalMoveOriginKind<'tcx>,
binds_to: Vec<Local>,
},
// Everything that isn't from pattern matching.
OtherIllegalMove {
original_path: Place<'tcx>,
span: Span,
kind: IllegalMoveOriginKind<'tcx>,
},
}

impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
pub(crate) fn report_move_errors(&mut self, move_errors: Vec<MoveError<'tcx>>) {
pub(crate) fn report_move_errors(&mut self, move_errors: Vec<(Place<'tcx>, MoveError<'tcx>)>) {
let grouped_errors = self.group_move_errors(move_errors);
for error in grouped_errors {
self.report(error);
}
}

fn group_move_errors(&self, errors: Vec<MoveError<'tcx>>) -> Vec<GroupedMoveError<'tcx>> {
fn group_move_errors(
&self,
errors: Vec<(Place<'tcx>, MoveError<'tcx>)>
) -> Vec<GroupedMoveError<'tcx>> {
let mut grouped_errors = Vec::new();
for error in errors {
self.append_to_grouped_errors(&mut grouped_errors, error);
for (original_path, error) in errors {
self.append_to_grouped_errors(&mut grouped_errors, original_path, error);
}
grouped_errors
}

fn append_to_grouped_errors(
&self,
grouped_errors: &mut Vec<GroupedMoveError<'tcx>>,
original_path: Place<'tcx>,
error: MoveError<'tcx>,
) {
match error {
Expand Down Expand Up @@ -116,6 +123,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
self.append_binding_error(
grouped_errors,
kind,
original_path,
move_from,
*local,
opt_match_place,
Expand All @@ -127,6 +135,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
}
grouped_errors.push(GroupedMoveError::OtherIllegalMove {
span: stmt_source_info.span,
original_path,
kind,
});
}
Expand All @@ -137,6 +146,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
&self,
grouped_errors: &mut Vec<GroupedMoveError<'tcx>>,
kind: IllegalMoveOriginKind<'tcx>,
original_path: Place<'tcx>,
move_from: &Place<'tcx>,
bind_to: Local,
match_place: &Option<Place<'tcx>>,
Expand Down Expand Up @@ -176,6 +186,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
grouped_errors.push(GroupedMoveError::MovesFromMatchPlace {
span,
move_from: match_place.clone(),
original_path,
kind,
binds_to,
});
Expand Down Expand Up @@ -206,6 +217,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
grouped_errors.push(GroupedMoveError::MovesFromPattern {
span: match_span,
move_from: mpi,
original_path,
kind,
binds_to: vec![bind_to],
});
Expand All @@ -215,13 +227,23 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {

fn report(&mut self, error: GroupedMoveError<'tcx>) {
let (mut err, err_span) = {
let (span, kind): (Span, &IllegalMoveOriginKind) = match error {
GroupedMoveError::MovesFromMatchPlace { span, ref kind, .. }
| GroupedMoveError::MovesFromPattern { span, ref kind, .. }
| GroupedMoveError::OtherIllegalMove { span, ref kind } => (span, kind),
};
let (span, original_path, kind): (Span, &Place<'tcx>, &IllegalMoveOriginKind) =
match error {
GroupedMoveError::MovesFromMatchPlace {
span,
ref original_path,
ref kind,
..
} |
GroupedMoveError::MovesFromPattern { span, ref original_path, ref kind, .. } |
GroupedMoveError::OtherIllegalMove { span, ref original_path, ref kind } => {
(span, original_path, kind)
},
};
let origin = Origin::Mir;
debug!("report: span={:?}, kind={:?}", span, kind);
debug!("report: original_path={:?} span={:?}, kind={:?} \
original_path.is_upvar_field_projection={:?}", original_path, span, kind,
original_path.is_upvar_field_projection(self.mir, &self.tcx));
(
match kind {
IllegalMoveOriginKind::Static => {
Expand All @@ -237,17 +259,11 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
.tcx
.cannot_move_out_of_interior_noncopy(span, ty, None, origin),
ty::TyClosure(def_id, closure_substs)
if !self.mir.upvar_decls.is_empty()
&& {
match place {
Place::Projection(ref proj) => {
proj.base == Place::Local(Local::new(1))
}
Place::Promoted(_) |
Place::Local(_) | Place::Static(_) => unreachable!(),
}
} =>
{
if !self.mir.upvar_decls.is_empty() &&
original_path.strip_deref_projections()
.is_upvar_field_projection(self.mir, &self.tcx)
.is_some()
=> {
let closure_kind_ty =
closure_substs.closure_kind_ty(def_id, self.tcx);
let closure_kind = closure_kind_ty.to_opt_closure_kind();
Expand All @@ -267,7 +283,19 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
place_description={:?}", closure_kind_ty, closure_kind,
place_description);

self.tcx.cannot_move_out_of(span, place_description, origin)
let mut diag = self.tcx.cannot_move_out_of(
span, place_description, origin);

if let Some(field) = original_path.is_upvar_field_projection(
self.mir, &self.tcx) {
let upvar_decl = &self.mir.upvar_decls[field.index()];
let upvar_hir_id = upvar_decl.var_hir_id.assert_crate_local();
let upvar_node_id = self.tcx.hir.hir_to_node_id(upvar_hir_id);
let upvar_span = self.tcx.hir.span(upvar_node_id);
diag.span_label(upvar_span, "captured outer variable");
}

diag
}
_ => self
.tcx
Expand Down
30 changes: 3 additions & 27 deletions src/librustc_mir/borrow_check/mutability_errors.rs
Expand Up @@ -71,7 +71,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
));

item_msg = format!("`{}`", access_place_desc.unwrap());
if self.is_upvar(access_place) {
if access_place.is_upvar_field_projection(self.mir, &self.tcx).is_some() {
reason = ", as it is not declared as mutable".to_string();
} else {
let name = self.mir.upvar_decls[upvar_index.index()].debug_name;
Expand All @@ -90,7 +90,8 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
the_place_err.ty(self.mir, self.tcx).to_ty(self.tcx)
));

reason = if self.is_upvar(access_place) {
reason = if access_place.is_upvar_field_projection(self.mir,
&self.tcx).is_some() {
", as it is a captured variable in a `Fn` closure".to_string()
} else {
", as `Fn` closures cannot mutate their captured variables".to_string()
Expand Down Expand Up @@ -394,31 +395,6 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {

err.buffer(&mut self.errors_buffer);
}

// Does this place refer to what the user sees as an upvar
fn is_upvar(&self, place: &Place<'tcx>) -> bool {
match *place {
Place::Projection(box Projection {
ref base,
elem: ProjectionElem::Field(_, _),
}) => {
let base_ty = base.ty(self.mir, self.tcx).to_ty(self.tcx);
is_closure_or_generator(base_ty)
}
Place::Projection(box Projection {
base:
Place::Projection(box Projection {
ref base,
elem: ProjectionElem::Field(upvar_index, _),
}),
elem: ProjectionElem::Deref,
}) => {
let base_ty = base.ty(self.mir, self.tcx).to_ty(self.tcx);
is_closure_or_generator(base_ty) && self.mir.upvar_decls[upvar_index.index()].by_ref
}
_ => false,
}
}
}

fn suggest_ampmut_self<'cx, 'gcx, 'tcx>(
Expand Down
15 changes: 9 additions & 6 deletions src/librustc_mir/dataflow/move_paths/builder.rs
Expand Up @@ -27,7 +27,7 @@ struct MoveDataBuilder<'a, 'gcx: 'tcx, 'tcx: 'a> {
mir: &'a Mir<'tcx>,
tcx: TyCtxt<'a, 'gcx, 'tcx>,
data: MoveData<'tcx>,
errors: Vec<MoveError<'tcx>>,
errors: Vec<(Place<'tcx>, MoveError<'tcx>)>,
}

impl<'a, 'gcx, 'tcx> MoveDataBuilder<'a, 'gcx, 'tcx> {
Expand Down Expand Up @@ -186,7 +186,9 @@ impl<'b, 'a, 'gcx, 'tcx> Gatherer<'b, 'a, 'gcx, 'tcx> {
}

impl<'a, 'gcx, 'tcx> MoveDataBuilder<'a, 'gcx, 'tcx> {
fn finalize(self) -> Result<MoveData<'tcx>, (MoveData<'tcx>, Vec<MoveError<'tcx>>)> {
fn finalize(
self
) -> Result<MoveData<'tcx>, (MoveData<'tcx>, Vec<(Place<'tcx>, MoveError<'tcx>)>)> {
debug!("{}", {
debug!("moves for {:?}:", self.mir.span);
for (j, mo) in self.data.moves.iter_enumerated() {
Expand All @@ -207,9 +209,10 @@ impl<'a, 'gcx, 'tcx> MoveDataBuilder<'a, 'gcx, 'tcx> {
}
}

pub(super) fn gather_moves<'a, 'gcx, 'tcx>(mir: &Mir<'tcx>, tcx: TyCtxt<'a, 'gcx, 'tcx>)
-> Result<MoveData<'tcx>,
(MoveData<'tcx>, Vec<MoveError<'tcx>>)> {
pub(super) fn gather_moves<'a, 'gcx, 'tcx>(
mir: &Mir<'tcx>,
tcx: TyCtxt<'a, 'gcx, 'tcx>
) -> Result<MoveData<'tcx>, (MoveData<'tcx>, Vec<(Place<'tcx>, MoveError<'tcx>)>)> {
let mut builder = MoveDataBuilder::new(mir, tcx);

builder.gather_args();
Expand Down Expand Up @@ -407,7 +410,7 @@ impl<'b, 'a, 'gcx, 'tcx> Gatherer<'b, 'a, 'gcx, 'tcx> {
let path = match self.move_path_for(place) {
Ok(path) | Err(MoveError::UnionMove { path }) => path,
Err(error @ MoveError::IllegalMove { .. }) => {
self.builder.errors.push(error);
self.builder.errors.push((place.clone(), error));
return;
}
};
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/dataflow/move_paths/mod.rs
Expand Up @@ -313,7 +313,7 @@ impl<'tcx> MoveError<'tcx> {

impl<'a, 'gcx, 'tcx> MoveData<'tcx> {
pub fn gather_moves(mir: &Mir<'tcx>, tcx: TyCtxt<'a, 'gcx, 'tcx>)
-> Result<Self, (Self, Vec<MoveError<'tcx>>)> {
-> Result<Self, (Self, Vec<(Place<'tcx>, MoveError<'tcx>)>)> {
builder::gather_moves(mir, tcx)
}
}
2 changes: 2 additions & 0 deletions src/test/ui/borrowck/borrowck-in-static.nll.stderr
@@ -1,6 +1,8 @@
error[E0507]: cannot move out of captured variable in an `Fn` closure
--> $DIR/borrowck-in-static.rs:15:17
|
LL | let x = Box::new(0);
| - captured outer variable
LL | Box::new(|| x) //~ ERROR cannot move out of captured outer variable
| ^ cannot move out of captured variable in an `Fn` closure

Expand Down
@@ -1,6 +1,9 @@
error[E0507]: cannot move out of captured variable in an `Fn` closure
--> $DIR/unboxed-closures-move-upvar-from-non-once-ref-closure.rs:21:9
|
LL | let y = vec![format!("World")];
| - captured outer variable
LL | call(|| {
LL | y.into_iter();
| ^ cannot move out of captured variable in an `Fn` closure

Expand Down
Expand Up @@ -31,6 +31,9 @@ LL | f.f.call_mut(())
error[E0507]: cannot move out of captured variable in an `FnMut` closure
--> $DIR/borrowck-call-is-borrow-issue-12224.rs:66:13
|
LL | let mut f = move |g: Box<FnMut(isize)>, b: isize| {
| ----- captured outer variable
...
LL | foo(f);
| ^ cannot move out of captured variable in an `FnMut` closure

Expand Down

0 comments on commit 2c6b0e5

Please sign in to comment.