Rollup merge of #70950 - nikomatsakis:leak-check-nll-2, r=matthewjasper

extend NLL checker to understand `'empty` combined with universes

This PR extends the NLL region checker to understand `'empty` combined with universes. In particular, it means that the NLL region checker no longer considers `exists<R2> { forall<R1> { R1: R2 } }` to be provable. This is work towards #59490, but we're not all the way there. One thing in particular it does not address is error messages.

The modifications to the NLL region inference code turned out to be simpler than expected. The main change is to require that if `R1: R2` then `universe(R1) <= universe(R2)`.

This constraint follows from the region lattice (shown below), because we assume then that `R2` is "at least" `empty(Universe(R2))`, and hence if `R1: R2` (i.e., `R1 >= R2` on the lattice) then `R1` must be in some universe that can name `'empty(Universe(R2))`, which requires that `Universe(R1) <= Universe(R2)`.

```
static ----------+-----...------+       (greatest)
|                |              |
early-bound and  |              |
free regions     |              |
|                |              |
scope regions    |              |
|                |              |
empty(root)   placeholder(U1)   |
|            /                  |
|           /         placeholder(Un)
empty(U1) --         /
|                   /
...                /
|                 /
empty(Un) --------                      (smallest)
```

I also made what turned out to be a somewhat unrelated change to add a special region to represent `'empty(U0)`, which we use (somewhat hackily) to indicate well-formedness checks in some parts of the compiler. This fixes #68550.

I did some investigation into fixing the error message situation. That's a bit trickier: the existing "nice region error" code around placeholders relies on having better error tracing than NLL currently provides, so that it knows (e.g.) that the constraint arose from applying a trait impl and things like that. I feel like I was hoping *not* to do such fine-grained tracing in NLL, and it seems like we...largely...got away with that. I'm not sure yet if we'll have to add more tracing information or if there is some sort of alternative.

It's worth pointing out though that I've not kind of shifted my opinion on whose job it should be to enforce lifetimes: I tend to think we ought to be moving back towards *something like* the leak-check (just not the one we *had*). If we took that approach, it would actually resolve this aspect of the error message problem, because we would be resolving 'higher-ranked errors' in the trait solver itself, and hence we wouldn't have to thread as much causal information back to the region checker. I think it would also help us with removing the leak check while not breaking some of the existing crates out there.

Regardless, I think it's worth landing this change, because it was relatively simple and it aligns the set of programs that NLL accepts with those that are accepted by the main region checker, and hence should at least *help* us in migration (though I guess we still also have to resolve the existing crates that rely on leak check for coherence).

r? @matthewjasper

Author: Dylan-DPC

src/librustc_data_structures/graph/scc/mod.rs

@@ -47,6 +47,11 @@ impl<N: Idx, S: Idx> Sccs<N, S> {
     }
 
     /// Returns an iterator over the SCCs in the graph.
+    ///
+    /// The SCCs will be iterated in **dependency order** (or **post order**),
+    /// meaning that if `S1 -> S2`, we will visit `S2` first and `S1` after.
+    /// This is convenient when the edges represent dependencies: when you visit
+    /// `S1`, the value for `S2` will already have been computed.
     pub fn all_sccs(&self) -> impl Iterator<Item = S> {
         (0..self.scc_data.len()).map(S::new)
     }

src/librustc_infer/infer/mod.rs

@@ -472,6 +472,9 @@ pub enum NLLRegionVariableOrigin {
     /// from a `for<'a> T` binder). Meant to represent "any region".
     Placeholder(ty::PlaceholderRegion),
 
+    /// The variable we create to represent `'empty(U0)`.
+    RootEmptyRegion,
+
     Existential {
         /// If this is true, then this variable was created to represent a lifetime
         /// bound in a `for` binder. For example, it might have been created to
@@ -493,6 +496,7 @@ impl NLLRegionVariableOrigin {
             NLLRegionVariableOrigin::FreeRegion => true,
             NLLRegionVariableOrigin::Placeholder(..) => true,
             NLLRegionVariableOrigin::Existential { .. } => false,
+            NLLRegionVariableOrigin::RootEmptyRegion => false,
         }
     }
 

src/librustc_mir/borrow_check/region_infer/mod.rs

@@ -6,7 +6,6 @@ use rustc_data_structures::frozen::Frozen;
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_data_structures::graph::scc::Sccs;
 use rustc_hir::def_id::DefId;
-use rustc_index::bit_set::BitSet;
 use rustc_index::vec::IndexVec;
 use rustc_infer::infer::canonical::QueryOutlivesConstraint;
 use rustc_infer::infer::region_constraints::{GenericKind, VarInfos, VerifyBound};
@@ -315,16 +314,81 @@ impl<'tcx> RegionInferenceContext<'tcx> {
     /// SCC could have as well. This implies that the SCC must have
     /// the minimum, or narrowest, universe.
     fn compute_scc_universes(
-        constraints_scc: &Sccs<RegionVid, ConstraintSccIndex>,
+        constraint_sccs: &Sccs<RegionVid, ConstraintSccIndex>,
         definitions: &IndexVec<RegionVid, RegionDefinition<'tcx>>,
     ) -> IndexVec<ConstraintSccIndex, ty::UniverseIndex> {
-        let num_sccs = constraints_scc.num_sccs();
+        let num_sccs = constraint_sccs.num_sccs();
         let mut scc_universes = IndexVec::from_elem_n(ty::UniverseIndex::MAX, num_sccs);
 
+        debug!("compute_scc_universes()");
+
+        // For each region R in universe U, ensure that the universe for the SCC
+        // that contains R is "no bigger" than U. This effectively sets the universe
+        // for each SCC to be the minimum of the regions within.
         for (region_vid, region_definition) in definitions.iter_enumerated() {
-            let scc = constraints_scc.scc(region_vid);
+            let scc = constraint_sccs.scc(region_vid);
             let scc_universe = &mut scc_universes[scc];
-            *scc_universe = ::std::cmp::min(*scc_universe, region_definition.universe);
+            let scc_min = std::cmp::min(region_definition.universe, *scc_universe);
+            if scc_min != *scc_universe {
+                *scc_universe = scc_min;
+                debug!(
+                    "compute_scc_universes: lowered universe of {scc:?} to {scc_min:?} \
+                    because it contains {region_vid:?} in {region_universe:?}",
+                    scc = scc,
+                    scc_min = scc_min,
+                    region_vid = region_vid,
+                    region_universe = region_definition.universe,
+                );
+            }
+        }
+
+        // Walk each SCC `A` and `B` such that `A: B`
+        // and ensure that universe(A) can see universe(B).
+        //
+        // This serves to enforce the 'empty/placeholder' hierarchy
+        // (described in more detail on `RegionKind`):
+        //
+        // ```
+        // static -----+
+        //   |         |
+        // empty(U0) placeholder(U1)
+        //   |      /
+        // empty(U1)
+        // ```
+        //
+        // In particular, imagine we have variables R0 in U0 and R1
+        // created in U1, and constraints like this;
+        //
+        // ```
+        // R1: !1 // R1 outlives the placeholder in U1
+        // R1: R0 // R1 outlives R0
+        // ```
+        //
+        // Here, we wish for R1 to be `'static`, because it
+        // cannot outlive `placeholder(U1)` and `empty(U0)` any other way.
+        //
+        // Thanks to this loop, what happens is that the `R1: R0`
+        // constraint lowers the universe of `R1` to `U0`, which in turn
+        // means that the `R1: !1` constraint will (later) cause
+        // `R1` to become `'static`.
+        for scc_a in constraint_sccs.all_sccs() {
+            for &scc_b in constraint_sccs.successors(scc_a) {
+                let scc_universe_a = scc_universes[scc_a];
+                let scc_universe_b = scc_universes[scc_b];
+                let scc_universe_min = std::cmp::min(scc_universe_a, scc_universe_b);
+                if scc_universe_a != scc_universe_min {
+                    scc_universes[scc_a] = scc_universe_min;
+
+                    debug!(
+                        "compute_scc_universes: lowered universe of {scc_a:?} to {scc_universe_min:?} \
+                        because {scc_a:?}: {scc_b:?} and {scc_b:?} is in universe {scc_universe_b:?}",
+                        scc_a = scc_a,
+                        scc_b = scc_b,
+                        scc_universe_min = scc_universe_min,
+                        scc_universe_b = scc_universe_b
+                    );
+                }
+            }
         }
 
         debug!("compute_scc_universes: scc_universe = {:#?}", scc_universes);
@@ -416,7 +480,8 @@ impl<'tcx> RegionInferenceContext<'tcx> {
                     }
                 }
 
-                NLLRegionVariableOrigin::Existential { .. } => {
+                NLLRegionVariableOrigin::RootEmptyRegion
+                | NLLRegionVariableOrigin::Existential { .. } => {
                     // For existential, regions, nothing to do.
                 }
             }
@@ -550,47 +615,27 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         // SCC. For each SCC, we visit its successors and compute
         // their values, then we union all those values to get our
         // own.
-        let visited = &mut BitSet::new_empty(self.constraint_sccs.num_sccs());
-        for scc_index in self.constraint_sccs.all_sccs() {
-            self.propagate_constraint_sccs_if_new(scc_index, visited);
+        let constraint_sccs = self.constraint_sccs.clone();
+        for scc in constraint_sccs.all_sccs() {
+            self.compute_value_for_scc(scc);
         }
 
         // Sort the applied member constraints so we can binary search
         // through them later.
         self.member_constraints_applied.sort_by_key(|applied| applied.member_region_scc);
     }
 
-    /// Computes the value of the SCC `scc_a` if it has not already
-    /// been computed. The `visited` parameter is a bitset
-    #[inline]
-    fn propagate_constraint_sccs_if_new(
-        &mut self,
-        scc_a: ConstraintSccIndex,
-        visited: &mut BitSet<ConstraintSccIndex>,
-    ) {
-        if visited.insert(scc_a) {
-            self.propagate_constraint_sccs_new(scc_a, visited);
-        }
-    }
-
     /// Computes the value of the SCC `scc_a`, which has not yet been
-    /// computed. This works by first computing all successors of the
-    /// SCC (if they haven't been computed already) and then unioning
-    /// together their elements.
-    fn propagate_constraint_sccs_new(
-        &mut self,
-        scc_a: ConstraintSccIndex,
-        visited: &mut BitSet<ConstraintSccIndex>,
-    ) {
+    /// computed, by unioning the values of its successors.
+    /// Assumes that all successors have been computed already
+    /// (which is assured by iterating over SCCs in dependency order).
+    fn compute_value_for_scc(&mut self, scc_a: ConstraintSccIndex) {
         let constraint_sccs = self.constraint_sccs.clone();
 
         // Walk each SCC `B` such that `A: B`...
         for &scc_b in constraint_sccs.successors(scc_a) {
             debug!("propagate_constraint_sccs: scc_a = {:?} scc_b = {:?}", scc_a, scc_b);
 
-            // ...compute the value of `B`...
-            self.propagate_constraint_sccs_if_new(scc_b, visited);
-
             // ...and add elements from `B` into `A`. One complication
             // arises because of universes: If `B` contains something
             // that `A` cannot name, then `A` can only contain `B` if
@@ -1258,7 +1303,8 @@ impl<'tcx> RegionInferenceContext<'tcx> {
                     self.check_bound_universal_region(fr, placeholder, errors_buffer);
                 }
 
-                NLLRegionVariableOrigin::Existential { .. } => {
+                NLLRegionVariableOrigin::RootEmptyRegion
+                | NLLRegionVariableOrigin::Existential { .. } => {
                     // nothing to check here
                 }
             }
@@ -1360,7 +1406,8 @@ impl<'tcx> RegionInferenceContext<'tcx> {
                     self.check_bound_universal_region(fr, placeholder, errors_buffer);
                 }
 
-                NLLRegionVariableOrigin::Existential { .. } => {
+                NLLRegionVariableOrigin::RootEmptyRegion
+                | NLLRegionVariableOrigin::Existential { .. } => {
                     // nothing to check here
                 }
             }
@@ -1633,9 +1680,9 @@ impl<'tcx> RegionInferenceContext<'tcx> {
                 universe1.cannot_name(placeholder.universe)
             }
 
-            NLLRegionVariableOrigin::FreeRegion | NLLRegionVariableOrigin::Existential { .. } => {
-                false
-            }
+            NLLRegionVariableOrigin::RootEmptyRegion
+            | NLLRegionVariableOrigin::FreeRegion
+            | NLLRegionVariableOrigin::Existential { .. } => false,
         }
     }
 
@@ -1773,6 +1820,12 @@ impl<'tcx> RegionInferenceContext<'tcx> {
     /// Finds some region R such that `fr1: R` and `R` is live at `elem`.
     crate fn find_sub_region_live_at(&self, fr1: RegionVid, elem: Location) -> RegionVid {
         debug!("find_sub_region_live_at(fr1={:?}, elem={:?})", fr1, elem);
+        debug!("find_sub_region_live_at: {:?} is in scc {:?}", fr1, self.constraint_sccs.scc(fr1));
+        debug!(
+            "find_sub_region_live_at: {:?} is in universe {:?}",
+            fr1,
+            self.scc_universes[self.constraint_sccs.scc(fr1)]
+        );
         self.find_constraint_paths_between_regions(fr1, |r| {
             // First look for some `r` such that `fr1: r` and `r` is live at `elem`
             debug!(
@@ -1794,13 +1847,16 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         .or_else(|| {
             // If we fail to find THAT, it may be that `fr1` is a
             // placeholder that cannot "fit" into its SCC. In that
-            // case, there should be some `r` where `fr1: r`, both
-            // `fr1` and `r` are in the same SCC, and `fr1` is a
+            // case, there should be some `r` where `fr1: r` and `fr1` is a
             // placeholder that `r` cannot name. We can blame that
             // edge.
+            //
+            // Remember that if `R1: R2`, then the universe of R1
+            // must be able to name the universe of R2, because R2 will
+            // be at least `'empty(Universe(R2))`, and `R1` must be at
+            // larger than that.
             self.find_constraint_paths_between_regions(fr1, |r| {
-                self.constraint_sccs.scc(fr1) == self.constraint_sccs.scc(r)
-                    && self.cannot_name_placeholder(r, fr1)
+                self.cannot_name_placeholder(r, fr1)
             })
         })
         .map(|(_path, r)| r)
@@ -1944,7 +2000,8 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         let blame_source = match from_region_origin {
             NLLRegionVariableOrigin::FreeRegion
             | NLLRegionVariableOrigin::Existential { from_forall: false } => true,
-            NLLRegionVariableOrigin::Placeholder(_)
+            NLLRegionVariableOrigin::RootEmptyRegion
+            | NLLRegionVariableOrigin::Placeholder(_)
             | NLLRegionVariableOrigin::Existential { from_forall: true } => false,
         };
 

src/librustc_mir/borrow_check/type_check/constraint_conversion.rs

@@ -160,10 +160,6 @@ impl<'a, 'b, 'tcx> TypeOutlivesDelegate<'tcx> for &'a mut ConstraintConversion<'
         a: ty::Region<'tcx>,
         b: ty::Region<'tcx>,
     ) {
-        // FIXME -- this is not the fix I would prefer
-        if let ty::ReEmpty(ty::UniverseIndex::ROOT) = a {
-            return;
-        }
         let b = self.to_region_vid(b);
         let a = self.to_region_vid(a);
         self.add_outlives(b, a);
@@ -176,10 +172,6 @@ impl<'a, 'b, 'tcx> TypeOutlivesDelegate<'tcx> for &'a mut ConstraintConversion<'
         a: ty::Region<'tcx>,
         bound: VerifyBound<'tcx>,
     ) {
-        // FIXME: I'd prefer if NLL had a notion of empty
-        if let ty::ReEmpty(ty::UniverseIndex::ROOT) = a {
-            return;
-        }
         let type_test = self.verify_to_type_test(kind, a, bound);
         self.add_type_test(type_test);
     }

src/librustc_mir/borrow_check/universal_regions.rs

@@ -54,6 +54,13 @@ pub struct UniversalRegions<'tcx> {
     /// The total number of universal region variables instantiated.
     num_universals: usize,
 
+    /// A special region variable created for the `'empty(U0)` region.
+    /// Note that this is **not** a "universal" region, as it doesn't
+    /// represent a universally bound placeholder or any such thing.
+    /// But we do create it here in this type because it's a useful region
+    /// to have around in a few limited cases.
+    pub root_empty: RegionVid,
+
     /// The "defining" type for this function, with all universal
     /// regions instantiated. For a closure or generator, this is the
     /// closure type, but for a top-level function it's the `FnDef`.
@@ -317,7 +324,11 @@ impl<'tcx> UniversalRegions<'tcx> {
 
     /// See `UniversalRegionIndices::to_region_vid`.
     pub fn to_region_vid(&self, r: ty::Region<'tcx>) -> RegionVid {
-        self.indices.to_region_vid(r)
+        if let ty::ReEmpty(ty::UniverseIndex::ROOT) = r {
+            self.root_empty
+        } else {
+            self.indices.to_region_vid(r)
+        }
     }
 
     /// As part of the NLL unit tests, you can annotate a function with
@@ -473,10 +484,16 @@ impl<'cx, 'tcx> UniversalRegionsBuilder<'cx, 'tcx> {
             _ => None,
         };
 
+        let root_empty = self
+            .infcx
+            .next_nll_region_var(NLLRegionVariableOrigin::RootEmptyRegion)
+            .to_region_vid();
+
         UniversalRegions {
             indices,
             fr_static,
             fr_fn_body,
+            root_empty,
             first_extern_index,
             first_local_index,
             num_universals,

src/test/mir-opt/nll/named-lifetimes-basic/rustc.use_x.nll.0.mir

@@ -13,27 +13,28 @@
 | '_#2r | U0 | {bb0[0..=1], '_#2r}
 | '_#3r | U0 | {bb0[0..=1], '_#3r}
 | '_#4r | U0 | {bb0[0..=1], '_#4r}
-| '_#5r | U0 | {bb0[0..=1], '_#1r}
-| '_#6r | U0 | {bb0[0..=1], '_#2r}
-| '_#7r | U0 | {bb0[0..=1], '_#1r}
-| '_#8r | U0 | {bb0[0..=1], '_#3r}
+| '_#5r | U0 | {}
+| '_#6r | U0 | {bb0[0..=1], '_#1r}
+| '_#7r | U0 | {bb0[0..=1], '_#2r}
+| '_#8r | U0 | {bb0[0..=1], '_#1r}
+| '_#9r | U0 | {bb0[0..=1], '_#3r}
 |
 | Inference Constraints
 | '_#0r live at {bb0[0..=1]}
 | '_#1r live at {bb0[0..=1]}
 | '_#2r live at {bb0[0..=1]}
 | '_#3r live at {bb0[0..=1]}
 | '_#4r live at {bb0[0..=1]}
-| '_#1r: '_#5r due to BoringNoLocation at All($DIR/named-lifetimes-basic.rs:12:26: 12:27)
-| '_#1r: '_#7r due to BoringNoLocation at All($DIR/named-lifetimes-basic.rs:12:54: 12:55)
-| '_#2r: '_#6r due to BoringNoLocation at All($DIR/named-lifetimes-basic.rs:12:42: 12:43)
-| '_#3r: '_#8r due to BoringNoLocation at All($DIR/named-lifetimes-basic.rs:12:66: 12:67)
-| '_#5r: '_#1r due to BoringNoLocation at All($DIR/named-lifetimes-basic.rs:12:26: 12:27)
-| '_#6r: '_#2r due to BoringNoLocation at All($DIR/named-lifetimes-basic.rs:12:42: 12:43)
-| '_#7r: '_#1r due to BoringNoLocation at All($DIR/named-lifetimes-basic.rs:12:54: 12:55)
-| '_#8r: '_#3r due to BoringNoLocation at All($DIR/named-lifetimes-basic.rs:12:66: 12:67)
+| '_#1r: '_#6r due to BoringNoLocation at All($DIR/named-lifetimes-basic.rs:12:26: 12:27)
+| '_#1r: '_#8r due to BoringNoLocation at All($DIR/named-lifetimes-basic.rs:12:54: 12:55)
+| '_#2r: '_#7r due to BoringNoLocation at All($DIR/named-lifetimes-basic.rs:12:42: 12:43)
+| '_#3r: '_#9r due to BoringNoLocation at All($DIR/named-lifetimes-basic.rs:12:66: 12:67)
+| '_#6r: '_#1r due to BoringNoLocation at All($DIR/named-lifetimes-basic.rs:12:26: 12:27)
+| '_#7r: '_#2r due to BoringNoLocation at All($DIR/named-lifetimes-basic.rs:12:42: 12:43)
+| '_#8r: '_#1r due to BoringNoLocation at All($DIR/named-lifetimes-basic.rs:12:54: 12:55)
+| '_#9r: '_#3r due to BoringNoLocation at All($DIR/named-lifetimes-basic.rs:12:66: 12:67)
 |
-fn use_x(_1: &'_#5r mut i32, _2: &'_#6r u32, _3: &'_#7r u32, _4: &'_#8r u32) -> bool {
+fn use_x(_1: &'_#6r mut i32, _2: &'_#7r u32, _3: &'_#8r u32, _4: &'_#9r u32) -> bool {
     debug w => _1;                       // in scope 0 at $DIR/named-lifetimes-basic.rs:12:26: 12:27
     debug x => _2;                       // in scope 0 at $DIR/named-lifetimes-basic.rs:12:42: 12:43
     debug y => _3;                       // in scope 0 at $DIR/named-lifetimes-basic.rs:12:54: 12:55

src/test/mir-opt/nll/region-subtyping-basic.rs

@@ -7,7 +7,9 @@
 
 #![allow(warnings)]
 
-fn use_x(_: usize) -> bool { true }
+fn use_x(_: usize) -> bool {
+    true
+}
 
 // EMIT_MIR_FOR_EACH_BIT_WIDTH
 // EMIT_MIR rustc.main.nll.0.mir