fail obligations that depend on erroring obligations

Fix a bug where an obligation that depend on an erroring obligation would
be regarded as successful, leading to global cache pollution and random
lossage.

Fixes #33723.
Fixes #34503.

2 files changed, 67 insertions, 8 deletions

diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs
index b713b2285a6..c079146edbf 100644
--- a/src/librustc_data_structures/obligation_forest/mod.rs
+++ b/src/librustc_data_structures/obligation_forest/mod.rs
@@ -208,11 +208,17 @@ impl<O: ForestObligation> ObligationForest<O> {
     ///
     /// This CAN be done in a snapshot
     pub fn register_obligation(&mut self, obligation: O) {
-        self.register_obligation_at(obligation, None)
+        // Ignore errors here - there is no guarantee of success.
+        let _ = self.register_obligation_at(obligation, None);
     }
 
-    fn register_obligation_at(&mut self, obligation: O, parent: Option<NodeIndex>) {
-        if self.done_cache.contains(obligation.as_predicate()) { return }
+    // returns Err(()) if we already know this obligation failed.
+    fn register_obligation_at(&mut self, obligation: O, parent: Option<NodeIndex>)
+                              -> Result<(), ()>
+    {
+        if self.done_cache.contains(obligation.as_predicate()) {
+            return Ok(())
+        }
 
         match self.waiting_cache.entry(obligation.as_predicate().clone()) {
             Entry::Occupied(o) => {
@@ -226,6 +232,11 @@ impl<O: ForestObligation> ObligationForest<O> {
                         self.nodes[o.get().get()].dependents.push(parent);
                     }
                 }
+                if let NodeState::Error = self.nodes[o.get().get()].state.get() {
+                    Err(())
+                } else {
+                    Ok(())
+                }
             }
             Entry::Vacant(v) => {
                 debug!("register_obligation_at({:?}, {:?}) - ok",
@@ -233,8 +244,9 @@ impl<O: ForestObligation> ObligationForest<O> {
                 v.insert(NodeIndex::new(self.nodes.len()));
                 self.cache_list.push(obligation.as_predicate().clone());
                 self.nodes.push(Node::new(parent, obligation));
+                Ok(())
             }
-        };
+        }
     }
 
     /// Convert all remaining obligations to the given error.
@@ -306,12 +318,19 @@ impl<O: ForestObligation> ObligationForest<O> {
                 Ok(Some(children)) => {
                     // if we saw a Some(_) result, we are not (yet) stalled
                     stalled = false;
+                    self.nodes[index].state.set(NodeState::Success);
+
                     for child in children {
-                        self.register_obligation_at(child,
-                                                    Some(NodeIndex::new(index)));
+                        let st = self.register_obligation_at(
+                            child,
+                            Some(NodeIndex::new(index))
+                        );
+                        if let Err(()) = st {
+                            // error already reported - propagate it
+                            // to our node.
+                            self.error_at(index);
+                        }
                     }
-
-                    self.nodes[index].state.set(NodeState::Success);
                 }
                 Err(err) => {
                     let backtrace = self.error_at(index);
diff --git a/src/librustc_data_structures/obligation_forest/test.rs b/src/librustc_data_structures/obligation_forest/test.rs
index 8eac8892a3e..a95b2b84b34 100644
--- a/src/librustc_data_structures/obligation_forest/test.rs
+++ b/src/librustc_data_structures/obligation_forest/test.rs
@@ -418,3 +418,43 @@ fn orphan() {
     let errors = forest.to_errors(());
     assert_eq!(errors.len(), 0);
 }
+
+#[test]
+fn simultaneous_register_and_error() {
+    // check that registering a failed obligation works correctly
+    let mut forest = ObligationForest::new();
+    forest.register_obligation("A");
+    forest.register_obligation("B");
+
+    let Outcome { completed: ok, errors: err, .. } =
+        forest.process_obligations(&mut C(|obligation| {
+            match *obligation {
+                "A" => Err("An error"),
+                "B" => Ok(Some(vec!["A"])),
+                _ => unreachable!(),
+            }
+        }, |_|{}));
+    assert_eq!(ok.len(), 0);
+    assert_eq!(err, vec![super::Error {
+        error: "An error",
+        backtrace: vec!["A"]
+    }]);
+
+    let mut forest = ObligationForest::new();
+    forest.register_obligation("B");
+    forest.register_obligation("A");
+
+    let Outcome { completed: ok, errors: err, .. } =
+        forest.process_obligations(&mut C(|obligation| {
+            match *obligation {
+                "A" => Err("An error"),
+                "B" => Ok(Some(vec!["A"])),
+                _ => unreachable!(),
+            }
+        }, |_|{}));
+    assert_eq!(ok.len(), 0);
+    assert_eq!(err, vec![super::Error {
+        error: "An error",
+        backtrace: vec!["A"]
+    }]);
+}