Auto merge of #77720 - matthewjasper:fix-trait-ices, r=nikomatsakis

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`

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,
 

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<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);
         })
     }
 

compiler/rustc_trait_selection/src/traits/select/confirmation.rs

@@ -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))
@@ -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
@@ -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();
 
@@ -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)
         })
     }
 
@@ -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);
@@ -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 upcast_trait_ref = upcast_trait_ref.unwrap();
+        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,
+        );
+
+        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.
@@ -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(

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),
                     }
                 }
@@ -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,
@@ -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>,
@@ -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)
@@ -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
@@ -1414,7 +1409,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                 | BuiltinUnsizeCandidate
                 | BuiltinCandidate { .. }
                 | TraitAliasCandidate(..),
-                ObjectCandidate | ProjectionCandidate(_),
+                ObjectCandidate(_) | ProjectionCandidate(_),
             ) => false,
 
             (&ImplCandidate(other_def), &ImplCandidate(victim_def)) => {
@@ -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>,

src/test/ui/associated-types/normalization-probe-cycle.rs

@@ -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() {}

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<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>;
+}