diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index f5e849f4196..de61f4f2b40 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -777,13 +777,28 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> { // Otherwise, just a plain error. match assignee_cmt.note { mc::NoteClosureEnv(upvar_id) => { - self.bccx.span_err( - assignment_span, - 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"); + // If this is an `Fn` closure, it simply can't mutate upvars. + // If it's an `FnMut` closure, the original variable was declared immutable. + // We need to determine which is the case here. + let kind = match assignee_cmt.upvar().unwrap().cat { + mc::cat_upvar(mc::Upvar { kind, .. }) => kind, + _ => unreachable!() + }; + if kind == ty::FnUnboxedClosureKind { + self.bccx.span_err( + assignment_span, + 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"); + } else { + 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()); + } } _ => match opt_loan_path(&assignee_cmt) { Some(lp) => { @@ -825,12 +840,20 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> { mc::cat_rvalue(..) | mc::cat_static_item | mc::cat_deref(_, _, mc::UnsafePtr(..)) | - mc::cat_deref(_, _, mc::BorrowedPtr(..)) | mc::cat_deref(_, _, mc::Implicit(..)) => { assert_eq!(cmt.mutbl, mc::McDeclared); return; } + mc::cat_deref(_, _, mc::BorrowedPtr(..)) => { + assert_eq!(cmt.mutbl, mc::McDeclared); + // We need to drill down to upvar if applicable + match cmt.upvar() { + Some(b) => cmt = b, + None => return + } + } + mc::cat_deref(b, _, mc::OwnedPtr) => { assert_eq!(cmt.mutbl, mc::McInherited); cmt = b; diff --git a/src/librustc/middle/borrowck/mod.rs b/src/librustc/middle/borrowck/mod.rs index 4620b8b00f4..b09e9105f3f 100644 --- a/src/librustc/middle/borrowck/mod.rs +++ b/src/librustc/middle/borrowck/mod.rs @@ -625,7 +625,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { match err.code { err_mutbl => { let descr = match err.cmt.note { - mc::NoteClosureEnv(_) => { + mc::NoteClosureEnv(_) | mc::NoteUpvarRef(_) => { self.cmt_to_string(&*err.cmt) } _ => match opt_loan_path(&err.cmt) { @@ -761,11 +761,20 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> { match code { 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"); + mc::NoteClosureEnv(upvar_id) | mc::NoteUpvarRef(upvar_id) => { + // If this is an `Fn` closure, it simply can't mutate upvars. + // If it's an `FnMut` closure, the original variable was declared immutable. + // We need to determine which is the case here. + let kind = match err.cmt.upvar().unwrap().cat { + mc::cat_upvar(mc::Upvar { kind, .. }) => kind, + _ => unreachable!() + }; + if kind == ty::FnUnboxedClosureKind { + self.tcx.sess.span_note( + self.tcx.map.span(upvar_id.closure_expr_id), + "consider changing this closure to take \ + self by mutable reference"); + } } _ => {} } diff --git a/src/librustc/middle/mem_categorization.rs b/src/librustc/middle/mem_categorization.rs index 0cac3fcdba1..08e14e8034e 100644 --- a/src/librustc/middle/mem_categorization.rs +++ b/src/librustc/middle/mem_categorization.rs @@ -655,52 +655,55 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> { // FnOnce | copied | upvar -> &'up bk // old stack | N/A | upvar -> &'env mut -> &'up bk // old proc/once | copied | N/A + let var_ty = if_ok!(self.node_ty(var_id)); + let upvar_id = ty::UpvarId { var_id: var_id, closure_expr_id: fn_node_id }; - // Do we need to deref through an env reference? - let has_env_deref = kind != ty::FnOnceUnboxedClosureKind; - // Mutability of original variable itself let var_mutbl = MutabilityCategory::from_local(self.tcx(), var_id); - // Mutability of environment dereference - let env_mutbl = match kind { - ty::FnOnceUnboxedClosureKind => var_mutbl, - ty::FnMutUnboxedClosureKind => McInherited, - ty::FnUnboxedClosureKind => McImmutable + // Construct information about env pointer dereference, if any + let mutbl = match kind { + ty::FnOnceUnboxedClosureKind => None, // None, env is by-value + ty::FnMutUnboxedClosureKind => match mode { // Depends on capture type + ast::CaptureByValue => Some(var_mutbl), // Mutable if the original var is + ast::CaptureByRef => Some(McDeclared) // Mutable regardless + }, + ty::FnUnboxedClosureKind => Some(McImmutable) // Never mutable }; + let env_info = mutbl.map(|env_mutbl| { + // Look up the node ID of the closure body so we can construct + // a free region within it + let fn_body_id = { + let fn_expr = match self.tcx().map.find(fn_node_id) { + Some(ast_map::NodeExpr(e)) => e, + _ => unreachable!() + }; - // Look up the node ID of the closure body so we can construct - // a free region within it - let fn_body_id = { - let fn_expr = match self.tcx().map.find(fn_node_id) { - Some(ast_map::NodeExpr(e)) => e, - _ => unreachable!() + match fn_expr.node { + ast::ExprFnBlock(_, _, ref body) | + ast::ExprProc(_, ref body) | + ast::ExprUnboxedFn(_, _, _, ref body) => body.id, + _ => unreachable!() + } }; - match fn_expr.node { - ast::ExprFnBlock(_, _, ref body) | - ast::ExprProc(_, ref body) | - ast::ExprUnboxedFn(_, _, _, ref body) => body.id, - _ => unreachable!() - } - }; + // Region of environment pointer + let env_region = ty::ReFree(ty::FreeRegion { + scope_id: fn_body_id, + bound_region: ty::BrEnv + }); - // Region of environment pointer - let env_region = ty::ReFree(ty::FreeRegion { - scope_id: fn_body_id, - bound_region: ty::BrEnv + let env_ptr = BorrowedPtr(if env_mutbl.is_mutable() { + ty::MutBorrow + } else { + ty::ImmBorrow + }, env_region); + + (env_mutbl, env_ptr) }); - let env_ptr = BorrowedPtr(if env_mutbl.is_mutable() { - ty::MutBorrow - } else { - ty::ImmBorrow - }, env_region); - - let var_ty = if_ok!(self.node_ty(var_id)); - // First, switch by capture mode Ok(match mode { ast::CaptureByValue => { @@ -717,25 +720,27 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> { note: NoteNone }; - if has_env_deref { - // We need to add the env deref. This means that - // the above is actually immutable and has a ref - // type. However, nothing should actually look at - // the type, so we can get away with stuffing a - // `ty_err` in there instead of bothering to - // construct a proper one. - base.mutbl = McImmutable; - base.ty = ty::mk_err(); - Rc::new(cmt_ { - id: id, - span: span, - cat: cat_deref(Rc::new(base), 0, env_ptr), - mutbl: env_mutbl, - ty: var_ty, - note: NoteClosureEnv(upvar_id) - }) - } else { - Rc::new(base) + match env_info { + Some((env_mutbl, env_ptr)) => { + // We need to add the env deref. This means + // that the above is actually immutable and + // has a ref type. However, nothing should + // actually look at the type, so we can get + // away with stuffing a `ty_err` in there + // instead of bothering to construct a proper + // one. + base.mutbl = McImmutable; + base.ty = ty::mk_err(); + Rc::new(cmt_ { + id: id, + span: span, + cat: cat_deref(Rc::new(base), 0, env_ptr), + mutbl: env_mutbl, + ty: var_ty, + note: NoteClosureEnv(upvar_id) + }) + } + None => Rc::new(base) } }, ast::CaptureByRef => { @@ -755,16 +760,18 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> { note: NoteNone }; - // As in the by-value case, add env deref if needed - if has_env_deref { - base = cmt_ { - id: id, - span: span, - cat: cat_deref(Rc::new(base), 0, env_ptr), - mutbl: env_mutbl, - ty: ty::mk_err(), - note: NoteClosureEnv(upvar_id) - }; + match env_info { + Some((env_mutbl, env_ptr)) => { + base = cmt_ { + id: id, + span: span, + cat: cat_deref(Rc::new(base), 0, env_ptr), + mutbl: env_mutbl, + ty: ty::mk_err(), + note: NoteClosureEnv(upvar_id) + }; + } + None => {} } // Look up upvar borrow so we can get its region diff --git a/src/test/compile-fail/unboxed-closure-immutable-capture.rs b/src/test/compile-fail/unboxed-closure-immutable-capture.rs new file mode 100644 index 00000000000..e28abaf2b1f --- /dev/null +++ b/src/test/compile-fail/unboxed-closure-immutable-capture.rs @@ -0,0 +1,31 @@ +// Copyright 2014 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(unboxed_closures)] + +// Test that even unboxed closures that are capable of mutating their +// environment cannot mutate captured variables that have not been +// declared mutable (#18335) + +fn set(x: &mut uint) { *x = 0; } + +fn main() { + let x = 0u; + move |&mut:| x = 1; //~ ERROR cannot assign + move |&mut:| set(&mut x); //~ ERROR cannot borrow + move |:| x = 1; //~ ERROR cannot assign + move |:| set(&mut x); //~ ERROR cannot borrow + |&mut:| x = 1; //~ ERROR cannot assign + // FIXME: this should be `cannot borrow` (issue #18330) + |&mut:| set(&mut x); //~ ERROR cannot assign + |:| x = 1; //~ ERROR cannot assign + // FIXME: this should be `cannot borrow` (issue #18330) + |:| set(&mut x); //~ ERROR cannot assign +} diff --git a/src/test/run-pass/unboxed-closures-move-mutable.rs b/src/test/run-pass/unboxed-closures-move-mutable.rs new file mode 100644 index 00000000000..f7e1e46e54d --- /dev/null +++ b/src/test/run-pass/unboxed-closures-move-mutable.rs @@ -0,0 +1,28 @@ +// Copyright 2014 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(unboxed_closures)] +#![deny(unused_mut)] + +// Test that mutating a mutable upvar in a capture-by-value unboxed +// closure does not ice (issue #18238) and marks the upvar as used +// mutably so we do not get a spurious warning about it not needing to +// be declared mutable (issue #18336). + +fn main() { + { + let mut x = 0u; + move |&mut:| x += 1; + } + { + let mut x = 0u; + move |:| x += 1; + } +}