From 6c43a649313a2a9d03923598c6a5b260624e8f66 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Thu, 8 Oct 2020 21:49:36 +0100 Subject: [PATCH 1/2] Fix ICE from projection cycle Cycles in normalization can cause evaluations to change from Unknown to Err. This means that some selection that were applicable no longer are. To avoid this: * Selection candidates that are known to be applicable are prefered over candidates that are not. * We don't ICE if a candidate is no longer applicable. --- .../src/traits/select/confirmation.rs | 18 +++++-------- .../src/traits/select/mod.rs | 11 +++----- .../normalization-probe-cycle.rs | 25 +++++++++++++++++++ 3 files changed, 34 insertions(+), 20 deletions(-) create mode 100644 src/test/ui/associated-types/normalization-probe-cycle.rs diff --git a/compiler/rustc_trait_selection/src/traits/select/confirmation.rs b/compiler/rustc_trait_selection/src/traits/select/confirmation.rs index 37d619d594215..7fd588ffce30c 100644 --- a/compiler/rustc_trait_selection/src/traits/select/confirmation.rs +++ b/compiler/rustc_trait_selection/src/traits/select/confirmation.rs @@ -69,7 +69,7 @@ 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)) } @@ -120,7 +120,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { &mut self, obligation: &TraitObligation<'tcx>, idx: usize, - ) -> Vec> { + ) -> Result>, SelectionError<'tcx>> { self.infcx.commit_unconditionally(|_| { let tcx = self.tcx(); @@ -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 { @@ -181,7 +175,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { } } - obligations + Ok(obligations) }) } diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index b838602e76ca3..ce7b963473f45 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -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), } } @@ -1382,9 +1377,9 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { } (ProjectionCandidate(i), ProjectionCandidate(j)) => { - // Arbitrarily pick the first candidate for backwards + // 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(_)) diff --git a/src/test/ui/associated-types/normalization-probe-cycle.rs b/src/test/ui/associated-types/normalization-probe-cycle.rs new file mode 100644 index 0000000000000..9c1a488e95175 --- /dev/null +++ b/src/test/ui/associated-types/normalization-probe-cycle.rs @@ -0,0 +1,25 @@ +// Regression test for #77656 + +// check-pass + +trait Value: PartialOrd {} + +impl Value for T {} + +trait Distance +where + Self: PartialOrd<::Value>, + Self: PartialOrd, +{ + type Value: Value; +} + +impl Distance for T { + type Value = T; +} + +trait Proximity { + type Distance: Distance; +} + +fn main() {} From 50dde2e4d842a65f4c04bd8e27626ba6f1656849 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Thu, 8 Oct 2020 21:52:40 +0100 Subject: [PATCH 2/2] Normalize when finding trait object candidates --- compiler/rustc_middle/src/traits/select.rs | 5 +- .../src/traits/select/candidate_assembly.rs | 28 ++++--- .../src/traits/select/confirmation.rs | 79 +++++++++---------- .../src/traits/select/mod.rs | 24 +++--- src/test/ui/traits/normalize-super-trait.rs | 37 +++++++++ 5 files changed, 107 insertions(+), 66 deletions(-) create mode 100644 src/test/ui/traits/normalize-super-trait.rs diff --git a/compiler/rustc_middle/src/traits/select.rs b/compiler/rustc_middle/src/traits/select.rs index 358ead507b4d0..c570ad3273d4e 100644 --- a/compiler/rustc_middle/src/traits/select.rs +++ b/compiler/rustc_middle/src/traits/select.rs @@ -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, diff --git a/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs b/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs index fdf1641c98676..b0bfb4ad17371 100644 --- a/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs +++ b/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs @@ -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`, // but `Foo` is declared as `trait Foo: Bar`. - 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); }) } diff --git a/compiler/rustc_trait_selection/src/traits/select/confirmation.rs b/compiler/rustc_trait_selection/src/traits/select/confirmation.rs index 7fd588ffce30c..872b8e85f563f 100644 --- a/compiler/rustc_trait_selection/src/traits/select/confirmation.rs +++ b/compiler/rustc_trait_selection/src/traits/select/confirmation.rs @@ -73,6 +73,11 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { 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)) @@ -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 @@ -365,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>, SelectionError<'tcx>> { let tcx = self.tcx(); + debug!(?obligation, ?index, "confirm_object_candidate"); let trait_predicate = self.infcx.replace_bound_vars_with_placeholders(&obligation.predicate); @@ -393,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. @@ -495,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( diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index ce7b963473f45..4cc4bc0acdab6 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -1174,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, @@ -1202,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>, @@ -1352,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) @@ -1376,20 +1376,20 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { is_global(cand) && other.evaluation.must_apply_modulo_regions() } - (ProjectionCandidate(i), ProjectionCandidate(j)) => { + (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 } - (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 @@ -1409,7 +1409,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { | BuiltinUnsizeCandidate | BuiltinCandidate { .. } | TraitAliasCandidate(..), - ObjectCandidate | ProjectionCandidate(_), + ObjectCandidate(_) | ProjectionCandidate(_), ) => false, (&ImplCandidate(other_def), &ImplCandidate(victim_def)) => { @@ -1885,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>, diff --git a/src/test/ui/traits/normalize-super-trait.rs b/src/test/ui/traits/normalize-super-trait.rs new file mode 100644 index 0000000000000..021a93eacff1f --- /dev/null +++ b/src/test/ui/traits/normalize-super-trait.rs @@ -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 { + fn is_base(&self); +} + +trait Derived: Base + Base<()> { + fn is_derived(&self); +} + +fn f(obj: &dyn Derived

) { + obj.is_derived(); + Base::::is_base(obj); + Base::<()>::is_base(obj); +} + +fn main() { + let x: fn(_) = f::<()>; + let x: fn(_) = f::; +}