This version mostly works!

I've switched to encoding our new outlives-static constraints
with two extra parameters: the source and drain of a path in the
original constraint graph that caused a placeholder issue.

This handles all of the soundness issues, leaving 16 failing
diagnostics.

Author: amandasystems

compiler/rustc_borrowck/src/constraints/mod.rs

@@ -2,6 +2,7 @@ use std::fmt;
 use std::ops::Index;
 
 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;
@@ -68,6 +69,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
@@ -116,41 +138,67 @@ impl<'tcx> OutlivesConstraintSet<'tcx> {
         let mut added_constraints = false;
 
         for scc in sccs.all_sccs() {
-            // No point in adding 'static: 'static!
-            // This micro-optimisation makes somewhat sense
-            // because static outlives *everything*.
-            if scc == sccs.scc(fr_static) {
-                continue;
-            }
-
             let annotation = sccs.annotation(scc);
 
             // If this SCC participates in a universe violation
             // e.g. if it reaches a region with a universe smaller than
-            // the largest region reached, or if this placeholder
-            // reaches another placeholder, add a requirement that it must
-            // outlive `'static`.
-            if let Some(offending_region) = annotation.placeholder_violation(&sccs) {
-                assert!(
-                    annotation.representative != offending_region,
-                    "Attemtping to blame a constraint for itself!"
-                );
-                // Optimisation opportunity: this will add more constraints than
-                // needed for correctness, since an SCC upstream of another with
+            // the largest region reached, add a requirement that it must
+            // outlive `'static`. Here we get to know which reachable region
+            // caused the violation.
+            if let Some(to) = annotation.universe_violation() {
+                // Optimisation opportunity: this will potentially 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.
+                // outlive static. However, some of those may be useful for error
+                // reporting.
                 added_constraints = true;
-                let scc_representative_outlives_static = OutlivesConstraint {
-                    sup: annotation.representative,
-                    sub: fr_static,
-                    category: ConstraintCategory::IllegalPlaceholder(offending_region),
-                    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);
+                self.add_placeholder_violation_constraint(
+                    annotation.representative,
+                    annotation.representative,
+                    to,
+                    fr_static,
+                );
+            }
+        }
+
+        // The second kind of violation: a placeholder reaching another placeholder.
+        // OPTIMIZATION: This one is even more optimisable since it adds constraints for every
+        // placeholder in an SCC.
+        for rvid in definitions.iter_enumerated().filter_map(|(rvid, definition)| {
+            if matches!(definition.origin, NllRegionVariableOrigin::Placeholder { .. }) {
+                Some(rvid)
+            } else {
+                None
             }
+        }) {
+            let scc = sccs.scc(rvid);
+            let annotation = sccs.annotation(scc);
+
+            // 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:?}"
+            );
+            added_constraints = true;
+            self.add_placeholder_violation_constraint(
+                annotation.representative,
+                rvid,
+                other_placeholder,
+                fr_static,
+            );
         }
 
         if added_constraints {

compiler/rustc_borrowck/src/diagnostics/region_errors.rs

@@ -59,7 +59,7 @@ impl<'tcx> ConstraintDescription for ConstraintCategory<'tcx> {
             | ConstraintCategory::Boring
             | ConstraintCategory::BoringNoLocation
             | ConstraintCategory::Internal
-            | ConstraintCategory::IllegalPlaceholder(_) => "",
+            | ConstraintCategory::IllegalPlaceholder(..) => "",
         }
     }
 }

compiler/rustc_borrowck/src/region_infer/graphviz.rs

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

compiler/rustc_borrowck/src/region_infer/mod.rs

@@ -75,7 +75,7 @@ pub struct RegionTracker {
     max_universe_placeholder_reached: ReachablePlaceholder,
 
     /// The smallest universe index reachable form the nodes of this SCC.
-    min_reachable_universe: UniverseIndex,
+    min_reachable_universe: (UniverseIndex, RegionVid),
 
     /// The representative Region Variable Id for this SCC. We prefer
     /// placeholders over existentially quantified variables, otherwise
@@ -86,10 +86,10 @@ pub struct RegionTracker {
     representative_origin: RepresentativeOrigin,
 
     /// The smallest reachable placeholder from this SCC (including in it).
-    min_reachable_placeholder: Option<RegionVid>,
+    pub(crate) min_reachable_placeholder: Option<RegionVid>,
 
     /// The largest reachable placeholder from this SCC (including in it).
-    max_reachable_placeholder: Option<RegionVid>,
+    pub(crate) max_reachable_placeholder: Option<RegionVid>,
 
     /// Is there at least one placeholder in this SCC?
     contains_placeholder: bool,
@@ -142,7 +142,7 @@ impl RegionTracker {
 
         Self {
             max_universe_placeholder_reached,
-            min_reachable_universe: definition.universe,
+            min_reachable_universe: (definition.universe, rvid),
             representative: rvid,
             representative_origin,
             min_reachable_placeholder: representative_if_placeholder,
@@ -151,24 +151,6 @@ impl RegionTracker {
         }
     }
 
-    /// Return true if this SCC contains a placeholder that
-    /// reaches another placeholder, through other SCCs or within
-    /// it.
-    fn placeholder_reaches_placeholder(&self) -> bool {
-        // If min and max are different then at least two placeholders
-        // must be reachable from us. It remains to determine if and
-        // whose problem that is.
-        //
-        // If we are not a placeholder
-        // we are seeing upstream placeholders, which may be fine, or
-        // if it is a problem it's the problem for other placeholders.
-        //
-        // If we *are* a placeholder, we are reaching at least one other
-        // placeholder upstream.
-        self.contains_placeholder
-            && self.min_reachable_placeholder != self.max_reachable_placeholder
-    }
-
     /// If the representative is a placeholder, return it,
     /// otherwise return None.
     fn placeholder_representative(&self) -> Option<RegionVid> {
@@ -181,7 +163,7 @@ impl RegionTracker {
 
     /// The smallest-indexed universe reachable from and/or in this SCC.
     fn min_universe(self) -> UniverseIndex {
-        self.min_reachable_universe
+        self.min_reachable_universe.0
     }
 
     fn merge_reachable_placeholders(&mut self, other: &Self) {
@@ -215,58 +197,37 @@ impl RegionTracker {
             std::cmp::min(self.min_reachable_universe, other.min_reachable_universe);
     }
 
-    /// Returns an offending region if the annotated SCC reaches a placeholder
-    /// with a universe larger than the smallest reachable one,
-    /// or if a placeholder reaches another placeholder, `None` otherwise.
-    pub(crate) fn placeholder_violation(
-        &self,
-        sccs: &Sccs<RegionVid, ConstraintSccIndex, Self>,
-    ) -> Option<RegionVid> {
-        // Note: we arbitrarily prefer universe violations
-        // to placeholder-reaches-placeholder violations.
-        // violations.
-
-        // Case 1: a universe violation
-        if let ReachablePlaceholder::Placeholder {
+    /// Figure out if there is a universe violation going on.
+    /// This can happen in two cases: either one of our placeholders
+    /// had its universe lowered from reaching a region with a lower universe,
+    /// (in which case we blame the lower universe's region), or because we reached
+    /// a larger universe (in which case we blame the larger universe's region).
+    pub(crate) fn universe_violation(&self) -> Option<RegionVid> {
+        let ReachablePlaceholder::Placeholder {
             universe: max_reached_universe,
-            rvid: belonging_to_rvid,
+            rvid: large_u_rvid,
         } = self.max_universe_placeholder_reached
-        {
-            if self.min_universe().cannot_name(max_reached_universe) {
-                return Some(belonging_to_rvid);
-            }
-        }
-
-        // Case 2: a placeholder (in our SCC) reaches another placeholder
-        if self.placeholder_reaches_placeholder() {
-            // We know that this SCC contains at least one placeholder
-            // and that at least two placeholders are reachable from
-            // this SCC.
-            //
-            // We try to pick one that isn't in our SCC, if possible.
-            // We *always* pick one that is not equal to the representative.
-
-            // Unwrap safety: we know both these values are Some, since
-            // there are two reachable placeholders at least.
-            let min_reachable = self.min_reachable_placeholder.unwrap();
+        else {
+            return None;
+        };
 
-            if sccs.scc(min_reachable) != sccs.scc(self.representative) {
-                return Some(min_reachable);
-            }
+        if !self.min_universe().cannot_name(max_reached_universe) {
+            return None;
+        };
 
-            // Either the largest reachable placeholder is outside our SCC,
-            // or we *must* blame a placeholder in our SCC since the violation
-            // happens inside of it. It's slightly easier to always arbitrarily
-            // pick the largest one, so we do. This also nicely guarantees that
-            // we don't pick the representative, since the representative is the
-            // smallest placeholder by index in the SCC if it is a placeholder
-            // so in order for it to also be the largest reachable min would
-            // have to be equal to max, but then we would only have reached one
-            // placeholder.
-            return Some(self.max_reachable_placeholder.unwrap());
-        }
+        debug!("Universe {max_reached_universe:?} is too large for its SCC!");
+        // We originally had a large enough universe to fit all our reachable
+        // placeholders, but had it lowered because we also reached something
+        // small-universed. In this case, that's to blame!
+        let to_blame = if self.representative == large_u_rvid {
+            debug!("{:?} lowered our universe!", self.min_reachable_universe);
+            self.min_reachable_universe.1
+        } else {
+            // The problem is that we, who have a small universe, reach a large one.
+            large_u_rvid
+        };
 
-        None
+        Some(to_blame)
     }
 }
 
@@ -1948,14 +1909,18 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         // relation, redirect the search to the placeholder to blame.
         if self.is_static(to) {
             for constraint in path.iter() {
-                let ConstraintCategory::IllegalPlaceholder(culprit_r) = constraint.category else {
+                let ConstraintCategory::IllegalPlaceholder(culprit_from, culprit_to) =
+                    constraint.category
+                else {
                     continue;
                 };
 
-                debug!("{culprit_r:?} is the reason {from:?}: 'static!");
+                debug!("{culprit_from:?}: {culprit_to:?} is the reason {from:?}: 'static!");
                 // FIXME: think: this may be for transitive reasons and
                 // we may have to do this arbitrarily many times. Or may we?
-                return self.find_constraint_path_to(from, |r| r == culprit_r, false).unwrap();
+                return self
+                    .find_constraint_path_to(culprit_from, |r| r == culprit_to, false)
+                    .unwrap();
             }
         }
         // No funny business; just return the path!

compiler/rustc_middle/src/mir/query.rs

@@ -273,8 +273,9 @@ pub enum ConstraintCategory<'tcx> {
     Internal,
 
     /// An internal constraint derived from an illegal placeholder relation
-    /// to this region.
-    IllegalPlaceholder(RegionVid),
+    /// to this region. The arguments are a source -> drain of a path
+    /// that caused the problem, used when reporting errors.
+    IllegalPlaceholder(RegionVid, RegionVid),
 }
 
 #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash)]