Deduplicate errors in the obligation forest.

orium · orium · commit 6bfa6aa87255 · 2018-09-30T20:01:35.000+01:00
Fixes #40827.
diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs
@@ -65,6 +65,12 @@ pub enum ProcessResult<O, E> {
     Error(E),
 }
 
+#[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)]
+struct ObligationTreeId(usize);
+
+type ObligationTreeIdGenerator =
+    ::std::iter::Map<::std::ops::RangeFrom<usize>, fn(usize) -> ObligationTreeId>;
+
 pub struct ObligationForest<O: ForestObligation> {
     /// The list of obligations. In between calls to
     /// `process_obligations`, this list only contains nodes in the
@@ -79,11 +85,25 @@ pub struct ObligationForest<O: ForestObligation> {
     /// at a higher index than its parent. This is needed by the
     /// backtrace iterator (which uses `split_at`).
     nodes: Vec<Node<O>>,
+
     /// A cache of predicates that have been successfully completed.
     done_cache: FxHashSet<O::Predicate>,
+
     /// An cache of the nodes in `nodes`, indexed by predicate.
     waiting_cache: FxHashMap<O::Predicate, NodeIndex>,
+
     scratch: Option<Vec<usize>>,
+
+    obligation_tree_id_generator: ObligationTreeIdGenerator,
+
+    /// Per tree error cache.  This is used to deduplicate errors,
+    /// which is necessary to avoid trait resolution overflow in
+    /// some cases.
+    ///
+    /// See [this][details] for details.
+    ///
+    /// [details]: https://github.com/rust-lang/rust/pull/53255#issuecomment-421184780
+    error_cache: FxHashMap<ObligationTreeId, FxHashSet<O::Predicate>>,
 }
 
 #[derive(Debug)]
@@ -99,6 +119,9 @@ struct Node<O> {
     /// Obligations that depend on this obligation for their
     /// completion. They must all be in a non-pending state.
     dependents: Vec<NodeIndex>,
+
+    /// Identifier of the obligation tree to which this node belongs.
+    obligation_tree_id: ObligationTreeId,
 }
 
 /// The state of one node in some tree within the forest. This
@@ -165,6 +188,8 @@ impl<O: ForestObligation> ObligationForest<O> {
             done_cache: FxHashSet(),
             waiting_cache: FxHashMap(),
             scratch: Some(vec![]),
+            obligation_tree_id_generator: (0..).map(|i| ObligationTreeId(i)),
+            error_cache: FxHashMap(),
         }
     }
 
@@ -214,9 +239,29 @@ impl<O: ForestObligation> ObligationForest<O> {
             Entry::Vacant(v) => {
                 debug!("register_obligation_at({:?}, {:?}) - ok, new index is {}",
                        obligation, parent, self.nodes.len());
-                v.insert(NodeIndex::new(self.nodes.len()));
-                self.nodes.push(Node::new(parent, obligation));
-                Ok(())
+
+                let obligation_tree_id = match parent {
+                    Some(p) => {
+                        let parent_node = &self.nodes[p.get()];
+                        parent_node.obligation_tree_id
+                    }
+                    None => self.obligation_tree_id_generator.next().unwrap()
+                };
+
+                let already_failed =
+                    parent.is_some()
+                        && self.error_cache
+                            .get(&obligation_tree_id)
+                            .map(|errors| errors.contains(obligation.as_predicate()))
+                            .unwrap_or(false);
+
+                if already_failed {
+                    Err(())
+                } else {
+                    v.insert(NodeIndex::new(self.nodes.len()));
+                    self.nodes.push(Node::new(parent, obligation, obligation_tree_id));
+                    Ok(())
+                }
             }
         }
     }
@@ -251,6 +296,15 @@ impl<O: ForestObligation> ObligationForest<O> {
             .collect()
     }
 
+    fn insert_into_error_cache(&mut self, node_index: usize) {
+        let node = &self.nodes[node_index];
+
+        self.error_cache
+            .entry(node.obligation_tree_id)
+            .or_insert_with(|| FxHashSet())
+            .insert(node.obligation.as_predicate().clone());
+    }
+
     /// Perform a pass through the obligation list. This must
     /// be called in a loop until `outcome.stalled` is false.
     ///
@@ -264,22 +318,15 @@ impl<O: ForestObligation> ObligationForest<O> {
         let mut stalled = true;
 
         for index in 0..self.nodes.len() {
-            debug!("process_obligations: node {} == {:?}",
-                   index,
-                   self.nodes[index]);
+            debug!("process_obligations: node {} == {:?}", index, self.nodes[index]);
 
             let result = match self.nodes[index] {
-                Node { ref state, ref mut obligation, .. }
-                    if state.get() == NodeState::Pending =>
-                {
-                    processor.process_obligation(obligation)
-                }
+                Node { ref state, ref mut obligation, .. } if state.get() == NodeState::Pending =>
+                    processor.process_obligation(obligation),
                 _ => continue
             };
 
-            debug!("process_obligations: node {} got result {:?}",
-                   index,
-                   result);
+            debug!("process_obligations: node {} got result {:?}", index, result);
 
             match result {
                 ProcessResult::Unchanged => {
@@ -420,13 +467,13 @@ impl<O: ForestObligation> ObligationForest<O> {
         }
 
         while let Some(i) = error_stack.pop() {
-            let node = &self.nodes[i];
-
-            match node.state.get() {
+            match self.nodes[i].state.get() {
                 NodeState::Error => continue,
-                _ => node.state.set(NodeState::Error)
+                _ => self.nodes[i].state.set(NodeState::Error),
             }
 
+            let node = &self.nodes[i];
+
             error_stack.extend(
                 node.parent.iter().chain(node.dependents.iter()).map(|x| x.get())
             );
@@ -514,6 +561,7 @@ impl<O: ForestObligation> ObligationForest<O> {
                     self.waiting_cache.remove(self.nodes[i].obligation.as_predicate());
                     node_rewrites[i] = nodes_len;
                     dead_nodes += 1;
+                    self.insert_into_error_cache(i);
                 }
                 NodeState::OnDfsStack | NodeState::Success => unreachable!()
             }
@@ -587,12 +635,17 @@ impl<O: ForestObligation> ObligationForest<O> {
 }
 
 impl<O> Node<O> {
-    fn new(parent: Option<NodeIndex>, obligation: O) -> Node<O> {
+    fn new(
+        parent: Option<NodeIndex>,
+        obligation: O,
+        obligation_tree_id: ObligationTreeId
+    ) -> Node<O> {
         Node {
             obligation,
             state: Cell::new(NodeState::Pending),
             parent,
             dependents: vec![],
+            obligation_tree_id,
         }
     }
 }
diff --git a/src/test/ui/issue-40827.rs b/src/test/ui/issue-40827.rs
@@ -0,0 +1,27 @@
+// Copyright 2018 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.
+
+use std::rc::Rc;
+use std::sync::Arc;
+
+struct Foo(Arc<Bar>);
+
+enum Bar {
+    A(Rc<Foo>),
+    B(Option<Foo>),
+}
+
+fn f<T: Send>(_: T) {}
+
+fn main() {
+    f(Foo(Arc::new(Bar::B(None))));
+    //~^ ERROR E0277
+    //~| ERROR E0277
+}
diff --git a/src/test/ui/issue-40827.stderr b/src/test/ui/issue-40827.stderr
@@ -0,0 +1,35 @@
+error[E0277]: `std::rc::Rc<Foo>` cannot be sent between threads safely
+  --> $DIR/issue-40827.rs:24:5
+   |
+LL |     f(Foo(Arc::new(Bar::B(None))));
+   |     ^ `std::rc::Rc<Foo>` cannot be sent between threads safely
+   |
+   = help: within `Bar`, the trait `std::marker::Send` is not implemented for `std::rc::Rc<Foo>`
+   = note: required because it appears within the type `Bar`
+   = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<Bar>`
+   = note: required because it appears within the type `Foo`
+note: required by `f`
+  --> $DIR/issue-40827.rs:21:1
+   |
+LL | fn f<T: Send>(_: T) {}
+   | ^^^^^^^^^^^^^^^^^^^
+
+error[E0277]: `std::rc::Rc<Foo>` cannot be shared between threads safely
+  --> $DIR/issue-40827.rs:24:5
+   |
+LL |     f(Foo(Arc::new(Bar::B(None))));
+   |     ^ `std::rc::Rc<Foo>` cannot be shared between threads safely
+   |
+   = help: within `Bar`, the trait `std::marker::Sync` is not implemented for `std::rc::Rc<Foo>`
+   = note: required because it appears within the type `Bar`
+   = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<Bar>`
+   = note: required because it appears within the type `Foo`
+note: required by `f`
+  --> $DIR/issue-40827.rs:21:1
+   |
+LL | fn f<T: Send>(_: T) {}
+   | ^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0277`.
diff --git a/src/test/ui/recursion/recursive-requirements.rs b/src/test/ui/recursion/recursive-requirements.rs
@@ -23,5 +23,7 @@ pub struct Bar {
 }
 
 fn main() {
-    let _: AssertSync<Foo> = unimplemented!(); //~ ERROR E0275
+    let _: AssertSync<Foo> = unimplemented!();
+    //~^ ERROR E0277
+    //~| ERROR E0277
 }
diff --git a/src/test/ui/recursion/recursive-requirements.stderr b/src/test/ui/recursion/recursive-requirements.stderr
@@ -1,15 +1,33 @@
-error[E0275]: overflow evaluating the requirement `Foo: std::marker::Sync`
+error[E0277]: `*const Bar` cannot be shared between threads safely
   --> $DIR/recursive-requirements.rs:26:12
    |
-LL |     let _: AssertSync<Foo> = unimplemented!(); //~ ERROR E0275
-   |            ^^^^^^^^^^^^^^^
+LL |     let _: AssertSync<Foo> = unimplemented!();
+   |            ^^^^^^^^^^^^^^^ `*const Bar` cannot be shared between threads safely
    |
-   = help: consider adding a `#![recursion_limit="128"]` attribute to your crate
-   = note: required because it appears within the type `std::marker::PhantomData<Foo>`
+   = help: within `Foo`, the trait `std::marker::Sync` is not implemented for `*const Bar`
+   = note: required because it appears within the type `Foo`
+note: required by `AssertSync`
+  --> $DIR/recursive-requirements.rs:13:1
+   |
+LL | struct AssertSync<T: Sync>(PhantomData<T>);
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error[E0277]: `*const Foo` cannot be shared between threads safely
+  --> $DIR/recursive-requirements.rs:26:12
+   |
+LL |     let _: AssertSync<Foo> = unimplemented!();
+   |            ^^^^^^^^^^^^^^^ `*const Foo` cannot be shared between threads safely
+   |
+   = help: within `Foo`, the trait `std::marker::Sync` is not implemented for `*const Foo`
    = note: required because it appears within the type `Bar`
    = note: required because it appears within the type `std::marker::PhantomData<Bar>`
    = note: required because it appears within the type `Foo`
+note: required by `AssertSync`
+  --> $DIR/recursive-requirements.rs:13:1
+   |
+LL | struct AssertSync<T: Sync>(PhantomData<T>);
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-error: aborting due to previous error
+error: aborting due to 2 previous errors
 
-For more information about this error, try `rustc --explain E0275`.
+For more information about this error, try `rustc --explain E0277`.

Original file line number	Diff line number	Diff line change
`@@ -23,5 +23,7 @@ pub struct Bar {`
`23`	`23`	`}`
`24`	`24`
`25`	`25`	`fn main() {`
`26`		`- let _: AssertSync<Foo> = unimplemented!(); //~ ERROR E0275`
	`26`	`+ let _: AssertSync<Foo> = unimplemented!();`
	`27`	`+ //~^ ERROR E0277`
	`28`	`+ //~\| ERROR E0277`
`27`	`29`	`}`