Skip to content

Commit

Permalink
Auto merge of #85868 - Aaron1011:projection-cache, r=jackh726
Browse files Browse the repository at this point in the history
Preserve most sub-obligations in the projection cache

Fixes #85360

When we evaluate a projection predicate, we may produce sub-obligations. During trait evaluation, evaluating these sub-obligations might cause us to produce `EvaluatedToOkModuloRegions`.

When we cache the result of projection in our projection cache, we try to throw away some of the sub-obligations, so that we don't need to re-evaluate/process them the next time we need to perform this particular projection. However, we may end up throwing away predicates that will (recursively) evaluate to `EvaluatedToOkModuloRegions`. If we do, then the result of evaluating a predicate will depend on the state of the predicate cache - this is global untracked state, which interacts badly with incremental compilation.

To fix this, we now only discard global predicates that evaluate to `EvaluatedToOk`. This ensures that any predicates that (may) evaluate to `EvaluatedToOkModuloRegions` are kept in the cache, and influence the results of any queries which perform this projection.
  • Loading branch information
bors committed Sep 2, 2021
2 parents b834c4c + 611191f commit 371f3cd
Show file tree
Hide file tree
Showing 13 changed files with 62 additions and 82 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,9 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {

debug!("report_overflow_error_cycle: cycle={:?}", cycle);

self.report_overflow_error(&cycle[0], false);
// The 'deepest' obligation is most likely to have a useful
// cause 'backtrace'
self.report_overflow_error(cycle.iter().max_by_key(|p| p.recursion_depth).unwrap(), false);
}

fn report_selection_error(
Expand Down
71 changes: 24 additions & 47 deletions compiler/rustc_trait_selection/src/traits/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use super::PredicateObligation;
use super::Selection;
use super::SelectionContext;
use super::SelectionError;
use super::TraitQueryMode;
use super::{
ImplSourceClosureData, ImplSourceDiscriminantKindData, ImplSourceFnPointerData,
ImplSourceGeneratorData, ImplSourcePointeeData, ImplSourceUserDefinedData,
Expand All @@ -18,7 +19,7 @@ use super::{Normalized, NormalizedTy, ProjectionCacheEntry, ProjectionCacheKey};

use crate::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
use crate::infer::{InferCtxt, InferOk, LateBoundRegionConversionTime};
use crate::traits::error_reporting::InferCtxtExt;
use crate::traits::error_reporting::InferCtxtExt as _;
use rustc_data_structures::stack::ensure_sufficient_stack;
use rustc_errors::ErrorReported;
use rustc_hir::def_id::DefId;
Expand Down Expand Up @@ -912,6 +913,7 @@ fn opt_normalize_projection_type<'a, 'b, 'tcx>(
}

let obligation = Obligation::with_depth(cause.clone(), depth, param_env, projection_ty);

match project_type(selcx, &obligation) {
Ok(ProjectedTy::Progress(Progress {
ty: projected_ty,
Expand All @@ -925,7 +927,7 @@ fn opt_normalize_projection_type<'a, 'b, 'tcx>(
let projected_ty = selcx.infcx().resolve_vars_if_possible(projected_ty);
debug!(?projected_ty, ?depth, ?projected_obligations);

let result = if projected_ty.has_projections() {
let mut result = if projected_ty.has_projections() {
let mut normalizer = AssocTypeNormalizer::new(
selcx,
param_env,
Expand All @@ -942,8 +944,26 @@ fn opt_normalize_projection_type<'a, 'b, 'tcx>(
Normalized { value: projected_ty, obligations: projected_obligations }
};

let cache_value = prune_cache_value_obligations(infcx, &result);
infcx.inner.borrow_mut().projection_cache().insert_ty(cache_key, cache_value);
let mut canonical =
SelectionContext::with_query_mode(selcx.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
projected_obligation.is_global(canonical.tcx())
&& canonical
.evaluate_root_obligation(projected_obligation)
.map_or(false, |res| res.must_apply_considering_regions())
});

infcx.inner.borrow_mut().projection_cache().insert_ty(cache_key, result.clone());
obligations.extend(result.obligations);
Ok(Some(result.value))
}
Expand Down Expand Up @@ -974,49 +994,6 @@ fn opt_normalize_projection_type<'a, 'b, '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, 'tcx>(
infcx: &'a InferCtxt<'a, 'tcx>,
result: &NormalizedTy<'tcx>,
) -> NormalizedTy<'tcx> {
if infcx.unresolved_type_vars(&result.value).is_none() {
return NormalizedTy { value: result.value, obligations: vec![] };
}

let mut obligations: Vec<_> = result
.obligations
.iter()
.filter(|obligation| {
let bound_predicate = obligation.predicate.kind();
match bound_predicate.skip_binder() {
// We found a `T: Foo<X = U>` 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<X = ?1>` and `?1: Bar<X =
// ?0>`).
ty::PredicateKind::Projection(data) => {
infcx.unresolved_type_vars(&bound_predicate.rebind(data.ty)).is_some()
}

// We are only interested in `T: Foo<X = U>` 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 `<T as Trait>::Item`, but `T: Trait` does not
/// hold. In various error cases, we cannot generate a valid
/// normalized projection. Therefore, we create an inference variable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ impl<'cx, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'cx, 'tcx> {
// Run canonical query. If overflow occurs, rerun from scratch but this time
// in standard trait query mode so that overflow is handled appropriately
// within `SelectionContext`.
self.tcx.evaluate_obligation(c_pred)
self.tcx.at(obligation.cause.span(self.tcx)).evaluate_obligation(c_pred)
}

// Helper function that canonicalizes and runs the query. If an
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/traits/select/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -682,7 +682,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
}
});

debug!(?result);
debug!("finished: {:?} from {:?}", result, obligation);

result
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ where
use std::convert::TryFrom;
<[T; N.get()]>::try_from(())
//~^ error: the trait bound
//~^^ error: mismatched types
//~| error: the trait bound
//~| error: mismatched types
}

fn main() {}
12 changes: 6 additions & 6 deletions src/test/ui/impl-trait/auto-trait-leak.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ note: ...which requires building MIR for `cycle1`...
LL | fn cycle1() -> impl Clone {
| ^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires type-checking `cycle1`...
--> $DIR/auto-trait-leak.rs:12:1
--> $DIR/auto-trait-leak.rs:14:5
|
LL | fn cycle1() -> impl Clone {
| ^^^^^^^^^^^^^^^^^^^^^^^^^
LL | send(cycle2().clone());
| ^^^^
= note: ...which requires evaluating trait selection obligation `impl std::clone::Clone: std::marker::Send`...
note: ...which requires computing type of `cycle2::{opaque#0}`...
--> $DIR/auto-trait-leak.rs:19:16
Expand Down Expand Up @@ -66,10 +66,10 @@ note: ...which requires building MIR for `cycle2`...
LL | fn cycle2() -> impl Clone {
| ^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires type-checking `cycle2`...
--> $DIR/auto-trait-leak.rs:19:1
--> $DIR/auto-trait-leak.rs:20:5
|
LL | fn cycle2() -> impl Clone {
| ^^^^^^^^^^^^^^^^^^^^^^^^^
LL | send(cycle1().clone());
| ^^^^
= note: ...which requires evaluating trait selection obligation `impl std::clone::Clone: std::marker::Send`...
= note: ...which again requires computing type of `cycle1::{opaque#0}`, completing the cycle
note: cycle used when checking item types in top-level module
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/traits/cycle-cache-err-60010.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ struct Runtime<DB: Database> {
}
struct SalsaStorage {
_parse: <ParseQuery as Query<RootDatabase>>::Data,
//~^ ERROR overflow
}

impl Database for RootDatabase {
Expand Down Expand Up @@ -67,7 +68,6 @@ pub(crate) fn goto_implementation(db: &RootDatabase) -> u32 {
// we used to fail to report an error here because we got the
// caching wrong.
SourceDatabase::parse(db);
//~^ ERROR overflow
22
}

Expand Down
16 changes: 8 additions & 8 deletions src/test/ui/traits/cycle-cache-err-60010.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error[E0275]: overflow evaluating the requirement `SalsaStorage: RefUnwindSafe`
--> $DIR/cycle-cache-err-60010.rs:69:5
--> $DIR/cycle-cache-err-60010.rs:27:13
|
LL | SourceDatabase::parse(db);
| ^^^^^^^^^^^^^^^^^^^^^
LL | _parse: <ParseQuery as Query<RootDatabase>>::Data,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: required because it appears within the type `*const SalsaStorage`
= note: required because it appears within the type `Unique<SalsaStorage>`
Expand All @@ -18,15 +18,15 @@ note: required because it appears within the type `RootDatabase`
LL | struct RootDatabase {
| ^^^^^^^^^^^^
note: required because of the requirements on the impl of `SourceDatabase` for `RootDatabase`
--> $DIR/cycle-cache-err-60010.rs:43:9
--> $DIR/cycle-cache-err-60010.rs:44:9
|
LL | impl<T> SourceDatabase for T
| ^^^^^^^^^^^^^^ ^
note: required by `SourceDatabase::parse`
--> $DIR/cycle-cache-err-60010.rs:14:5
note: required because of the requirements on the impl of `Query<RootDatabase>` for `ParseQuery`
--> $DIR/cycle-cache-err-60010.rs:37:10
|
LL | fn parse(&self) {
| ^^^^^^^^^^^^^^^
LL | impl<DB> Query<DB> for ParseQuery
| ^^^^^^^^^ ^^^^^^^^^^

error: aborting due to previous error

Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/traits/inductive-overflow/lifetime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ impl<'a> Y for C<'a> {
struct C<'a>(&'a ());
struct X<T: Y>(T::P);

impl<T: NotAuto> NotAuto for Box<T> {}
impl<T: Y> NotAuto for X<T> where T::P: NotAuto {} //~ NOTE: required
impl<T: NotAuto> NotAuto for Box<T> {} //~ NOTE: required
impl<T: Y> NotAuto for X<T> where T::P: NotAuto {}
impl<'a> NotAuto for C<'a> {}

fn is_send<S: NotAuto>() {}
Expand All @@ -26,6 +26,6 @@ fn main() {
// Should only be a few notes.
is_send::<X<C<'static>>>();
//~^ ERROR overflow evaluating
//~| 2 redundant requirements hidden
//~| 3 redundant requirements hidden
//~| required because of
}
12 changes: 6 additions & 6 deletions src/test/ui/traits/inductive-overflow/lifetime.stderr
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
error[E0275]: overflow evaluating the requirement `Box<X<C<'_>>>: NotAuto`
error[E0275]: overflow evaluating the requirement `X<C<'_>>: NotAuto`
--> $DIR/lifetime.rs:27:5
|
LL | is_send::<X<C<'static>>>();
| ^^^^^^^^^^^^^^^^^^^^^^^^
|
note: required because of the requirements on the impl of `NotAuto` for `X<C<'_>>`
--> $DIR/lifetime.rs:19:12
note: required because of the requirements on the impl of `NotAuto` for `Box<X<C<'_>>>`
--> $DIR/lifetime.rs:18:18
|
LL | impl<T: Y> NotAuto for X<T> where T::P: NotAuto {}
| ^^^^^^^ ^^^^
= note: 2 redundant requirements hidden
LL | impl<T: NotAuto> NotAuto for Box<T> {}
| ^^^^^^^ ^^^^^^
= note: 3 redundant requirements hidden
= note: required because of the requirements on the impl of `NotAuto` for `X<C<'static>>`
note: required by a bound in `is_send`
--> $DIR/lifetime.rs:22:15
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/traits/inductive-overflow/simultaneous.stderr
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error[E0275]: overflow evaluating the requirement `{integer}: Tweedledee`
error[E0275]: overflow evaluating the requirement `{integer}: Tweedledum`
--> $DIR/simultaneous.rs:18:5
|
LL | is_ee(4);
Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/type-alias-impl-trait/auto-trait-leakage3.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ LL | type Foo = impl std::fmt::Debug;
| ^^^^^^^^^^^^^^^^^^^^
|
note: ...which requires type-checking `m::bar`...
--> $DIR/auto-trait-leakage3.rs:14:5
--> $DIR/auto-trait-leakage3.rs:15:9
|
LL | pub fn bar() {
| ^^^^^^^^^^^^
LL | is_send(foo());
| ^^^^^^^
= note: ...which requires evaluating trait selection obligation `impl std::fmt::Debug: std::marker::Send`...
= note: ...which again requires computing type of `m::Foo::{opaque#0}`, completing the cycle
note: cycle used when checking item types in module `m`
Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/type-alias-impl-trait/inference-cycle.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ LL | type Foo = impl std::fmt::Debug;
| ^^^^^^^^^^^^^^^^^^^^
|
note: ...which requires type-checking `m::bar`...
--> $DIR/inference-cycle.rs:14:5
--> $DIR/inference-cycle.rs:15:9
|
LL | pub fn bar() {
| ^^^^^^^^^^^^
LL | is_send(foo()); // Today: error
| ^^^^^^^
= note: ...which requires evaluating trait selection obligation `impl std::fmt::Debug: std::marker::Send`...
= note: ...which again requires computing type of `m::Foo::{opaque#0}`, completing the cycle
note: cycle used when checking item types in module `m`
Expand Down

0 comments on commit 371f3cd

Please sign in to comment.