Remove the single-variant enum special case in pattern matching

[meithecatte]

The question of "when does matching an enum against a pattern of one of its variants read its discriminant" is currently an underspecified part of the language, causing weird behavior around borrowck, drop order, and UB.

Of course, in the common cases, the discriminant must be read to distinguish the variant of the enum, but currently the following exceptions are implemented:

1. If the enum has only one variant, we currently skip the discriminant read.

   • This has the advantage that single-variant enums behave the same way as structs in this regard.

   • However, it means that if the discriminant exists in the layout, we can't say that this discriminant being invalid is UB. This makes me particularly uneasy in its interactions with niches – consider the following example (playground), where miri currently doesn't detect any UB (because the semantics don't specify any):

     Example 1

     #![allow(dead_code)]
     use core::mem::{size_of, transmute};
     
     #[repr(u8)]
     enum Inner {
         X(u8),
     }
     
     enum Outer {
         A(Inner),
         B(u8),
     }
     
     fn f(x: &Inner) {
         match x {
             Inner::X(v) => {
                 println!("{v}");
             }
         }
     }
     
     fn main() {
         assert_eq!(size_of::<Inner>(), 2);
         assert_eq!(size_of::<Outer>(), 2);
         let x = Outer::B(42);
         let y = &x;
         f(unsafe { transmute(y) });
     }

2. For the purpose of the above, enums with marked with #[non_exhaustive] are always considered to have multiple variants when observed from foreign crates, but the actual number of variants is considered in the current crate.

   • This means that whether code has UB can depend on which crate it is in: Single-variant exception for match consequences for #[non_exhaustive] #147722
   • In another case of #[non_exhaustive] affecting the runtime semantics, its presence or absence can change what gets captured by a closure, and by extension, the drop order: Single-variant exception for match consequences for #[non_exhaustive] #147722 (comment)
   • Also at the above link, there is an example where removing #[non_exhaustive] can cause borrowck to suddenly start failing in another crate.

3. Moreover, we currently make a more specific check: we only read the discriminant if there is more than one inhabited variant in the enum.

   • This means that the semantics can differ between foo<!>, and a copy of foo where T was manually replaced with !: Adding generics affect whether code has UB or not, according to Miri #146803

   • Moreover, due to the privacy rules for inhabitedness, it means that the semantics of code can depend on the module in which it is located.

   • Additionally, this inhabitedness rule is even uglier due to the fact that closure capture analysis needs to happen before we can determine whether types are uninhabited, which means that whether the discriminant read happens has a different answer specifically for capture analysis.

   • For the two above points, see the following example (playground):

     Example 2

     #![allow(unused)]
     
     mod foo {
         enum Never {}
         struct PrivatelyUninhabited(Never);
         pub enum A {
             V(String, String),
             Y(PrivatelyUninhabited),
         }
         
         fn works(mut x: A) {
             let a = match x {
                 A::V(ref mut a, _) => a,
                 _ => unreachable!(),
             };
             
             let b = match x {
                 A::V(_, ref mut b) => b,
                 _ => unreachable!(),
             };
         
             a.len(); b.len();
         }
         
         fn fails(mut x: A) {
             let mut f = || match x {
                 A::V(ref mut a, _) => (),
                 _ => unreachable!(),
             };
             
             let mut g = || match x {
                 A::V(_, ref mut b) => (),
                 _ => unreachable!(),
             };
         
             f(); g();
         }
     }
     
     use foo::A;
     
     fn fails(mut x: A) {
         let a = match x {
             A::V(ref mut a, _) => a,
             _ => unreachable!(),
         };
         
         let b = match x {
             A::V(_, ref mut b) => b,
             _ => unreachable!(),
         };
     
         a.len(); b.len();
     }
     
     
     fn fails2(mut x: A) {
         let mut f = || match x {
             A::V(ref mut a, _) => (),
             _ => unreachable!(),
         };
         
         let mut g = || match x {
             A::V(_, ref mut b) => (),
             _ => unreachable!(),
         };
     
         f(); g();
     }

In light of the above, and following the discussion at #138961 and #147722, this PR makes it so that, operationally, matching on an enum always reads its discriminant.

Note that this is a breaking change, due to the aforementioned changes in borrow checking behavior, new UB (or at least UB newly detected by miri), as well as drop order around closure captures. However, it seems to me that the combination of this PR with #138961 should have smaller real-world impact than #138961 by itself.

Fixes #142394
Fixes #146590
Fixes #146803 (though already marked as duplicate)
Fixes #147722
Fixes rust-lang/miri#4778
Fixes rust-lang/cargo#16417

r? @Nadrieril @RalfJung

@rustbot label +A-closures +A-patterns +T-opsem +T-lang

[rustbot]

Some changes occurred in match lowering

cc @Nadrieril

The Miri subtree was changed

cc @rust-lang/miri

[rustbot]

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

[meithecatte]

Looks like I messed up the syntax slightly 😅

r? @RalfJung

[meithecatte]

Ah, it's just that you can only request a review from one person at a time. Makes sense. I trust that the PR will make its way to the interested parties either way, but just in case, cc @theemathas and @traviscross, who were involved in previous discussions.

Preparing a sister PR to the Rust Reference is on my TODO list.

[RalfJung]

I'm also not really on the review rotation, so I won't be able to do the lead review here -- sorry. I'll try to take a look at the parts relevant to Miri, but the match lowering itself is outside my comfort zone.

@rustbot reroll

[Zalathar]

For the tests that need to be adjusted, is it possible to make the adjustments before the main changes, and still have the tests pass?

That can be a nicer approach, if it's viable.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[meithecatte]

For the tests that need to be adjusted, is it possible to make the adjustments before the main changes, and still have the tests pass?

That can be a nicer approach, if it's viable.

Depends on which tests we're talking about. The FileCheck changes in mir-opt/unreachable_enum_branching.rs could be turned into a separate commit that'd pass when put first. For the rest of the changes in mir-opt and codegen-llvm, it is at the very least an iffy idea, as it is at the very least quite difficult to perform the canonicalization that's necessary within FileCheck.

As for the changes in tests/ui, it is also not viable, because the semantics are being changed and the tests reflect that.

[craterbot]

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

[craterbot]

🚧 Experiment pr-150681 is now running

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

[traviscross]

We talked about this in the lang call today.

For my part, to move this along, I'm interested in separating this into two:

1. Removing the same-crate/different-crate distinction on non-exhaustive enums: matching on a non-exhaustive enum should always do a discriminant read.
2. Doing a discriminant read when matching on an exhaustive enum with one non-empty variant.

The worst flaw of the current behavior is that the opsem is crate dependent. Doing the first thing fixes that. And it makes sense conceptually. Marking an enum as non-exhaustive should cause the enum to act as if there may be other variants. There's no good reason this as if rule shouldn't apply to the defining crate too.

The second one, for me, is less of a clear call. If an enum is exhaustive, then its structure has been committed to (even if not exported, this amounts to an internal commitment, for the moment, within the crate). There's a reasonableness to the programmer and the compiler relying on the details of this structure. And relying on this structure allows for more code that morally should compile to compile due to how this affects borrow checker behavior.

This does then unfortunately mean that removing #[non_exhaustive] from a public enum is a breaking change. That indeed could be a bit surprising, but we have a number of surprising SemVer hazards. Maybe what we buy in trade for this is worth it.

In any case, I'd like to break these questions apart so we can consider them separately.

[RalfJung]

The second one, for me, is less of a clear call. If an enum is exhaustive, then its structure has been committed to (even if not exported, this amounts to an internal commitment, for the moment, within the crate). There's a reasonableness to the programmer and the compiler relying on the details of this structure. And relying on this structure allows for more code that morally should compile to compile due to how this affects borrow checker behavior.

I don't see a good motivation for having such a special case for single-variant enums. People can just convert the enum to a struct if that's the behavior they want. Having fewer special cases is generally a good thing IMO. :)

Note that the current code also exempts 0-variant enums in a similar way, though I am not entirely sure what the consequences of that are.

[RalfJung]

Oh, and furthermore the logic in compiler/rustc_mir_build/src/builder/matches/match_pair.rs that this PR changes currently causes us to violate the substitution principle: in generic code, a discriminant read will be omitted, but when the types become concrete and an enum variant turns out to he uninhabited, then there is no longer a discriminant read.

[craterbot]

🎉 Experiment pr-150681 is completed!
📊 267 regressed and 300 fixed (776123 total)
📊 3565 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-150681/retry-regressed-list.txt ↩

[traviscross]

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

[craterbot]

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

[craterbot]

🚧 Experiment pr-150681-1 is now running

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

[craterbot]

🎉 Experiment pr-150681-1 is completed!
📊 54 regressed and 37 fixed (3797 total)
📊 338 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-150681-1/retry-regressed-list.txt ↩

[obi1kenobi]

There's no good reason this as if rule shouldn't apply to the defining crate too.

I believe this was likely considered, but mentioning it explicitly just in case: pattern matching currently also does not adhere to the as if rule in the defining crate.

Should we assume that pattern matching in the defining crate should also be forced to handle a wildcard case, for the sake of the as if rule? In my opinion that would be an ergonomics regression.

Of course, not doing so is also surprising — then the as if rule for non-exhaustive enums applies some times and not other times.

[traviscross]

Should we assume that pattern matching in the defining crate should also be forced to handle a wildcard case, for the sake of the as if rule?

If not doing so were causing us other problems, then yes. As long as it's a pure ergonomics win with no drawbacks, leaving it alone seems better.

[meithecatte]

Note that the current code also exempts 0-variant enums in a similar way, though I am not entirely sure what the consequences of that are.

It seems to me that the behavior here is specifically for "what happens if we match an enum against one of its variants", and therefore it simply cannot affect 0-variant enums.

The second one, for me, is less of a clear call. If an enum is exhaustive, then its structure has been committed to (even if not exported, this amounts to an internal commitment, for the moment, within the crate). There's a reasonableness to the programmer and the compiler relying on the details of this structure. And relying on this structure allows for more code that morally should compile to compile due to how this affects borrow checker behavior.

This discussion has made me realize that simply referring to long, preexisting threads to justify the change was probably a bit sloppy on my part; I have significantly expanded the frontmatter to properly argue for all three aspects of this change. See in particular "Example 1", the motivating example for the plain "just a single variant" case.

[RalfJung]

Among the crates.io results of that crater report, there seem to be 2 true positives:

• https://crater-reports.s3.amazonaws.com/pr-150681-1/try%23db823df02fd0c2cf67b43025ac3fef3f2d743245/reg/lrlex-0.14.0/log.txt
• https://crater-reports.s3.amazonaws.com/pr-150681-1/try%23db823df02fd0c2cf67b43025ac3fef3f2d743245/reg/xdg-termfilechooser-bridge-0.2.6/log.txt

[JonathanBrouwer]

(I'll unsubcribe from this PR, when this is ready for T-compiler review, feel free to ping or reroll)

[meithecatte]

Yup, as well as some github-only projects. All of the failures are for matching on a single-variant enum without any fields, that typically seems to not implement Copy simply because the need hasn't arisen yet.

One particularly interesting case is one where the match is being done on a value that has already been moved. Minimized:

enum NonCopy {
    Meow,
}

fn f(x: NonCopy) {
    drop(x);
    match x {
        NonCopy::Meow => (),
    }
}

[traviscross]

Thanks for the elaboration in the PR description. That's helpful.

3. Moreover, we currently make a more specific check: we only read the discriminant if there is more than one inhabited variant in the enum

This conflicts with what we documented in the Reference for #138961 in rust-lang/reference#1837. In type.closure.capture.precision.discriminants.uninhabited-variants, we say:

r[type.closure.capture.precision.discriminants.uninhabited-variants]
Even if all variants but the one being matched against are uninhabited, making the pattern [irrefutable][patterns.refutable], the discriminant is still read if it otherwise would be.

As you point out, this is treated as true for closure capture analysis, but as documented, we said something stronger than that.

For my part, I think we should indeed make that true.

I'd still like to separate out your item number 1 (one-variant rule) from numbers 2 (local non-exhaustive exception) and 3 (one-inhabited-variant partial exception). Basically, I think it's really clear that we should do 2 and 3, and I want to move those forward. I'd propose FCP merge immediately on this PR restricted to 2 and 3.

For item number 1, I'd prefer to see that as a separate proposal with a self-standing motivation after 2 and 3 are fixed. Otherwise, it's too easy for the closer-call item to skate by on the compelling motivation of the easier calls. In my own head, evaluating this issue, I'm feeling the pull of that bleed-over myself. Maybe in the end it will turn out to be the right thing to do, but I see stronger reasons to be skeptical of that one, and so I'd prefer to consider it as a second step.

As I understand the crater results, separating it in this way would also have the virtue of avoiding, for the moment, the observed breakage.

[RalfJung]

It seems to me that the behavior here is specifically for "what happens if we match an enum against one of its variants", and therefore it simply cannot affect 0-variant enums.

Hm... we should however still emit some sort of code for the implicit ! pattern when matching on 0-variant enums. But that's a separate discussion.

One particularly interesting case is one where the match is being done on a value that has already been moved. Minimized:

Wow, a fun example. Seems unlikely we ever actually intended that to be accepted.^^

So I feel fairly strongly that we want to do step 1 as well. But it doesn't all have to be in the same PR of course.