When matching on enums, read the discriminant even if there's only a single variant

[meithecatte]

Synopsis

This PR removes a special case, so that matching on enums that only have a single variant still emits a discriminant read in MIR. This is motivated by some weirdness being caused by this special case, as documented in #147722 and #151786 and summarized below.

The PR is a follow-up to #150681, where it has been decided that this change should be split into a separate PR.

Motivation

Consider the following example (playground):

#![allow(dead_code)]
use core::mem::{size_of, transmute};

#[repr(u8)]
enum Inner {
    X(u8),
}

enum Outer {
    A(Inner),
    B(u8),
}

fn main() {
    assert_eq!(size_of::<Inner>(), 2);
    assert_eq!(size_of::<Outer>(), 2);
    let x = Outer::B(42);
    match unsafe { transmute::<&Outer, &Inner>(&x) } {
        Inner::X(v) => {
            println!("{v}");
        }
    }
}

Here, we have a single-variant enum Inner, which we're forcing to have a discriminant with #[repr(u8)], thus creating a niche that otherwise wouldn't be there. Then we create a reference to Inner with an invalid discriminant. Thus, you would expect that matching on this reference should trigger UB. However, currently matching on a single-variant enum does not read the discriminant at all, and so Miri accepts this example.

The weirdness doesn't end here, however. If you add a #[non_exhaustive] attribute to the Inner enum, Miri starts detecting UB (playground). This is because if an enum is #[non_exhaustive], we must treat it just as if it already had multiple variants, to prevent semver hazards (and following #150681, we do this even for non_exhaustive enums defined in the current crate, to avoid having the operational semantics depend on which crate the code is in).

In short, we are in a position where the #[non_exhaustive] attribute can introduce UB – though arguably one that should already be there regardless.

Further semver compatibility hazards

Whether the read of the discriminant happens or not can also be observed through means other than Miri, due to the Rust 2021 closure capturing behavior (playground):

// #[non_exhaustive] // try uncommenting this
pub enum Inner {
    X(u8, String),
}

pub fn f(x: Inner) -> impl FnOnce() {
    // if Inner is #[non_exhaustive], the discriminant read causes us to capture
    // the entire enum, and because we consume the String that's inside, we capture
    // the enum by value. Thus, the closure is 'static.
    //
    // On the other hand, if the enum *isn't* marked as #[non_exhaustive], then
    // we capture the two fields separately: `a` gets captured by reference,
    // while `b` is captured by value. Thus, the closure is only valid within
    // this function and cannot be returned.
    || {
        match x {
            Inner::X(a, b) => drop((a, b)),
        }
    }
}

This means that removing a #[non_exhaustive] attribute can cause some other code to stop passing the borrow checker, which is quite surprising and unintuitive.

Proposed changes

This PR makes it so that the MIR includes a read of the discriminant whenever we match on an enum – even if the enum happens to have a single variant (and therefore the discriminant is probably zero-sized).

This removes #[non_exhaustive] from opsem considerations, resolves the unintuitive compatibility interactions, and adds UB where you would expect it.

Fixes #147722.
Fixes #151786.
Fixes rust-lang/cargo#16417.

Other considerations

From a previous attempt at this change, we have had a couple of regressions on crater – though some of those already got addressed. The pattern that occurs in practice is a single-variant field-less enum that wasn't getting captured at all, and now becomes captured. A preliminary glance suggests that none of the issues found by the previous crater run should be hard to fix, however.

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in match lowering

cc @Nadrieril

[rustbot]

r? @mati865

rustbot has assigned @mati865.
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 69 candidates
• Random selection from 11 candidates

[RalfJung]

This seems like it could use revisions to avoid duplicating the code? Same for "single_variant_uninit".

[meithecatte]

Ah, I wasn't aware that revisions are a thing! I would've done that in the first place had I known about it.

[RalfJung]

r? @Nadrieril

Will also need t-lang discussion. Is the writeup up-to-date for that? Remember to make it self-contained and focused on the surface-language-level impact. I would suggest to lead with the example, add some comments to explain what happens, explain why it should be UB, and then move towards how this PR makes it UB.

[rustbot]

Nadrieril is not on the review rotation at the moment.
They may take a while to respond.

[mati865]

Maybe nominate it for the discussion? Or are you waiting on the author's response?

[meithecatte]

@RalfJung I assume your question implies that the current PR description isn't sufficiently self-contained – indeed, I think I did a little "curse of knowledge" thing regarding this because in the few percent of my time that I've spent working on rustc, this tiny corner of the language is all I've been thinking of across the last year 😅

I'll rewrite the description with T-lang in mind.

[meithecatte]

T-lang nomination

This PR changes matching logic to always read the discriminant of the enum being matched, even of it only has one variant. This results in more consistent behavior and makes #[non_exhaustive] irrelevant wrt opsem considerations. See PR description for examples and motivation.

@rustbot label +I-lang-nominated

[RalfJung]

Thanks!
Should we re-do a crater run? Maybe just for the crates that were found to be problematic last time?

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: bbf43bc (bbf43bc9d5c8ad2a2e38c9004e5a59b269820b3b, parent: c2efcc4ae006a6b2761cb42572fc9cee0d1ce4af)

[RalfJung]

The first run here was the version of that PR that also included this change, right?

@craterbot run mode=build-and-test crates=https://crater-reports.s3.amazonaws.com/pr-150681/retry-regressed-list.txt p=1

[craterbot]

👌 Experiment pr-154756 created and queued.
🤖 Automatically detected try build bbf43bc
🔍 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

[meithecatte]

Yeah, this PR is pretty much just a rebase of that version of the previous PR.

[craterbot]

🚧 Experiment pr-154756 is now running

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

[craterbot]

🎉 Experiment pr-154756 is completed!
📊 42 regressed and 39 fixed (3788 total)
📊 293 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-154756/retry-regressed-list.txt ↩

[joshtriplett]

(We looked at this in today's @rust-lang/lang meeting, but we're going to wait to look further until there's an analysis of the crater results.)

[RalfJung]

@meithecatte could you look at those results (once you have time) to see which true positives remain, and possibly submit PRs to the affected crates?