replace constant regions with a post-inference check

nikomatsakis · nikomatsakis · commit e38464654eef · 2017-11-10T08:02:07.000-05:00
Rather than declaring some region variables to be constant, and
reporting errors when they would have to change, we instead populate
each free region X with a minimal set of points (the CFG plus end(X)),
and then we let inference do its thing. This may add other `end(Y)`
points into X; we can then check after the fact that indeed `X: Y`
holds.

This requires a bit of "blame" detection to find where the bad
constraint came from: we are currently using a pretty dumb
algorithm. Good place for later expansion.
diff --git a/src/librustc/middle/free_region.rs b/src/librustc/middle/free_region.rs
@@ -63,28 +63,28 @@ impl<'a, 'gcx, 'tcx> RegionRelations<'a, 'gcx, 'tcx> {
                            -> bool {
         let result = sub_region == super_region || {
             match (sub_region, super_region) {
-                (&ty::ReEmpty, _) |
-                (_, &ty::ReStatic) =>
+                (ty::ReEmpty, _) |
+                (_, ty::ReStatic) =>
                     true,
 
-                (&ty::ReScope(sub_scope), &ty::ReScope(super_scope)) =>
-                    self.region_scope_tree.is_subscope_of(sub_scope, super_scope),
+                (ty::ReScope(sub_scope), ty::ReScope(super_scope)) =>
+                    self.region_scope_tree.is_subscope_of(*sub_scope, *super_scope),
 
-                (&ty::ReScope(sub_scope), &ty::ReEarlyBound(ref br)) => {
+                (ty::ReScope(sub_scope), ty::ReEarlyBound(ref br)) => {
                     let fr_scope = self.region_scope_tree.early_free_scope(self.tcx, br);
-                    self.region_scope_tree.is_subscope_of(sub_scope, fr_scope)
+                    self.region_scope_tree.is_subscope_of(*sub_scope, fr_scope)
                 }
 
-                (&ty::ReScope(sub_scope), &ty::ReFree(ref fr)) => {
+                (ty::ReScope(sub_scope), ty::ReFree(fr)) => {
                     let fr_scope = self.region_scope_tree.free_scope(self.tcx, fr);
-                    self.region_scope_tree.is_subscope_of(sub_scope, fr_scope)
+                    self.region_scope_tree.is_subscope_of(*sub_scope, fr_scope)
                 }
 
-                (&ty::ReEarlyBound(_), &ty::ReEarlyBound(_)) |
-                (&ty::ReFree(_), &ty::ReEarlyBound(_)) |
-                (&ty::ReEarlyBound(_), &ty::ReFree(_)) |
-                (&ty::ReFree(_), &ty::ReFree(_)) =>
-                    self.free_regions.relation.contains(&sub_region, &super_region),
+                (ty::ReEarlyBound(_), ty::ReEarlyBound(_)) |
+                (ty::ReFree(_), ty::ReEarlyBound(_)) |
+                (ty::ReEarlyBound(_), ty::ReFree(_)) |
+                (ty::ReFree(_), ty::ReFree(_)) =>
+                    self.free_regions.sub_free_regions(sub_region, super_region),
 
                 _ =>
                     false,
@@ -161,7 +161,7 @@ impl<'tcx> FreeRegionMap<'tcx> {
     // Record that `'sup:'sub`. Or, put another way, `'sub <= 'sup`.
     // (with the exception that `'static: 'x` is not notable)
     pub fn relate_regions(&mut self, sub: Region<'tcx>, sup: Region<'tcx>) {
-        if (is_free(sub) || *sub == ty::ReStatic) && is_free(sup) {
+        if is_free_or_static(sub) && is_free(sup) {
             self.relation.add(sub, sup)
         }
     }
@@ -183,6 +183,20 @@ impl<'tcx> FreeRegionMap<'tcx> {
         result
     }
 
+    /// Tests whether `sub <= sup`. Both must be free regions or
+    /// `'static`.
+    pub fn sub_free_regions<'a, 'gcx>(&self,
+                                      sub: Region<'tcx>,
+                                      sup: Region<'tcx>)
+                                      -> bool {
+        assert!(is_free_or_static(sub) && is_free_or_static(sup));
+        if let ty::ReStatic = sup {
+            true // `'a <= 'static` is just always true, and not stored in the relation explicitly
+        } else {
+            self.relation.contains(&sub, &sup)
+        }
+    }
+
     /// Returns all regions that are known to outlive `r_a`. For
     /// example, in a function:
     ///
@@ -204,6 +218,13 @@ fn is_free(r: Region) -> bool {
     }
 }
 
+fn is_free_or_static(r: Region) -> bool {
+    match *r {
+        ty::ReStatic => true,
+        _ => is_free(r),
+    }
+}
+
 impl_stable_hash_for!(struct FreeRegionMap<'tcx> {
     relation
 });
diff --git a/src/librustc_mir/transform/nll/region_infer.rs b/src/librustc_mir/transform/nll/region_infer.rs
@@ -11,6 +11,7 @@
 use super::RegionIndex;
 use super::free_regions::FreeRegions;
 use rustc::infer::InferCtxt;
+use rustc::middle::free_region::FreeRegionMap;
 use rustc::mir::{Location, Mir};
 use rustc::ty;
 use rustc_data_structures::indexed_vec::{Idx, IndexVec};
@@ -49,17 +50,15 @@ pub struct RegionInferenceContext<'tcx> {
 
     /// The constraints we have accumulated and used during solving.
     constraints: Vec<Constraint>,
+
+    free_region_map: &'tcx FreeRegionMap<'tcx>,
 }
 
 #[derive(Default)]
 struct RegionDefinition<'tcx> {
     /// If this is a free-region, then this is `Some(X)` where `X` is
     /// the name of the region.
     name: Option<ty::Region<'tcx>>,
-
-    /// If true, this is a constant region which cannot grow larger.
-    /// This is used for named regions as well as `'static`.
-    constant: bool,
 }
 
 /// The value of an individual region variable. Region variables
@@ -113,7 +112,7 @@ pub struct Constraint {
     point: Location,
 }
 
-impl<'a, 'gcx, 'tcx> RegionInferenceContext<'tcx> {
+impl<'tcx> RegionInferenceContext<'tcx> {
     /// Creates a new region inference context with a total of
     /// `num_region_variables` valid inference variables; the first N
     /// of those will be constant regions representing the free
@@ -131,6 +130,7 @@ impl<'a, 'gcx, 'tcx> RegionInferenceContext<'tcx> {
             inferred_values: None,
             constraints: Vec::new(),
             free_regions: Vec::new(),
+            free_region_map: free_regions.free_region_map,
         };
 
         result.init_free_regions(free_regions, mir);
@@ -158,9 +158,9 @@ impl<'a, 'gcx, 'tcx> RegionInferenceContext<'tcx> {
     /// is just itself. R1 (`'b`) in contrast also outlives `'a` and
     /// hence contains R0 and R1.
     fn init_free_regions(&mut self, free_regions: &FreeRegions<'tcx>, mir: &Mir<'tcx>) {
-        let &FreeRegions {
-            ref indices,
-            ref free_region_map,
+        let FreeRegions {
+            indices,
+            free_region_map: _,
         } = free_regions;
 
         // For each free region X:
@@ -171,7 +171,6 @@ impl<'a, 'gcx, 'tcx> RegionInferenceContext<'tcx> {
 
             // Initialize the name and a few other details.
             self.definitions[variable].name = Some(free_region);
-            self.definitions[variable].constant = true;
 
             // Add all nodes in the CFG to `definition.value`.
             for (block, block_data) in mir.basic_blocks().iter_enumerated() {
@@ -188,13 +187,6 @@ impl<'a, 'gcx, 'tcx> RegionInferenceContext<'tcx> {
             // Add `end(X)` into the set for X.
             self.liveness_constraints[variable].add_free_region(variable);
 
-            // Go through each region Y that outlives X (i.e., where
-            // Y: X is true). Add `end(X)` into the set for `Y`.
-            for superregion in free_region_map.regions_that_outlive(&free_region) {
-                let superregion_index = RegionIndex::new(indices[superregion]);
-                self.liveness_constraints[superregion_index].add_free_region(variable);
-            }
-
             debug!(
                 "init_free_regions: region variable for `{:?}` is `{:?}` with value `{:?}`",
                 free_region,
@@ -253,84 +245,151 @@ impl<'a, 'gcx, 'tcx> RegionInferenceContext<'tcx> {
     }
 
     /// Perform region inference.
-    pub(super) fn solve(&mut self, infcx: &InferCtxt<'a, 'gcx, 'tcx>, mir: &Mir<'tcx>) {
+    pub(super) fn solve(&mut self, infcx: &InferCtxt<'_, '_, 'tcx>, mir: &Mir<'tcx>) {
         assert!(self.inferred_values.is_none(), "values already inferred");
-        let errors = self.propagate_constraints(mir);
-
-        // worst error msg ever
-        for (fr1, span, fr2) in errors {
-            infcx.tcx.sess.span_err(
-                span,
-                &format!(
-                    "free region `{}` does not outlive `{}`",
-                    self.definitions[fr1].name.unwrap(),
-                    self.definitions[fr2].name.unwrap()
-                ),
-            );
+
+        // Find the minimal regions that can solve the constraints. This is infallible.
+        self.propagate_constraints(mir);
+
+        // Now, see whether any of the constraints were too strong. In particular,
+        // we want to check for a case where a free region exceeded its bounds.
+        // Consider:
+        //
+        //     fn foo<'a, 'b>(x: &'a u32) -> &'b u32 { x }
+        //
+        // In this case, returning `x` requires `&'a u32 <: &'b u32`
+        // and hence we establish (transitively) a constraint that
+        // `'a: 'b`. The `propagate_constraints` code above will
+        // therefore add `end('a)` into the region for `'b` -- but we
+        // have no evidence that `'b` outlives `'a`, so we want to report
+        // an error.
+        for free_region in &self.free_regions {
+            self.check_free_region(infcx, *free_region);
+        }
+    }
+
+    fn check_free_region(&self, infcx: &InferCtxt<'_, '_, 'tcx>, fr: RegionIndex) {
+        let inferred_values = self.inferred_values.as_ref().unwrap();
+        let fr_definition = &self.definitions[fr];
+        let fr_name = fr_definition.name.unwrap();
+        let fr_value = &inferred_values[fr];
+
+        // Find every region `o` such that `fr: o`
+        // (because `fr` includes `end(o)`).
+        for &outlived_fr in &fr_value.free_regions {
+            // `fr` includes `end(fr)`, that's not especially
+            // interesting.
+            if fr == outlived_fr {
+                continue;
+            }
+
+            let outlived_fr_definition = &self.definitions[outlived_fr];
+            let outlived_fr_name = outlived_fr_definition.name.unwrap();
+
+            // Check that `o <= fr`. If not, report an error.
+            if !self.free_region_map
+                .sub_free_regions(outlived_fr_name, fr_name)
+            {
+                // worst error msg ever
+                let blame_span = self.blame_span(fr, outlived_fr);
+                infcx.tcx.sess.span_err(
+                    blame_span,
+                    &format!(
+                        "free region `{}` does not outlive `{}`",
+                        fr_name,
+                        outlived_fr_name
+                    ),
+                );
+            }
         }
     }
 
     /// Propagate the region constraints: this will grow the values
     /// for each region variable until all the constraints are
     /// satisfied. Note that some values may grow **too** large to be
     /// feasible, but we check this later.
-    fn propagate_constraints(&mut self, mir: &Mir<'tcx>) -> Vec<(RegionIndex, Span, RegionIndex)> {
+    fn propagate_constraints(&mut self, mir: &Mir<'tcx>) {
         let mut changed = true;
         let mut dfs = Dfs::new(mir);
-        let mut error_regions = FxHashSet();
-        let mut errors = vec![];
 
         // The initial values for each region are derived from the liveness
         // constraints we have accumulated.
         let mut inferred_values = self.liveness_constraints.clone();
 
         while changed {
             changed = false;
+            debug!("propagate_constraints: --------------------");
             for constraint in &self.constraints {
-                debug!("constraint: {:?}", constraint);
+                debug!("propagate_constraints: constraint={:?}", constraint);
+
                 let sub = &inferred_values[constraint.sub].clone();
                 let sup_value = &mut inferred_values[constraint.sup];
 
-                debug!("    sub (before): {:?}", sub);
-                debug!("    sup (before): {:?}", sup_value);
+                // Grow the value as needed to accommodate the
+                // outlives constraint.
 
-                if !self.definitions[constraint.sup].constant {
-                    // If this is not a constant, then grow the value as needed to
-                    // accommodate the outlives constraint.
+                if dfs.copy(sub, sup_value, constraint.point) {
+                    debug!("propagate_constraints:   sub={:?}", sub);
+                    debug!("propagate_constraints:   sup={:?}", sup_value);
+                    changed = true;
+                }
+            }
+            debug!("\n");
+        }
 
-                    if dfs.copy(sub, sup_value, constraint.point) {
-                        changed = true;
-                    }
+        self.inferred_values = Some(inferred_values);
+    }
 
-                    debug!("    sup (after) : {:?}", sup_value);
-                    debug!("    changed     : {:?}", changed);
-                } else {
-                    // If this is a constant, check whether it *would
-                    // have* to grow in order for the constraint to be
-                    // satisfied. If so, create an error.
-
-                    let mut sup_value = &mut sup_value.clone();
-                    if dfs.copy(sub, sup_value, constraint.point) {
-                        // Constant values start out with the entire
-                        // CFG, so it must be some new free region
-                        // that was added. Find one.
-                        let &new_region = sup_value
-                            .free_regions
-                            .difference(&sup_value.free_regions)
-                            .next()
-                            .unwrap();
-                        debug!("    new_region : {:?}", new_region);
-                        if error_regions.insert(constraint.sup) {
-                            errors.push((constraint.sup, constraint.span, new_region));
-                        }
+    /// Tries to finds a good span to blame for the fact that `fr1`
+    /// contains `fr2`.
+    fn blame_span(&self, fr1: RegionIndex, fr2: RegionIndex) -> Span {
+        // Find everything that influenced final value of `fr`.
+        let influenced_fr1 = self.dependencies(fr1);
+
+        // Try to find some outlives constraint `'X: fr2` where `'X`
+        // influenced `fr1`. Blame that.
+        //
+        // NB, this is a pretty bad choice most of the time. In
+        // particular, the connection between `'X` and `fr1` may not
+        // be obvious to the user -- not to mention the naive notion
+        // of dependencies, which doesn't account for the locations of
+        // contraints at all. But it will do for now.
+        for constraint in &self.constraints {
+            if constraint.sub == fr2 && influenced_fr1[constraint.sup] {
+                return constraint.span;
+            }
+        }
+
+        bug!(
+            "could not find any constraint to blame for {:?}: {:?}",
+            fr1,
+            fr2
+        );
+    }
+
+    /// Finds all regions whose values `'a` may depend on in some way.
+    /// Basically if there exists a constraint `'a: 'b @ P`, then `'b`
+    /// and `dependencies('b)` will be in the final set.
+    ///
+    /// Used during error reporting, extremely naive and inefficient.
+    fn dependencies(&self, r0: RegionIndex) -> IndexVec<RegionIndex, bool> {
+        let mut result_set = IndexVec::from_elem(false, &self.definitions);
+        let mut changed = true;
+        result_set[r0] = true;
+
+        while changed {
+            changed = false;
+            for constraint in &self.constraints {
+                if result_set[constraint.sup] {
+                    if !result_set[constraint.sub] {
+                        result_set[constraint.sub] = true;
+                        changed = true;
                     }
                 }
             }
-            debug!("\n");
         }
 
-        self.inferred_values = Some(inferred_values);
-        errors
+        result_set
     }
 }
 
diff --git a/src/test/mir-opt/nll/named-lifetimes-basic.rs b/src/test/mir-opt/nll/named-lifetimes-basic.rs
@@ -27,7 +27,7 @@ fn main() {
 // END RUST SOURCE
 // START rustc.use_x.nll.0.mir
 // | '_#0r: {bb0[0], bb0[1], '_#0r}
-// | '_#1r: {bb0[0], bb0[1], '_#0r, '_#1r}
+// | '_#1r: {bb0[0], bb0[1], '_#1r}
 // | '_#2r: {bb0[0], bb0[1], '_#2r}
 // ...
 // fn use_x(_1: &'_#0r mut i32, _2: &'_#1r u32, _3: &'_#0r u32, _4: &'_#2r u32) -> bool {
