Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rustc: include ParamEnv in global trait select/eval cache keys. #66963

Merged
merged 1 commit into from
Dec 11, 2019
Merged

rustc: include ParamEnv in global trait select/eval cache keys. #66963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions src/librustc/traits/project.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1079,12 +1079,10 @@ fn assemble_candidates_from_impls<'cx, 'tcx>(
if !is_default {
true
} else if obligation.param_env.reveal == Reveal::All {
debug_assert!(!poly_trait_ref.needs_infer());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, did you see this debug assertion fail?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in a run-pass specialization test, as I assume this can happen inside a temporary inference context inside a normalization query.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This little bit of code is making me nervous. I'm wondering if we might get into some scenario where we have an unresolved inference variable and so we opt not to reveal the item. I think this would mean we normalize to (e.g.) <T as Foo<?X>>::Bar. But then later the inference variable ?X gets resolved in such a way that we could have revealed the value. This could lead to an inconsistency that causes some ICEs or something.

I think I would feel better if we flagged cases that involve inference variables as 'ambiguous'. I have to remember how that is done in this code, I can check if it has any impact.

Still, I could probably be persuaded to land it as is.

Copy link
Member Author

@eddyb eddyb Dec 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure not revealing is "ambiguous", to the extent that projection would be.
Doesn't ambiguity result in not making progress, in general?

This could lead to an inconsistency that causes some ICEs or something.

Wouldn't that same situation apply to non-specialized impls when e.g. there's a Foo<A> impl and a Foo<B> one and so ?X being resolved later to one of them would allow revealing Bar?

I don't mind ICEs caused indirectly by this, considering this was even more ICE-y before, but also keep in mind this requires specialization, which is still unstable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, looks like the assert was added by... me, in #35091, and it assumed that only monomorphic codegen would use the Reveal::All codepath (I was curious and dug through the history).

if !poly_trait_ref.needs_subst() {
true
} else {
false
}
// NOTE(eddyb) inference variables can resolve to parameters, so
// assume `poly_trait_ref` isn't monomorphic, if it contains any.
let poly_trait_ref = selcx.infcx().resolve_vars_if_possible(&poly_trait_ref);
!poly_trait_ref.needs_infer() && !poly_trait_ref.needs_subst()
} else {
false
}
Expand Down
25 changes: 15 additions & 10 deletions src/librustc/traits/select.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,10 @@ struct TraitObligationStack<'prev, 'tcx> {
#[derive(Clone, Default)]
pub struct SelectionCache<'tcx> {
hashmap: Lock<
FxHashMap<ty::TraitRef<'tcx>, WithDepNode<SelectionResult<'tcx, SelectionCandidate<'tcx>>>>,
FxHashMap<
ty::ParamEnvAnd<'tcx, ty::TraitRef<'tcx>>,
WithDepNode<SelectionResult<'tcx, SelectionCandidate<'tcx>>>,
>,
>,
}

Expand Down Expand Up @@ -490,7 +493,9 @@ impl<'tcx> From<OverflowError> for SelectionError<'tcx> {

#[derive(Clone, Default)]
pub struct EvaluationCache<'tcx> {
hashmap: Lock<FxHashMap<ty::PolyTraitRef<'tcx>, WithDepNode<EvaluationResult>>>,
hashmap: Lock<
FxHashMap<ty::ParamEnvAnd<'tcx, ty::PolyTraitRef<'tcx>>, WithDepNode<EvaluationResult>>,
>,
}

impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
Expand Down Expand Up @@ -1143,15 +1148,15 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
let tcx = self.tcx();
if self.can_use_global_caches(param_env) {
let cache = tcx.evaluation_cache.hashmap.borrow();
if let Some(cached) = cache.get(&trait_ref) {
if let Some(cached) = cache.get(&param_env.and(trait_ref)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so, in this case the only purpose is to capture the reveal mode, correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I can change to be literally that if e.g. it's a perf problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't expect that to have a perf impact, but might be worth including in a comment somewhere.

return Some(cached.get(tcx));
}
}
self.infcx
.evaluation_cache
.hashmap
.borrow()
.get(&trait_ref)
.get(&param_env.and(trait_ref))
.map(|v| v.get(tcx))
}

Expand Down Expand Up @@ -1182,7 +1187,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
.evaluation_cache
.hashmap
.borrow_mut()
.insert(trait_ref, WithDepNode::new(dep_node, result));
.insert(param_env.and(trait_ref), WithDepNode::new(dep_node, result));
return;
}
}
Expand All @@ -1195,7 +1200,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
.evaluation_cache
.hashmap
.borrow_mut()
.insert(trait_ref, WithDepNode::new(dep_node, result));
.insert(param_env.and(trait_ref), WithDepNode::new(dep_node, result));
}

/// For various reasons, it's possible for a subobligation
Expand Down Expand Up @@ -1602,15 +1607,15 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
let trait_ref = &cache_fresh_trait_pred.skip_binder().trait_ref;
if self.can_use_global_caches(param_env) {
let cache = tcx.selection_cache.hashmap.borrow();
if let Some(cached) = cache.get(&trait_ref) {
if let Some(cached) = cache.get(&param_env.and(*trait_ref)) {
return Some(cached.get(tcx));
}
}
self.infcx
.selection_cache
.hashmap
.borrow()
.get(trait_ref)
.get(&param_env.and(*trait_ref))
.map(|v| v.get(tcx))
}

Expand Down Expand Up @@ -1671,7 +1676,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
tcx.selection_cache
.hashmap
.borrow_mut()
.insert(trait_ref, WithDepNode::new(dep_node, candidate));
.insert(param_env.and(trait_ref), WithDepNode::new(dep_node, candidate));
return;
}
}
Expand All @@ -1685,7 +1690,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
.selection_cache
.hashmap
.borrow_mut()
.insert(trait_ref, WithDepNode::new(dep_node, candidate));
.insert(param_env.and(trait_ref), WithDepNode::new(dep_node, candidate));
}

fn assemble_candidates<'o>(
Expand Down
1 change: 1 addition & 0 deletions src/test/ui/type-alias-impl-trait/bound_reduction2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ trait TraitWithAssoc {

type Foo<V> = impl Trait<V>;
//~^ ERROR could not find defining uses
//~| ERROR the trait bound `T: TraitWithAssoc` is not satisfied

trait Trait<U> {}

Expand Down
14 changes: 12 additions & 2 deletions src/test/ui/type-alias-impl-trait/bound_reduction2.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
error[E0277]: the trait bound `T: TraitWithAssoc` is not satisfied
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, this error is definitely not great. Do you understand what's going on here? (I don't, at least not yet)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is because T ends up in the context of the impl Trait which doesn't have the parameter nor the relevant bound in its ParamEnv.
It's not great but this is a pre-existing bug in the existential type aliases feature that was being hidden because the Reveal mode was getting ignored after the first time.

--> $DIR/bound_reduction2.rs:10:1
|
LL | type Foo<V> = impl Trait<V>;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `TraitWithAssoc` is not implemented for `T`
...
LL | fn foo_desugared<T: TraitWithAssoc>(_: T) -> Foo<T::Assoc> {
| -- help: consider further restricting this bound: `T: TraitWithAssoc +`

error: defining opaque type use does not fully define opaque type: generic parameter `V` is specified as concrete type `<T as TraitWithAssoc>::Assoc`
--> $DIR/bound_reduction2.rs:17:1
--> $DIR/bound_reduction2.rs:18:1
|
LL | / fn foo_desugared<T: TraitWithAssoc>(_: T) -> Foo<T::Assoc> {
LL | | ()
Expand All @@ -12,5 +21,6 @@ error: could not find defining uses
LL | type Foo<V> = impl Trait<V>;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 2 previous errors
error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0277`.