From bd005a242a3125c432a76f982dab5a8d59e970f2 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 1 May 2019 13:10:01 -0400 Subject: [PATCH 1/2] forego caching for all participants in cycles, apart from root node --- src/librustc/traits/select.rs | 50 ++++++++++++- src/test/ui/traits/cycle-cache-err-60010.rs | 71 +++++++++++++++++++ .../ui/traits/cycle-cache-err-60010.stderr | 20 ++++++ 3 files changed, 139 insertions(+), 2 deletions(-) create mode 100644 src/test/ui/traits/cycle-cache-err-60010.rs create mode 100644 src/test/ui/traits/cycle-cache-err-60010.stderr diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index 8beabe058cf4f..3c4d48b17b9f3 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -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, + 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, } } diff --git a/src/test/ui/traits/cycle-cache-err-60010.rs b/src/test/ui/traits/cycle-cache-err-60010.rs new file mode 100644 index 0000000000000..45aa1b3c52239 --- /dev/null +++ b/src/test/ui/traits/cycle-cache-err-60010.rs @@ -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 { + type Data; +} +trait SourceDatabase { + fn parse(&self) { + loop {} + } +} + +struct ParseQuery; +struct RootDatabase { + _runtime: Runtime, +} +struct Runtime { + _storage: Box, +} +struct SalsaStorage { + _parse: >::Data, //~ ERROR overflow +} + +impl Database for RootDatabase { //~ ERROR overflow + type Storage = SalsaStorage; +} +impl HasQueryGroup for RootDatabase {} +impl Query for ParseQuery +where + DB: SourceDatabase, + DB: Database, +{ + type Data = RootDatabase; +} +impl 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: RefUnwindSafe` + // - `Runtime: RefUnwindSafe` + // - requires `DB::Storage: RefUnwindSafe` (`SalsaStorage: RefUnwindSafe`) + // - `SalsaStorage: RefUnwindSafe` + // - requires `>::Data: RefUnwindSafe`, + // which means `ParseQuery: Query` + // - `ParseQuery: Query` + // - 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() {} diff --git a/src/test/ui/traits/cycle-cache-err-60010.stderr b/src/test/ui/traits/cycle-cache-err-60010.stderr new file mode 100644 index 0000000000000..9192f7ba2e3b0 --- /dev/null +++ b/src/test/ui/traits/cycle-cache-err-60010.stderr @@ -0,0 +1,20 @@ +error[E0275]: overflow evaluating the requirement `RootDatabase: SourceDatabase` + --> $DIR/cycle-cache-err-60010.rs:27:5 + | +LL | _parse: >::Data, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: required because of the requirements on the impl of `Query` 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` 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`. From decd6d366018823a8b1116b346bc778eb010accd Mon Sep 17 00:00:00 2001 From: Felix S Klock II Date: Mon, 13 May 2019 13:29:49 +0200 Subject: [PATCH 2/2] modify comment modify the comment on `in_cycle` to reflect changes requested by ariel and myself. --- src/librustc/traits/select.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs index 3c4d48b17b9f3..b333f4b96514d 100644 --- a/src/librustc/traits/select.rs +++ b/src/librustc/traits/select.rs @@ -173,10 +173,15 @@ struct TraitObligationStack<'prev, 'tcx: 'prev> { /// well as the second instance of `A: AutoTrait`) to supress /// caching. /// - /// This is a simple, targeted fix. The correct fix requires + /// This is a simple, targeted fix. A more-performant 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. + /// tree, and then cache the entire tree at once. In any case, the + /// performance impact here shouldn't be so horrible: every time + /// this is hit, we do cache at least one trait, so we only + /// evaluate each member of a cycle up to N times, where N is the + /// length of the cycle. This means the performance impact is + /// bounded and we shouldn't have any terrible worst-cases. in_cycle: Cell, previous: TraitObligationStackList<'prev, 'tcx>,