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

Jump to bottom

Provide span for declaration of captured variables #53164

Merged
merged 3 commits into from
Aug 15, 2018
Merged

Provide span for declaration of captured variables #53164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51b0552
Added some debug logging.
davidtwco Aug 7, 2018
2c6b0e5
Label definition of captured variables in errors.
davidtwco Aug 7, 2018
5532e9d
Improved how upvars are detected when presenting errors using prefixes.
davidtwco Aug 7, 2018
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
14 changes: 8 additions & 6 deletions src/librustc/mir/tcx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,29 +127,31 @@ 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
}
},
_ => None,
},
}
_ => None,
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/borrow_check/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
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
84 changes: 62 additions & 22 deletions src/librustc_mir/borrow_check/move_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,12 @@
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;
use borrow_check::prefixes::PrefixSet;
use dataflow::move_paths::{IllegalMoveOrigin, IllegalMoveOriginKind};
use dataflow::move_paths::{LookupResult, MoveError, MovePathIndex};
use util::borrowck_errors::{BorrowckErrors, Origin};
Expand All @@ -38,6 +39,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 +48,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 +124,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 +136,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 +147,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 +187,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 +218,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,12 +228,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: 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 @@ -231,22 +255,17 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
// borrow to provide feedback about why this
// was a move rather than a copy.
let ty = place.ty(self.mir, self.tcx).to_ty(self.tcx);
let is_upvar_field_projection =
self.prefixes(&original_path, PrefixSet::All)
.any(|p| p.is_upvar_field_projection(self.mir, &self.tcx)
.is_some());
match ty.sty {
ty::TyArray(..) | ty::TySlice(..) => self
.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() && is_upvar_field_projection
=> {
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 @@ -262,7 +281,28 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
}
None => bug!("closure kind not inferred by borrowck"),
};
self.tcx.cannot_move_out_of(span, place_description, origin)
debug!("report: closure_kind_ty={:?} closure_kind={:?} \
place_description={:?}", closure_kind_ty, closure_kind,
place_description);

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

for prefix in self.prefixes(&original_path, PrefixSet::All) {
if let Some(field) = prefix.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");
break;
}
}

diag
}
_ => self
.tcx
Expand Down
42 changes: 15 additions & 27 deletions src/librustc_mir/borrow_check/mutability_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,18 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
error_access: AccessKind,
location: Location,
) {
debug!(
"report_mutability_error(\
access_place={:?}, span={:?}, the_place_err={:?}, error_access={:?}, location={:?},\
)",
access_place, span, the_place_err, error_access, location,
);

let mut err;
let item_msg;
let reason;
let access_place_desc = self.describe_place(access_place);
debug!("report_mutability_error: access_place_desc={:?}", access_place_desc);

match the_place_err {
Place::Local(local) => {
Expand All @@ -63,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 @@ -82,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 @@ -155,6 +164,8 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
}) => bug!("Unexpected immutable place."),
}

debug!("report_mutability_error: item_msg={:?}, reason={:?}", item_msg, reason);

// `act` and `acted_on` are strings that let us abstract over
// the verbs used in some diagnostic messages.
let act;
Expand Down Expand Up @@ -199,6 +210,8 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
}
};

debug!("report_mutability_error: act={:?}, acted_on={:?}", act, acted_on);

match the_place_err {
// We want to suggest users use `let mut` for local (user
// variable) mutations...
Expand Down Expand Up @@ -382,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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Loading