Auto merge of #34605 - arielb1:bug-in-the-jungle, r=eddyb

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.

r? @eddyb since @nikomatsakis is on vacation

beta-nominating because of the massive lossage potential (e.g. with `Copy` this could lead to random memory leaks), plus this is a regression.

Author: bors

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,15 +232,21 @@ 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",
                        obligation, parent);
                 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);

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"]
+    }]);
+}

src/test/run-pass/issue-34503.rs

@@ -0,0 +1,20 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+fn main() {
+    struct X;
+    trait Foo<T> {
+        fn foo(&self) where (T, Option<T>): Ord {}
+        fn bar(&self, x: &Option<T>) -> bool
+        where Option<T>: Ord { *x < *x }
+    }
+    impl Foo<X> for () {}
+    let _ = &() as &Foo<X>;
+}