From 02f1a565fee32ac9710b19a73539646d6454b085 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 18 Jan 2022 23:03:55 -0500 Subject: [PATCH] Properly track `DepNode`s in trait evaluation provisional cache Fixes #92987 During evaluation of an auto trait predicate, we may encounter a cycle. This causes us to store the evaluation result in a special 'provisional cache;. If we later end up determining that the type can legitimately implement the auto trait despite the cycle, we remove the entry from the provisional cache, and insert it into the evaluation cache. Additionally, trait evaluation creates a special anonymous `DepNode`. All queries invoked during the predicate evaluation are added as outoging dependency edges from the `DepNode`. This `DepNode` is then store in the evaluation cache - if a different query ends up reading from the cache entry, it will also perform a read of the stored `DepNode`. As a result, the cached evaluation will still end up (transitively) incurring all of the same dependencies that it would if it actually performed the uncached evaluation (e.g. a call to `type_of` to determine constituent types). Previously, we did not correctly handle the interaction between the provisional cache and the created `DepNode`. Storing an evaluation result in the provisional cache would cause us to lose the `DepNode` created during the evaluation. If we later moved the entry from the provisional cache to the evaluation cache, we would use the `DepNode` associated with the evaluation that caused us to 'complete' the cycle, not the evaluatoon where we first discovered the cycle. As a result, future reads from the evaluation cache would miss some incremental compilation dependencies that would have otherwise been added if the evaluation was *not* cached. Under the right circumstances, this could lead to us trying to force a query with a no-longer-existing `DefPathHash`, since we were missing the (red) dependency edge that would have caused us to bail out before attempting forcing. This commit makes the provisional cache store the `DepNode` create during the provisional evaluation. When we move an entry from the provisional cache to the evaluation cache, we create a *new* `DepNode` that has dependencies going to *both* of the evaluation `DepNodes` we have available. This ensures that cached reads will incur all of the necessary dependency edges. --- .../src/traits/select/mod.rs | 63 +++++++++++++++---- .../issue-92987-provisional-dep-node.rs | 24 +++++++ 2 files changed, 75 insertions(+), 12 deletions(-) create mode 100644 src/test/incremental/issue-92987-provisional-dep-node.rs diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index 1414c742635c4..947fcdea7ab25 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -765,14 +765,38 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { debug!(?result, "CACHE MISS"); self.insert_evaluation_cache(param_env, fresh_trait_pred, dep_node, result); - stack.cache().on_completion(stack.dfn, |fresh_trait_pred, provisional_result| { - self.insert_evaluation_cache( - param_env, - fresh_trait_pred, - dep_node, - provisional_result.max(result), - ); - }); + stack.cache().on_completion( + stack.dfn, + |fresh_trait_pred, provisional_result, provisional_dep_node| { + // Create a new `DepNode` that has dependencies on: + // * The `DepNode` for the original evaluation that resulted in a provisional cache + // entry being crated + // * The `DepNode` for the *current* evaluation, which resulted in us completing + // provisional caches entries and inserting them into the evaluation cache + // + // This ensures that when a query reads this entry from the evaluation cache, + // it will end up (transitively) dependening on all of the incr-comp dependencies + // created during the evaluation of this trait. For example, evaluating a trait + // will usually require us to invoke `type_of(field_def_id)` to determine the + // constituent types, and we want any queries reading from this evaluation + // cache entry to end up with a transitive `type_of(field_def_id`)` dependency. + // + // By using `in_task`, we're also creating an edge from the *current* query + // to the newly-created `combined_dep_node`. This is probably redundant, + // but it's better to add too many dep graph edges than to add too few + // dep graph edges. + let ((), combined_dep_node) = self.in_task(|this| { + this.tcx().dep_graph.read_index(provisional_dep_node); + this.tcx().dep_graph.read_index(dep_node); + }); + self.insert_evaluation_cache( + param_env, + fresh_trait_pred, + combined_dep_node, + provisional_result.max(result), + ); + }, + ); } else { debug!(?result, "PROVISIONAL"); debug!( @@ -781,7 +805,13 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { fresh_trait_pred, stack.depth, reached_depth, ); - stack.cache().insert_provisional(stack.dfn, reached_depth, fresh_trait_pred, result); + stack.cache().insert_provisional( + stack.dfn, + reached_depth, + fresh_trait_pred, + result, + dep_node, + ); } Ok(result) @@ -2506,6 +2536,11 @@ struct ProvisionalEvaluation { from_dfn: usize, reached_depth: usize, result: EvaluationResult, + /// The `DepNodeIndex` created for the `evaluate_stack` call for this provisional + /// evaluation. When we create an entry in the evaluation cache using this provisional + /// cache entry (see `on_completion`), we use this `dep_node` to ensure that future reads from + /// the cache will have all of the necessary incr comp dependencies tracked. + dep_node: DepNodeIndex, } impl<'tcx> Default for ProvisionalEvaluationCache<'tcx> { @@ -2548,6 +2583,7 @@ impl<'tcx> ProvisionalEvaluationCache<'tcx> { reached_depth: usize, fresh_trait_pred: ty::PolyTraitPredicate<'tcx>, result: EvaluationResult, + dep_node: DepNodeIndex, ) { debug!(?from_dfn, ?fresh_trait_pred, ?result, "insert_provisional"); @@ -2573,7 +2609,10 @@ impl<'tcx> ProvisionalEvaluationCache<'tcx> { } } - map.insert(fresh_trait_pred, ProvisionalEvaluation { from_dfn, reached_depth, result }); + map.insert( + fresh_trait_pred, + ProvisionalEvaluation { from_dfn, reached_depth, result, dep_node }, + ); } /// Invoked when the node with dfn `dfn` does not get a successful @@ -2624,7 +2663,7 @@ impl<'tcx> ProvisionalEvaluationCache<'tcx> { fn on_completion( &self, dfn: usize, - mut op: impl FnMut(ty::PolyTraitPredicate<'tcx>, EvaluationResult), + mut op: impl FnMut(ty::PolyTraitPredicate<'tcx>, EvaluationResult, DepNodeIndex), ) { debug!(?dfn, "on_completion"); @@ -2633,7 +2672,7 @@ impl<'tcx> ProvisionalEvaluationCache<'tcx> { { debug!(?fresh_trait_pred, ?eval, "on_completion"); - op(fresh_trait_pred, eval.result); + op(fresh_trait_pred, eval.result, eval.dep_node); } } } diff --git a/src/test/incremental/issue-92987-provisional-dep-node.rs b/src/test/incremental/issue-92987-provisional-dep-node.rs new file mode 100644 index 0000000000000..a48a8373c2b59 --- /dev/null +++ b/src/test/incremental/issue-92987-provisional-dep-node.rs @@ -0,0 +1,24 @@ +// revisions: rpass1 rpass2 + +// Regression test for issue #92987 +// Tests that we properly manage `DepNode`s during trait evaluation +// involing an auto-trait cycle. + +#[cfg(rpass1)] +struct CycleOne(Box); + +#[cfg(rpass2)] +enum CycleOne { + Variant(Box) +} + +struct CycleTwo(CycleOne); + +fn assert_send() {} + +fn bar() { + assert_send::(); + assert_send::(); +} + +fn main() {}