Skip to content

Commit

Permalink
Auto merge of #77720 - matthewjasper:fix-trait-ices, r=nikomatsakis
Browse files Browse the repository at this point in the history
Fix trait solving ICEs

- Selection candidates that are known to be applicable are preferred
  over candidates that are not.
- Don't ICE if a projection/object candidate is no longer applicable
  (this can happen due to cycles in normalization)
- Normalize supertraits when finding trait object candidates

Closes #77653
Closes #77656

r? `@nikomatsakis`
  • Loading branch information
bors committed Oct 22, 2020
2 parents 500ddc5 + 50dde2e commit a9cd294
Show file tree
Hide file tree
Showing 6 changed files with 141 additions and 86 deletions.
5 changes: 4 additions & 1 deletion compiler/rustc_middle/src/traits/select.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,10 @@ pub enum SelectionCandidate<'tcx> {

TraitAliasCandidate(DefId),

ObjectCandidate,
/// Matching `dyn Trait` with a supertrait of `Trait`. The index is the
/// position in the iterator returned by
/// `rustc_infer::traits::util::supertraits`.
ObjectCandidate(usize),

BuiltinObjectCandidate,

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -642,24 +642,30 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {

debug!(?poly_trait_ref, "assemble_candidates_from_object_ty");

let poly_trait_predicate = self.infcx().resolve_vars_if_possible(&obligation.predicate);
let placeholder_trait_predicate =
self.infcx().replace_bound_vars_with_placeholders(&poly_trait_predicate);

// Count only those upcast versions that match the trait-ref
// we are looking for. Specifically, do not only check for the
// correct trait, but also the correct type parameters.
// For example, we may be trying to upcast `Foo` to `Bar<i32>`,
// but `Foo` is declared as `trait Foo: Bar<u32>`.
let upcast_trait_refs = util::supertraits(self.tcx(), poly_trait_ref)
.filter(|upcast_trait_ref| {
self.infcx
.probe(|_| self.match_poly_trait_ref(obligation, *upcast_trait_ref).is_ok())
let candidate_supertraits = util::supertraits(self.tcx(), poly_trait_ref)
.enumerate()
.filter(|&(_, upcast_trait_ref)| {
self.infcx.probe(|_| {
self.match_normalize_trait_ref(
obligation,
upcast_trait_ref,
placeholder_trait_predicate.trait_ref,
)
.is_ok()
})
})
.count();
.map(|(idx, _)| ObjectCandidate(idx));

if upcast_trait_refs > 1 {
// Can be upcast in many ways; need more type information.
candidates.ambiguous = true;
} else if upcast_trait_refs == 1 {
candidates.vec.push(ObjectCandidate);
}
candidates.vec.extend(candidate_supertraits);
})
}

Expand Down
97 changes: 44 additions & 53 deletions compiler/rustc_trait_selection/src/traits/select/confirmation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,15 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
}

ProjectionCandidate(idx) => {
let obligations = self.confirm_projection_candidate(obligation, idx);
let obligations = self.confirm_projection_candidate(obligation, idx)?;
Ok(ImplSource::Param(obligations))
}

ObjectCandidate(idx) => {
let data = self.confirm_object_candidate(obligation, idx)?;
Ok(ImplSource::Object(data))
}

ClosureCandidate => {
let vtable_closure = self.confirm_closure_candidate(obligation)?;
Ok(ImplSource::Closure(vtable_closure))
Expand All @@ -97,11 +102,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
Ok(ImplSource::TraitAlias(data))
}

ObjectCandidate => {
let data = self.confirm_object_candidate(obligation);
Ok(ImplSource::Object(data))
}

BuiltinObjectCandidate => {
// This indicates something like `Trait + Send: Send`. In this case, we know that
// this holds because that's what the object type is telling us, and there's really
Expand All @@ -120,7 +120,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
&mut self,
obligation: &TraitObligation<'tcx>,
idx: usize,
) -> Vec<PredicateObligation<'tcx>> {
) -> Result<Vec<PredicateObligation<'tcx>>, SelectionError<'tcx>> {
self.infcx.commit_unconditionally(|_| {
let tcx = self.tcx();

Expand Down Expand Up @@ -148,19 +148,13 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
&mut obligations,
);

obligations.extend(
obligations.extend(self.infcx.commit_if_ok(|_| {
self.infcx
.at(&obligation.cause, obligation.param_env)
.sup(placeholder_trait_predicate.trait_ref.to_poly_trait_ref(), candidate)
.map(|InferOk { obligations, .. }| obligations)
.unwrap_or_else(|_| {
bug!(
"Projection bound `{:?}` was applicable to `{:?}` but now is not",
candidate,
obligation
);
}),
);
.map_err(|_| Unimplemented)
})?);

if let ty::Projection(..) = placeholder_self_ty.kind() {
for predicate in tcx.predicates_of(def_id).instantiate_own(tcx, substs).predicates {
Expand All @@ -181,7 +175,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
}
}

obligations
Ok(obligations)
})
}

Expand Down Expand Up @@ -371,9 +365,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
fn confirm_object_candidate(
&mut self,
obligation: &TraitObligation<'tcx>,
) -> ImplSourceObjectData<'tcx, PredicateObligation<'tcx>> {
debug!(?obligation, "confirm_object_candidate");
index: usize,
) -> Result<ImplSourceObjectData<'tcx, PredicateObligation<'tcx>>, SelectionError<'tcx>> {
let tcx = self.tcx();
debug!(?obligation, ?index, "confirm_object_candidate");

let trait_predicate =
self.infcx.replace_bound_vars_with_placeholders(&obligation.predicate);
Expand All @@ -399,43 +394,39 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
})
.with_self_ty(self.tcx(), self_ty);

let mut upcast_trait_ref = None;
let mut nested = vec![];
let vtable_base;

{
// We want to find the first supertrait in the list of
// supertraits that we can unify with, and do that
// unification. We know that there is exactly one in the list
// where we can unify, because otherwise select would have
// reported an ambiguity. (When we do find a match, also
// record it for later.)
let nonmatching = util::supertraits(tcx, ty::Binder::dummy(object_trait_ref))
.take_while(|&t| {
match self.infcx.commit_if_ok(|_| {
self.infcx
.at(&obligation.cause, obligation.param_env)
.sup(obligation_trait_ref, t)
.map(|InferOk { obligations, .. }| obligations)
.map_err(|_| ())
}) {
Ok(obligations) => {
upcast_trait_ref = Some(t);
nested.extend(obligations);
false
}
Err(_) => true,
}
});
let mut supertraits = util::supertraits(tcx, ty::Binder::dummy(object_trait_ref));

// Additionally, for each of the non-matching predicates that
// we pass over, we sum up the set of number of vtable
// entries, so that we can compute the offset for the selected
// trait.
vtable_base = nonmatching.map(|t| super::util::count_own_vtable_entries(tcx, t)).sum();
}
// For each of the non-matching predicates that
// we pass over, we sum up the set of number of vtable
// entries, so that we can compute the offset for the selected
// trait.
let vtable_base = supertraits
.by_ref()
.take(index)
.map(|t| super::util::count_own_vtable_entries(tcx, t))
.sum();

let unnormalized_upcast_trait_ref =
supertraits.next().expect("supertraits iterator no longer has as many elements");

let upcast_trait_ref = normalize_with_depth_to(
self,
obligation.param_env,
obligation.cause.clone(),
obligation.recursion_depth + 1,
&unnormalized_upcast_trait_ref,
&mut nested,
);

let upcast_trait_ref = upcast_trait_ref.unwrap();
nested.extend(self.infcx.commit_if_ok(|_| {
self.infcx
.at(&obligation.cause, obligation.param_env)
.sup(obligation_trait_ref, upcast_trait_ref)
.map(|InferOk { obligations, .. }| obligations)
.map_err(|_| Unimplemented)
})?);

// Check supertraits hold. This is so that their associated type bounds
// will be checked in the code below.
Expand Down Expand Up @@ -501,7 +492,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
}

debug!(?nested, "object nested obligations");
ImplSourceObjectData { upcast_trait_ref, vtable_base, nested }
Ok(ImplSourceObjectData { upcast_trait_ref, vtable_base, nested })
}

fn confirm_fn_pointer_candidate(
Expand Down
35 changes: 14 additions & 21 deletions compiler/rustc_trait_selection/src/traits/select/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -518,12 +518,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
result
}
Ok(Ok(None)) => Ok(EvaluatedToAmbig),
// EvaluatedToRecur might also be acceptable here, but use
// Unknown for now because it means that we won't dismiss a
// selection candidate solely because it has a projection
// cycle. This is closest to the previous behavior of
// immediately erroring.
Ok(Err(project::InProgress)) => Ok(EvaluatedToUnknown),
Ok(Err(project::InProgress)) => Ok(EvaluatedToRecur),
Err(_) => Ok(EvaluatedToErr),
}
}
Expand Down Expand Up @@ -1179,7 +1174,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
if let ty::PredicateAtom::Trait(pred, _) = bound_predicate.skip_binder() {
let bound = bound_predicate.rebind(pred.trait_ref);
if self.infcx.probe(|_| {
match self.match_projection(
match self.match_normalize_trait_ref(
obligation,
bound,
placeholder_trait_predicate.trait_ref,
Expand Down Expand Up @@ -1207,7 +1202,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
/// Equates the trait in `obligation` with trait bound. If the two traits
/// can be equated and the normalized trait bound doesn't contain inference
/// variables or placeholders, the normalized bound is returned.
fn match_projection(
fn match_normalize_trait_ref(
&mut self,
obligation: &TraitObligation<'tcx>,
trait_bound: ty::PolyTraitRef<'tcx>,
Expand Down Expand Up @@ -1357,10 +1352,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
| BuiltinUnsizeCandidate
| BuiltinCandidate { .. }
| TraitAliasCandidate(..)
| ObjectCandidate
| ObjectCandidate(_)
| ProjectionCandidate(_),
) => !is_global(cand),
(ObjectCandidate | ProjectionCandidate(_), ParamCandidate(ref cand)) => {
(ObjectCandidate(_) | ProjectionCandidate(_), ParamCandidate(ref cand)) => {
// Prefer these to a global where-clause bound
// (see issue #50825).
is_global(cand)
Expand All @@ -1381,20 +1376,20 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
is_global(cand) && other.evaluation.must_apply_modulo_regions()
}

(ProjectionCandidate(i), ProjectionCandidate(j)) => {
// Arbitrarily pick the first candidate for backwards
(ProjectionCandidate(i), ProjectionCandidate(j))
| (ObjectCandidate(i), ObjectCandidate(j)) => {
// Arbitrarily pick the lower numbered candidate for backwards
// compatibility reasons. Don't let this affect inference.
i > j && !needs_infer
i < j && !needs_infer
}
(ObjectCandidate, ObjectCandidate) => bug!("Duplicate object candidate"),
(ObjectCandidate, ProjectionCandidate(_))
| (ProjectionCandidate(_), ObjectCandidate) => {
(ObjectCandidate(_), ProjectionCandidate(_))
| (ProjectionCandidate(_), ObjectCandidate(_)) => {
bug!("Have both object and projection candidate")
}

// Arbitrarily give projection and object candidates priority.
(
ObjectCandidate | ProjectionCandidate(_),
ObjectCandidate(_) | ProjectionCandidate(_),
ImplCandidate(..)
| ClosureCandidate
| GeneratorCandidate
Expand All @@ -1414,7 +1409,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
| BuiltinUnsizeCandidate
| BuiltinCandidate { .. }
| TraitAliasCandidate(..),
ObjectCandidate | ProjectionCandidate(_),
ObjectCandidate(_) | ProjectionCandidate(_),
) => false,

(&ImplCandidate(other_def), &ImplCandidate(victim_def)) => {
Expand Down Expand Up @@ -1890,9 +1885,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {

/// Normalize `where_clause_trait_ref` and try to match it against
/// `obligation`. If successful, return any predicates that
/// result from the normalization. Normalization is necessary
/// because where-clauses are stored in the parameter environment
/// unnormalized.
/// result from the normalization.
fn match_where_clause_trait_ref(
&mut self,
obligation: &TraitObligation<'tcx>,
Expand Down
25 changes: 25 additions & 0 deletions src/test/ui/associated-types/normalization-probe-cycle.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Regression test for #77656

// check-pass

trait Value: PartialOrd {}

impl<T: PartialOrd> Value for T {}

trait Distance
where
Self: PartialOrd<<Self as Distance>::Value>,
Self: PartialOrd,
{
type Value: Value;
}

impl<T: Value> Distance for T {
type Value = T;
}

trait Proximity<T = Self> {
type Distance: Distance;
}

fn main() {}
37 changes: 37 additions & 0 deletions src/test/ui/traits/normalize-super-trait.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Regression test for #77653
// When monomorphizing `f` we need to prove `dyn Derived<()>: Base<()>`. This
// requires us to normalize the `Base<<() as Proj>::S>` to `Base<()>` when
// comparing the supertrait `Derived<()>` to the expected trait.

// build-pass

trait Proj {
type S;
}

impl Proj for () {
type S = ();
}

impl Proj for i32 {
type S = i32;
}

trait Base<T> {
fn is_base(&self);
}

trait Derived<B: Proj>: Base<B::S> + Base<()> {
fn is_derived(&self);
}

fn f<P: Proj>(obj: &dyn Derived<P>) {
obj.is_derived();
Base::<P::S>::is_base(obj);
Base::<()>::is_base(obj);
}

fn main() {
let x: fn(_) = f::<()>;
let x: fn(_) = f::<i32>;
}

0 comments on commit a9cd294

Please sign in to comment.