Use associated types for SCC annotations, per code review suggestion

Author: amandasystems

compiler/rustc_borrowck/src/region_infer/mod.rs

@@ -110,9 +110,7 @@ impl<'d, 'tcx, A: scc::Annotation> SccAnnotations<'d, 'tcx, A> {
     }
 }
 
-impl scc::Annotations<RegionVid, ConstraintSccIndex, RegionTracker>
-    for SccAnnotations<'_, '_, RegionTracker>
-{
+impl scc::Annotations<RegionVid> for SccAnnotations<'_, '_, RegionTracker> {
     fn new(&self, element: RegionVid) -> RegionTracker {
         RegionTracker::new(element, &self.definitions[element])
     }
@@ -121,6 +119,9 @@ impl scc::Annotations<RegionVid, ConstraintSccIndex, RegionTracker>
         let idx = self.scc_to_annotation.push(annotation);
         assert!(idx == scc);
     }
+
+    type Ann = RegionTracker;
+    type SccIdx = ConstraintSccIndex;
 }
 
 impl RegionTracker {
@@ -786,10 +787,6 @@ impl<'tcx> RegionInferenceContext<'tcx> {
         debug!(value = ?self.scc_values.region_value_str(scc_a));
     }
 
-    fn scc_annotations(&self) -> &IndexVec<ConstraintSccIndex, RegionTracker> {
-        &self.scc_annotations
-    }
-
     /// Invoked for each `R0 member of [R1..Rn]` constraint.
     ///
     /// `scc` is the SCC containing R0, and `choice_regions` are the
@@ -831,7 +828,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
 
         // If the member region lives in a higher universe, we currently choose
         // the most conservative option by leaving it unchanged.
-        if !self.scc_annotations()[scc].min_universe().is_root() {
+        if !self.scc_universe(scc).is_root() {
             return;
         }
 
@@ -907,8 +904,8 @@ impl<'tcx> RegionInferenceContext<'tcx> {
     /// in `scc_a`. Used during constraint propagation, and only once
     /// the value of `scc_b` has been computed.
     fn universe_compatible(&self, scc_b: ConstraintSccIndex, scc_a: ConstraintSccIndex) -> bool {
-        let a_annotation = self.scc_annotations()[scc_a];
-        let b_annotation = self.scc_annotations()[scc_b];
+        let a_annotation = self.scc_annotations[scc_a];
+        let b_annotation = self.scc_annotations[scc_b];
         let a_universe = a_annotation.min_universe();
 
         // If scc_b's declared universe is a subset of
@@ -1024,7 +1021,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
             "lower_bound = {:?} r_scc={:?} universe={:?}",
             lower_bound,
             r_scc,
-            self.scc_annotations()[r_scc].min_universe()
+            self.scc_universe(r_scc)
         );
         // If the type test requires that `T: 'a` where `'a` is a
         // placeholder from another universe, that effectively requires
@@ -1505,7 +1502,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
     /// The minimum universe of any variable reachable from this
     /// SCC, inside or outside of it.
     fn scc_universe(&self, scc: ConstraintSccIndex) -> UniverseIndex {
-        self.scc_annotations()[scc].min_universe()
+        self.scc_annotations[scc].min_universe()
     }
 
     /// Checks the final value for the free region `fr` to see if it
@@ -2249,7 +2246,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
     }
 
     pub(crate) fn liveness_constraints(&self) -> &LivenessValues {

compiler/rustc_data_structures/src/graph/scc/mod.rs

@@ -10,6 +10,7 @@
 
 use std::assert_matches::debug_assert_matches;
 use std::fmt::Debug;
+use std::marker::PhantomData;
 use std::ops::Range;
 
 use rustc_index::{Idx, IndexSlice, IndexVec};
@@ -49,16 +50,21 @@ pub trait Annotation: Debug + Copy {
 }
 
 /// An accumulator for annotations.
-pub trait Annotations<N: Idx, S: Idx, A: Annotation> {
-    fn new(&self, element: N) -> A;
-    fn annotate_scc(&mut self, scc: S, annotation: A);
+pub trait Annotations<N: Idx> {
+    type Ann: Annotation;
+    type SccIdx: Idx + Ord;
+
+    fn new(&self, element: N) -> Self::Ann;
+    fn annotate_scc(&mut self, scc: Self::SccIdx, annotation: Self::Ann);
 }
 
 /// The nil annotation accumulator, which does nothing.
-impl<N: Idx, S: Idx> Annotations<N, S, ()> for () {
-    fn new(&self, _element: N) -> () {
-        ()
-    }
+struct NoAnnotations<S: Idx + Ord>(PhantomData<S>);
+
+impl<N: Idx, S: Idx + Ord> Annotations<N> for NoAnnotations<S> {
+    type SccIdx = S;
+    type Ann = ();
+    fn new(&self, _element: N) {}
     fn annotate_scc(&mut self, _scc: S, _annotation: ()) {}
 }
 
@@ -112,13 +118,13 @@ struct SccData<S: Idx> {
 impl<N: Idx, S: Idx + Ord> Sccs<N, S> {
     /// Compute SCCs without annotations.
     pub fn new(graph: &impl Successors<Node = N>) -> Self {
-        Self::new_with_annotation(graph, &mut ())
+        Self::new_with_annotation(graph, &mut NoAnnotations(PhantomData::<S>))
     }
 
     /// Compute SCCs and annotate them with a user-supplied annotation
-    pub fn new_with_annotation<A: Annotation, AA: Annotations<N, S, A>>(
+    pub fn new_with_annotation<A: Annotations<N, SccIdx = S>>(
         graph: &impl Successors<Node = N>,
-        annotations: &mut AA,
+        annotations: &mut A,
     ) -> Self {
         SccsConstruction::construct(graph, annotations)
     }
@@ -141,13 +147,7 @@ impl<N: Idx, S: Idx + Ord> Sccs<N, S> {
     pub fn all_sccs(&self) -> impl Iterator<Item = S> + 'static {
         (0..self.scc_data.len()).map(S::new)
     }
-    /*
-        /// Returns an iterator over the SCC annotations in the graph
-        /// The order is the same as `all_sccs()`, dependency order.
-        pub fn all_annotations(&self, annotations: &A) -> impl Iterator<Item = (S, A)> + use<'_, N, S, A> {
-            self.all_sccs().map(|scc| (scc, self.annotation(scc)))
-        }
-    */
+
     /// Returns the SCC to which a node `r` belongs.
     pub fn scc(&self, r: N) -> S {
         self.scc_indices[r]
@@ -223,19 +223,17 @@ impl<S: Idx> SccData<S> {
     }
 }
 
-struct SccsConstruction<'c, 'a, G, S, A, AA>
+struct SccsConstruction<'c, 'a, G, A>
 where
     G: DirectedGraph + Successors,
-    S: Idx,
-    A: Annotation,
-    AA: Annotations<G::Node, S, A>,
+    A: Annotations<G::Node>,
 {
     graph: &'c G,
 
     /// The state of each node; used during walk to record the stack
     /// and after walk to record what cycle each node ended up being
     /// in.
-    node_states: IndexVec<G::Node, NodeState<G::Node, S, A>>,
+    node_states: IndexVec<G::Node, NodeState<G::Node, A::SccIdx, A::Ann>>,
 
     /// The stack of nodes that we are visiting as part of the DFS.
     node_stack: Vec<G::Node>,
@@ -244,21 +242,21 @@ where
     /// position in this stack, and when we encounter a successor SCC,
     /// we push it on the stack. When we complete an SCC, we can pop
     /// everything off the stack that was found along the way.
-    successors_stack: Vec<S>,
+    successors_stack: Vec<A::SccIdx>,
 
     /// A set used to strip duplicates. As we accumulate successors
     /// into the successors_stack, we sometimes get duplicate entries.
     /// We use this set to remove those -- we also keep its storage
     /// around between successors to amortize memory allocation costs.
-    duplicate_set: FxHashSet<S>,
+    duplicate_set: FxHashSet<A::SccIdx>,
 
-    scc_data: SccData<S>,
+    scc_data: SccData<A::SccIdx>,
 
-    annotations: &'a mut AA,
+    annotations: &'a mut A,
 }
 
 #[derive(Copy, Clone, Debug)]
-enum NodeState<N, S, A> {
+enum NodeState<N, S, A: Annotation> {
     /// This node has not yet been visited as part of the DFS.
     ///
     /// After SCC construction is complete, this state ought to be
@@ -289,7 +287,7 @@ enum NodeState<N, S, A> {
 
 /// The state of walking a given node.
 #[derive(Copy, Clone, Debug)]
-enum WalkReturn<S, A> {
+enum WalkReturn<S, A: Annotation> {
     /// The walk found a cycle, but the entire component is not known to have
     /// been fully walked yet. We only know the minimum depth of  this
     /// component in a minimum spanning tree of the graph. This component
@@ -302,12 +300,10 @@ enum WalkReturn<S, A> {
     Complete { scc_index: S, annotation: A },
 }
 
-impl<'c, 'a, G, S, A, AA> SccsConstruction<'c, 'a, G, S, A, AA>
+impl<'c, 'a, G, A> SccsConstruction<'c, 'a, G, A>
 where
     G: DirectedGraph + Successors,
-    S: Idx,
-    A: Annotation,
-    AA: Annotations<G::Node, S, A>,
+    A: Annotations<G::Node>,
 {
     /// Identifies SCCs in the graph `G` and computes the resulting
     /// DAG. This uses a variant of [Tarjan's
@@ -323,7 +319,7 @@ where
     /// Additionally, we keep track of a current annotation of the SCC.
     ///
     /// [wikipedia]: https://bit.ly/2EZIx84
-    fn construct(graph: &'c G, annotations: &'a mut AA) -> Sccs<G::Node, S> {
+    fn construct(graph: &'c G, annotations: &'a mut A) -> Sccs<G::Node, A::SccIdx> {
         let num_nodes = graph.num_nodes();
 
         let mut this = Self {
@@ -349,7 +345,7 @@ where
         Sccs { scc_indices, scc_data: this.scc_data }
     }
 
-    fn start_walk_from(&mut self, node: G::Node) -> WalkReturn<S, A> {
+    fn start_walk_from(&mut self, node: G::Node) -> WalkReturn<A::SccIdx, A::Ann> {
         self.inspect_node(node).unwrap_or_else(|| self.walk_unvisited_node(node))
     }
 
@@ -365,7 +361,7 @@ where
     /// Otherwise, we are looking at a node that has already been
     /// completely visited. We therefore return `WalkReturn::Complete`
     /// with its associated SCC index.
-    fn inspect_node(&mut self, node: G::Node) -> Option<WalkReturn<S, A>> {
+    fn inspect_node(&mut self, node: G::Node) -> Option<WalkReturn<A::SccIdx, A::Ann>> {
         Some(match self.find_state(node) {
             NodeState::InCycle { scc_index, annotation } => {
                 WalkReturn::Complete { scc_index, annotation }
@@ -388,7 +384,7 @@ where
     /// of `r2` (and updates `r` to reflect current result). This is
     /// basically the "find" part of a standard union-find algorithm
     /// (with path compression).
-    fn find_state(&mut self, mut node: G::Node) -> NodeState<G::Node, S, A> {
+    fn find_state(&mut self, mut node: G::Node) -> NodeState<G::Node, A::SccIdx, A::Ann> {
         // To avoid recursion we temporarily reuse the `parent` of each
         // InCycleWith link to encode a downwards link while compressing
         // the path. After we have found the root or deepest node being
@@ -511,7 +507,7 @@ where
     /// caller decide avoids mutual recursion between the two methods and allows
     /// us to maintain an allocated stack for nodes on the path between calls.
     #[instrument(skip(self, initial), level = "trace")]
-    fn walk_unvisited_node(&mut self, initial: G::Node) -> WalkReturn<S, A> {
+    fn walk_unvisited_node(&mut self, initial: G::Node) -> WalkReturn<A::SccIdx, A::Ann> {
         trace!("Walk unvisited node: {initial:?}");
         struct VisitingNodeFrame<G: DirectedGraph, Successors, A> {
             node: G::Node,

compiler/rustc_data_structures/src/graph/scc/tests.rs

@@ -8,7 +8,7 @@ struct MaxReached(usize);
 struct Maxes(IndexVec<usize, MaxReached>, fn(usize) -> usize);
 type UsizeSccs = Sccs<usize, usize>;
 
-impl Annotations<usize, usize, MaxReached> for Maxes {
+impl Annotations<usize> for Maxes {
     fn new(&self, element: usize) -> MaxReached {
         MaxReached(self.1(element))
     }
@@ -17,6 +17,9 @@ impl Annotations<usize, usize, MaxReached> for Maxes {
         let i = self.0.push(annotation);
         assert!(i == scc);
     }
+
+    type Ann = MaxReached;
+    type SccIdx = usize;
 }
 
 impl Maxes {
@@ -57,7 +60,7 @@ impl MinMaxes {
     }
 }
 
-impl Annotations<usize, usize, MinMaxIn> for MinMaxes {
+impl Annotations<usize> for MinMaxes {
     fn new(&self, element: usize) -> MinMaxIn {
         self.1(element)
     }
@@ -66,6 +69,9 @@ impl Annotations<usize, usize, MinMaxIn> for MinMaxes {
         let i = self.0.push(annotation);
         assert!(i == scc);
     }
+
+    type Ann = MinMaxIn;
+    type SccIdx = usize;
 }
 
 impl Annotation for MinMaxIn {