Skip to content

Cleanup mir/borrowck #55122

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.

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

Merged
merged 5 commits into from
Oct 18, 2018
Merged
Changes from all commits
Commits
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
100 changes: 48 additions & 52 deletions src/librustc_mir/borrow_check/borrow_set.rs
Original file line number Diff line number Diff line change
@@ -92,12 +92,12 @@ impl<'tcx> fmt::Display for BorrowData<'tcx> {
mir::BorrowKind::Mut { .. } => "mut ",
};
let region = self.region.to_string();
let region = if region.len() > 0 {
format!("{} ", region)
let separator = if !region.is_empty() {
" "
} else {
region
""
};
write!(w, "&{}{}{:?}", region, kind, self.borrowed_place)
write!(w, "&{}{}{}{:?}", region, separator, kind, self.borrowed_place)
}
}

@@ -244,7 +244,7 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {
K: Clone + Eq + Hash,
V: Eq + Hash,
{
map.entry(k.clone()).or_insert(FxHashSet()).insert(v);
map.entry(k.clone()).or_default().insert(v);
}
}

@@ -261,57 +261,53 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {
// ... check whether we (earlier) saw a 2-phase borrow like
//
// TMP = &mut place
match self.pending_activations.get(temp) {
Some(&borrow_index) => {
let borrow_data = &mut self.idx_vec[borrow_index];

// Watch out: the use of TMP in the borrow itself
// doesn't count as an activation. =)
if borrow_data.reserve_location == location && context == PlaceContext::Store {
return;
}
if let Some(&borrow_index) = self.pending_activations.get(temp) {
let borrow_data = &mut self.idx_vec[borrow_index];

if let TwoPhaseActivation::ActivatedAt(other_location) =
borrow_data.activation_location {
span_bug!(
self.mir.source_info(location).span,
"found two uses for 2-phase borrow temporary {:?}: \
{:?} and {:?}",
temp,
location,
other_location,
);
}
// Watch out: the use of TMP in the borrow itself
// doesn't count as an activation. =)
if borrow_data.reserve_location == location && context == PlaceContext::Store {
return;
}

// Otherwise, this is the unique later use
// that we expect.
borrow_data.activation_location = match context {
// The use of TMP in a shared borrow does not
// count as an actual activation.
PlaceContext::Borrow { kind: mir::BorrowKind::Shared, .. }
| PlaceContext::Borrow { kind: mir::BorrowKind::Shallow, .. } => {
TwoPhaseActivation::NotActivated
}
_ => {
// Double check: This borrow is indeed a two-phase borrow (that is,
// we are 'transitioning' from `NotActivated` to `ActivatedAt`) and
// we've not found any other activations (checked above).
assert_eq!(
borrow_data.activation_location,
TwoPhaseActivation::NotActivated,
"never found an activation for this borrow!",
);

self.activation_map
.entry(location)
.or_default()
.push(borrow_index);
TwoPhaseActivation::ActivatedAt(location)
}
};
if let TwoPhaseActivation::ActivatedAt(other_location) =
borrow_data.activation_location {
span_bug!(
self.mir.source_info(location).span,
"found two uses for 2-phase borrow temporary {:?}: \
{:?} and {:?}",
temp,
location,
other_location,
);
}

None => {}
// Otherwise, this is the unique later use
// that we expect.
borrow_data.activation_location = match context {
// The use of TMP in a shared borrow does not
// count as an actual activation.
PlaceContext::Borrow { kind: mir::BorrowKind::Shared, .. }
| PlaceContext::Borrow { kind: mir::BorrowKind::Shallow, .. } => {
TwoPhaseActivation::NotActivated
}
_ => {
// Double check: This borrow is indeed a two-phase borrow (that is,
// we are 'transitioning' from `NotActivated` to `ActivatedAt`) and
// we've not found any other activations (checked above).
assert_eq!(
borrow_data.activation_location,
TwoPhaseActivation::NotActivated,
"never found an activation for this borrow!",
);

self.activation_map
.entry(location)
.or_default()
.push(borrow_index);
TwoPhaseActivation::ActivatedAt(location)
}
};
}
}
}
341 changes: 163 additions & 178 deletions src/librustc_mir/borrow_check/error_reporting.rs
Original file line number Diff line number Diff line change
@@ -77,9 +77,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
if move_out_indices.is_empty() {
let root_place = self.prefixes(&used_place, PrefixSet::All).last().unwrap();

if self.uninitialized_error_reported
.contains(&root_place.clone())
{
if self.uninitialized_error_reported.contains(root_place) {
debug!(
"report_use_of_moved_or_uninitialized place: error about {:?} suppressed",
root_place
@@ -188,11 +186,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
let tables = self.infcx.tcx.typeck_tables_of(id);
let node_id = self.infcx.tcx.hir.as_local_node_id(id).unwrap();
let hir_id = self.infcx.tcx.hir.node_to_hir_id(node_id);
if tables.closure_kind_origins().get(hir_id).is_some() {
false
} else {
true
}

tables.closure_kind_origins().get(hir_id).is_none()
}
_ => true,
};
@@ -582,7 +577,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
fn report_local_value_does_not_live_long_enough(
&mut self,
context: Context,
name: &String,
name: &str,
scope_tree: &Lrc<ScopeTree>,
borrow: &BorrowData<'tcx>,
drop_span: Span,
@@ -1195,10 +1190,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
Place::Static(ref static_) => self.describe_field_from_ty(&static_.ty, field),
Place::Projection(ref proj) => match proj.elem {
ProjectionElem::Deref => self.describe_field(&proj.base, field),
ProjectionElem::Downcast(def, variant_index) => format!(
"{}",
def.variants[variant_index].fields[field.index()].ident
),
ProjectionElem::Downcast(def, variant_index) =>
def.variants[variant_index].fields[field.index()].ident.to_string(),
ProjectionElem::Field(_, field_type) => {
self.describe_field_from_ty(&field_type, field)
}
@@ -1366,191 +1359,184 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
"annotate_argument_and_return_for_borrow: location={:?}",
location
);
match &self.mir[location.block]
.statements
.get(location.statement_index)
if let Some(&Statement { kind: StatementKind::Assign(ref reservation, _), ..})
= &self.mir[location.block].statements.get(location.statement_index)
{
Some(&Statement {
kind: StatementKind::Assign(ref reservation, _),
..
}) => {
debug!(
"annotate_argument_and_return_for_borrow: reservation={:?}",
reservation
);
// Check that the initial assignment of the reserve location is into a temporary.
let mut target = *match reservation {
Place::Local(local) if self.mir.local_kind(*local) == LocalKind::Temp => local,
_ => return None,
};

// Next, look through the rest of the block, checking if we are assigning the
// `target` (that is, the place that contains our borrow) to anything.
let mut annotated_closure = None;
for stmt in &self.mir[location.block].statements[location.statement_index + 1..] {
debug!(
"annotate_argument_and_return_for_borrow: reservation={:?}",
reservation
"annotate_argument_and_return_for_borrow: target={:?} stmt={:?}",
target, stmt
);
// Check that the initial assignment of the reserve location is into a temporary.
let mut target = *match reservation {
Place::Local(local) if self.mir.local_kind(*local) == LocalKind::Temp => local,
_ => return None,
};

// Next, look through the rest of the block, checking if we are assigning the
// `target` (that is, the place that contains our borrow) to anything.
let mut annotated_closure = None;
for stmt in &self.mir[location.block].statements[location.statement_index + 1..] {
if let StatementKind::Assign(Place::Local(assigned_to), box rvalue) = &stmt.kind
{
debug!(
"annotate_argument_and_return_for_borrow: target={:?} stmt={:?}",
target, stmt
"annotate_argument_and_return_for_borrow: assigned_to={:?} \
rvalue={:?}",
assigned_to, rvalue
);
if let StatementKind::Assign(Place::Local(assigned_to), box rvalue) = &stmt.kind
// Check if our `target` was captured by a closure.
if let Rvalue::Aggregate(
box AggregateKind::Closure(def_id, substs),
operands,
) = rvalue
{
debug!(
"annotate_argument_and_return_for_borrow: assigned_to={:?} \
rvalue={:?}",
assigned_to, rvalue
);
// Check if our `target` was captured by a closure.
if let Rvalue::Aggregate(
box AggregateKind::Closure(def_id, substs),
operands,
) = rvalue
{
for operand in operands {
let assigned_from = match operand {
Operand::Copy(assigned_from) | Operand::Move(assigned_from) => {
assigned_from
}
_ => continue,
};
debug!(
"annotate_argument_and_return_for_borrow: assigned_from={:?}",
for operand in operands {
let assigned_from = match operand {
Operand::Copy(assigned_from) | Operand::Move(assigned_from) => {
assigned_from
);

// Find the local from the operand.
let assigned_from_local = match assigned_from.local() {
Some(local) => local,
None => continue,
};

if assigned_from_local != target {
continue;
}
_ => continue,
};
debug!(
"annotate_argument_and_return_for_borrow: assigned_from={:?}",
assigned_from
);

// If a closure captured our `target` and then assigned
// into a place then we should annotate the closure in
// case it ends up being assigned into the return place.
annotated_closure = self.annotate_fn_sig(
*def_id,
self.infcx.closure_sig(*def_id, *substs),
);
debug!(
"annotate_argument_and_return_for_borrow: \
annotated_closure={:?} assigned_from_local={:?} \
assigned_to={:?}",
annotated_closure, assigned_from_local, assigned_to
);

if *assigned_to == mir::RETURN_PLACE {
// If it was assigned directly into the return place, then
// return now.
return annotated_closure;
} else {
// Otherwise, update the target.
target = *assigned_to;
}
// Find the local from the operand.
let assigned_from_local = match assigned_from.local() {
Some(local) => local,
None => continue,
};

if assigned_from_local != target {
continue;
}

// If none of our closure's operands matched, then skip to the next
// statement.
continue;
// If a closure captured our `target` and then assigned
// into a place then we should annotate the closure in
// case it ends up being assigned into the return place.
annotated_closure = self.annotate_fn_sig(
*def_id,
self.infcx.closure_sig(*def_id, *substs),
);
debug!(
"annotate_argument_and_return_for_borrow: \
annotated_closure={:?} assigned_from_local={:?} \
assigned_to={:?}",
annotated_closure, assigned_from_local, assigned_to
);

if *assigned_to == mir::RETURN_PLACE {
// If it was assigned directly into the return place, then
// return now.
return annotated_closure;
} else {
// Otherwise, update the target.
target = *assigned_to;
}
}

// Otherwise, look at other types of assignment.
let assigned_from = match rvalue {
Rvalue::Ref(_, _, assigned_from) => assigned_from,
Rvalue::Use(operand) => match operand {
Operand::Copy(assigned_from) | Operand::Move(assigned_from) => {
assigned_from
}
_ => continue,
},
_ => continue,
};
debug!(
"annotate_argument_and_return_for_borrow: \
assigned_from={:?}",
assigned_from,
);
// If none of our closure's operands matched, then skip to the next
// statement.
continue;
}

// Find the local from the rvalue.
let assigned_from_local = match assigned_from.local() {
Some(local) => local,
None => continue,
};
debug!(
"annotate_argument_and_return_for_borrow: \
assigned_from_local={:?}",
assigned_from_local,
);
// Otherwise, look at other types of assignment.
let assigned_from = match rvalue {
Rvalue::Ref(_, _, assigned_from) => assigned_from,
Rvalue::Use(operand) => match operand {
Operand::Copy(assigned_from) | Operand::Move(assigned_from) => {
assigned_from
}
_ => continue,
},
_ => continue,
};
debug!(
"annotate_argument_and_return_for_borrow: \
assigned_from={:?}",
assigned_from,
);

// Check if our local matches the target - if so, we've assigned our
// borrow to a new place.
if assigned_from_local != target {
continue;
}
// Find the local from the rvalue.
let assigned_from_local = match assigned_from.local() {
Some(local) => local,
None => continue,
};
debug!(
"annotate_argument_and_return_for_borrow: \
assigned_from_local={:?}",
assigned_from_local,
);

// If we assigned our `target` into a new place, then we should
// check if it was the return place.
debug!(
"annotate_argument_and_return_for_borrow: \
assigned_from_local={:?} assigned_to={:?}",
assigned_from_local, assigned_to
);
if *assigned_to == mir::RETURN_PLACE {
// If it was then return the annotated closure if there was one,
// else, annotate this function.
return annotated_closure.or_else(fallback);
}
// Check if our local matches the target - if so, we've assigned our
// borrow to a new place.
if assigned_from_local != target {
continue;
}

// If we didn't assign into the return place, then we just update
// the target.
target = *assigned_to;
// If we assigned our `target` into a new place, then we should
// check if it was the return place.
debug!(
"annotate_argument_and_return_for_borrow: \
assigned_from_local={:?} assigned_to={:?}",
assigned_from_local, assigned_to
);
if *assigned_to == mir::RETURN_PLACE {
// If it was then return the annotated closure if there was one,
// else, annotate this function.
return annotated_closure.or_else(fallback);
}

// If we didn't assign into the return place, then we just update
// the target.
target = *assigned_to;
}
}

// Check the terminator if we didn't find anything in the statements.
let terminator = &self.mir[location.block].terminator();
// Check the terminator if we didn't find anything in the statements.
let terminator = &self.mir[location.block].terminator();
debug!(
"annotate_argument_and_return_for_borrow: target={:?} terminator={:?}",
target, terminator
);
if let TerminatorKind::Call {
destination: Some((Place::Local(assigned_to), _)),
args,
..
} = &terminator.kind
{
debug!(
"annotate_argument_and_return_for_borrow: target={:?} terminator={:?}",
target, terminator
"annotate_argument_and_return_for_borrow: assigned_to={:?} args={:?}",
assigned_to, args
);
if let TerminatorKind::Call {
destination: Some((Place::Local(assigned_to), _)),
args,
..
} = &terminator.kind
{
for operand in args {
let assigned_from = match operand {
Operand::Copy(assigned_from) | Operand::Move(assigned_from) => {
assigned_from
}
_ => continue,
};
debug!(
"annotate_argument_and_return_for_borrow: assigned_to={:?} args={:?}",
assigned_to, args
"annotate_argument_and_return_for_borrow: assigned_from={:?}",
assigned_from,
);
for operand in args {
let assigned_from = match operand {
Operand::Copy(assigned_from) | Operand::Move(assigned_from) => {
assigned_from
}
_ => continue,
};

if let Some(assigned_from_local) = assigned_from.local() {
debug!(
"annotate_argument_and_return_for_borrow: assigned_from={:?}",
assigned_from,
"annotate_argument_and_return_for_borrow: assigned_from_local={:?}",
assigned_from_local,
);

if let Some(assigned_from_local) = assigned_from.local() {
debug!(
"annotate_argument_and_return_for_borrow: assigned_from_local={:?}",
assigned_from_local,
);

if *assigned_to == mir::RETURN_PLACE && assigned_from_local == target {
return annotated_closure.or_else(fallback);
}
if *assigned_to == mir::RETURN_PLACE && assigned_from_local == target {
return annotated_closure.or_else(fallback);
}
}
}
}
_ => {}
}

// If we haven't found an assignment into the return place, then we need not add
@@ -1605,13 +1591,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
// Need to use the `rustc::ty` types to compare against the
// `return_region`. Then use the `rustc::hir` type to get only
// the lifetime span.
match &fn_decl.inputs[index].node {
hir::TyKind::Rptr(lifetime, _) => {
// With access to the lifetime, we can get
// the span of it.
arguments.push((*argument, lifetime.span));
}
_ => bug!("ty type is a ref but hir type is not"),
if let hir::TyKind::Rptr(lifetime, _) = &fn_decl.inputs[index].node {
// With access to the lifetime, we can get
// the span of it.
arguments.push((*argument, lifetime.span));
} else {
bug!("ty type is a ref but hir type is not");
}
}
}
@@ -1794,8 +1779,8 @@ impl<'tcx> AnnotatedBorrowFnSignature<'tcx> {
ty::RegionKind::RePlaceholder(ty::Placeholder { name: br, .. }),
_,
_,
) => with_highlight_region_for_bound_region(*br, counter, || format!("{}", ty)),
_ => format!("{}", ty),
) => with_highlight_region_for_bound_region(*br, counter, || ty.to_string()),
_ => ty.to_string(),
}
}

@@ -1806,9 +1791,9 @@ impl<'tcx> AnnotatedBorrowFnSignature<'tcx> {
ty::TyKind::Ref(region, _, _) => match region {
ty::RegionKind::ReLateBound(_, br)
| ty::RegionKind::RePlaceholder(ty::Placeholder { name: br, .. }) => {
with_highlight_region_for_bound_region(*br, counter, || format!("{}", region))
with_highlight_region_for_bound_region(*br, counter, || region.to_string())
}
_ => format!("{}", region),
_ => region.to_string(),
},
_ => bug!("ty for annotation of borrow region is not a reference"),
}
26 changes: 11 additions & 15 deletions src/librustc_mir/borrow_check/mod.rs
Original file line number Diff line number Diff line change
@@ -284,7 +284,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
let temporary_used_locals: FxHashSet<Local> = mbcx
.used_mut
.iter()
.filter(|&local| !mbcx.mir.local_decls[*local].is_user_variable.is_some())
.filter(|&local| mbcx.mir.local_decls[*local].is_user_variable.is_none())
.cloned()
.collect();
mbcx.gather_used_muts(temporary_used_locals);
@@ -342,7 +342,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
diag.buffer(&mut mbcx.errors_buffer);
}

if mbcx.errors_buffer.len() > 0 {
if !mbcx.errors_buffer.is_empty() {
mbcx.errors_buffer.sort_by_key(|diag| diag.span.primary_span());

if tcx.migrate_borrowck() {
@@ -1009,13 +1009,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
return Control::Continue;
}

error_reported = true;
match kind {
ReadKind::Copy => {
error_reported = true;
this.report_use_while_mutably_borrowed(context, place_span, borrow)
}
ReadKind::Borrow(bk) => {
error_reported = true;
this.report_conflicting_borrow(context, place_span, bk, &borrow)
}
}
@@ -1045,25 +1044,22 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
Read(..) | Write(..) => {}
}

error_reported = true;
match kind {
WriteKind::MutableBorrow(bk) => {
error_reported = true;
this.report_conflicting_borrow(context, place_span, bk, &borrow)
}
WriteKind::StorageDeadOrDrop => {
error_reported = true;
this.report_borrowed_value_does_not_live_long_enough(
context,
borrow,
place_span,
Some(kind))
}
WriteKind::Mutate => {
error_reported = true;
this.report_illegal_mutation_of_borrowed(context, place_span, borrow)
}
WriteKind::Move => {
error_reported = true;
this.report_move_out_while_borrowed(context, place_span, &borrow)
}
}
@@ -1593,7 +1589,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
Place::Local(_) => panic!("should have move path for every Local"),
Place::Projection(_) => panic!("PrefixSet::All meant don't stop for Projection"),
Place::Promoted(_) |
Place::Static(_) => return Err(NoMovePathFound::ReachedStatic),
Place::Static(_) => Err(NoMovePathFound::ReachedStatic),
}
}

@@ -1885,18 +1881,18 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
}

// at this point, we have set up the error reporting state.
if previously_initialized {
return if previously_initialized {
self.report_mutability_error(
place,
span,
the_place_err,
error_access,
location,
);
return true;
true
} else {
return false;
}
false
};
}

fn is_local_ever_initialized(&self,
@@ -1911,7 +1907,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
return Some(index);
}
}
return None;
None
}

/// Adds the place into the used mutable variables set
@@ -2171,7 +2167,7 @@ impl ContextKind {
fn new(self, loc: Location) -> Context {
Context {
kind: self,
loc: loc,
loc,
}
}
}
6 changes: 3 additions & 3 deletions src/librustc_mir/borrow_check/move_errors.rs
Original file line number Diff line number Diff line change
@@ -331,7 +331,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
_ => {
let source = self.borrowed_content_source(place);
self.infcx.tcx.cannot_move_out_of(
span, &format!("{}", source), origin
span, &source.to_string(), origin
)
},
}
@@ -469,9 +469,9 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
let binding_span = bind_to.source_info.span;

if j == 0 {
err.span_label(binding_span, format!("data moved here"));
err.span_label(binding_span, "data moved here");
} else {
err.span_label(binding_span, format!("...and here"));
err.span_label(binding_span, "...and here");
}

if binds_to.len() == 1 {
7 changes: 3 additions & 4 deletions src/librustc_mir/borrow_check/mutability_errors.rs
Original file line number Diff line number Diff line change
@@ -408,7 +408,6 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
.map(|replacement| (pattern_span, replacement))
}

//
ClearCrossCrate::Set(mir::BindingForm::RefForGuard) => unreachable!(),

ClearCrossCrate::Clear => bug!("saw cleared local state"),
@@ -505,7 +504,7 @@ impl<'a, 'gcx, 'tcx> MirBorrowckCtxt<'a, 'gcx, 'tcx> {
);

let extra = if found {
String::from("")
String::new()
} else {
format!(", but it is not implemented for `{}`",
substs.type_at(0))
@@ -573,7 +572,7 @@ fn suggest_ampmut<'cx, 'gcx, 'tcx>(
opt_ty_info: Option<Span>,
) -> (Span, String) {
let locations = mir.find_assignments(local);
if locations.len() > 0 {
if !locations.is_empty() {
let assignment_rhs_span = mir.source_info(locations[0]).span;
if let Ok(src) = tcx.sess.source_map().span_to_snippet(assignment_rhs_span) {
if let (true, Some(ws_pos)) = (
@@ -584,7 +583,7 @@ fn suggest_ampmut<'cx, 'gcx, 'tcx>(
let ty = &src[ws_pos..];
return (assignment_rhs_span, format!("&{} mut {}", lt_name, ty));
} else if src.starts_with('&') {
let borrowed_expr = src[1..].to_string();
let borrowed_expr = &src[1..];
return (assignment_rhs_span, format!("&mut {}", borrowed_expr));
}
}