mgca: Register ConstArgHasType when normalizing projection consts

[lapla-cogito]

View all comments

fixes #152962

When running CTFE on a MIR body, normalizing a type const within that body can change the type of the resulting value, causing the MIR to become ill-formed. Since no prior error has been reported at that point, MIR validation fires a span_bug!. The existing ConstArgHasType check in wfcheck::check_type_const does catch this at the definition site, but due to query evaluation ordering, the normalization path can reach MIR validation before that check has run.

Fix this by registering a ConstArgHasType(ct, expected_ty) obligation/goal when normalizing projection consts (both trait and inherent), in both the old and new trait solvers. This ensures the type mismatch is reported as an error during normalization itself, which taints the MIR before validation runs.

The first commit fixes the original case reported in the issue. The second commit fixes a different ICE pattern reported within the issue (see #152962 (comment)).

r? BoxyUwU

[rustbot]

BoxyUwU is currently at their maximum review capacity.
They may take a while to respond.

[lapla-cogito]

Boxy and I discussed the fix approach and settled on registering a nested goal when normalizing type consts to verify that the normalized term has the type of the type const item. However, this implementation applies the same nested goal to all associated consts, not just type consts. This better expresses the invariant that the normalized result of a const should match its declared type (I think only type consts currently go through this path, but this also guards against future changes).

[BoxyUwU]

can you add an equivalent test for free type consts, e.g:

type const N: usize = "this isn't a usize";
fn f() -> [u8; const { N }] {}

actuall;y it seems like this doesn't even ICE on nightly rn 🤔 can you look into why this doesn't ICE but the other stuff does. would like to properly understand why we need this for projections but not free type consts.

When a type const has a value whose type doesn't match the declared type, normalizing a reference to it can trigger CTFE, which in turn runs MIR validation on the const body

the type const has no associated const body. so i think what's happening here is that we normalize everything in a MIR body and then do MIR validation on it afterwards, rather than normalization causing validation to happen 🤔

[lapla-cogito]

can you add an equivalent test for free type consts

actuall;y it seems like this doesn't even ICE on nightly rn 🤔

Ah, I didn't consider that. Indeed, in such cases, it seems ICE doesn't occur.

IIUC, the key is the ordering of check_type_wf(). For free type consts, check_type_const() runs during par_items(), which catches the type mismatch and emits an error. CTFE of inline consts like const { N } happens later (during MIR_borrow_checking phase, after check_crate completes), so has_errors() is already Some by then and MIR validation suppresses the span_bug!.

For trait/inherent impl type consts, the impl block's check_well_formed() runs during par_items() and can trigger CTFE via compare_impl_item (trait) or signature normalization (inherent). But check_type_const() for the associated item doesn't run until par_impl_items(), which hasn't started yet. So no error has been reported when MIR validation fires. Therefore, I added a test to verify that ICE doesn't occur for free type consts cases as well, thanks.

[BoxyUwU]

IIUC, the key is the ordering of check_type_wf().

oooh okay I think I get it. this test case relies on evaluating the anon const during wfcheck of free items and results in an ICE. does this PR fix this variant: ?

//@ compile-flags: -Zvalidate-mir
#![feature(min_generic_const_args)]

type const X: usize = const { N };
type const N: usize = "this isn't a usize";

[BoxyUwU]

wrt your PR description it's a bit wrong right now:

When a type const has a value whose type doesn't match the declared type, normalizing a reference to it can trigger CTFE, which in turn runs MIR validation on the const body. The MIR body contains a type mismatch, and since no prior error has been reported at that point, MIR validation fires a span_bug!.

it's not that normalizing a usage of the type const triggers CTFE. rather that when running CTFE on a MIR body we may wind up normalizing a type const in the body, which can change the type of the value causing the MIR to become illformed.

can you update the description

[BoxyUwU]

View changes since the review

can you add a new method to EvalCtxt which handles taking the normalized to Term and the alias, and handles checking for if its a Const and adding the ConstArgHasType goal. then the logic between here and normalizing projections (and presumably also free aliases once you fix that) can be shared.

it also gives a good place to put a comment about why we're even doing this ✨

[lapla-cogito]

Ah nice idea, thanks.

and presumably also free aliases once you fix that

Free aliases does not trigger an ICE because check_type_const() runs in the same phase. Adding ConstArgHasType would cause a double error, so I don't believe the new method added here is necessary for free aliases.

[BoxyUwU]

View changes since the review

same for the old solver's normalization routine

[BoxyUwU]

thanks for working on this ✨

[BoxyUwU]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[lapla-cogito]

IIUC, the key is the ordering of check_type_wf().

oooh okay I think I get it. this test case relies on evaluating the anon const during wfcheck of free items and results in an ICE. does this PR fix this variant: ?

//@ compile-flags: -Zvalidate-mir
#![feature(min_generic_const_args)]

type const X: usize = const { N };
type const N: usize = "this isn't a usize";

I tested this variant and adding ConstArgHasType to normalize_free_alias does not fix it. The obligation is registered in the outer wfcheck context, but the ICE happens inside the AnonConst's CTFE -> MIR validation, where has_errors() hasn't been updated yet. This would need a different approach to fix, so I think it's out of scope for this PR 🤔

edit: Of course, even if this PR doesn't address such cases, it would be possible for me to handle them later.

[lapla-cogito]

@rustbot ready

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[BoxyUwU]

This would need a different approach to fix, so I think it's out of scope for this

I don't think I understand the distinction you're drawing here 🤔 If the ICE is caused by validating the const { N } then that's the same issue as what this PR is solving for associated constants. We'll be normalizing that N somewhere and wherever that is needs a ConstArgHasType goal

You're right that this could be done in a follow-up PR though if this is a fundamentally different issue it might need a fundamentally different solution at which point this PR might not be needed after all. Can you try and figure out what codepath is normalizing the N in const { N } and make sure there's actually a ConstArgHasType goal getting proven when doing so

[lapla-cogito]

What I said here seems incorrect (sorry) though my conclusion that the free alias case shouldn't be addressed in this PR remains unchanged

I don't think I understand the distinction you're drawing here 🤔 If the ICE is caused by validating the const { N } then that's the same issue as what this PR is solving for associated constants. We'll be normalizing that N somewhere and wherever that is needs a ConstArgHasType goal

While the projection case this PR originally addresses can be resolved within the obligation system as already implemented, for the free alias in AnonConst scenario, the N in const { N } is embedded as an unevaluated const N during MIR construction. Normalization is not performed by the trait solver (it can be verified by inserting a random debug eprintln at the top of normalize_free_alias()) but instead handled by PostAnalysisNormalize (a MIR pass), after which Subtyper detects the type inconsistency and inserts a CastKind::Subtype cast. Subsequent MIR validation then ICEs on this ill-formed cast. From my understanding, this cannot be resolved by ConstArgHasType goals or the trait solver itself.

Can you try and figure out what codepath is normalizing the N in const { N } and make sure there's actually a ConstArgHasType goal getting proven when doing so

Therefore, cases like this don't go through the obligation system at all.