-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
Fix ICE: can't type-check body of DefId for issue #148729 #150799
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
Conversation
|
rustbot has assigned @matthewjasper. Use |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@matthewjasper are there any other cases that your aware of? I played around a bit with this after making the changes and not coming across any other cases of this exact ICE. @BoxyUwU Your doing a lot working on min_const_generic_args. So probably want to look this over as well. |
This comment has been minimized.
This comment has been minimized.
|
r? BoxyUwU |
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.
Can you add tests from both of the fixed issues
| if is_type_const(tcx, def_id) { | ||
| let uneval = ty::UnevaluatedConst::new(def_id, args); | ||
|
|
||
| let ct = ty::Const::new_unevaluated(tcx, uneval); | ||
| let const_ = Const::Ty(ty, ct); | ||
| return ConstOperand { user_ty: None, span, const_ }; | ||
| } | ||
|
|
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 think type_const can't be applied to const blocks so this should be unnecessary
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.
Okay easy enough, I can remove that.
| } | ||
| ExprKind::NamedConst { def_id, args, ref user_ty } => { | ||
| let user_ty = user_ty.as_ref().and_then(push_cuta); | ||
| if is_type_const(tcx, def_id) { |
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.
You link the same issue twice in your description, but I think one of them is supposed to be for #150615?
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.
Sorry, I could have been clearer. I mainly meant I noticed a couple different cases of triggering the span bug check in #148729.
The check:
rust/compiler/rustc_hir_typeck/src/lib.rs
Lines 123 to 125 in 32fe406
| let body_id = node.body_id().unwrap_or_else(|| { | |
| span_bug!(span, "can't type-check body of {:?}", def_id); | |
| }); |
I noticed when testing around various permutations of the below I ended up at the same check.
trait BadTr {
const NUM: usize;
}
struct GoodS;
impl BadTr for GoodS {
#[type_const]
const NUM: = 84;
}and
#[type_const] const A: u8 = A;Lastly, looking over that issue you just linked, I think this fixes that too.
on this note it wouldn't surprise me if there are more cases but they can probably all be handled independently (ie different PRs) as each ICE is gonna be a random unrelated part of the compiler which isn't aware of type_consts yet |
This comment was marked as resolved.
This comment was marked as resolved.
Sure, I can get to that to this evening. --Edit-- |
Ah, okay thanks can do. |
idk if it helps but here are some of the query-stacks that lead to this ICE while fuzzing for around 5 mins |
|
@Keith-Cancel yeah the examples from that issue should be added as tests too since theyre fixed by this PR. @matthiaskrgr do you have lines from further up the call stack than the edit: I've been informed those do go past the eval_ methods 😆 |
@matthewjasper Do you have the code or fuzzing script that generated those ices? |
d9b2fec to
28a7809
Compare
|
Some changes occurred to the CTFE machinery Some changes occurred to constck cc @fee1-dead |
This comment has been minimized.
This comment has been minimized.
|
@BoxyUwU So I added some tests like asked. I also did move the is_type_const() function to some where more central so if it needs updated it's in a central location. I am curious what the code the fuzzer generated that found some more ICE. Depending on the cause hopefully it is something I can address. |
|
@BoxyUwU So when playing around and experimenting more with mgca with this fix, I did manage to find one more ICE. Before the fix it's ICEs with It seems in this case the inherent named const has no args. So I made a slight change instead for Code in question: #![expect(incomplete_features)]
#![feature(min_generic_const_args)]
pub struct A;
impl A {
#[type_const]
const LEN: usize = 4;
#[allow(unused_braces)]
fn arr() -> [u8; const { Self::LEN }] {
return [0u8; const { Self::LEN }];
}
}
fn main() {
let _ = A::arr();
} |
| let ct = if args.is_empty() { | ||
| tcx.const_of_item(def_id).instantiate_identity() |
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.
The underlying cause of the ICE you fix by doing this instantiate_identity and no args check is probably that we aren't handling inherent associated consts elsewhere in mGCA. I think we shouldn't handle this here and instead continue letting it ICE and this will get fixed at some point once IACs are handled properly.
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.
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'd file a new issue and label it as min_generic_const_args
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'd file a new issue and label it as min_generic_const_args
Can do.
@BoxyUwU Also should I squash my commits and get it ready for a merge, or should I wait to hear from @matthewjasper since it sounds like he may have found some related ICEs?
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.
If you can squash that would be great. I think there is more ICEs here but those can be follow up PRs and I'd like to get these fixes in tree as not being able to use type_consts in HIR bodies is quite a big flaw in the current impl 😅
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.
If you can squash that would be great. I think there is more ICEs here but those can be follow up PRs and I'd like to get these fixes in tree as not being able to use type_consts in HIR bodies is quite a big flaw in the current impl 😅
Yea definitely makes it hard to use type_consts otherwise.
|
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.
… a body. Handling for inherent associated consts is missing elsewhere, remove so it can be handled later in that handling. Diagnostic not always be emitted on associated constant Add a test case and Fix for a different ICE I encountered. I noticed when trying various permuations of the test case code to see if I could find anymore ICEs. I did, but not one that I expected. So in the instances of the a named const not having any args, insantiate it directly. Since it is likely an inherent assocaiated const. Added tests. Centralize the is_type_const() logic. I also noticed basically the exact same check in other part the code. Const blocks can't be a type_const, therefore this check is uneeded. Fix comment spelling error. get_all_attrs is not valid to call for all DefIds it seems. Make sure that if the type is omitted for a type_const that we don't ICE. Co-Authored-By: Boxy <[email protected]>
|
@BoxyUwU Not sure if you saw, but I did squash it all too one commit and rebase it to main. |
|
@bors r+ rollup |
Fix ICE: can't type-check body of DefId for issue rust-lang#148729 This commit fixes the issue rust-lang#148729 for min_const_generic_args rust-lang#132980. It's pretty small PR. The first commit makes sure that the `type_const`s are made into normal consts in const expressions. The next one just handles the case rust-lang#148729 of where the type of the const was omitted at which point it was trying to treat a `type_const` again as a regular const. That obviously will fail since a type_const does not have a body. @rustbot label +F-associated_const_equality +F-min_generic_const_args +I-ICE
Fix ICE: can't type-check body of DefId for issue rust-lang#148729 This commit fixes the issue rust-lang#148729 for min_const_generic_args rust-lang#132980. It's pretty small PR. The first commit makes sure that the `type_const`s are made into normal consts in const expressions. The next one just handles the case rust-lang#148729 of where the type of the const was omitted at which point it was trying to treat a `type_const` again as a regular const. That obviously will fail since a type_const does not have a body. @rustbot label +F-associated_const_equality +F-min_generic_const_args +I-ICE
Fix ICE: can't type-check body of DefId for issue rust-lang#148729 This commit fixes the issue rust-lang#148729 for min_const_generic_args rust-lang#132980. It's pretty small PR. The first commit makes sure that the `type_const`s are made into normal consts in const expressions. The next one just handles the case rust-lang#148729 of where the type of the const was omitted at which point it was trying to treat a `type_const` again as a regular const. That obviously will fail since a type_const does not have a body. @rustbot label +F-associated_const_equality +F-min_generic_const_args +I-ICE
Rollup of 11 pull requests Successful merges: - #148196 (Implement create_dir_all() to operate iteratively instead of recursively) - #150494 ( Fix dso_local for external statics with linkage) - #150788 (THIR patterns: Replace `AscribeUserType` and `ExpandedConstant` wrappers with per-node data) - #150799 (Fix ICE: can't type-check body of DefId for issue #148729) - #150804 (Remove std_detect_file_io and std_detect_dlsym_getauxval features) - #150852 (std: sys: fs: uefi: Implement File::write) - #150871 (Use f64 NaN in documentation instead of sqrt(-1.0)) - #150878 (Emit an error for linking staticlibs on BPF) - #150911 (Add missing documentation for globs feature) - #150913 (compiler: Forward attributes to eii-expanded macros) - #150916 (Once again, reorganize the EII tests a bit) r? @ghost
Rollup of 12 pull requests Successful merges: - #150947 (alloctests: Don't run the longer partial-sort tests under Miri) - #148196 (Implement create_dir_all() to operate iteratively instead of recursively) - #150494 ( Fix dso_local for external statics with linkage) - #150788 (THIR patterns: Replace `AscribeUserType` and `ExpandedConstant` wrappers with per-node data) - #150799 (Fix ICE: can't type-check body of DefId for issue #148729) - #150804 (Remove std_detect_file_io and std_detect_dlsym_getauxval features) - #150852 (std: sys: fs: uefi: Implement File::write) - #150871 (Use f64 NaN in documentation instead of sqrt(-1.0)) - #150878 (Emit an error for linking staticlibs on BPF) - #150911 (Add missing documentation for globs feature) - #150913 (compiler: Forward attributes to eii-expanded macros) - #150916 (Once again, reorganize the EII tests a bit) r? @ghost
Rollup merge of #150799 - mcga, r=BoxyUwU Fix ICE: can't type-check body of DefId for issue #148729 This commit fixes #148729 for min_const_generic_args #132980. It's pretty small PR. The first commit makes sure that the `type_const`s are made into normal consts in const expressions. The next one just handles the case #148729 of where the type of the const was omitted at which point it was trying to treat a `type_const` again as a regular const. That obviously will fail since a type_const does not have a body. @rustbot label +F-associated_const_equality +F-min_generic_const_args +I-ICE
This commit fixes #148729 for min_const_generic_args #132980.
It's pretty small PR. The first commit makes sure that the
type_consts are made into normal consts in const expressions.The next one just handles the case #148729 of where the type of the const was omitted at which point it was trying to treat a
type_constagain as a regular const. That obviously will fail since a type_const does not have a body.@rustbot label +F-associated_const_equality +F-min_generic_const_args +I-ICE