auto merge of #17784 : bkoropoff/rust/issue-17780, r=pcwalton

This fixes a soundness problem where `Fn` unboxed closures can mutate free variables in the environment.
The following presently builds:

```rust
#![feature(unboxed_closures, overloaded_calls)]

fn main() {
    let mut x = 0u;
    let _f = |&:| x = 42;
}
```

However, this is equivalent to writing the following, which borrowck rightly rejects:

```rust
struct F<'a> {
    x: &'a mut uint
}

impl<'a> Fn<(),()> for F<'a> {
    #[rust_call_abi_hack]
    fn call(&self, _: ()) {
        *self.x = 42; // error: cannot assign to data in a `&` reference
    }
}

fn main() {
    let mut x = 0u;
    let _f = F { x: &mut x };
}
```

This problem is unique to unboxed closures; boxed closures cannot be invoked through an immutable reference and are not subject to it.

This change marks upvars of `Fn` unboxed closures as freely aliasable in mem_categorization, which causes borrowck to reject attempts to mutate or mutably borrow them.

@zwarich pointed out that even with this change, there are remaining soundness issues related to regionck (issue #17403).  This region issue affects boxed closures as well.

Closes issue #17780

Author: bors

src/librustc/middle/borrowck/check_loans.rs

@@ -854,6 +854,12 @@ 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());
+                }
+
                 _ => {}
             }
 

src/librustc/middle/borrowck/gather_loans/gather_moves.rs

@@ -133,16 +133,15 @@ 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_copied_upvar(mc::CopiedUpvar { onceness: ast::Many, .. }) => {
+        mc::cat_upvar(..) | mc::cat_static_item => {
             Some(cmt.clone())
         }
 
-        // Can move out of captured upvars only if the destination closure
-        // type is 'once'. 1-shot stack closures emit the copied_upvar form
-        // (see mem_categorization.rs).
-        mc::cat_copied_upvar(mc::CopiedUpvar { onceness: ast::Once, .. }) => {
-            None
+        mc::cat_copied_upvar(mc::CopiedUpvar { kind: kind, .. }) => {
+            match kind.onceness() {
+                ast::Once => None,
+                ast::Many => Some(cmt.clone())
+            }
         }
 
         mc::cat_rvalue(..) |

src/librustc/middle/borrowck/gather_loans/move_error.rs

@@ -115,8 +115,15 @@ 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 |
-        mc::cat_copied_upvar(mc::CopiedUpvar { onceness: ast::Many, .. }) => {
+        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 => {
             bccx.span_err(
                 move_from.span,
                 format!("cannot move out of {}",

src/librustc/middle/borrowck/gather_loans/restrictions.rs

@@ -72,7 +72,7 @@ impl<'a, 'tcx> RestrictionsContext<'a, 'tcx> {
                 SafeIf(lp.clone(), vec![lp])
             }
 
-            mc::cat_upvar(upvar_id, _) => {
+            mc::cat_upvar(upvar_id, _, _) => {
                 // R-Variable, captured into closure
                 let lp = Rc::new(LpUpvar(upvar_id));
                 SafeIf(lp.clone(), vec![lp])

src/librustc/middle/borrowck/mod.rs

@@ -354,18 +354,22 @@ pub fn opt_loan_path(cmt: &mc::cmt) -> Option<Rc<LoanPath>> {
 
     match cmt.cat {
         mc::cat_rvalue(..) |
-        mc::cat_static_item |
-        mc::cat_copied_upvar(mc::CopiedUpvar { onceness: ast::Many, .. }) => {
+        mc::cat_static_item => {
+            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_upvar(ty::UpvarId {var_id: id, closure_expr_id: proc_id}, _, _) |
         mc::cat_copied_upvar(mc::CopiedUpvar { upvar_id: id,
-                                               onceness: _,
+                                               kind: _,
                                                capturing_proc: proc_id }) => {
             let upvar_id = ty::UpvarId{ var_id: id, closure_expr_id: proc_id };
             Some(Rc::new(LpUpvar(upvar_id)))
@@ -724,6 +728,13 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
                     format!("{} in an aliasable location",
                              prefix).as_slice());
             }
+            mc::AliasableClosure(id) => {
+                self.tcx.sess.span_err(span,
+                                       format!("{} in a free variable from an \
+                                               immutable unboxed closure", prefix).as_slice());
+                span_note!(self.tcx.sess, self.tcx.map.span(id),
+                           "consider changing this closure to take self by mutable reference");
+            }
             mc::AliasableStatic(..) |
             mc::AliasableStaticMut(..) => {
                 self.tcx.sess.span_err(

src/librustc/middle/mem_categorization.rs

@@ -83,7 +83,8 @@ pub enum categorization {
     cat_rvalue(ty::Region),            // temporary val, argument is its scope
     cat_static_item,
     cat_copied_upvar(CopiedUpvar),     // upvar copied into proc env
-    cat_upvar(ty::UpvarId, ty::UpvarBorrow), // by ref upvar from stack closure
+    cat_upvar(ty::UpvarId, ty::UpvarBorrow,
+              Option<ty::UnboxedClosureKind>), // by ref upvar from stack or unboxed closure
     cat_local(ast::NodeId),            // local variable
     cat_deref(cmt, uint, PointerKind), // deref of a ptr
     cat_interior(cmt, InteriorKind),   // something interior: field, tuple, etc
@@ -93,10 +94,27 @@ pub enum categorization {
     // (*1) downcast is only required if the enum has more than one variant
 }
 
+#[deriving(Clone, PartialEq)]
+pub enum CopiedUpvarKind {
+    Boxed(ast::Onceness),
+    Unboxed(ty::UnboxedClosureKind)
+}
+
+impl CopiedUpvarKind {
+    pub fn onceness(&self) -> ast::Onceness {
+        match *self {
+            Boxed(onceness) => onceness,
+            Unboxed(ty::FnUnboxedClosureKind) |
+            Unboxed(ty::FnMutUnboxedClosureKind) => ast::Many,
+            Unboxed(ty::FnOnceUnboxedClosureKind) => ast::Once
+        }
+    }
+}
+
 #[deriving(Clone, PartialEq)]
 pub struct CopiedUpvar {
     pub upvar_id: ast::NodeId,
-    pub onceness: ast::Onceness,
+    pub kind: CopiedUpvarKind,
     pub capturing_proc: ast::NodeId,
 }
 
@@ -571,14 +589,14 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> {
 
                       };
                       if var_is_refd {
-                          self.cat_upvar(id, span, var_id, fn_node_id)
+                          self.cat_upvar(id, span, var_id, fn_node_id, None)
                       } else {
                           Ok(Rc::new(cmt_ {
                               id:id,
                               span:span,
                               cat:cat_copied_upvar(CopiedUpvar {
                                   upvar_id: var_id,
-                                  onceness: closure_ty.onceness,
+                                  kind: Boxed(closure_ty.onceness),
                                   capturing_proc: fn_node_id,
                               }),
                               mutbl: MutabilityCategory::from_local(self.tcx(), var_id),
@@ -591,20 +609,15 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> {
                                                  .unboxed_closures()
                                                  .borrow();
                       let kind = unboxed_closures.get(&closure_id).kind;
-                      let onceness = match kind {
-                          ty::FnUnboxedClosureKind |
-                          ty::FnMutUnboxedClosureKind => ast::Many,
-                          ty::FnOnceUnboxedClosureKind => ast::Once,
-                      };
                       if self.typer.capture_mode(fn_node_id) == ast::CaptureByRef {
-                          self.cat_upvar(id, span, var_id, fn_node_id)
+                          self.cat_upvar(id, span, var_id, fn_node_id, Some(kind))
                       } else {
                           Ok(Rc::new(cmt_ {
                               id: id,
                               span: span,
                               cat: cat_copied_upvar(CopiedUpvar {
                                   upvar_id: var_id,
-                                  onceness: onceness,
+                                  kind: Unboxed(kind),
                                   capturing_proc: fn_node_id,
                               }),
                               mutbl: MutabilityCategory::from_local(self.tcx(), var_id),
@@ -638,7 +651,8 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> {
                  id: ast::NodeId,
                  span: Span,
                  var_id: ast::NodeId,
-                 fn_node_id: ast::NodeId)
+                 fn_node_id: ast::NodeId,
+                 kind: Option<ty::UnboxedClosureKind>)
                  -> McResult<cmt> {
         /*!
          * Upvars through a closure are in fact indirect
@@ -666,7 +680,7 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> {
         let base_cmt = Rc::new(cmt_ {
             id:id,
             span:span,
-            cat:cat_upvar(upvar_id, upvar_borrow),
+            cat:cat_upvar(upvar_id, upvar_borrow, kind),
             mutbl:McImmutable,
             ty:upvar_ty,
         });
@@ -1233,6 +1247,7 @@ pub enum InteriorSafety {
 
 pub enum AliasableReason {
     AliasableBorrowed,
+    AliasableClosure(ast::NodeId), // Aliasable due to capture by unboxed closure expr
     AliasableOther,
     AliasableStatic(InteriorSafety),
     AliasableStaticMut(InteriorSafety),
@@ -1287,18 +1302,29 @@ impl cmt_ {
                 b.freely_aliasable(ctxt)
             }
 
-            cat_copied_upvar(CopiedUpvar {onceness: ast::Once, ..}) |
             cat_rvalue(..) |
             cat_local(..) |
-            cat_upvar(..) |
             cat_deref(_, _, UnsafePtr(..)) => { // yes, it's aliasable, but...
                 None
             }
 
-            cat_copied_upvar(CopiedUpvar {onceness: ast::Many, ..}) => {
-                Some(AliasableOther)
+            cat_copied_upvar(CopiedUpvar {kind: kind, capturing_proc: id, ..}) => {
+                match kind {
+                    Boxed(ast::Once) |
+                    Unboxed(ty::FnOnceUnboxedClosureKind) |
+                    Unboxed(ty::FnMutUnboxedClosureKind) => None,
+                    Boxed(_) => Some(AliasableOther),
+                    Unboxed(_) => Some(AliasableClosure(id))
+                }
+            }
+
+            cat_upvar(ty::UpvarId { closure_expr_id: id, .. }, _,
+                      Some(ty::FnUnboxedClosureKind)) => {
+                Some(AliasableClosure(id))
             }
 
+            cat_upvar(..) => None,
+
             cat_static_item(..) => {
                 let int_safe = if ty::type_interior_is_unsafe(ctxt, self.ty) {
                     InteriorUnsafe

src/librustc/middle/typeck/check/regionck.rs

@@ -1552,7 +1552,7 @@ fn link_reborrowed_region(rcx: &Rcx,
 
     // Detect references to an upvar `x`:
     let cause = match ref_cmt.cat {
-        mc::cat_upvar(ref upvar_id, _) => {
+        mc::cat_upvar(ref upvar_id, _, _) => {
             let mut upvar_borrow_map =
                 rcx.fcx.inh.upvar_borrow_map.borrow_mut();
             match upvar_borrow_map.find_mut(upvar_id) {
@@ -1686,7 +1686,7 @@ fn adjust_upvar_borrow_kind_for_mut(rcx: &Rcx,
             mc::cat_deref(base, _, mc::BorrowedPtr(..)) |
             mc::cat_deref(base, _, mc::Implicit(..)) => {
                 match base.cat {
-                    mc::cat_upvar(ref upvar_id, _) => {
+                    mc::cat_upvar(ref upvar_id, _, _) => {
                         // if this is an implicit deref of an
                         // upvar, then we need to modify the
                         // borrow_kind of the upvar to make sure it
@@ -1739,7 +1739,7 @@ fn adjust_upvar_borrow_kind_for_unique(rcx: &Rcx, cmt: mc::cmt) {
             mc::cat_deref(base, _, mc::BorrowedPtr(..)) |
             mc::cat_deref(base, _, mc::Implicit(..)) => {
                 match base.cat {
-                    mc::cat_upvar(ref upvar_id, _) => {
+                    mc::cat_upvar(ref upvar_id, _, _) => {
                         // if this is an implicit deref of an
                         // upvar, then we need to modify the
                         // borrow_kind of the upvar to make sure it

src/test/compile-fail/issue-17780.rs

@@ -0,0 +1,50 @@
+// 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, overloaded_calls)]
+
+fn set(x: &mut uint) { *x = 5; }
+
+fn main() {
+    // By-ref captures
+    {
+        let mut x = 0u;
+        let _f = |&:| x = 42;
+        //~^ ERROR cannot assign to data in a free
+        // variable from an immutable unboxed closure
+
+        let mut y = 0u;
+        let _g = |&:| set(&mut y);
+        //~^ ERROR cannot borrow data mutably in a free
+        // variable from an immutable unboxed closure
+
+        let mut z = 0u;
+        let _h = |&mut:| { set(&mut z); |&:| z = 42; };
+        //~^ ERROR cannot assign to data in a
+        // free variable from an immutable unboxed closure
+    }
+    // By-value captures
+    {
+        let mut x = 0u;
+        let _f = move |&:| x = 42;
+        //~^ ERROR cannot assign to data in a free
+        // variable from an immutable unboxed closure
+
+        let mut y = 0u;
+        let _g = move |&:| set(&mut y);
+        //~^ ERROR cannot borrow data mutably in a free
+        // variable from an immutable unboxed closure
+
+        let mut z = 0u;
+        let _h = move |&mut:| { set(&mut z); move |&:| z = 42; };
+        //~^ ERROR cannot assign to data in a free
+        // variable from an immutable unboxed closure
+    }
+}

src/test/run-pass/unboxed-closures-by-ref.rs

@@ -28,8 +28,8 @@ fn main() {
     let mut x = 0u;
     let y = 2u;
 
-    call_fn(|&:| x += y);
+    call_fn(|&:| assert_eq!(x, 0));
     call_fn_mut(|&mut:| x += y);
     call_fn_once(|:| x += y);
-    assert_eq!(x, y * 3);
+    assert_eq!(x, y * 2);
 }