prioritize param-env candidates

lcnr · lcnr · commit 2b0f5721c190 · 2023-04-10T09:16:33.000+02:00
diff --git a/compiler/rustc_middle/src/infer/canonical.rs b/compiler/rustc_middle/src/infer/canonical.rs
@@ -80,6 +80,18 @@ impl CanonicalVarValues<'_> {
             }
         })
     }
+
+    pub fn is_identity_modulo_regions(&self) -> bool {
+        self.var_values.iter().enumerate().all(|(bv, arg)| match arg.unpack() {
+            ty::GenericArgKind::Lifetime(_) => true,
+            ty::GenericArgKind::Type(ty) => {
+                matches!(*ty.kind(), ty::Bound(ty::INNERMOST, bt) if bt.var.as_usize() == bv)
+            }
+            ty::GenericArgKind::Const(ct) => {
+                matches!(ct.kind(), ty::ConstKind::Bound(ty::INNERMOST, bc) if bc.as_usize() == bv)
+            }
+        })
+    }
 }
 
 /// When we canonicalize a value to form a query, we wind up replacing
diff --git a/compiler/rustc_trait_selection/src/solve/assembly.rs b/compiler/rustc_trait_selection/src/solve/assembly.rs
@@ -4,8 +4,8 @@ use super::search_graph::OverflowHandler;
 #[cfg(doc)]
 use super::trait_goals::structural_traits::*;
 use super::{EvalCtxt, SolverMode};
+use crate::solve::CanonicalResponseExt;
 use crate::traits::coherence;
-use itertools::Itertools;
 use rustc_data_structures::fx::FxIndexSet;
 use rustc_hir::def_id::DefId;
 use rustc_infer::traits::query::NoSolution;
@@ -547,61 +547,41 @@ impl<'tcx> EvalCtxt<'_, 'tcx> {
         }
     }
 
+    /// If there are multiple ways to prove a trait or projection goal, we have
+    /// to somehow try to merge the candidates into one. If that fails, we return
+    /// ambiguity.
     #[instrument(level = "debug", skip(self), ret)]
     pub(super) fn merge_candidates(
         &mut self,
         mut candidates: Vec<Candidate<'tcx>>,
     ) -> QueryResult<'tcx> {
-        match candidates.len() {
-            0 => return Err(NoSolution),
-            1 => return Ok(candidates.pop().unwrap().result),
-            _ => {}
+        // First try merging all candidates. This is complete and fully sound.
+        let responses = candidates.iter().map(|c| c.result).collect::<Vec<_>>();
+        if let Some(result) = self.try_merge_responses(&responses) {
+            return Ok(result);
         }
 
-        if candidates.len() > 1 {
-            let mut i = 0;
-            'outer: while i < candidates.len() {
-                for j in (0..candidates.len()).filter(|&j| i != j) {
-                    if self.candidate_should_be_dropped_in_favor_of(&candidates[i], &candidates[j])
-                    {
-                        debug!(candidate = ?candidates[i], "Dropping candidate #{}/{}", i, candidates.len());
-                        candidates.swap_remove(i);
-                        continue 'outer;
+        // We then check whether we should prioritize `ParamEnv` candidates.
+        //
+        // Doing so is incomplete and would therefore be unsound during coherence.
+        match self.solver_mode() {
+            SolverMode::Coherence => (),
+            // Prioritize `ParamEnv` candidates only if they do not guide inference.
+            //
+            // This is still incomplete as we may add incorrect region bounds.
+            SolverMode::Normal => {
+                let param_env_responses = candidates
+                    .iter()
+                    .filter(|c| matches!(c.source, CandidateSource::ParamEnv(_)))
+                    .map(|c| c.result)
+                    .collect::<Vec<_>>();
+                if let Some(result) = self.try_merge_responses(&param_env_responses) {
+                    if result.has_only_region_constraints() {
+                        return Ok(result);
                     }
                 }
-
-                debug!(candidate = ?candidates[i], "Retaining candidate #{}/{}", i, candidates.len());
-                i += 1;
-            }
-
-            // If there are *STILL* multiple candidates that have *different* response
-            // results, give up and report ambiguity.
-            if candidates.len() > 1 && !candidates.iter().map(|cand| cand.result).all_equal() {
-                let certainty = if candidates.iter().all(|x| {
-                    matches!(x.result.value.certainty, Certainty::Maybe(MaybeCause::Overflow))
-                }) {
-                    Certainty::Maybe(MaybeCause::Overflow)
-                } else {
-                    Certainty::AMBIGUOUS
-                };
-                return self.evaluate_added_goals_and_make_canonical_response(certainty);
             }
         }
-
-        Ok(candidates.pop().unwrap().result)
-    }
-
-    fn candidate_should_be_dropped_in_favor_of(
-        &self,
-        candidate: &Candidate<'tcx>,
-        other: &Candidate<'tcx>,
-    ) -> bool {
-        // FIXME: implement this
-        match (candidate.source, other.source) {
-            (CandidateSource::Impl(_), _)
-            | (CandidateSource::ParamEnv(_), _)
-            | (CandidateSource::AliasBound, _)
-            | (CandidateSource::BuiltinImpl, _) => false,
-        }
+        self.flounder(&responses)
     }
 }
diff --git a/compiler/rustc_trait_selection/src/solve/mod.rs b/compiler/rustc_trait_selection/src/solve/mod.rs
@@ -46,6 +46,8 @@ enum SolverMode {
 
 trait CanonicalResponseExt {
     fn has_no_inference_or_external_constraints(&self) -> bool;
+
+    fn has_only_region_constraints(&self) -> bool;
 }
 
 impl<'tcx> CanonicalResponseExt for Canonical<'tcx, Response<'tcx>> {
@@ -54,6 +56,11 @@ impl<'tcx> CanonicalResponseExt for Canonical<'tcx, Response<'tcx>> {
             && self.value.var_values.is_identity()
             && self.value.external_constraints.opaque_types.is_empty()
     }
+
+    fn has_only_region_constraints(&self) -> bool {
+        self.value.var_values.is_identity_modulo_regions()
+            && self.value.external_constraints.opaque_types.is_empty()
+    }
 }
 
 impl<'a, 'tcx> EvalCtxt<'a, 'tcx> {
@@ -221,12 +228,17 @@ impl<'a, 'tcx> EvalCtxt<'a, 'tcx> {
             (Some(alias_lhs), Some(alias_rhs)) => {
                 debug!("both sides are aliases");
 
-                let candidates = vec![
-                    // LHS normalizes-to RHS
-                    evaluate_normalizes_to(self, alias_lhs, rhs, direction, Invert::No),
-                    // RHS normalizes-to RHS
-                    evaluate_normalizes_to(self, alias_rhs, lhs, direction, Invert::Yes),
-                    // Relate via substs
+                let mut candidates = Vec::new();
+                // LHS normalizes-to RHS
+                candidates.extend(
+                    evaluate_normalizes_to(self, alias_lhs, rhs, direction, Invert::No).ok(),
+                );
+                // RHS normalizes-to RHS
+                candidates.extend(
+                    evaluate_normalizes_to(self, alias_rhs, lhs, direction, Invert::Yes).ok(),
+                );
+                // Relate via substs
+                candidates.extend(
                     self.probe(|ecx| {
                         let span = tracing::span!(
                             tracing::Level::DEBUG,
@@ -247,11 +259,16 @@ impl<'a, 'tcx> EvalCtxt<'a, 'tcx> {
                         }
 
                         ecx.evaluate_added_goals_and_make_canonical_response(Certainty::Yes)
-                    }),
-                ];
+                    })
+                    .ok(),
+                );
                 debug!(?candidates);
 
-                self.try_merge_responses(candidates.into_iter())
+                if let Some(merged) = self.try_merge_responses(&candidates) {
+                    Ok(merged)
+                } else {
+                    self.flounder(&candidates)
+                }
             }
         }
     }
@@ -289,43 +306,51 @@ impl<'tcx> EvalCtxt<'_, 'tcx> {
         debug!("added_goals={:?}", &self.nested_goals.goals[current_len..]);
     }
 
-    #[instrument(level = "debug", skip(self, responses))]
+    /// Try to merge multiple possible ways to prove a goal, if that is not possible returns `None`.
+    ///
+    /// In this case we tend to flounder and return ambiguity by calling `[EvalCtxt::flounder]`.
+    #[instrument(level = "debug", skip(self), ret)]
     fn try_merge_responses(
         &mut self,
-        responses: impl Iterator<Item = QueryResult<'tcx>>,
-    ) -> QueryResult<'tcx> {
-        let candidates = responses.into_iter().flatten().collect::<Box<[_]>>();
-
-        if candidates.is_empty() {
-            return Err(NoSolution);
+        responses: &[CanonicalResponse<'tcx>],
+    ) -> Option<CanonicalResponse<'tcx>> {
+        if responses.is_empty() {
+            return None;
         }
 
         // FIXME(-Ztrait-solver=next): We should instead try to find a `Certainty::Yes` response with
         // a subset of the constraints that all the other responses have.
-        let one = candidates[0];
-        if candidates[1..].iter().all(|resp| resp == &one) {
-            return Ok(one);
+        let one = responses[0];
+        if responses[1..].iter().all(|&resp| resp == one) {
+            return Some(one);
         }
 
-        if let Some(response) = candidates.iter().find(|response| {
-            response.value.certainty == Certainty::Yes
-                && response.has_no_inference_or_external_constraints()
-        }) {
-            return Ok(*response);
-        }
+        responses
+            .iter()
+            .find(|response| {
+                response.value.certainty == Certainty::Yes
+                    && response.has_no_inference_or_external_constraints()
+            })
+            .copied()
+    }
 
-        let certainty = candidates.iter().fold(Certainty::AMBIGUOUS, |certainty, response| {
+    /// If we fail to merge responses we flounder and return overflow or ambiguity.
+    #[instrument(level = "debug", skip(self), ret)]
+    fn flounder(&mut self, responses: &[CanonicalResponse<'tcx>]) -> QueryResult<'tcx> {
+        if responses.is_empty() {
+            return Err(NoSolution);
+        }
+        let certainty = responses.iter().fold(Certainty::AMBIGUOUS, |certainty, response| {
             certainty.unify_and(response.value.certainty)
         });
-        // FIXME(-Ztrait-solver=next): We should take the intersection of the constraints on all the
-        // responses and use that for the constraints of this ambiguous response.
-        debug!(">1 response, bailing with {certainty:?}");
+
         let response = self.evaluate_added_goals_and_make_canonical_response(certainty);
-        if let Ok(response) = &response {
+        if let Ok(response) = response {
             assert!(response.has_no_inference_or_external_constraints());
+            Ok(response)
+        } else {
+            bug!("failed to make floundered response: {responses:?}");
         }
-
-        response
     }
 }
 
diff --git a/tests/ui/traits/new-solver/alias_eq_dont_use_normalizes_to_if_substs_eq.rs b/tests/ui/traits/new-solver/alias_eq_dont_use_normalizes_to_if_substs_eq.rs
@@ -1,7 +1,7 @@
 // compile-flags: -Ztrait-solver=next
 
 // check that when computing `alias-eq(<() as Foo<u16, T>>::Assoc, <() as Foo<?0, T>>::Assoc)`
-//  we do not infer `?0 = u8` via the `for<STOP> (): Foo<u8, STOP>` impl or `?0 = u16` by
+// we do not infer `?0 = u8` via the `for<STOP> (): Foo<u8, STOP>` impl or `?0 = u16` by
 // relating substs as either could be a valid solution.
 
 trait Foo<T, STOP> {
diff --git a/tests/ui/traits/new-solver/prefer-candidate-no-constraints.rs b/tests/ui/traits/new-solver/prefer-candidate-no-constraints.rs
@@ -0,0 +1,22 @@
+// compile-flags: -Ztrait-solver=next
+// check-pass
+
+trait Foo {}
+
+impl<T> Foo for T {}
+
+trait Bar {}
+
+struct Wrapper<'a, T>(&'a T);
+
+impl<'a, T> Bar for Wrapper<'a, T> where &'a T: Foo {}
+// We need to satisfy `&'a T: Foo` when checking that this impl is WF
+// that can either be satisfied via the param-env, or via an impl.
+//
+// When satisfied via the param-env, since each lifetime is canonicalized
+// separately, we end up getting extra region constraints.
+//
+// However, when satisfied via the impl, there are no region constraints,
+// and we can short-circuit a response with no external constraints.
+
+fn main() {}
diff --git a/tests/ui/traits/new-solver/prefer-param-env-on-ambiguity.rs b/tests/ui/traits/new-solver/prefer-param-env-on-ambiguity.rs
@@ -0,0 +1,10 @@
+// compile-flags: -Ztrait-solver=next
+// check-pass
+
+trait Foo<'a> {}
+trait Bar<'a> {}
+
+impl<'a, T: Bar<'a>> Foo<'a> for T {}
+impl<T> Bar<'static> for T {}
+
+fn main() {}
diff --git a/tests/ui/traits/new-solver/recursive-self-normalization-2.stderr b/tests/ui/traits/new-solver/recursive-self-normalization-2.stderr
@@ -1,9 +1,16 @@
-error[E0282]: type annotations needed
+error[E0283]: type annotations needed: cannot satisfy `<T as Foo1>::Assoc1: Bar`
   --> $DIR/recursive-self-normalization-2.rs:15:5
    |
 LL |     needs_bar::<T::Assoc1>();
-   |     ^^^^^^^^^^^^^^^^^^^^^^ cannot infer type of the type parameter `S` declared on the function `needs_bar`
+   |     ^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: cannot satisfy `<T as Foo1>::Assoc1: Bar`
+note: required by a bound in `needs_bar`
+  --> $DIR/recursive-self-normalization-2.rs:12:17
+   |
+LL | fn needs_bar<S: Bar>() {}
+   |                 ^^^ required by this bound in `needs_bar`
 
 error: aborting due to previous error
 
-For more information about this error, try `rustc --explain E0282`.
+For more information about this error, try `rustc --explain E0283`.
diff --git a/tests/ui/traits/new-solver/recursive-self-normalization.stderr b/tests/ui/traits/new-solver/recursive-self-normalization.stderr
@@ -1,9 +1,16 @@
-error[E0282]: type annotations needed
+error[E0283]: type annotations needed: cannot satisfy `<T as Foo>::Assoc: Bar`
   --> $DIR/recursive-self-normalization.rs:11:5
    |
 LL |     needs_bar::<T::Assoc>();
-   |     ^^^^^^^^^^^^^^^^^^^^^ cannot infer type of the type parameter `S` declared on the function `needs_bar`
+   |     ^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: cannot satisfy `<T as Foo>::Assoc: Bar`
+note: required by a bound in `needs_bar`
+  --> $DIR/recursive-self-normalization.rs:8:17
+   |
+LL | fn needs_bar<S: Bar>() {}
+   |                 ^^^ required by this bound in `needs_bar`
 
 error: aborting due to previous error
 
-For more information about this error, try `rustc --explain E0282`.
+For more information about this error, try `rustc --explain E0283`.

Original file line number	Diff line number	Diff line change
`@@ -80,6 +80,18 @@ impl CanonicalVarValues<'_> {`
`80`	`80`	`}`
`81`	`81`	`})`
`82`	`82`	`}`
	`83`	`+`
	`84`	`+ pub fn is_identity_modulo_regions(&self) -> bool {`
	`85`	`+ self.var_values.iter().enumerate().all(\|(bv, arg)\| match arg.unpack() {`
	`86`	`+ ty::GenericArgKind::Lifetime(_) => true,`
	`87`	`+ ty::GenericArgKind::Type(ty) => {`
	`88`	`+ matches!(*ty.kind(), ty::Bound(ty::INNERMOST, bt) if bt.var.as_usize() == bv)`
	`89`	`+ }`
	`90`	`+ ty::GenericArgKind::Const(ct) => {`
	`91`	`+ matches!(ct.kind(), ty::ConstKind::Bound(ty::INNERMOST, bc) if bc.as_usize() == bv)`
	`92`	`+ }`
	`93`	`+ })`
	`94`	`+ }`
`83`	`95`	`}`
`84`	`96`
`85`	`97`	`/// When we canonicalize a value to form a query, we wind up replacing`