Avoid computing layout of enums with non-int discriminants

[sjwang05]

View all comments

Currently, enums with explicit non-int discriminants, such as 1..=10, correctly error with E0308 and E0732 during typeck. However, layout_of never sees this error, since AdtDef::discriminants() falls back to a default value when eval_explicit_discr returns an Err, causing the layout of the enum to be broken. If this enum then appears in a promoted, we expect CTFE to fail; however, since layout_of technically "succeeded," just with a broken layout, our enum isn't in allowed_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_uncached in compiler/rustc_ty_utils/src/layout.rs to evaluate all explicit enum discriminants before computing layout of the type, and early-returns with a LayoutError::ReferncesError if eval fails. CTFE then sees this LayoutError as allowed_in_infallible, avoiding the ICE.

fixes #138660

[rustbot]

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

[rustbot]

r? @Kivooeo

rustbot has assigned @Kivooeo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 73 candidates
• Random selection from 20 candidates

[sjwang05]

I don't expect the loop to impact perf significantly, since later on in the same function we construct an iterator by calling eval_explicit_discr via AdtDef::discriminants(), which later on in layout_of_enum gets eagerly consumed into a BTreeSet. Calling this loop now just means those const_eval_poly results are cached, hence we avoid duplicating too much work. A perf run may still be warranted though?

[Kivooeo]

@bors try @rust-timer queue

[sjwang05]

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.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 023d36e (023d36e8abdd31106a2c96f6803147c3dbac7b1c, parent: 61d7280f3c4c63fa24c56bdaa9a446151b5a30dc)

[theemathas]

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

error[E0391]: cycle detected when type-checking `Foo::One::{constant#0}`
  --> src/lib.rs:10:45
   |
10 |     One = unsafe { std::mem::zeroed::<<[u8; size_of::<Foo>()] as Tra...
   |                                             ^^^^^^^^^^^^^^^^
   |
note: ...which requires evaluating type-level constant...
  --> src/lib.rs:10:45
   |
10 |     One = unsafe { std::mem::zeroed::<<[u8; size_of::<Foo>()] as Tra...
   |                                             ^^^^^^^^^^^^^^^^
note: ...which requires const-evaluating + checking `Foo::One::{constant#0}::{constant#0}`...
  --> src/lib.rs:10:45
   |
10 |     One = unsafe { std::mem::zeroed::<<[u8; size_of::<Foo>()] as Tra...
   |                                             ^^^^^^^^^^^^^^^^
note: ...which requires const-evaluating + checking `Foo::One::{constant#0}::{constant#0}`...
  --> library/core/src/mem/mod.rs:374:4
note: ...which requires simplifying constant for the type system `core::mem::SizedTypeProperties::SIZE`...
  --> library/core/src/mem/mod.rs:1379:4
note: ...which requires const-evaluating + checking `core::mem::SizedTypeProperties::SIZE`...
  --> library/core/src/mem/mod.rs:1379:4
note: ...which requires computing layout of `Foo`...
  --> src/lib.rs:9:1
   |
 9 | pub enum Foo {
   | ^^^^^^^^^^^^
   = note: ...which again requires type-checking `Foo::One::{constant#0}`, completing the cycle
note: cycle used when match-checking `Foo::One::{constant#0}`
  --> src/lib.rs:10:11
   |
10 | ... = unsafe { std::mem::zeroed::<<[u8; size_of::<Foo>()] as Trait>::Assoc>() },
   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: see https://rustc-dev-guide.rust-lang.org/overview.html#queries and https://rustc-dev-guide.rust-lang.org/query.html for more information

For more information about this error, try `rustc --explain E0391`.
error: could not compile `foo` (lib) due to 1 previous error

[theemathas]

Might be an acceptable breaking change? Not sure. Would require a crater run if we go ahead with this.

[rust-timer]

Finished benchmarking commit (023d36e): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking 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 count

This 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.

Cycles

Results (primary -2.2%, secondary -3.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.2%	[-2.2%, -2.2%]	1
Improvements ✅ (secondary)	-3.3%	[-3.3%, -3.3%]	1
All ❌✅ (primary)	-2.2%	[-2.2%, -2.2%]	1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 514.705s -> 515.476s (0.15%)
Artifact size: 401.25 MiB -> 400.75 MiB (-0.13%)

[sjwang05]

Thanks, it turns out that this simpler version causes the same error with this PR for the same reason:

use std::mem::size_of;

pub enum Foo {
    One = size_of::<[u8; size_of::<Foo>()]>() as isize,
}

When type-checking One = size_of::<[u8; size_of::<Foo>()]>() as isize, we first need to resolve the type [u8; size_of::<Foo>()], which requires computing the layout of Foo in order to evaluate size_of, which triggers typeck on Foo's discriminants, which...etc.

I looked into why this doesn't cause the same query cycles on stable/nightly, and it turns out this code, with 2 variants instead of 1, doesn't compile on both current stable and nightly:

use std::mem::size_of;

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>() },
    Two,
}

stable playground link
nightly

It seems like the reason why this happens for 2+ variants but not 1 is because the discriminants() iterator only gets consumed when there are 2+ variants; when there is only 1 variant, the enum gets treated like a struct by layout computation, so its discriminants are never evaluated during layout. So looks like this kind of cycle error is a preexisting issue, just that this PR broadens the impact to univariant enums as well and probably makes it harder to fix in the future.

I do definitely think a crater run is still warranted.

[theemathas]

Preparing for crater run.

@bors try

[rust-bors]

💥 Test timed out after 21600s

[Kivooeo]

Preparing for crater run.

But it was done above 🤔

[Kivooeo]

let's start with check-only, i don't think if this can change runtime behaviour. (if it can please rerun with right mode or tell me about it i will rerun)

@craterbot check

[craterbot]

👌 Experiment pr-157562 created and queued.
🤖 Automatically detected try build 023d36e
⚠️ Try build based on commit 183e882, but latest commit is 88d314a. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[theemathas]

@Kivooeo The crater run you started is based on a previous version of the PR, not the latest version. Here's the diff of the two versions.

@craterbot cancel

@bors try

[craterbot]

🗑️ Experiment pr-157562 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[Kivooeo]

@Kivooeo The crater run you started is based on a previous version of the PR, not the latest version. Here's the diff of the two versions.

oh! didn't knew it works like this, TIL

[rust-bors]

☀️ Try build successful (CI)
Build commit: b946eda (b946edaa85de45f124065f52b1bc55ba2fb4f9fb, parent: 029c9e18dd1f4668e1d42bb187c1c263dfe20093)

[theemathas]

@craterbot check

[craterbot]

👌 Experiment pr-157562 created and queued.
🤖 Automatically detected try build b946eda
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[theemathas]

@craterbot cancel

See #157814

[craterbot]

🗑️ Experiment pr-157562 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more