Auto merge of rust-lang#130227 - amandasystems:remove-placeholders-completely, r=<try>

[WIP] Remove placeholders completely

This PR does shotgun surgery on borrowck to remove all special handling of placeholders, completely replacing them with a preprocessing step that rewrites placeholder leaks into constraints, removing constraint propagation of placeholders and all logic used to detect placeholder violations during error reporting. This finishes what rust-lang#123720 started.

The new method works like this:
1. during SCC construction, some information about SCC membership and reachability is retained
2. just after SCC construction, a constraint `r - (from: to_invalid) - > 'static` is added when `r` is the representative of an SCC and
   1. that SCC either has had its universe shrunk because it reaches a region with a smaller one (in which case `to_invalid` is the smallest-universed region it reaches), or if it reaches a region with a too large universe that isn't part of the SCC (in which case `to_invalid` is the region with a too large universe). In either case, `from` is also `r`.
 2.  some region `reaches` in `r`'s SCC reaches another placeholder, `reached`, in which case the added constraint is `r -> (reaches: reached) 'static`. Through clever choice of defaults (chosing minimum elements), `reached` will be `r` if at all possible.

When tracing errors for diagnostics one of these special constraints along a path are treated much like a HTTP redirect: if we are explaining `from: to` and reach an edge with `reaches: invalid` we stop the search and start following `reaches: invalid` instead. When doing this the implicit edges `x: 'static` for every region `x` are ignored, since the search would otherwise be able to cheat by going through 'static and re-find the same edge again.

A bunch of optimisations are possible:
- ~~Conservatively add constraints, e.g. one per SCC. May worsen error tracing!~~
- as a final pass, allow fusing the annotations for the SCC after adding the extra constraints to remove unnecessary information and save memory. This could be done cheaply since we already iterate over the entire set of SCCs.
- currently, if constraints are added the entire set of SCCs are recomputed. This is of course rather wasteful, and we could do better. Especially since SCCs are added in dependency order. This would require a fully separate SCC module since the dynamic SCC combo we'd need now shares almost no properties with regular SCC computation. Given that this is meant to be a temporary work-around, that seems like too much work.

There are a bunch of rather nice bonuses:
- We now don't need to expose region indices in `MirTypeckRegionConstraints` to the entire crate. The only entry point is `placeholder_region()` so correctness of the indices is now guaranteed
- A lot of things that were previously iterations over lists is now a single lookup
- The constraint graph search functions are simple and at least one of them can now take a proper region as target rather than a predicate function. The only case that needs the predicate argument  to `find_constraint_path_to()` is `find_sub_region_live_at()`, which may or may not be possible to work around.

 r​? nikomatsakis

Author: bors

compiler/rustc_borrowck/src/constraints/graph.rs

@@ -106,15 +106,17 @@ impl<D: ConstraintGraphDirection> ConstraintGraph<D> {
     }
 
     /// Given a region `R`, iterate over all constraints `R: R1`.
+    /// if `static_region` is `None`, do not yield implicit
+    /// `'static -> a` edges.
     pub(crate) fn outgoing_edges<'a, 'tcx>(
         &'a self,
         region_sup: RegionVid,
         constraints: &'a OutlivesConstraintSet<'tcx>,
-        static_region: RegionVid,
+        static_region: Option<RegionVid>,
     ) -> Edges<'a, 'tcx, D> {
         //if this is the `'static` region and the graph's direction is normal,
         //then setup the Edges iterator to return all regions #53178
-        if region_sup == static_region && D::is_normal() {
+        if Some(region_sup) == static_region && D::is_normal() {
             Edges {
                 graph: self,
                 constraints,
@@ -135,7 +137,7 @@ pub(crate) struct Edges<'a, 'tcx, D: ConstraintGraphDirection> {
     constraints: &'a OutlivesConstraintSet<'tcx>,
     pointer: Option<OutlivesConstraintIndex>,
     next_static_idx: Option<usize>,
-    static_region: RegionVid,
+    static_region: Option<RegionVid>,
 }
 
 impl<'a, 'tcx, D: ConstraintGraphDirection> Iterator for Edges<'a, 'tcx, D> {
@@ -153,8 +155,12 @@ impl<'a, 'tcx, D: ConstraintGraphDirection> Iterator for Edges<'a, 'tcx, D> {
                 Some(next_static_idx + 1)
             };
 
+            let Some(static_region) = self.static_region else {
+                return None;
+            };
+
             Some(OutlivesConstraint {
-                sup: self.static_region,
+                sup: static_region,
                 sub: next_static_idx.into(),
                 locations: Locations::All(DUMMY_SP),
                 span: DUMMY_SP,
@@ -194,7 +200,11 @@ impl<'a, 'tcx, D: ConstraintGraphDirection> RegionGraph<'a, 'tcx, D> {
     /// there exists a constraint `R: R1`.
     pub(crate) fn outgoing_regions(&self, region_sup: RegionVid) -> Successors<'a, 'tcx, D> {
         Successors {
-            edges: self.constraint_graph.outgoing_edges(region_sup, self.set, self.static_region),
+            edges: self.constraint_graph.outgoing_edges(
+                region_sup,
+                self.set,
+                Some(self.static_region),
+            ),
         }
     }
 }

compiler/rustc_borrowck/src/constraints/mod.rs

@@ -1,7 +1,9 @@
 use std::fmt;
 use std::ops::Index;
 
+use rustc_data_structures::fx::FxHashSet;
 use rustc_index::{IndexSlice, IndexVec};
+use rustc_infer::infer::NllRegionVariableOrigin;
 use rustc_middle::mir::ConstraintCategory;
 use rustc_middle::ty::{RegionVid, TyCtxt, VarianceDiagInfo};
 use rustc_span::Span;
@@ -69,6 +71,27 @@ impl<'tcx> OutlivesConstraintSet<'tcx> {
         })
     }
 
+    /// There is a placeholder violation; add a requirement
+    /// that some SCC outlive static and explain which region
+    /// reaching which other region caused that.
+    fn add_placeholder_violation_constraint(
+        &mut self,
+        outlives_static: RegionVid,
+        blame_from: RegionVid,
+        blame_to: RegionVid,
+        fr_static: RegionVid,
+    ) {
+        self.push(OutlivesConstraint {
+            sup: outlives_static,
+            sub: fr_static,
+            category: ConstraintCategory::IllegalPlaceholder(blame_from, blame_to),
+            locations: Locations::All(rustc_span::DUMMY_SP),
+            span: rustc_span::DUMMY_SP,
+            variance_info: VarianceDiagInfo::None,
+            from_closure: false,
+        });
+    }
+
     /// This method handles Universe errors by rewriting the constraint
     /// graph. For each strongly connected component in the constraint
     /// graph such that there is a series of constraints
@@ -79,7 +102,7 @@ impl<'tcx> OutlivesConstraintSet<'tcx> {
     /// eventually go away.
     ///
     /// For a more precise definition, see the documentation for
-    /// [`RegionTracker::has_incompatible_universes()`].
+    /// [`RegionTracker`] and its methods!.
     ///
     /// This edge case used to be handled during constraint propagation
     /// by iterating over the strongly connected components in the constraint
@@ -112,44 +135,83 @@ impl<'tcx> OutlivesConstraintSet<'tcx> {
         let fr_static = universal_regions.fr_static;
         let sccs = self.compute_sccs(fr_static, definitions);
 
-        // Changed to `true` if we added any constraints to `self` and need to
-        // recompute SCCs.
-        let mut added_constraints = false;
+        // Is this SCC already outliving static directly or transitively?
+        let mut outlives_static = FxHashSet::default();
 
-        for scc in sccs.all_sccs() {
-            // No point in adding 'static: 'static!
-            // This micro-optimisation makes somewhat sense
-            // because static outlives *everything*.
+        for (scc, annotation) in sccs.all_annotations() {
             if scc == sccs.scc(fr_static) {
+                // No use adding 'static: 'static.
                 continue;
             }
 
-            let annotation = sccs.annotation(scc);
-
-            // If this SCC participates in a universe violation,
+            // If this SCC participates in a universe violation
             // e.g. if it reaches a region with a universe smaller than
             // the largest region reached, add a requirement that it must
-            // outlive `'static`.
-            if annotation.has_incompatible_universes() {
-                // Optimisation opportunity: this will add more constraints than
-                // needed for correctness, since an SCC upstream of another with
-                // a universe violation will "infect" its downstream SCCs to also
-                // outlive static.
-                added_constraints = true;
-                let scc_representative_outlives_static = OutlivesConstraint {
-                    sup: annotation.representative,
-                    sub: fr_static,
-                    category: ConstraintCategory::IllegalUniverse,
-                    locations: Locations::All(rustc_span::DUMMY_SP),
-                    span: rustc_span::DUMMY_SP,
-                    variance_info: VarianceDiagInfo::None,
-                    from_closure: false,
-                };
-                self.push(scc_representative_outlives_static);
+            // outlive `'static`. Here we get to know which reachable region
+            // caused the violation.
+            if let Some(to) = annotation.universe_violation() {
+                outlives_static.insert(scc);
+                self.add_placeholder_violation_constraint(
+                    annotation.representative,
+                    annotation.representative,
+                    to,
+                    fr_static,
+                );
+            }
+        }
+
+        // Note: it's possible to sort this iterator by SCC and get dependency order,
+        // which makes it easy to only add only one constraint per future cycle.
+        // However, this worsens diagnostics and requires iterating over
+        // all successors to determine if we outlive static transitively,
+        // a cost you pay even if you have no placeholders.
+        let placeholders_and_sccs =
+            definitions.iter_enumerated().filter_map(|(rvid, definition)| {
+                if matches!(definition.origin, NllRegionVariableOrigin::Placeholder { .. }) {
+                    Some((sccs.scc(rvid), rvid))
+                } else {
+                    None
+                }
+            });
+
+        // The second kind of violation: a placeholder reaching another placeholder.
+        for (scc, rvid) in placeholders_and_sccs {
+            let annotation = sccs.annotation(scc);
+
+            if sccs.scc(fr_static) == scc || outlives_static.contains(&scc) {
+                debug!("{:?} already outlives (or is) static", annotation.representative);
+                continue;
             }
+
+            // Unwrap safety: since this is our SCC it must contain us, which is
+            // at worst min AND max, but it has at least one or there is a bug.
+            let min = annotation.min_reachable_placeholder.unwrap();
+            let max = annotation.max_reachable_placeholder.unwrap();
+
+            // Good path: Nothing to see here, at least no other placeholders!
+            if min == max {
+                continue;
+            }
+
+            // Bad path: figure out who we illegally reached.
+            // Note that this will prefer the representative if it is a
+            // placeholder, since the representative has the smallest index!
+            let other_placeholder = if min != rvid { min } else { max };
+
+            debug!(
+                "Placeholder {rvid:?} of SCC {scc:?} reaches other placeholder {other_placeholder:?}"
+            );
+            outlives_static.insert(scc);
+            self.add_placeholder_violation_constraint(
+                annotation.representative,
+                rvid,
+                other_placeholder,
+                fr_static,
+            );
         }
 
-        if added_constraints {
+        if !outlives_static.is_empty() {
+            debug!("The following SCCs had :'static constraints added: {:?}", outlives_static);
             // We changed the constraint set and so must recompute SCCs.
             self.compute_sccs(fr_static, definitions)
         } else {

compiler/rustc_borrowck/src/diagnostics/bound_region_errors.rs

@@ -181,24 +181,12 @@ trait TypeOpInfo<'tcx> {
             bound: placeholder.bound,
         });
 
-        let error_region =
-            if let RegionElement::PlaceholderRegion(error_placeholder) = error_element {
-                let adjusted_universe =
-                    error_placeholder.universe.as_u32().checked_sub(base_universe.as_u32());
-                adjusted_universe.map(|adjusted| {
-                    ty::Region::new_placeholder(tcx, ty::Placeholder {
-                        universe: adjusted.into(),
-                        bound: error_placeholder.bound,
-                    })
-                })
-            } else {
-                None
-            };
-
         debug!(?placeholder_region);
 
+        // FIXME: this is obviously weird; this whole argument now does nothing and maybe
+        // it should?
         let span = cause.span;
-        let nice_error = self.nice_error(mbcx, cause, placeholder_region, error_region);
+        let nice_error = self.nice_error(mbcx, cause, placeholder_region, None);
 
         debug!(?nice_error);
 

compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs

@@ -488,7 +488,7 @@ impl<'tcx> MirBorrowckCtxt<'_, '_, 'tcx> {
         let (blame_constraint, extra_info) = self.regioncx.best_blame_constraint(
             borrow_region,
             NllRegionVariableOrigin::FreeRegion,
-            |r| self.regioncx.provides_universal_region(r, borrow_region, outlived_region),
+            outlived_region,
         );
         let BlameConstraint { category, from_closure, cause, .. } = blame_constraint;
 

compiler/rustc_borrowck/src/diagnostics/region_errors.rs

@@ -61,7 +61,7 @@ impl<'tcx> ConstraintDescription for ConstraintCategory<'tcx> {
             | ConstraintCategory::Boring
             | ConstraintCategory::BoringNoLocation
             | ConstraintCategory::Internal
-            | ConstraintCategory::IllegalUniverse => "",
+            | ConstraintCategory::IllegalPlaceholder(..) => "",
         }
     }
 }
@@ -206,52 +206,35 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
         &self,
         diag: &mut Diag<'_>,
         lower_bound: RegionVid,
-    ) {
+    ) -> Option<()> {
         let mut suggestions = vec![];
         let hir = self.infcx.tcx.hir();
 
-        // find generic associated types in the given region 'lower_bound'
-        let gat_id_and_generics = self
-            .regioncx
-            .placeholders_contained_in(lower_bound)
-            .map(|placeholder| {
-                if let Some(id) = placeholder.bound.kind.get_id()
-                    && let Some(placeholder_id) = id.as_local()
-                    && let gat_hir_id = self.infcx.tcx.local_def_id_to_hir_id(placeholder_id)
-                    && let Some(generics_impl) = self
-                        .infcx
-                        .tcx
-                        .parent_hir_node(self.infcx.tcx.parent_hir_id(gat_hir_id))
-                        .generics()
-                {
-                    Some((gat_hir_id, generics_impl))
-                } else {
-                    None
-                }
-            })
-            .collect::<Vec<_>>();
-        debug!(?gat_id_and_generics);
-
         // find higher-ranked trait bounds bounded to the generic associated types
+        let scc = self.regioncx.constraint_sccs().scc(lower_bound);
+        let placeholder: ty::PlaceholderRegion = self.regioncx.placeholder_representative(scc)?;
+        let placeholder_id = placeholder.bound.kind.get_id()?.as_local()?;
+        let gat_hir_id = self.infcx.tcx.local_def_id_to_hir_id(placeholder_id);
+        let generics_impl =
+            self.infcx.tcx.parent_hir_node(self.infcx.tcx.parent_hir_id(gat_hir_id)).generics()?;
+
         let mut hrtb_bounds = vec![];
-        gat_id_and_generics.iter().flatten().for_each(|(gat_hir_id, generics)| {
-            for pred in generics.predicates {
-                let BoundPredicate(WhereBoundPredicate { bound_generic_params, bounds, .. }) = pred
-                else {
-                    continue;
-                };
-                if bound_generic_params
-                    .iter()
-                    .rfind(|bgp| self.infcx.tcx.local_def_id_to_hir_id(bgp.def_id) == *gat_hir_id)
-                    .is_some()
-                {
-                    for bound in *bounds {
-                        hrtb_bounds.push(bound);
-                    }
+
+        for pred in generics_impl.predicates {
+            let BoundPredicate(WhereBoundPredicate { bound_generic_params, bounds, .. }) = pred
+            else {
+                continue;
+            };
+            if bound_generic_params
+                .iter()
+                .rfind(|bgp| self.infcx.tcx.local_def_id_to_hir_id(bgp.def_id) == gat_hir_id)
+                .is_some()
+            {
+                for bound in *bounds {
+                    hrtb_bounds.push(bound);
                 }
             }
-        });
-        debug!(?hrtb_bounds);
+        }
 
         hrtb_bounds.iter().for_each(|bound| {
             let Trait(PolyTraitRef { trait_ref, span: trait_span, .. }, _) = bound else {
@@ -300,6 +283,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
                 Applicability::MaybeIncorrect,
             );
         }
+        Some(())
     }
 
     /// Produces nice borrowck error diagnostics for all the errors collected in `nll_errors`.
@@ -446,9 +430,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
         debug!("report_region_error(fr={:?}, outlived_fr={:?})", fr, outlived_fr);
 
         let (blame_constraint, extra_info) =
-            self.regioncx.best_blame_constraint(fr, fr_origin, |r| {
-                self.regioncx.provides_universal_region(r, fr, outlived_fr)
-            });
+            self.regioncx.best_blame_constraint(fr, fr_origin, outlived_fr);
         let BlameConstraint { category, cause, variance_info, .. } = blame_constraint;
 
         debug!("report_region_error: category={:?} {:?} {:?}", category, cause, variance_info);

compiler/rustc_borrowck/src/nll.rs

@@ -122,15 +122,13 @@ pub(crate) fn compute_regions<'a, 'tcx>(
     // base constraints generated by the type-check.
     let var_origins = infcx.get_region_var_origins();
     let MirTypeckRegionConstraints {
-        placeholder_indices,
-        placeholder_index_to_region: _,
         liveness_constraints,
         mut outlives_constraints,
         mut member_constraints,
         universe_causes,
         type_tests,
+        ..
     } = constraints;
-    let placeholder_indices = Rc::new(placeholder_indices);
 
     // If requested, emit legacy polonius facts.
     polonius::emit_facts(
@@ -158,7 +156,6 @@ pub(crate) fn compute_regions<'a, 'tcx>(
         infcx,
         var_origins,
         universal_regions,
-        placeholder_indices,
         universal_region_relations,
         outlives_constraints,
         member_constraints,

compiler/rustc_borrowck/src/region_infer/graphviz.rs

@@ -12,6 +12,9 @@ use rustc_middle::ty::UniverseIndex;
 use super::*;
 
 fn render_outlives_constraint(constraint: &OutlivesConstraint<'_>) -> String {
+    if let ConstraintCategory::IllegalPlaceholder(from, to) = constraint.category {
+        return format!("b/c {from:?}: {to:?}");
+    }
     match constraint.locations {
         Locations::All(_) => "All(...)".to_string(),
         Locations::Single(loc) => format!("{loc:?}"),