Handle empty matches cleanly in exhaustiveness checking

[Nadrieril]

This removes the special-casing of empty matches that was done in check_match. This fixes most of #55123.
Somewhat unrelatedly, I also made _match.rs more self-contained, because I think it's cleaner.

r? @varkor
@rustbot modify labels: +A-exhaustiveness-checking

[varkor]

As far as I can tell, the only stable code this affects are those using non_exhaustive, correct? I am curious to see how many issues this does cause in existing code, because one might expect that this is an uncommon scenario (considering the correct pattern, i.e. no arms, is correctly accepted on stable already, and matching on an empty enum in the same crate seems like an unusual thing to do).

@bors try

[bors]

⌛ Trying commit b9da2b3 with merge bdf62782912af0337bb64e58890f4edbfb4e075b...

[bors]

☀️ Try build successful - checks-actions
Build commit: bdf62782912af0337bb64e58890f4edbfb4e075b (bdf62782912af0337bb64e58890f4edbfb4e075b)

[Nadrieril]

As far as I can tell, the only stable code this affects are those using non_exhaustive, correct?

It's the opposite: non_exhaustive already had the correct behavior, and this affects only code without non_exhaustive. Still, I would expect this to be very rare, the fix is trivial (remove the branch), and it's only a warning anyways.

[varkor]

Does this change any code that's not using never_type or exhaustive_patterns? Or will it only affect code that uses one of the related feature gates?

[Nadrieril]

Does this change any code that's not using never_type or exhaustive_patterns? Or will it only affect code that uses one of the related feature gates?

It does change stable code, and code without those gates. The only change this PR does is in the following case:

enum Void{}
fn foo(x: Void) {
	match x {
		_ => println!("foo")
	}
}

Before, the branch was not linted as unreachable. Now it is. The PR does nothing else. People who use unreachable_patterns would have already have the unreachable warning. The ! type behaves like Void both before and after this PR, so people using ! would see the same new unreachable warning.

[varkor]

@Nadrieril: thanks for clarifying. There are so many interacting feature gates with uninhabited types that I find it a little hard to keep track sometimes. As far as I could tell, there were no tests without feature gates whose output changed here: presumably this means we haven't yet got a test in this particular situation. Could you add a test (both same-crate and cross-crate) with this example (there may already be suitable test files that they can be added to), so we make sure we're covering these?

[Nadrieril]

As far as I could tell, there were no tests without feature gates whose output changed here: presumably this means we haven't yet got a test in this particular situation.

The match-empty test does have the never_type feature gate, but that's just because it tests some things involving the never type. All its tests that don't have an explicit ! in them behave as if there was no feature gate. So I consider this feature tested. I also just added a test for cross-crate empty enum.
Does that feel enough to you?

[varkor]

All its tests that don't have an explicit ! in them behave as if there was no feature gate.

It used to be that simply enabling the ! type would change the behaviour regarding uninhabited types, but that may no longer be the case.

I'm happy with the implementation and tests now. I just want to make the lang team aware of this change. If there are no comments soon, I'll approve.

[varkor]

cc @rust-lang/lang: this pull request changes behaviour of exhaustiveness checking to lint against _ when no values are possible because a type is uninhabited. See the comment here: #78995 (comment).

enum Void {}
fn foo(x: Void) {
	match x {
		_ => println!("foo") // used not to warn, but now triggers the `unreachable_patterns` lint
	}
}

This fixes a long-standing inconsistency in unreachable_patterns regarding uninhabited types. I expect there's no issue making this change, but wanted to draw attention to it in case anyone expects this could be problematic.

[Nadrieril]

It used to be that simply enabling the ! type would change the behaviour regarding uninhabited types, but that may no longer be the case.

Oh I see. I don't believe this is still the case. I haven't encountered any branching on this feature gate in any of the match checking code or in related code. I believe all this behavior is now gated under exhaustive_patterns.

[scottmcm]

Thanks for the ping, @varkor! It makes sense to me to lint here. And empty enums are uncommon enough that I'd be very surprised if this were a churn problem.

One thing that's probably covered somewhere but I didn't see scanning the PR so I figured I'd ask to double-check: Do we have a test that this doesn't trigger for matching on #[non_exhaustive] enum Void {} when in a different crate?

[Nadrieril]

One thing that's probably covered somewhere but I didn't see scanning the PR so I figured I'd ask to double-check: Do we have a test that this doesn't trigger for matching on #[non_exhaustive] enum Void {} when in a different crate?

Yep, that's well-tested both same-crate and cross-crate by the thorough non_exhaustive-specific tests. The reason you didn't see it in the PR is that indeed the behavior didn't change in the cross-crate case.
EDIT: found it

rust/src/test/ui/rfc-2008-non-exhaustive/enum.rs

Lines 6 to 11 in 69821cf

	fn empty(x: EmptyNonExhaustiveEnum) {
	match x {} //~ ERROR type `EmptyNonExhaustiveEnum` is non-empty
	match x {
	_ => {}, // ok
	}
	}

[Mark-Simulacrum]

@bors try @rust-timer queue to make sure this isn't hitting anything like #51085 (comment) (though I have no reason to believe it is).

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 69821cf with merge 8e571541536bd9efc598a6905a9604e6aedf9134...

[bors]

☀️ Try build successful - checks-actions
Build commit: 8e571541536bd9efc598a6905a9604e6aedf9134 (8e571541536bd9efc598a6905a9604e6aedf9134)

[rust-timer]

Queued 8e571541536bd9efc598a6905a9604e6aedf9134 with parent c6a6105, future comparison URL.

[rust-timer]

Finished benchmarking try commit (8e571541536bd9efc598a6905a9604e6aedf9134): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

[Nadrieril]

to make sure this isn't hitting anything like #51085 (comment) (though I have no reason to believe it is).

I was aware of this so I made sure to avoid calling is_uninhabited more than we previously did :).
EDIT: oops, apparently quotes trigger the build bots
REEDIT: also edits x)

[varkor]

Thanks for the fix, @Nadrieril!

@bors r+

[bors]

📌 Commit 69821cf has been approved by varkor

[bors]

⌛ Testing commit 69821cf with merge 8256379...

[bors]

☀️ Test successful - checks-actions
Approved by: varkor
Pushing 8256379 to master...

[nikomatsakis]

Nice :)