From d558353f2860298fabb898923dc41c99190a5ab4 Mon Sep 17 00:00:00 2001 From: lcnr Date: Thu, 10 Aug 2023 12:35:34 +0200 Subject: [PATCH] make the provisional cache slightly less broken --- .../src/solve/search_graph/cache.rs | 32 +++++---- .../src/solve/search_graph/mod.rs | 65 +++++++++++-------- .../cycles/inductive-not-on-stack.rs | 2 +- .../cycles/inductive-not-on-stack.stderr | 15 +---- 4 files changed, 59 insertions(+), 55 deletions(-) diff --git a/compiler/rustc_trait_selection/src/solve/search_graph/cache.rs b/compiler/rustc_trait_selection/src/solve/search_graph/cache.rs index 56f126e9157..be48447e27c 100644 --- a/compiler/rustc_trait_selection/src/solve/search_graph/cache.rs +++ b/compiler/rustc_trait_selection/src/solve/search_graph/cache.rs @@ -19,21 +19,25 @@ rustc_index::newtype_index! { #[derive(Debug, Clone)] pub(super) struct ProvisionalEntry<'tcx> { - // In case we have a coinductive cycle, this is the - // the currently least restrictive result of this goal. - pub(super) response: QueryResult<'tcx>, - // In case of a cycle, the position of deepest stack entry involved - // in that cycle. This is monotonically decreasing in the stack as all - // elements between the current stack element in the deepest stack entry - // involved have to also be involved in that cycle. - // - // We can only move entries to the global cache once we're complete done - // with the cycle. If this entry has not been involved in a cycle, - // this is just its own depth. + /// In case we have a coinductive cycle, this is the + /// the current provisional result of this goal. + /// + /// This starts out as `None` for all goals and gets to some + /// when the goal gets popped from the stack or we rerun evaluation + /// for this goal to reach a fixpoint. + pub(super) response: Option>, + /// In case of a cycle, the position of deepest stack entry involved + /// in that cycle. This is monotonically decreasing in the stack as all + /// elements between the current stack element in the deepest stack entry + /// involved have to also be involved in that cycle. + /// + /// We can only move entries to the global cache once we're complete done + /// with the cycle. If this entry has not been involved in a cycle, + /// this is just its own depth. pub(super) depth: StackDepth, - // The goal for this entry. Should always be equal to the corresponding goal - // in the lookup table. + /// The goal for this entry. Should always be equal to the corresponding goal + /// in the lookup table. pub(super) input: CanonicalInput<'tcx>, } @@ -92,7 +96,7 @@ impl<'tcx> ProvisionalCache<'tcx> { self.entries[entry_index].depth } - pub(super) fn provisional_result(&self, entry_index: EntryIndex) -> QueryResult<'tcx> { + pub(super) fn provisional_result(&self, entry_index: EntryIndex) -> Option> { self.entries[entry_index].response } } diff --git a/compiler/rustc_trait_selection/src/solve/search_graph/mod.rs b/compiler/rustc_trait_selection/src/solve/search_graph/mod.rs index 21c8d476902..b860f374c0a 100644 --- a/compiler/rustc_trait_selection/src/solve/search_graph/mod.rs +++ b/compiler/rustc_trait_selection/src/solve/search_graph/mod.rs @@ -13,7 +13,7 @@ use rustc_middle::traits::solve::CacheData; use rustc_middle::traits::solve::{CanonicalInput, Certainty, EvaluationCache, QueryResult}; use rustc_middle::ty::TyCtxt; use rustc_session::Limit; -use std::{collections::hash_map::Entry, mem}; +use std::collections::hash_map::Entry; rustc_index::newtype_index! { pub struct StackDepth {} @@ -216,8 +216,8 @@ impl<'tcx> SearchGraph<'tcx> { cycle_participants: Default::default(), }; assert_eq!(self.stack.push(entry), depth); - let response = Self::response_no_constraints(tcx, input, Certainty::Yes); - let entry_index = cache.entries.push(ProvisionalEntry { response, depth, input }); + let entry_index = + cache.entries.push(ProvisionalEntry { response: None, depth, input }); v.insert(entry_index); } // We have a nested goal which relies on a goal `root` deeper in the stack. @@ -243,23 +243,31 @@ impl<'tcx> SearchGraph<'tcx> { root.cycle_participants.insert(e.input); } - // NOTE: The goals on the stack aren't the only goals involved in this cycle. - // We can also depend on goals which aren't part of the stack but coinductively - // depend on the stack themselves. We already checked whether all the goals - // between these goals and their root on the stack. This means that as long as - // each goal in a cycle is checked for coinductivity by itself, simply checking - // the stack is enough. - if self.stack.raw[stack_depth.index()..] - .iter() - .all(|g| g.input.value.goal.predicate.is_coinductive(tcx)) - { - // If we're in a coinductive cycle, we have to retry proving the current goal - // until we reach a fixpoint. - self.stack[stack_depth].has_been_used = true; - return cache.provisional_result(entry_index); + // If we're in a cycle, we have to retry proving the current goal + // until we reach a fixpoint. + self.stack[stack_depth].has_been_used = true; + return if let Some(result) = cache.provisional_result(entry_index) { + result } else { - return Self::response_no_constraints(tcx, input, Certainty::OVERFLOW); - } + // If we don't have a provisional result yet, the goal has to + // still be on the stack. + let mut goal_on_stack = false; + let mut is_coinductive = true; + for entry in self.stack.raw[stack_depth.index()..] + .iter() + .skip_while(|entry| entry.input != input) + { + goal_on_stack = true; + is_coinductive &= entry.input.value.goal.predicate.is_coinductive(tcx); + } + debug_assert!(goal_on_stack); + + if is_coinductive { + Self::response_no_constraints(tcx, input, Certainty::Yes) + } else { + Self::response_no_constraints(tcx, input, Certainty::OVERFLOW) + } + }; } } @@ -288,15 +296,18 @@ impl<'tcx> SearchGraph<'tcx> { let provisional_entry_index = *cache.lookup_table.get(&stack_entry.input).unwrap(); let provisional_entry = &mut cache.entries[provisional_entry_index]; - let prev_response = mem::replace(&mut provisional_entry.response, response); - if stack_entry.has_been_used && prev_response != response { - // If so, remove all entries whose result depends on this goal - // from the provisional cache... + if stack_entry.has_been_used + && provisional_entry.response.map_or(true, |r| r != response) + { + // If so, update the provisional result for this goal and remove + // all entries whose result depends on this goal from the provisional + // cache... // - // That's not completely correct, as a nested goal can also + // That's not completely correct, as a nested goal can also only // depend on a goal which is lower in the stack so it doesn't // actually depend on the current goal. This should be fairly // rare and is hopefully not relevant for performance. + provisional_entry.response = Some(response); #[allow(rustc::potential_query_instability)] cache.lookup_table.retain(|_key, index| *index <= provisional_entry_index); cache.entries.truncate(provisional_entry_index.index() + 1); @@ -315,8 +326,8 @@ impl<'tcx> SearchGraph<'tcx> { }); // We're now done with this goal. In case this goal is involved in a larger cycle - // do not remove it from the provisional cache and do not add it to the global - // cache. + // do not remove it from the provisional cache and update its provisional result. + // We only add the root of cycles to the global cache. // // It is not possible for any nested goal to depend on something deeper on the // stack, as this would have also updated the depth of the current goal. @@ -348,6 +359,8 @@ impl<'tcx> SearchGraph<'tcx> { dep_node, result, ) + } else { + provisional_entry.response = Some(result); } result diff --git a/tests/ui/traits/new-solver/cycles/inductive-not-on-stack.rs b/tests/ui/traits/new-solver/cycles/inductive-not-on-stack.rs index f06b98a79cf..3cfe7ab87f6 100644 --- a/tests/ui/traits/new-solver/cycles/inductive-not-on-stack.rs +++ b/tests/ui/traits/new-solver/cycles/inductive-not-on-stack.rs @@ -39,7 +39,7 @@ fn impls_ar() {} fn main() { impls_a::<()>(); - //~^ ERROR overflow evaluating the requirement `(): A` + // FIXME(-Ztrait-solver=next): This is broken and should error. impls_ar::<()>(); //~^ ERROR overflow evaluating the requirement `(): AR` diff --git a/tests/ui/traits/new-solver/cycles/inductive-not-on-stack.stderr b/tests/ui/traits/new-solver/cycles/inductive-not-on-stack.stderr index 33fac603cbd..0e1c86c1bb3 100644 --- a/tests/ui/traits/new-solver/cycles/inductive-not-on-stack.stderr +++ b/tests/ui/traits/new-solver/cycles/inductive-not-on-stack.stderr @@ -1,16 +1,3 @@ -error[E0275]: overflow evaluating the requirement `(): A` - --> $DIR/inductive-not-on-stack.rs:41:5 - | -LL | impls_a::<()>(); - | ^^^^^^^^^^^^^ - | - = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`inductive_not_on_stack`) -note: required by a bound in `impls_a` - --> $DIR/inductive-not-on-stack.rs:25:15 - | -LL | fn impls_a() {} - | ^ required by this bound in `impls_a` - error[E0275]: overflow evaluating the requirement `(): AR` --> $DIR/inductive-not-on-stack.rs:44:5 | @@ -24,6 +11,6 @@ note: required by a bound in `impls_ar` LL | fn impls_ar() {} | ^^ required by this bound in `impls_ar` -error: aborting due to 2 previous errors +error: aborting due to previous error For more information about this error, try `rustc --explain E0275`.