Auto merge of #148329 - LorrensP-2158466:closure-prop, r=lcnr

Better closure requirement propagation.

Fixes #148289
Fixes #104477
Should unblock #148187

When propagating closure requirements we:
- no longer try to find a post dominiting region for the list of non-local lower bounds of `longer_fr`.
- we filter the list of `longer_fr-: shorter_fr+` to only these constraints between external regions which are required regardless. If no such constraint exists, we fall back to propagating all of them.

See the [FCP](#148329 (comment)) for more information.

r? `@lcnr`

Author: bors

compiler/rustc_borrowck/src/region_infer/mod.rs

@@ -1275,29 +1275,81 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         shorter_fr: RegionVid,
         propagated_outlives_requirements: &mut Option<&mut Vec<ClosureOutlivesRequirement<'tcx>>>,
     ) -> RegionRelationCheckResult {
-        if let Some(propagated_outlives_requirements) = propagated_outlives_requirements
-            // Shrink `longer_fr` until we find a non-local region (if we do).
-            // We'll call it `fr-` -- it's ever so slightly smaller than
+        if let Some(propagated_outlives_requirements) = propagated_outlives_requirements {
+            // Shrink `longer_fr` until we find some non-local regions.
+            // We'll call them `longer_fr-` -- they are ever so slightly smaller than
             // `longer_fr`.
-            && let Some(fr_minus) = self.universal_region_relations.non_local_lower_bound(longer_fr)
-        {
-            debug!("try_propagate_universal_region_error: fr_minus={:?}", fr_minus);
+            let longer_fr_minus = self.universal_region_relations.non_local_lower_bounds(longer_fr);
+
+            debug!("try_propagate_universal_region_error: fr_minus={:?}", longer_fr_minus);
+
+            // If we don't find a any non-local regions, we should error out as there is nothing
+            // to propagate.
+            if longer_fr_minus.is_empty() {
+                return RegionRelationCheckResult::Error;
+            }
 
             let blame_constraint = self
                 .best_blame_constraint(longer_fr, NllRegionVariableOrigin::FreeRegion, shorter_fr)
                 .0;
 
-            // Grow `shorter_fr` until we find some non-local regions. (We
-            // always will.)  We'll call them `shorter_fr+` -- they're ever
-            // so slightly larger than `shorter_fr`.
+            // Grow `shorter_fr` until we find some non-local regions.
+            // We will always find at least one: `'static`. We'll call
+            // them `shorter_fr+` -- they're ever so slightly larger
+            // than `shorter_fr`.
             let shorter_fr_plus =
                 self.universal_region_relations.non_local_upper_bounds(shorter_fr);
             debug!("try_propagate_universal_region_error: shorter_fr_plus={:?}", shorter_fr_plus);
-            for fr in shorter_fr_plus {
-                // Push the constraint `fr-: shorter_fr+`
+
+            // We then create constraints `longer_fr-: shorter_fr+` that may or may not
+            // be propagated (see below).
+            let mut constraints = vec![];
+            for fr_minus in longer_fr_minus {
+                for shorter_fr_plus in &shorter_fr_plus {
+                    constraints.push((fr_minus, *shorter_fr_plus));
+                }
+            }
+
+            // We only need to propagate at least one of the constraints for
+            // soundness. However, we want to avoid arbitrary choices here
+            // and currently don't support returning OR constraints.
+            //
+            // If any of the `shorter_fr+` regions are already outlived by `longer_fr-`,
+            // we propagate only those.
+            //
+            // Consider this example (`'b: 'a` == `a -> b`), where we try to propagate `'d: 'a`:
+            // a --> b --> d
+            //  \
+            //   \-> c
+            // Here, `shorter_fr+` of `'a` == `['b, 'c]`.
+            // Propagating `'d: 'b` is correct and should occur; `'d: 'c` is redundant because of
+            // `'d: 'b` and could reject valid code.
+            //
+            // So we filter the constraints to regions already outlived by `longer_fr-`, but if
+            // the filter yields an empty set, we fall back to the original one.
+            let subset: Vec<_> = constraints
+                .iter()
+                .filter(|&&(fr_minus, shorter_fr_plus)| {
+                    self.eval_outlives(fr_minus, shorter_fr_plus)
+                })
+                .copied()
+                .collect();
+            let propagated_constraints = if subset.is_empty() { constraints } else { subset };
+            debug!(
+                "try_propagate_universal_region_error: constraints={:?}",
+                propagated_constraints
+            );
+
+            assert!(
+                !propagated_constraints.is_empty(),
+                "Expected at least one constraint to propagate here"
+            );
+
+            for (fr_minus, fr_plus) in propagated_constraints {
+                // Push the constraint `long_fr-: shorter_fr+`
                 propagated_outlives_requirements.push(ClosureOutlivesRequirement {
                     subject: ClosureOutlivesSubject::Region(fr_minus),
-                    outlived_free_region: fr,
+                    outlived_free_region: fr_plus,
                     blame_span: blame_constraint.cause.span,
                     category: blame_constraint.category,
                 });

compiler/rustc_borrowck/src/type_check/free_region_relations.rs

@@ -94,28 +94,10 @@ impl UniversalRegionRelations<'_> {
     /// words, returns the largest (*) known region `fr1` that (a) is
     /// outlived by `fr` and (b) is not local.
     ///
-    /// (*) If there are multiple competing choices, we pick the "postdominating"
-    /// one. See `TransitiveRelation::postdom_upper_bound` for details.
-    pub(crate) fn non_local_lower_bound(&self, fr: RegionVid) -> Option<RegionVid> {
+    /// (*) If there are multiple competing choices, we return all of them.
+    pub(crate) fn non_local_lower_bounds(&self, fr: RegionVid) -> Vec<RegionVid> {
         debug!("non_local_lower_bound(fr={:?})", fr);
-        let lower_bounds = self.non_local_bounds(&self.outlives, fr);
-
-        // In case we find more than one, reduce to one for
-        // convenience. This is to prevent us from generating more
-        // complex constraints, but it will cause spurious errors.
-        let post_dom = self.outlives.mutual_immediate_postdominator(lower_bounds);
-
-        debug!("non_local_bound: post_dom={:?}", post_dom);
-
-        post_dom.and_then(|post_dom| {
-            // If the mutual immediate postdom is not local, then
-            // there is no non-local result we can return.
-            if !self.universal_regions.is_local_free_region(post_dom) {
-                Some(post_dom)
-            } else {
-                None
-            }
-        })
+        self.non_local_bounds(&self.outlives, fr)
     }
 
     /// Helper for `non_local_upper_bounds` and `non_local_lower_bounds`.

…ropagate-approximated-fail-no-postdom.rs …pagate-approximated-both-lower-bounds.rstests/ui/nll/closure-requirements/propagate-approximated-fail-no-postdom.rs renamed to tests/ui/nll/closure-requirements/propagate-approximated-both-lower-bounds.rs tests/ui/nll/closure-requirements/propagate-approximated-fail-no-postdom.rs renamed to tests/ui/nll/closure-requirements/propagate-approximated-both-lower-bounds.rs

@@ -1,6 +1,5 @@
-// Test where we fail to approximate due to demanding a postdom
-// relationship between our upper bounds.
-
+// Test that we can propagate multiple region errors for closure constraints
+// where the longer region has multiple non-local lower bounds without any postdominating one.
 //@ compile-flags:-Zverbose-internals
 
 #![feature(rustc_attrs)]
@@ -13,9 +12,8 @@ use std::cell::Cell;
 // 'x: 'b
 // 'c: 'y
 //
-// we have to prove that `'x: 'y`. We currently can only approximate
-// via a postdominator -- hence we fail to choose between `'a` and
-// `'b` here and report the error in the closure.
+// we have to prove that `'x: 'y`. We find non-local lower bounds of 'x to be 'a and 'b and
+// non-local upper bound of 'y to be 'c. So we propagate `'b: 'c` and `'a: 'c`.
 fn establish_relationships<'a, 'b, 'c, F>(
     _cell_a: Cell<&'a u32>,
     _cell_b: Cell<&'b u32>,
@@ -36,14 +34,16 @@ fn demand_y<'x, 'y>(_cell_x: Cell<&'x u32>, _cell_y: Cell<&'y u32>, _y: &'y u32)
 
 #[rustc_regions]
 fn supply<'a, 'b, 'c>(cell_a: Cell<&'a u32>, cell_b: Cell<&'b u32>, cell_c: Cell<&'c u32>) {
+    //~vv ERROR lifetime may not live long enough
+    //~v ERROR lifetime may not live long enough
     establish_relationships(
         cell_a,
         cell_b,
         cell_c,
         |_outlives1, _outlives2, _outlives3, x, y| {
             // Only works if 'x: 'y:
             let p = x.get();
-            demand_y(x, y, p) //~ ERROR
+            demand_y(x, y, p)
         },
     );
 }

tests/ui/nll/closure-requirements/propagate-approximated-both-lower-bounds.stderr

@@ -0,0 +1,79 @@
+note: external requirements
+  --> $DIR/propagate-approximated-both-lower-bounds.rs:43:9
+   |
+LL |         |_outlives1, _outlives2, _outlives3, x, y| {
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: defining type: supply::{closure#0} with closure args [
+               i16,
+               for<Region(BrAnon), Region(BrAnon)> extern "rust-call" fn((std::cell::Cell<&'?1 &'^0 u32>, std::cell::Cell<&'?2 &'^0 u32>, std::cell::Cell<&'^1 &'?3 u32>, std::cell::Cell<&'^0 u32>, std::cell::Cell<&'^1 u32>)),
+               (),
+           ]
+   = note: late-bound region is '?7
+   = note: late-bound region is '?8
+   = note: late-bound region is '?4
+   = note: late-bound region is '?5
+   = note: late-bound region is '?6
+   = note: number of external vids: 7
+   = note: where '?2: '?3
+   = note: where '?1: '?3
+
+note: no external requirements
+  --> $DIR/propagate-approximated-both-lower-bounds.rs:36:1
+   |
+LL | fn supply<'a, 'b, 'c>(cell_a: Cell<&'a u32>, cell_b: Cell<&'b u32>, cell_c: Cell<&'c u32>) {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: defining type: supply
+
+error: lifetime may not live long enough
+  --> $DIR/propagate-approximated-both-lower-bounds.rs:39:5
+   |
+LL |   fn supply<'a, 'b, 'c>(cell_a: Cell<&'a u32>, cell_b: Cell<&'b u32>, cell_c: Cell<&'c u32>) {
+   |             --      -- lifetime `'c` defined here
+   |             |
+   |             lifetime `'a` defined here
+...
+LL | /     establish_relationships(
+LL | |         cell_a,
+LL | |         cell_b,
+LL | |         cell_c,
+...  |
+LL | |         },
+LL | |     );
+   | |_____^ argument requires that `'a` must outlive `'c`
+   |
+   = help: consider adding the following bound: `'a: 'c`
+   = note: requirement occurs because of the type `Cell<&'?10 u32>`, which makes the generic argument `&'?10 u32` invariant
+   = note: the struct `Cell<T>` is invariant over the parameter `T`
+   = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
+
+error: lifetime may not live long enough
+  --> $DIR/propagate-approximated-both-lower-bounds.rs:39:5
+   |
+LL |   fn supply<'a, 'b, 'c>(cell_a: Cell<&'a u32>, cell_b: Cell<&'b u32>, cell_c: Cell<&'c u32>) {
+   |                 --  -- lifetime `'c` defined here
+   |                 |
+   |                 lifetime `'b` defined here
+...
+LL | /     establish_relationships(
+LL | |         cell_a,
+LL | |         cell_b,
+LL | |         cell_c,
+...  |
+LL | |         },
+LL | |     );
+   | |_____^ argument requires that `'b` must outlive `'c`
+   |
+   = help: consider adding the following bound: `'b: 'c`
+   = note: requirement occurs because of the type `Cell<&'?10 u32>`, which makes the generic argument `&'?10 u32` invariant
+   = note: the struct `Cell<T>` is invariant over the parameter `T`
+   = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
+
+help: the following changes may resolve your lifetime errors
+   |
+   = help: add bound `'a: 'c`
+   = help: add bound `'b: 'c`
+
+error: aborting due to 2 previous errors
+

tests/ui/nll/closure-requirements/propagate-approximated-fail-no-postdom.stderr

@@ -0,0 +1,14 @@
+//@ check-pass
+// This checks that the compiler does not require that 'a: 'b. '_ has 'a and 'b as non-local
+// upper bounds, but the compiler should not propagate 'a: 'b OR 'b: 'a when checking
+// the closures. If it did, this would fail to compile, eventhough it's a valid program.
+// PR #148329 explains this in detail.
+
+struct MyTy<'x, 'a, 'b>(std::cell::Cell<(&'x &'a u8, &'x &'b u8)>);
+fn wf<T>(_: T) {}
+fn test<'a, 'b>() {
+    |_: &'a u8, x: MyTy<'_, 'a, 'b>| wf(x);
+    |x: MyTy<'_, 'a, 'b>, _: &'a u8| wf(x);
+}
+
+fn main(){}

tests/ui/regions/closure-prop-issue-104477-case1.rs

@@ -0,0 +1,12 @@
+//@ check-pass
+// This test checks that the compiler propagates outlives requirements for both
+// non-local lower bounds ['a, 'b] of '_, instead of conservatively finding a post-dominiting one
+// from those 2.
+
+struct MyTy<'a, 'b, 'x>(std::cell::Cell<(&'a &'x str, &'b &'x str)>);
+fn wf<T>(_: T) {}
+fn test<'a, 'b, 'x>() {
+    |x: MyTy<'a, 'b, '_>| wf(x);
+}
+
+fn main() {}

tests/ui/regions/closure-prop-issue-104477-case2.rs

@@ -0,0 +1,26 @@
+//@ check-pass
+// This test checks that the compiler does not propagate 'd: 'c when propagating region errors
+// for the closure argument. If it did, this would fail to compile, eventhough it's a valid program.
+// It should only propagate 'd: 'b.
+// PR #148329 explains this in detail.
+
+#[derive(Clone, Copy)]
+struct Inv<'a>(*mut &'a ());
+impl<'a> Inv<'a> {
+    fn outlived_by<'b: 'a>(self, _: Inv<'b>) {}
+}
+struct OutlivedBy<'a, 'b: 'a>(Inv<'a>, Inv<'b>);
+
+fn closure_arg<'b, 'c, 'd>(
+    _: impl for<'a> FnOnce(Inv<'a>, OutlivedBy<'a, 'b>, OutlivedBy<'a, 'c>, Inv<'d>),
+) {
+}
+fn foo<'b, 'c, 'd: 'b>() {
+    closure_arg::<'b, 'c, 'd>(|a, b, c, d| {
+        a.outlived_by(b.1);
+        a.outlived_by(c.1);
+        b.1.outlived_by(d);
+    });
+}
+
+fn main() {}