auto merge of #17869 : bkoropoff/rust/bound-all-the-upvars, r=nikomat…

…sakis

This PR is based on #17784, which fixes closure soundness problems in borrowck.  Only the last two commits are unique to this PR.

My understanding of regionck is still evolving, so I'm not sure if this is the right approach.  Feedback is appreciated.

- In `link_reborrowed_region`, we 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 mutability of the borrow
  is adjusted.
- When asked to establish a region link for an upvar, we link it to
  the region of the closure body.  This creates the necessary
  constraint to stop unsound reborrows from the closure environment.

This partially (maybe completely) solves issue #17403.  Remaining work:

- This is only known to help with by-ref upvars.  I have not looked at
  by-value upvars yet to see if they can cause problems.
- The error diagnostics that result from failed region inference are
  pretty inscrutible.

src/librustc/metadata/tydecode.rs

@@ -277,6 +277,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

@@ -73,15 +73,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) => {
@@ -630,17 +621,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))
+                        }
                     }
                 };
 
@@ -732,8 +728,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");
             }
@@ -760,7 +756,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!(),
             }