Skip to content

Commit 1cc938a

Browse files
alexcrichtonalexcrichton
committed
rollup merge of rust-lang#18337 : bkoropoff/unboxed-imm-upvar-fixes
2 parents 175d6a7 + 5662bba commit 1cc938a

File tree

5 files changed

+174
-76
lines changed

5 files changed

+174
-76
lines changed

‎src/librustc/middle/borrowck/check_loans.rs

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -777,13 +777,28 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
777777
// Otherwise, just a plain error.
778778
match assignee_cmt.note {
779779
mc::NoteClosureEnv(upvar_id) => {
780-
self.bccx.span_err(
781-
assignment_span,
782-
format!("cannot assign to {}",
783-
self.bccx.cmt_to_string(&*assignee_cmt)).as_slice());
784-
self.bccx.span_note(
785-
self.tcx().map.span(upvar_id.closure_expr_id),
786-
"consider changing this closure to take self by mutable reference");
780+
// If this is an `Fn` closure, it simply can't mutate upvars.
781+
// If it's an `FnMut` closure, the original variable was declared immutable.
782+
// We need to determine which is the case here.
783+
let kind = match assignee_cmt.upvar().unwrap().cat {
784+
mc::cat_upvar(mc::Upvar { kind, .. }) => kind,
785+
_ => unreachable!()
786+
};
787+
if kind == ty::FnUnboxedClosureKind {
788+
self.bccx.span_err(
789+
assignment_span,
790+
format!("cannot assign to {}",
791+
self.bccx.cmt_to_string(&*assignee_cmt)).as_slice());
792+
self.bccx.span_note(
793+
self.tcx().map.span(upvar_id.closure_expr_id),
794+
"consider changing this closure to take self by mutable reference");
795+
} else {
796+
self.bccx.span_err(
797+
assignment_span,
798+
format!("cannot assign to {} {}",
799+
assignee_cmt.mutbl.to_user_str(),
800+
self.bccx.cmt_to_string(&*assignee_cmt)).as_slice());
801+
}
787802
}
788803
_ => match opt_loan_path(&assignee_cmt) {
789804
Some(lp) => {
@@ -825,12 +840,20 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
825840
mc::cat_rvalue(..) |
826841
mc::cat_static_item |
827842
mc::cat_deref(_, _, mc::UnsafePtr(..)) |
828-
mc::cat_deref(_, _, mc::BorrowedPtr(..)) |
829843
mc::cat_deref(_, _, mc::Implicit(..)) => {
830844
assert_eq!(cmt.mutbl, mc::McDeclared);
831845
return;
832846
}
833847

848+
mc::cat_deref(_, _, mc::BorrowedPtr(..)) => {
849+
assert_eq!(cmt.mutbl, mc::McDeclared);
850+
// We need to drill down to upvar if applicable
851+
match cmt.upvar() {
852+
Some(b) => cmt = b,
853+
None => return
854+
}
855+
}
856+
834857
mc::cat_deref(b, _, mc::OwnedPtr) => {
835858
assert_eq!(cmt.mutbl, mc::McInherited);
836859
cmt = b;

‎src/librustc/middle/borrowck/mod.rs

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -625,7 +625,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
625625
match err.code {
626626
err_mutbl => {
627627
let descr = match err.cmt.note {
628-
mc::NoteClosureEnv(_) => {
628+
mc::NoteClosureEnv(_) | mc::NoteUpvarRef(_) => {
629629
self.cmt_to_string(&*err.cmt)
630630
}
631631
_ => match opt_loan_path(&err.cmt) {
@@ -761,11 +761,20 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
761761
match code {
762762
err_mutbl(..) => {
763763
match err.cmt.note {
764-
mc::NoteClosureEnv(upvar_id) => {
765-
self.tcx.sess.span_note(
766-
self.tcx.map.span(upvar_id.closure_expr_id),
767-
"consider changing this closure to take \
768-
self by mutable reference");
764+
mc::NoteClosureEnv(upvar_id) | mc::NoteUpvarRef(upvar_id) => {
765+
// If this is an `Fn` closure, it simply can't mutate upvars.
766+
// If it's an `FnMut` closure, the original variable was declared immutable.
767+
// We need to determine which is the case here.
768+
let kind = match err.cmt.upvar().unwrap().cat {
769+
mc::cat_upvar(mc::Upvar { kind, .. }) => kind,
770+
_ => unreachable!()
771+
};
772+
if kind == ty::FnUnboxedClosureKind {
773+
self.tcx.sess.span_note(
774+
self.tcx.map.span(upvar_id.closure_expr_id),
775+
"consider changing this closure to take \
776+
self by mutable reference");
777+
}
769778
}
770779
_ => {}
771780
}

‎src/librustc/middle/mem_categorization.rs

Lines changed: 69 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -655,51 +655,54 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> {
655655
// FnOnce | copied | upvar -> &'up bk
656656
// old stack | N/A | upvar -> &'env mut -> &'up bk
657657
// old proc/once | copied | N/A
658+
let var_ty = if_ok!(self.node_ty(var_id));
659+
658660
let upvar_id = ty::UpvarId { var_id: var_id,
659661
closure_expr_id: fn_node_id };
660662

661-
// Do we need to deref through an env reference?
662-
let has_env_deref = kind != ty::FnOnceUnboxedClosureKind;
663-
664663
// Mutability of original variable itself
665664
let var_mutbl = MutabilityCategory::from_local(self.tcx(), var_id);
666665

667-
// Mutability of environment dereference
668-
let env_mutbl = match kind {
669-
ty::FnOnceUnboxedClosureKind => var_mutbl,
670-
ty::FnMutUnboxedClosureKind => McInherited,
671-
ty::FnUnboxedClosureKind => McImmutable
666+
// Construct information about env pointer dereference, if any
667+
let mutbl = match kind {
668+
ty::FnOnceUnboxedClosureKind => None, // None, env is by-value
669+
ty::FnMutUnboxedClosureKind => match mode { // Depends on capture type
670+
ast::CaptureByValue => Some(var_mutbl), // Mutable if the original var is
671+
ast::CaptureByRef => Some(McDeclared) // Mutable regardless
672+
},
673+
ty::FnUnboxedClosureKind => Some(McImmutable) // Never mutable
672674
};
675+
let env_info = mutbl.map(|env_mutbl| {
676+
// Look up the node ID of the closure body so we can construct
677+
// a free region within it
678+
let fn_body_id = {
679+
let fn_expr = match self.tcx().map.find(fn_node_id) {
680+
Some(ast_map::NodeExpr(e)) => e,
681+
_ => unreachable!()
682+
};
673683

674-
// Look up the node ID of the closure body so we can construct
675-
// a free region within it
676-
let fn_body_id = {
677-
let fn_expr = match self.tcx().map.find(fn_node_id) {
678-
Some(ast_map::NodeExpr(e)) => e,
679-
_ => unreachable!()
684+
match fn_expr.node {
685+
ast::ExprFnBlock(_, _, ref body) |
686+
ast::ExprProc(_, ref body) |
687+
ast::ExprUnboxedFn(_, _, _, ref body) => body.id,
688+
_ => unreachable!()
689+
}
680690
};
681691

682-
match fn_expr.node {
683-
ast::ExprFnBlock(_, _, ref body) |
684-
ast::ExprProc(_, ref body) |
685-
ast::ExprUnboxedFn(_, _, _, ref body) => body.id,
686-
_ => unreachable!()
687-
}
688-
};
692+
// Region of environment pointer
693+
let env_region = ty::ReFree(ty::FreeRegion {
694+
scope_id: fn_body_id,
695+
bound_region: ty::BrEnv
696+
});
689697

690-
// Region of environment pointer
691-
let env_region = ty::ReFree(ty::FreeRegion {
692-
scope_id: fn_body_id,
693-
bound_region: ty::BrEnv
694-
});
695-
696-
let env_ptr = BorrowedPtr(if env_mutbl.is_mutable() {
697-
ty::MutBorrow
698-
} else {
699-
ty::ImmBorrow
700-
}, env_region);
698+
let env_ptr = BorrowedPtr(if env_mutbl.is_mutable() {
699+
ty::MutBorrow
700+
} else {
701+
ty::ImmBorrow
702+
}, env_region);
701703

702-
let var_ty = if_ok!(self.node_ty(var_id));
704+
(env_mutbl, env_ptr)
705+
});
703706

704707
// First, switch by capture mode
705708
Ok(match mode {
@@ -717,25 +720,27 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> {
717720
note: NoteNone
718721
};
719722

720-
if has_env_deref {
721-
// We need to add the env deref. This means that
722-
// the above is actually immutable and has a ref
723-
// type. However, nothing should actually look at
724-
// the type, so we can get away with stuffing a
725-
// `ty_err` in there instead of bothering to
726-
// construct a proper one.
727-
base.mutbl = McImmutable;
728-
base.ty = ty::mk_err();
729-
Rc::new(cmt_ {
730-
id: id,
731-
span: span,
732-
cat: cat_deref(Rc::new(base), 0, env_ptr),
733-
mutbl: env_mutbl,
734-
ty: var_ty,
735-
note: NoteClosureEnv(upvar_id)
736-
})
737-
} else {
738-
Rc::new(base)
723+
match env_info {
724+
Some((env_mutbl, env_ptr)) => {
725+
// We need to add the env deref. This means
726+
// that the above is actually immutable and
727+
// has a ref type. However, nothing should
728+
// actually look at the type, so we can get
729+
// away with stuffing a `ty_err` in there
730+
// instead of bothering to construct a proper
731+
// one.
732+
base.mutbl = McImmutable;
733+
base.ty = ty::mk_err();
734+
Rc::new(cmt_ {
735+
id: id,
736+
span: span,
737+
cat: cat_deref(Rc::new(base), 0, env_ptr),
738+
mutbl: env_mutbl,
739+
ty: var_ty,
740+
note: NoteClosureEnv(upvar_id)
741+
})
742+
}
743+
None => Rc::new(base)
739744
}
740745
},
741746
ast::CaptureByRef => {
@@ -755,16 +760,18 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> {
755760
note: NoteNone
756761
};
757762

758-
// As in the by-value case, add env deref if needed
759-
if has_env_deref {
760-
base = cmt_ {
761-
id: id,
762-
span: span,
763-
cat: cat_deref(Rc::new(base), 0, env_ptr),
764-
mutbl: env_mutbl,
765-
ty: ty::mk_err(),
766-
note: NoteClosureEnv(upvar_id)
767-
};
763+
match env_info {
764+
Some((env_mutbl, env_ptr)) => {
765+
base = cmt_ {
766+
id: id,
767+
span: span,
768+
cat: cat_deref(Rc::new(base), 0, env_ptr),
769+
mutbl: env_mutbl,
770+
ty: ty::mk_err(),
771+
note: NoteClosureEnv(upvar_id)
772+
};
773+
}
774+
None => {}
768775
}
769776

770777
// Look up upvar borrow so we can get its region

‎src/test/compile-fail/unboxed-closure-immutable-capture.rs

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
#![feature(unboxed_closures)]
12+
13+
// Test that even unboxed closures that are capable of mutating their
14+
// environment cannot mutate captured variables that have not been
15+
// declared mutable (#18335)
16+
17+
fn set(x: &mut uint) { *x = 0; }
18+
19+
fn main() {
20+
let x = 0u;
21+
move |&mut:| x = 1; //~ ERROR cannot assign
22+
move |&mut:| set(&mut x); //~ ERROR cannot borrow
23+
move |:| x = 1; //~ ERROR cannot assign
24+
move |:| set(&mut x); //~ ERROR cannot borrow
25+
|&mut:| x = 1; //~ ERROR cannot assign
26+
// FIXME: this should be `cannot borrow` (issue #18330)
27+
|&mut:| set(&mut x); //~ ERROR cannot assign
28+
|:| x = 1; //~ ERROR cannot assign
29+
// FIXME: this should be `cannot borrow` (issue #18330)
30+
|:| set(&mut x); //~ ERROR cannot assign
31+
}

‎src/test/run-pass/unboxed-closures-move-mutable.rs

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
#![feature(unboxed_closures)]
12+
#![deny(unused_mut)]
13+
14+
// Test that mutating a mutable upvar in a capture-by-value unboxed
15+
// closure does not ice (issue #18238) and marks the upvar as used
16+
// mutably so we do not get a spurious warning about it not needing to
17+
// be declared mutable (issue #18336).
18+
19+
fn main() {
20+
{
21+
let mut x = 0u;
22+
move |&mut:| x += 1;
23+
}
24+
{
25+
let mut x = 0u;
26+
move |:| x += 1;
27+
}
28+
}

0 commit comments

Comments
 (0)