Make operational semantics of pattern matching independent of crate and module

[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. introduces the following changes to this behavior:

• matching on a #[non_exhaustive] enum will always introduce a discriminant read, regardless of whether the enum is from an external crate
• uninhabited variants now count just like normal ones, and don't get skipped in the checks

As per the discussion below, the resolution for point (1) above is that it should land as part of a separate PR, so that the subtler decision can be more carefully considered.

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 parts of #147722
Fixes rust-lang/miri#4778

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.

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

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

I've modified the PR to only do parts 2 and 3 for now. While I was at it, I made sure to first introduce the ui tests, so that the diffs for the later commits clearly show the semantics changes using these tests as examples. I decided against doing the same for the miri tests, as moving the tests between pass/ and fail/ is a little unwieldy.

(btw, sorry for the extra roundtrip on tidy, I've finally tried out Jujutsu for doing this patchstack surgery, and haven't yet figured out how to get the pre-push hook working with that)

Do we want crater again? In theory all the breaking changes found by the previous crater run should be resolved by postponing point 1, but in practice, there tend to be some differences between theory and practice sometimes...

[traviscross]

Thanks @meithecatte.

Proposal: Let's adopt item numbers 2 (removing local non-exhaustive exception) and 3 (removing one-inhabited-variant partial exception).

@rfcbot fcp merge lang,opsem

Let's think about Reference text (cc @rust-lang/lang-docs). Probably we'll change type.closure.capture.precision.discriminants.non_exhaustive in this way:

r[type.closure.capture.precision.discriminants.non_exhaustive]
If [#[non_exhaustive]][attributes.type-system.non_exhaustive] is applied to an enum defined in an external crate, the enum is treated as having multiple variants for the purpose of deciding whether a read occurs, even if it actually has only one variant.

For item 3, the Reference was already correct, I think. Anything else?

@rustbot labels +S-waiting-on-documentation

We'll run crater over the list from the last crater run.

@bors try

Also CC @rust-lang/fls.

[rust-rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @CAD97
• @JakobDegen
• @RalfJung
• @digama0
• @joshtriplett
• @nikomatsakis
• @saethlin
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[CAD97]

Resolving points (2) and (3) as proposed seems like a clear improvement to me.

@rustbot reviewed

FWIW, my assumption on point (1) would have been that we elide reading the discriminant if and only if the discriminant can be fully elided from the layout (and thus reading it is a no-op w.r.t. opsem). This feels like a special case optimization(?) for single-variant enums that was overlooked when introducing repr(int), as IIUC prior to that addition, all single-variant enums didn't store a discriminant. IIRC eliding the discriminant read was first done before MIR borrowck, even, meaning the "optimization" wouldn't even have served to make matching non-capturing until MIR borrowck came along.

Certainly smells to me like an "optimization" to avoid emitting a "useless" MIR statement that turned out to be not so meaningless later on.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 36577a2 (36577a2b3cb42b8f23dec84e9203116bad6051e8, parent: 7704328ba5ae8d6ce0ac303c9d5a1a1605906766)

[traviscross]

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

[craterbot]

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

[Zalathar]

It looks like a lot of the preliminary test changes could potentially be merged on their own, separately from the main changes in this PR.

Would it make sense to extract some of those into a separate PR to land first, to make this PR smaller and more focused on the actual proposed changes?

[meithecatte]

Yeah, feels like the first four commits could probably be a separate PR. I've created #151207.