From 04964d7e4c9de4b0b8d771e2cdd1b5ef2166c85f Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 24 Nov 2021 11:23:08 -0600 Subject: [PATCH 1/2] Use a fresh `InferCtxt` when we 'speculatively' evaluate predicates The existing `InferCtxt` may already have in-progress projections for some of the predicates we may (recursively evaluate). This can cause us to incorrect produce (and cache!) `EvaluatedToRecur`, leading us to incorrectly mark a predicate as unimplemented. We now create a fresh `InferCtxt` for each sub-obligation that we 'speculatively' evaluate. This causes us to miss out on some legitimate caching opportunities, but ensures that our speculative evaluation cannot pollute any of the caches from the 'real' `InferCtxt`. The evaluation can still update *global* caches, but our global caches don't have any notion of 'in progress', so this is fine. Fixes #90662 --- .../src/traits/project.rs | 20 ++++++++--- .../traits/issue-90662-projection-caching.rs | 34 +++++++++++++++++++ 2 files changed, 50 insertions(+), 4 deletions(-) create mode 100644 src/test/ui/traits/issue-90662-projection-caching.rs diff --git a/compiler/rustc_trait_selection/src/traits/project.rs b/compiler/rustc_trait_selection/src/traits/project.rs index b8c66931cbe52..e8f115b3e1b8c 100644 --- a/compiler/rustc_trait_selection/src/traits/project.rs +++ b/compiler/rustc_trait_selection/src/traits/project.rs @@ -16,6 +16,7 @@ use super::{ ImplSourceGeneratorData, ImplSourcePointeeData, ImplSourceUserDefinedData, }; use super::{Normalized, NormalizedTy, ProjectionCacheEntry, ProjectionCacheKey}; +use rustc_infer::infer::TyCtxtInferExt; use crate::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind}; use crate::infer::{InferCtxt, InferOk, LateBoundRegionConversionTime}; @@ -944,8 +945,8 @@ fn opt_normalize_projection_type<'a, 'b, 'tcx>( Normalized { value: projected_ty, obligations: projected_obligations } }; - let mut canonical = - SelectionContext::with_query_mode(selcx.infcx(), TraitQueryMode::Canonical); + let tcx = selcx.infcx().tcx; + result.obligations.drain_filter(|projected_obligation| { // If any global obligations always apply, considering regions, then we don't // need to include them. The `is_global` check rules out inference variables, @@ -957,10 +958,21 @@ fn opt_normalize_projection_type<'a, 'b, 'tcx>( // the result to `EvaluatedToOkModuloRegions`, while an // `EvaluatedToOk` obligation will never change the result. // See #85360 for more details - projected_obligation.is_global(canonical.tcx()) - && canonical + if !projected_obligation.is_global(tcx) { + return false; + } + + // We create a fresh `InferCtxt` for each predicate we speculatively evaluate, + // so that we won't create (and cache) any spurious projection cycles in the main + // `InferCtxt` + tcx.infer_ctxt().enter(|infcx| { + let mut canonical = + SelectionContext::with_query_mode(&infcx, TraitQueryMode::Canonical); + + canonical .evaluate_root_obligation(projected_obligation) .map_or(false, |res| res.must_apply_considering_regions()) + }) }); if use_cache { diff --git a/src/test/ui/traits/issue-90662-projection-caching.rs b/src/test/ui/traits/issue-90662-projection-caching.rs new file mode 100644 index 0000000000000..879f30071bfdb --- /dev/null +++ b/src/test/ui/traits/issue-90662-projection-caching.rs @@ -0,0 +1,34 @@ +// check-pass + +// Regression test for issue #90662 +// Tests that projection caching does not cause a spurious error + +trait HasProvider {} +trait Provider { + type Interface: ?Sized; +} + +trait Repository {} +trait Service {} + +struct DbConnection; +impl Provider for DbConnection { + type Interface = DbConnection; +} + +struct RepositoryImpl; +impl> Provider for RepositoryImpl { + type Interface = dyn Repository; +} + +struct ServiceImpl; +impl> Provider for ServiceImpl { + type Interface = dyn Service; +} + +struct TestModule; +impl HasProvider<>::Interface> for TestModule {} +impl HasProvider<>::Interface> for TestModule {} +impl HasProvider<>::Interface> for TestModule {} + +fn main() {} From 931a3a631e80348068f49ce16fc73a7d54e020a3 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sat, 27 Nov 2021 13:21:51 -0600 Subject: [PATCH 2/2] Re-use the fresh InferCtxt --- .../src/traits/project.rs | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/project.rs b/compiler/rustc_trait_selection/src/traits/project.rs index e8f115b3e1b8c..9ca00d5e58913 100644 --- a/compiler/rustc_trait_selection/src/traits/project.rs +++ b/compiler/rustc_trait_selection/src/traits/project.rs @@ -947,32 +947,32 @@ fn opt_normalize_projection_type<'a, 'b, 'tcx>( let tcx = selcx.infcx().tcx; - result.obligations.drain_filter(|projected_obligation| { - // If any global obligations always apply, considering regions, then we don't - // need to include them. The `is_global` check rules out inference variables, - // so there's no need for the caller of `opt_normalize_projection_type` - // to evaluate them. - // Note that we do *not* discard obligations that evaluate to - // `EvaluatedtoOkModuloRegions`. Evaluating these obligations - // inside of a query (e.g. `evaluate_obligation`) can change - // the result to `EvaluatedToOkModuloRegions`, while an - // `EvaluatedToOk` obligation will never change the result. - // See #85360 for more details - if !projected_obligation.is_global(tcx) { - return false; - } - - // We create a fresh `InferCtxt` for each predicate we speculatively evaluate, - // so that we won't create (and cache) any spurious projection cycles in the main - // `InferCtxt` - tcx.infer_ctxt().enter(|infcx| { - let mut canonical = - SelectionContext::with_query_mode(&infcx, TraitQueryMode::Canonical); + // We create a fresh `InferCtxt` for speculative evaluation + // so that we won't create (and cache) any spurious projection cycles in the main + // `InferCtxt` + tcx.infer_ctxt().enter(|infcx| { + let mut canonical = + SelectionContext::with_query_mode(&infcx, TraitQueryMode::Canonical); + + result.obligations.drain_filter(|projected_obligation| { + // If any global obligations always apply, considering regions, then we don't + // need to include them. The `is_global` check rules out inference variables, + // so there's no need for the caller of `opt_normalize_projection_type` + // to evaluate them. + // Note that we do *not* discard obligations that evaluate to + // `EvaluatedtoOkModuloRegions`. Evaluating these obligations + // inside of a query (e.g. `evaluate_obligation`) can change + // the result to `EvaluatedToOkModuloRegions`, while an + // `EvaluatedToOk` obligation will never change the result. + // See #85360 for more details + if !projected_obligation.is_global(tcx) { + return false; + } canonical .evaluate_root_obligation(projected_obligation) .map_or(false, |res| res.must_apply_considering_regions()) - }) + }); }); if use_cache {