Avoid computing layout of enums with non-int discriminants#157562
Avoid computing layout of enums with non-int discriminants#157562sjwang05 wants to merge 1 commit into
Conversation
|
This PR changes a file inside |
|
r? @Kivooeo rustbot has assigned @Kivooeo. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
I don't expect the loop to impact perf significantly, since later on in the same function we construct an iterator by calling |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Avoid computing layout of enums with non-int discriminants
This comment has been minimized.
This comment has been minimized.
183e882 to
0ba35a5
Compare
This comment was marked as resolved.
This comment was marked as resolved.
0ba35a5 to
88d314a
Compare
|
Thanks for pointing that out, the previous version caused a cycle error, so now we just check the cached typeck results to see if those have errors instead of trying to const-eval the discriminant. |
This comment has been minimized.
This comment has been minimized.
|
This PR is still technically a breaking change, since now the following code fails to compile: pub trait Trait {
type Assoc;
}
impl Trait for [u8; 0] {
type Assoc = isize;
}
pub enum Foo {
One = unsafe { std::mem::zeroed::<<[u8; size_of::<Foo>()] as Trait>::Assoc>() },
}Error with this PR |
|
Might be an acceptable breaking change? Not sure. Would require a crater run if we go ahead with this. |
|
Finished benchmarking commit (023d36e): comparison URL. Overall result: no relevant changes - 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 countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)This perf run didn't have relevant results for this metric. CyclesResults (primary -2.2%, secondary -3.3%)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.705s -> 515.476s (0.15%) |
Currently, enums with explicit non-int discriminants, such as
1..=10, correctly error with E0308 and E0732 during typeck. However,layout_ofnever sees this error, sinceAdtDef::discriminants()falls back to a default value wheneval_explicit_discrreturns anErr, causing the layout of the enum to be broken. If this enum then appears in a promoted, we expect CTFE to fail; however, sincelayout_oftechnically "succeeded," just with a broken layout, our enum isn't inallowed_in_infallible, so CTFE expects it to succeed. CTFE failing then causes us to ICE with"interpret const eval failure of...which is not in required_consts".This PR modifies
layout_of_uncachedincompiler/rustc_ty_utils/src/layout.rsto evaluate all explicit enum discriminants before computing layout of the type, and early-returns with aLayoutError::ReferncesErrorif eval fails. CTFE then sees thisLayoutErrorasallowed_in_infallible, avoiding the ICE.fixes #138660