mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Rollup merge of #55122 - ljedrz:cleanup_mir_borrowck, r=Mark-Simulacrum

Browse Source 
Cleanup mir/borrowck

- remove a redundant `.clone()`
- a few string tweaks
- deduplicate assignments and `return`s
- simplify common patterns
- remove redundant `return`s
This commit is contained in:
kennytm 2018-10-18 10:47:32 +08:00
commit f8fa3da77b
No known key found for this signature in database
GPG Key ID: FEF6C8051D0E013C
5 changed files with 233 additions and 257 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

102
src/librustc_mir/borrow_check/borrow_set.rs
View File

@ -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];
if let Some(&borrow_index) = self.pending_activations.get(temp) {
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 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,
);
}
// 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)
}
};
// 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;
}
None => {}
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,
);
}
// 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)
}
};
}
}
}

349
src/librustc_mir/borrow_check/error_reporting.rs
View File

@ -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={:?}",
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;
}
// 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;
}
}
// If none of our closure's operands matched, then skip to the next
// statement.
continue;
}
// Otherwise, look at other types of assignment.
let assigned_from = match rvalue {
Rvalue::Ref(_, _, assigned_from) => assigned_from,
Rvalue::Use(operand) => match operand {
for operand in operands {
let assigned_from = 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,
);
};
debug!(
"annotate_argument_and_return_for_borrow: assigned_from={:?}",
assigned_from
);
// 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,
);
// Find the local from the operand.
let assigned_from_local = match assigned_from.local() {
Some(local) => local,
None => continue,
};
// 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 assigned_from_local != target {
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;
}
}
// 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;
// If none of our closure's operands matched, then skip to the next
// statement.
continue;
}
}
// 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: assigned_to={:?} args={:?}",
assigned_to, args
);
for operand in args {
let assigned_from = match operand {
// 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,
);
// 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,
);
// 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 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();
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: assigned_to={:?} args={:?}",
assigned_to, args
);
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_from={:?}",
assigned_from,
);
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
src/librustc_mir/borrow_check/mod.rs
View File

@ -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,13 +1044,12 @@ 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,
@ -1059,11 +1057,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
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,7 +1881,7 @@ 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,
@ -1893,10 +1889,10 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
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
src/librustc_mir/borrow_check/move_errors.rs
View File

@ -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
src/librustc_mir/borrow_check/mutability_errors.rs
View File

@ -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));
}
}