Do not ICE when matching an uninhabited enum's field

[pnkfelix]

Fix #69191

[rust-highfive]

r? @davidtwco

(rust_highfive has picked a reviewer for you, use r? to override)

[Centril]

cc @RalfJung

[pnkfelix]

The reason the test is written in that manner is that I had played with a few other methods of fixing this bug; one in particular messed around with the logic here:

rust/src/librustc/ty/layout.rs

Lines 781 to 795 in b818ccc

	let present_first = match present_first {
	present_first @ Some(_) => present_first,
	// Uninhabited because it has no variants, or only absent ones.
	None if def.is_enum() => return tcx.layout_raw(param_env.and(tcx.types.never)),
	// if it's a struct, still compute a layout so that we can still compute the
	// field offsets
	None => Some(VariantIdx::new(0)),
	};
	let is_struct = !def.is_enum() ||
	// Only one variant is present.
	(present_second.is_none() &&
	// Representation optimizations are allowed.
	!def.repr.inhibit_enum_layout_opt());
	if is_struct {

where I was trying to replace the def.is_enum() logic in the match with something that instead checked !is_struct from the boolean defined below that match.

Long story short, that approach didn't work; but when I was working on it, I was worried enough about the handling of #[repr(C)] and #[repr(<int-type>)] that I wanted explicit test variants that checked those cases.

[pnkfelix]

Either @oli-obk or @eddyb would be suitable candidates for reviewing this.

[pnkfelix]

(also, the reason that the code doesn't just unconditionally throw_ub! even for fields that are within range is that doing so would break a regression test from #64506.)

I'm sure there is a better solution to this problem, in terms of handling the scenario in a more disciplined manner. I laid out three different choices on zulip, but this is the one I felt most confident about being good enough to backport to beta in the short term. (Because all it does it injects a throw_ub! invocation immediately before something that would otherwise cause the compiler to ICE.)

[oli-obk]

@bors r+

[bors]

📌 Commit 9712fa4 has been approved by oli-obk

[RalfJung]

bors r+

FWIW, I don't think this is the right fix. At the very least we should add a comment saying FIXME temporary hack to work around incoherence between layout computation and MIR building.

In other words, I consider it quite reasonable to ICE when MIR building says "get field 0 of that variant", but consulting that variant's layout says "nope there are no fields here". This also has to cause issues in codegen, which likely already carries a comparable hack. So is every backend supposed to separately carry hacks working around what is ultimately an issue in the middle-end?

[oli-obk]

It is not the correct fix, I agree, but this is getting into stable soon. I am opening a workspace locally later today to start working on a real fix

[oli-obk]

@bors r+

[bors]

📌 Commit 40809b0 has been approved by oli-obk

[eddyb]

@bors rollup=never (just to make it easier to cancel this PR if we get a different fix going)

[eddyb]

@bors r- (in favor of #69768)

[bors]

💡 This pull request was already approved, no need to approve it again.

• There's another pull request that is currently being tested, blocking this pull request: delay_span_bug when codegen cannot select obligation #69614

[bors]

📌 Commit b4422fb has been approved by oli

[bors]

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

[pnkfelix]

Also, assuming this does end up in nightly, its fine for it to be part of a rollup.

[pnkfelix]

@bors rollup+

[pnkfelix]

@bors p=9

[nikomatsakis]

Discussed in today's design meeting, decided to accept this change for beta backport.

[pietroalbini]

@bors treeclosed-

[bors]

⌛ Testing commit b4422fb with merge 24b67c42c7ed3b589eb21be5d8ead7f646a16038...

[rust-highfive]

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

[bors]

💔 Test failed - checks-azure

[pietroalbini]

@bors retry

[bors]

⌛ Testing commit b4422fb with merge 2890b37...

[bors]

☀️ Test successful - checks-azure
Approved by: oli
Pushing 2890b37 to master...