Auto merge of #27892 - nikomatsakis:issue-27583, r=pnkfelix

Issue #27583 was caused by the fact that `LUB('a,'b)` yielded `'static`, even if there existed a region `'tcx:'a+'b`. This PR replaces the old very hacky code for computing how free regions relate to one another with something rather more robust. This solves the issue for #27583, though I think that similar bizarro bugs can no doubt arise in other ways -- the root of the problem is that the region-inference code was written in an era when a LUB always existed, but that hasn't held for some time. To *truly* solve this problem, it needs to be generalized to cope with that reality. But let's leave that battle for another day.

r? @aturon

Author: bors

src/librustc/middle/free_region.rs

@@ -8,26 +8,26 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-//! This file defines
-
+//! This file handles the relationships between free regions --
+//! meaning lifetime parameters. Ordinarily, free regions are
+//! unrelated to one another, but they can be related vai implied or
+//! explicit bounds.  In that case, we track the bounds using the
+//! `TransitiveRelation` type and use that to decide when one free
+//! region outlives another and so forth.
+
+use middle::ty::{self, FreeRegion, Region};
 use middle::wf::ImpliedBound;
-use middle::ty::{self, FreeRegion};
-use util::common::can_reach;
-use util::nodemap::{FnvHashMap, FnvHashSet};
+use rustc_data_structures::transitive_relation::TransitiveRelation;
 
 #[derive(Clone)]
 pub struct FreeRegionMap {
-    /// `map` maps from a free region `a` to a list of
-    /// free regions `bs` such that `a <= b for all b in bs`
-    map: FnvHashMap<FreeRegion, Vec<FreeRegion>>,
-    /// regions that are required to outlive (and therefore be
-    /// equal to) 'static.
-    statics: FnvHashSet<FreeRegion>
+    // Stores the relation `a < b`, where `a` and `b` are regions.
+    relation: TransitiveRelation<Region>
 }
 
 impl FreeRegionMap {
     pub fn new() -> FreeRegionMap {
-        FreeRegionMap { map: FnvHashMap(), statics: FnvHashSet() }
+        FreeRegionMap { relation: TransitiveRelation::new() }
     }
 
     pub fn relate_free_regions_from_implied_bounds<'tcx>(&mut self,
@@ -84,22 +84,38 @@ impl FreeRegionMap {
     }
 
     fn relate_to_static(&mut self, sup: FreeRegion) {
-        self.statics.insert(sup);
+        self.relation.add(ty::ReStatic, ty::ReFree(sup));
     }
 
     fn relate_free_regions(&mut self, sub: FreeRegion, sup: FreeRegion) {
-       let mut sups = self.map.entry(sub).or_insert(Vec::new());
-        if !sups.contains(&sup) {
-            sups.push(sup);
-        }
+        self.relation.add(ty::ReFree(sub), ty::ReFree(sup))
     }
 
     /// Determines whether two free regions have a subregion relationship
     /// by walking the graph encoded in `map`.  Note that
     /// it is possible that `sub != sup` and `sub <= sup` and `sup <= sub`
     /// (that is, the user can give two different names to the same lifetime).
     pub fn sub_free_region(&self, sub: FreeRegion, sup: FreeRegion) -> bool {
-        can_reach(&self.map, sub, sup) || self.is_static(&sup)
+        let result = sub == sup || {
+            let sub = ty::ReFree(sub);
+            let sup = ty::ReFree(sup);
+            self.relation.contains(&sub, &sup) || self.relation.contains(&ty::ReStatic, &sup)
+        };
+        debug!("sub_free_region(sub={:?}, sup={:?}) = {:?}", sub, sup, result);
+        result
+    }
+
+    pub fn lub_free_regions(&self, fr_a: FreeRegion, fr_b: FreeRegion) -> Region {
+        let r_a = ty::ReFree(fr_a);
+        let r_b = ty::ReFree(fr_b);
+        let result = if fr_a == fr_b { r_a } else {
+            match self.relation.postdom_upper_bound(&r_a, &r_b) {
+                None => ty::ReStatic,
+                Some(r) => *r,
+            }
+        };
+        debug!("lub_free_regions(fr_a={:?}, fr_b={:?}) = {:?}", fr_a, fr_b, result);
+        result
     }
 
     /// Determines whether one region is a subregion of another.  This is intended to run *after
@@ -109,10 +125,7 @@ impl FreeRegionMap {
                            sub_region: ty::Region,
                            super_region: ty::Region)
                            -> bool {
-        debug!("is_subregion_of(sub_region={:?}, super_region={:?})",
-               sub_region, super_region);
-
-        sub_region == super_region || {
+        let result = sub_region == super_region || {
             match (sub_region, super_region) {
                 (ty::ReEmpty, _) |
                 (_, ty::ReStatic) =>
@@ -121,23 +134,47 @@ impl FreeRegionMap {
                 (ty::ReScope(sub_scope), ty::ReScope(super_scope)) =>
                     tcx.region_maps.is_subscope_of(sub_scope, super_scope),
 
-                (ty::ReScope(sub_scope), ty::ReFree(ref fr)) =>
-                    tcx.region_maps.is_subscope_of(sub_scope, fr.scope.to_code_extent()),
+                (ty::ReScope(sub_scope), ty::ReFree(fr)) =>
+                    tcx.region_maps.is_subscope_of(sub_scope, fr.scope.to_code_extent()) ||
+                    self.is_static(fr),
 
                 (ty::ReFree(sub_fr), ty::ReFree(super_fr)) =>
                     self.sub_free_region(sub_fr, super_fr),
 
-                (ty::ReStatic, ty::ReFree(ref sup_fr)) => self.is_static(sup_fr),
+                (ty::ReStatic, ty::ReFree(sup_fr)) =>
+                    self.is_static(sup_fr),
 
                 _ =>
                     false,
             }
-        }
+        };
+        debug!("is_subregion_of(sub_region={:?}, super_region={:?}) = {:?}",
+               sub_region, super_region, result);
+        result
     }
 
     /// Determines whether this free-region is required to be 'static
-    pub fn is_static(&self, super_region: &ty::FreeRegion) -> bool {
+    pub fn is_static(&self, super_region: ty::FreeRegion) -> bool {
         debug!("is_static(super_region={:?})", super_region);
-        self.statics.iter().any(|s| can_reach(&self.map, *s, *super_region))
+        self.relation.contains(&ty::ReStatic, &ty::ReFree(super_region))
     }
 }
+
+#[cfg(test)]
+fn free_region(index: u32) -> FreeRegion {
+    use middle::region::DestructionScopeData;
+    FreeRegion { scope: DestructionScopeData::new(0),
+                 bound_region: ty::BoundRegion::BrAnon(index) }
+}
+
+#[test]
+fn lub() {
+    // a very VERY basic test, but see the tests in
+    // TransitiveRelation, which are much more thorough.
+    let frs: Vec<_> = (0..3).map(|i| free_region(i)).collect();
+    let mut map = FreeRegionMap::new();
+    map.relate_free_regions(frs[0], frs[2]);
+    map.relate_free_regions(frs[1], frs[2]);
+    assert_eq!(map.lub_free_regions(frs[0], frs[1]), ty::ReFree(frs[2]));
+}
+

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

@@ -812,8 +812,8 @@ impl<'a, 'tcx> RegionVarBindings<'a, 'tcx> {
             ReScope(self.tcx.region_maps.nearest_common_ancestor(a_id, b_id))
           }
 
-          (ReFree(ref a_fr), ReFree(ref b_fr)) => {
-             self.lub_free_regions(free_regions, a_fr, b_fr)
+          (ReFree(a_fr), ReFree(b_fr)) => {
+            free_regions.lub_free_regions(a_fr, b_fr)
           }
 
           // For these types, we cannot define any additional
@@ -825,35 +825,6 @@ impl<'a, 'tcx> RegionVarBindings<'a, 'tcx> {
         }
     }
 
-    /// Computes a region that encloses both free region arguments. Guarantee that if the same two
-    /// regions are given as argument, in any order, a consistent result is returned.
-    fn lub_free_regions(&self,
-                        free_regions: &FreeRegionMap,
-                        a: &FreeRegion,
-                        b: &FreeRegion)
-                        -> ty::Region
-    {
-        return match a.cmp(b) {
-            Less => helper(self, free_regions, a, b),
-            Greater => helper(self, free_regions, b, a),
-            Equal => ty::ReFree(*a)
-        };
-
-        fn helper(_this: &RegionVarBindings,
-                  free_regions: &FreeRegionMap,
-                  a: &FreeRegion,
-                  b: &FreeRegion) -> ty::Region
-        {
-            if free_regions.sub_free_region(*a, *b) {
-                ty::ReFree(*b)
-            } else if free_regions.sub_free_region(*b, *a) {
-                ty::ReFree(*a)
-            } else {
-                ty::ReStatic
-            }
-        }
-    }
-
     fn glb_concrete_regions(&self,
                             free_regions: &FreeRegionMap,
                             a: Region,
@@ -892,8 +863,8 @@ impl<'a, 'tcx> RegionVarBindings<'a, 'tcx> {
                             b));
             }
 
-            (ReFree(ref fr), ReScope(s_id)) |
-            (ReScope(s_id), ReFree(ref fr)) => {
+            (ReFree(fr), ReScope(s_id)) |
+            (ReScope(s_id), ReFree(fr)) => {
                 let s = ReScope(s_id);
                 // Free region is something "at least as big as
                 // `fr.scope_id`."  If we find that the scope `fr.scope_id` is bigger

src/librustc/util/common.rs

@@ -203,49 +203,6 @@ pub fn block_query<P>(b: &ast::Block, p: P) -> bool where P: FnMut(&ast::Expr) -
     return v.flag;
 }
 
-/// K: Eq + Hash<S>, V, S, H: Hasher<S>
-///
-/// Determines whether there exists a path from `source` to `destination`.  The
-/// graph is defined by the `edges_map`, which maps from a node `S` to a list of
-/// its adjacent nodes `T`.
-///
-/// Efficiency note: This is implemented in an inefficient way because it is
-/// typically invoked on very small graphs. If the graphs become larger, a more
-/// efficient graph representation and algorithm would probably be advised.
-pub fn can_reach<T, S>(edges_map: &HashMap<T, Vec<T>, S>, source: T,
-                       destination: T) -> bool
-    where S: HashState, T: Hash + Eq + Clone,
-{
-    if source == destination {
-        return true;
-    }
-
-    // Do a little breadth-first-search here.  The `queue` list
-    // doubles as a way to detect if we've seen a particular FR
-    // before.  Note that we expect this graph to be an *extremely
-    // shallow* tree.
-    let mut queue = vec!(source);
-    let mut i = 0;
-    while i < queue.len() {
-        match edges_map.get(&queue[i]) {
-            Some(edges) => {
-                for target in edges {
-                    if *target == destination {
-                        return true;
-                    }
-
-                    if !queue.iter().any(|x| x == target) {
-                        queue.push((*target).clone());
-                    }
-                }
-            }
-            None => {}
-        }
-        i += 1;
-    }
-    return false;
-}
-
 /// Memoizes a one-argument closure using the given RefCell containing
 /// a type implementing MutableMap to serve as a cache.
 ///