Rollup merge of #60444 - nikomatsakis:issue-60010-cycle-error-investigation, r=pnkfelix

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

This is a targeted fix for #60010, which uncovered a pretty bad failure of our caching strategy in the face of coinductive cycles. The problem is explained in the comment in the PR on the new field, `in_cycle`, but I'll reproduce it here:

> 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.

I'm not sure what the impact of this fix will be in terms of existing crates or performance: we were accepting incorrect code before, so there will perhaps be some regressions, and we are now caching less.

As the comment above notes, we could do a lot better than this fix, but that would involve more invasive rewrites. I thought it best to start with something simple.

r? @pnkfelix -- but let's do crater/perf run
cc @arielb1

3 files changed, 144 insertions, 2 deletions

diff --git a/src/librustc/traits/select.rs b/src/librustc/traits/select.rs
index d68e2be9ea0..c4be85050db 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,36 @@ 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. 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. 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<bool>,
+
     previous: TraitObligationStackList<'prev, 'tcx>,
 }
 
@@ -840,8 +871,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 +987,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
@@ -3690,6 +3740,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 00000000000..45aa1b3c522
--- /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<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() {}
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 00000000000..9192f7ba2e3
--- /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: <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`.