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.

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

Use PredicateObligations instead of Predicates #69745

Merged
merged 2 commits into from
Apr 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion src/librustc_infer/infer/outlives/verify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,10 @@ impl<'cx, 'tcx> VerifyBoundCx<'cx, 'tcx> {
let identity_proj = tcx.mk_projection(assoc_item_def_id, identity_substs);
self.collect_outlives_from_predicate_list(
move |ty| ty == identity_proj,
traits::elaborate_predicates(tcx, trait_predicates),
traits::elaborate_predicates(tcx, trait_predicates)
.into_iter()
.map(|o| o.predicate)
.collect::<Vec<_>>(),
Comment on lines +300 to +302
Copy link
Member

@eddyb eddyb Apr 17, 2020

Choose a reason for hiding this comment

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

Ahh I wonder if it's stuff like this - this is very wasteful and unlikely to be needed.

)
.map(|b| b.1)
}
Expand Down
68 changes: 48 additions & 20 deletions src/librustc_infer/traits/util.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
use smallvec::smallvec;

use crate::traits::{Obligation, ObligationCause, PredicateObligation};
use rustc_data_structures::fx::FxHashSet;
use rustc_middle::ty::outlives::Component;
use rustc_middle::ty::{self, ToPolyTraitRef, ToPredicate, TyCtxt, WithConstness};
use rustc_span::Span;

pub fn anonymize_predicate<'tcx>(
tcx: TyCtxt<'tcx>,
Expand Down Expand Up @@ -87,7 +89,7 @@ impl<T: AsRef<ty::Predicate<'tcx>>> Extend<T> for PredicateSet<'tcx> {
/// holds as well. Similarly, if we have `trait Foo: 'static`, and we know that
/// `T: Foo`, then we know that `T: 'static`.
pub struct Elaborator<'tcx> {
stack: Vec<ty::Predicate<'tcx>>,
stack: Vec<PredicateObligation<'tcx>>,
visited: PredicateSet<'tcx>,
}

Expand All @@ -112,35 +114,60 @@ pub fn elaborate_predicates<'tcx>(
) -> Elaborator<'tcx> {
let mut visited = PredicateSet::new(tcx);
predicates.retain(|pred| visited.insert(pred));
Elaborator { stack: predicates, visited }
let obligations: Vec<_> =
predicates.into_iter().map(|predicate| predicate_obligation(predicate, None)).collect();
Comment on lines +117 to +118
Copy link
Member

Choose a reason for hiding this comment

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

Again, this is Vec -> Vec, which we should avoid ever doing. I wish I spotted this earlier, oof.

elaborate_obligations(tcx, obligations)
}

pub fn elaborate_obligations<'tcx>(
tcx: TyCtxt<'tcx>,
mut obligations: Vec<PredicateObligation<'tcx>>,
) -> Elaborator<'tcx> {
let mut visited = PredicateSet::new(tcx);
obligations.retain(|obligation| visited.insert(&obligation.predicate));
Elaborator { stack: obligations, visited }
}

fn predicate_obligation<'tcx>(
predicate: ty::Predicate<'tcx>,
span: Option<Span>,
) -> PredicateObligation<'tcx> {
let mut cause = ObligationCause::dummy();
if let Some(span) = span {
cause.span = span;
}
Obligation { cause, param_env: ty::ParamEnv::empty(), recursion_depth: 0, predicate }
}
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved

impl Elaborator<'tcx> {
pub fn filter_to_traits(self) -> FilterToTraits<Self> {
FilterToTraits::new(self)
}

fn elaborate(&mut self, predicate: &ty::Predicate<'tcx>) {
fn elaborate(&mut self, obligation: &PredicateObligation<'tcx>) {
let tcx = self.visited.tcx;
match *predicate {
match obligation.predicate {
ty::Predicate::Trait(ref data, _) => {
// Get predicates declared on the trait.
let predicates = tcx.super_predicates_of(data.def_id());

let predicates = predicates
.predicates
.iter()
.map(|(pred, _)| pred.subst_supertrait(tcx, &data.to_poly_trait_ref()));
debug!("super_predicates: data={:?} predicates={:?}", data, predicates.clone());
let obligations = predicates.predicates.iter().map(|(pred, span)| {
predicate_obligation(
pred.subst_supertrait(tcx, &data.to_poly_trait_ref()),
Some(*span),
)
});
debug!("super_predicates: data={:?} predicates={:?}", data, &obligations);

// Only keep those bounds that we haven't already seen.
// This is necessary to prevent infinite recursion in some
// cases. One common case is when people define
// `trait Sized: Sized { }` rather than `trait Sized { }`.
let visited = &mut self.visited;
let predicates = predicates.filter(|pred| visited.insert(pred));
let obligations =
obligations.filter(|obligation| visited.insert(&obligation.predicate));

self.stack.extend(predicates);
self.stack.extend(obligations);
}
ty::Predicate::WellFormed(..) => {
// Currently, we do not elaborate WF predicates,
Expand Down Expand Up @@ -221,25 +248,26 @@ impl Elaborator<'tcx> {
None
}
})
.filter(|p| visited.insert(p)),
.filter(|p| visited.insert(p))
.map(|p| predicate_obligation(p, None)),
);
}
}
}
}

impl Iterator for Elaborator<'tcx> {
type Item = ty::Predicate<'tcx>;
type Item = PredicateObligation<'tcx>;

fn size_hint(&self) -> (usize, Option<usize>) {
(self.stack.len(), None)
}

fn next(&mut self) -> Option<ty::Predicate<'tcx>> {
fn next(&mut self) -> Option<Self::Item> {
// Extract next item from top-most stack frame, if any.
if let Some(pred) = self.stack.pop() {
self.elaborate(&pred);
Some(pred)
if let Some(obligation) = self.stack.pop() {
self.elaborate(&obligation);
Some(obligation)
} else {
None
}
Expand Down Expand Up @@ -282,12 +310,12 @@ impl<I> FilterToTraits<I> {
}
}

impl<'tcx, I: Iterator<Item = ty::Predicate<'tcx>>> Iterator for FilterToTraits<I> {
impl<'tcx, I: Iterator<Item = PredicateObligation<'tcx>>> Iterator for FilterToTraits<I> {
type Item = ty::PolyTraitRef<'tcx>;

fn next(&mut self) -> Option<ty::PolyTraitRef<'tcx>> {
while let Some(pred) = self.base_iterator.next() {
if let ty::Predicate::Trait(data, _) = pred {
while let Some(obligation) = self.base_iterator.next() {
if let ty::Predicate::Trait(data, _) = obligation.predicate {
return Some(data.to_poly_trait_ref());
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/transform/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ impl<'tcx> MirPass<'tcx> for ConstProp {
.collect();
if !traits::normalize_and_test_predicates(
tcx,
traits::elaborate_predicates(tcx, predicates).collect(),
traits::elaborate_predicates(tcx, predicates).map(|o| o.predicate).collect(),
) {
trace!("ConstProp skipped for {:?}: found unsatisfiable predicates", source.def_id());
return;
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_trait_selection/opaque_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1255,8 +1255,8 @@ crate fn required_region_bounds(
assert!(!erased_self_ty.has_escaping_bound_vars());

traits::elaborate_predicates(tcx, predicates)
.filter_map(|predicate| {
match predicate {
.filter_map(|obligation| {
match obligation.predicate {
ty::Predicate::Projection(..)
| ty::Predicate::Trait(..)
| ty::Predicate::Subtype(..)
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_trait_selection/traits/auto_trait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,8 @@ impl AutoTraitFinder<'tcx> {

computed_preds.extend(user_computed_preds.iter().cloned());
let normalized_preds =
elaborate_predicates(tcx, computed_preds.iter().cloned().collect());
elaborate_predicates(tcx, computed_preds.iter().cloned().collect())
.map(|o| o.predicate);
new_env =
ty::ParamEnv::new(tcx.mk_predicates(normalized_preds), param_env.reveal, None);
}
Expand Down
10 changes: 6 additions & 4 deletions src/librustc_trait_selection/traits/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -976,8 +976,8 @@ impl<'a, 'tcx> InferCtxtPrivExt<'tcx> for InferCtxt<'a, 'tcx> {
}
};

for implication in super::elaborate_predicates(self.tcx, vec![*cond]) {
if let ty::Predicate::Trait(implication, _) = implication {
for obligation in super::elaborate_predicates(self.tcx, vec![*cond]) {
if let ty::Predicate::Trait(implication, _) = obligation.predicate {
let error = error.to_poly_trait_ref();
let implication = implication.to_poly_trait_ref();
// FIXME: I'm just not taking associated types at all here.
Expand Down Expand Up @@ -1387,7 +1387,9 @@ impl<'a, 'tcx> InferCtxtPrivExt<'tcx> for InferCtxt<'a, 'tcx> {
(self.tcx.sess.source_map().span_to_snippet(span), &obligation.cause.code)
{
let generics = self.tcx.generics_of(*def_id);
if !generics.params.is_empty() && !snippet.ends_with('>') {
if generics.params.iter().filter(|p| p.name.as_str() != "Self").next().is_some()
&& !snippet.ends_with('>')
{
// FIXME: To avoid spurious suggestions in functions where type arguments
// where already supplied, we check the snippet to make sure it doesn't
// end with a turbofish. Ideally we would have access to a `PathSegment`
Expand All @@ -1405,7 +1407,7 @@ impl<'a, 'tcx> InferCtxtPrivExt<'tcx> for InferCtxt<'a, 'tcx> {
// | `Tt::const_val::<[i8; 123]>::<T>`
// ...
// LL | const fn const_val<T: Sized>() -> usize {
// | --------- - required by this bound in `Tt::const_val`
// | - required by this bound in `Tt::const_val`
// |
// = note: cannot satisfy `_: Tt`

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,9 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
}
}

if let ObligationCauseCode::ItemObligation(item) = obligation.cause.code {
if let ObligationCauseCode::ItemObligation(item)
| ObligationCauseCode::BindingObligation(item, _) = obligation.cause.code
{
// FIXME: maybe also have some way of handling methods
// from other traits? That would require name resolution,
// which we might want to be some sort of hygienic.
Expand Down
13 changes: 10 additions & 3 deletions src/librustc_trait_selection/traits/error_reporting/suggestions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -948,7 +948,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
/// --> $DIR/issue-64130-2-send.rs:21:5
/// |
/// LL | fn is_send<T: Send>(t: T) { }
/// | ------- ---- required by this bound in `is_send`
/// | ---- required by this bound in `is_send`
/// ...
/// LL | is_send(bar());
/// | ^^^^^^^ future returned by `bar` is not send
Expand Down Expand Up @@ -1345,7 +1345,6 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
ObligationCauseCode::ItemObligation(item_def_id) => {
let item_name = tcx.def_path_str(item_def_id);
let msg = format!("required by `{}`", item_name);

if let Some(sp) = tcx.hir().span_if_local(item_def_id) {
let sp = tcx.sess.source_map().guess_head_span(sp);
err.span_label(sp, &msg);
Expand All @@ -1357,7 +1356,15 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
let item_name = tcx.def_path_str(item_def_id);
let msg = format!("required by this bound in `{}`", item_name);
if let Some(ident) = tcx.opt_item_name(item_def_id) {
err.span_label(ident.span, "");
let sm = self.tcx.sess.source_map();
let same_line =
match (sm.lookup_line(ident.span.hi()), sm.lookup_line(span.lo())) {
(Ok(l), Ok(r)) => l.line == r.line,
_ => true,
};
if !ident.span.overlaps(span) && !same_line {
err.span_label(ident.span, "");
}
}
if span != DUMMY_SP {
err.span_label(span, &msg);
Expand Down
4 changes: 3 additions & 1 deletion src/librustc_trait_selection/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,9 @@ pub fn normalize_param_env_or_error<'tcx>(
);

let mut predicates: Vec<_> =
util::elaborate_predicates(tcx, unnormalized_env.caller_bounds.to_vec()).collect();
util::elaborate_predicates(tcx, unnormalized_env.caller_bounds.to_vec())
.map(|obligation| obligation.predicate)
.collect();
Comment on lines -300 to +302
Copy link
Member

Choose a reason for hiding this comment

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

Tempted to try and find every allocation in the trait system and rewrite it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you going to be doing it or should I try and do some of it now?

Copy link
Member

Choose a reason for hiding this comment

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

If you want to play around with it, feel free to! I was mostly considering taking this over in case you don't have time for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I won't have time to look at elaborate_obligations, which is the one that might have the biggest impact, but I can have a small PR shortly where I'm replacing a bunch of these arguments with impl Iterator<Item = PredicateObligation> so that it becomes clearer where we're collecting multiple times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at this in closer detail, I think I might be able to simplify this a bit.


debug!("normalize_param_env_or_error: elaborated-predicates={:?}", predicates);

Expand Down
2 changes: 1 addition & 1 deletion src/librustc_trait_selection/traits/object_safety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ fn generics_require_sized_self(tcx: TyCtxt<'_>, def_id: DefId) -> bool {
// Search for a predicate like `Self : Sized` amongst the trait bounds.
let predicates = tcx.predicates_of(def_id);
let predicates = predicates.instantiate_identity(tcx).predicates;
elaborate_predicates(tcx, predicates).any(|predicate| match predicate {
elaborate_predicates(tcx, predicates).any(|obligation| match obligation.predicate {
ty::Predicate::Trait(ref trait_pred, _) => {
trait_pred.def_id() == sized_def_id && trait_pred.skip_binder().self_ty().is_param(0)
}
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_trait_selection/traits/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -900,7 +900,7 @@ fn assemble_candidates_from_trait_def<'cx, 'tcx>(
// If so, extract what we know from the trait and try to come up with a good answer.
let trait_predicates = tcx.predicates_of(def_id);
let bounds = trait_predicates.instantiate(tcx, substs);
let bounds = elaborate_predicates(tcx, bounds.predicates);
let bounds = elaborate_predicates(tcx, bounds.predicates).map(|o| o.predicate);
assemble_candidates_from_predicates(
selcx,
obligation,
Expand Down Expand Up @@ -1162,7 +1162,7 @@ fn confirm_object_candidate<'cx, 'tcx>(

// select only those projections that are actually projecting an
// item with the correct name
let env_predicates = env_predicates.filter_map(|p| match p {
let env_predicates = env_predicates.filter_map(|o| match o.predicate {
ty::Predicate::Projection(data) => {
if data.projection_def_id() == obligation.predicate.item_def_id {
Some(data)
Expand Down
16 changes: 9 additions & 7 deletions src/librustc_trait_selection/traits/wf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -312,19 +312,18 @@ impl<'a, 'tcx> WfPredicates<'a, 'tcx> {
let item = self.item;

if let Elaborate::All = elaborate {
let predicates = obligations.iter().map(|obligation| obligation.predicate).collect();
let implied_obligations = traits::elaborate_predicates(tcx, predicates);
let implied_obligations = implied_obligations.map(|pred| {
let implied_obligations = traits::util::elaborate_obligations(tcx, obligations.clone());
Copy link
Member

Choose a reason for hiding this comment

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

At a glance this seems wasteful but it's not worse than it was before. Presumably we can avoid this clone altogether, but I haven't checked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason I didn't strip them outright was because of retain calls in elaborate_predicates, but if we can change all cases of Vec<Pred> argument types into impl Iterator<Item = Pred>, we should be faster than before.

let implied_obligations = implied_obligations.map(|obligation| {
let mut cause = cause.clone();
extend_cause_with_original_assoc_item_obligation(
tcx,
trait_ref,
item,
&mut cause,
&pred,
&obligation.predicate,
tcx.associated_items(trait_ref.def_id).in_definition_order().copied(),
);
traits::Obligation::new(cause, param_env, pred)
traits::Obligation::new(cause, param_env, obligation.predicate)
});
self.out.extend(implied_obligations);
}
Expand Down Expand Up @@ -613,11 +612,14 @@ impl<'a, 'tcx> WfPredicates<'a, 'tcx> {
substs: SubstsRef<'tcx>,
) -> Vec<traits::PredicateObligation<'tcx>> {
let predicates = self.infcx.tcx.predicates_of(def_id).instantiate(self.infcx.tcx, substs);
let cause = self.cause(traits::ItemObligation(def_id));
predicates
.predicates
.into_iter()
.map(|pred| traits::Obligation::new(cause.clone(), self.param_env, pred))
.zip(predicates.spans.into_iter())
.map(|(pred, span)| {
let cause = self.cause(traits::BindingObligation(def_id, span));
traits::Obligation::new(cause, self.param_env, pred)
})
.filter(|pred| !pred.has_escaping_bound_vars())
.collect()
}
Expand Down
6 changes: 3 additions & 3 deletions src/librustc_typeck/astconv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1601,12 +1601,12 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
for (base_trait_ref, span, constness) in regular_traits_refs_spans {
assert_eq!(constness, Constness::NotConst);

for trait_ref in traits::elaborate_trait_ref(tcx, base_trait_ref) {
for obligation in traits::elaborate_trait_ref(tcx, base_trait_ref) {
debug!(
"conv_object_ty_poly_trait_ref: observing object predicate `{:?}`",
trait_ref
obligation.predicate
);
match trait_ref {
match obligation.predicate {
ty::Predicate::Trait(pred, _) => {
associated_types.entry(span).or_default().extend(
tcx.associated_items(pred.def_id())
Expand Down
6 changes: 4 additions & 2 deletions src/librustc_typeck/check/method/confirm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -573,13 +573,15 @@ impl<'a, 'tcx> ConfirmContext<'a, 'tcx> {
};

traits::elaborate_predicates(self.tcx, predicates.predicates.clone())
.filter_map(|predicate| match predicate {
.filter_map(|obligation| match obligation.predicate {
ty::Predicate::Trait(trait_pred, _) if trait_pred.def_id() == sized_def_id => {
let span = predicates
.predicates
.iter()
.zip(predicates.spans.iter())
.filter_map(|(p, span)| if *p == predicate { Some(*span) } else { None })
.filter_map(
|(p, span)| if *p == obligation.predicate { Some(*span) } else { None },
)
.next()
.unwrap_or(rustc_span::DUMMY_SP);
Some((trait_pred, span))
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_typeck/check/wfcheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1226,7 +1226,8 @@ fn check_false_global_bounds(fcx: &FnCtxt<'_, '_>, span: Span, id: hir::HirId) {
// Check elaborated bounds.
let implied_obligations = traits::elaborate_predicates(fcx.tcx, predicates);

for pred in implied_obligations {
for obligation in implied_obligations {
let pred = obligation.predicate;
// Match the existing behavior.
if pred.is_global() && !pred.has_late_bound_regions() {
let pred = fcx.normalize_associated_types_in(span, &pred);
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_typeck/collect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1650,7 +1650,7 @@ fn predicates_of(tcx: TyCtxt<'_>, def_id: DefId) -> ty::GenericPredicates<'_> {
// prove that the trait applies to the types that were
// used, and adding the predicate into this list ensures
// that this is done.
let span = tcx.def_span(def_id);
let span = tcx.sess.source_map().guess_head_span(tcx.def_span(def_id));
result.predicates =
tcx.arena.alloc_from_iter(result.predicates.iter().copied().chain(std::iter::once((
ty::TraitRef::identity(tcx, def_id).without_const().to_predicate(),
Expand Down
Loading