Fix ICE on standard constants in type constraints (min_generic_const_args)#157191
Fix ICE on standard constants in type constraints (min_generic_const_args)#157191sunny026 wants to merge 1 commit into
Conversation
rustbot
added
S-waiting-on-review
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tiif (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Hey, thanks for a PR, few things:
- Is there a filled issue about this ICE? If so, please link this via adding
Fixes <issue number>in your description - Question about the guard you've added, it's a bit tricky and I'm not really confident in my knowledges about const stuff, but: doesn't if something is
AssocConstimplies that this is "const"? Because this check reads a little uneasy to me. Same forConstandAnonConst - Can you please add a test, likely somewhere in
tests/ui/constsmaybe
This comment has been minimized.
This comment has been minimized.
|
Hey @Kivooeo, thanks for the review! To answer your questions: Issue number: There is no existing filled issue for this ICE. I ran into it naturally while experimenting with min_generic_const_args in type constraints. I can create an issue for tracking purposes if you'd prefer, but otherwise it's just a direct fix. The is_type_const guard: You're completely right that AssocConst and Const imply they are constants. However, the tcx.is_type_const(uv.def) method isn't checking if it's a constant in general—it checks the specific is_type_const boolean field that lives inside the DefKind::AssocConst { is_type_const: bool } and DefKind::Const { is_type_const: bool } enum variants. We only want to apply this specific normalization path if the constant is actually being evaluated in a type context (i.e. as a generic const argument). If is_type_const is false, it's just a normal runtime constant, and passing it through the old logic was what caused the tcx.def_kind parent resolution to panic. The guard ensures we only capture actual type-level constants. Adding a test: My latest commit actually blesses an existing test in tests/ui/const-generics/gca/ambiguous-on-failed-eval-with-vars-fail.rs. Before my fix, that test would ICE internally on the fallback, but now it correctly emits E0308: mismatched types instead of panicking! Do you think that test is sufficient since it exercises this exact code path, or would you still like me to add a dedicated, isolated test in tests/ui/consts? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
rustbot
added
has-merge-commits
…args) Resolves an Internal Compiler Error (ICE) that occurs when standard constants are used where type const is expected under min_generic_const_args. In the old trait solver's normalize.rs, standard consts were being blindly treated as alias projections. The solver would panic with DefId(...) does not have a const_of_item when attempting to extract metadata for an item that wasn't encoded as a type const. This PR adds the missing tcx.is_type_const guards. If a constant is not a type const, it falls through to the standard evaluation branch, gracefully emitting an E0308 type mismatch error instead of crashing.
rustbot
removed
S-waiting-on-author
|
@Kivooeo can u review? |
|
Hey, it's been only two days since you opened PR (review might take up to two weeks, because people are busy with other things than Rust) and I'm not assigned reviewer on that PR. I could assign myself for sure, but I'm very busy this week and therefore I'm trying to resolve work/life things first, thanks for understanding and your patience |
There was a problem hiding this comment.
Hi, thank you for your contribution!
I don't really understand what you mean by Before my fix, that test would ICE internally on the fallback, the test tests/ui/const-generics/gca/ambiguous-on-failed-eval-with-vars-fail.rs is not ICE'ing on either old solver or new solver. So I will prefer adding a new test that demonstrates the ICE.
I am not the right person to judge the correctness of this PR, so I will roll the mgca expert for you.
r? @BoxyUwU
| fn test_proj() { | ||
| let (mut arr, mut arr_with_weird_len) = proj(); | ||
| //[next]~^ ERROR type annotations needed | ||
| arr_with_weird_len = [(); 10]; | ||
| //[old]~^ ERROR mismatched types | ||
| } |
There was a problem hiding this comment.
This is more of a note for @BoxyUwU
I only highlight this specific example, but the same reasoning applies to other tests. Previously in both old solver and new solver, the statement arr_with_weird_len = [(); 10]; did not err in both old solver and new solver, but now it started to error only in old solver, which makes me doubt if this behavior is desirable.
There was a problem hiding this comment.
fun, worth noting GCA isn't supported by the old solver so the behaviour here is somewhat 🤷♀️
|
|
|
was this ICE only on the old solver and not the new solver? somewhat confused, there definitely needs to be a test which goes from ICE->pass on this PR but that isn't present |
|
@rustbot author |
rustbot
removed
the
S-waiting-on-review
|
Reminder, once the PR becomes ready for a review, use |
rustbot
added
the
S-waiting-on-author
|
This worries me a bit - AFAIK, UnevaluatedConstKind::Projection/Inherent/Free with non-type-consts should only be reachable under full GCA, not mGCA. I intentionally did not implement them in the old solver, and require -Znext-solver for full GCA. The fact these are reachable under mGCA under the old solver worries we might have accidentally stabilized something we shouldn't have.
I'm confused by this comment a bit, sorry - you say that test currently ICEs, but, CI is passing right now, with that test not ICEing... I'm missing something! |
7 participants
This PR fixes an internal compiler error (ICE) where
tcx.def_kindpanics when attempting to resolve un-evaluated constant aliases whose parent definition is not a type const.By adding explicit
tcx.is_type_const(uv.def)guards toDefKind::AssocConstandDefKind::Const, and explicitly capturingDefKind::AnonConst | DefKind::Const { .. } | DefKind::AssocConst { .. }in the fallback block, we ensure that constants without themin_generic_const_argsattribute are handled gracefully rather than triggering an unreachable panic.