Skip to content

Commit

Permalink
Auto merge of rust-lang#97674 - nnethercote:oblig-forest-tweaks, r=ni…
Browse files Browse the repository at this point in the history 
…komatsakis

Obligation forest tweaks

A few minor improvements to the code.

r? `@nikomatsakis`
  • Loading branch information
@bors
bors committed Jun 20, 2022
2 parents 4104596 + 32741d5 commit 1d60108
Show file tree
Hide file tree
Showing 3 changed files with 115 additions and 153 deletions.
155 changes: 72 additions & 83 deletions compiler/rustc_data_structures/src/obligation_forest/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,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
Expand All @@ -53,13 +53,10 @@
//! 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.
//!
//! Upon completion, 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.
//!
//! ### Implementation details
//!
Expand Down Expand Up @@ -99,6 +96,8 @@ pub trait ObligationProcessor {
type Obligation: ForestObligation;
type Error: Debug;

fn needs_process_obligation(&self, obligation: &Self::Obligation) -> bool;

fn process_obligation(
&mut self,
obligation: &mut Self::Obligation,
Expand Down Expand Up @@ -146,7 +145,7 @@ pub struct ObligationForest<O: ForestObligation> {

/// A cache of the nodes in `nodes`, indexed by predicate. Unfortunately,
/// its contents are not guaranteed to match those of `nodes`. See the
/// comments in [`Self::process_obligation` for details.
/// comments in `Self::process_obligation` for details.
active_cache: FxHashMap<O::CacheKey, usize>,

/// A vector reused in [Self::compress()] and [Self::find_cycles_from_node()],
Expand Down Expand Up @@ -260,8 +259,6 @@ pub trait OutcomeTrait {
type Obligation;

fn new() -> Self;
fn mark_not_stalled(&mut self);
fn is_stalled(&self) -> bool;
fn record_completed(&mut self, outcome: &Self::Obligation);
fn record_error(&mut self, error: Self::Error);
}
Expand All @@ -270,30 +267,14 @@ pub trait OutcomeTrait {
pub struct Outcome<O, E> {
/// Backtrace of obligations that were found to be in error.
pub errors: Vec<Error<O, E>>,

/// If true, then we saw no successful obligations, which means
/// there is no point in further iteration. This is based on the
/// assumption that when trait matching returns `Error` or
/// `Unchanged`, those results do not affect environmental
/// inference state. (Note that if we invoke `process_obligations`
/// with no pending obligations, stalled will be true.)
pub stalled: bool,
}

impl<O, E> OutcomeTrait for Outcome<O, E> {
type Error = Error<O, E>;
type Obligation = O;

fn new() -> Self {
Self { stalled: true, errors: vec![] }
}

fn mark_not_stalled(&mut self) {
self.stalled = false;
}

fn is_stalled(&self) -> bool {
self.stalled
Self { errors: vec![] }
}

fn record_completed(&mut self, _outcome: &Self::Obligation) {
Expand Down Expand Up @@ -415,10 +396,7 @@ impl<O: ForestObligation> ObligationForest<O> {
.insert(node.obligation.as_cache_key());
}

/// Performs a pass through the obligation list. This must
/// be called in a loop until `outcome.stalled` is false.
///
/// This _cannot_ be unrolled (presently, at least).
/// Performs a fixpoint computation over the obligation list.
#[inline(never)]
pub fn process_obligations<P, OUT>(&mut self, processor: &mut P) -> OUT
where
Expand All @@ -427,55 +405,69 @@ impl<O: ForestObligation> ObligationForest<O> {
{
let mut outcome = OUT::new();

// Note that the loop body can append new nodes, and those new nodes
// will then be processed by subsequent iterations of the loop.
//
// We can't use an iterator for the loop because `self.nodes` is
// appended to and the borrow checker would complain. We also can't use
// `for index in 0..self.nodes.len() { ... }` because the range would
// be computed with the initial length, and we would miss the appended
// nodes. Therefore we use a `while` loop.
let mut index = 0;
while let Some(node) = self.nodes.get_mut(index) {
// `processor.process_obligation` can modify the predicate within
// `node.obligation`, and that predicate is the key used for
// `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.
if node.state.get() != NodeState::Pending {
index += 1;
continue;
}

match processor.process_obligation(&mut node.obligation) {
ProcessResult::Unchanged => {
// No change in state.
// Fixpoint computation: we repeat until the inner loop stalls.
loop {
let mut has_changed = false;

// Note that the loop body can append new nodes, and those new nodes
// will then be processed by subsequent iterations of the loop.
//
// We can't use an iterator for the loop because `self.nodes` is
// appended to and the borrow checker would complain. We also can't use
// `for index in 0..self.nodes.len() { ... }` because the range would
// be computed with the initial length, and we would miss the appended
// nodes. Therefore we use a `while` loop.
let mut index = 0;
while let Some(node) = self.nodes.get_mut(index) {
if node.state.get() != NodeState::Pending
|| !processor.needs_process_obligation(&node.obligation)
{
index += 1;
continue;
}
ProcessResult::Changed(children) => {
// We are not (yet) stalled.
outcome.mark_not_stalled();
node.state.set(NodeState::Success);

for child in children {
let st = self.register_obligation_at(child, Some(index));
if let Err(()) = st {
// Error already reported - propagate it
// to our node.
self.error_at(index);

// `processor.process_obligation` can modify the predicate within
// `node.obligation`, and that predicate is the key used for
// `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.

match processor.process_obligation(&mut node.obligation) {
ProcessResult::Unchanged => {
// No change in state.
}
ProcessResult::Changed(children) => {
// We are not (yet) stalled.
has_changed = true;
node.state.set(NodeState::Success);

for child in children {
let st = self.register_obligation_at(child, Some(index));
if let Err(()) = st {
// Error already reported - propagate it
// to our node.
self.error_at(index);
}
}
}
ProcessResult::Error(err) => {
has_changed = true;
outcome.record_error(Error { error: err, backtrace: self.error_at(index) });
}
}
ProcessResult::Error(err) => {
outcome.mark_not_stalled();
outcome.record_error(Error { error: err, backtrace: self.error_at(index) });
}
index += 1;
}

// If unchanged, then we saw no successful obligations, which means
// there is no point in further iteration. This is based on the
// assumption that when trait matching returns `Error` or
// `Unchanged`, those results do not affect environmental inference
// state. (Note that this will occur if we invoke
// `process_obligations` with no pending obligations.)
if !has_changed {
break;
}
index += 1;
}

// There's no need to perform marking, cycle processing and compression when nothing
// changed.
if !outcome.is_stalled() {
self.mark_successes();
self.process_cycles(processor);
self.compress(|obl| outcome.record_completed(obl));
Expand Down Expand Up @@ -634,17 +626,14 @@ impl<O: ForestObligation> ObligationForest<O> {
}
}
NodeState::Done => {
// This lookup can fail because the contents of
// The removal lookup might fail because the contents 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, _)) =
self.active_cache.remove_entry(&node.obligation.as_cache_key())
{
self.done_cache.insert(predicate);
} else {
self.done_cache.insert(node.obligation.as_cache_key().clone());
}
let cache_key = node.obligation.as_cache_key();
self.active_cache.remove(&cache_key);
self.done_cache.insert(cache_key);

// Extract the success stories.
outcome_cb(&node.obligation);
node_rewrites[index] = orig_nodes_len;
Expand Down
15 changes: 5 additions & 10 deletions compiler/rustc_data_structures/src/obligation_forest/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ struct ClosureObligationProcessor<OF, BF, O, E> {
struct TestOutcome<O, E> {
pub completed: Vec<O>,
pub errors: Vec<Error<O, E>>,
pub stalled: bool,
}

impl<O, E> OutcomeTrait for TestOutcome<O, E>
Expand All @@ -31,15 +30,7 @@ where
type Obligation = O;

fn new() -> Self {
Self { errors: vec![], stalled: false, completed: vec![] }
}

fn mark_not_stalled(&mut self) {
self.stalled = false;
}

fn is_stalled(&self) -> bool {
self.stalled
Self { errors: vec![], completed: vec![] }
}

fn record_completed(&mut self, outcome: &Self::Obligation) {
Expand Down Expand Up @@ -74,6 +65,10 @@ where
type Obligation = O;
type Error = E;

fn needs_process_obligation(&self, _obligation: &Self::Obligation) -> bool {
true
}

fn process_obligation(
&mut self,
obligation: &mut Self::Obligation,
Expand Down
Loading

0 comments on commit 1d60108

Please sign in to comment.