-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
trait_selection: fix type-const eval ICE #152040
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
base: main
Are you sure you want to change the base?
Conversation
|
|
| if !type_const_stack.insert(uv.def) { | ||
| let guar = tcx.dcx().span_err( | ||
| tcx.def_span(uv.def), | ||
| "cycle detected when evaluating type-level constant", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should detect a cyclic error in this place though, maybe we can handle it better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm don't know how/where/when we invoke try_evaluate_const (during normalization of course, but …) but ideally (?) either the query system would detect cycles / overflows for us (but I guess no queries are involved here) or we have a simple depth param similar to what type normalization does instead of storing visited definitions. idk, imma wait for Boxy
|
Does this code compiles under your impl? |
3fc70e5 to
e853189
Compare
This comment has been minimized.
This comment has been minimized.
|
@Human9000-bit Yeah, it should. Added a rev so that we can make sure it. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some preliminary comments & suggestions. I'm now gonna pass the assignment to Boxy since I don't feel comfortable with this part of the code (yet).
|
|
||
| trait NewTrait {} | ||
|
|
||
| impl NewTrait for () {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By adding this impl, this test doesn't reproduce the ICE on nightly/main.
| impl NewTrait for () {} |
| #![feature(generic_const_items, min_generic_const_args)] | ||
| #![feature(adt_const_params)] | ||
| #![allow(incomplete_features)] | ||
| #![allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is implied in UI tests.
| #![allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test isn't minimized and contains a lot of unrelated elements. Here's a smaller one:
// issue: <https://github.com/rust-lang/rust/issues/151631>
//@ compile-flags: -Znext-solver
#![feature(min_generic_const_args)]
#![expect(incomplete_features)]
trait SuperTrait {}
trait Trait: SuperTrait {
type const K: u32;
}
impl Trait for () {
type const K: u32 = const { 1 };
}
fn check(_: impl Trait<K = 0>) {}
fn main() {
check(());
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I tweaked during developing then forgot to revert. Thank you! Fixed in 1641602.
| } | ||
| }; | ||
|
|
||
| if tcx.is_type_const(uv.def) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this change makes sense, I'm not an expert in mGCA. Therefore I'll assign @BoxyUwU instead.
| trait Owner { | ||
| #[type_const] | ||
| const C<const N: u32>: u32; | ||
| //~^ ERROR cycle detected when evaluating type-level constant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this error would point to the C in the trait impl.
| if !type_const_stack.insert(uv.def) { | ||
| let guar = tcx.dcx().span_err( | ||
| tcx.def_span(uv.def), | ||
| "cycle detected when evaluating type-level constant", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm don't know how/where/when we invoke try_evaluate_const (during normalization of course, but …) but ideally (?) either the query system would detect cycles / overflows for us (but I guess no queries are involved here) or we have a simple depth param similar to what type normalization does instead of storing visited definitions. idk, imma wait for Boxy
e853189 to
1641602
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. |
Fixes #151631, fixes #151477
r? @fmease
I'd recommend reviewing commit-by-commit, the diff is less-readable to address a cyclic issue.