Add the `min_exhaustive_patterns` feature gate #118803

Nadrieril · 2023-12-10T18:26:29Z

Motivation

Pattern-matching on empty types is tricky around unsafe code. For that reason, current stable rust conservatively requires arms for empty types in all but the simplest case. It has long been the intention to allow omitting empty arms when it's safe to do so. The exhaustive_patterns feature allows the omission of all empty arms, but hasn't been stabilized because that was deemed dangerous around unsafe code.

Proposal

This feature aims to stabilize an uncontroversial subset of exhaustive_patterns. Namely: when min_exhaustive_patterns is enabled and the data we're matching on is guaranteed to be valid by rust's operational semantics, then we allow empty arms to be omitted. E.g.:

let x: Result<T, !> = foo();
match x { // ok
    Ok(y) => ...,
}
let Ok(y) = x; // ok

If the place is not guaranteed to hold valid data (namely ptr dereferences, ref dereferences (conservatively) and union field accesses), then we keep stable behavior i.e. we (usually) require arms for the empty cases.

unsafe {
    let ptr: *const Result<u32, !> = ...;
    match *ptr {
        Ok(x) => { ... }
        Err(_) => { ... } // still required
    }
}
let foo: Result<u32, &!> = ...;
match foo {
    Ok(x) => { ... }
    Err(&_) => { ... } // still required because of the dereference
}
unsafe {
    let ptr: *const ! = ...;
    match *ptr {} // already allowed on stable
}

Note that we conservatively consider that a valid reference can point to invalid data, hence we don't allow arms of type &! and similar cases to be omitted. This could eventually change depending on opsem decisions. Whenever opsem is undecided on a case, we conservatively keep today's stable behavior.

I proposed this behavior in the never_patterns feature gate but it makes sense on its own and could be stabilized more quickly. The two proposals nicely complement each other.

Unresolved Questions

Part of the question is whether this requires an RFC. I'd argue this doesn't need one since there is no design question beyond the intent to omit unreachable patterns, but I'm aware the problem can be framed in ways that require design (I'm thinking of the original never patterns proposal, which would frame this behavior as "auto-nevering" happening).

EDIT: I initially proposed a future-compatibility lint as part of this feature, I don't anymore.

rustbot · 2023-12-10T18:26:38Z

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

Nadrieril · 2023-12-10T18:57:23Z

(I'll create the tracking issue if this is deemed a good idea)

Nilstrieb · 2023-12-10T22:01:08Z

I haven't looked at the code, one thing that's unclear from reading the description (which is very good :3): you say that references may not point to valid data, and that matching on a place that may not be valid will require an arm in the future. Does this mean that enum Void {} fn a(v: &Void) { match *v {} } will lint and stop compiling in the future? That sounds very bad if it's the case, I've seen that pattern (with the reference, not a raw pointer) all over the place.

Nadrieril · 2023-12-11T06:45:30Z

Does this mean that enum Void {} fn a(v: &Void) { match *v {} } will lint and stop compiling in the future?

That is indeed my intention. But not if that breaks a lot of code. I'll do a crater run to see.

FWIW, the lint currently suggests match {*v} {} instead, which makes the read explicit, and once we have never patterns it will suggest match *v { ! }. If there's a lot of breakage, I'm hoping the nice machine-applicable suggestion means we can change that over an edition. But y'know it's fine if it has to stay as a warning forever.

from reading the description (which is very good :3)

🥰

bors · 2023-12-11T15:29:07Z

☔ The latest upstream changes (presumably #118823) made this pull request unmergeable. Please resolve the merge conflicts.

compiler/rustc_lint_defs/src/builtin.rs

RalfJung · 2023-12-12T07:30:37Z

There's one case where stable rust behavior is incompatible with this proposal: when the scrutinee type is either an empty enum or the never type, then stable rust allows an empty match ... {} always. Under this proposal, this should not be allowed if the data is not guaranteed valid. Therefore this PR adds a warn-by-default lint that detects this case and suggests an alternative. The intention is to turn this into a hard error at some later point if it doesn't break too much code.

This will start warning immediately on stable, right?

In that case we should probably get t-lang approval for this.

RalfJung · 2023-12-12T07:32:13Z

the lint currently suggests match {*v} {} instead, which makes the read explicit,

Hm... I'm not sure I am a fan of this. I think many people will be confused by why that even makes a difference. The fact that blocks force a place-to-val coercion is not super widely known I think.

I think the better proposal would be let val = *ptr; match val {}. Not sure if that's possible with our suggestion system though. Or maybe match ptr.read() {}?

RalfJung · 2023-12-12T07:44:31Z

you say that references may not point to valid data, and that matching on a place that may not be valid will require an arm in the future. Does this mean that enum Void {} fn a(v: &Void) { match *v {} } will lint and stop compiling in the future? That sounds very bad if it's the case, I've seen that pattern (with the reference, not a raw pointer) all over the place.

I've started a t-opsem Zulip discussion about this: https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/Matching.20on.20references.20to.20uninhabted.20types

Nadrieril · 2023-12-12T08:02:07Z

This will start warning immediately on stable, right?

No, the lint is gated under min_exhaustive_patterns for now.

I think many people will be confused by why that even makes a difference

Hm, fair. I eventually want to suggest match *x { ! } anyway, I could just move the lint to never_patterns.

RalfJung · 2023-12-12T08:56:04Z

No, the lint is gated under min_exhaustive_patterns for now.

Doesn't that just affect whether you can allow the lint? Or does it also effect whether the lint is even enabled? I don't know how feature-gated lints work.

Nadrieril · 2023-12-12T09:14:49Z

Doesn't that just affect whether you can allow the lint? Or does it also effect whether the lint is even enabled? I don't know how feature-gated lints work.

Ah yeah it does, but what I meant is that I also only emit it when the feature gate is on. This PR doesn't change anything when the feature gate is off.

RalfJung · 2023-12-12T09:22:39Z

Ah I see, sounds good. No need to involve t-lang then.

But it'd still be good to see what t-opsem thinks. My current sense (also based on what @Nilstrieb said above about this being a common pattern) is it may be better to say we assume that all the values in the match expression satisfy their safety invariant. That's usually what we do in Rust, since most Rust is safe code. That would mean only raw pointer derefs and unions fields should stop working with empty matches, which I also assume will cause way fewer regressions.

Nadrieril · 2023-12-12T09:49:53Z

Ok! If that's deemed a reasonable assumption then I'd like that too.

Nadrieril · 2023-12-20T23:29:29Z

Alright, I have removed the lint as this will require more decisions. The intent of this feature is to stabilize an uncontroversial subset of exhaustive_patterns so the lint can be done separately.

I'll clean this up and open a tracking issue so we can merge this and decide what to do about &! later.

Nadrieril · 2024-01-20T15:37:54Z

Alright, this is ready again!

This PR adds a feature gate as per the T-lang experimental feature process. The tracking issue is #119612. @scottmcm is my T-lang liaison for this (as part of the never patterns work).

I removed the lint that sparked discussion. Now the feature gate is only about allowing more match arms to be omitted.

@rustbot ready

compiler-errors · 2024-01-25T21:58:03Z

compiler/rustc_pattern_analysis/src/usefulness.rs

-    let empty_arms_are_unreachable = place_validity.is_known_valid() && can_omit_empty_arms;
+    // Whether empty patterns are counted as useful or not. We only warn an empty arm unreachable if
+    // it is guaranteed unreachable by the opsem (i.e. if the place is `known_valid`).
+    let empty_arms_are_unreachable = place_validity.is_known_valid()


it was hard to understand what was going on here until i noticed that:

these bools flipped order

the old can_omit_empty_arms was "inlined" into empty_arms_are_unreachable

the new can_omit_empty_arms is not enabled for the new min feature gate.

I tried so hard to make it legible :') The new can_omit_empty_arms is enabled with the feature gate, but only exactly when empty_arms_are_unreachable is true, i.e. the place must be known_valid.

I tried to present it as "the base case is can_omit_empty_arms == empty_arms_are_unreachable, on top of which there are two exceptions" but I guess that wasn't clear

Ah, right -- empty_arms_are_unreachable || in the new can_omit_empty_arms.

compiler-errors · 2024-01-25T21:58:19Z

@bors r+

bors · 2024-01-25T21:58:22Z

📌 Commit 95a14d4 has been approved by compiler-errors

It is now in the queue for this repository.

… r=compiler-errors Add the `min_exhaustive_patterns` feature gate ## Motivation Pattern-matching on empty types is tricky around unsafe code. For that reason, current stable rust conservatively requires arms for empty types in all but the simplest case. It has long been the intention to allow omitting empty arms when it's safe to do so. The [`exhaustive_patterns`](rust-lang#51085) feature allows the omission of all empty arms, but hasn't been stabilized because that was deemed dangerous around unsafe code. ## Proposal This feature aims to stabilize an uncontroversial subset of exhaustive_patterns. Namely: when `min_exhaustive_patterns` is enabled and the data we're matching on is guaranteed to be valid by rust's operational semantics, then we allow empty arms to be omitted. E.g.: ```rust let x: Result<T, !> = foo(); match x { // ok Ok(y) => ..., } let Ok(y) = x; // ok ``` If the place is not guaranteed to hold valid data (namely ptr dereferences, ref dereferences (conservatively) and union field accesses), then we keep stable behavior i.e. we (usually) require arms for the empty cases. ```rust unsafe { let ptr: *const Result<u32, !> = ...; match *ptr { Ok(x) => { ... } Err(_) => { ... } // still required } } let foo: Result<u32, &!> = ...; match foo { Ok(x) => { ... } Err(&_) => { ... } // still required because of the dereference } unsafe { let ptr: *const ! = ...; match *ptr {} // already allowed on stable } ``` Note that we conservatively consider that a valid reference can point to invalid data, hence we don't allow arms of type `&!` and similar cases to be omitted. This could eventually change depending on [opsem decisions](rust-lang/unsafe-code-guidelines#413). Whenever opsem is undecided on a case, we conservatively keep today's stable behavior. I proposed this behavior in the [`never_patterns`](rust-lang#118155) feature gate but it makes sense on its own and could be stabilized more quickly. The two proposals nicely complement each other. ## Unresolved Questions Part of the question is whether this requires an RFC. I'd argue this doesn't need one since there is no design question beyond the intent to omit unreachable patterns, but I'm aware the problem can be framed in ways that require design (I'm thinking of the [original never patterns proposal](https://smallcultfollowing.com/babysteps/blog/2018/08/13/never-patterns-exhaustive-matching-and-uninhabited-types-oh-my/), which would frame this behavior as "auto-nevering" happening). EDIT: I initially proposed a future-compatibility lint as part of this feature, I don't anymore.

…mpiler-errors Rollup of 8 pull requests Successful merges: - rust-lang#118803 (Add the `min_exhaustive_patterns` feature gate) - rust-lang#119286 (show linker output even if the linker succeeds) - rust-lang#119616 (Add a new `wasm32-wasi-preview2` target) - rust-lang#120124 (Split assembly tests for ELF and MachO) - rust-lang#120201 (Bump some deps with syn 1.0 dependencies) - rust-lang#120204 (Builtin macros effectively have implicit #[collapse_debuginfo(yes)]) - rust-lang#120322 (Don't manually resolve async closures in `rustc_resolve`) - rust-lang#120356 (Fix broken markdown in csky-unknown-linux-gnuabiv2.md) r? `@ghost` `@rustbot` modify labels: rollup

… r=compiler-errors Add the `min_exhaustive_patterns` feature gate ## Motivation Pattern-matching on empty types is tricky around unsafe code. For that reason, current stable rust conservatively requires arms for empty types in all but the simplest case. It has long been the intention to allow omitting empty arms when it's safe to do so. The [`exhaustive_patterns`](rust-lang#51085) feature allows the omission of all empty arms, but hasn't been stabilized because that was deemed dangerous around unsafe code. ## Proposal This feature aims to stabilize an uncontroversial subset of exhaustive_patterns. Namely: when `min_exhaustive_patterns` is enabled and the data we're matching on is guaranteed to be valid by rust's operational semantics, then we allow empty arms to be omitted. E.g.: ```rust let x: Result<T, !> = foo(); match x { // ok Ok(y) => ..., } let Ok(y) = x; // ok ``` If the place is not guaranteed to hold valid data (namely ptr dereferences, ref dereferences (conservatively) and union field accesses), then we keep stable behavior i.e. we (usually) require arms for the empty cases. ```rust unsafe { let ptr: *const Result<u32, !> = ...; match *ptr { Ok(x) => { ... } Err(_) => { ... } // still required } } let foo: Result<u32, &!> = ...; match foo { Ok(x) => { ... } Err(&_) => { ... } // still required because of the dereference } unsafe { let ptr: *const ! = ...; match *ptr {} // already allowed on stable } ``` Note that we conservatively consider that a valid reference can point to invalid data, hence we don't allow arms of type `&!` and similar cases to be omitted. This could eventually change depending on [opsem decisions](rust-lang/unsafe-code-guidelines#413). Whenever opsem is undecided on a case, we conservatively keep today's stable behavior. I proposed this behavior in the [`never_patterns`](rust-lang#118155) feature gate but it makes sense on its own and could be stabilized more quickly. The two proposals nicely complement each other. ## Unresolved Questions Part of the question is whether this requires an RFC. I'd argue this doesn't need one since there is no design question beyond the intent to omit unreachable patterns, but I'm aware the problem can be framed in ways that require design (I'm thinking of the [original never patterns proposal](https://smallcultfollowing.com/babysteps/blog/2018/08/13/never-patterns-exhaustive-matching-and-uninhabited-types-oh-my/), which would frame this behavior as "auto-nevering" happening). EDIT: I initially proposed a future-compatibility lint as part of this feature, I don't anymore.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#107464 (Add `str::Lines::remainder`) - rust-lang#118803 (Add the `min_exhaustive_patterns` feature gate) - rust-lang#119466 (Initial implementation of `str::from_raw_parts[_mut]`) - rust-lang#120053 (Specialize `Bytes` on `StdinLock<'_>`) - rust-lang#120124 (Split assembly tests for ELF and MachO) - rust-lang#120204 (Builtin macros effectively have implicit #[collapse_debuginfo(yes)]) - rust-lang#120322 (Don't manually resolve async closures in `rustc_resolve`) - rust-lang#120356 (Fix broken markdown in csky-unknown-linux-gnuabiv2.md) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#118803 - Nadrieril:min-exhaustive-patterns, r=compiler-errors Add the `min_exhaustive_patterns` feature gate ## Motivation Pattern-matching on empty types is tricky around unsafe code. For that reason, current stable rust conservatively requires arms for empty types in all but the simplest case. It has long been the intention to allow omitting empty arms when it's safe to do so. The [`exhaustive_patterns`](rust-lang#51085) feature allows the omission of all empty arms, but hasn't been stabilized because that was deemed dangerous around unsafe code. ## Proposal This feature aims to stabilize an uncontroversial subset of exhaustive_patterns. Namely: when `min_exhaustive_patterns` is enabled and the data we're matching on is guaranteed to be valid by rust's operational semantics, then we allow empty arms to be omitted. E.g.: ```rust let x: Result<T, !> = foo(); match x { // ok Ok(y) => ..., } let Ok(y) = x; // ok ``` If the place is not guaranteed to hold valid data (namely ptr dereferences, ref dereferences (conservatively) and union field accesses), then we keep stable behavior i.e. we (usually) require arms for the empty cases. ```rust unsafe { let ptr: *const Result<u32, !> = ...; match *ptr { Ok(x) => { ... } Err(_) => { ... } // still required } } let foo: Result<u32, &!> = ...; match foo { Ok(x) => { ... } Err(&_) => { ... } // still required because of the dereference } unsafe { let ptr: *const ! = ...; match *ptr {} // already allowed on stable } ``` Note that we conservatively consider that a valid reference can point to invalid data, hence we don't allow arms of type `&!` and similar cases to be omitted. This could eventually change depending on [opsem decisions](rust-lang/unsafe-code-guidelines#413). Whenever opsem is undecided on a case, we conservatively keep today's stable behavior. I proposed this behavior in the [`never_patterns`](rust-lang#118155) feature gate but it makes sense on its own and could be stabilized more quickly. The two proposals nicely complement each other. ## Unresolved Questions Part of the question is whether this requires an RFC. I'd argue this doesn't need one since there is no design question beyond the intent to omit unreachable patterns, but I'm aware the problem can be framed in ways that require design (I'm thinking of the [original never patterns proposal](https://smallcultfollowing.com/babysteps/blog/2018/08/13/never-patterns-exhaustive-matching-and-uninhabited-types-oh-my/), which would frame this behavior as "auto-nevering" happening). EDIT: I initially proposed a future-compatibility lint as part of this feature, I don't anymore.

Nadrieril added the A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns label Dec 10, 2023

rustbot assigned cjgillot Dec 10, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 10, 2023

This comment has been minimized.

Sign in to view

Nadrieril mentioned this pull request Dec 10, 2023

Tracking Issue for Never Patterns (never_patterns) #118155

Open

5 tasks

Nadrieril force-pushed the min-exhaustive-patterns branch from de5f50b to c49123d Compare December 11, 2023 22:24

This comment has been minimized.

Sign in to view

Nadrieril mentioned this pull request Dec 11, 2023

Don't warn an empty pattern unreachable if we're not sure the data is valid #118308

Merged

RalfJung reviewed Dec 12, 2023

View reviewed changes

compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

Nadrieril force-pushed the min-exhaustive-patterns branch from c49123d to 8587f11 Compare December 20, 2023 13:33

This comment has been minimized.

Sign in to view

Nadrieril force-pushed the min-exhaustive-patterns branch from 8587f11 to 55df836 Compare December 20, 2023 13:43

This comment has been minimized.

Sign in to view

Nadrieril force-pushed the min-exhaustive-patterns branch from 55df836 to 944e869 Compare December 20, 2023 23:28

Nadrieril added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 10, 2024

This comment was marked as resolved.

Sign in to view

Nadrieril force-pushed the min-exhaustive-patterns branch from 782c768 to aace614 Compare January 18, 2024 18:31

This comment was marked as outdated.

Sign in to view

Nadrieril force-pushed the min-exhaustive-patterns branch from aace614 to a92a331 Compare January 20, 2024 14:42

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 20, 2024

Nadrieril added 2 commits January 24, 2024 23:52

Add feature gate

886108b

Implement feature gate logic

95a14d4

Nadrieril force-pushed the min-exhaustive-patterns branch from a92a331 to 95a14d4 Compare January 24, 2024 23:12

compiler-errors reviewed Jan 25, 2024

View reviewed changes

compiler-errors approved these changes Jan 25, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 25, 2024

compiler-errors mentioned this pull request Jan 26, 2024

Rollup of 8 pull requests #120359

Closed

This was referenced Jan 26, 2024

Rollup of 9 pull requests #120364

Closed

Rollup of 8 pull requests #120365

Merged

bors merged commit a37fa37 into rust-lang:master Jan 26, 2024
11 checks passed

rustbot added this to the 1.77.0 milestone Jan 26, 2024

Nadrieril deleted the min-exhaustive-patterns branch January 26, 2024 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the `min_exhaustive_patterns` feature gate #118803

Add the `min_exhaustive_patterns` feature gate #118803

Nadrieril commented Dec 10, 2023 •

edited

rustbot commented Dec 10, 2023

This comment has been minimized.

Nadrieril commented Dec 10, 2023

Nilstrieb commented Dec 10, 2023

Nadrieril commented Dec 11, 2023

bors commented Dec 11, 2023

This comment has been minimized.

RalfJung commented Dec 12, 2023

RalfJung commented Dec 12, 2023 •

edited

RalfJung commented Dec 12, 2023

Nadrieril commented Dec 12, 2023

RalfJung commented Dec 12, 2023

Nadrieril commented Dec 12, 2023

RalfJung commented Dec 12, 2023 •

edited

Nadrieril commented Dec 12, 2023

This comment was marked as outdated.

This comment has been minimized.

This comment has been minimized.

Nadrieril commented Dec 20, 2023 •

edited

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as outdated.

Nadrieril commented Jan 20, 2024 •

edited

compiler-errors Jan 25, 2024

Nadrieril Jan 25, 2024

compiler-errors Jan 25, 2024

compiler-errors commented Jan 25, 2024

bors commented Jan 25, 2024

Add the min_exhaustive_patterns feature gate #118803

Add the min_exhaustive_patterns feature gate #118803

Conversation

Nadrieril commented Dec 10, 2023 • edited

Motivation

Proposal

Unresolved Questions

rustbot commented Dec 10, 2023

This comment has been minimized.

Nadrieril commented Dec 10, 2023

Nilstrieb commented Dec 10, 2023

Nadrieril commented Dec 11, 2023

bors commented Dec 11, 2023

This comment has been minimized.

RalfJung commented Dec 12, 2023

RalfJung commented Dec 12, 2023 • edited

RalfJung commented Dec 12, 2023

Nadrieril commented Dec 12, 2023

RalfJung commented Dec 12, 2023

Nadrieril commented Dec 12, 2023

RalfJung commented Dec 12, 2023 • edited

Nadrieril commented Dec 12, 2023

This comment was marked as outdated.

This comment has been minimized.

This comment has been minimized.

Nadrieril commented Dec 20, 2023 • edited

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as outdated.

Nadrieril commented Jan 20, 2024 • edited

compiler-errors Jan 25, 2024

Choose a reason for hiding this comment

Nadrieril Jan 25, 2024

Choose a reason for hiding this comment

compiler-errors Jan 25, 2024

Choose a reason for hiding this comment

compiler-errors commented Jan 25, 2024

bors commented Jan 25, 2024

Add the `min_exhaustive_patterns` feature gate #118803

Add the `min_exhaustive_patterns` feature gate #118803

Nadrieril commented Dec 10, 2023 •

edited

RalfJung commented Dec 12, 2023 •

edited

RalfJung commented Dec 12, 2023 •

edited

Nadrieril commented Dec 20, 2023 •

edited

Nadrieril commented Jan 20, 2024 •

edited