Rollup merge of rust-lang#109724 - lcnr:prioritize-env, r=compiler-errors

prioritize param env candidates if they don't guide type inference

intended to supersede rust-lang#109579. We disable the prioritization during coherence to maintain completeness.

Long term we can hopefully replace this hack with adding OR to external constraints at which point the only relevant part when merging responses is whether they guide type inference in the same way.

Reuses `try_merge_responses` for assembly and the cleanest way to impl that was to actually split that so that `try_merge_responses` returns `None` if we fail to merge them and then add `flounder` which is used afterwards which is allowed to lower the certainty of our responses.

If, in the future, we add the ability to merge candidates `YES: ?0 = Vec<u32>` and `YES: ?0 = Vec<i32>` to `AMBIG: ?0 = Vec<?1>`, this should happen in `flounder`.

r? `@compiler-errors` `@BoxyUwU`

Author: Dylan-DPC

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

compiler/rustc_middle/src/traits/solve.rs

@@ -56,9 +56,19 @@ pub enum Certainty {
 impl Certainty {
     pub const AMBIGUOUS: Certainty = Certainty::Maybe(MaybeCause::Ambiguity);
 
-    /// When proving multiple goals using **AND**, e.g. nested obligations for an impl,
-    /// use this function to unify the certainty of these goals
-    pub fn unify_and(self, other: Certainty) -> Certainty {
+    /// Use this function to merge the certainty of multiple nested subgoals.
+    ///
+    /// Given an impl like `impl<T: Foo + Bar> Baz for T {}`, we have 2 nested
+    /// subgoals whenever we use the impl as a candidate: `T: Foo` and `T: Bar`.
+    /// If evaluating `T: Foo` results in ambiguity and `T: Bar` results in
+    /// success, we merge these two responses. This results in ambiguity.
+    ///
+    /// If we unify ambiguity with overflow, we return overflow. This doesn't matter
+    /// inside of the solver as we distinguish ambiguity from overflow. It does
+    /// however matter for diagnostics. If `T: Foo` resulted in overflow and `T: Bar`
+    /// in ambiguity without changing the inference state, we still want to tell the
+    /// user that `T: Baz` results in overflow.
+    pub fn unify_with(self, other: Certainty) -> Certainty {
         match (self, other) {
             (Certainty::Yes, Certainty::Yes) => Certainty::Yes,
             (Certainty::Yes, Certainty::Maybe(_)) => other,

compiler/rustc_trait_selection/src/solve/assembly.rs renamed to compiler/rustc_trait_selection/src/solve/assembly/mod.rs

@@ -1,11 +1,9 @@
 //! Code shared by trait and projection goals for candidate assembly.
 
 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;
@@ -16,6 +14,8 @@ use rustc_middle::ty::TypeFoldable;
 use rustc_middle::ty::{self, Ty, TyCtxt};
 use std::fmt::Debug;
 
+pub(super) mod structural_traits;
+
 /// A candidate is a possible way to prove a goal.
 ///
 /// It consists of both the `source`, which describes how that goal would be proven,
@@ -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)
     }
 }

compiler/rustc_trait_selection/src/solve/trait_goals/structural_traits.rs renamed to compiler/rustc_trait_selection/src/solve/assembly/structural_traits.rs

@@ -11,7 +11,7 @@ use crate::solve::EvalCtxt;
 //
 // For types with an "existential" binder, i.e. generator witnesses, we also
 // instantiate the binder with placeholders eagerly.
-pub(super) fn instantiate_constituent_tys_for_auto_trait<'tcx>(
+pub(in crate::solve) fn instantiate_constituent_tys_for_auto_trait<'tcx>(
     ecx: &EvalCtxt<'_, 'tcx>,
     ty: Ty<'tcx>,
 ) -> Result<Vec<Ty<'tcx>>, NoSolution> {
@@ -87,7 +87,7 @@ pub(super) fn instantiate_constituent_tys_for_auto_trait<'tcx>(
     }
 }
 
-fn replace_erased_lifetimes_with_bound_vars<'tcx>(
+pub(in crate::solve) fn replace_erased_lifetimes_with_bound_vars<'tcx>(
     tcx: TyCtxt<'tcx>,
     ty: Ty<'tcx>,
 ) -> ty::Binder<'tcx, Ty<'tcx>> {
@@ -108,7 +108,7 @@ fn replace_erased_lifetimes_with_bound_vars<'tcx>(
     ty::Binder::bind_with_vars(ty, bound_vars)
 }
 
-pub(super) fn instantiate_constituent_tys_for_sized_trait<'tcx>(
+pub(in crate::solve) fn instantiate_constituent_tys_for_sized_trait<'tcx>(
     ecx: &EvalCtxt<'_, 'tcx>,
     ty: Ty<'tcx>,
 ) -> Result<Vec<Ty<'tcx>>, NoSolution> {
@@ -158,7 +158,7 @@ pub(super) fn instantiate_constituent_tys_for_sized_trait<'tcx>(
     }
 }
 
-pub(super) fn instantiate_constituent_tys_for_copy_clone_trait<'tcx>(
+pub(in crate::solve) fn instantiate_constituent_tys_for_copy_clone_trait<'tcx>(
     ecx: &EvalCtxt<'_, 'tcx>,
     ty: Ty<'tcx>,
 ) -> Result<Vec<Ty<'tcx>>, NoSolution> {
@@ -224,7 +224,7 @@ pub(super) fn instantiate_constituent_tys_for_copy_clone_trait<'tcx>(
 }
 
 // Returns a binder of the tupled inputs types and output type from a builtin callable type.
-pub(crate) fn extract_tupled_inputs_and_output_from_callable<'tcx>(
+pub(in crate::solve) fn extract_tupled_inputs_and_output_from_callable<'tcx>(
     tcx: TyCtxt<'tcx>,
     self_ty: Ty<'tcx>,
     goal_kind: ty::ClosureKind,
@@ -337,7 +337,13 @@ pub(crate) fn extract_tupled_inputs_and_output_from_callable<'tcx>(
 /// additional step of eagerly folding the associated types in the where
 /// clauses of the impl. In this example, that means replacing
 /// `<Self as Foo>::Bar` with `Ty` in the first impl.
-pub(crate) fn predicates_for_object_candidate<'tcx>(
+///
+// FIXME: This is only necessary as `<Self as Trait>::Assoc: ItemBound`
+// bounds in impls are trivially proven using the item bound candidates.
+// This is unsound in general and once that is fixed, we don't need to
+// normalize eagerly here. See https://github.com/lcnr/solver-woes/issues/9
+// for more details.
+pub(in crate::solve) fn predicates_for_object_candidate<'tcx>(
     ecx: &EvalCtxt<'_, 'tcx>,
     param_env: ty::ParamEnv<'tcx>,
     trait_ref: ty::TraitRef<'tcx>,

compiler/rustc_trait_selection/src/solve/eval_ctxt.rs

@@ -357,7 +357,7 @@ impl<'a, 'tcx> EvalCtxt<'a, 'tcx> {
                             // deal with `has_changed` in the next iteration.
                             new_goals.normalizes_to_hack_goal =
                                 Some(this.resolve_vars_if_possible(goal));
-                            has_changed = has_changed.map_err(|c| c.unify_and(certainty));
+                            has_changed = has_changed.map_err(|c| c.unify_with(certainty));
                         }
                     }
                 }
@@ -378,7 +378,7 @@ impl<'a, 'tcx> EvalCtxt<'a, 'tcx> {
                         Certainty::Yes => {}
                         Certainty::Maybe(_) => {
                             new_goals.goals.push(goal);
-                            has_changed = has_changed.map_err(|c| c.unify_and(certainty));
+                            has_changed = has_changed.map_err(|c| c.unify_with(certainty));
                         }
                     }
                 }

compiler/rustc_trait_selection/src/solve/eval_ctxt/canonical.rs

@@ -50,7 +50,7 @@ impl<'tcx> EvalCtxt<'_, 'tcx> {
         certainty: Certainty,
     ) -> QueryResult<'tcx> {
         let goals_certainty = self.try_evaluate_added_goals()?;
-        let certainty = certainty.unify_and(goals_certainty);
+        let certainty = certainty.unify_with(goals_certainty);
 
         let external_constraints = self.compute_external_query_constraints()?;
 

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| {
-            certainty.unify_and(response.value.certainty)
+    /// 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_with(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
     }
 }
 

compiler/rustc_trait_selection/src/solve/project_goals.rs

@@ -1,7 +1,6 @@
 use crate::traits::specialization_graph;
 
-use super::assembly;
-use super::trait_goals::structural_traits;
+use super::assembly::{self, structural_traits};
 use super::EvalCtxt;
 use rustc_errors::ErrorGuaranteed;
 use rustc_hir::def::DefKind;