Pick the first small-universed region, this fixes two bugs

amandasystems · amandasystems · commit d08cb3def682 · 2025-01-23T16:21:00.000+01:00
diff --git a/compiler/rustc_borrowck/src/eliminate_placeholders.rs b/compiler/rustc_borrowck/src/eliminate_placeholders.rs
@@ -5,6 +5,8 @@
 //! This logic is provisional and should be removed once the trait
 //! solver can handle this kind of constraint.
 
+use std::collections::VecDeque;
+
 use rustc_data_structures::fx::FxHashSet;
 use rustc_data_structures::graph::scc::{self, Sccs};
 use rustc_index::IndexVec;
@@ -13,6 +15,7 @@ use rustc_infer::infer::region_constraints::{VarInfos, VerifyBound};
 use rustc_middle::ty::{Region, RegionVid, TyCtxt, UniverseIndex};
 use tracing::{debug, instrument};
 
+use crate::constraints::graph::{ConstraintGraph, Normal};
 use crate::constraints::{ConstraintSccIndex, OutlivesConstraintSet};
 use crate::diagnostics::RegionErrorKind;
 use crate::member_constraints::MemberConstraintSet;
@@ -105,9 +108,15 @@ struct RegionTracker {
 
 impl scc::Annotation for RegionTracker {
     fn merge_scc(self, other: Self) -> Self {
+        debug!("{:?} << {:?}", self.representative, other.representative);
+        let min_universe = if other.min_universe.0 < self.min_universe.0 {
+            other.min_universe
+        } else {
+            self.min_universe
+        };
         Self {
             reachable_placeholders: self.reachable_placeholders.merge(other.reachable_placeholders),
-            min_universe: self.min_universe.min(other.min_universe),
+            min_universe,
             representative: self.representative.merge_scc(other.representative),
         }
     }
@@ -150,39 +159,6 @@ impl RegionTracker {
         self.min_universe.0
     }
 
-    /// 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).
-    fn universe_violation(&self) -> Option<RegionVid> {
-        let PlaceholderReachability::Placeholders { max_universe: (max_u, max_u_rvid), .. } =
-            self.reachable_placeholders
-        else {
-            return None;
-        };
-
-        let (min_u, min_u_rvid) = self.min_universe;
-
-        if min_u.can_name(max_u) {
-            return None;
-        }
-
-        debug!("Universe {max_u:?} is too large for its SCC!");
-        let to_blame = if self.representative.rvid() == max_u_rvid {
-            // 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!
-            debug!("{min_u_rvid:?} lowered our universe to {min_u:?}");
-            min_u_rvid
-        } else {
-            // The problem is that we, who have a small universe, reach a large one.
-            max_u_rvid
-        };
-
-        Some(to_blame)
-    }
-
     /// Determine if this SCC reaches a placeholder that isn't `placeholder_rvid`,
     /// returning it if that is the case. This prefers the placeholder with the
     /// smallest region variable ID.
@@ -250,6 +226,7 @@ impl scc::Annotations<RegionVid, ConstraintSccIndex, Representative>
     }
 }
 
+#[instrument(skip(definitions, sccs, annotations), ret, level = "debug")]
 fn find_placeholder_mismatch_errors<'tcx>(
     definitions: &IndexVec<RegionVid, RegionDefinition<'tcx>>,
     sccs: &Sccs<RegionVid, ConstraintSccIndex>,
@@ -397,7 +374,7 @@ pub(crate) fn rewrite_higher_kinded_outlives_as_constraints<'tcx>(
     let sccs = outlives_constraints.compute_sccs(fr_static, definitions.len(), &mut annotations);
 
     let outlives_static =
-        rewrite_outlives(&sccs, &annotations, fr_static, &mut outlives_constraints);
+        rewrite_outlives(&sccs, &annotations, fr_static, &mut outlives_constraints, &definitions);
 
     let placeholder_errors = find_placeholder_mismatch_errors(&definitions, &sccs, &annotations);
 
@@ -467,35 +444,112 @@ fn rewrite_outlives<'tcx>(
     annotations: &SccAnnotations<'_, '_, RegionTracker>,
     fr_static: RegionVid,
     outlives_constraints: &mut OutlivesConstraintSet<'tcx>,
+    definitions: &IndexVec<RegionVid, RegionDefinition<'tcx>>,
 ) -> FxHashSet<ConstraintSccIndex> {
     // Is this SCC already outliving static directly or transitively?
     let mut outlives_static = FxHashSet::default();
 
+    let mut memoised_constraint_graph: Option<ConstraintGraph<Normal>> = None;
+
     for scc in sccs.all_sccs() {
         let annotation: RegionTracker = annotations.scc_to_annotation[scc];
         if scc == sccs.scc(fr_static) {
             // No use adding 'static: 'static.
             continue;
         }
 
-        // 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`. Here we get to know which reachable region
-        // caused the violation.
-        if let Some(to) = annotation.universe_violation() {
-            outlives_static.insert(scc);
-            outlives_constraints.add_placeholder_violation_constraint(
-                annotation.representative_rvid(),
-                annotation.representative_rvid(),
-                to,
+        // Figure out if there is a universe violation in this SCC.
+        // 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).
+        let PlaceholderReachability::Placeholders { max_universe: (max_u, max_u_rvid), .. } =
+            annotation.reachable_placeholders
+        else {
+            continue;
+        };
+
+        let (min_u, _) = annotation.min_universe;
+
+        if min_u.can_name(max_u) {
+            continue;
+        }
+
+        debug!("Universe {max_u:?} is too large for its SCC!");
+        let blame_to = if annotation.representative.rvid() == max_u_rvid {
+            // We originally had a large enough universe to fit all our reachable
+            // placeholders, but had it lowered because we also absorbed something
+            // small-universed. In this case, that's to blame!
+            let small_universed_rvid = find_region(
+                outlives_constraints,
+                memoised_constraint_graph
+                    .get_or_insert_with(|| outlives_constraints.graph(definitions.len())),
+                definitions,
+                max_u_rvid,
+                |r: RegionVid| definitions[r].universe == min_u,
                 fr_static,
             );
-        }
+            debug!("{small_universed_rvid:?} lowered our universe to {min_u:?}");
+            small_universed_rvid
+        } else {
+            // The problem is that we, who have a small universe, reach a large one.
+            max_u_rvid
+        };
+
+        outlives_static.insert(scc);
+        outlives_constraints.add_placeholder_violation_constraint(
+            annotation.representative_rvid(),
+            annotation.representative_rvid(),
+            blame_to,
+            fr_static,
+        );
     }
     outlives_static
 }
 
+/// Find a region matching a predicate in a set of constraints, using BFS.
+// FIXME(amandasystems) this is at least partially duplicated code to the constraint search in `region_infer`.
+// It's probably also very expensive.
+fn find_region<'tcx>(
+    constraints: &OutlivesConstraintSet<'tcx>,
+    graph: &ConstraintGraph<Normal>,
+    definitions: &IndexVec<RegionVid, RegionDefinition<'tcx>>,
+    start_region: RegionVid,
+    target_test: impl Fn(RegionVid) -> bool,
+    fr_static: RegionVid,
+) -> RegionVid {
+    #[derive(Clone, PartialEq, Eq, Debug)]
+    enum Trace {
+        StartRegion,
+        NotVisited,
+        Visited,
+    }
+
+    let mut context = IndexVec::from_elem(Trace::NotVisited, definitions);
+    context[start_region] = Trace::StartRegion;
+
+    let mut deque = VecDeque::new();
+    deque.push_back(start_region);
+
+    while let Some(r) = deque.pop_front() {
+        if target_test(r) {
+            return r;
+        }
+
+        let outgoing_edges_from_graph = graph.outgoing_edges(r, constraints, Some(fr_static));
+
+        for constraint in outgoing_edges_from_graph {
+            debug_assert_eq!(constraint.sup, r);
+            let sub_region = constraint.sub;
+            if let Trace::NotVisited = context[sub_region] {
+                context[sub_region] = Trace::Visited;
+                deque.push_back(sub_region);
+            }
+        }
+    }
+    unreachable!("Should have found something!");
+}
+
 #[instrument(skip(sccs, scc_annotations, universal_regions), ret)]
 fn bound_has_universe_violation<'t>(
     bound: &VerifyBound<'t>,
diff --git a/compiler/rustc_borrowck/src/nll.rs b/compiler/rustc_borrowck/src/nll.rs
@@ -210,6 +210,10 @@ pub(crate) fn compute_regions<'a, 'tcx>(
         for error in placeholder_errors.into_iter() {
             nll_errors.push(error);
         }
+    } else {
+        debug!(
+            "Errors already reported, skipping these placeholder errors: {placeholder_errors:?}"
+        );
     }
 
     if let Some(guar) = nll_errors.has_errors() {
diff --git a/compiler/rustc_borrowck/src/region_infer/mod.rs b/compiler/rustc_borrowck/src/region_infer/mod.rs
@@ -742,6 +742,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
 
             // Type-test failed. Report the error.
             let erased_generic_kind = infcx.tcx.erase_regions(type_test.generic_kind);
+
             let original_lower_bound = type_test
                 .original
                 .as_ref()
@@ -1757,6 +1758,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
             | NllRegionVariableOrigin::Existential { from_forall: true } => false,
         };
 
+        // FIXME(amandasystems) This closure is tooe big to inline, it should be a method if plausible.
         // To pick a constraint to blame, we organize constraints by how interesting we expect them
         // to be in diagnostics, then pick the most interesting one closest to either the source or
         // the target on our constraint path.
@@ -1773,7 +1775,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
                 constraint.category
             };
 
-            match category {
+            let interest = match category {
                 // Returns usually provide a type to blame and have specially written diagnostics,
                 // so prioritize them.
                 ConstraintCategory::Return(_) => 0,
@@ -1824,10 +1826,14 @@ impl<'tcx> RegionInferenceContext<'tcx> {
                 // `BoringNoLocation` constraints can point to user-written code, but are less
                 // specific, and are not used for relations that would make sense to blame.
                 ConstraintCategory::BoringNoLocation => 6,
+                ConstraintCategory::IllegalPlaceholder(_, _) => 7,
                 // Do not blame internal constraints.
-                ConstraintCategory::Internal => 7,
-                ConstraintCategory::IllegalPlaceholder(_, _) => 8,
-            }
+                ConstraintCategory::Internal => 8,
+            };
+
+            debug!("constraint {constraint:?} category: {category:?}, interest: {interest:?}");
+
+            interest
         };
 
         let best_choice = if blame_source {
diff --git a/tests/ui/higher-ranked/higher-ranked-lifetime-error.stderr b/tests/ui/higher-ranked/higher-ranked-lifetime-error.stderr
@@ -2,7 +2,7 @@ error: implementation of `FnMut` is not general enough
   --> $DIR/higher-ranked-lifetime-error.rs:12:5
    |
 LL |     assert_all::<_, &String>(id);
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ implementation of `FnMut` is not general enough
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^ implementation of `FnMut` is not general enough
    |
    = note: `for<'a> fn(&'a String) -> &'a String {id}` must implement `FnMut<(&String,)>`
    = note: ...but it actually implements `FnMut<(&'0 String,)>`, for some specific lifetime `'0`

Original file line number	Diff line number	Diff line change
`@@ -210,6 +210,10 @@ pub(crate) fn compute_regions<'a, 'tcx>(`
`210`	`210`	`for error in placeholder_errors.into_iter() {`
`211`	`211`	`nll_errors.push(error);`
`212`	`212`	`}`
	`213`	`+ } else {`
	`214`	`+ debug!(`
	`215`	`+ "Errors already reported, skipping these placeholder errors: {placeholder_errors:?}"`
	`216`	`+ );`
`213`	`217`	`}`
`214`	`218`
`215`	`219`	`if let Some(guar) = nll_errors.has_errors() {`
Original file line number	Diff line number	Diff line change
@@ -2,7 +2,7 @@ error: implementation of `FnMut` is not general enough
`2`	`2`	`--> $DIR/higher-ranked-lifetime-error.rs:12:5`
`3`	`3`	`\|`
`4`	`4`	`LL \| assert_all::<_, &String>(id);`
`5`		- \| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ implementation of `FnMut` is not general enough
	`5`	+ \| ^^^^^^^^^^^^^^^^^^^^^^^^ implementation of `FnMut` is not general enough
`6`	`6`	`\|`
`7`	`7`	= note: `for<'a> fn(&'a String) -> &'a String {id}` must implement `FnMut<(&String,)>`
`8`	`8`	= note: ...but it actually implements `FnMut<(&'0 String,)>`, for some specific lifetime `'0`