Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

-Znext-solver=coherence: suggest increasing recursion limit #121497

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions compiler/rustc_infer/src/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,16 +135,18 @@ pub struct FulfillmentError<'tcx> {

#[derive(Clone)]
pub enum FulfillmentErrorCode<'tcx> {
/// Inherently impossible to fulfill; this trait is implemented if and only if it is already implemented.
/// Inherently impossible to fulfill; this trait is implemented if and only
/// if it is already implemented.
Cycle(Vec<PredicateObligation<'tcx>>),
SelectionError(SelectionError<'tcx>),
ProjectionError(MismatchedProjectionTypes<'tcx>),
SubtypeError(ExpectedFound<Ty<'tcx>>, TypeError<'tcx>), // always comes from a SubtypePredicate
ConstEquateError(ExpectedFound<Const<'tcx>>, TypeError<'tcx>),
Ambiguity {
/// Overflow reported from the new solver `-Znext-solver`, which will
/// be reported as an regular error as opposed to a fatal error.
overflow: bool,
/// Overflow is only `Some(suggest_recursion_limit)` when using the next generation
/// trait solver `-Znext-solver`. With the old solver overflow is eagerly handled by
/// emitting a fatal error instead.
overflow: Option<bool>,
compiler-errors marked this conversation as resolved.
Show resolved Hide resolved
},
}

Expand Down
6 changes: 4 additions & 2 deletions compiler/rustc_infer/src/traits/structural_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,10 @@ impl<'tcx> fmt::Debug for traits::FulfillmentErrorCode<'tcx> {
ConstEquateError(ref a, ref b) => {
write!(f, "CodeConstEquateError({a:?}, {b:?})")
}
Ambiguity { overflow: false } => write!(f, "Ambiguity"),
Ambiguity { overflow: true } => write!(f, "Overflow"),
Ambiguity { overflow: None } => write!(f, "Ambiguity"),
Ambiguity { overflow: Some(suggest_increasing_limit) } => {
write!(f, "Overflow({suggest_increasing_limit})")
}
Cycle(ref cycle) => write!(f, "Cycle({cycle:?})"),
}
}
Expand Down
30 changes: 20 additions & 10 deletions compiler/rustc_middle/src/traits/solve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ pub enum Certainty {

impl Certainty {
pub const AMBIGUOUS: Certainty = Certainty::Maybe(MaybeCause::Ambiguity);
pub const OVERFLOW: Certainty = Certainty::Maybe(MaybeCause::Overflow);

/// Use this function to merge the certainty of multiple nested subgoals.
///
Expand All @@ -79,16 +78,13 @@ impl Certainty {
(Certainty::Yes, Certainty::Yes) => Certainty::Yes,
(Certainty::Yes, Certainty::Maybe(_)) => other,
(Certainty::Maybe(_), Certainty::Yes) => self,
(Certainty::Maybe(MaybeCause::Ambiguity), Certainty::Maybe(MaybeCause::Ambiguity)) => {
Certainty::Maybe(MaybeCause::Ambiguity)
}
(Certainty::Maybe(MaybeCause::Ambiguity), Certainty::Maybe(MaybeCause::Overflow))
| (Certainty::Maybe(MaybeCause::Overflow), Certainty::Maybe(MaybeCause::Ambiguity))
| (Certainty::Maybe(MaybeCause::Overflow), Certainty::Maybe(MaybeCause::Overflow)) => {
Certainty::Maybe(MaybeCause::Overflow)
}
(Certainty::Maybe(a), Certainty::Maybe(b)) => Certainty::Maybe(a.unify_with(b)),
}
}

pub const fn overflow(suggest_increasing_limit: bool) -> Certainty {
Certainty::Maybe(MaybeCause::Overflow { suggest_increasing_limit })
}
}

/// Why we failed to evaluate a goal.
Expand All @@ -99,7 +95,21 @@ pub enum MaybeCause {
/// or we hit a case where we just don't bother, e.g. `?x: Trait` goals.
Ambiguity,
/// We gave up due to an overflow, most often by hitting the recursion limit.
Overflow,
Overflow { suggest_increasing_limit: bool },
}

impl MaybeCause {
fn unify_with(self, other: MaybeCause) -> MaybeCause {
match (self, other) {
(MaybeCause::Ambiguity, MaybeCause::Ambiguity) => MaybeCause::Ambiguity,
(MaybeCause::Ambiguity, MaybeCause::Overflow { .. }) => other,
(MaybeCause::Overflow { .. }, MaybeCause::Ambiguity) => self,
(
MaybeCause::Overflow { suggest_increasing_limit: a },
MaybeCause::Overflow { suggest_increasing_limit: b },
) => MaybeCause::Overflow { suggest_increasing_limit: a || b },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't suggest increasing the limit if we hit a true cycle, correct? I feel like this should be &&, because increasing the limit here when one of the branches is false (i.e. cycle) will not have a positive effect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or hitting the fixpoint limit, which also won't be improved by increasing the limit.

Copy link
Contributor Author

@lcnr lcnr Feb 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will, in case one of the goals fails or has no constraints, if we have two nested goals:

  • non recursion_limit cycle
  • recursion_limit increase -> error

then we should suggest increasing the recursion limit as it would change the result to NoSolution which may then remove ambiguity elsewhere

}
}
}

#[derive(Debug, PartialEq, Eq, Clone, Copy, Hash, HashStable, TypeFoldable, TypeVisitable)]
Expand Down
6 changes: 4 additions & 2 deletions compiler/rustc_trait_selection/src/solve/alias_relate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,13 @@ impl<'tcx> EvalCtxt<'_, 'tcx> {
let Goal { param_env, predicate: (lhs, rhs, direction) } = goal;

let Some(lhs) = self.try_normalize_term(param_env, lhs)? else {
return self.evaluate_added_goals_and_make_canonical_response(Certainty::OVERFLOW);
return self
.evaluate_added_goals_and_make_canonical_response(Certainty::overflow(true));
};

let Some(rhs) = self.try_normalize_term(param_env, rhs)? else {
return self.evaluate_added_goals_and_make_canonical_response(Certainty::OVERFLOW);
return self
.evaluate_added_goals_and_make_canonical_response(Certainty::overflow(true));
};

let variance = match direction {
Expand Down
33 changes: 16 additions & 17 deletions compiler/rustc_trait_selection/src/solve/eval_ctxt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use rustc_infer::infer::{
BoundRegionConversionTime, DefineOpaqueTypes, InferCtxt, InferOk, TyCtxtInferExt,
};
use rustc_infer::traits::query::NoSolution;
use rustc_infer::traits::solve::MaybeCause;
use rustc_infer::traits::ObligationCause;
use rustc_middle::infer::canonical::CanonicalVarInfos;
use rustc_middle::infer::unify_key::{ConstVariableOrigin, ConstVariableOriginKind};
Expand All @@ -29,7 +30,7 @@ use std::ops::ControlFlow;
use crate::traits::vtable::{count_own_vtable_entries, prepare_vtable_segments, VtblSegment};

use super::inspect::ProofTreeBuilder;
use super::{search_graph, GoalEvaluationKind};
use super::{search_graph, GoalEvaluationKind, FIXPOINT_STEP_LIMIT};
use super::{search_graph::SearchGraph, Goal};
use super::{GoalSource, SolverMode};
pub use select::InferCtxtSelectExt;
Expand Down Expand Up @@ -154,10 +155,6 @@ impl<'a, 'tcx> EvalCtxt<'a, 'tcx> {
self.search_graph.solver_mode()
}

pub(super) fn local_overflow_limit(&self) -> usize {
self.search_graph.local_overflow_limit()
}

/// Creates a root evaluation context and search graph. This should only be
/// used from outside of any evaluation, and other methods should be preferred
/// over using this manually (such as [`InferCtxtEvalExt::evaluate_root_goal`]).
Expand All @@ -167,7 +164,7 @@ impl<'a, 'tcx> EvalCtxt<'a, 'tcx> {
f: impl FnOnce(&mut EvalCtxt<'_, 'tcx>) -> R,
) -> (R, Option<inspect::GoalEvaluation<'tcx>>) {
let mode = if infcx.intercrate { SolverMode::Coherence } else { SolverMode::Normal };
let mut search_graph = search_graph::SearchGraph::new(infcx.tcx, mode);
let mut search_graph = search_graph::SearchGraph::new(mode);

let mut ecx = EvalCtxt {
search_graph: &mut search_graph,
Expand Down Expand Up @@ -388,16 +385,18 @@ impl<'a, 'tcx> EvalCtxt<'a, 'tcx> {
&& source != GoalSource::ImplWhereBound
};

if response.value.certainty == Certainty::OVERFLOW && !keep_overflow_constraints() {
(Certainty::OVERFLOW, false)
} else {
let has_changed = !response.value.var_values.is_identity_modulo_regions()
|| !response.value.external_constraints.opaque_types.is_empty();

let certainty =
self.instantiate_and_apply_query_response(param_env, original_values, response);
(certainty, has_changed)
if let Certainty::Maybe(MaybeCause::Overflow { .. }) = response.value.certainty
&& !keep_overflow_constraints()
{
return (response.value.certainty, false);
}

let has_changed = !response.value.var_values.is_identity_modulo_regions()
|| !response.value.external_constraints.opaque_types.is_empty();

let certainty =
self.instantiate_and_apply_query_response(param_env, original_values, response);
(certainty, has_changed)
}

fn compute_goal(&mut self, goal: Goal<'tcx, ty::Predicate<'tcx>>) -> QueryResult<'tcx> {
Expand Down Expand Up @@ -466,8 +465,8 @@ impl<'a, 'tcx> EvalCtxt<'a, 'tcx> {
let inspect = self.inspect.new_evaluate_added_goals();
let inspect = core::mem::replace(&mut self.inspect, inspect);

let mut response = Ok(Certainty::OVERFLOW);
for _ in 0..self.local_overflow_limit() {
let mut response = Ok(Certainty::overflow(false));
for _ in 0..FIXPOINT_STEP_LIMIT {
// FIXME: This match is a bit ugly, it might be nice to change the inspect
// stuff to use a closure instead. which should hopefully simplify this a bit.
match self.evaluate_added_goals_step() {
Expand Down
Loading
Loading