Skip to content

Commit

Permalink
more eagerly discard constraints on overflow
Browse files Browse the repository at this point in the history
  • Loading branch information
lcnr committed Sep 12, 2024
1 parent 7c7372b commit 675c99f
Show file tree
Hide file tree
Showing 5 changed files with 27 additions and 35 deletions.
15 changes: 15 additions & 0 deletions compiler/rustc_next_trait_solver/src/solve/eval_ctxt/canonical.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
33 changes: 7 additions & 26 deletions compiler/rustc_next_trait_solver/src/solve/eval_ctxt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -370,20 +370,19 @@ 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);
}
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.
Expand All @@ -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<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();
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/solve/fulfill.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand Down
10 changes: 2 additions & 8 deletions compiler/rustc_type_ir/src/solve/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@ impl<I: Interner, P> Goal<I, P> {
/// 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 {
Expand All @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions tests/ui/traits/next-solver/issue-118950-root-region.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit 675c99f

Please sign in to comment.