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 typeck all explicit enum discriminants before computing layout, 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

[craterbot]

🚧 Experiment pr-157562 is now running

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

[craterbot]

🎉 Experiment pr-157562 is completed!
📊 0 regressed and 0 fixed (14695 total)
📊 623 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-157562/retry-regressed-list.txt ↩

[Kivooeo]

Given that there's no regressions in crater I think it should be fine to merge as-is? Or it still needs an FCP (t-lang for example)?

[sjwang05]

I think it'd be fine to merge as-is? Since the multiple-discr case is already broken with the same cycle error; this PR just makes it happen for the single-discr case as well. But I'm not sure either, maybe someone from t-lang or something might think differently.

[traviscross]

This makes sense to me. We're signing off on the breaking change (with no observed breakage). Anyone else want in on this FCP? (E.g., @rust-lang/types, @rust-lang/opsem.) Let me know if so and I'll propose again.

@rfcbot fcp merge lang

[rust-rfcbot]

@traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[oli-obk]

While I think as a pure consistency of the impl thing without real interesting language implications this should just have been a types FCP, I think it's a no-brainer so let's just land it.

[BoxyUwU]

I haven't sorted my thoughts here but the vibes feel off to me about doing a lang FCP for having a new edge between two queries in the compiler. I would expect that we wind up introducing new query edges all the time in ways that might cause additional query cycles (though I'm not sure...) and doing a lang FCP every time that happens seems like a lot 🤔

Similarly I don't think it makes sense for types or opsem to be on this FCP? There isn't really anything type system or opsem related, the only thing that's actually being signed off on is the (zero) breakage caused by calling a query at an earlier point in the compiler.

This feels like the kind of thing I would be comfortable just passing off to pretty much any T-compiler reviewer 🤔

[scottmcm]

Happy to say that trickiness like this with size_of we can break in this case, so

@rfcbot reviewed

---

That said, I'm definitely sympathetic to whether there's actually a spec-level question here. I can't imagine that which queries are triggered when and exactly which things trigger cycle errors is something that we're writing into the spec? If so, then maybe there isn't a FCP thing? But also if there's a known breakage caused there should be anyway?

Torn. I guess if nothing else just having the FCP so people are warning in TWiR is good.

[nikomatsakis]

@rfcbot reviewed

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[nikomatsakis]

I can't imagine that which queries are triggered when and exactly which things trigger cycle errors is something that we're writing into the spec?

So I agree that queries are not part of the spec. I do think that "what requires what" at some level should be part of the spec, in so far as it accepts what programs are accepted.

[joshtriplett]

@rfcbot reviewed

[tmandry]

The way I see the FCP is that we are approving a change to the language spec, even though that section is currently imaginary (the language is underspecified by the reference). I think the simplest model is one where lang, or one of its delegated subteams, approve those changes.

This is consistent with how the types team operates, because they have a dual lang/compiler role. On the lang side, they manage a jagged subset of the design space where the only specification we have is the implementation. In creating the team, lang delegated to their best judgment how to adapt that implementation toward a smoother language over time, one we can better reason about and specify in the long run. I expect that as that continues, they will naturally start to own parts of the reference text.