Stabilize min_exhaustive_patterns

[Nadrieril]

Stabilisation report

I propose we stabilize the min_exhaustive_patterns language feature.

With this feature, patterns of empty types are considered unreachable when matched by-value. This allows:

enum Void {}
fn foo() -> Result<u32, Void>;

fn main() {
  let Ok(x) = foo();
  // also
  match foo() {
    Ok(x) => ...,
  }
}

This is a subset of the long-unstable exhaustive_patterns feature. That feature is blocked because omitting empty patterns is tricky when not matched by-value. This PR stabilizes the by-value case, which is not tricky.

The not-by-value cases (behind references, pointers, and unions) stay as they are today, e.g.

enum Void {}
fn foo() -> Result<u32, &Void>;

fn main() {
  let Ok(x) = foo(); // ERROR: missing `Err(_)`
}

The consequence on existing code is some extra "unreachable pattern" warnings. This is fully backwards-compatible.

Comparison with today's rust

This proposal only affects match checking of empty types (i.e. types with no valid values). Non-empty types behave the same with or without this feature. Note that everything below is phrased in terms of match but applies equallly to if let and other pattern-matching expressions.

To be precise, a visibly empty type is:

• an enum with no variants;
• the never type !;
• a struct with a visible field of a visibly empty type (and no #[non_exhaustive] annotation);
• a tuple where one of the types is visibly empty;
• en enum with all variants visibly empty (and no #[non_exhaustive] annotation);
• a [T; N] with N != 0 and T visibly empty;
• all other types are nonempty.

(An extra change was proposed below: that we ignore #[non_exhaustive] for structs since adding fields cannot turn an empty struct into a non-empty one)

For normal types, exhaustiveness checking requires that we list all variants (or use a wildcard). For empty types it's more subtle: in some cases we require a _ pattern even though there are no valid values that can match it. This is where the difference lies regarding this feature.

Today's rust

Under today's rust, a _ is required for all empty types, except specifically: if the matched expression is of type ! (the never type) or EmptyEnum (where EmptyEnum is an enum with no variants), then the _ is not required.

let foo: Result<u32, !> = ...;
match foo {
    Ok(x) => ...,
    Err(_) => ..., // required
}
let foo: Result<u32, &!> = ...;
match foo {
    Ok(x) => ...,
    Err(_) => ..., // required
}
let foo: &! = ...;
match foo {
    _ => ..., // required
}
fn blah(foo: (u32, !)) {
    match foo {
        _ => ..., // required
    }
}
unsafe {
    let ptr: *const ! = ...;
    match *ptr {} // allowed
    let ptr: *const (u32, !) = ...;
    match *ptr {
        (x, _) => { ... } // required
    }
    let ptr: *const Result<u32, !> = ...;
    match *ptr {
        Ok(x) => { ... }
        Err(_) => { ... } // required
    }
}

After this PR

After this PR, a pattern of an empty type can be omitted if (and only if):

• the match scrutinee expression has type ! or EmptyEnum (like before);
• or the empty type is matched by value (that's the new behavior).

In all other cases, a _ is required to match on an empty type.

let foo: Result<u32, !> = ...;
match foo {
    Ok(x) => ..., // `Err` not required
}
let foo: Result<u32, &!> = ...;
match foo {
    Ok(x) => ...,
    Err(_) => ..., // required because `!` is under a dereference
}
let foo: &! = ...;
match foo {
    _ => ..., // required because `!` is under a dereference
}
fn blah(foo: (u32, !)) {
    match foo {} // allowed
}
unsafe {
    let ptr: *const ! = ...;
    match *ptr {} // allowed
    let ptr: *const (u32, !) = ...;
    match *ptr {
        (x, _) => { ... } // required because the matched place is under a (pointer) dereference
    }
    let ptr: *const Result<u32, !> = ...;
    match *ptr {
        Ok(x) => { ... }
        Err(_) => { ... } // required because the matched place is under a (pointer) dereference
    }
}

Documentation

The reference does not say anything specific about exhaustiveness checking, hence there is nothing to update there. The nomicon does, I opened rust-lang/nomicon#445 to reflect the changes.

Tests

The relevant tests are in tests/ui/pattern/usefulness/empty-types.rs.

Unresolved Questions

None that I know of.

try-job: dist-aarch64-apple

[rustbot]

r? @cjgillot

rustbot has assigned @cjgillot.
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

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

The Miri subtree was changed

cc @rust-lang/miri

[Nadrieril]

(nomicon fix is ready: rust-lang/nomicon#445)

[rust-timer]

Finished benchmarking commit (8291d68): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.3%, 1.5%]	13
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[0.3%, 1.5%]	13

Max RSS (memory usage)

Results (primary 0.5%, secondary -2.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.4%	[0.5%, 2.4%]	8
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.0%	[-3.1%, -1.4%]	3
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	0.5%	[-3.1%, 2.4%]	11

Cycles

Results (primary 1.5%, secondary 2.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.5%	[1.3%, 1.6%]	3
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.5%	[1.3%, 1.6%]	3

Binary size

Results (primary -0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.1%, -0.1%]	1

Bootstrap: 760.23s -> 757.146s (-0.41%)
Artifact size: 339.28 MiB -> 339.27 MiB (-0.01%)

[Nadrieril]

Quick thought, clippy nowadays takes rust-version into account for many lints. Does the compiler do the same, in particular for the lint for unreachable match arms? Would this be an easy addition, if it's not a thing yet?

@jplatte apologies, it seems I entirely overlooked your comment at the time. The compiler doesn't do the same, I doubt it is even aware of rust-version. I imagine you'd want this to prevent churn on unsuspecting users? The mechanism for that is editions, I don't think the churn here is sufficient to warrant that. I'll make the lint rustfixable to make this easier too.

[jplatte]

I imagine you'd want this to prevent churn on unsuspecting users?

Not so much, actually. I think churn is much more likely to happen for libraries that don't declare rust-version at all, whose authors don't really think about the concept.

It's more that I think people who do declare a rust-version and want to keep it low-ish are going to find this lint annoying, because they wouldn't want to bump MSRV to satisfy it, so all those people will have to explicitly silence it (which is not the case for similar clippy lints).

The mechanism for that is editions, I don't think the churn here is sufficient to warrant that.

What would be the harm in making it allow-by-default in current editions though? It's not that long until the next edition anyways. I think this lint will cause churn for some library writers (in the form of CI breaking, because warnings are set to deny), and I don't think the benefits of slightly shorter code is actually worth it (even though I'm actually pretty excited about this feature shipping!).

[Nadrieril]

It's more that I think people who do declare a rust-version and want to keep it low-ish are going to find this lint annoying, because they wouldn't want to bump MSRV to satisfy it, so all those people will have to explicitly silence it (which is not the case for similar clippy lints).

Oh yeah, I didn't think of that.

What would be the harm in making it allow-by-default in current editions though?

None indeed. The feature is available to users who want it even without the warning. I'm not sure how to call the shots here though.

[seanmonstar]

Just to chime in here, I noticed in hyper's CI that this started failing (yes, I deny warnings in CI, it's the best way I notice changes). What's interesting is that the warning seems to be emitted in the same version that it even became possible to omit the pattern in the first place. So, in order to support older rust version (even just current stable, 1.80), I can't appease the warning. I can only turn it off.

Would it make sense to promote the lint to a warning a few releases later?

For the record, I have code that's matching on a Result<T, Infallible>, which an Err(never) => match never {} arm.

[kpreid]

I filed issue #129031 to give this matter more visibility.

[rylev]

@Nadrieril @fee1-dead seems this has a negative impact on the performance of coherence checking which I imagined is expected, right? I'm not sure if there's anything we would want to do given that there's simply more work happening. Thoughts?

[Nadrieril]

@rylev yeah, at the very least it's calculating inhabitedness of essentially all types involed in patterns, which has unavoidable cost.