Assorted unboxed closure fixes

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_discr(b, _) |
                     mc::cat_deref(b, _, mc::OwnedPtr) => {
                         assert_eq!(cmt.mutbl, mc::McInherited);

src/librustc/middle/borrowck/mod.rs

@@ -626,7 +626,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) {
@@ -762,11 +762,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");
+                        }
                     }
                     _ => {}
                 }

src/librustc/middle/mem_categorization.rs

@@ -656,51 +656,54 @@ 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);
+            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));
+            (env_mutbl, env_ptr)
+        });
 
         // First, switch by capture mode
         Ok(match mode {
@@ -718,25 +721,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 => {
@@ -756,16 +761,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

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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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
+}

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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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;
+    }
+}