Check substs of ConstKind::Unevaluated in generalize

[lcnr]

This prevents the stack overflow mentioned in #72129 (comment).

While I think this change makes sense, I am not completely certain and there may be a better solution I am missing.

The stack overflow was caused by unifying a type inference variable with an unevaluated const which had exactly this inference variable in it's substitutions. Meaning something has probably already gone wrong when reaching this.

i.e: we previously ended up with

TyVar(_#1t) := [
    ();
    Const { ty: usize, val: Unevaluated(DefId(0:7 ~ issue_69654_run_pass[317d]::{{impl}}[0]::{{constant}}[0]), [TyVar(_#1t)], None) }
]

in the TypeVariableTable.

r? @nikomatsakis @varkor

[lcnr]

This is not the correct fix here, currently discussing a better solution on zulip: https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/substs.20of.20Unevaluated.20consts

[lcnr]

Well, seems like it is at least partially the correct approach 😆

[varkor]

Is this an existing pattern? It feels like a hack, because relating something to itself should be a no-op. Why does this work?

[lcnr]

This is an existing pattern, we already (ab)use type relations in a lot of places where want to traverse a given type/const/whatever:

rust/src/librustc_infer/infer/combine.rs

Line 534 in e070765

assert_eq!(t, t2); // we are abusing TypeRelation here; both LHS and RHS ought to be ==

or

rust/src/librustc_infer/infer/nll_relate/mod.rs

Line 818 in e070765

impl<D> TypeRelation<'tcx> for TypeGeneralizer<'me, 'tcx, D>

[varkor]

Would you be able to give a small example of what's going wrong as part of a comment in the code?

[lcnr]

later this week 😆 I have to think of a good example where this check is required and not just
a stopgap because we have unused substs in ConstKind::Unevaluated.

[nikomatsakis]

Left some thoughts and comments, checking my understanding here.

[nikomatsakis]

Huh, so, actually, what does subtyping even mean with respect to const kind parameters? I assume that T<C1> <: T<C2> only if C1 == C2, right? In other words, if we have something like ?X <: Foo<?Y, ?C>, we probably want to instantiate Foo with Foo<?Z, ?C> -- i.e., the constant still has to be equal..?

But I guess the point is that it only has to be equal after evaluation, and hence we do want a fresh variable ?C2 and the requirement that ?C1 = ?C2, which really means that the evaluate to the same value?

[lcnr]

I don't quite get why subtyping is relevant here 🤔

Currently looking into getting a potentially more helpful example, the minimized const generics example is

#![feature(const_generics)]
trait Bar<const M: usize> {}
impl<const N: usize> Bar<N> for A<{ 6 + 1 }> {}

struct A<const N: usize>
where
    A<N>: Bar<N>;

fn main() {
    let _ = A;
}

[lcnr]

And the same problem for ty vars

#![feature(const_generics)]

// The goal is is to get an unevaluated const `ct` with a `Ty::Infer(TyVar(_#1t)` subst.
//
// If we are then able to infer `ty::Infer(TyVar(_#1t) := Ty<ct>`, we should be able
// to get a stack overflow.

struct Foo<const N: usize>;

trait Bind<T> {
    fn bind() -> (T, Self);
}

// `N` has to be `ConstKind::Unevaluated`.
impl<T> Bind<T> for Foo<{ 6 + 1 }> {
    fn bind() -> (T, Self) {
        (panic!(), Foo)
    }
}

fn main() {
    let (mut t, foo) = Foo::bind();
    // `t` is `ty::Infer(TyVar(_#1t))`,
    // `foo` contains `ty::Infer(TyVar(_#1t))` in its substs
    t = foo; // ! MISSED OCCURS CHECK => STACKOVERFLOW !
}

[lcnr]

Further minimized, we don't even need explicit const params

#![feature(const_generics)]

// The goal is is to get an unevaluated const `ct` with a `Ty::Infer(TyVar(_#1t)` subst.
//
// If we are then able to infer `ty::Infer(TyVar(_#1t) := Ty<ct>`, we should be able
// to get a stack overflow.
fn bind<T>() -> (T, [u8; 6 + 1]) {
    todo!()
} 

fn main() {
    let (mut t, foo) = bind();
    // `t` is `ty::Infer(TyVar(_#1t))`,
    // `foo` contains `ty::Infer(TyVar(_#1t))` in its substs
    t = foo; // ! MISSED OCCURS CHECK => STACKOVERFLOW !
}

[nikomatsakis]

This comment is kind of uninformative, yeah. It'd be nice to have an example. But I guess it comes down to the point I was raising earlier, right? That two 'unevaluated' constants can be equal so long as they are equal after evaluation, and we don't know yet which of the substitutions will be important?

[lcnr]

I think I found the underlying problem causing the stack overflows, as this PR is currently not enough.

Afaict we need some kind of occurs check in

rust/src/librustc_infer/infer/combine.rs

Lines 160 to 166 in 8a6d434

	(ty::ConstKind::Infer(InferConst::Var(vid)), _) => {
	return self.unify_const_variable(a_is_expected, vid, b);
	}
	(_, ty::ConstKind::Infer(InferConst::Var(vid))) => {
	return self.unify_const_variable(!a_is_expected, vid, a);
	}

We currently unify ConstKind::Infer(InferConst::Var(_cvar)) with ConstKind::Unevaluated(def, substs = [ConstKind::Infer(InferConst::Var(_cvar))]) which looking at the substs in generalize is not enough (as we only generalize
for type vars).

We probably need something similar to sub_root_var to correctly detect cycles.

Does this seem correct?

[lcnr]

an actual fix is in #74040