Rollup merge of #130273 - lcnr:overflow-no-constraints, r=compiler-errors

more eagerly discard constraints on overflow

We always discard the results of overflowing goals inside of the trait solver. We previously did so when instantiating the response in `evaluate_goal`. Canonicalizing results only to later discard them is also  inefficient 🤷

It's simpler and nicer to debug to eagerly discard constraints inside of the query itself.

r? ``@compiler-errors``

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

@@ -122,6 +122,21 @@ pub(in crate::solve) fn evaluate_added_goals_and_make_canonical_response(
             (certainty, NestedNormalizationGoals::empty())
         };
 
+        if let Certainty::Maybe(cause @ MaybeCause::Overflow { .. }) = certainty {
+            // If we have overflow, it's probable that we're substituting a type
+            // into itself infinitely and any partial substitutions in the query
+            // response are probably not useful anyways, so just return an empty
+            // query response.
+            //
+            // This may prevent us from potentially useful inference, e.g.
+            // 2 candidates, one ambiguous and one overflow, which both
+            // have the same inference constraints.
+            //
+            // Changing this to retain some constraints in the future
+            // won't be a breaking change, so this is good enough for now.
+            return Ok(self.make_ambiguous_response_no_constraints(cause));
+        }
+
         let external_constraints =
             self.compute_external_query_constraints(certainty, normalization_nested_goals);
         let (var_values, mut external_constraints) = (self.var_values, external_constraints)

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

@@ -17,7 +17,7 @@
 use crate::solve::inspect::{self, ProofTreeBuilder};
 use crate::solve::search_graph::SearchGraph;
 use crate::solve::{
-    CanonicalInput, CanonicalResponse, Certainty, Goal, GoalEvaluationKind, GoalSource, MaybeCause,
+    CanonicalInput, CanonicalResponse, Certainty, Goal, GoalEvaluationKind, GoalSource,
     NestedNormalizationGoals, NoSolution, PredefinedOpaquesData, QueryResult, SolverMode,
     FIXPOINT_STEP_LIMIT,
 };
@@ -370,7 +370,7 @@ pub(super) fn evaluate_goal_raw(
             canonical_goal,
             &mut goal_evaluation,
         );
-        let canonical_response = match canonical_response {
+        let response = match canonical_response {
             Err(e) => {
                 self.inspect.goal_evaluation(goal_evaluation);
                 return Err(e);
@@ -378,12 +378,11 @@ pub(super) fn evaluate_goal_raw(
             Ok(response) => response,
         };
 
-        let (normalization_nested_goals, certainty, has_changed) = self
+        let has_changed = !response.value.var_values.is_identity_modulo_regions()
-            .instantiate_response_discarding_overflow(
+            || !response.value.external_constraints.opaque_types.is_empty();
-                goal.param_env,
+
-                orig_values,
+        let (normalization_nested_goals, certainty) =
-                canonical_response,
+            self.instantiate_and_apply_query_response(goal.param_env, orig_values, response);
-            );
         self.inspect.goal_evaluation(goal_evaluation);
         // FIXME: We previously had an assert here that checked that recomputing
         // a goal after applying its constraints did not change its response.
@@ -398,24 +397,6 @@ pub(super) fn evaluate_goal_raw(
         Ok((normalization_nested_goals, has_changed, certainty))
     }
 
-    fn instantiate_response_discarding_overflow(
-        &mut self,
-        param_env: I::ParamEnv,
-        original_values: Vec<I::GenericArg>,
-        response: CanonicalResponse<I>,
-    ) -> (NestedNormalizationGoals<I>, Certainty, bool) {
-        if let Certainty::Maybe(MaybeCause::Overflow { .. }) = response.value.certainty {
-            return (NestedNormalizationGoals::empty(), response.value.certainty, false);
-        }
-
-        let has_changed = !response.value.var_values.is_identity_modulo_regions()
-            || !response.value.external_constraints.opaque_types.is_empty();
-
-        let (normalization_nested_goals, certainty) =
-            self.instantiate_and_apply_query_response(param_env, original_values, response);
-        (normalization_nested_goals, certainty, has_changed)
-    }
-
     fn compute_goal(&mut self, goal: Goal<I, I::Predicate>) -> QueryResult<I> {
         let Goal { param_env, predicate } = goal;
         let kind = predicate.kind();

compiler/rustc_trait_selection/src/solve/fulfill.rs

@@ -302,7 +302,7 @@ fn fulfillment_error_for_stalled<'tcx>(
             Ok((_, Certainty::Maybe(MaybeCause::Overflow { suggest_increasing_limit }))) => (
                 FulfillmentErrorCode::Ambiguity { overflow: Some(suggest_increasing_limit) },
                 // Don't look into overflows because we treat overflows weirdly anyways.
-                // In `instantiate_response_discarding_overflow` we set `has_changed = false`,
+                // We discard the inference constraints from overflowing goals, so
                 // recomputing the goal again during `find_best_leaf_obligation` may apply
                 // inference guidance that makes other goals go from ambig -> pass, for example.
                 //

compiler/rustc_type_ir/src/solve/mod.rs

@@ -117,7 +117,8 @@ pub fn with<Q>(self, cx: I, predicate: impl Upcast<I, Q>) -> Goal<I, Q> {
 /// Why a specific goal has to be proven.
 ///
 /// This is necessary as we treat nested goals different depending on
-/// their source.
+/// their source. This is currently mostly used by proof tree visitors
+/// but will be used by cycle handling in the future.
 #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "nightly", derive(HashStable_NoContext))]
 pub enum GoalSource {
@@ -126,13 +127,6 @@ pub enum GoalSource {
     ///
     /// FIXME(-Znext-solver=coinductive): Explain how and why this
     /// changes whether cycles are coinductive.
-    ///
-    /// This also impacts whether we erase constraints on overflow.
-    /// Erasing constraints is generally very useful for perf and also
-    /// results in better error messages by avoiding spurious errors.
-    /// We do not erase overflow constraints in `normalizes-to` goals unless
-    /// they are from an impl where-clause. This is necessary due to
-    /// backwards compatibility, cc trait-system-refactor-initiatitive#70.
     ImplWhereBound,
     /// Instantiating a higher-ranked goal and re-proving it.
     InstantiateHigherRanked,

tests/ui/traits/next-solver/issue-118950-root-region.stderr

@@ -37,6 +37,8 @@ LL | impl<T> Overlap<T> for T {}
 LL |
 LL | impl<T> Overlap<for<'a> fn(Assoc<'a, T>)> for T where Missing: Overlap<T> {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `fn(_)`
+   |
+   = note: this behavior recently changed as a result of a bug fix; see rust-lang/rust#56105 for details
 
 error: aborting due to 3 previous errors; 1 warning emitted