Fix soundness bug in treatment of closure upvars by regionck

- Unify the representations of `cat_upvar` and `cat_copied_upvar`
- In `link_reborrowed_region`, account for the ability of upvars to
  change their mutability due to later processing.  A map of recursive
  region links we may want to establish in the future is maintained,
  with the links being established when the kind of the borrow is
  adjusted.
- When categorizing upvars, add an explicit deref that represents the
  closure environment pointer for closures that do not take the
  environment by value.  The region for the implicit pointer is an
  anonymous free region type introduced for this purpose.  This
  creates the necessary constraint to prevent unsound reborrows from
  the environment.
- Add a note to categorizations to make it easier to tell when extra
  dereferences have been inserted by an upvar without having to
  perform deep pattern matching.
- Adjust borrowck to deal with the changes.  Where `cat_upvar` and
  `cat_copied_upvar` were previously treated differently, they are
  now both treated roughly like local variables within the closure
  body, as the explicit derefs now ensure proper behavior.  However,
  error diagnostics had to be changed to explicitly look through the
  extra dereferences to avoid producing confusing messages about
  references not present in the source code.

Closes issue #17403.  Remaining work:

- The error diagnostics that result from failed region inference are
  pretty inscrutible and should be improved.

Code like the following is now rejected:

    let mut x = 0u;
    let f = || &mut x;
    let y = f();
    let z = f(); // multiple mutable references to the same location

This also breaks code that uses a similar construction even if it does
not go on to violate aliasability semantics.  Such code will need to
be reworked in some way, such as by using a capture-by-value closure
type.

[breaking-change]

Author: bkoropoff

src/librustc/metadata/tydecode.rs

@@ -276,6 +276,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")
     }
 }

src/librustc/metadata/tyencode.rs

@@ -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|");
+        }
     }
 }
 

src/librustc/middle/astencode.rs

@@ -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),
         }

src/librustc/middle/borrowck/check_loans.rs

@@ -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;
@@ -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(..)) |
@@ -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());
-                }
-
                 _ => {}
             }
 

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

@@ -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
         }
 

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

@@ -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
@@ -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 => {

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

@@ -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 {}",

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

@@ -72,15 +72,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])
             }
 

src/librustc/middle/borrowck/mod.rs

@@ -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) => {
@@ -629,17 +620,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))
+                        }
                     }
                 };
 
@@ -731,8 +727,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");
             }
@@ -759,7 +755,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(

src/librustc/middle/check_static.rs

@@ -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,
             }
@@ -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!(),
             }