From 675c99f4d54693af8c7af162968838d6e75ffbee Mon Sep 17 00:00:00 2001 From: lcnr Date: Thu, 12 Sep 2024 14:32:44 +0200 Subject: [PATCH] more eagerly discard constraints on overflow --- .../src/solve/eval_ctxt/canonical.rs | 15 +++++++++ .../src/solve/eval_ctxt/mod.rs | 33 ++++--------------- .../src/solve/fulfill.rs | 2 +- compiler/rustc_type_ir/src/solve/mod.rs | 10 ++---- .../issue-118950-root-region.stderr | 2 ++ 5 files changed, 27 insertions(+), 35 deletions(-) diff --git a/compiler/rustc_next_trait_solver/src/solve/eval_ctxt/canonical.rs b/compiler/rustc_next_trait_solver/src/solve/eval_ctxt/canonical.rs index 1c00f5f8b4173..5acfec3dee33c 100644 --- a/compiler/rustc_next_trait_solver/src/solve/eval_ctxt/canonical.rs +++ b/compiler/rustc_next_trait_solver/src/solve/eval_ctxt/canonical.rs @@ -122,6 +122,21 @@ where (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) diff --git a/compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs b/compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs index 3f2f34d3255f7..15b123ebdf934 100644 --- a/compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs +++ b/compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs @@ -17,7 +17,7 @@ use crate::delegate::SolverDelegate; 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 @@ where 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 @@ where Ok(response) => response, }; - let (normalization_nested_goals, certainty, has_changed) = self - .instantiate_response_discarding_overflow( - goal.param_env, - orig_values, - canonical_response, - ); + 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(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 @@ where Ok((normalization_nested_goals, has_changed, certainty)) } - fn instantiate_response_discarding_overflow( - &mut self, - param_env: I::ParamEnv, - original_values: Vec, - response: CanonicalResponse, - ) -> (NestedNormalizationGoals, 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) -> QueryResult { let Goal { param_env, predicate } = goal; let kind = predicate.kind(); diff --git a/compiler/rustc_trait_selection/src/solve/fulfill.rs b/compiler/rustc_trait_selection/src/solve/fulfill.rs index f5f36f40f7e6e..cfc73e2e47e87 100644 --- a/compiler/rustc_trait_selection/src/solve/fulfill.rs +++ b/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. // diff --git a/compiler/rustc_type_ir/src/solve/mod.rs b/compiler/rustc_type_ir/src/solve/mod.rs index 96998d2ec9f2c..a0f7658212f4b 100644 --- a/compiler/rustc_type_ir/src/solve/mod.rs +++ b/compiler/rustc_type_ir/src/solve/mod.rs @@ -117,7 +117,8 @@ impl Goal { /// 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, diff --git a/tests/ui/traits/next-solver/issue-118950-root-region.stderr b/tests/ui/traits/next-solver/issue-118950-root-region.stderr index d14b9d217da23..7c3e22fb4014a 100644 --- a/tests/ui/traits/next-solver/issue-118950-root-region.stderr +++ b/tests/ui/traits/next-solver/issue-118950-root-region.stderr @@ -37,6 +37,8 @@ LL | impl Overlap for T {} LL | LL | impl Overlap fn(Assoc<'a, T>)> for T where Missing: Overlap {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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