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.

[meithecatte]

The way #138961 comes up as a breaking change in practice is when it causes closure captures to be more granular, and as a result some values get borrowed when previously they would get moved together with the enum they were contained in. The change this forces to the user's code is typically very simple, see e.g. https://github.com/tryandromeda/andromeda/pull/43/changes

With the initial, full version of the current PR, we would again be capturing the entire enum in these cases, thus avoiding any similar instances of that breakage that happened to not get caught by crater.

However, the cut down version of this PR, which only affects enums containing uninhabited variants or marked non-exhaustive, this effect is less likely to occur (I can imagine the non-exhaustive case happening in real code, but it essentially limits it to when the code pattern in question happens to be matching on an enum that's part of the public API of the crate being compiled).

Either way, I think this is not worth backporting, because user code the previous PR broke should be really easy to fix, while this PR also has practical breakage of its own.

[workingjubilee]

Then I am withdrawing the beta nomination as the inclination of T-compiler members reviewing the backport decision in #t-compiler/backports > #150681: beta-nominated was also to decline.

[traviscross]

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?

I've posted a Reference PR for this change in:

• Remove exception WRT same-crate non_exhaustive reads reference#2162

(It still needs to be approved before we merge this PR; @meithecatte, if you would, have a look and confirm that fully covers what we're doing here.)

[rust-rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

[traviscross]

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

@JonathanBrouwer: The FCP is complete. The Reference PR is ready. This is good to go as far as lang is concerned.

[JonathanBrouwer]

I reviewed the code change and it looks like a relatively simple code change with good tests :)
I am not familiar with the code, and would usually request another reviewer to verify, but looks like @Nadrieril already did so I'm happy :)

@bors r=JonathanBrouwer,Nadrieril rollup=never

[rust-bors]

📌 Commit af302a6 has been approved by JonathanBrouwer,Nadrieril

It is now in the queue for this repository.

[rust-bors]

☀️ Test successful - CI
Approved by: JonathanBrouwer,Nadrieril
Duration: 3h 36m 16s
Pushing f846389 to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 5d04477 (parent) -> f846389 (this PR)

Test differences

Show 15 test diffs

Stage 1

• [ui] tests/ui/closures/2229_closure_analysis/match/partial-move-drop-order.rs: [missing] -> pass (J1)
• [ui] tests/ui/closures/2229_closure_analysis/match/partial-move.rs: [missing] -> pass (J1)
• [ui] tests/ui/match/borrowck-uninhabited.rs: [missing] -> pass (J1)
• [ui] tests/ui/match/uninhabited-granular-moves.rs: [missing] -> pass (J1)

Stage 2

• [ui] tests/ui/closures/2229_closure_analysis/match/partial-move-drop-order.rs: [missing] -> pass (J0)
• [ui] tests/ui/closures/2229_closure_analysis/match/partial-move.rs: [missing] -> pass (J0)
• [ui] tests/ui/match/borrowck-uninhabited.rs: [missing] -> pass (J0)
• [ui] tests/ui/match/uninhabited-granular-moves.rs: [missing] -> pass (J0)

Additionally, 7 doctest diffs were found. These are ignored, as they are noisy.

Job group index

• J0: aarch64-apple, aarch64-gnu, aarch64-gnu-llvm-20-1, aarch64-msvc-1, arm-android, armhf-gnu, dist-i586-gnu-i586-i686-musl, i686-gnu-1, i686-gnu-nopt-1, i686-msvc-1, test-various, x86_64-gnu, x86_64-gnu-debug, x86_64-gnu-gcc, x86_64-gnu-llvm-20, x86_64-gnu-llvm-20-2, x86_64-gnu-llvm-21-2, x86_64-gnu-nopt, x86_64-gnu-stable, x86_64-mingw-1, x86_64-msvc-1
• J1: x86_64-gnu-llvm-20-3, x86_64-gnu-llvm-21-3

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard f8463896a9b36a04899c013bd8825a7fd29dd7a4 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. dist-x86_64-apple: 1h 58m -> 2h 24m (+22.0%)
2. aarch64-apple: 2h 55m -> 3h 28m (+18.8%)
3. pr-check-2: 42m 59s -> 34m 56s (-18.7%)
4. x86_64-gnu-stable: 2h 24m -> 2h 1m (-15.5%)
5. pr-check-1: 32m 59s -> 27m 53s (-15.5%)
6. i686-gnu-2: 1h 41m -> 1h 27m (-13.7%)
7. i686-gnu-1: 2h 18m -> 2h 1m (-12.1%)
8. aarch64-msvc-1: 1h 58m -> 2h 10m (+10.4%)
9. x86_64-gnu: 2h 6m -> 2h 19m (+10.0%)
10. x86_64-gnu-llvm-20: 1h 22m -> 1h 14m (-9.6%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (f846389): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

• If the regression was expected or you think it can be justified,
  please write a comment with sufficient written justification, and add
  @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
• If you think that you know of a way to resolve the regression, try to create
  a new PR with a fix for the regression.
• If you do not understand the regression or you think that it is just noise,
  you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
  were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.1%, 1.1%]	7
Regressions ❌ (secondary)	0.2%	[0.1%, 0.2%]	12
Improvements ✅ (primary)	-2.6%	[-2.6%, -2.6%]	1
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	0.2%	[-2.6%, 1.1%]	8

Max RSS (memory usage)

Results (primary 0.3%, secondary -1.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.6%	[0.5%, 7.8%]	5
Regressions ❌ (secondary)	1.7%	[1.5%, 1.8%]	2
Improvements ✅ (primary)	-2.4%	[-3.5%, -1.8%]	6
Improvements ✅ (secondary)	-1.9%	[-2.9%, -0.7%]	7
All ❌✅ (primary)	0.3%	[-3.5%, 7.8%]	11

Cycles

Results (primary -2.3%, secondary -0.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.2%	[2.7%, 3.7%]	3
Improvements ✅ (primary)	-2.3%	[-2.6%, -2.0%]	2
Improvements ✅ (secondary)	-4.4%	[-7.4%, -2.0%]	3
All ❌✅ (primary)	-2.3%	[-2.6%, -2.0%]	2

Binary size

Results (primary 0.4%, secondary 0.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.9%	[0.0%, 2.9%]	7
Regressions ❌ (secondary)	0.5%	[0.1%, 0.9%]	2
Improvements ✅ (primary)	-0.3%	[-0.5%, -0.1%]	6
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	3
All ❌✅ (primary)	0.4%	[-0.5%, 2.9%]	13

Bootstrap: 480.975s -> 480.91s (-0.01%)
Artifact size: 397.98 MiB -> 397.91 MiB (-0.02%)