do not prefer trivially true candidates with -Znext-solver

compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs

@@ -1339,7 +1339,7 @@ where
                     candidates.retain(|c| matches!(c.source, CandidateSource::ParamEnv(_)));
                 }
 
-                if let Some((response, _)) = self.try_merge_candidates(&candidates) {
+                if let Some(response) = self.try_merge_candidates(&candidates) {
                     Ok(response)
                 } else {
                     self.flounder(&candidates).map_err(Into::into)
@@ -1361,7 +1361,7 @@ where
                 // means we can just ignore inference constraints and don't have to special-case
                 // constraining the normalized-to `term`.
                 self.filter_specialized_impls(AllowInferenceConstraints::Yes, &mut candidates);
-                if let Some((response, _)) = self.try_merge_candidates(&candidates) {
+                if let Some(response) = self.try_merge_candidates(&candidates) {
                     Ok(response)
                 } else {
                     self.flounder(&candidates).map_err(Into::into)

compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs

@@ -18,9 +18,10 @@ use rustc_type_ir::solve::{
 };
 use rustc_type_ir::transmute::Answer;
 use rustc_type_ir::{
-    self as ty, ClauseKind, Const, InferCtxtLike, Interner, MayBeErased, OpaqueTypeKey,
-    PredicateKind, PredicateProxy, Region, RegionVid, TypeFoldable, TypeSuperVisitable,
-    TypeVisitable, TypeVisitableExt, TypeVisitor, TypingMode, max_universe,
+    self as ty, CanonicalQueryInput, ClauseKind, Const, InferCtxtLike, Interner, MayBeErased,
+    OpaqueTypeKey, PredicateKind, PredicateProxy, Region, RegionVid, TypeFoldable,
+    TypeSuperVisitable, TypeVisitable, TypeVisitableExt, TypeVisitor, TypingMode,
+    TypingModeEqWrapper, max_universe,
 };
 use thin_vec::ThinVec;
 use tracing::{Level, debug, instrument, trace, warn};
@@ -583,6 +584,62 @@ where
         )
     }
 
+    /// Whether two canonical responses are exactly equal except for their
+    /// region constraints.
+    pub(super) fn responses_equal_modulo_region_constraints(
+        &self,
+        a: &CanonicalResponse<I>,
+        b: &CanonicalResponse<I>,
+    ) -> bool {
+        if a == b {
+            return true;
+        }
+
+        // We currently only ever use this function if `a` doesn't have region constraints.
+        // This means we only recanonicalize `b`. Could change this if necessary.
+        debug_assert!(a.value.external_constraints.region_constraints.is_empty());
+
+        // This is quite ugly. We instantiate the response as if it were a query input
+        // and then recanonicalize it without its region constraints.
+        //
+        // We can't simply compare all other fields of the response here as there can
+        // be some inference variables and placeholders created inside of a query which
+        // are only mentioned in the region constraints. This then changes the indices
+        // of things canonicalized later.
+        let cx: I = self.cx();
+        let input: CanonicalQueryInput<I, Response<I>> = CanonicalQueryInput {
+            canonical: b.clone(),
+            typing_mode: TypingModeEqWrapper(self.typing_mode()),
+        };
+        let (ref delegate, instantiated_response, _) = D::build_with_canonical(cx, &input);
+        let Response { certainty, var_values, var_values_is_identity, external_constraints } =
+            instantiated_response;
+        let ExternalConstraintsData {
+            region_constraints: _,
+            opaque_types,
+            normalization_nested_goals,
+        } = &*external_constraints;
+        let b_no_regions = canonicalize_response(
+            delegate,
+            ty::UniverseIndex::ROOT,
+            Response {
+                certainty,
+                var_values,
+                var_values_is_identity,
+                external_constraints: self.cx().mk_external_constraints(ExternalConstraintsData {
+                    region_constraints: match cx.assumptions_on_binders() {
+                        true => ExternalRegionConstraints::NextGen(RegionConstraint::new_true()),
+                        false => ExternalRegionConstraints::Old(vec![]),
+                    },
+                    opaque_types: opaque_types.clone(),
+                    normalization_nested_goals: normalization_nested_goals.clone(),
+                }),
+            },
+        );
+
+        a == &b_no_regions
+    }
+
     pub(super) fn ignore_candidate_head_usages(&mut self, usages: CandidateHeadUsages) {
         self.search_graph.ignore_candidate_head_usages(usages);
     }

compiler/rustc_next_trait_solver/src/solve/mod.rs

@@ -82,35 +82,6 @@ fn has_only_region_constraints<I: Interner>(response: ty::Canonical<I, Response<
         && normalization_nested_goals.is_empty()
 }
 
-/// Whether two canonical responses are exactly equal except for their
-/// region constraints.
-fn equal_response_modulo_region_constraints<I: Interner>(
-    a: &CanonicalResponse<I>,
-    b: &CanonicalResponse<I>,
-) -> bool {
-    let CanonicalResponse {
-        max_universe: a_max_universe,
-        var_kinds: a_var_kinds,
-        value:
-            Response {
-                certainty: a_certainty,
-                var_values: a_var_values,
-                var_values_is_identity: _, // ignore because it's purely derived from `var_values`
-                external_constraints: a_external_constraints,
-            },
-    } = a;
-
-    let ExternalConstraintsData { region_constraints: _, opaque_types, normalization_nested_goals } =
-        &**a_external_constraints;
-
-    a_max_universe == &b.max_universe
-        && a_var_kinds == &b.var_kinds
-        && a_var_values == &b.value.var_values
-        && a_certainty == &b.value.certainty
-        && opaque_types == &b.value.external_constraints.opaque_types
-        && normalization_nested_goals == &b.value.external_constraints.normalization_nested_goals
-}
-
 impl<'a, D, I> EvalCtxt<'a, D>
 where
     D: SolverDelegate<Interner = I>,
@@ -310,62 +281,50 @@ where
     }
 }
 
-#[derive(Debug)]
-enum MergeCandidateInfo {
-    AlwaysApplicable(usize),
-    EqualResponse,
-}
-
 impl<D, I> EvalCtxt<'_, D>
 where
     D: SolverDelegate<Interner = I>,
     I: Interner,
 {
     /// Try to merge multiple possible ways to prove a goal, if that is not possible returns `None`.
+    /// We only merge candidates which are equal modulo region constraints. Long-term, we want to
+    /// emit an OR of the constraints of all candidates, for now we fail with ambiguity unless all
+    /// candidates have the same constraints or one candidate has no region constraints at all.
     ///
     /// In this case we tend to flounder and return ambiguity by calling `[EvalCtxt::flounder]`.
     #[instrument(level = "trace", skip(self), ret)]
     fn try_merge_candidates(
         &mut self,
         candidates: &[Candidate<I>],
-    ) -> Option<(CanonicalResponse<I>, MergeCandidateInfo)> {
-        if candidates.is_empty() {
+    ) -> Option<CanonicalResponse<I>> {
+        let Some((first, rest)) = candidates.split_first() else {
             return None;
-        }
-
-        let always_applicable = candidates.iter().enumerate().find(|(_, candidate)| {
-            candidate.result.value.certainty == Certainty::Yes
-                && has_no_inference_or_external_constraints(candidate.result)
-        });
+        };
 
-        if let Some((i, candidate)) = always_applicable {
-            return Some((candidate.result, MergeCandidateInfo::AlwaysApplicable(i)));
+        // If all candidates are equal, just return that result.
+        if rest.iter().all(|c| first.result == c.result) {
+            return Some(first.result);
         }
 
-        let one: CanonicalResponse<I> = candidates[0].result;
-
-        if candidates[1..]
+        // If not, check whether one of the candidates has no region constraints.
+        // Long-term we likely want to also handle cases where one candidate has
+        // strictly weaker region constraints or emit an OR of the region constraints
+        // if candidates are otherwise equal.
+        //
+        // While we could also prefer candidates with weaker non-region constraints,
+        // that's currently not necessary. We intentionally do not prefer candidates
+        // if there's still am ambiguous candidate. We do so to potentially better
+        // handle region uniquification at the start of MIR borrowck, see #163928 for
+        // more details.
+        if let Some(choice) = candidates
             .iter()
-            .all(|candidate| equal_response_modulo_region_constraints(&one, &candidate.result))
+            .find(|c| c.result.value.external_constraints.region_constraints.is_empty())
         {
-            let region_constraints = &one.value.external_constraints.region_constraints;
-            if candidates[1..].iter().all(|candidate| {
-                &candidate.result.value.external_constraints.region_constraints
-                    == region_constraints
-            }) {
-                return Some((one, MergeCandidateInfo::EqualResponse));
-            }
-
-            // If candidates differ only in region constraints, their merged region
-            // constraints are an `Or` of their respective constraints. If one of them
-            // has no region constraints, the `Or` constraint evaluates to `true`.
-            //
-            // This is a special case of `-Zassumptions-on-binders` and should be
-            // replaced eventually.
-            if let Some(candidate) = candidates.iter().find(|candidate| {
-                candidate.result.value.external_constraints.region_constraints.is_empty()
-            }) {
-                return Some((candidate.result, MergeCandidateInfo::EqualResponse));
+            if candidates
+                .iter()
+                .all(|c| self.responses_equal_modulo_region_constraints(&choice.result, &c.result))
+            {
+                return Some(choice.result);
             }
         }
 

compiler/rustc_next_trait_solver/src/solve/trait_goals.rs

@@ -24,7 +24,7 @@ use crate::solve::assembly::{
 use crate::solve::inspect::ProbeKind;
 use crate::solve::{
     BuiltinImplSource, CandidateSource, Certainty, EvalCtxt, Goal, GoalSource, MaybeCause,
-    MergeCandidateInfo, NoSolution, ParamEnvSource, StalledOnCoroutines, flatten_answer_tree,
+    NoSolution, ParamEnvSource, StalledOnCoroutines, flatten_answer_tree,
     has_only_region_constraints,
 };
 
@@ -1600,7 +1600,7 @@ where
             .all(|candidate| matches!(candidate.source, CandidateSource::BuiltinImpl(_)));
 
         if is_marker || all_builtin {
-            self.try_merge_candidates(candidates).map(|(response, _)| response)
+            self.try_merge_candidates(candidates)
         } else if candidates.len() > 1 {
             None
         } else {
@@ -1613,7 +1613,7 @@ where
         candidates: &[Candidate<I>],
         proven_via: TraitGoalProvenVia,
     ) -> (CanonicalResponse<I>, Option<TraitGoalProvenVia>) {
-        if let Some((response, _)) = self.try_merge_candidates(candidates) {
+        if let Some(response) = self.try_merge_candidates(candidates) {
             (response, Some(proven_via))
         } else {
             (self.bail_with_ambiguity(candidates), None)
@@ -1678,6 +1678,7 @@ where
             let alias_bounds: Vec<_> = candidates
                 .extract_if(.., |c| matches!(c.source, CandidateSource::AliasBound(..)))
                 .collect();
+            candidates.into_iter().for_each(|c| self.ignore_candidate_head_usages(c.head_usages));
             return Ok(self.merge_candidates_or_bail_with_ambiguity(
                 &alias_bounds,
                 TraitGoalProvenVia::AliasBound,
@@ -1693,38 +1694,10 @@ where
             let where_bounds: Vec<_> = candidates
                 .extract_if(.., |c| matches!(c.source, CandidateSource::ParamEnv(_)))
                 .collect();
-            let Some((response, info)) = self.try_merge_candidates(&where_bounds) else {
+            candidates.into_iter().for_each(|c| self.ignore_candidate_head_usages(c.head_usages));
+            let Some(response) = self.try_merge_candidates(&where_bounds) else {
                 return Ok((self.bail_with_ambiguity(&where_bounds), None));
             };
-            match info {
-                // If there's an always applicable candidate, the result of all
-                // other candidates does not matter. This means we can ignore
-                // them when checking whether we've reached a fixpoint.
-                //
-                // We always prefer the first always applicable candidate, even if a
-                // later candidate is also always applicable and would result in fewer
-                // reruns. We could slightly improve this by e.g. searching for another
-                // always applicable candidate which doesn't depend on any cycle heads.
-                //
-                // NOTE: This is optimization is observable in case there is an always
-                // applicable global candidate and another non-global candidate which only
-                // applies because of a provisional result. I can't even think of a test
-                // case where this would occur and even then, this would not be unsound.
-                // Supporting this makes the code more involved, so I am just going to
-                // ignore this for now.
-                MergeCandidateInfo::AlwaysApplicable(i) => {
-                    for (j, c) in where_bounds.into_iter().enumerate() {
-                        if i != j {
-                            self.ignore_candidate_head_usages(c.head_usages)
-                        }
-                    }
-                    // If a where-bound does not apply, we don't actually get a
-                    // candidate for it. We manually track the head usages
-                    // of all failed `ParamEnv` candidates instead.
-                    self.ignore_candidate_head_usages(failed_candidate_info.param_env_head_usages);
-                }
-                MergeCandidateInfo::EqualResponse => {}
-            }
             return Ok((response, Some(TraitGoalProvenVia::ParamEnv)));
         }
 
@@ -1733,6 +1706,7 @@ where
             let alias_bounds: Vec<_> = candidates
                 .extract_if(.., |c| matches!(c.source, CandidateSource::AliasBound(_)))
                 .collect();
+            candidates.into_iter().for_each(|c| self.ignore_candidate_head_usages(c.head_usages));
             return Ok(self.merge_candidates_or_bail_with_ambiguity(
                 &alias_bounds,
                 TraitGoalProvenVia::AliasBound,

tests/ui/traits/next-solver/assembly/item-bound-eq-modulo-placeholder-constraint.rs

@@ -0,0 +1,29 @@
+//@ revisions: current next
+//@ ignore-compare-mode-next-solver (explicit revisions)
+//@[next] compile-flags: -Znext-solver
+//@ check-pass
+
+// When proving `C::Val<'a>: for<'b> PartialEq<C::Val<'b>>` there are
+// two candidates:
+// - `C::Val<'a>: PartialEq<C::Val<'a>>`
+// - `for<'b> C::Val<'a>: PartialEq<C::Val<'b>>`
+//
+// The first candidate results in an `'!b == 'a` constraint while the
+// second one has no region constraints. The constraint of the first
+// candidate means the canonical response has an entry for `'b` in its
+// `var_values`. This previously caused us to not prefer the second
+// constraint.
+//
+// FIXME(trait-system-refactor-initiative#305): Now, actually, the first
+// candidate should just result in a leak check failure, but doesn't.
+// That's a separate issue though :>
+trait Cursor {
+    type Val<'a>: PartialEq + for<'b> PartialEq<Self::Val<'b>>;
+}
+impl<C: Cursor> Cursor for &C {
+    // for<'b> C::Val<'a>: PartialEq<C::Val<'b>>
+    // -  C::Val<'a>: PartialEq<C::Val<'a>>`
+    // -  for<'b> C::Val<'a>: PartialEq<C::Val<'b>>
+    type Val<'a> = C::Val<'a>;
+}
+fn main() {}