Use an enum for SCC representatives, plus other code review

Author: amandasystems

compiler/rustc_borrowck/src/eliminate_placeholders.rs

@@ -18,7 +18,7 @@ use crate::consumers::OutlivesConstraint;
 use crate::diagnostics::UniverseInfo;
 use crate::member_constraints::MemberConstraintSet;
 use crate::region_infer::values::{LivenessValues, PlaceholderIndices};
-use crate::region_infer::{ConstraintSccs, RegionDefinition, TypeTest};
+use crate::region_infer::{ConstraintSccs, RegionDefinition, Representative, TypeTest};
 use crate::ty::VarianceDiagInfo;
 use crate::type_check::free_region_relations::UniversalRegionRelations;
 use crate::type_check::{Locations, MirTypeckRegionConstraints};
@@ -76,36 +76,23 @@ pub(crate) struct RegionTracker {
     /// The smallest universe index reachable form the nodes of this SCC.
     min_reachable_universe: UniverseIndex,
 
-    /// The representative Region Variable Id for this SCC. We prefer
-    /// placeholders over existentially quantified variables, otherwise
-    ///  it's the one with the smallest Region Variable ID.
-    pub(crate) representative: RegionVid,
-
-    /// Is the current representative a placeholder?
-    representative_is_placeholder: bool,
-
-    /// Is the current representative existentially quantified?
-    representative_is_existential: bool,
+    /// The representative Region Variable Id for this SCC.
+    pub(crate) representative: Representative,
 }
 
 impl RegionTracker {
     pub(crate) fn new(rvid: RegionVid, definition: &RegionDefinition<'_>) -> Self {
-        let (representative_is_placeholder, representative_is_existential) = match definition.origin
-        {
-            NllRegionVariableOrigin::FreeRegion => (false, false),
-            NllRegionVariableOrigin::Placeholder(_) => (true, false),
-            NllRegionVariableOrigin::Existential { .. } => (false, true),
-        };
-
         let placeholder_universe =
-            if representative_is_placeholder { definition.universe } else { UniverseIndex::ROOT };
+            if matches!(definition.origin, NllRegionVariableOrigin::Placeholder(_)) {
+                definition.universe
+            } else {
+                UniverseIndex::ROOT
+            };
 
         Self {
             max_placeholder_universe_reached: placeholder_universe,
             min_reachable_universe: definition.universe,
-            representative: rvid,
-            representative_is_placeholder,
-            representative_is_existential,
+            representative: Representative::new(rvid, definition),
         }
     }
 
@@ -139,21 +126,10 @@ impl RegionTracker {
 }
 
 impl scc::Annotation for RegionTracker {
-    fn merge_scc(mut self, mut other: Self) -> Self {
-        // Prefer any placeholder over any existential
-        if other.representative_is_placeholder && self.representative_is_existential {
-            other.merge_min_max_seen(&self);
-            return other;
-        }
-
-        if self.representative_is_placeholder && other.representative_is_existential
-            || (self.representative <= other.representative)
-        {
-            self.merge_min_max_seen(&other);
-            return self;
-        }
-        other.merge_min_max_seen(&self);
-        other
+    fn merge_scc(mut self, other: Self) -> Self {
+        self.representative = self.representative.merge_scc(other.representative);
+        self.merge_min_max_seen(&other);
+        self
     }
 
     fn merge_reached(mut self, other: Self) -> Self {
@@ -267,13 +243,14 @@ pub(crate) fn rewrite_higher_kinded_outlives_as_constraints<'tcx>(
             )
         };
 
+    let mut scc_annotations = SccAnnotations::init(&definitions);
+    let constraint_sccs = compute_sccs(&outlives_constraints, &mut scc_annotations);
+
     // This code structure is a bit convoluted because it allows for a planned
     // future change where the early return here has a different type of annotation
     // that does much less work.
     if !has_placeholders {
         debug!("No placeholder regions found; skipping rewriting logic!");
-        let mut scc_annotations = SccAnnotations::init(&definitions);
-        let constraint_sccs = compute_sccs(&outlives_constraints, &mut scc_annotations);
 
         return LoweredConstraints {
             type_tests,
@@ -289,13 +266,14 @@ pub(crate) fn rewrite_higher_kinded_outlives_as_constraints<'tcx>(
     }
     debug!("Placeholders present; activating placeholder handling logic!");
 
-    let mut annotations = SccAnnotations::init(&definitions);
-    let sccs = compute_sccs(&outlives_constraints, &mut annotations);
-
-    let outlives_static =
-        rewrite_outlives(&sccs, &annotations, fr_static, &mut outlives_constraints);
+    let outlives_static = rewrite_placeholder_outlives(
+        &constraint_sccs,
+        &scc_annotations,
+        fr_static,
+        &mut outlives_constraints,
+    );
 
-    let (sccs, scc_annotations) = if !outlives_static.is_empty() {
+    let (constraint_sccs, scc_annotations) = if !outlives_static.is_empty() {
         debug!("The following SCCs had :'static constraints added: {:?}", outlives_static);
         let mut annotations = SccAnnotations::init(&definitions);
 
@@ -307,11 +285,11 @@ pub(crate) fn rewrite_higher_kinded_outlives_as_constraints<'tcx>(
     } else {
         // If we didn't add any back-edges; no more work needs doing
         debug!("No constraints rewritten!");
-        (sccs, annotations.scc_to_annotation)
+        (constraint_sccs, scc_annotations.scc_to_annotation)
     };
 
     LoweredConstraints {
-        constraint_sccs: sccs,
+        constraint_sccs,
         definitions,
         scc_annotations,
         member_constraints,
@@ -323,7 +301,7 @@ pub(crate) fn rewrite_higher_kinded_outlives_as_constraints<'tcx>(
     }
 }
 
-fn rewrite_outlives<'tcx>(
+fn rewrite_placeholder_outlives<'tcx>(
     sccs: &Sccs<RegionVid, ConstraintSccIndex>,
     annotations: &SccAnnotations<'_, '_, RegionTracker>,
     fr_static: RegionVid,
@@ -356,7 +334,7 @@ fn rewrite_outlives<'tcx>(
             // outlive static.
             outlives_static.insert(scc);
             let scc_representative_outlives_static = OutlivesConstraint {
-                sup: annotation.representative,
+                sup: annotation.representative.rvid(),
                 sub: fr_static,
                 category: ConstraintCategory::IllegalUniverse,
                 locations: Locations::All(rustc_span::DUMMY_SP),

compiler/rustc_borrowck/src/region_infer/mod.rs

@@ -4,7 +4,7 @@ use std::rc::Rc;
 use rustc_data_structures::binary_search_util;
 use rustc_data_structures::frozen::Frozen;
 use rustc_data_structures::fx::{FxIndexMap, FxIndexSet};
-use rustc_data_structures::graph::scc::Sccs;
+use rustc_data_structures::graph::scc::{self, Sccs};
 use rustc_errors::Diag;
 use rustc_hir::def_id::CRATE_DEF_ID;
 use rustc_index::IndexVec;
@@ -50,6 +50,47 @@ mod reverse_sccs;
 
 pub(crate) mod values;
 
+/// The representative region variable for an SCC, tagged by its origin.
+/// We prefer placeholders over existentially quantified variables, otherwise
+/// it's the one with the smallest Region Variable ID. In other words,
+/// the order of this enumeration really matters!
+#[derive(Copy, Debug, Clone, PartialEq, PartialOrd, Eq, Ord)]
+pub(crate) enum Representative {
+    FreeRegion(RegionVid),
+    Placeholder(RegionVid),
+    Existential(RegionVid),
+}
+
+impl Representative {
+    pub(crate) fn rvid(self) -> RegionVid {
+        match self {
+            Representative::FreeRegion(region_vid)
+            | Representative::Placeholder(region_vid)
+            | Representative::Existential(region_vid) => region_vid,
+        }
+    }
+
+    pub(crate) fn new(r: RegionVid, definition: &RegionDefinition<'_>) -> Self {
+        match definition.origin {
+            NllRegionVariableOrigin::FreeRegion => Representative::FreeRegion(r),
+            NllRegionVariableOrigin::Placeholder(_) => Representative::Placeholder(r),
+            NllRegionVariableOrigin::Existential { .. } => Representative::Existential(r),
+        }
+    }
+}
+
+impl scc::Annotation for Representative {
+    fn merge_scc(self, other: Self) -> Self {
+        // Just pick the smallest one. Note that we order by tag first!
+        std::cmp::min(self, other)
+    }
+
+    // For reachability, we do nothing since the representative doesn't change.
+    fn merge_reached(self, _other: Self) -> Self {
+        self
+    }
+}
+
 pub(crate) type ConstraintSccs = Sccs<RegionVid, ConstraintSccIndex>;
 
 pub struct RegionInferenceContext<'tcx> {
@@ -2086,7 +2127,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
     /// they *must* be equal (though not having the same repr does not
     /// mean they are unequal).
     fn scc_representative(&self, scc: ConstraintSccIndex) -> RegionVid {
-        self.scc_annotations[scc].representative
+        self.scc_annotations[scc].representative.rvid()
     }
 
     pub(crate) fn liveness_constraints(&self) -> &LivenessValues {