Skip to content
Permalink
Browse files

forego caching for all participants in cycles, apart from root node

  • Loading branch information...
nikomatsakis committed May 1, 2019
1 parent 47e0803 commit bd005a242a3125c432a76f982dab5a8d59e970f2
@@ -43,6 +43,7 @@ use crate::hir;
use rustc_data_structures::bit_set::GrowableBitSet;
use rustc_data_structures::sync::Lock;
use rustc_target::spec::abi::Abi;
use std::cell::Cell;
use std::cmp;
use std::fmt::{self, Display};
use std::iter;
@@ -153,6 +154,31 @@ struct TraitObligationStack<'prev, 'tcx: 'prev> {
/// selection-context's freshener. Used to check for recursion.
fresh_trait_ref: ty::PolyTraitRef<'tcx>,

/// Starts out as false -- if, during evaluation, we encounter a
/// cycle, then we will set this flag to true for all participants
/// in the cycle (apart from the "head" node). These participants
/// will then forego caching their results. This is not the most
/// efficient solution, but it addresses #60010. The problem we
/// are trying to prevent:
///
/// - If you have `A: AutoTrait` requires `B: AutoTrait` and `C: NonAutoTrait`
/// - `B: AutoTrait` requires `A: AutoTrait` (coinductive cycle, ok)
/// - `C: NonAutoTrait` requires `A: AutoTrait` (non-coinductive cycle, not ok)
///
/// you don't want to cache that `B: AutoTrait` or `A: AutoTrait`
/// is `EvaluatedToOk`; this is because they were only considered
/// ok on the premise that if `A: AutoTrait` held, but we indeed
/// encountered a problem (later on) with `A: AutoTrait. So we
/// currently set a flag on the stack node for `B: AutoTrait` (as
/// well as the second instance of `A: AutoTrait`) to supress
/// caching.
///
/// This is a simple, targeted fix. The correct fix requires
/// deeper changes, but would permit more caching: we could
/// basically defer caching until we have fully evaluated the
/// tree, and then cache the entire tree at once.
in_cycle: Cell<bool>,

previous: TraitObligationStackList<'prev, 'tcx>,
}

@@ -840,8 +866,16 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
let (result, dep_node) = self.in_task(|this| this.evaluate_stack(&stack));
let result = result?;

debug!("CACHE MISS: EVAL({:?})={:?}", fresh_trait_ref, result);
self.insert_evaluation_cache(obligation.param_env, fresh_trait_ref, dep_node, result);
if !stack.in_cycle.get() {
debug!("CACHE MISS: EVAL({:?})={:?}", fresh_trait_ref, result);
self.insert_evaluation_cache(obligation.param_env, fresh_trait_ref, dep_node, result);
} else {
debug!(
"evaluate_trait_predicate_recursively: skipping cache because {:?} \
is a cycle participant",
fresh_trait_ref,
);
}

Ok(result)
}
@@ -948,6 +982,17 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
{
debug!("evaluate_stack({:?}) --> recursive", stack.fresh_trait_ref);

// If we have a stack like `A B C D E A`, where the top of
// the stack is the final `A`, then this will iterate over
// `A, E, D, C, B` -- i.e., all the participants apart
// from the cycle head. We mark them as participating in a
// cycle. This suppresses caching for those nodes. See
// `in_cycle` field for more details.
for item in stack.iter().take(rec_index + 1) {
debug!("evaluate_stack: marking {:?} as cycle participant", item.fresh_trait_ref);
item.in_cycle.set(true);
}

// Subtle: when checking for a coinductive cycle, we do
// not compare using the "freshened trait refs" (which
// have erased regions) but rather the fully explicit
@@ -3692,6 +3737,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
TraitObligationStack {
obligation,
fresh_trait_ref,
in_cycle: Cell::new(false),
previous: previous_stack,
}
}
@@ -0,0 +1,71 @@
// Test that we properly detect the cycle amongst the traits
// here and report an error.

use std::panic::RefUnwindSafe;

trait Database {
type Storage;
}
trait HasQueryGroup {}
trait Query<DB> {
type Data;
}
trait SourceDatabase {
fn parse(&self) {
loop {}
}
}

struct ParseQuery;
struct RootDatabase {
_runtime: Runtime<RootDatabase>,
}
struct Runtime<DB: Database> {
_storage: Box<DB::Storage>,
}
struct SalsaStorage {
_parse: <ParseQuery as Query<RootDatabase>>::Data, //~ ERROR overflow
}

impl Database for RootDatabase { //~ ERROR overflow
type Storage = SalsaStorage;
}
impl HasQueryGroup for RootDatabase {}
impl<DB> Query<DB> for ParseQuery
where
DB: SourceDatabase,
DB: Database,
{
type Data = RootDatabase;
}
impl<T> SourceDatabase for T
where
T: RefUnwindSafe,
T: HasQueryGroup,
{
}

pub(crate) fn goto_implementation(db: &RootDatabase) -> u32 {
// This is not satisfied:
//
// - `RootDatabase: SourceDatabase`
// - requires `RootDatabase: RefUnwindSafe` + `RootDatabase: HasQueryGroup`
// - `RootDatabase: RefUnwindSafe`
// - requires `Runtime<RootDatabase>: RefUnwindSafe`
// - `Runtime<RootDatabase>: RefUnwindSafe`
// - requires `DB::Storage: RefUnwindSafe` (`SalsaStorage: RefUnwindSafe`)
// - `SalsaStorage: RefUnwindSafe`
// - requires `<ParseQuery as Query<RootDatabase>>::Data: RefUnwindSafe`,
// which means `ParseQuery: Query<RootDatabase>`
// - `ParseQuery: Query<RootDatabase>`
// - requires `RootDatabase: SourceDatabase`,
// - `RootDatabase: SourceDatabase` is already on the stack, so we have a
// cycle with non-coinductive participants
//
// we used to fail to report an error here because we got the
// caching wrong.
SourceDatabase::parse(db);
22
}

fn main() {}
@@ -0,0 +1,20 @@
error[E0275]: overflow evaluating the requirement `RootDatabase: SourceDatabase`
--> $DIR/cycle-cache-err-60010.rs:27:5
|
LL | _parse: <ParseQuery as Query<RootDatabase>>::Data,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: required because of the requirements on the impl of `Query<RootDatabase>` for `ParseQuery`

error[E0275]: overflow evaluating the requirement `RootDatabase: SourceDatabase`
--> $DIR/cycle-cache-err-60010.rs:30:6
|
LL | impl Database for RootDatabase {
| ^^^^^^^^
|
= note: required because of the requirements on the impl of `Query<RootDatabase>` for `ParseQuery`
= note: required because it appears within the type `SalsaStorage`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0275`.

0 comments on commit bd005a2

Please sign in to comment.
You can’t perform that action at this time.