Auto merge of rust-lang#117560 - lqd:issue-117146, r=matthewjasper

Compute polonius loan scopes over the region graph

In issue rust-lang#117146 a loan flows into an SCC containing a placeholder, and whose representative is an existential region. Since we currently compute loan scopes by looking at SCCs and their representatives only, polonius would compute kill points for this loan here whereas NLLs would not of course.

There are a few ways to fix this:
- don't try to be efficient by doing the computation over SCCs, and simply look for free regions and placeholders in the successors of the issuing region.
- change how the SCC representatives are picked, biasing towards placeholders over existential regions. They *shouldn't* matter much, but some downstream code may subtly depend on the current scheme (though no tests fail if we do such a change). This is for unrelated reasons also the way rust-lang#116891 changes the representative computation. So that PR would also fix issue rust-lang#117146.
- try to remove placeholders from the main path, and contain them to a pre-pass + a post-pass kind of polonius leak check. If possible, it would fix this issue by turning an outlives constraints to a placeholder into a constraint to 'static. This should also fix the issue, as the representative would be the free region in the SCC. We want to prototype this change to see if it's possible to try to simplify the borrowck main path from having to deal with placeholders and higher-ranked subtyping 🤞.

I'd like to take advantage of fuzzing and a crater run sooner rather than later, so that we grow more confidence that the 2 models are indeed equivalent empirically. Therefore this PR implements option 1 to fix the issue now.

We can take care of efficiency later after validation, and once we implement option 3 (which could also impact option 2 and that associated PR, maybe the lack of placeholders could remove the need to change the representative computation) to traverse SCCs and their representative again.

(Or we maybe will have some kind of naive position-dependent outlives propagation by then and this code would have been changed)

Fixes rust-lang#117146.

r? `@matthewjasper`

compiler/rustc_borrowck/src/dataflow.rs

@@ -273,11 +273,10 @@ impl<'tcx> PoloniusOutOfScopePrecomputer<'_, 'tcx> {
     ) {
         let sccs = self.regioncx.constraint_sccs();
         let universal_regions = self.regioncx.universal_regions();
-        let issuing_region_scc = sccs.scc(issuing_region);
 
         // We first handle the cases where the loan doesn't go out of scope, depending on the issuing
         // region's successors.
-        for scc in sccs.depth_first_search(issuing_region_scc) {
+        for successor in self.regioncx.region_graph().depth_first_search(issuing_region) {
             // 1. Via applied member constraints
             //
             // The issuing region can flow into the choice regions, and they are either:
@@ -290,6 +289,7 @@ impl<'tcx> PoloniusOutOfScopePrecomputer<'_, 'tcx> {
             // For additional insurance via fuzzing and crater, we verify that the constraint's min
             // choice indeed escapes the function. In the future, we could e.g. turn this check into
             // a debug assert and early return as an optimization.
+            let scc = sccs.scc(successor);
             for constraint in self.regioncx.applied_member_constraints(scc) {
                 if universal_regions.is_universal_region(constraint.min_choice) {
                     return;
@@ -300,7 +300,7 @@ impl<'tcx> PoloniusOutOfScopePrecomputer<'_, 'tcx> {
             //
             // If the issuing region outlives such a region, its loan escapes the function and
             // cannot go out of scope. We can early return.
-            if self.regioncx.scc_is_live_at_all_points(scc) {
+            if self.regioncx.is_region_live_at_all_points(successor) {
                 return;
             }
         }

compiler/rustc_borrowck/src/region_infer/mod.rs

@@ -22,11 +22,10 @@ use rustc_middle::traits::ObligationCauseCode;
 use rustc_middle::ty::{self, RegionVid, Ty, TyCtxt, TypeFoldable, TypeVisitableExt};
 use rustc_span::Span;
 
+use crate::constraints::graph::{self, NormalConstraintGraph, RegionGraph};
 use crate::dataflow::BorrowIndex;
 use crate::{
-    constraints::{
-        graph::NormalConstraintGraph, ConstraintSccIndex, OutlivesConstraint, OutlivesConstraintSet,
-    },
+    constraints::{ConstraintSccIndex, OutlivesConstraint, OutlivesConstraintSet},
     diagnostics::{RegionErrorKind, RegionErrors, UniverseInfo},
     member_constraints::{MemberConstraintSet, NllMemberConstraintIndex},
     nll::PoloniusOutput,
@@ -2293,19 +2292,21 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         self.constraint_sccs.as_ref()
     }
 
-    /// Returns whether the given SCC is live at all points: whether the representative is a
+    /// Access to the region graph, built from the outlives constraints.
+    pub(crate) fn region_graph(&self) -> RegionGraph<'_, 'tcx, graph::Normal> {
+        self.constraint_graph.region_graph(&self.constraints, self.universal_regions.fr_static)
+    }
+
+    /// Returns whether the given region is considered live at all points: whether it is a
     /// placeholder or a free region.
-    pub(crate) fn scc_is_live_at_all_points(&self, scc: ConstraintSccIndex) -> bool {
+    pub(crate) fn is_region_live_at_all_points(&self, region: RegionVid) -> bool {
         // FIXME: there must be a cleaner way to find this information. At least, when
         // higher-ranked subtyping is abstracted away from the borrowck main path, we'll only
         // need to check whether this is a universal region.
-        let representative = self.scc_representatives[scc];
-        let origin = self.var_infos[representative].origin;
+        let origin = self.region_definition(region).origin;
         let live_at_all_points = matches!(
             origin,
-            RegionVariableOrigin::Nll(
-                NllRegionVariableOrigin::Placeholder(_) | NllRegionVariableOrigin::FreeRegion
-            )
+            NllRegionVariableOrigin::Placeholder(_) | NllRegionVariableOrigin::FreeRegion
         );
         live_at_all_points
     }

tests/ui/nll/polonius/location-insensitive-scopes-issue-117146.nll.stderr

@@ -0,0 +1,40 @@
+error[E0597]: `a` does not live long enough
+  --> $DIR/location-insensitive-scopes-issue-117146.rs:10:18
+   |
+LL |     let b = |_| &a;
+   |             --- -^
+   |             |   ||
+   |             |   |borrowed value does not live long enough
+   |             |   returning this value requires that `a` is borrowed for `'static`
+   |             value captured here
+...
+LL | }
+   | - `a` dropped here while still borrowed
+   |
+note: due to current limitations in the borrow checker, this implies a `'static` lifetime
+  --> $DIR/location-insensitive-scopes-issue-117146.rs:20:22
+   |
+LL | fn bad<F: Fn(&()) -> &()>(_: F) {}
+   |                      ^^^
+
+error: implementation of `Fn` is not general enough
+  --> $DIR/location-insensitive-scopes-issue-117146.rs:13:5
+   |
+LL |     bad(&b);
+   |     ^^^^^^^ implementation of `Fn` is not general enough
+   |
+   = note: closure with signature `fn(&'2 ()) -> &()` must implement `Fn<(&'1 (),)>`, for any lifetime `'1`...
+   = note: ...but it actually implements `Fn<(&'2 (),)>`, for some specific lifetime `'2`
+
+error: implementation of `FnOnce` is not general enough
+  --> $DIR/location-insensitive-scopes-issue-117146.rs:13:5
+   |
+LL |     bad(&b);
+   |     ^^^^^^^ implementation of `FnOnce` is not general enough
+   |
+   = note: closure with signature `fn(&'2 ()) -> &()` must implement `FnOnce<(&'1 (),)>`, for any lifetime `'1`...
+   = note: ...but it actually implements `FnOnce<(&'2 (),)>`, for some specific lifetime `'2`
+
+error: aborting due to 3 previous errors
+
+For more information about this error, try `rustc --explain E0597`.

tests/ui/nll/polonius/location-insensitive-scopes-issue-117146.polonius.stderr

@@ -0,0 +1,40 @@
+error[E0597]: `a` does not live long enough
+  --> $DIR/location-insensitive-scopes-issue-117146.rs:10:18
+   |
+LL |     let b = |_| &a;
+   |             --- -^
+   |             |   ||
+   |             |   |borrowed value does not live long enough
+   |             |   returning this value requires that `a` is borrowed for `'static`
+   |             value captured here
+...
+LL | }
+   | - `a` dropped here while still borrowed
+   |
+note: due to current limitations in the borrow checker, this implies a `'static` lifetime
+  --> $DIR/location-insensitive-scopes-issue-117146.rs:20:22
+   |
+LL | fn bad<F: Fn(&()) -> &()>(_: F) {}
+   |                      ^^^
+
+error: implementation of `Fn` is not general enough
+  --> $DIR/location-insensitive-scopes-issue-117146.rs:13:5
+   |
+LL |     bad(&b);
+   |     ^^^^^^^ implementation of `Fn` is not general enough
+   |
+   = note: closure with signature `fn(&'2 ()) -> &()` must implement `Fn<(&'1 (),)>`, for any lifetime `'1`...
+   = note: ...but it actually implements `Fn<(&'2 (),)>`, for some specific lifetime `'2`
+
+error: implementation of `FnOnce` is not general enough
+  --> $DIR/location-insensitive-scopes-issue-117146.rs:13:5
+   |
+LL |     bad(&b);
+   |     ^^^^^^^ implementation of `FnOnce` is not general enough
+   |
+   = note: closure with signature `fn(&'2 ()) -> &()` must implement `FnOnce<(&'1 (),)>`, for any lifetime `'1`...
+   = note: ...but it actually implements `FnOnce<(&'2 (),)>`, for some specific lifetime `'2`
+
+error: aborting due to 3 previous errors
+
+For more information about this error, try `rustc --explain E0597`.

tests/ui/nll/polonius/location-insensitive-scopes-issue-117146.rs

@@ -0,0 +1,20 @@
+// This is a non-regression test for issue #117146, where NLL and `-Zpolonius=next` computed
+// different loan scopes when a region flowed into an SCC whose representative was an existential
+// region.
+
+// revisions: nll polonius
+// [polonius] compile-flags: -Zpolonius=next
+
+fn main() {
+    let a = ();
+    let b = |_| &a;
+    //[nll]~^ ERROR `a` does not live long enough
+    //[polonius]~^^ ERROR `a` does not live long enough
+    bad(&b);
+    //[nll]~^ ERROR implementation of `Fn`
+    //[nll]~| ERROR implementation of `FnOnce`
+    //[polonius]~^^^ ERROR implementation of `Fn`
+    //[polonius]~| ERROR implementation of `FnOnce`
+}
+
+fn bad<F: Fn(&()) -> &()>(_: F) {}