Skip to content

Commit

Permalink
MIR changes to improve NLL cannot mutate errors
Browse files Browse the repository at this point in the history 
Alway use unique instead of mutable borrows immutable upvars.
Mark variables that are references for a match guard
  • Loading branch information
@matthewjasper
matthewjasper committed Jul 20, 2018
1 parent 509cbf3 commit c3dbdfe
Show file tree
Hide file tree
Showing 5 changed files with 190 additions and 50 deletions.
3 changes: 3 additions & 0 deletions src/librustc/mir/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,8 @@ pub enum BindingForm<'tcx> {
Var(VarBindingForm<'tcx>),
/// Binding for a `self`/`&self`/`&mut self` binding where the type is implicit.
ImplicitSelf,
/// Reference used in a guard expression to ensure immutability.
RefForGuard,
}

CloneTypeFoldableAndLiftImpls! { BindingForm<'tcx>, }
Expand All @@ -555,6 +557,7 @@ mod binding_form_impl {
match self {
Var(binding) => binding.hash_stable(hcx, hasher),
ImplicitSelf => (),
RefForGuard => (),
}
}
}
Expand Down
18 changes: 18 additions & 0 deletions src/librustc_mir/borrow_check/error_reporting.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,24 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
autoderef,
&including_downcast,
)?;
} else if let Place::Local(local) = proj.base {
if let Some(ClearCrossCrate::Set(BindingForm::RefForGuard))
= self.mir.local_decls[local].is_user_variable {
self.append_place_to_string(
&proj.base,
buf,
autoderef,
&including_downcast,
)?;
} else {
buf.push_str(&"*");
self.append_place_to_string(
&proj.base,
buf,
autoderef,
&including_downcast,
)?;
}
} else {
buf.push_str(&"*");
self.append_place_to_string(
Expand Down
117 changes: 116 additions & 1 deletion src/librustc_mir/build/expr/as_rvalue.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,27 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
this.consume_by_copy_or_move(place)
}
_ => {
unpack!(block = this.as_operand(block, scope, upvar))
// Turn mutable borrow captures into unique
// borrow captures when capturing an immutable
// variable. This is sound because the mutation
// that caused the capture will cause an error.
match upvar.kind {
ExprKind::Borrow {
borrow_kind: BorrowKind::Mut {
allow_two_phase_borrow: false
},
region,
arg,
} => unpack!(block = this.limit_capture_mutability(
upvar.span,
upvar.ty,
scope,
block,
arg,
region,
)),
_ => unpack!(block = this.as_operand(block, scope, upvar)),
}
}
}
})
Expand Down Expand Up @@ -393,6 +413,101 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
}
}

fn limit_capture_mutability(
&mut self,
upvar_span: Span,
upvar_ty: Ty<'tcx>,
temp_lifetime: Option<region::Scope>,
mut block: BasicBlock,
arg: ExprRef<'tcx>,
region: &'tcx ty::RegionKind,
) -> BlockAnd<Operand<'tcx>> {
let this = self;

let source_info = this.source_info(upvar_span);
let temp = this.local_decls.push(LocalDecl::new_temp(upvar_ty, upvar_span));

this.cfg.push(block, Statement {
source_info,
kind: StatementKind::StorageLive(temp)
});

let arg_place = unpack!(block = this.as_place(block, arg));

let mutability = match arg_place {
Place::Local(local) => this.local_decls[local].mutability,
Place::Projection(box Projection {
base: Place::Local(local),
elem: ProjectionElem::Deref,
}) => {
debug_assert!(
if let Some(ClearCrossCrate::Set(BindingForm::RefForGuard))
= this.local_decls[local].is_user_variable {
true
} else {
false
},
"Unexpected capture place",
);
this.local_decls[local].mutability
}
Place::Projection(box Projection {
ref base,
elem: ProjectionElem::Field(upvar_index, _),
})
| Place::Projection(box Projection {
base: Place::Projection(box Projection {
ref base,
elem: ProjectionElem::Field(upvar_index, _),
}),
elem: ProjectionElem::Deref,
}) => {
// Not projected from the implicit `self` in a closure.
debug_assert!(
match *base {
Place::Local(local) => local == Local::new(1),
Place::Projection(box Projection {
ref base,
elem: ProjectionElem::Deref,
}) => *base == Place::Local(Local::new(1)),
_ => false,
},
"Unexpected capture place"
);
// Not in a closure
debug_assert!(
this.upvar_decls.len() > upvar_index.index(),
"Unexpected capture place"
);
this.upvar_decls[upvar_index.index()].mutability
}
_ => bug!("Unexpected capture place"),
};

let borrow_kind = match mutability {
Mutability::Not => BorrowKind::Unique,
Mutability::Mut => BorrowKind::Mut { allow_two_phase_borrow: false },
};

this.cfg.push_assign(
block,
source_info,
&Place::Local(temp),
Rvalue::Ref(region, borrow_kind, arg_place),
);

// In constants, temp_lifetime is None. We should not need to drop
// anything because no values with a destructor can be created in
// a constant at this time, even if the type may need dropping.
if let Some(temp_lifetime) = temp_lifetime {
this.schedule_drop_storage_and_value(
upvar_span, temp_lifetime, &Place::Local(temp), upvar_ty,
);
}

block.and(Operand::Move(Place::Local(temp)))
}

// Helper to get a `-1` value of the appropriate type
fn neg_1_literal(&mut self, span: Span, ty: Ty<'tcx>) -> Operand<'tcx> {
let param_ty = ty::ParamEnv::empty().and(self.hir.tcx().lift_to_global(&ty).unwrap());
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/build/matches/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1198,7 +1198,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
visibility_scope,
// FIXME: should these secretly injected ref_for_guard's be marked as `internal`?
internal: false,
is_user_variable: None,
is_user_variable: Some(ClearCrossCrate::Set(BindingForm::RefForGuard)),
});
LocalsForNode::Three { val_for_guard, ref_for_guard, for_arm_body }
} else {
Expand Down
100 changes: 52 additions & 48 deletions src/librustc_mir/build/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ struct Builder<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
/// (A match binding can have two locals; the 2nd is for the arm's guard.)
var_indices: NodeMap<LocalsForNode>,
local_decls: IndexVec<Local, LocalDecl<'tcx>>,
upvar_decls: Vec<UpvarDecl>,
unit_temp: Option<Place<'tcx>>,

/// cached block with the RESUME terminator; this is created
Expand Down Expand Up @@ -472,11 +473,52 @@ fn construct_fn<'a, 'gcx, 'tcx, A>(hir: Cx<'a, 'gcx, 'tcx>,

let tcx = hir.tcx();
let span = tcx.hir.span(fn_id);

// Gather the upvars of a closure, if any.
let upvar_decls: Vec<_> = tcx.with_freevars(fn_id, |freevars| {
freevars.iter().map(|fv| {
let var_id = fv.var_id();
let var_hir_id = tcx.hir.node_to_hir_id(var_id);
let closure_expr_id = tcx.hir.local_def_id(fn_id);
let capture = hir.tables().upvar_capture(ty::UpvarId {
var_id: var_hir_id,
closure_expr_id: LocalDefId::from_def_id(closure_expr_id),
});
let by_ref = match capture {
ty::UpvarCapture::ByValue => false,
ty::UpvarCapture::ByRef(..) => true
};
let mut decl = UpvarDecl {
debug_name: keywords::Invalid.name(),
var_hir_id: ClearCrossCrate::Set(var_hir_id),
by_ref,
mutability: Mutability::Not,
};
if let Some(hir::map::NodeBinding(pat)) = tcx.hir.find(var_id) {
if let hir::PatKind::Binding(_, _, ident, _) = pat.node {
decl.debug_name = ident.name;

if let Some(&bm) = hir.tables.pat_binding_modes().get(pat.hir_id) {
if bm == ty::BindByValue(hir::MutMutable) {
decl.mutability = Mutability::Mut;
} else {
decl.mutability = Mutability::Not;
}
} else {
tcx.sess.delay_span_bug(pat.span, "missing binding mode");
}
}
}
decl
}).collect()
});

let mut builder = Builder::new(hir.clone(),
span,
arguments.len(),
safety,
return_ty);
return_ty,
upvar_decls);

let fn_def_id = tcx.hir.local_def_id(fn_id);
let call_site_scope = region::Scope::CallSite(body.value.hir_id.local_id);
Expand Down Expand Up @@ -519,46 +561,7 @@ fn construct_fn<'a, 'gcx, 'tcx, A>(hir: Cx<'a, 'gcx, 'tcx>,
info!("fn_id {:?} has attrs {:?}", closure_expr_id,
tcx.get_attrs(closure_expr_id));

// Gather the upvars of a closure, if any.
let upvar_decls: Vec<_> = tcx.with_freevars(fn_id, |freevars| {
freevars.iter().map(|fv| {
let var_id = fv.var_id();
let var_hir_id = tcx.hir.node_to_hir_id(var_id);
let closure_expr_id = tcx.hir.local_def_id(fn_id);
let capture = hir.tables().upvar_capture(ty::UpvarId {
var_id: var_hir_id,
closure_expr_id: LocalDefId::from_def_id(closure_expr_id),
});
let by_ref = match capture {
ty::UpvarCapture::ByValue => false,
ty::UpvarCapture::ByRef(..) => true
};
let mut decl = UpvarDecl {
debug_name: keywords::Invalid.name(),
var_hir_id: ClearCrossCrate::Set(var_hir_id),
by_ref,
mutability: Mutability::Not,
};
if let Some(hir::map::NodeBinding(pat)) = tcx.hir.find(var_id) {
if let hir::PatKind::Binding(_, _, ident, _) = pat.node {
decl.debug_name = ident.name;

if let Some(&bm) = hir.tables.pat_binding_modes().get(pat.hir_id) {
if bm == ty::BindByValue(hir::MutMutable) {
decl.mutability = Mutability::Mut;
} else {
decl.mutability = Mutability::Not;
}
} else {
tcx.sess.delay_span_bug(pat.span, "missing binding mode");
}
}
}
decl
}).collect()
});

let mut mir = builder.finish(upvar_decls, yield_ty);
let mut mir = builder.finish(yield_ty);
mir.spread_arg = spread_arg;
mir
}
Expand All @@ -571,7 +574,7 @@ fn construct_const<'a, 'gcx, 'tcx>(hir: Cx<'a, 'gcx, 'tcx>,
let ty = hir.tables().expr_ty_adjusted(ast_expr);
let owner_id = tcx.hir.body_owner(body_id);
let span = tcx.hir.span(owner_id);
let mut builder = Builder::new(hir.clone(), span, 0, Safety::Safe, ty);
let mut builder = Builder::new(hir.clone(), span, 0, Safety::Safe, ty, vec![]);

let mut block = START_BLOCK;
let expr = builder.hir.mirror(ast_expr);
Expand All @@ -590,7 +593,7 @@ fn construct_const<'a, 'gcx, 'tcx>(hir: Cx<'a, 'gcx, 'tcx>,
TerminatorKind::Unreachable);
}

builder.finish(vec![], None)
builder.finish(None)
}

fn construct_error<'a, 'gcx, 'tcx>(hir: Cx<'a, 'gcx, 'tcx>,
Expand All @@ -599,18 +602,19 @@ fn construct_error<'a, 'gcx, 'tcx>(hir: Cx<'a, 'gcx, 'tcx>,
let owner_id = hir.tcx().hir.body_owner(body_id);
let span = hir.tcx().hir.span(owner_id);
let ty = hir.tcx().types.err;
let mut builder = Builder::new(hir, span, 0, Safety::Safe, ty);
let mut builder = Builder::new(hir, span, 0, Safety::Safe, ty, vec![]);
let source_info = builder.source_info(span);
builder.cfg.terminate(START_BLOCK, source_info, TerminatorKind::Unreachable);
builder.finish(vec![], None)
builder.finish(None)
}

impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
fn new(hir: Cx<'a, 'gcx, 'tcx>,
span: Span,
arg_count: usize,
safety: Safety,
return_ty: Ty<'tcx>)
return_ty: Ty<'tcx>,
upvar_decls: Vec<UpvarDecl>)
-> Builder<'a, 'gcx, 'tcx> {
let lint_level = LintLevel::Explicit(hir.root_lint_level);
let mut builder = Builder {
Expand All @@ -628,6 +632,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
breakable_scopes: vec![],
local_decls: IndexVec::from_elem_n(LocalDecl::new_return_place(return_ty,
span), 1),
upvar_decls,
var_indices: NodeMap(),
unit_temp: None,
cached_resume_block: None,
Expand All @@ -645,7 +650,6 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
}

fn finish(self,
upvar_decls: Vec<UpvarDecl>,
yield_ty: Option<Ty<'tcx>>)
-> Mir<'tcx> {
for (index, block) in self.cfg.basic_blocks.iter().enumerate() {
Expand All @@ -661,7 +665,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
yield_ty,
self.local_decls,
self.arg_count,
upvar_decls,
self.upvar_decls,
self.fn_span
)
}
Expand Down

0 comments on commit c3dbdfe

Please sign in to comment.