diff --git a/compiler/rustc_trait_selection/src/lib.rs b/compiler/rustc_trait_selection/src/lib.rs index 4097e1577e1..bf3c8643f0d 100644 --- a/compiler/rustc_trait_selection/src/lib.rs +++ b/compiler/rustc_trait_selection/src/lib.rs @@ -14,6 +14,7 @@ #![feature(bool_to_option)] #![feature(box_patterns)] #![feature(drain_filter)] +#![feature(hash_drain_filter)] #![feature(in_band_lifetimes)] #![feature(iter_zip)] #![feature(never_type)] diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index 08d452900c8..a292de148a6 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -636,8 +636,8 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { if let Some(result) = stack.cache().get_provisional(fresh_trait_ref) { debug!(?result, "PROVISIONAL CACHE HIT"); - stack.update_reached_depth(stack.cache().current_reached_depth()); - return Ok(result); + stack.update_reached_depth(result.reached_depth); + return Ok(result.result); } // Check if this is a match for something already on the @@ -661,7 +661,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { debug!(?result, "CACHE MISS"); self.insert_evaluation_cache(obligation.param_env, fresh_trait_ref, dep_node, result); - stack.cache().on_completion(stack.depth, |fresh_trait_ref, provisional_result| { + stack.cache().on_completion(stack.dfn, |fresh_trait_ref, provisional_result| { self.insert_evaluation_cache( obligation.param_env, fresh_trait_ref, @@ -2162,7 +2162,7 @@ impl<'o, 'tcx> TraitObligationStack<'o, 'tcx> { /// required accessing something from the stack at depth `reached_depth`. fn update_reached_depth(&self, reached_depth: usize) { assert!( - self.depth > reached_depth, + self.depth >= reached_depth, "invoked `update_reached_depth` with something under this stack: \ self.depth={} reached_depth={}", self.depth, @@ -2235,23 +2235,6 @@ struct ProvisionalEvaluationCache<'tcx> { /// next "depth first number" to issue -- just a counter dfn: Cell, - /// Stores the "coldest" depth (bottom of stack) reached by any of - /// the evaluation entries. The idea here is that all things in the provisional - /// cache are always dependent on *something* that is colder in the stack: - /// therefore, if we add a new entry that is dependent on something *colder still*, - /// we have to modify the depth for all entries at once. - /// - /// Example: - /// - /// Imagine we have a stack `A B C D E` (with `E` being the top of - /// the stack). We cache something with depth 2, which means that - /// it was dependent on C. Then we pop E but go on and process a - /// new node F: A B C D F. Now F adds something to the cache with - /// depth 1, meaning it is dependent on B. Our original cache - /// entry is also dependent on B, because there is a path from E - /// to C and then from C to F and from F to B. - reached_depth: Cell, - /// Map from cache key to the provisionally evaluated thing. /// The cache entries contain the result but also the DFN in which they /// were added. The DFN is used to clear out values on failure. @@ -2275,12 +2258,13 @@ struct ProvisionalEvaluationCache<'tcx> { #[derive(Copy, Clone, Debug)] struct ProvisionalEvaluation { from_dfn: usize, + reached_depth: usize, result: EvaluationResult, } impl<'tcx> Default for ProvisionalEvaluationCache<'tcx> { fn default() -> Self { - Self { dfn: Cell::new(0), reached_depth: Cell::new(usize::MAX), map: Default::default() } + Self { dfn: Cell::new(0), map: Default::default() } } } @@ -2295,22 +2279,17 @@ impl<'tcx> ProvisionalEvaluationCache<'tcx> { /// Check the provisional cache for any result for /// `fresh_trait_ref`. If there is a hit, then you must consider /// it an access to the stack slots at depth - /// `self.current_reached_depth()` and above. - fn get_provisional(&self, fresh_trait_ref: ty::PolyTraitRef<'tcx>) -> Option { + /// `reached_depth` (from the returned value). + fn get_provisional( + &self, + fresh_trait_ref: ty::PolyTraitRef<'tcx>, + ) -> Option { debug!( ?fresh_trait_ref, - reached_depth = ?self.reached_depth.get(), "get_provisional = {:#?}", self.map.borrow().get(&fresh_trait_ref), ); - Some(self.map.borrow().get(&fresh_trait_ref)?.result) - } - - /// Current value of the `reached_depth` counter -- all the - /// provisional cache entries are dependent on the item at this - /// depth. - fn current_reached_depth(&self) -> usize { - self.reached_depth.get() + Some(self.map.borrow().get(&fresh_trait_ref)?.clone()) } /// Insert a provisional result into the cache. The result came @@ -2324,13 +2303,31 @@ impl<'tcx> ProvisionalEvaluationCache<'tcx> { fresh_trait_ref: ty::PolyTraitRef<'tcx>, result: EvaluationResult, ) { - debug!(?from_dfn, ?reached_depth, ?fresh_trait_ref, ?result, "insert_provisional"); - let r_d = self.reached_depth.get(); - self.reached_depth.set(r_d.min(reached_depth)); + debug!(?from_dfn, ?fresh_trait_ref, ?result, "insert_provisional"); - debug!(reached_depth = self.reached_depth.get()); + let mut map = self.map.borrow_mut(); - self.map.borrow_mut().insert(fresh_trait_ref, ProvisionalEvaluation { from_dfn, result }); + // Subtle: when we complete working on the DFN `from_dfn`, anything + // that remains in the provisional cache must be dependent on some older + // stack entry than `from_dfn`. We have to update their depth with our transitive + // depth in that case or else it would be referring to some popped note. + // + // Example: + // A (reached depth 0) + // ... + // B // depth 1 -- reached depth = 0 + // C // depth 2 -- reached depth = 1 (should be 0) + // B + // A // depth 0 + // D (reached depth 1) + // C (cache -- reached depth = 2) + for (_k, v) in &mut *map { + if v.from_dfn >= from_dfn { + v.reached_depth = reached_depth.min(v.reached_depth); + } + } + + map.insert(fresh_trait_ref, ProvisionalEvaluation { from_dfn, reached_depth, result }); } /// Invoked when the node with dfn `dfn` does not get a successful @@ -2358,25 +2355,40 @@ impl<'tcx> ProvisionalEvaluationCache<'tcx> { /// was a failure, then `on_failure` should have been invoked /// already). The callback `op` will be invoked for each /// provisional entry that we can now confirm. + /// + /// Note that we may still have provisional cache items remaining + /// in the cache when this is done. For example, if there is a + /// cycle: + /// + /// * A depends on... + /// * B depends on A + /// * C depends on... + /// * D depends on C + /// * ... + /// + /// Then as we complete the C node we will have a provisional cache + /// with results for A, B, C, and D. This method would clear out + /// the C and D results, but leave A and B provisional. + /// + /// This is determined based on the DFN: we remove any provisional + /// results created since `dfn` started (e.g., in our example, dfn + /// would be 2, representing the C node, and hence we would + /// remove the result for D, which has DFN 3, but not the results for + /// A and B, which have DFNs 0 and 1 respectively). fn on_completion( &self, - depth: usize, + dfn: usize, mut op: impl FnMut(ty::PolyTraitRef<'tcx>, EvaluationResult), ) { - debug!(?depth, reached_depth = ?self.reached_depth.get(), "on_completion"); + debug!(?dfn, "on_completion"); - if self.reached_depth.get() < depth { - debug!("on_completion: did not yet reach depth to complete"); - return; - } - - for (fresh_trait_ref, eval) in self.map.borrow_mut().drain() { + for (fresh_trait_ref, eval) in + self.map.borrow_mut().drain_filter(|_k, eval| eval.from_dfn >= dfn) + { debug!(?fresh_trait_ref, ?eval, "on_completion"); op(fresh_trait_ref, eval.result); } - - self.reached_depth.set(usize::MAX); } } diff --git a/src/test/ui/cycle-me.rs b/src/test/ui/cycle-me.rs deleted file mode 100644 index eb1ffe51ffc..00000000000 --- a/src/test/ui/cycle-me.rs +++ /dev/null @@ -1,31 +0,0 @@ -#![feature(rustc_attrs)] - -// A (reached depth 0) -// ... -// B // depth 1 -- reached depth = 0 -// C // depth 2 -- reached depth = 1 (should be 0) -// B -// A // depth 0 -// D (reached depth 1) -// C (cache -- reached depth = 2) - -struct A { - b: B, - c: C, -} - -struct B { - c: C, - a: Option>, -} - -struct C { - b: Option>, -} - -#[rustc_evaluate_where_clauses] -fn test() {} - -fn main() { - test::(); -} diff --git a/src/test/ui/traits/cache-reached-depth-ice.rs b/src/test/ui/traits/cache-reached-depth-ice.rs new file mode 100644 index 00000000000..4318e07d07a --- /dev/null +++ b/src/test/ui/traits/cache-reached-depth-ice.rs @@ -0,0 +1,45 @@ +#![feature(rustc_attrs)] + +// Test for a particular corner case where the evaluation +// cache can get out of date. The problem here is that +// when we cache C, we have observed that it reaches +// to depth 2 (the node for B), but we later realize +// that B itself depends on A (reached depth 0). We +// failed to update the depth for C transitively, which +// resulted in an assertion failure when it was referenced +// from D. +// +// A (reached depth 0) +// E +// B // depth 2 -- reached depth = 0 +// C // depth 3 -- reached depth = 2 (should be 0) +// B +// A // depth 0 +// D (depth 1) +// C (cache -- reached depth = 2) + +struct A { + e: E, + d: C, +} + +struct E { + b: B, +} + +struct B { + a: Option>, + c: C, +} + +struct C { + b: Option>, +} + +#[rustc_evaluate_where_clauses] +fn test() {} + +fn main() { + test::(); + //~^ ERROR evaluate(Binder(TraitPredicate(), [])) = Ok(EvaluatedToOk) +} diff --git a/src/test/ui/traits/cache-reached-depth-ice.stderr b/src/test/ui/traits/cache-reached-depth-ice.stderr new file mode 100644 index 00000000000..5e662970bb9 --- /dev/null +++ b/src/test/ui/traits/cache-reached-depth-ice.stderr @@ -0,0 +1,11 @@ +error: evaluate(Binder(TraitPredicate(), [])) = Ok(EvaluatedToOk) + --> $DIR/cache-reached-depth-ice.rs:43:5 + | +LL | fn test() {} + | ---- predicate +... +LL | test::(); + | ^^^^^^^^^ + +error: aborting due to previous error + diff --git a/src/test/ui/traits/issue-83538-tainted-cache-after-cycle.rs b/src/test/ui/traits/issue-83538-tainted-cache-after-cycle.rs index 1b357575cb8..e186570167d 100644 --- a/src/test/ui/traits/issue-83538-tainted-cache-after-cycle.rs +++ b/src/test/ui/traits/issue-83538-tainted-cache-after-cycle.rs @@ -54,13 +54,13 @@ where } fn main() { - // The only ERROR included here is the one that is totally wrong: + // Key is that Vec is "ok" and Third is "ok modulo regions": forward(); - //~^ ERROR evaluate(Binder(TraitPredicate( as std::marker::Unpin>), [])) = Ok(EvaluatedToOkModuloRegions) + //~^ ERROR evaluate(Binder(TraitPredicate( as std::marker::Unpin>), [])) = Ok(EvaluatedToOk) //~| ERROR evaluate(Binder(TraitPredicate( as std::marker::Unpin>), [])) = Ok(EvaluatedToOkModuloRegions) reverse(); - //~^ ERROR evaluate(Binder(TraitPredicate( as std::marker::Unpin>), [])) = Ok(EvaluatedToOkModuloRegions) + //~^ ERROR evaluate(Binder(TraitPredicate( as std::marker::Unpin>), [])) = Ok(EvaluatedToOk) //~| ERROR evaluate(Binder(TraitPredicate( as std::marker::Unpin>), [])) = Ok(EvaluatedToOkModuloRegions) } diff --git a/src/test/ui/traits/issue-83538-tainted-cache-after-cycle.stderr b/src/test/ui/traits/issue-83538-tainted-cache-after-cycle.stderr index 82e00c3b97f..bfe3e76b214 100644 --- a/src/test/ui/traits/issue-83538-tainted-cache-after-cycle.stderr +++ b/src/test/ui/traits/issue-83538-tainted-cache-after-cycle.stderr @@ -1,4 +1,4 @@ -error: evaluate(Binder(TraitPredicate( as std::marker::Unpin>), [])) = Ok(EvaluatedToOkModuloRegions) +error: evaluate(Binder(TraitPredicate( as std::marker::Unpin>), [])) = Ok(EvaluatedToOk) --> $DIR/issue-83538-tainted-cache-after-cycle.rs:59:5 | LL | Vec: Unpin, @@ -25,7 +25,7 @@ LL | Third: Unpin, LL | reverse(); | ^^^^^^^ -error: evaluate(Binder(TraitPredicate( as std::marker::Unpin>), [])) = Ok(EvaluatedToOkModuloRegions) +error: evaluate(Binder(TraitPredicate( as std::marker::Unpin>), [])) = Ok(EvaluatedToOk) --> $DIR/issue-83538-tainted-cache-after-cycle.rs:63:5 | LL | Vec: Unpin,