Code review adjustments to naming, comments, docstrings

Co-authored-by: lcnr <[email protected]>

Author: amandasystems

compiler/rustc_borrowck/src/diagnostics/region_errors.rs

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

compiler/rustc_borrowck/src/handle_placeholders.rs

@@ -163,6 +163,9 @@ impl RegionTracker {
 
     /// Determine if the tracked universes of the two SCCs are compatible.
     pub(crate) fn universe_compatible_with(&self, other: Self) -> bool {
+        // HACK: We first check whether we can name the highest existential universe
+        // of `other`. This only exists to avoid errors in case that scc already
+        // depends on a placeholder it cannot name itself.
         self.max_nameable_universe().can_name(other.max_nameable_universe())
             || other.reachable_placeholders.can_be_named_by(self.max_nameable_universe())
     }
@@ -419,12 +422,12 @@ fn rewrite_placeholder_outlives<'tcx>(
             annotation.representative
         );
         // We only add one `r: 'static` constraint per SCC, where `r` is the SCC representative.
-        // That constraint is annotated with some outlives relation `tries: unnameable` where
-        // `unnameable` is unnameable from `tries` and there is a path in the constraint
-        // graph between them.
+        // That constraint is annotated with some outlives relation `lt: unnameable` where
+        // `unnameable` is unnameable from `lt` and there is a path in the constraint graph
+        // between them.
         //
-        // We prefer the representative as `tries` in all cases but one: where the problem
-        // is that the SCC has had its universe lowered to accomodate some other region and
+        // We prefer the representative, `r`, as `lt` in all cases but one: where the problem
+        // is that the SCC has had its universe lowered to accommodate some other region and
         // no longer can name its representative. In that case, we blame `r: low_u`, where `low_u`
         // cannot name `r` so that any explanation always starts with the SCC representative.
         let blame_to = if annotation.representative.rvid() == max_u_rvid {
@@ -455,7 +458,7 @@ fn rewrite_placeholder_outlives<'tcx>(
         outlives_constraints.push(OutlivesConstraint {
             sup: annotation.representative.rvid(),
             sub: fr_static,
-            category: ConstraintCategory::IllegalPlaceholder(
+            category: ConstraintCategory::OutlivesUnnameablePlaceholder(
                 annotation.representative.rvid(),
                 blame_to,
             ),

compiler/rustc_borrowck/src/region_infer/graphviz.rs

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

compiler/rustc_borrowck/src/region_infer/mod.rs

@@ -1730,38 +1730,6 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         None
     }
 
-    /// Find a path of outlives constraints from `from` to `to`,
-    /// taking placeholder blame constraints into account, e.g.
-    /// if there is a relationship where `r1` reaches `r2` and
-    /// r2 has a larger universe or if r1 and r2 both come from
-    /// placeholder regions.
-    ///
-    /// Returns the path. It panics if there is no such path.
-    fn path_to_modulo_placeholders(
-        &self,
-        from: RegionVid,
-        to: RegionVid,
-    ) -> Vec<OutlivesConstraint<'tcx>> {
-        let path = self.constraint_path_between_regions(from, to).unwrap();
-
-        // If we are looking for a path to 'static, and we are passing
-        // through a constraint synthesised from an illegal placeholder
-        // relation, redirect the search to the placeholder to blame.
-        // FIXME: the performance of this is Not Great, and the logic
-        // should be folded into the search itself if possible.
-        for constraint in path.iter() {
-            let ConstraintCategory::IllegalPlaceholder(cl_fr, culprit) = constraint.category else {
-                continue;
-            };
-
-            debug!("{culprit:?} is the reason {from:?}: 'static!");
-            return self.constraint_path_to(cl_fr, |r| r == culprit, false).unwrap().0;
-        }
-
-        // No funny business; just return the path!
-        path
-    }
-
     /// Finds some region R such that `fr1: R` and `R` is live at `location`.
     #[instrument(skip(self), level = "trace", ret)]
     pub(crate) fn find_sub_region_live_at(&self, fr1: RegionVid, location: Location) -> RegionVid {
@@ -1823,8 +1791,24 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         to_region: RegionVid,
     ) -> (BlameConstraint<'tcx>, Vec<OutlivesConstraint<'tcx>>) {
         assert!(from_region != to_region, "Trying to blame a region for itself!");
-        // Find all paths
-        let path = self.path_to_modulo_placeholders(from_region, to_region);
+
+        let path = self.constraint_path_between_regions(from_region, to_region).unwrap();
+
+        // If we are passing through a constraint added because `'lt: 'unnameable`,
+        // where cannot name `'unnameable`, redirect search towards `'unnameable`.
+        let path = if let Some((lt, unnameable)) = path.iter().find_map(|c| {
+            if let ConstraintCategory::OutlivesUnnameablePlaceholder(lt, unnameable) = c.category {
+                Some((lt, unnameable))
+            } else {
+                None
+            }
+        }) {
+            // This the `false` argument is what prevents circular reasoning here!
+            self.constraint_path_to(lt, |r| r == unnameable, false).unwrap().0
+        } else {
+            path
+        };
+
         debug!(
             "path={:#?}",
             path.iter()
@@ -1965,7 +1949,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
                 // specific, and are not used for relations that would make sense to blame.
                 ConstraintCategory::BoringNoLocation => 6,
                 // Do not blame internal constraints.
-                ConstraintCategory::IllegalPlaceholder(_, _) => 7,
+                ConstraintCategory::OutlivesUnnameablePlaceholder(_, _) => 7,
                 ConstraintCategory::Internal => 8,
             };
 
@@ -2003,7 +1987,10 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         };
 
         assert!(
-            !matches!(best_constraint.category, ConstraintCategory::IllegalPlaceholder(_, _)),
+            !matches!(
+                best_constraint.category,
+                ConstraintCategory::OutlivesUnnameablePlaceholder(_, _)
+            ),
             "Illegal placeholder constraint blamed; should have redirected to other region relation"
         );
 

compiler/rustc_middle/src/mir/query.rs

@@ -149,10 +149,11 @@ pub enum ConstraintCategory<'tcx> {
     /// A constraint that doesn't correspond to anything the user sees.
     Internal,
 
-    /// An internal constraint derived from an illegal placeholder relation
-    /// to this region. The arguments are a source -> drain of a path
-    /// that caused the problem, used when reporting errors.
-    IllegalPlaceholder(
+    /// An internal constraint added when a region outlives a placeholder
+    /// it cannot name and therefore has to outlive `'static`. The arguments
+    /// are a source -> drain of a path that caused the problem and always
+    /// leads to `'static`.
+    OutlivesUnnameablePlaceholder(
         #[type_foldable(identity)]
         #[type_visitable(ignore)]
         ty::RegionVid,