auto merge of #14873 : pnkfelix/rust/fsk-dataflow-revisions, r=nikoma…

…tsakis

Fix #6298.  Fix  #13767.

This also includes some drive by fixes for some other issues, noted in the commits.

I still need to integrate regression tests for some cases that I noticed were missing from our unit test suite (i.e. things that compiling rustc exposes that should have been exposed when doing `make check-stage1`).  So do not land this yet, until I get the chance to add those tests.

I just wanted to get the review process started soon, since this has been long in the coming.

src/librustc/middle/borrowck/check_loans.rs

@@ -242,7 +242,7 @@ impl<'a> CheckLoanCtxt<'a> {
         let mut loan_path = loan_path;
         loop {
             match *loan_path {
-                LpVar(_) => {
+                LpVar(_) | LpUpvar(_) => {
                     break;
                 }
                 LpExtend(ref lp_base, _, _) => {
@@ -632,7 +632,7 @@ impl<'a> CheckLoanCtxt<'a> {
          */
 
         match **lp {
-            LpVar(_) => {
+            LpVar(_) | LpUpvar(_) => {
                 // assigning to `x` does not require that `x` is initialized
             }
             LpExtend(ref lp_base, _, LpInterior(_)) => {

src/librustc/middle/borrowck/doc.rs

@@ -948,7 +948,7 @@ The borrow checker is also in charge of ensuring that:
 
 These are two separate dataflow analyses built on the same
 framework. Let's look at checking that memory is initialized first;
-the checking of immutable local variabe assignments works in a very
+the checking of immutable local variable assignments works in a very
 similar way.
 
 To track the initialization of memory, we actually track all the

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

@@ -395,7 +395,8 @@ impl<'a> GatherLoanCtxt<'a> {
         //! from a local variable, mark the mutability decl as necessary.
 
         match *loan_path {
-            LpVar(local_id) => {
+            LpVar(local_id) |
+            LpUpvar(ty::UpvarId{ var_id: local_id, closure_expr_id: _ }) => {
                 self.tcx().used_mut_nodes.borrow_mut().insert(local_id);
             }
             LpExtend(ref base, mc::McInherited, _) => {
@@ -445,8 +446,8 @@ impl<'a> GatherLoanCtxt<'a> {
         //! with immutable `&` pointers, because borrows of such pointers
         //! do not require restrictions and hence do not cause a loan.
 
+        let lexical_scope = lp.kill_scope(self.bccx.tcx);
         let rm = &self.bccx.tcx.region_maps;
-        let lexical_scope = rm.var_scope(lp.node_id());
         if rm.is_subscope_of(lexical_scope, loan_scope) {
             lexical_scope
         } else {

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

@@ -67,13 +67,23 @@ impl<'a> RestrictionsContext<'a> {
             }
 
             mc::cat_local(local_id) |
-            mc::cat_arg(local_id) |
-            mc::cat_upvar(ty::UpvarId {var_id: local_id, ..}, _) => {
-                // R-Variable
+            mc::cat_arg(local_id) => {
+                // R-Variable, locally declared
                 let lp = Rc::new(LpVar(local_id));
                 SafeIf(lp.clone(), vec!(lp))
             }
 
+            mc::cat_upvar(upvar_id, _) => {
+                // R-Variable, captured into closure
+                let lp = Rc::new(LpUpvar(upvar_id));
+                SafeIf(lp.clone(), vec!(lp))
+            }
+
+            mc::cat_copied_upvar(..) => {
+                // FIXME(#2152) allow mutation of upvars
+                Safe
+            }
+
             mc::cat_downcast(cmt_base) => {
                 // When we borrow the interior of an enum, we have to
                 // ensure the enum itself is not mutated, because that
@@ -107,7 +117,6 @@ impl<'a> RestrictionsContext<'a> {
                 self.extend(result, cmt.mutbl, LpDeref(pk))
             }
 
-            mc::cat_copied_upvar(..) | // FIXME(#2152) allow mutation of upvars
             mc::cat_static_item(..) => {
                 Safe
             }

src/librustc/middle/borrowck/mod.rs

@@ -12,7 +12,9 @@
 
 #![allow(non_camel_case_types)]
 
+use middle::cfg;
 use middle::dataflow::DataFlowContext;
+use middle::dataflow::BitwiseOperator;
 use middle::dataflow::DataFlowOperator;
 use middle::def;
 use euv = middle::expr_use_visitor;
@@ -126,20 +128,28 @@ fn borrowck_fn(this: &mut BorrowckCtxt,
     let id_range = ast_util::compute_id_range_for_fn_body(fk, decl, body, sp, id);
     let (all_loans, move_data) =
         gather_loans::gather_loans_in_fn(this, decl, body);
+    let cfg = cfg::CFG::new(this.tcx, body);
+
     let mut loan_dfcx =
         DataFlowContext::new(this.tcx,
+                             "borrowck",
+                             Some(decl),
+                             &cfg,
                              LoanDataFlowOperator,
                              id_range,
                              all_loans.len());
     for (loan_idx, loan) in all_loans.iter().enumerate() {
         loan_dfcx.add_gen(loan.gen_scope, loan_idx);
         loan_dfcx.add_kill(loan.kill_scope, loan_idx);
     }
-    loan_dfcx.propagate(body);
+    loan_dfcx.add_kills_from_flow_exits(&cfg);
+    loan_dfcx.propagate(&cfg, body);
 
     let flowed_moves = move_data::FlowedMoveData::new(move_data,
                                                       this.tcx,
+                                                      &cfg,
                                                       id_range,
+                                                      decl,
                                                       body);
 
     check_loans::check_loans(this, &loan_dfcx, flowed_moves,
@@ -191,6 +201,7 @@ pub struct Loan {
 #[deriving(PartialEq, Eq, Hash)]
 pub enum LoanPath {
     LpVar(ast::NodeId),               // `x` in doc.rs
+    LpUpvar(ty::UpvarId),             // `x` captured by-value into closure
     LpExtend(Rc<LoanPath>, mc::MutabilityCategory, LoanPathElem)
 }
 
@@ -200,11 +211,25 @@ pub enum LoanPathElem {
     LpInterior(mc::InteriorKind) // `LV.f` in doc.rs
 }
 
+pub fn closure_to_block(closure_id: ast::NodeId,
+                    tcx: &ty::ctxt) -> ast::NodeId {
+    match tcx.map.get(closure_id) {
+        ast_map::NodeExpr(expr) => match expr.node {
+            ast::ExprProc(_decl, block) |
+            ast::ExprFnBlock(_decl, block) => { block.id }
+            _ => fail!("encountered non-closure id: {}", closure_id)
+        },
+        _ => fail!("encountered non-expr id: {}", closure_id)
+    }
+}
+
 impl LoanPath {
-    pub fn node_id(&self) -> ast::NodeId {
+    pub fn kill_scope(&self, tcx: &ty::ctxt) -> ast::NodeId {
         match *self {
-            LpVar(local_id) => local_id,
-            LpExtend(ref base, _, _) => base.node_id()
+            LpVar(local_id) => tcx.region_maps.var_scope(local_id),
+            LpUpvar(upvar_id) =>
+                closure_to_block(upvar_id.closure_expr_id, tcx),
+            LpExtend(ref base, _, _) => base.kill_scope(tcx),
         }
     }
 }
@@ -224,12 +249,18 @@ pub fn opt_loan_path(cmt: &mc::cmt) -> Option<Rc<LoanPath>> {
         }
 
         mc::cat_local(id) |
-        mc::cat_arg(id) |
-        mc::cat_copied_upvar(mc::CopiedUpvar { upvar_id: id, .. }) |
-        mc::cat_upvar(ty::UpvarId {var_id: id, ..}, _) => {
+        mc::cat_arg(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,
+                                               onceness: _,
+                                               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_deref(ref cmt_base, _, pk) => {
             opt_loan_path(cmt_base).map(|lp| {
                 Rc::new(LpExtend(lp, cmt.mutbl, LpDeref(pk)))
@@ -683,6 +714,7 @@ impl<'a> BorrowckCtxt<'a> {
                                    loan_path: &LoanPath,
                                    out: &mut String) {
         match *loan_path {
+            LpUpvar(ty::UpvarId{ var_id: id, closure_expr_id: _ }) |
             LpVar(id) => {
                 out.push_str(ty::local_var_name_str(self.tcx, id).get());
             }
@@ -724,7 +756,7 @@ impl<'a> BorrowckCtxt<'a> {
                 self.append_autoderefd_loan_path_to_str(&**lp_base, out)
             }
 
-            LpVar(..) | LpExtend(_, _, LpInterior(..)) => {
+            LpVar(..) | LpUpvar(..) | LpExtend(_, _, LpInterior(..)) => {
                 self.append_loan_path_to_str(loan_path, out)
             }
         }
@@ -753,15 +785,17 @@ fn is_statement_scope(tcx: &ty::ctxt, region: ty::Region) -> bool {
      }
 }
 
-impl DataFlowOperator for LoanDataFlowOperator {
+impl BitwiseOperator for LoanDataFlowOperator {
     #[inline]
-    fn initial_value(&self) -> bool {
-        false // no loans in scope by default
+    fn join(&self, succ: uint, pred: uint) -> uint {
+        succ | pred // loans from both preds are in scope
     }
+}
 
+impl DataFlowOperator for LoanDataFlowOperator {
     #[inline]
-    fn join(&self, succ: uint, pred: uint) -> uint {
-        succ | pred // loans from both preds are in scope
+    fn initial_value(&self) -> bool {
+        false // no loans in scope by default
     }
 }
 
@@ -784,6 +818,12 @@ impl Repr for LoanPath {
                 (format!("$({})", tcx.map.node_to_str(id))).to_string()
             }
 
+            &LpUpvar(ty::UpvarId{ var_id, closure_expr_id }) => {
+                let s = tcx.map.node_to_str(var_id);
+                let s = format!("$({} captured by id={})", s, closure_expr_id);
+                s.to_string()
+            }
+
             &LpExtend(ref lp, _, LpDeref(_)) => {
                 (format!("{}.*", lp.repr(tcx))).to_string()
             }

src/librustc/middle/borrowck/move_data.rs

@@ -20,7 +20,9 @@ use std::rc::Rc;
 use std::uint;
 use std::collections::{HashMap, HashSet};
 use middle::borrowck::*;
+use middle::cfg;
 use middle::dataflow::DataFlowContext;
+use middle::dataflow::BitwiseOperator;
 use middle::dataflow::DataFlowOperator;
 use euv = middle::expr_use_visitor;
 use middle::ty;
@@ -229,7 +231,7 @@ impl MoveData {
         }
 
         let index = match *lp {
-            LpVar(..) => {
+            LpVar(..) | LpUpvar(..) => {
                 let index = MovePathIndex(self.paths.borrow().len());
 
                 self.paths.borrow_mut().push(MovePath {
@@ -300,7 +302,7 @@ impl MoveData {
             }
             None => {
                 match **lp {
-                    LpVar(..) => { }
+                    LpVar(..) | LpUpvar(..) => { }
                     LpExtend(ref b, _, _) => {
                         self.add_existing_base_paths(b, result);
                     }
@@ -416,6 +418,11 @@ impl MoveData {
                     let path = *self.path_map.borrow().get(&path.loan_path);
                     self.kill_moves(path, kill_id, dfcx_moves);
                 }
+                LpUpvar(ty::UpvarId { var_id: _, closure_expr_id }) => {
+                    let kill_id = closure_to_block(closure_expr_id, tcx);
+                    let path = *self.path_map.borrow().get(&path.loan_path);
+                    self.kill_moves(path, kill_id, dfcx_moves);
+                }
                 LpExtend(..) => {}
             }
         }
@@ -428,6 +435,10 @@ impl MoveData {
                     let kill_id = tcx.region_maps.var_scope(id);
                     dfcx_assign.add_kill(kill_id, assignment_index);
                 }
+                LpUpvar(ty::UpvarId { var_id: _, closure_expr_id }) => {
+                    let kill_id = closure_to_block(closure_expr_id, tcx);
+                    dfcx_assign.add_kill(kill_id, assignment_index);
+                }
                 LpExtend(..) => {
                     tcx.sess.bug("var assignment for non var path");
                 }
@@ -499,22 +510,33 @@ impl MoveData {
 impl<'a> FlowedMoveData<'a> {
     pub fn new(move_data: MoveData,
                tcx: &'a ty::ctxt,
+               cfg: &'a cfg::CFG,
                id_range: ast_util::IdRange,
+               decl: &ast::FnDecl,
                body: &ast::Block)
                -> FlowedMoveData<'a> {
         let mut dfcx_moves =
             DataFlowContext::new(tcx,
+                                 "flowed_move_data_moves",
+                                 Some(decl),
+                                 cfg,
                                  MoveDataFlowOperator,
                                  id_range,
                                  move_data.moves.borrow().len());
         let mut dfcx_assign =
             DataFlowContext::new(tcx,
+                                 "flowed_move_data_assigns",
+                                 Some(decl),
+                                 cfg,
                                  AssignDataFlowOperator,
                                  id_range,
                                  move_data.var_assignments.borrow().len());
         move_data.add_gen_kills(tcx, &mut dfcx_moves, &mut dfcx_assign);
-        dfcx_moves.propagate(body);
-        dfcx_assign.propagate(body);
+        dfcx_moves.add_kills_from_flow_exits(cfg);
+        dfcx_assign.add_kills_from_flow_exits(cfg);
+        dfcx_moves.propagate(cfg, body);
+        dfcx_assign.propagate(cfg, body);
+
         FlowedMoveData {
             move_data: move_data,
             dfcx_moves: dfcx_moves,
@@ -659,12 +681,21 @@ impl<'a> FlowedMoveData<'a> {
     }
 }
 
+impl BitwiseOperator for MoveDataFlowOperator {
+    #[inline]
+    fn join(&self, succ: uint, pred: uint) -> uint {
+        succ | pred // moves from both preds are in scope
+    }
+}
+
 impl DataFlowOperator for MoveDataFlowOperator {
     #[inline]
     fn initial_value(&self) -> bool {
         false // no loans in scope by default
     }
+}
 
+impl BitwiseOperator for AssignDataFlowOperator {
     #[inline]
     fn join(&self, succ: uint, pred: uint) -> uint {
         succ | pred // moves from both preds are in scope
@@ -676,9 +707,4 @@ impl DataFlowOperator for AssignDataFlowOperator {
     fn initial_value(&self) -> bool {
         false // no assignments in scope by default
     }
-
-    #[inline]
-    fn join(&self, succ: uint, pred: uint) -> uint {
-        succ | pred // moves from both preds are in scope
-    }
 }