mgca: Register ConstArgHasType when normalizing projection consts#154853
mgca: Register ConstArgHasType when normalizing projection consts#154853lapla-cogito wants to merge 7 commits into
ConstArgHasType when normalizing projection consts#154853Conversation
|
|
This comment has been minimized.
This comment has been minimized.
b93677c to
672dfb8
Compare
This comment has been minimized.
This comment has been minimized.
| assoc_term_own_obligations(selcx, obligation, &mut nested); | ||
| Progress { term: term.instantiate(tcx, args), obligations: nested } | ||
| let instantiated_term: Term<'tcx> = term.instantiate(tcx, args); | ||
| if let Some(ct) = instantiated_term.as_const() { |
There was a problem hiding this comment.
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).
This comment has been minimized.
This comment has been minimized.
672dfb8 to
90e8301
Compare
|
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.
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 🤔 |
4af1458 to
7dc2ef4
Compare
This comment has been minimized.
This comment has been minimized.
Ah, I didn't consider that. Indeed, in such cases, it seems ICE doesn't occur. IIUC, the key is the ordering of For trait/inherent impl type consts, the impl block's |
This comment has been minimized.
This comment has been minimized.
7dc2ef4 to
c89665d
Compare
This comment has been minimized.
This comment has been minimized.
c89665d to
3d241ce
Compare
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"; |
|
wrt your PR description it's a bit wrong right now:
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 |
| tcx.const_of_item(alias_term.def_id).instantiate(tcx, args).into() | ||
| }; | ||
|
|
||
| if let Some(ct) = term.as_const() { |
There was a problem hiding this comment.
same for the old solver's normalization routine
|
thanks for working on this ✨ |
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
I tested this variant and adding edit: Of course, even if this PR doesn't address such cases, it would be possible for me to handle them later. |
|
@rustbot ready |
4e17133 to
00b1870
Compare
This comment has been minimized.
This comment has been minimized.
00b1870 to
cb7ca45
Compare
This comment has been minimized.
This comment has been minimized.
cb7ca45 to
ff1972a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…projection normalization
ff1972a to
1240be2
Compare
|
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. |
| ty::PredicateKind::Clause(ty::ClauseKind::RegionOutlives(..)) | ||
| | ty::PredicateKind::Clause(ty::ClauseKind::TypeOutlives(..)) => false, | ||
| | ty::PredicateKind::Clause(ty::ClauseKind::TypeOutlives(..)) | ||
| | ty::PredicateKind::Clause(ty::ClauseKind::ConstArgHasType(..)) => false, |
There was a problem hiding this comment.
oh this is slightly scuffed actually 🤔 we probably don't to change this but then I guess try_normalize_erasing_regions ICEs due to dropping ConstArgHasType goals on the floor...
can you revert this and then instead of registering the ConstArgHasType goals in try_fold_free_or_assoc for query_normalize instead handle it in the normalize_canonicalized_x queries:
tcx.normalize_canonicalized_projection(c_term)
tcx.normalize_canonicalized_free_alias(c_term)
tcx.normalize_canonicalized_inherent_projection(c_term)
There was a problem hiding this comment.
This change results in a more sensible error message being emitted in the type-const-free-value-type-mismatch.rs test.
c0f7e12 to
e8390d8
Compare
| /// | ||
| /// Additionally, when `term` is a const, this registers a `ConstArgHasType` | ||
| /// goal to ensure that the const value's type matches the type of the | ||
| /// alias it was normalized from, preventing ICEs from type mismatches. |
There was a problem hiding this comment.
sorry for butting in kinda late - some optional feedback here:
The way that I would expect this to work is that things are wfchecked at definition site before they're attempted to be evaluated (and so would be tainted, and this usage would never even get to this point of attempting to normalize it). You write in the PR description:
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.
I presume there is some cursed arcane reason why we cannot easily fix the "due to query evaluation ordering" part of that sentence, and why this approach of slapping on ConstArgHasType obligations to the normalization result is needed.
Would you mind writing that cursed arcane reason into a comment here? I find comments of the style "hey reader, you're not insane, this is indeed wonky and not what you would expect, because [...]" to be extremely helpful ❤️
|
@bors try @rust-timer queue we should check perf incase it matters :3 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mgca: Register `ConstArgHasType` when normalizing projection consts
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (4787dc1): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary 3.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 514.264s -> 515.211s (0.18%) |
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 existingConstArgHasTypecheck inwfcheck::check_type_constdoes 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