Layout and MIR field accesses are incoherent

[RalfJung]

I propose that we should have the following invariant: Whenever, anywhere in MIR, there is a field projection with some index i, then i is actually a valid field index in the (statically known) layout of the type being projected into.

Right now, this is not the case. This code:

pub enum Void {}

enum UninhabitedUnivariant { _Variant(Void), }

fn main() {
    let _seed: UninhabitedUnivariant = None.unwrap();
    match _seed {
        UninhabitedUnivariant::_Variant(_x) => {}
    }
}

generates the following MIR:

    bb1: {
        StorageDead(_2);                 // bb1[0]: scope 0 at src/main.rs:6:52: 6:53
        StorageLive(_3);                 // bb1[1]: scope 1 at src/main.rs:8:41: 8:43
        _3 = move ((_1 as _Variant).0: Void); // bb1[2]: scope 1 at src/main.rs:8:41: 8:43
        StorageDead(_3);                 // bb1[3]: scope 1 at src/main.rs:9:5: 9:6
        StorageDead(_1);                 // bb1[4]: scope 0 at src/main.rs:10:1: 10:2
        return;                          // bb1[5]: scope 0 at src/main.rs:10:2: 10:2
    }

but the layout of _Variant is that of a union with 0 fields.

A consequence of this incoherence is that all codegen backends and Miri (and possibly more MIR consumers) all need to be on their guard when considering field projections, always protecting against the case where the projection is ill-formed (usually that happens by special-casing uninhabited types before considering the projection).

I think instead of putting that burden on every MIR consumer, we should fix either the layout or the MIR to not violate the invariant in the first place.

Cc @eddyb with whom I anyway recently talked about those odd Union layouts. Is there some fundamental issue with the invariant I am proposing? Also Cc @oli-obk @wesleywiser

If we decide to go with this, we should

• Revert Do not ICE when matching an uninhabited enum's field #69753
• Check for other cases in Miri/const-prop that just work around such broken MIR/layout
• Check for the corresponding cases in codegen and other MIR consumers -- mostly, add assertion here.

[eddyb]

I wrote #69753 (comment), before taking another look at the code, which revealed the problem:

So this is the relevant code, that (I thought) would kick in and compute the right number of fields:

rust/src/librustc/ty/layout.rs

Lines 2025 to 2044 in 4a1b69d

	Variants::Single { index } => {
	// Deny calling for_variant more than once for non-Single enums.
	if let Ok(layout) = cx.layout_of(this.ty).to_result() {
	assert_eq!(layout.variants, Variants::Single { index });
	}
	let fields = match this.ty.kind {
	ty::Adt(def, _) => def.variants[variant_index].fields.len(),
	_ => bug!(),
	};
	let tcx = cx.tcx();
	tcx.intern_layout(LayoutDetails {
	variants: Variants::Single { index: variant_index },
	fields: FieldPlacement::Union(fields),
	abi: Abi::Uninhabited,
	largest_niche: None,
	align: tcx.data_layout.i8_align,
	size: Size::ZERO,
	})
	}

However, just before it we have:

rust/src/librustc/ty/layout.rs

Line 2023 in 4a1b69d

Variants::Single { index } if index == variant_index => this.details,

So what's happening is the first variant of every uninhabited enum will get confused with the enum as a whole (which right now has the layout of !, i.e. fields: Union(0)).

Removing that line will probably fix this, but I haven't tried it.
EDIT: removing would break inhabited univariant enums, it should be made conditional on fields != Union(0) instead (i.e. add that to the existing match guard of that arm).

[RalfJung]

Glad to hear that we agree on this invariant. :)

We fixed this months ago. I don't understand why this is a problem again now.

Doesn't the fact that codegen, borrowck and so on didn't ICE here indicate that they all still contain some "is_uninhabited" check that ought to be unnecessary? If we can find and remove that check, it would mean we'd see such issues much earlier (we'd see them on any MIR statement that codegen runs on, not just any statement that const-prop runs on).

[eddyb]

We fixed this months ago. I don't understand why this is a problem again now.

I was wrong when I said that, in that it's still broken for variant index 0.

they all still contain some "is_uninhabited" check that ought to be unnecessary?

I'm not sure I could see how that factors into this too much.
A lot of stuff skips dead code (for good reason), so it would be hard to get good coverage.

---

In case you didn't see it, I edited #69763 (comment) with the right fix, AFAICT.

Long-term we'll want to change the layout to use Variants::Multiple more uniformly, and have:

• a () DiscriminantKind for univariant enums
• a ! DiscriminantKind for uninhabited enums

(we'd probably have to move discr: Scalar into DiscriminantKind::{Tag,NicheFilling})

[RalfJung]

So looks like #66250 removed the assertion that demonstrates the incoherence, instead of fixing the issue -- and no issue was opened to track the problem so it just got forgotten until someone re-discovered the ICE. :(

[RalfJung]

The fixes for asserting field access sanity in Miri and codegen have landed.
I am not aware of any other such hacks in Miri (where we "work around" weird layout). There is the issue with SetDiscriminant and uninhabited variants bailing out early, but I got convinced some time ago that that is just needed -- and saying "SetDiscriminant for an uninhabited variant is insta-UB" makes enough sense. I just went over place.rs and operand.rs and everything seems good.

So, I think this can be closed. :)

[eddyb]

@RalfJung What about the second half of #69763 (comment)? Right now the layouts for enums with 0 or 1 present (inhabited or with non-ZST fields) variants are pretty artificial, and I think they should be fixed.

[RalfJung]

Agreed, but that sounds like a separate issue to me? At least there aren't field accesses to variants that don't exist, or stuff like that.

Would that proposal also help with SetDiscriminant, i.e. would we still need to bail out explicitly and early for uninhabited variants?

[eddyb]

would we still need to bail out explicitly and early for uninhabited variants

If the whole enum is uninhabited, it would be an exhaustive match like Tag vs NicheFilling today.

If a specific variant is uninhabited, you could probably let the regular discriminant writing code handle it, although there's always the risk that the way that code is implemented would write a different variant's tag/niche (e.g. by truncation, if the uninhabited variant is just outside).

Agreed, but that sounds like a separate issue to me?

Hmm I thought you wanted to keep this open until layout reflected what MIR could access more directly, instead of faking it with layout_of(Enum) = layout_of(!) etc.

[RalfJung]

I was mostly concerned with fields for this issue, not variants. I guess I got used to variants being a mess. ;) None of the text in the OP makes sense for the variant case though, I think?

And then there's also the thing where primitives are "fieldless unions", and maybe instead there should be FieldPlacement::Atom or so to indicate that there are no fields (but there is still data).

[eddyb]

I was mostly concerned with fields for this issue, not variants.

Variants are what contain fields. The reason for any oddities with fields is because the variants they're in aren't actually in the layout so they have to be faked on the spot (which is what I think we should fix).

[RalfJung]

Okay, I made a new issue for that: #70399