From 3b506d9694beaed3b07c9a4fec472646c9e9e17d Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 24 Sep 2019 13:25:29 +1000 Subject: [PATCH 01/11] Remove `NodeState::OnDfsStack`. It's not necessary; cycles (which are rare) can be detected by looking at the node stack. This change speeds things up slightly, as well as simplifying the code a little. --- .../obligation_forest/mod.rs | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 1c7109fe500e0..3cbd955a72815 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -235,10 +235,6 @@ enum NodeState { /// This obligation was resolved to an error. Error nodes are /// removed from the vector by the compression step. Error, - - /// This is a temporary state used in DFS loops to detect cycles, - /// it should not exist outside of these DFSes. - OnDfsStack, } #[derive(Debug)] @@ -491,7 +487,6 @@ impl ObligationForest { NodeState::Pending => {}, NodeState::Success => self.find_cycles_from_node(&mut stack, processor, index), NodeState::Waiting | NodeState::Done | NodeState::Error => {}, - NodeState::OnDfsStack => self.find_cycles_from_node(&mut stack, processor, index), } } @@ -506,20 +501,25 @@ impl ObligationForest { { let node = &self.nodes[index]; match node.state.get() { - NodeState::OnDfsStack => { - let rpos = stack.iter().rposition(|&n| n == index).unwrap(); - processor.process_backedge(stack[rpos..].iter().map(GetObligation(&self.nodes)), - PhantomData); - } NodeState::Success => { - node.state.set(NodeState::OnDfsStack); - stack.push(index); - for &index in node.dependents.iter() { - self.find_cycles_from_node(stack, processor, index); + match stack.iter().rposition(|&n| n == index) { + None => { + stack.push(index); + for &index in node.dependents.iter() { + self.find_cycles_from_node(stack, processor, index); + } + stack.pop(); + node.state.set(NodeState::Done); + } + Some(rpos) => { + // Cycle detected. + processor.process_backedge( + stack[rpos..].iter().map(GetObligation(&self.nodes)), + PhantomData + ); + } } - stack.pop(); - node.state.set(NodeState::Done); - }, + } NodeState::Waiting | NodeState::Pending => { // This node is still reachable from some pending node. We // will get to it when they are all processed. @@ -598,7 +598,7 @@ impl ObligationForest { fn mark_as_waiting_from(&self, node: &Node) { match node.state.get() { - NodeState::Waiting | NodeState::Error | NodeState::OnDfsStack => return, + NodeState::Waiting | NodeState::Error => return, NodeState::Success => node.state.set(NodeState::Waiting), NodeState::Pending | NodeState::Done => {}, } @@ -659,7 +659,7 @@ impl ObligationForest { dead_nodes += 1; self.insert_into_error_cache(index); } - NodeState::OnDfsStack | NodeState::Success => unreachable!() + NodeState::Success => unreachable!() } } From 4a7fb8b13ad9a42e96ebbeaef8ddc1ebca3d65c1 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 24 Sep 2019 13:26:47 +1000 Subject: [PATCH 02/11] Rename `node_index` as `index`. For consistency with other variables in this file. --- src/librustc_data_structures/obligation_forest/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 3cbd955a72815..de584a22e9c99 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -373,8 +373,8 @@ impl ObligationForest { .collect() } - fn insert_into_error_cache(&mut self, node_index: usize) { - let node = &self.nodes[node_index]; + fn insert_into_error_cache(&mut self, index: usize) { + let node = &self.nodes[index]; self.error_cache .entry(node.obligation_tree_id) .or_default() From 85a56cbd88931db032417429d45c75d5049d0c2e Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 24 Sep 2019 14:55:39 +1000 Subject: [PATCH 03/11] Inline `mark_as_waiting_from`. It has a single call site, and the code is easier to read this way. --- .../obligation_forest/mod.rs | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index de584a22e9c99..3cf3bfc1fd4fe 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -570,7 +570,19 @@ impl ObligationForest { #[inline(always)] fn inlined_mark_neighbors_as_waiting_from(&self, node: &Node) { for &index in node.dependents.iter() { - self.mark_as_waiting_from(&self.nodes[index]); + let node = &self.nodes[index]; + match node.state.get() { + NodeState::Waiting | NodeState::Error => {} + NodeState::Success => { + node.state.set(NodeState::Waiting); + // This call site is cold. + self.uninlined_mark_neighbors_as_waiting_from(node); + } + NodeState::Pending | NodeState::Done => { + // This call site is cold. + self.uninlined_mark_neighbors_as_waiting_from(node); + } + } } } @@ -596,17 +608,6 @@ impl ObligationForest { } } - fn mark_as_waiting_from(&self, node: &Node) { - match node.state.get() { - NodeState::Waiting | NodeState::Error => return, - NodeState::Success => node.state.set(NodeState::Waiting), - NodeState::Pending | NodeState::Done => {}, - } - - // This call site is cold. - self.uninlined_mark_neighbors_as_waiting_from(node); - } - /// Compresses the vector, removing all popped nodes. This adjusts /// the indices and hence invalidates any outstanding /// indices. Cannot be used during a transaction. From 7130e6c0733ca4e6d8fc1dd08a882ff3547c94ba Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 24 Sep 2019 15:35:10 +1000 Subject: [PATCH 04/11] Tweak the control flow in `process_obligations()`. --- src/librustc_data_structures/obligation_forest/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 3cf3bfc1fd4fe..77eb0beeec302 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -404,10 +404,10 @@ impl ObligationForest { // `self.active_cache`. This means that `self.active_cache` can get // out of sync with `nodes`. It's not very common, but it does // happen, and code in `compress` has to allow for it. - let result = match node.state.get() { - NodeState::Pending => processor.process_obligation(&mut node.obligation), - _ => continue - }; + if node.state.get() != NodeState::Pending { + continue; + } + let result = processor.process_obligation(&mut node.obligation); debug!("process_obligations: node {} got result {:?}", index, result); From ea726501e1b86d8efa5c800a7a218aed7ee1503c Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 24 Sep 2019 18:22:55 +1000 Subject: [PATCH 05/11] Use `filter` and `map` in `to_errors`. --- .../obligation_forest/mod.rs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 77eb0beeec302..ccc8aa288d484 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -348,15 +348,16 @@ impl ObligationForest { /// Converts all remaining obligations to the given error. pub fn to_errors(&mut self, error: E) -> Vec> { - let mut errors = vec![]; - for (index, node) in self.nodes.iter().enumerate() { - if let NodeState::Pending = node.state.get() { - errors.push(Error { + let errors = self.nodes.iter().enumerate() + .filter(|(_index, node)| node.state.get() == NodeState::Pending) + .map(|(index, _node)| { + Error { error: error.clone(), backtrace: self.error_at(index), - }); - } - } + } + }) + .collect(); + let successful_obligations = self.compress(DoCompleted::Yes); assert!(successful_obligations.unwrap().is_empty()); errors @@ -366,10 +367,9 @@ impl ObligationForest { pub fn map_pending_obligations(&self, f: F) -> Vec

where F: Fn(&O) -> P { - self.nodes - .iter() - .filter(|n| n.state.get() == NodeState::Pending) - .map(|n| f(&n.obligation)) + self.nodes.iter() + .filter(|node| node.state.get() == NodeState::Pending) + .map(|node| f(&node.obligation)) .collect() } From 22943ee27c28efe76054a49ce20f0ab808b0599f Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 24 Sep 2019 19:00:42 +1000 Subject: [PATCH 06/11] Remove unnecessary uses of `ObligationForest::scratch`. They don't help performance at all, and just complicate things. --- .../obligation_forest/mod.rs | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index ccc8aa288d484..766e1d3fae32d 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -151,9 +151,8 @@ pub struct ObligationForest { /// comments in `process_obligation` for details. active_cache: FxHashMap, - /// A scratch vector reused in various operations, to avoid allocating new - /// vectors. - scratch: RefCell>, + /// A vector reused in compress(), to avoid allocating new vectors. + node_rewrites: RefCell>, obligation_tree_id_generator: ObligationTreeIdGenerator, @@ -275,7 +274,7 @@ impl ObligationForest { nodes: vec![], done_cache: Default::default(), active_cache: Default::default(), - scratch: RefCell::new(vec![]), + node_rewrites: RefCell::new(vec![]), obligation_tree_id_generator: (0..).map(ObligationTreeId), error_cache: Default::default(), } @@ -472,8 +471,7 @@ impl ObligationForest { fn process_cycles

(&self, processor: &mut P) where P: ObligationProcessor { - let mut stack = self.scratch.replace(vec![]); - debug_assert!(stack.is_empty()); + let mut stack = vec![]; debug!("process_cycles()"); @@ -493,7 +491,6 @@ impl ObligationForest { debug!("process_cycles: complete"); debug_assert!(stack.is_empty()); - self.scratch.replace(stack); } fn find_cycles_from_node

(&self, stack: &mut Vec, processor: &mut P, index: usize) @@ -533,7 +530,7 @@ impl ObligationForest { /// Returns a vector of obligations for `p` and all of its /// ancestors, putting them into the error state in the process. fn error_at(&self, mut index: usize) -> Vec { - let mut error_stack = self.scratch.replace(vec![]); + let mut error_stack: Vec = vec![]; let mut trace = vec![]; loop { @@ -562,7 +559,6 @@ impl ObligationForest { error_stack.extend(node.dependents.iter()); } - self.scratch.replace(error_stack); trace } @@ -617,7 +613,7 @@ impl ObligationForest { #[inline(never)] fn compress(&mut self, do_completed: DoCompleted) -> Option> { let nodes_len = self.nodes.len(); - let mut node_rewrites: Vec<_> = self.scratch.replace(vec![]); + let mut node_rewrites: Vec<_> = self.node_rewrites.replace(vec![]); node_rewrites.extend(0..nodes_len); let mut dead_nodes = 0; @@ -667,7 +663,7 @@ impl ObligationForest { // No compression needed. if dead_nodes == 0 { node_rewrites.truncate(0); - self.scratch.replace(node_rewrites); + self.node_rewrites.replace(node_rewrites); return if do_completed == DoCompleted::Yes { Some(vec![]) } else { None }; } @@ -690,7 +686,7 @@ impl ObligationForest { self.apply_rewrites(&node_rewrites); node_rewrites.truncate(0); - self.scratch.replace(node_rewrites); + self.node_rewrites.replace(node_rewrites); successful } From 6fb1f37888f2e306344adaab0adc5f0f4a454de0 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 25 Sep 2019 11:35:01 +1000 Subject: [PATCH 07/11] Introduce some intermediate variables that aid readability. --- src/librustc_data_structures/obligation_forest/mod.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 766e1d3fae32d..5b99e2ae140bb 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -300,9 +300,10 @@ impl ObligationForest { match self.active_cache.entry(obligation.as_predicate().clone()) { Entry::Occupied(o) => { + let index = *o.get(); debug!("register_obligation_at({:?}, {:?}) - duplicate of {:?}!", - obligation, parent, o.get()); - let node = &mut self.nodes[*o.get()]; + obligation, parent, index); + let node = &mut self.nodes[index]; if let Some(parent_index) = parent { // If the node is already in `active_cache`, it has already // had its chance to be marked with a parent. So if it's @@ -337,7 +338,8 @@ impl ObligationForest { if already_failed { Err(()) } else { - v.insert(self.nodes.len()); + let new_index = self.nodes.len(); + v.insert(new_index); self.nodes.push(Node::new(parent, obligation, obligation_tree_id)); Ok(()) } From 9e67f19eee97e9afb4d030b1949dc8084da64067 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 25 Sep 2019 12:03:31 +1000 Subject: [PATCH 08/11] Convert some `match` expressions to `if`s. These make the code more concise. --- .../obligation_forest/mod.rs | 57 +++++++------------ 1 file changed, 21 insertions(+), 36 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 5b99e2ae140bb..1ff07bd67f17f 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -481,12 +481,8 @@ impl ObligationForest { // For some benchmarks this state test is extremely // hot. It's a win to handle the no-op cases immediately to avoid // the cost of the function call. - match node.state.get() { - // Match arms are in order of frequency. Pending, Success and - // Waiting dominate; the others are rare. - NodeState::Pending => {}, - NodeState::Success => self.find_cycles_from_node(&mut stack, processor, index), - NodeState::Waiting | NodeState::Done | NodeState::Error => {}, + if node.state.get() == NodeState::Success { + self.find_cycles_from_node(&mut stack, processor, index); } } @@ -499,34 +495,25 @@ impl ObligationForest { where P: ObligationProcessor { let node = &self.nodes[index]; - match node.state.get() { - NodeState::Success => { - match stack.iter().rposition(|&n| n == index) { - None => { - stack.push(index); - for &index in node.dependents.iter() { - self.find_cycles_from_node(stack, processor, index); - } - stack.pop(); - node.state.set(NodeState::Done); - } - Some(rpos) => { - // Cycle detected. - processor.process_backedge( - stack[rpos..].iter().map(GetObligation(&self.nodes)), - PhantomData - ); + if node.state.get() == NodeState::Success { + match stack.iter().rposition(|&n| n == index) { + None => { + stack.push(index); + for &index in node.dependents.iter() { + self.find_cycles_from_node(stack, processor, index); } + stack.pop(); + node.state.set(NodeState::Done); + } + Some(rpos) => { + // Cycle detected. + processor.process_backedge( + stack[rpos..].iter().map(GetObligation(&self.nodes)), + PhantomData + ); } } - NodeState::Waiting | NodeState::Pending => { - // This node is still reachable from some pending node. We - // will get to it when they are all processed. - } - NodeState::Done | NodeState::Error => { - // Already processed that node. - } - }; + } } /// Returns a vector of obligations for `p` and all of its @@ -553,12 +540,10 @@ impl ObligationForest { while let Some(index) = error_stack.pop() { let node = &self.nodes[index]; - match node.state.get() { - NodeState::Error => continue, - _ => node.state.set(NodeState::Error), + if node.state.get() != NodeState::Error { + node.state.set(NodeState::Error); + error_stack.extend(node.dependents.iter()); } - - error_stack.extend(node.dependents.iter()); } trace From 8a62bb1a1d2ab3f937192c80794252716920c4f0 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 25 Sep 2019 13:02:24 +1000 Subject: [PATCH 09/11] Rename `nodes_len` and use it in a few more places. --- .../obligation_forest/mod.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 1ff07bd67f17f..1aac3ef314b23 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -599,9 +599,9 @@ impl ObligationForest { /// on these nodes may be present. This is done by e.g., `process_cycles`. #[inline(never)] fn compress(&mut self, do_completed: DoCompleted) -> Option> { - let nodes_len = self.nodes.len(); + let orig_nodes_len = self.nodes.len(); let mut node_rewrites: Vec<_> = self.node_rewrites.replace(vec![]); - node_rewrites.extend(0..nodes_len); + node_rewrites.extend(0..orig_nodes_len); let mut dead_nodes = 0; // Now move all popped nodes to the end. Try to keep the order. @@ -610,7 +610,7 @@ impl ObligationForest { // self.nodes[0..index - dead_nodes] are the first remaining nodes // self.nodes[index - dead_nodes..index] are all dead // self.nodes[index..] are unchanged - for index in 0..self.nodes.len() { + for index in 0..orig_nodes_len { let node = &self.nodes[index]; match node.state.get() { NodeState::Pending | NodeState::Waiting => { @@ -631,7 +631,7 @@ impl ObligationForest { } else { self.done_cache.insert(node.obligation.as_predicate().clone()); } - node_rewrites[index] = nodes_len; + node_rewrites[index] = orig_nodes_len; dead_nodes += 1; } NodeState::Error => { @@ -639,7 +639,7 @@ impl ObligationForest { // tests must come up with a different type on every type error they // check against. self.active_cache.remove(node.obligation.as_predicate()); - node_rewrites[index] = nodes_len; + node_rewrites[index] = orig_nodes_len; dead_nodes += 1; self.insert_into_error_cache(index); } @@ -667,7 +667,7 @@ impl ObligationForest { }) .collect()) } else { - self.nodes.truncate(self.nodes.len() - dead_nodes); + self.nodes.truncate(orig_nodes_len - dead_nodes); None }; self.apply_rewrites(&node_rewrites); @@ -679,13 +679,13 @@ impl ObligationForest { } fn apply_rewrites(&mut self, node_rewrites: &[usize]) { - let nodes_len = node_rewrites.len(); + let orig_nodes_len = node_rewrites.len(); for node in &mut self.nodes { let mut i = 0; while i < node.dependents.len() { let new_index = node_rewrites[node.dependents[i]]; - if new_index >= nodes_len { + if new_index >= orig_nodes_len { node.dependents.swap_remove(i); if i == 0 && node.has_parent { // We just removed the parent. @@ -702,7 +702,7 @@ impl ObligationForest { // removal of nodes within `compress` can fail. See above. self.active_cache.retain(|_predicate, index| { let new_index = node_rewrites[*index]; - if new_index >= nodes_len { + if new_index >= orig_nodes_len { false } else { *index = new_index; From 2883c258f1a6dd82f6acd174b71d74e04e645f6d Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 25 Sep 2019 13:03:05 +1000 Subject: [PATCH 10/11] Remove an out-of-date sentence in a comment. --- src/librustc_data_structures/obligation_forest/mod.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 1aac3ef314b23..f194fef49e3ad 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -591,9 +591,8 @@ impl ObligationForest { } } - /// Compresses the vector, removing all popped nodes. This adjusts - /// the indices and hence invalidates any outstanding - /// indices. Cannot be used during a transaction. + /// Compresses the vector, removing all popped nodes. This adjusts the + /// indices and hence invalidates any outstanding indices. /// /// Beforehand, all nodes must be marked as `Done` and no cycles /// on these nodes may be present. This is done by e.g., `process_cycles`. From a820672f6c947c5f487d10a011b9b1e65c29f693 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 26 Sep 2019 14:18:37 +1000 Subject: [PATCH 11/11] Avoid the popping loop at the end of `compress()`. By collecting the done obligations (when necessary) in the main loop. This makes the code cleaner. The commit also changes the order in which successful obligations are returned -- they are now returned in the registered order, rather than reversed. Because this order doesn't actually matter, being only used by tests, the commit uses `sort()` to make the test agnostic w.r.t. the order. --- .../obligation_forest/mod.rs | 46 ++++++++----------- .../obligation_forest/tests.rs | 28 ++++++++--- 2 files changed, 40 insertions(+), 34 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index f194fef49e3ad..68e9be5e06575 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -600,10 +600,13 @@ impl ObligationForest { fn compress(&mut self, do_completed: DoCompleted) -> Option> { let orig_nodes_len = self.nodes.len(); let mut node_rewrites: Vec<_> = self.node_rewrites.replace(vec![]); + debug_assert!(node_rewrites.is_empty()); node_rewrites.extend(0..orig_nodes_len); let mut dead_nodes = 0; + let mut removed_done_obligations: Vec = vec![]; - // Now move all popped nodes to the end. Try to keep the order. + // Now move all Done/Error nodes to the end, preserving the order of + // the Pending/Waiting nodes. // // LOOP INVARIANT: // self.nodes[0..index - dead_nodes] are the first remaining nodes @@ -620,7 +623,7 @@ impl ObligationForest { } NodeState::Done => { // This lookup can fail because the contents of - // `self.active_cache` is not guaranteed to match those of + // `self.active_cache` are not guaranteed to match those of // `self.nodes`. See the comment in `process_obligation` // for more details. if let Some((predicate, _)) = @@ -630,6 +633,10 @@ impl ObligationForest { } else { self.done_cache.insert(node.obligation.as_predicate().clone()); } + if do_completed == DoCompleted::Yes { + // Extract the success stories. + removed_done_obligations.push(node.obligation.clone()); + } node_rewrites[index] = orig_nodes_len; dead_nodes += 1; } @@ -638,43 +645,28 @@ impl ObligationForest { // tests must come up with a different type on every type error they // check against. self.active_cache.remove(node.obligation.as_predicate()); + self.insert_into_error_cache(index); node_rewrites[index] = orig_nodes_len; dead_nodes += 1; - self.insert_into_error_cache(index); } NodeState::Success => unreachable!() } } - // No compression needed. - if dead_nodes == 0 { - node_rewrites.truncate(0); - self.node_rewrites.replace(node_rewrites); - return if do_completed == DoCompleted::Yes { Some(vec![]) } else { None }; - } - - // Pop off all the nodes we killed and extract the success stories. - let successful = if do_completed == DoCompleted::Yes { - Some((0..dead_nodes) - .map(|_| self.nodes.pop().unwrap()) - .flat_map(|node| { - match node.state.get() { - NodeState::Error => None, - NodeState::Done => Some(node.obligation), - _ => unreachable!() - } - }) - .collect()) - } else { + if dead_nodes > 0 { + // Remove the dead nodes and rewrite indices. self.nodes.truncate(orig_nodes_len - dead_nodes); - None - }; - self.apply_rewrites(&node_rewrites); + self.apply_rewrites(&node_rewrites); + } node_rewrites.truncate(0); self.node_rewrites.replace(node_rewrites); - successful + if do_completed == DoCompleted::Yes { + Some(removed_done_obligations) + } else { + None + } } fn apply_rewrites(&mut self, node_rewrites: &[usize]) { diff --git a/src/librustc_data_structures/obligation_forest/tests.rs b/src/librustc_data_structures/obligation_forest/tests.rs index e20466572a26f..54b6f6d0adc6c 100644 --- a/src/librustc_data_structures/obligation_forest/tests.rs +++ b/src/librustc_data_structures/obligation_forest/tests.rs @@ -116,7 +116,9 @@ fn push_pop() { _ => unreachable!(), } }, |_| {}), DoCompleted::Yes); - assert_eq!(ok.unwrap(), vec!["A.3", "A.1", "A.3.i"]); + let mut ok = ok.unwrap(); + ok.sort(); + assert_eq!(ok, vec!["A.1", "A.3", "A.3.i"]); assert_eq!(err, vec![Error { error: "A is for apple", @@ -132,7 +134,9 @@ fn push_pop() { _ => panic!("unexpected obligation {:?}", obligation), } }, |_| {}), DoCompleted::Yes); - assert_eq!(ok.unwrap(), vec!["D.2.i", "D.2"]); + let mut ok = ok.unwrap(); + ok.sort(); + assert_eq!(ok, vec!["D.2", "D.2.i"]); assert_eq!(err, vec![Error { error: "D is for dumb", @@ -172,7 +176,9 @@ fn success_in_grandchildren() { _ => unreachable!(), } }, |_| {}), DoCompleted::Yes); - assert_eq!(ok.unwrap(), vec!["A.3", "A.1"]); + let mut ok = ok.unwrap(); + ok.sort(); + assert_eq!(ok, vec!["A.1", "A.3"]); assert!(err.is_empty()); let Outcome { completed: ok, errors: err, .. } = @@ -193,7 +199,9 @@ fn success_in_grandchildren() { _ => unreachable!(), } }, |_| {}), DoCompleted::Yes); - assert_eq!(ok.unwrap(), vec!["A.2.i.a", "A.2.i", "A.2", "A"]); + let mut ok = ok.unwrap(); + ok.sort(); + assert_eq!(ok, vec!["A", "A.2", "A.2.i", "A.2.i.a"]); assert!(err.is_empty()); let Outcome { completed: ok, errors: err, .. } = @@ -261,7 +269,9 @@ fn diamond() { } }, |_|{}), DoCompleted::Yes); assert_eq!(d_count, 1); - assert_eq!(ok.unwrap(), vec!["D", "A.2", "A.1", "A"]); + let mut ok = ok.unwrap(); + ok.sort(); + assert_eq!(ok, vec!["A", "A.1", "A.2", "D"]); assert_eq!(err.len(), 0); let errors = forest.to_errors(()); @@ -323,7 +333,9 @@ fn done_dependency() { _ => unreachable!(), } }, |_|{}), DoCompleted::Yes); - assert_eq!(ok.unwrap(), vec!["C: Sized", "B: Sized", "A: Sized"]); + let mut ok = ok.unwrap(); + ok.sort(); + assert_eq!(ok, vec!["A: Sized", "B: Sized", "C: Sized"]); assert_eq!(err.len(), 0); forest.register_obligation("(A,B,C): Sized"); @@ -361,7 +373,9 @@ fn orphan() { _ => unreachable!(), } }, |_|{}), DoCompleted::Yes); - assert_eq!(ok.unwrap(), vec!["C2", "C1"]); + let mut ok = ok.unwrap(); + ok.sort(); + assert_eq!(ok, vec!["C1", "C2"]); assert_eq!(err.len(), 0); let Outcome { completed: ok, errors: err, .. } =