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

Fix closure upvar soundness bug in regionck #17869

Merged
merged 4 commits into from
Oct 17, 2014
Merged

Fix closure upvar soundness bug in regionck #17869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9094aab
Fix soundness bug in treatment of closure upvars by regionck
bkoropoff Oct 8, 2014
a8f90bc
Update test for issue 17780 since diagnostic message have changed
bkoropoff Oct 12, 2014
a5e1aeb
Add regression test for issue #17403
bkoropoff Oct 8, 2014
fdd69ac
Add failure tests for moving out of unboxed closure environments
bkoropoff Oct 15, 2014
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
1 change: 1 addition & 0 deletions src/librustc/metadata/tydecode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ fn parse_bound_region(st: &mut PState, conv: conv_did) -> ty::BoundRegion {
assert_eq!(next(st), '|');
ty::BrFresh(id)
}
'e' => ty::BrEnv,
_ => fail!("parse_bound_region: bad input")
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/librustc/metadata/tyencode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,9 @@ fn enc_bound_region(w: &mut SeekableMemWriter, cx: &ctxt, br: ty::BoundRegion) {
ty::BrFresh(id) => {
mywrite!(w, "f{}|", id);
}
ty::BrEnv => {
mywrite!(w, "e|");
}
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/librustc/middle/astencode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,8 @@ impl tr for ty::BoundRegion {
fn tr(&self, dcx: &DecodeContext) -> ty::BoundRegion {
match *self {
ty::BrAnon(_) |
ty::BrFresh(_) => *self,
ty::BrFresh(_) |
ty::BrEnv => *self,
ty::BrNamed(id, ident) => ty::BrNamed(dcx.tr_def_id(id),
ident),
}
Expand Down
47 changes: 24 additions & 23 deletions src/librustc/middle/borrowck/check_loans.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,21 +775,32 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
}

// Otherwise, just a plain error.
match opt_loan_path(&assignee_cmt) {
Some(lp) => {
self.bccx.span_err(
assignment_span,
format!("cannot assign to {} {} `{}`",
assignee_cmt.mutbl.to_user_str(),
self.bccx.cmt_to_string(&*assignee_cmt),
self.bccx.loan_path_to_string(&*lp)).as_slice());
}
None => {
match assignee_cmt.note {
mc::NoteClosureEnv(upvar_id) => {
self.bccx.span_err(
assignment_span,
format!("cannot assign to {} {}",
assignee_cmt.mutbl.to_user_str(),
format!("cannot assign to {}",
self.bccx.cmt_to_string(&*assignee_cmt)).as_slice());
self.bccx.span_note(
self.tcx().map.span(upvar_id.closure_expr_id),
"consider changing this closure to take self by mutable reference");
}
_ => match opt_loan_path(&assignee_cmt) {
Some(lp) => {
self.bccx.span_err(
assignment_span,
format!("cannot assign to {} {} `{}`",
assignee_cmt.mutbl.to_user_str(),
self.bccx.cmt_to_string(&*assignee_cmt),
self.bccx.loan_path_to_string(&*lp)).as_slice());
}
None => {
self.bccx.span_err(
assignment_span,
format!("cannot assign to {} {}",
assignee_cmt.mutbl.to_user_str(),
self.bccx.cmt_to_string(&*assignee_cmt)).as_slice());
}
}
}
return;
Expand All @@ -805,16 +816,12 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
loop {
debug!("mark_variable_as_used_mut(cmt={})", cmt.repr(this.tcx()));
match cmt.cat.clone() {
mc::cat_copied_upvar(mc::CopiedUpvar { upvar_id: id, .. }) |
mc::cat_upvar(mc::Upvar { id: ty::UpvarId { var_id: id, .. }, .. }) |
mc::cat_local(id) => {
this.tcx().used_mut_nodes.borrow_mut().insert(id);
return;
}

mc::cat_upvar(..) => {
return;
}

mc::cat_rvalue(..) |
mc::cat_static_item |
mc::cat_deref(_, _, mc::UnsafePtr(..)) |
Expand Down Expand Up @@ -854,12 +861,6 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
check_for_aliasability_violation(this, span, b.clone());
}

mc::cat_copied_upvar(mc::CopiedUpvar {
kind: mc::Unboxed(ty::FnUnboxedClosureKind), ..}) => {
// Prohibit writes to capture-by-move upvars in non-once closures
check_for_aliasability_violation(this, span, guarantor.clone());
}

_ => {}
}

Expand Down
12 changes: 3 additions & 9 deletions src/librustc/middle/borrowck/gather_loans/gather_moves.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,19 +133,13 @@ fn check_and_get_illegal_move_origin(bccx: &BorrowckCtxt,
mc::cat_deref(_, _, mc::BorrowedPtr(..)) |
mc::cat_deref(_, _, mc::Implicit(..)) |
mc::cat_deref(_, _, mc::UnsafePtr(..)) |
mc::cat_upvar(..) | mc::cat_static_item => {
mc::cat_static_item => {
Some(cmt.clone())
}

mc::cat_copied_upvar(mc::CopiedUpvar { kind: kind, .. }) => {
match kind.onceness() {
ast::Once => None,
ast::Many => Some(cmt.clone())
}
}

mc::cat_rvalue(..) |
mc::cat_local(..) => {
mc::cat_local(..) |
mc::cat_upvar(..) => {
None
}

Expand Down
4 changes: 1 addition & 3 deletions src/librustc/middle/borrowck/gather_loans/lifetime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ impl<'a, 'tcx> GuaranteeLifetimeContext<'a, 'tcx> {

match cmt.cat {
mc::cat_rvalue(..) |
mc::cat_copied_upvar(..) | // L-Local
mc::cat_local(..) | // L-Local
mc::cat_upvar(..) |
mc::cat_deref(_, _, mc::BorrowedPtr(..)) | // L-Deref-Borrowed
Expand Down Expand Up @@ -165,8 +164,7 @@ impl<'a, 'tcx> GuaranteeLifetimeContext<'a, 'tcx> {
mc::cat_rvalue(temp_scope) => {
temp_scope
}
mc::cat_upvar(..) |
mc::cat_copied_upvar(_) => {
mc::cat_upvar(..) => {
ty::ReScope(self.item_scope_id)
}
mc::cat_static_item => {
Expand Down
10 changes: 1 addition & 9 deletions src/librustc/middle/borrowck/gather_loans/move_error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,15 +115,7 @@ fn report_cannot_move_out_of(bccx: &BorrowckCtxt, move_from: mc::cmt) {
mc::cat_deref(_, _, mc::BorrowedPtr(..)) |
mc::cat_deref(_, _, mc::Implicit(..)) |
mc::cat_deref(_, _, mc::UnsafePtr(..)) |
mc::cat_upvar(..) | mc::cat_static_item => {
bccx.span_err(
move_from.span,
format!("cannot move out of {}",
bccx.cmt_to_string(&*move_from)).as_slice());
}

mc::cat_copied_upvar(mc::CopiedUpvar { kind: kind, .. })
if kind.onceness() == ast::Many => {
mc::cat_static_item => {
bccx.span_err(
move_from.span,
format!("cannot move out of {}",
Expand Down
10 changes: 2 additions & 8 deletions src/librustc/middle/borrowck/gather_loans/restrictions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,15 +73,9 @@ impl<'a, 'tcx> RestrictionsContext<'a, 'tcx> {
SafeIf(lp.clone(), vec![lp])
}

mc::cat_upvar(upvar_id, _, _) => {
mc::cat_upvar(mc::Upvar { id, .. }) => {
// R-Variable, captured into closure
let lp = Rc::new(LpUpvar(upvar_id));
SafeIf(lp.clone(), vec![lp])
}

mc::cat_copied_upvar(mc::CopiedUpvar { upvar_id, .. }) => {
// R-Variable, copied/moved into closure
let lp = Rc::new(LpVar(upvar_id));
let lp = Rc::new(LpUpvar(id));
SafeIf(lp.clone(), vec![lp])
}

Expand Down
54 changes: 30 additions & 24 deletions src/librustc/middle/borrowck/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,21 +359,12 @@ pub fn opt_loan_path(cmt: &mc::cmt) -> Option<Rc<LoanPath>> {
None
}

mc::cat_copied_upvar(mc::CopiedUpvar { kind: kind, .. })
if kind.onceness() == ast::Many => {
None
}

mc::cat_local(id) => {
Some(Rc::new(LpVar(id)))
}

mc::cat_upvar(ty::UpvarId {var_id: id, closure_expr_id: proc_id}, _, _) |
mc::cat_copied_upvar(mc::CopiedUpvar { upvar_id: id,
kind: _,
capturing_proc: proc_id }) => {
let upvar_id = ty::UpvarId{ var_id: id, closure_expr_id: proc_id };
Some(Rc::new(LpUpvar(upvar_id)))
mc::cat_upvar(mc::Upvar { id, .. }) => {
Some(Rc::new(LpUpvar(id)))
}

mc::cat_deref(ref cmt_base, _, pk) => {
Expand Down Expand Up @@ -630,17 +621,22 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
pub fn bckerr_to_string(&self, err: &BckError) -> String {
match err.code {
err_mutbl => {
let descr = match opt_loan_path(&err.cmt) {
None => {
format!("{} {}",
err.cmt.mutbl.to_user_str(),
self.cmt_to_string(&*err.cmt))
let descr = match err.cmt.note {
mc::NoteClosureEnv(_) => {
self.cmt_to_string(&*err.cmt)
}
Some(lp) => {
format!("{} {} `{}`",
err.cmt.mutbl.to_user_str(),
self.cmt_to_string(&*err.cmt),
self.loan_path_to_string(&*lp))
_ => match opt_loan_path(&err.cmt) {
None => {
format!("{} {}",
err.cmt.mutbl.to_user_str(),
self.cmt_to_string(&*err.cmt))
}
Some(lp) => {
format!("{} {} `{}`",
err.cmt.mutbl.to_user_str(),
self.cmt_to_string(&*err.cmt),
self.loan_path_to_string(&*lp))
}
}
};

Expand Down Expand Up @@ -732,8 +728,8 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
}
mc::AliasableClosure(id) => {
self.tcx.sess.span_err(span,
format!("{} in a free variable from an \
immutable unboxed closure", prefix).as_slice());
format!("{} in a captured outer \
variable in an `Fn` closure", prefix).as_slice());
span_note!(self.tcx.sess, self.tcx.map.span(id),
"consider changing this closure to take self by mutable reference");
}
Expand All @@ -760,7 +756,17 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
pub fn note_and_explain_bckerr(&self, err: BckError) {
let code = err.code;
match code {
err_mutbl(..) => { }
err_mutbl(..) => {
match err.cmt.note {
mc::NoteClosureEnv(upvar_id) => {
self.tcx.sess.span_note(
self.tcx.map.span(upvar_id.closure_expr_id),
"consider changing this closure to take \
self by mutable reference");
}
_ => {}
}
}

err_out_of_scope(super_scope, sub_scope) => {
note_and_explain_region(
Expand Down
2 changes: 0 additions & 2 deletions src/librustc/middle/check_static.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,6 @@ impl euv::Delegate for GlobalChecker {
mc::cat_interior(ref cmt, _) => cur = cmt,

mc::cat_rvalue(..) |
mc::cat_copied_upvar(..) |
mc::cat_upvar(..) |
mc::cat_local(..) => break,
}
Expand Down Expand Up @@ -299,7 +298,6 @@ impl euv::Delegate for GlobalChecker {

mc::cat_downcast(..) |
mc::cat_discr(..) |
mc::cat_copied_upvar(..) |
mc::cat_upvar(..) |
mc::cat_local(..) => unreachable!(),
}
Expand Down
Loading