Auto merge of #24330 - pnkfelix:issue-24267, r=nikomatsakis

Extend rustc::middle::dataflow to allow filtering kills from flow-exits.

Fix borrowck analysis so that it will not treat a break that pops through an assignment
```rust
x = { ... break; ... }
```
as a kill of the "moved-out" bit for `x`.

Fix #24267.

[breaking-change], but really, its only breaking code that was already buggy.

4 files changed, 108 insertions, 23 deletions

diff --git a/src/librustc/middle/dataflow.rs b/src/librustc/middle/dataflow.rs
index b20e4c3f563..41b4495c5f0 100644
--- a/src/librustc/middle/dataflow.rs
+++ b/src/librustc/middle/dataflow.rs
@@ -64,8 +64,14 @@ pub struct DataFlowContext<'a, 'tcx: 'a, O> {
     /// bits generated as we exit the cfg node. Updated by `add_gen()`.
     gens: Vec<usize>,
 
-    /// bits killed as we exit the cfg node. Updated by `add_kill()`.
-    kills: Vec<usize>,
+    /// bits killed as we exit the cfg node, or non-locally jump over
+    /// it. Updated by `add_kill(KillFrom::ScopeEnd)`.
+    scope_kills: Vec<usize>,
+
+    /// bits killed as we exit the cfg node directly; if it is jumped
+    /// over, e.g. via `break`, the kills are not reflected in the
+    /// jump's effects. Updated by `add_kill(KillFrom::Execution)`.
+    action_kills: Vec<usize>,
 
     /// bits that are valid on entry to the cfg node. Updated by
     /// `propagate()`.
@@ -130,15 +136,23 @@ impl<'a, 'tcx, O:DataFlowOperator> pprust::PpAnn for DataFlowContext<'a, 'tcx, O
                 "".to_string()
             };
 
-            let kills = &self.kills[start .. end];
-            let kills_str = if kills.iter().any(|&u| u != 0) {
-                format!(" kill: {}", bits_to_string(kills))
+            let action_kills = &self.action_kills[start .. end];
+            let action_kills_str = if action_kills.iter().any(|&u| u != 0) {
+                format!(" action_kill: {}", bits_to_string(action_kills))
+            } else {
+                "".to_string()
+            };
+
+            let scope_kills = &self.scope_kills[start .. end];
+            let scope_kills_str = if scope_kills.iter().any(|&u| u != 0) {
+                format!(" scope_kill: {}", bits_to_string(scope_kills))
             } else {
                 "".to_string()
             };
 
-            try!(ps.synth_comment(format!("id {}: {}{}{}", id, entry_str,
-                                          gens_str, kills_str)));
+            try!(ps.synth_comment(
+                format!("id {}: {}{}{}{}", id, entry_str,
+                        gens_str, action_kills_str, scope_kills_str)));
             try!(pp::space(&mut ps.s));
         }
         Ok(())
@@ -187,6 +201,25 @@ fn build_nodeid_to_index(decl: Option<&ast::FnDecl>,
     }
 }
 
+/// Flag used by `add_kill` to indicate whether the provided kill
+/// takes effect only when control flows directly through the node in
+/// question, or if the kill's effect is associated with any
+/// control-flow directly through or indirectly over the node.
+#[derive(Copy, Clone, PartialEq, Debug)]
+pub enum KillFrom {
+    /// A `ScopeEnd` kill is one that takes effect when any control
+    /// flow goes over the node. A kill associated with the end of the
+    /// scope of a variable declaration `let x;` is an example of a
+    /// `ScopeEnd` kill.
+    ScopeEnd,
+
+    /// An `Execution` kill is one that takes effect only when control
+    /// flow goes through the node to completion. A kill associated
+    /// with an assignment statement `x = expr;` is an example of an
+    /// `Execution` kill.
+    Execution,
+}
+
 impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> {
     pub fn new(tcx: &'a ty::ctxt<'tcx>,
                analysis_name: &'static str,
@@ -206,8 +239,10 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> {
 
         let entry = if oper.initial_value() { usize::MAX } else {0};
 
-        let gens: Vec<_> = repeat(0).take(num_nodes * words_per_id).collect();
-        let kills: Vec<_> = repeat(0).take(num_nodes * words_per_id).collect();
+        let zeroes: Vec<_> = repeat(0).take(num_nodes * words_per_id).collect();
+        let gens: Vec<_> = zeroes.clone();
+        let kills1: Vec<_> = zeroes.clone();
+        let kills2: Vec<_> = zeroes;
         let on_entry: Vec<_> = repeat(entry).take(num_nodes * words_per_id).collect();
 
         let nodeid_to_index = build_nodeid_to_index(decl, cfg);
@@ -220,7 +255,8 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> {
             bits_per_id: bits_per_id,
             oper: oper,
             gens: gens,
-            kills: kills,
+            action_kills: kills1,
+            scope_kills: kills2,
             on_entry: on_entry
         }
     }
@@ -240,7 +276,7 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> {
         }
     }
 
-    pub fn add_kill(&mut self, id: ast::NodeId, bit: usize) {
+    pub fn add_kill(&mut self, kind: KillFrom, id: ast::NodeId, bit: usize) {
         //! Indicates that `id` kills `bit`
         debug!("{} add_kill(id={}, bit={})",
                self.analysis_name, id, bit);
@@ -250,7 +286,10 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> {
         let indices = get_cfg_indices(id, &self.nodeid_to_index);
         for &cfgidx in indices {
             let (start, end) = self.compute_id_range(cfgidx);
-            let kills = &mut self.kills[start.. end];
+            let kills = match kind {
+                KillFrom::Execution => &mut self.action_kills[start.. end],
+                KillFrom::ScopeEnd =>  &mut self.scope_kills[start.. end],
+            };
             set_bit(kills, bit);
         }
     }
@@ -264,7 +303,9 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> {
         let (start, end) = self.compute_id_range(cfgidx);
         let gens = &self.gens[start.. end];
         bitwise(bits, gens, &Union);
-        let kills = &self.kills[start.. end];
+        let kills = &self.action_kills[start.. end];
+        bitwise(bits, kills, &Subtract);
+        let kills = &self.scope_kills[start.. end];
         bitwise(bits, kills, &Subtract);
 
         debug!("{} apply_gen_kill(cfgidx={:?}, bits={}) [after]",
@@ -278,7 +319,8 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> {
 
         assert!(start < self.gens.len());
         assert!(end <= self.gens.len());
-        assert!(self.gens.len() == self.kills.len());
+        assert!(self.gens.len() == self.action_kills.len());
+        assert!(self.gens.len() == self.scope_kills.len());
         assert!(self.gens.len() == self.on_entry.len());
 
         (start, end)
@@ -412,7 +454,7 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> {
         cfg.graph.each_edge(|_edge_index, edge| {
             let flow_exit = edge.source();
             let (start, end) = self.compute_id_range(flow_exit);
-            let mut orig_kills = self.kills[start.. end].to_vec();
+            let mut orig_kills = self.scope_kills[start.. end].to_vec();
 
             let mut changed = false;
             for &node_id in &edge.data.exiting_scopes {
@@ -421,8 +463,12 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> {
                     Some(indices) => {
                         for &cfg_idx in indices {
                             let (start, end) = self.compute_id_range(cfg_idx);
-                            let kills = &self.kills[start.. end];
+                            let kills = &self.scope_kills[start.. end];
                             if bitwise(&mut orig_kills, kills, &Union) {
+                                debug!("scope exits: scope id={} \
+                                        (node={:?} of {:?}) added killset: {}",
+                                       node_id, cfg_idx, indices,
+                                       bits_to_string(kills));
                                 changed = true;
                             }
                         }
@@ -436,7 +482,7 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> {
             }
 
             if changed {
-                let bits = &mut self.kills[start.. end];
+                let bits = &mut self.scope_kills[start.. end];
                 debug!("{} add_kills_from_flow_exits flow_exit={:?} bits={} [before]",
                        self.analysis_name, flow_exit, mut_bits_to_string(bits));
                 bits.clone_from_slice(&orig_kills[..]);
diff --git a/src/librustc_borrowck/borrowck/mod.rs b/src/librustc_borrowck/borrowck/mod.rs
index db947a27472..502321d0759 100644
--- a/src/librustc_borrowck/borrowck/mod.rs
+++ b/src/librustc_borrowck/borrowck/mod.rs
@@ -24,6 +24,7 @@ use rustc::middle::cfg;
 use rustc::middle::dataflow::DataFlowContext;
 use rustc::middle::dataflow::BitwiseOperator;
 use rustc::middle::dataflow::DataFlowOperator;
+use rustc::middle::dataflow::KillFrom;
 use rustc::middle::expr_use_visitor as euv;
 use rustc::middle::mem_categorization as mc;
 use rustc::middle::region;
@@ -167,7 +168,7 @@ fn build_borrowck_dataflow_data<'a, 'tcx>(this: &mut BorrowckCtxt<'a, 'tcx>,
                              all_loans.len());
     for (loan_idx, loan) in all_loans.iter().enumerate() {
         loan_dfcx.add_gen(loan.gen_scope.node_id(), loan_idx);
-        loan_dfcx.add_kill(loan.kill_scope.node_id(), loan_idx);
+        loan_dfcx.add_kill(KillFrom::ScopeEnd, loan.kill_scope.node_id(), loan_idx);
     }
     loan_dfcx.add_kills_from_flow_exits(cfg);
     loan_dfcx.propagate(cfg, body);
diff --git a/src/librustc_borrowck/borrowck/move_data.rs b/src/librustc_borrowck/borrowck/move_data.rs
index 2d1b57243d1..1180717140e 100644
--- a/src/librustc_borrowck/borrowck/move_data.rs
+++ b/src/librustc_borrowck/borrowck/move_data.rs
@@ -18,6 +18,7 @@ use rustc::middle::cfg;
 use rustc::middle::dataflow::DataFlowContext;
 use rustc::middle::dataflow::BitwiseOperator;
 use rustc::middle::dataflow::DataFlowOperator;
+use rustc::middle::dataflow::KillFrom;
 use rustc::middle::expr_use_visitor as euv;
 use rustc::middle::ty;
 use rustc::util::nodemap::{FnvHashMap, NodeSet};
@@ -473,11 +474,13 @@ impl<'tcx> MoveData<'tcx> {
 
         for (i, assignment) in self.var_assignments.borrow().iter().enumerate() {
             dfcx_assign.add_gen(assignment.id, i);
-            self.kill_moves(assignment.path, assignment.id, dfcx_moves);
+            self.kill_moves(assignment.path, assignment.id,
+                            KillFrom::Execution, dfcx_moves);
         }
 
         for assignment in &*self.path_assignments.borrow() {
-            self.kill_moves(assignment.path, assignment.id, dfcx_moves);
+            self.kill_moves(assignment.path, assignment.id,
+                            KillFrom::Execution, dfcx_moves);
         }
 
         // Kill all moves related to a variable `x` when
@@ -487,7 +490,8 @@ impl<'tcx> MoveData<'tcx> {
                 LpVar(..) | LpUpvar(..) | LpDowncast(..) => {
                     let kill_scope = path.loan_path.kill_scope(tcx);
                     let path = *self.path_map.borrow().get(&path.loan_path).unwrap();
-                    self.kill_moves(path, kill_scope.node_id(), dfcx_moves);
+                    self.kill_moves(path, kill_scope.node_id(),
+                                    KillFrom::ScopeEnd, dfcx_moves);
                 }
                 LpExtend(..) => {}
             }
@@ -500,7 +504,9 @@ impl<'tcx> MoveData<'tcx> {
             match lp.kind {
                 LpVar(..) | LpUpvar(..) | LpDowncast(..) => {
                     let kill_scope = lp.kill_scope(tcx);
-                    dfcx_assign.add_kill(kill_scope.node_id(), assignment_index);
+                    dfcx_assign.add_kill(KillFrom::ScopeEnd,
+                                         kill_scope.node_id(),
+                                         assignment_index);
                 }
                 LpExtend(..) => {
                     tcx.sess.bug("var assignment for non var path");
@@ -568,6 +574,7 @@ impl<'tcx> MoveData<'tcx> {
     fn kill_moves(&self,
                   path: MovePathIndex,
                   kill_id: ast::NodeId,
+                  kill_kind: KillFrom,
                   dfcx_moves: &mut MoveDataFlow) {
         // We can only perform kills for paths that refer to a unique location,
         // since otherwise we may kill a move from one location with an
@@ -576,7 +583,9 @@ impl<'tcx> MoveData<'tcx> {
         let loan_path = self.path_loan_path(path);
         if loan_path_is_precise(&*loan_path) {
             self.each_applicable_move(path, |move_index| {
-                dfcx_moves.add_kill(kill_id, move_index.get());
+                debug!("kill_moves add_kill {:?} kill_id={} move_index={}",
+                       kill_kind, kill_id, move_index.get());
+                dfcx_moves.add_kill(kill_kind, kill_id, move_index.get());
                 true
             });
         }
diff --git a/src/test/compile-fail/issue-24267-flow-exit.rs b/src/test/compile-fail/issue-24267-flow-exit.rs
new file mode 100644
index 00000000000..4aca6bf38e1
--- /dev/null
+++ b/src/test/compile-fail/issue-24267-flow-exit.rs
@@ -0,0 +1,29 @@
+// Copyright 2015 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.
+
+// Ensure that we reject code when a nonlocal exit (`break`,
+// `continue`) causes us to pop over a needed assignment.
+
+pub fn main() {
+    foo1();
+    foo2();
+}
+
+pub fn foo1() {
+    let x: i32;
+    loop { x = break; }
+    println!("{}", x); //~ ERROR use of possibly uninitialized variable: `x`
+}
+
+pub fn foo2() {
+    let x: i32;
+    for _ in 0..10 { x = continue; }
+    println!("{}", x); //~ ERROR use of possibly uninitialized variable: `x`
+}