Document obligation forest changes

Author: Markus Westerlind

compiler/rustc_data_structures/src/logged_unification_table.rs

@@ -48,7 +48,7 @@ pub struct LoggedUnificationTableStorage<K: ut::UnifyKey, I: Idx = K> {
     modified_set: ms::ModifiedSet<I>,
 }
 
-/// UnificationTableStorage which logs which variables has been unfified with a value, allowing watchers
+/// UnificationTableStorage which logs which variables has been unified with a value, allowing watchers
 /// to only iterate over the changed variables instead of all variables
 pub struct LoggedUnificationTable<'a, K: ut::UnifyKey, I: Idx, L> {
     storage: &'a mut LoggedUnificationTableStorage<K, I>,
@@ -200,11 +200,11 @@ where
         self.storage.unify_log.unwatch_variable(index)
     }
 
-    /// Iterates through all unified variables since the last call to `drain_modified_set`
+    /// Iterates through all unified variables since the last call to `notify_watcher`
     /// passing the unified variable to `f`
-    pub fn drain_modified_set(&mut self, offset: &ms::Offset<I>, mut f: impl FnMut(I)) {
+    pub fn notify_watcher(&mut self, offset: &ms::Offset<I>, mut f: impl FnMut(I)) {
         let unify_log = &self.storage.unify_log;
-        self.storage.modified_set.drain(&mut self.undo_log, offset, |vid| {
+        self.storage.modified_set.notify_watcher(&mut self.undo_log, offset, |vid| {
             for &unified_vid in unify_log.get(vid) {
                 f(unified_vid);
             }

compiler/rustc_data_structures/src/modified_set.rs

@@ -13,6 +13,8 @@ enum UndoInner {
 #[derive(Copy, Clone, Debug)]
 pub struct Undo<I>(UndoInner, PhantomData<I>);
 
+/// Tracks which indices have been modified and allows watchers to registered and notified of these
+/// changes.
 #[derive(Clone, Debug)]
 pub struct ModifiedSet<T: Idx> {
     modified: Vec<T>,
@@ -26,32 +28,40 @@ impl<T: Idx> Default for ModifiedSet<T> {
 }
 
 impl<T: Idx> ModifiedSet<T> {
+    /// Creates a new `ModifiedSet`
     pub fn new() -> Self {
         Self::default()
     }
 
+    /// Marks `index` as "modified". A subsequent call to `drain` will notify the callback with
+    /// `index`
     pub fn set(&mut self, undo_log: &mut impl UndoLogs<Undo<T>>, index: T) {
         self.modified.push(index);
         undo_log.push(Undo(UndoInner::Add, PhantomData));
     }
 
-    pub fn drain(
+    /// Calls `f` with all the indices that have been modified since the last call to
+    /// `notify_watcher`
+    pub fn notify_watcher(
         &mut self,
         undo_log: &mut impl UndoLogs<Undo<T>>,
-        index: &Offset<T>,
+        watcher_offset: &Offset<T>,
         mut f: impl FnMut(T),
     ) {
-        let offset = &mut self.offsets[index.index];
+        let offset = &mut self.offsets[watcher_offset.index];
         if *offset < self.modified.len() {
             for &index in &self.modified[*offset..] {
                 f(index);
             }
-            undo_log
-                .push(Undo(UndoInner::Drain { index: index.index, offset: *offset }, PhantomData));
+            undo_log.push(Undo(
+                UndoInner::Drain { index: watcher_offset.index, offset: *offset },
+                PhantomData,
+            ));
             *offset = self.modified.len();
         }
     }
 
+    /// Clears the set of all modifications that have been drained by all watchers
     pub fn clear(&mut self) {
         let min = self.offsets.iter().copied().min().unwrap_or_else(|| self.modified.len());
         self.modified.drain(..min);
@@ -60,14 +70,24 @@ impl<T: Idx> ModifiedSet<T> {
         }
     }
 
+    /// Registers a new watcher on this set.
+    ///
+    /// NOTE: Watchers must be removed in the reverse order that they were registered
     pub fn register(&mut self) -> Offset<T> {
         let index = self.offsets.len();
         self.offsets.push(self.modified.len());
         Offset { index, _marker: PhantomData }
     }
 
+    /// De-registers a watcher on this set.
+    ///
+    /// NOTE: Watchers must be removed in the reverse order that they were registered
     pub fn deregister(&mut self, offset: Offset<T>) {
-        assert_eq!(offset.index, self.offsets.len() - 1);
+        assert_eq!(
+            offset.index,
+            self.offsets.len() - 1,
+            "Watchers must be removed in the reverse order that they were registered"
+        );
         self.offsets.pop();
         std::mem::forget(offset);
     }
@@ -88,6 +108,8 @@ impl<I: Idx> Rollback<Undo<I>> for ModifiedSet<I> {
     }
 }
 
+/// A registered offset into a `ModifiedSet`. Tracks how much a watcher has seen so far to avoid
+/// being notified of the same event twice.
 #[must_use]
 pub struct Offset<T> {
     index: usize,

compiler/rustc_data_structures/src/obligation_forest/mod.rs

@@ -1,3 +1,4 @@
+//! The `ObligationForest` is a utility data structure used in trait
 //! matching to track the set of outstanding obligations (those not yet
 //! resolved to success or error). It also tracks the "backtrace" of each
 //! pending obligation (why we are trying to figure this out in the first
@@ -41,7 +42,7 @@
 //!   now considered to be in error.
 //!
 //! When the call to `process_obligations` completes, you get back an `Outcome`,
-//! which includes three bits of information:
+//! which includes two bits of information:
 //!
 //! - `completed`: a list of obligations where processing was fully
 //!   completed without error (meaning that all transitive subobligations
@@ -52,13 +53,6 @@
 //!   all the obligations in `C` have been found completed.
 //! - `errors`: a list of errors that occurred and associated backtraces
 //!   at the time of error, which can be used to give context to the user.
-//! - `stalled`: if true, then none of the existing obligations were
-//!   *shallowly successful* (that is, no callback returned `Changed(_)`).
-//!   This implies that all obligations were either errors or returned an
-//!   ambiguous result, which means that any further calls to
-//!   `process_obligations` would simply yield back further ambiguous
-//!   results. This is used by the `FulfillmentContext` to decide when it
-//!   has reached a steady state.
 //!
 //! ### Implementation details
 //!
@@ -88,17 +82,17 @@ mod graphviz;
 mod tests;
 
 pub trait ForestObligation: Clone + Debug {
-    /// A key used to avoid evaluating the same obligation twice
+    /// A key used to avoid evaluating the same obligation twice.
     type CacheKey: Clone + hash::Hash + Eq + Debug;
-    /// The variable type used in the obligation when it could not yet be fulfilled
+    /// The variable type used in the obligation when it could not yet be fulfilled.
     type Variable: Clone + hash::Hash + Eq + Debug;
-    /// A type which tracks which variables has been unified
+    /// A type which tracks which variables has been unified.
     type WatcherOffset;
 
     /// Converts this `ForestObligation` suitable for use as a cache key.
     /// If two distinct `ForestObligations`s return the same cache key,
     /// then it must be sound to use the result of processing one obligation
-    /// (e.g. success for error) for the other obligation
+    /// (e.g. success for error) for the other obligation.
     fn as_cache_key(&self) -> Self::CacheKey;
 
     /// Returns which variables this obligation is currently stalled on. If the slice is empty then
@@ -127,7 +121,9 @@ pub trait ObligationProcessor {
     where
         I: Clone + Iterator<Item = &'c Self::Obligation>;
 
-    fn unblocked(
+    /// Calls `f` with all the variables that have been unblocked (instantiated) since the last call
+    /// to `notify_unblocked`.
+    fn notify_unblocked(
         &self,
         offset: &<Self::Obligation as ForestObligation>::WatcherOffset,
         f: impl FnMut(<Self::Obligation as ForestObligation>::Variable),
@@ -175,14 +171,15 @@ pub struct ObligationForest<O: ForestObligation> {
     /// number allowing them to be ordered when processing them.
     node_number: u32,
 
-    /// Stores the indices of the nodes currently in the pending state
+    /// Stores the indices of the nodes currently in the pending state.
     pending_nodes: Vec<NodeIndex>,
 
     /// Stores the indices of the nodes currently in the success or waiting states.
     /// Can also contain `Done` or `Error` nodes as `process_cycles` does not remove a node
     /// immediately but instead upon the next time that node is processed.
     success_or_waiting_nodes: Vec<NodeIndex>,
-    /// Stores the indices of the nodes currently in the error or done states
+
+    /// Stores the indices of the nodes currently in the error or done states.
     error_or_done_nodes: RefCell<Vec<NodeIndex>>,
 
     /// Nodes that have been removed and are ready to be reused (pure optimization to reuse
@@ -205,22 +202,26 @@ pub struct ObligationForest<O: ForestObligation> {
     /// [details]: https://github.com/rust-lang/rust/pull/53255#issuecomment-421184780
     error_cache: FxHashMap<ObligationTreeId, FxHashSet<O::CacheKey>>,
 
-    /// Stores which nodes would be unblocked once `O::Variable` is unified
+    /// Stores which nodes would be unblocked once `O::Variable` is unified.
     stalled_on: FxHashMap<O::Variable, Vec<NodeIndex>>,
-    /// Stores the node indices that are unblocked and should be processed at the next opportunity
+
+    /// Stores the node indices that are unblocked and should be processed at the next opportunity.
     unblocked: BinaryHeap<Unblocked>,
+
     /// Stores nodes which should be processed on the next iteration since the variables they are
-    /// actually blocked on are unknown
+    /// actually blocked on are unknown.
     stalled_on_unknown: Vec<NodeIndex>,
+
     /// The offset that this `ObligationForest` has registered. Should be de-registered before
     /// dropping this forest.
-    offset: Option<O::WatcherOffset>,
-    /// Reusable vector for storing unblocked nodes whose watch should be removed
+    ///
+    watcher_offset: Option<O::WatcherOffset>,
+    /// Reusable vector for storing unblocked nodes whose watch should be removed.
     temp_unblocked_nodes: Vec<O::Variable>,
 }
 
 /// Helper struct for use with `BinaryHeap` to process nodes in the order that they were added to
-/// the forest
+/// the forest.
 struct Unblocked {
     index: NodeIndex,
     order: u32,
@@ -248,13 +249,14 @@ struct Node<O: ForestObligation> {
     obligation: O,
     state: Cell<NodeState>,
 
-    /// A predicate (and its key) can changed during processing. If it does we need to register the
+    /// A predicate (and its key) can change during processing. If it does we need to register the
     /// old predicate so that we can remove or mark it as done if this node errors or is done.
     alternative_predicates: Vec<O::CacheKey>,
 
     /// Obligations that depend on this obligation for their completion. They
     /// must all be in a non-pending state.
     dependents: Vec<NodeIndex>,
+
     /// Obligations that this obligation depends on for their completion.
     reverse_dependents: Vec<NodeIndex>,
 
@@ -267,6 +269,9 @@ struct Node<O: ForestObligation> {
 
     /// Identifier of the obligation tree to which this node belongs.
     obligation_tree_id: ObligationTreeId,
+
+    /// Nodes must be processed in the order that they were added so we give each node a unique
+    /// number allowing them to be ordered when processing them.
     node_number: u32,
 }
 
@@ -292,8 +297,9 @@ where
         }
     }
 
-    /// Initializes a node, reusing the existing allocations
-    fn init(
+    /// Initializes a node, reusing the existing allocations. Used when removing a node from the
+    /// dead_nodes list
+    fn reinit(
         &mut self,
         parent: Option<NodeIndex>,
         obligation: O,
@@ -441,16 +447,19 @@ impl<O: ForestObligation> ObligationForest<O> {
             unblocked: Default::default(),
             stalled_on_unknown: Default::default(),
             temp_unblocked_nodes: Default::default(),
-            offset: None,
+            watcher_offset: None,
         }
     }
 
-    pub fn offset(&self) -> Option<&O::WatcherOffset> {
-        self.offset.as_ref()
+    /// Returns the `WatcherOffset` regsitered with the notification table. See the field
+    /// `ObligationForest::watcher_offset`.
+    pub fn watcher_offset(&self) -> Option<&O::WatcherOffset> {
+        self.watcher_offset.as_ref()
     }
 
-    pub fn take_offset(&mut self) -> Option<O::WatcherOffset> {
-        self.offset.take()
+    /// Removes the watcher_offset, allowing it to be deregistered
+    pub fn take_watcher_offset(&mut self) -> Option<O::WatcherOffset> {
+        self.watcher_offset.take()
     }
 
     /// Returns the total number of nodes in the forest that have not
@@ -523,7 +532,7 @@ impl<O: ForestObligation> ObligationForest<O> {
                     // otherwise allocate a new node
                     let new_index = if let Some(new_index) = self.dead_nodes.pop() {
                         let node = &mut self.nodes[new_index];
-                        node.init(parent, obligation, obligation_tree_id, node_number);
+                        node.reinit(parent, obligation, obligation_tree_id, node_number);
                         new_index
                     } else {
                         let new_index = self.nodes.len();
@@ -565,11 +574,7 @@ impl<O: ForestObligation> ObligationForest<O> {
     where
         F: Fn(&O) -> P,
     {
-        self.pending_nodes
-            .iter()
-            .map(|&index| &self.nodes[index])
-            .map(|node| f(&node.obligation))
-            .collect()
+        self.pending_nodes.iter().map(|&index| f(&self.nodes[index].obligation)).collect()
     }
 
     fn insert_into_error_cache(&mut self, index: NodeIndex) {
@@ -589,8 +594,8 @@ impl<O: ForestObligation> ObligationForest<O> {
         P: ObligationProcessor<Obligation = O>,
         OUT: OutcomeTrait<Obligation = O, Error = Error<O, P::Error>>,
     {
-        if self.offset.is_none() {
-            self.offset = Some(processor.register_variable_watcher());
+        if self.watcher_offset.is_none() {
+            self.watcher_offset = Some(processor.register_variable_watcher());
         }
         let mut errors = vec![];
         let mut stalled = true;
@@ -688,7 +693,10 @@ impl<O: ForestObligation> ObligationForest<O> {
         Outcome { completed, errors }
     }
 
-    #[inline(never)]
+    /// Checks which nodes have been unblocked since the last time this was called. All nodes that
+    /// were unblocked are added to the `unblocked` queue and all watches associated with the
+    /// variables blocking those nodes are deregistered (since they are now instantiated, they will
+    /// neither block a node, nor be instantiated again)
     fn unblock_nodes<P>(&mut self, processor: &mut P)
     where
         P: ObligationProcessor<Obligation = O>,
@@ -698,7 +706,7 @@ impl<O: ForestObligation> ObligationForest<O> {
         let unblocked = &mut self.unblocked;
         let temp_unblocked_nodes = &mut self.temp_unblocked_nodes;
         temp_unblocked_nodes.clear();
-        processor.unblocked(self.offset.as_ref().unwrap(), |var| {
+        processor.notify_unblocked(self.watcher_offset.as_ref().unwrap(), |var| {
             if let Some(unblocked_nodes) = stalled_on.remove(&var) {
                 for node_index in unblocked_nodes {
                     let node = &nodes[node_index];
@@ -861,9 +869,8 @@ impl<O: ForestObligation> ObligationForest<O> {
         }
     }
 
-    /// Compresses the vector, removing all popped nodes. This adjusts the
-    /// indices and hence invalidates any outstanding indices. `process_cycles`
-    /// must be run beforehand to remove any cycles on `Success` nodes.
+    /// Compresses the forest, moving all nodes marked `Done` or `Error` into `dead_nodes` for later reuse
+    /// `process_cycles` must be run beforehand to remove any cycles on `Success` nodes.
     #[inline(never)]
     fn compress(&mut self, do_completed: DoCompleted) -> Option<Vec<O>> {
         let mut removed_done_obligations: Vec<O> = vec![];
@@ -872,6 +879,8 @@ impl<O: ForestObligation> ObligationForest<O> {
         let mut error_or_done_nodes = mem::take(self.error_or_done_nodes.get_mut());
         for &index in &error_or_done_nodes {
             let node = &mut self.nodes[index];
+
+            // Remove this node from all the nodes that depends on it
             let reverse_dependents = mem::take(&mut node.reverse_dependents);
             for &reverse_index in &reverse_dependents {
                 let reverse_node = &mut self.nodes[reverse_index];
@@ -892,6 +901,8 @@ impl<O: ForestObligation> ObligationForest<O> {
                     if let Some(opt) = self.active_cache.get_mut(&node.obligation.as_cache_key()) {
                         *opt = CacheState::Done;
                     }
+                    // If the node's predicate changed at some point we mark all its alternate
+                    // predicates as done as well
                     for alt in &node.alternative_predicates {
                         if let Some(opt) = self.active_cache.get_mut(alt) {
                             *opt = CacheState::Done;
@@ -903,13 +914,17 @@ impl<O: ForestObligation> ObligationForest<O> {
                         removed_done_obligations.push(node.obligation.clone());
                     }
 
+                    // Store the node so it and its allocations can be used when another node is
+                    // allocated
                     self.dead_nodes.push(index);
                 }
                 NodeState::Error => {
                     // We *intentionally* remove the node from the cache at this point. Otherwise
                     // tests must come up with a different type on every type error they
                     // check against.
                     self.active_cache.remove(&node.obligation.as_cache_key());
+                    // If the node's predicate changed at some point we remove all its alternate
+                    // predicates as well
                     for alt in &node.alternative_predicates {
                         self.active_cache.remove(alt);
                     }

compiler/rustc_data_structures/src/obligation_forest/tests.rs

@@ -93,7 +93,7 @@ where
     {
     }
 
-    fn unblocked(
+    fn notify_unblocked(
         &self,
         _offset: &<Self::Obligation as ForestObligation>::WatcherOffset,
         _f: impl FnMut(<Self::Obligation as ForestObligation>::Variable),