Skip to content

Commit

Permalink
track overflowing goals for overfow errors
Browse files Browse the repository at this point in the history
  • Loading branch information
lcnr committed Feb 26, 2024
1 parent 79480ec commit 8003148
Show file tree
Hide file tree
Showing 5 changed files with 99 additions and 43 deletions.
94 changes: 67 additions & 27 deletions compiler/rustc_trait_selection/src/traits/coherence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use rustc_errors::{DiagnosticBuilder, EmissionGuarantee};
use rustc_hir::def::DefKind;
use rustc_hir::def_id::{DefId, LOCAL_CRATE};
use rustc_infer::infer::{DefineOpaqueTypes, InferCtxt, TyCtxtInferExt};
use rustc_infer::traits::{util, TraitEngine, TraitEngineExt};
use rustc_infer::traits::{util, FulfillmentErrorCode, TraitEngine, TraitEngineExt};
use rustc_middle::traits::query::NoSolution;
use rustc_middle::traits::solve::{CandidateSource, Certainty, Goal};
use rustc_middle::traits::specialization_graph::OverlapMode;
Expand All @@ -35,6 +35,8 @@ use rustc_span::DUMMY_SP;
use std::fmt::Debug;
use std::ops::ControlFlow;

use super::error_reporting::suggest_new_overflow_limit;

/// Whether we do the orphan check relative to this crate or
/// to some remote crate.
#[derive(Copy, Clone, Debug)]
Expand All @@ -56,6 +58,9 @@ pub struct OverlapResult<'tcx> {
/// `true` if the overlap might've been permitted before the shift
/// to universes.
pub involves_placeholder: bool,

/// Used in the new solver to suggest increasing the recursion limit.
pub overflowing_predicates: Vec<ty::Predicate<'tcx>>,
}

pub fn add_placeholder_note<G: EmissionGuarantee>(err: &mut DiagnosticBuilder<'_, G>) {
Expand All @@ -65,6 +70,18 @@ pub fn add_placeholder_note<G: EmissionGuarantee>(err: &mut DiagnosticBuilder<'_
);
}

pub fn suggest_increasing_recursion_limit<'tcx, G: EmissionGuarantee>(
tcx: TyCtxt<'tcx>,
err: &mut DiagnosticBuilder<'_, G>,
overflowing_predicates: &[ty::Predicate<'tcx>],
) {
for pred in overflowing_predicates {
err.note(format!("overflow evaluating the requirement `{}`", pred));
}

suggest_new_overflow_limit(tcx, err);
}

#[derive(Debug, Clone, Copy)]
enum TrackAmbiguityCauses {
Yes,
Expand Down Expand Up @@ -221,11 +238,11 @@ fn overlap<'tcx>(
),
);

let mut overflowing_predicates = Vec::new();
if overlap_mode.use_implicit_negative() {
if let Some(_failing_obligation) =
impl_intersection_has_impossible_obligation(selcx, &obligations)
{
return None;
match impl_intersection_has_impossible_obligation(selcx, &obligations) {
Ok(()) => return None,
Err(p) => overflowing_predicates = p,
}
}

Expand Down Expand Up @@ -261,7 +278,12 @@ fn overlap<'tcx>(
impl_header = deeply_normalize_for_diagnostics(&infcx, param_env, impl_header);
}

Some(OverlapResult { impl_header, intercrate_ambiguity_causes, involves_placeholder })
Some(OverlapResult {
impl_header,
intercrate_ambiguity_causes,
involves_placeholder,
overflowing_predicates,
})
}

#[instrument(level = "debug", skip(infcx), ret)]
Expand Down Expand Up @@ -305,10 +327,14 @@ fn equate_impl_headers<'tcx>(
/// of the two impls above to be empty.
///
/// Importantly, this works even if there isn't a `impl !Error for MyLocalType`.
///
/// If there is no impossible obligation, this returns a list of obligations which
/// overflowed by hitting the `recursion_limit` in the new solver. This is used
/// to improve the error message.
fn impl_intersection_has_impossible_obligation<'a, 'cx, 'tcx>(
selcx: &mut SelectionContext<'cx, 'tcx>,
obligations: &'a [PredicateObligation<'tcx>],
) -> Option<PredicateObligation<'tcx>> {
) -> Result<(), Vec<ty::Predicate<'tcx>>> {
let infcx = selcx.infcx;

if infcx.next_trait_solver() {
Expand All @@ -317,28 +343,42 @@ fn impl_intersection_has_impossible_obligation<'a, 'cx, 'tcx>(

// We only care about the obligations that are *definitely* true errors.
// Ambiguities do not prove the disjointness of two impls.
let mut errors = fulfill_cx.select_where_possible(infcx);
errors.pop().map(|err| err.obligation)
let errors = fulfill_cx.select_where_possible(infcx);
if errors.is_empty() {
let overflow_errors = fulfill_cx.collect_remaining_errors(infcx);
let overflowing_predicates = overflow_errors
.into_iter()
.filter(|e| match e.code {
FulfillmentErrorCode::Ambiguity { overflow: Some(true) } => true,
_ => false,
})
.map(|e| infcx.resolve_vars_if_possible(e.obligation.predicate))
.collect();
Err(overflowing_predicates)
} else {
Ok(())
}
} else {
obligations
.iter()
.find(|obligation| {
// We use `evaluate_root_obligation` to correctly track intercrate
// ambiguity clauses. We cannot use this in the new solver.
let evaluation_result = selcx.evaluate_root_obligation(obligation);

match evaluation_result {
Ok(result) => !result.may_apply(),
// If overflow occurs, we need to conservatively treat the goal as possibly holding,
// since there can be instantiations of this goal that don't overflow and result in
// success. This isn't much of a problem in the old solver, since we treat overflow
// fatally (this still can be encountered: <https://github.com/rust-lang/rust/issues/105231>),
// but in the new solver, this is very important for correctness, since overflow
// *must* be treated as ambiguity for completeness.
Err(_overflow) => false,
for obligation in obligations {
// We use `evaluate_root_obligation` to correctly track intercrate
// ambiguity clauses.
let evaluation_result = selcx.evaluate_root_obligation(obligation);

match evaluation_result {
Ok(result) => {
if !result.may_apply() {
return Ok(());
}
}
})
.cloned()
// If overflow occurs, we need to conservatively treat the goal as possibly holding,
// since there can be instantiations of this goal that don't overflow and result in
// success. While this isn't much of a problem in the old solver, since we treat overflow
// fatally, this still can be encountered: <https://github.com/rust-lang/rust/issues/105231>.
Err(_overflow) => {}
}
}

Err(Vec::new())
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ use crate::traits::{
};
use rustc_data_structures::fx::{FxHashMap, FxIndexMap};
use rustc_errors::{
codes::*, pluralize, struct_span_code_err, Applicability, DiagnosticBuilder, ErrorGuaranteed,
FatalError, MultiSpan, StashKey, StringPart,
codes::*, pluralize, struct_span_code_err, Applicability, DiagnosticBuilder, EmissionGuarantee,
ErrorGuaranteed, FatalError, MultiSpan, StashKey, StringPart,
};
use rustc_hir as hir;
use rustc_hir::def::{DefKind, Namespace, Res};
Expand Down Expand Up @@ -62,6 +62,22 @@ pub enum OverflowCause<'tcx> {
TraitSolver(ty::Predicate<'tcx>),
}

pub fn suggest_new_overflow_limit<'tcx, G: EmissionGuarantee>(
tcx: TyCtxt<'tcx>,
err: &mut DiagnosticBuilder<'_, G>,
) {
let suggested_limit = match tcx.recursion_limit() {
Limit(0) => Limit(2),
limit => limit * 2,
};
err.help(format!(
"consider increasing the recursion limit by adding a \
`#![recursion_limit = \"{}\"]` attribute to your crate (`{}`)",
suggested_limit,
tcx.crate_name(LOCAL_CRATE),
));
}

#[extension(pub trait TypeErrCtxtExt<'tcx>)]
impl<'tcx> TypeErrCtxt<'_, 'tcx> {
fn report_fulfillment_errors(
Expand Down Expand Up @@ -263,7 +279,7 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
};

if suggest_increasing_limit {
self.suggest_new_overflow_limit(&mut err);
suggest_new_overflow_limit(self.tcx, &mut err);
}

err
Expand Down Expand Up @@ -303,19 +319,6 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
);
}

fn suggest_new_overflow_limit(&self, err: &mut DiagnosticBuilder<'_>) {
let suggested_limit = match self.tcx.recursion_limit() {
Limit(0) => Limit(2),
limit => limit * 2,
};
err.help(format!(
"consider increasing the recursion limit by adding a \
`#![recursion_limit = \"{}\"]` attribute to your crate (`{}`)",
suggested_limit,
self.tcx.crate_name(LOCAL_CRATE),
));
}

/// Reports that a cycle was detected which led to overflow and halts
/// compilation. This is equivalent to `report_overflow_obligation` except
/// that we can give a more helpful error message (and, in particular,
Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_trait_selection/src/traits/specialize/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ pub struct OverlapError<'tcx> {
pub self_ty: Option<Ty<'tcx>>,
pub intercrate_ambiguity_causes: FxIndexSet<IntercrateAmbiguityCause<'tcx>>,
pub involves_placeholder: bool,
pub overflowing_predicates: Vec<ty::Predicate<'tcx>>,
}

/// Given the generic parameters for the requested impl, translate it to the generic parameters
Expand Down Expand Up @@ -435,6 +436,14 @@ fn report_conflicting_impls<'tcx>(
if overlap.involves_placeholder {
coherence::add_placeholder_note(err);
}

if !overlap.overflowing_predicates.is_empty() {
coherence::suggest_increasing_recursion_limit(
tcx,
err,
&overlap.overflowing_predicates,
);
}
}

let msg = DelayDm(|| {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ impl<'tcx> Children {
self_ty: self_ty.has_concrete_skeleton().then_some(self_ty),
intercrate_ambiguity_causes: overlap.intercrate_ambiguity_causes,
involves_placeholder: overlap.involves_placeholder,
overflowing_predicates: overlap.overflowing_predicates,
}
};

Expand Down
3 changes: 3 additions & 0 deletions tests/ui/traits/next-solver/coherence-fulfill-overflow.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ LL | impl<T: ?Sized + TwoW> Trait for W<T> {}
| ------------------------------------- first implementation here
LL | impl<T: ?Sized + TwoW> Trait for T {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<_>>>>>>>>>>>>>>>>>>>>>`
|
= note: overflow evaluating the requirement `W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<_>>>>>>>>>>>>>>>>>>>>>: TwoW`
= help: consider increasing the recursion limit by adding a `#![recursion_limit = "20"]` attribute to your crate (`coherence_fulfill_overflow`)

error: aborting due to 1 previous error

Expand Down

0 comments on commit 8003148

Please sign in to comment.