[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

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

[lcnr]

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

I wouldn't expect to happen too often. New query edges which may cycle mean there's a cyclic dependency going on, which is valuable to look into as e.g. the fact that there's cyclic reasoning between trait solving and check_item_well_formed is actually the root of an unsoundness: #135246

It does feel more T-types than T-lang to me. Or well, more accurately, it's enough of an implementation detail that I feel kinda iffy about having T-lang be responsible here and would like it to be delegated to a subteam. Due to a lack of better choices, I guess that subteam is T-types here 🤣

[RalfJung]

When I change code in const-eval I am usually not aware if whatever I call adds a new query edge, let alone if that can cause cycles.

[rust-rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

[sjwang05]

@Kivooeo when you have the time, could you take a look at this PR again? I think it should be ready to merge, though I'm not sure. sorry for the ping, and thanks! :D

[Kivooeo]

@bors r+ rollup

[rust-bors]

📌 Commit 88d314a has been approved by Kivooeo

It is now in the queue for this repository.