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

fix coherence error for very large tuples™ #132001

Merged
merged 2 commits into from
Oct 21, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
23 changes: 16 additions & 7 deletions compiler/rustc_next_trait_solver/src/solve/eval_ctxt/canonical.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use rustc_type_ir::fold::TypeFoldable;
use rustc_type_ir::inherent::*;
use rustc_type_ir::relate::solver_relating::RelateExt;
use rustc_type_ir::{self as ty, Canonical, CanonicalVarValues, InferCtxtLike, Interner};
use tracing::{instrument, trace};
use tracing::{debug, instrument, trace};

use crate::canonicalizer::{CanonicalizeMode, Canonicalizer};
use crate::delegate::SolverDelegate;
Expand Down Expand Up @@ -165,12 +165,21 @@ where
// HACK: We bail with overflow if the response would have too many non-region
// inference variables. This tends to only happen if we encounter a lot of
// ambiguous alias types which get replaced with fresh inference variables
// during generalization. This prevents a hang in nalgebra.
let num_non_region_vars = canonical.variables.iter().filter(|c| !c.is_region()).count();
if num_non_region_vars > self.cx().recursion_limit() {
return Ok(self.make_ambiguous_response_no_constraints(MaybeCause::Overflow {
suggest_increasing_limit: true,
}));
// during generalization. This prevents hangs caused by an exponential blowup,
// see tests/ui/traits/next-solver/coherence-alias-hang.rs.
//
// We don't do so for `NormalizesTo` goals as we erased the expected term and
// bailing with overflow here would prevent us from detecting a type-mismatch,
// causing a coherence error in diesel, see #131969. We still bail with verflow
Copy link
Member

@Noratrieb Noratrieb Oct 21, 2024

Choose a reason for hiding this comment

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

Suggested change
// causing a coherence error in diesel, see #131969. We still bail with verflow
// causing a coherence error in diesel, see #131969. We still bail with overflow

(i won't r-, you can if you want to or not)

Copy link
Member

Choose a reason for hiding this comment

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

verflow is actually a type-system specific concept :clueless:

Copy link
Member

Choose a reason for hiding this comment

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

ah i see, it describes the way crates of different versions flow through the type system

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed as part of #132026 :3

// when later returning from the parent AliasRelate goal.
if !self.is_normalizes_to_goal {
let num_non_region_vars = canonical.variables.iter().filter(|c| !c.is_region()).count();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let num_non_region_vars = canonical.variables.iter().filter(|c| !c.is_region()).count();
let num_non_region_vars = canonical.variables.iter().filter(|c| !c.is_region() && !c.is_existential()).count();

Copy link
Member

Choose a reason for hiding this comment

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

army after this

if num_non_region_vars > self.cx().recursion_limit() {
debug!(?num_non_region_vars, "too many inference variables -> overflow");
return Ok(self.make_ambiguous_response_no_constraints(MaybeCause::Overflow {
suggest_increasing_limit: true,
}));
}
}

Ok(canonical)
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_trait_selection/src/traits/coherence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,7 @@ fn equate_impl_headers<'tcx>(
}

/// The result of [fn impl_intersection_has_impossible_obligation].
#[derive(Debug)]
enum IntersectionHasImpossibleObligations<'tcx> {
Yes,
No {
Expand Down Expand Up @@ -328,6 +329,7 @@ enum IntersectionHasImpossibleObligations<'tcx> {
/// of the two impls above to be empty.
///
/// Importantly, this works even if there isn't a `impl !Error for MyLocalType`.
#[instrument(level = "debug", skip(selcx), ret)]
fn impl_intersection_has_impossible_obligation<'a, 'cx, 'tcx>(
selcx: &mut SelectionContext<'cx, 'tcx>,
obligations: &'a [PredicateObligation<'tcx>],
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
//@ check-pass
lcnr marked this conversation as resolved.
Show resolved Hide resolved

// When canonicalizing a response in the trait solver, we bail with overflow
// if there are too many non-region inference variables. Doing so in normalizes-to
// goals ends up hiding inference constraints in cases which we want to support,
// see #131969. To prevent this issue we do not check for too many inference
// variables in normalizes-to goals.
#![recursion_limit = "8"]

trait Bound {}
trait Trait {
type Assoc;
}


impl<T0, T1, T2, T3, T4, T5, T6, T7> Trait for (T0, T1, T2, T3, T4, T5, T6, T7)
where
T0: Trait,
T1: Trait,
T2: Trait,
T3: Trait,
T4: Trait,
T5: Trait,
T6: Trait,
T7: Trait,
(
T0::Assoc,
T1::Assoc,
T2::Assoc,
T3::Assoc,
T4::Assoc,
T5::Assoc,
T6::Assoc,
T7::Assoc,
): Clone,
{
type Assoc = (
T0::Assoc,
T1::Assoc,
T2::Assoc,
T3::Assoc,
T4::Assoc,
T5::Assoc,
T6::Assoc,
T7::Assoc,
);
}

trait Overlap {}
impl<T: Trait<Assoc = ()>> Overlap for T {}
impl<T0, T1, T2, T3, T4, T5, T6, T7> Overlap for (T0, T1, T2, T3, T4, T5, T6, T7) {}
fn main() {}
11 changes: 4 additions & 7 deletions tests/ui/traits/next-solver/overflow/coherence-alias-hang.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
//@ check-pass
//@ revisions: ai_current ai_next ia_current ia_next ii_current ii_next
//@[ai_next] compile-flags: -Znext-solver
//@[ia_next] compile-flags: -Znext-solver
//@[ii_next] compile-flags: -Znext-solver
//@ revisions: ai ia ii
lcnr marked this conversation as resolved.
Show resolved Hide resolved

// Regression test for nalgebra hang <https://github.com/rust-lang/rust/issues/130056>.

Expand All @@ -17,11 +14,11 @@ trait Trait {
type Assoc: ?Sized;
}
impl<T: ?Sized + Trait> Trait for W<T, T> {
#[cfg(any(ai_current, ai_next))]
#[cfg(ai)]
type Assoc = W<T::Assoc, Id<T::Assoc>>;
#[cfg(any(ia_current, ia_next))]
#[cfg(ia)]
type Assoc = W<Id<T::Assoc>, T::Assoc>;
#[cfg(any(ii_current, ii_next))]
#[cfg(ii)]
type Assoc = W<Id<T::Assoc>, Id<T::Assoc>>;
}

Expand Down
Loading