From 80447c38aaf680f15c5bd37e154fb0bf01b68573 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sat, 2 Sep 2017 08:35:07 -0400 Subject: [PATCH 1/3] limit and clear cache obligations opportunistically Keep **all** the obligations for every projection is wasteful of memory and compilation time. We only really care about those subobligations that may inform the result of the projection (i.e., may help to resolve any inference variables that appear within). Therefore, we can clear the subobligations from the cache that don't potentially affect the result of the projection. On every cache hit, we also take the opportunity to check if the type variables have been resolved *yet* and, if so, clear out the pending obligations. Fixes #43613 --- src/librustc/infer/mod.rs | 12 +++++++ src/librustc/infer/resolve.rs | 39 ++++++++++++++++++++- src/librustc/traits/project.rs | 64 ++++++++++++++++++++++++++++++---- src/librustc/ty/mod.rs | 4 +++ 4 files changed, 111 insertions(+), 8 deletions(-) diff --git a/src/librustc/infer/mod.rs b/src/librustc/infer/mod.rs index 21af92a25e684..d838b2226e863 100644 --- a/src/librustc/infer/mod.rs +++ b/src/librustc/infer/mod.rs @@ -1160,6 +1160,18 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { value.fold_with(&mut r) } + /// Returns true if `T` contains unresolved type variables. In the + /// process of visiting `T`, this will resolve (where possible) + /// type variables in `T`, but it never constructs the final, + /// resolved type, so it's more efficient than + /// `resolve_type_vars_if_possible()`. + pub fn any_unresolved_type_vars(&self, value: &T) -> bool + where T: TypeFoldable<'tcx> + { + let mut r = resolve::UnresolvedTypeFinder::new(self); + value.visit_with(&mut r) + } + pub fn resolve_type_and_region_vars_if_possible(&self, value: &T) -> T where T: TypeFoldable<'tcx> { diff --git a/src/librustc/infer/resolve.rs b/src/librustc/infer/resolve.rs index 639a330dc6e67..10899e42afb81 100644 --- a/src/librustc/infer/resolve.rs +++ b/src/librustc/infer/resolve.rs @@ -10,7 +10,7 @@ use super::{InferCtxt, FixupError, FixupResult}; use ty::{self, Ty, TyCtxt, TypeFoldable}; -use ty::fold::TypeFolder; +use ty::fold::{TypeFolder, TypeVisitor}; /////////////////////////////////////////////////////////////////////////// // OPPORTUNISTIC TYPE RESOLVER @@ -80,6 +80,43 @@ impl<'a, 'gcx, 'tcx> TypeFolder<'gcx, 'tcx> for OpportunisticTypeAndRegionResolv } } +/////////////////////////////////////////////////////////////////////////// +// UNRESOLVED TYPE FINDER + +/// The unresolved type **finder** walks your type and searches for +/// type variables that don't yet have a value. They get pushed into a +/// vector. It does not construct the fully resolved type (which might +/// involve some hashing and so forth). +pub struct UnresolvedTypeFinder<'a, 'gcx: 'a+'tcx, 'tcx: 'a> { + infcx: &'a InferCtxt<'a, 'gcx, 'tcx>, +} + +impl<'a, 'gcx, 'tcx> UnresolvedTypeFinder<'a, 'gcx, 'tcx> { + pub fn new(infcx: &'a InferCtxt<'a, 'gcx, 'tcx>) -> Self { + UnresolvedTypeFinder { infcx } + } +} + +impl<'a, 'gcx, 'tcx> TypeVisitor<'tcx> for UnresolvedTypeFinder<'a, 'gcx, 'tcx> { + fn visit_ty(&mut self, t: Ty<'tcx>) -> bool { + let t = self.infcx.shallow_resolve(t); + if t.has_infer_types() { + if let ty::TyInfer(_) = t.sty { + // Since we called `shallow_resolve` above, this must + // be an (as yet...) unresolved inference variable. + true + } else { + // Otherwise, visit its contents. + t.super_visit_with(self) + } + } else { + // Micro-optimize: no inference types at all Can't have unresolved type + // variables, no need to visit the contents. + false + } + } +} + /////////////////////////////////////////////////////////////////////////// // FULL TYPE RESOLUTION diff --git a/src/librustc/traits/project.rs b/src/librustc/traits/project.rs index 512cfee12b05f..242281a0c7726 100644 --- a/src/librustc/traits/project.rs +++ b/src/librustc/traits/project.rs @@ -25,7 +25,7 @@ use super::VtableImplData; use super::util; use hir::def_id::DefId; -use infer::InferOk; +use infer::{InferCtxt, InferOk}; use infer::type_variable::TypeVariableOrigin; use rustc_data_structures::snapshot_map::{Snapshot, SnapshotMap}; use syntax::ast; @@ -416,7 +416,8 @@ fn opt_normalize_projection_type<'a, 'b, 'gcx, 'tcx>( // bounds. It might be the case that we want two distinct caches, // or else another kind of cache entry. - match infcx.projection_cache.borrow_mut().try_start(cache_key) { + let cache_result = infcx.projection_cache.borrow_mut().try_start(cache_key); + match cache_result { Ok(()) => { } Err(ProjectionCacheEntry::Ambiguous) => { // If we found ambiguity the last time, that generally @@ -466,7 +467,7 @@ fn opt_normalize_projection_type<'a, 'b, 'gcx, 'tcx>( projection_ty); selcx.infcx().report_overflow_error(&obligation, false); } - Err(ProjectionCacheEntry::NormalizedTy(ty)) => { + Err(ProjectionCacheEntry::NormalizedTy(mut ty)) => { // If we find the value in the cache, then return it along // with the obligations that went along with it. Note // that, when using a fulfillment context, these @@ -479,6 +480,14 @@ fn opt_normalize_projection_type<'a, 'b, 'gcx, 'tcx>( debug!("opt_normalize_projection_type: \ found normalized ty `{:?}`", ty); + + // Once we have inferred everything we need to know, we + // can ignore the `obligations` from that point on. + if !infcx.any_unresolved_type_vars(&ty.value) { + infcx.projection_cache.borrow_mut().complete(cache_key); + ty.obligations = vec![]; + } + return Some(ty); } Err(ProjectionCacheEntry::Error) => { @@ -527,7 +536,10 @@ fn opt_normalize_projection_type<'a, 'b, 'gcx, 'tcx>( obligations, } }; - infcx.projection_cache.borrow_mut().insert_ty(cache_key, &result); + + let cache_value = prune_cache_value_obligations(infcx, &result); + infcx.projection_cache.borrow_mut().insert_ty(cache_key, cache_value); + Some(result) } Ok(ProjectedTy::NoProgress(projected_ty)) => { @@ -538,7 +550,7 @@ fn opt_normalize_projection_type<'a, 'b, 'gcx, 'tcx>( value: projected_ty, obligations: vec![] }; - infcx.projection_cache.borrow_mut().insert_ty(cache_key, &result); + infcx.projection_cache.borrow_mut().insert_ty(cache_key, result.clone()); Some(result) } Err(ProjectionTyError::TooManyCandidates) => { @@ -562,6 +574,44 @@ fn opt_normalize_projection_type<'a, 'b, 'gcx, 'tcx>( } } +/// If there are unresolved type variables, then we need to include +/// any subobligations that bind them, at least until those type +/// variables are fully resolved. +fn prune_cache_value_obligations<'a, 'gcx, 'tcx>(infcx: &'a InferCtxt<'a, 'gcx, 'tcx>, + result: &NormalizedTy<'tcx>) + -> NormalizedTy<'tcx> { + if !infcx.any_unresolved_type_vars(&result.value) { + return NormalizedTy { value: result.value, obligations: vec![] }; + } + + let mut obligations: Vec<_> = + result.obligations + .iter() + .filter(|obligation| match obligation.predicate { + // We found a `T: Foo` predicate, let's check + // if `U` references any unresolved type + // variables. In principle, we only care if this + // projection can help resolve any of the type + // variables found in `result.value` -- but we just + // check for any type variables here, for fear of + // indirect obligations (e.g., we project to `?0`, + // but we have `T: Foo` and `?1: Bar`). + ty::Predicate::Projection(ref data) => + !infcx.any_unresolved_type_vars(&data.ty()), + + // We are only interested in `T: Foo` predicates, whre + // `U` references one of `unresolved_type_vars`. =) + _ => false, + }) + .cloned() + .collect(); + + obligations.shrink_to_fit(); + + NormalizedTy { value: result.value, obligations } +} + /// If we are projecting `::Item`, but `T: Trait` does not /// hold. In various error cases, we cannot generate a valid /// normalized projection. Therefore, we create an inference variable @@ -1493,10 +1543,10 @@ impl<'tcx> ProjectionCache<'tcx> { } /// Indicates that `key` was normalized to `value`. - fn insert_ty(&mut self, key: ProjectionCacheKey<'tcx>, value: &NormalizedTy<'tcx>) { + fn insert_ty(&mut self, key: ProjectionCacheKey<'tcx>, value: NormalizedTy<'tcx>) { debug!("ProjectionCacheEntry::insert_ty: adding cache entry: key={:?}, value={:?}", key, value); - let fresh_key = self.map.insert(key, ProjectionCacheEntry::NormalizedTy(value.clone())); + let fresh_key = self.map.insert(key, ProjectionCacheEntry::NormalizedTy(value)); assert!(!fresh_key, "never started projecting `{:?}`", key); } diff --git a/src/librustc/ty/mod.rs b/src/librustc/ty/mod.rs index 1851e1b8d34bb..1920cdb3f738f 100644 --- a/src/librustc/ty/mod.rs +++ b/src/librustc/ty/mod.rs @@ -1017,6 +1017,10 @@ impl<'tcx> PolyProjectionPredicate<'tcx> { // levels. ty::Binder(self.0.projection_ty.trait_ref(tcx)) } + + pub fn ty(&self) -> Binder> { + Binder(self.skip_binder().ty) // preserves binding levels + } } pub trait ToPolyTraitRef<'tcx> { From ffd21b184ccf159c2574f1062b5f3b188a603148 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 6 Sep 2017 17:54:37 -0400 Subject: [PATCH 2/3] add in a "paranoid" trait bound --- src/librustc/traits/project.rs | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/librustc/traits/project.rs b/src/librustc/traits/project.rs index 242281a0c7726..a8c08387075ae 100644 --- a/src/librustc/traits/project.rs +++ b/src/librustc/traits/project.rs @@ -488,6 +488,13 @@ fn opt_normalize_projection_type<'a, 'b, 'gcx, 'tcx>( ty.obligations = vec![]; } + push_paranoid_cache_value_obligation(infcx, + param_env, + projection_ty, + cause, + depth, + &mut ty); + return Some(ty); } Err(ProjectionCacheEntry::Error) => { @@ -612,6 +619,33 @@ fn prune_cache_value_obligations<'a, 'gcx, 'tcx>(infcx: &'a InferCtxt<'a, 'gcx, NormalizedTy { value: result.value, obligations } } +/// Whenever we give back a cache result for a projection like `::Item ==> X`, we *always* include the obligation to prove +/// that `T: Trait` (we may also include some other obligations). This +/// may or may not be necessary -- in principle, all the obligations +/// that must be proven to show that `T: Trait` were also returned +/// when the cache was first populated. But there is a vague concern +/// that perhaps someone would not have proven those, but also not +/// have used a snapshot, in which case the cache could remain +/// populated even though `T: Trait` has not been shown. Returning +/// this "paranoid" obligation ensures that, no matter what has come +/// before, if you prove the subobligations, we at least know that `T: +/// Trait` is implemented. +fn push_paranoid_cache_value_obligation<'a, 'gcx, 'tcx>(infcx: &'a InferCtxt<'a, 'gcx, 'tcx>, + param_env: ty::ParamEnv<'tcx>, + projection_ty: ty::ProjectionTy<'tcx>, + cause: ObligationCause<'tcx>, + depth: usize, + result: &mut NormalizedTy<'tcx>) +{ + let trait_ref = projection_ty.trait_ref(infcx.tcx).to_poly_trait_ref(); + let trait_obligation = Obligation { cause, + recursion_depth: depth, + param_env, + predicate: trait_ref.to_predicate() }; + result.obligations.push(trait_obligation); +} + /// If we are projecting `::Item`, but `T: Trait` does not /// hold. In various error cases, we cannot generate a valid /// normalized projection. Therefore, we create an inference variable From c1dddcec06428f5a19adb936774b31d42b57d8af Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 7 Sep 2017 12:38:33 -0400 Subject: [PATCH 3/3] update comment --- src/librustc/traits/project.rs | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/librustc/traits/project.rs b/src/librustc/traits/project.rs index a8c08387075ae..fa5589ba5fe17 100644 --- a/src/librustc/traits/project.rs +++ b/src/librustc/traits/project.rs @@ -624,13 +624,24 @@ fn prune_cache_value_obligations<'a, 'gcx, 'tcx>(infcx: &'a InferCtxt<'a, 'gcx, /// that `T: Trait` (we may also include some other obligations). This /// may or may not be necessary -- in principle, all the obligations /// that must be proven to show that `T: Trait` were also returned -/// when the cache was first populated. But there is a vague concern -/// that perhaps someone would not have proven those, but also not -/// have used a snapshot, in which case the cache could remain -/// populated even though `T: Trait` has not been shown. Returning -/// this "paranoid" obligation ensures that, no matter what has come -/// before, if you prove the subobligations, we at least know that `T: -/// Trait` is implemented. +/// when the cache was first populated. But there are some vague concerns, +/// and so we take the precatuionary measure of including `T: Trait` in +/// the result: +/// +/// Concern #1. The current setup is fragile. Perhaps someone could +/// have failed to prove the concerns from when the cache was +/// populated, but also not have used a snapshot, in which case the +/// cache could remain populated even though `T: Trait` has not been +/// shown. In this case, the "other code" is at fault -- when you +/// project something, you are supposed to either have a snapshot or +/// else prove all the resulting obligations -- but it's still easy to +/// get wrong. +/// +/// Concern #2. Even within the snapshot, if those original +/// obligations are not yet proven, then we are able to do projections +/// that may yet turn out to be wrong. This *may* lead to some sort +/// of trouble, though we don't have a concrete example of how that +/// can occur yet. But it seems risky at best. fn push_paranoid_cache_value_obligation<'a, 'gcx, 'tcx>(infcx: &'a InferCtxt<'a, 'gcx, 'tcx>, param_env: ty::ParamEnv<'tcx>, projection_ty: ty::ProjectionTy<'tcx>,