Auto merge of #24527 - nikomatsakis:issue-24085, r=nikomatsakis

Expand the "givens" set to cover transitive relations.  The givens array
stores relationships like `'c <= '0` (where `'c` is a free region and
`'0` is an inference variable) that are derived from closure
arguments. These are (rather hackily) ignored for purposes of inference,
preventing spurious errors. The current code did not handle transitive
cases like `'c <= '0` and `'0 <= '1`. Fixes #24085.

r? @pnkfelix 
cc @bkoropoff

*But* I am not sure whether this fix will have a compile-time hit. I'd like to push to try branch observe cycle times.

Author: bors

src/librustc/middle/cfg/mod.rs

@@ -63,6 +63,7 @@ impl CFG {
     }
 
     pub fn node_is_reachable(&self, id: ast::NodeId) -> bool {
-        self.graph.depth_traverse(self.entry).any(|node| node.id() == id)
+        self.graph.depth_traverse(self.entry)
+                  .any(|idx| self.graph.node_data(idx).id() == id)
     }
 }

src/librustc/middle/infer/region_inference/mod.rs

@@ -984,14 +984,18 @@ impl<'a, 'tcx> RegionVarBindings<'a, 'tcx> {
 
         // Dorky hack to cause `dump_constraints` to only get called
         // if debug mode is enabled:
-        debug!("----() End constraint listing {:?}---", self.dump_constraints());
+        debug!("----() End constraint listing (subject={}) {:?}---",
+               subject, self.dump_constraints(subject));
         graphviz::maybe_print_constraints_for(self, subject);
 
+        let graph = self.construct_graph();
+        self.expand_givens(&graph);
         self.expansion(free_regions, &mut var_data);
         self.contraction(free_regions, &mut var_data);
         let values =
             self.extract_values_and_collect_conflicts(free_regions,
-                                                      &var_data[..],
+                                                      &var_data,
+                                                      &graph,
                                                       errors);
         self.collect_concrete_region_errors(free_regions, &values, errors);
         values
@@ -1010,13 +1014,38 @@ impl<'a, 'tcx> RegionVarBindings<'a, 'tcx> {
         }).collect()
     }
 
-    fn dump_constraints(&self) {
-        debug!("----() Start constraint listing ()----");
+    fn dump_constraints(&self, subject: ast::NodeId) {
+        debug!("----() Start constraint listing (subject={}) ()----", subject);
         for (idx, (constraint, _)) in self.constraints.borrow().iter().enumerate() {
             debug!("Constraint {} => {:?}", idx, constraint);
         }
     }
 
+    fn expand_givens(&self, graph: &RegionGraph) {
+        // Givens are a kind of horrible hack to account for
+        // constraints like 'c <= '0 that are known to hold due to
+        // closure signatures (see the comment above on the `givens`
+        // field). They should go away. But until they do, the role
+        // of this fn is to account for the transitive nature:
+        //
+        //     Given 'c <= '0
+        //     and   '0 <= '1
+        //     then  'c <= '1
+
+        let mut givens = self.givens.borrow_mut();
+        let seeds: Vec<_> = givens.iter().cloned().collect();
+        for (fr, vid) in seeds {
+            let seed_index = NodeIndex(vid.index as usize);
+            for succ_index in graph.depth_traverse(seed_index) {
+                let succ_index = succ_index.0 as u32;
+                if succ_index < self.num_vars() {
+                    let succ_vid = RegionVid { index: succ_index };
+                    givens.insert((fr, succ_vid));
+                }
+            }
+        }
+    }
+
     fn expansion(&self, free_regions: &FreeRegionMap, var_data: &mut [VarData]) {
         self.iterate_until_fixed_point("Expansion", |constraint| {
             debug!("expansion: constraint={:?} origin={:?}",
@@ -1258,6 +1287,7 @@ impl<'a, 'tcx> RegionVarBindings<'a, 'tcx> {
         &self,
         free_regions: &FreeRegionMap,
         var_data: &[VarData],
+        graph: &RegionGraph,
         errors: &mut Vec<RegionResolutionError<'tcx>>)
         -> Vec<VarValue>
     {
@@ -1276,8 +1306,6 @@ impl<'a, 'tcx> RegionVarBindings<'a, 'tcx> {
         // overlapping locations.
         let mut dup_vec: Vec<_> = repeat(u32::MAX).take(self.num_vars() as usize).collect();
 
-        let mut opt_graph = None;
-
         for idx in 0..self.num_vars() as usize {
             match var_data[idx].value {
                 Value(_) => {
@@ -1313,11 +1341,6 @@ impl<'a, 'tcx> RegionVarBindings<'a, 'tcx> {
                        starts to create problems we'll have to revisit
                        this portion of the code and think hard about it. =) */
 
-                    if opt_graph.is_none() {
-                        opt_graph = Some(self.construct_graph());
-                    }
-                    let graph = opt_graph.as_ref().unwrap();
-
                     let node_vid = RegionVid { index: idx as u32 };
                     match var_data[idx].classification {
                         Expanding => {

src/librustc_data_structures/graph/mod.rs

@@ -358,9 +358,9 @@ pub struct DepthFirstTraversal<'g, N:'g, E:'g> {
 }
 
 impl<'g, N:Debug, E:Debug> Iterator for DepthFirstTraversal<'g, N, E> {
-    type Item = &'g N;
+    type Item = NodeIndex;
 
-    fn next(&mut self) -> Option<&'g N> {
+    fn next(&mut self) -> Option<NodeIndex> {
         while let Some(idx) = self.stack.pop() {
             if !self.visited.insert(idx.node_id()) {
                 continue;
@@ -372,7 +372,7 @@ impl<'g, N:Debug, E:Debug> Iterator for DepthFirstTraversal<'g, N, E> {
                 }
             }
 
-            return Some(self.graph.node_data(idx));
+            return Some(idx);
         }
 
         return None;

src/librustc_trans/trans/base.rs

@@ -1142,7 +1142,8 @@ fn build_cfg(tcx: &ty::ctxt, id: ast::NodeId) -> (ast::NodeId, Option<cfg::CFG>)
 // return slot alloca. This can cause errors related to clean-up due to
 // the clobbering of the existing value in the return slot.
 fn has_nested_returns(tcx: &ty::ctxt, cfg: &cfg::CFG, blk_id: ast::NodeId) -> bool {
-    for n in cfg.graph.depth_traverse(cfg.entry) {
+    for index in cfg.graph.depth_traverse(cfg.entry) {
+        let n = cfg.graph.node_data(index);
         match tcx.map.find(n.id()) {
             Some(ast_map::NodeExpr(ex)) => {
                 if let ast::ExprRet(Some(ref ret_expr)) = ex.node {

src/test/run-pass/issue-24085.rs

@@ -0,0 +1,27 @@
+// 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.
+
+// Regression test for #24085. Errors were occuring in region
+// inference due to the requirement that `'a:b'`, which was getting
+// incorrectly translated in connection with the closure below.
+
+#[derive(Copy,Clone)]
+struct Path<'a:'b, 'b> {
+    x: &'a i32,
+    tail: Option<&'b Path<'a, 'b>>
+}
+
+#[allow(dead_code, unconditional_recursion)]
+fn foo<'a,'b,F>(p: Path<'a, 'b>, mut f: F)
+                where F: for<'c> FnMut(Path<'a, 'c>) {
+    foo(p, |x| f(x))
+}
+
+fn main() { }